-
Notifications
You must be signed in to change notification settings - Fork 385
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
CLDR-16838 Fix login bug: removeLoginCookies if query has email and password #3388
Conversation
final String jwtId = klm.getSubject(jwt); | ||
if (jwtId != null && !jwtId.isBlank() && !klm.jwtIsInExcludedSet(jwtId)) { | ||
if (!email.isEmpty() && !password.isEmpty()) { | ||
klm.addToExcludedSet(jwtId); |
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.
What's the reason ?
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.
Otherwise, if I'm logged in as Admin, then I click a link to log in as another user, it doesn't work -- I'm still logged in as Admin, not as the other user. That's the bug that needs fixing. This PR fixes the bug by excluding the Admin JWT so it no longer prevents switching to a different user.
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.
The exclusion only happens in the special case where email and password are included in URL, as in https://cldr-staging.unicode.org/cldr-apps/survey?email=...&pw=...
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.
ok. so it's not the excluded set, it's the loggedOutSet because you are wanting to ignore this JWT from now on.
i'd suggest renaming it or at least commenting that.
Also, at this point, you might call WebContext.removeLoginCookies()
or even .logout()
so that the JWT doesn't come back.
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.
since the excluded set isn't persistent, the JWT will cause the user to be logged back in next time ST restarts. Not sure if that's desireable or not. Clearing the login cookies might actually be better in that regard. then there's no need for an exclude set then.
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.
"the JWT will cause the user to be logged back in next time ST restarts" -- that was already the case, though, right? And in the typical case where the excluded/logged-out JWT is for Admin/TC/Manager, maybe that's appropriate.
"WebContext.removeLoginCookies() or even .logout() so that the JWT doesn't come back" -- OK, I'll try that
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.
removeLoginCookies works!
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.
"the JWT will cause the user to be logged back in next time ST restarts" -- that was already the case, though, right? And in the typical case where the excluded/logged-out JWT is for Admin/TC/Manager, maybe that's appropriate.
The odd part is that the behavior will change depending on whether ST restarted.
If you do whatever you do after clicking the password link, and then close the window, and come back an hour later to st.unicode.org:
- if ST has restarted, you'll be logged in as the Admin/TC/Manager
- if ST hasn't restarted, you'll be logged in as the user you clicked the password link for
removeLoginCookies makes it consistent. If you want to login, you need to login again.
…assword -Undeprecate UserRegistry.internalGetPassword, accessed not only by JSP but also by Auth.java -Fix some compiler warnings for Auth.java
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
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.
LGTM!
-Undeprecate UserRegistry.internalGetPassword, accessed not only by JSP but also by Auth.java
-Fix some compiler warnings for Auth.java
CLDR-16838
ALLOW_MANY_COMMITS=true