-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: Forward to redirect_to
param to when auth'd
#16821
Conversation
let transition; | ||
const isAuthed = this.auth.currentToken; | ||
// eslint-disable-next-line ember/no-controller-access-in-routes | ||
const controller = this.controllerFor('vault.cluster.auth'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this query param is on the top level vault
controller/route -
vault/ui/app/controllers/vault.js
Line 10 in e5b1f71
redirectTo: 'redirect_to', |
@@ -0,0 +1 @@ | |||
<LogoSplash /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the reason for having a new Route? So the UI can update straight away after it leaves the auth route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template should actually never show up since everything happens in the beforeModel
hook but I thought it prudent to have it just in case
) { | ||
return CLUSTER; | ||
} | ||
if (isAuthed && this.routeName === AUTH) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job navigating this! This "state machine" is an unfortunately dense part of the codebase. I think this change makes sense, and I think moving the redirect elsewhere (instead of in another if here) also helps keep this more understandable.
I had a question in another part about adding a redirect
route - do you plan on using that more than this one case? If not, would it make sense to move that functionality to auth route and move this clause up to where AUTH
is returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of my earlier attempts was to do it within auth
but that is also quite dense. I could imagine a world where the redirect becomes the new URL that users link to directly, and maybe we even add some functionality that checks the namespace permissions and log you out, then redirect to auth with the redirect_to
param intact so that the user can log in with credentials for the part of the app they're trying to get to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this will be a nice improvement for certain workflows! I had some small feedback but I'm +1 to the general approach - I think the only thing that's a must change before merging is the controllerFor
call to vault
instead of vault.cluster.auth
.
This adds behavior to the auth route where, if the user is already logged in and they attempt to go to the auth url with the
redirect_to
param present, they will get redirected to theredirect_to
route:Previously, attempting to go to the auth route when already logged in will always route you to the main secrets list: