Skip to content
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

Logout with wrapped token #14329

Merged
merged 2 commits into from
Mar 2, 2022
Merged

Logout with wrapped token #14329

merged 2 commits into from
Mar 2, 2022

Conversation

zofskeez
Copy link
Contributor

@zofskeez zofskeez commented Mar 1, 2022

This fix addresses an issue with the wrapped_token query parameter not working with the logout route. The user was presented with the login form rather than attempting to unwrap the supplied token and login automatically.

logout-wrapped-token.mp4

@zofskeez zofskeez added the ui label Mar 1, 2022
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 1, 2022 22:05 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 1, 2022 22:05 Inactive
@Monkeychip Monkeychip added this to the 1.10 milestone Mar 2, 2022
} else {
let params = `?with=${authType}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you have a namespace with a query param for a wrapped token?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the auth-form component, if wrappedToken is provided it will automatically attempt to unseal the token and if successful it preforms the same action as if the form has been filled out and the user hits submit manually. If a namespace is present in the query param it should behave as if the user selected one in the dropdown.

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is this a good candidate to add some test coverage for this scenario? Also does this need to be backported? I added the 1.10 release milestone as it's a bug. Not sure if this means we need to backport this to 1.10 or not because it's not rc-1.

@zofskeez
Copy link
Contributor Author

zofskeez commented Mar 2, 2022

I had a look and there were already Acceptance tests covering this, wrapped-token-test.js and redirect-to-test.js. I think the reason this wasn't caught is because of the check for the testing environment and then using replaceWith instead of location.assign. In the case of using replaceWith the query params from the transition are preserved. See #12035 for how this came about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants