This repository was archived by the owner on May 31, 2022. It is now read-only.
Preserve user authentication details on re-authentication #823
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have signed the contributors agreement. |
|
Looks like your change will fix my problem as well. I hope it will get accepted. If it does, I'll retest my specific use case and will reject my PR if it is no longer needed. |
jgrandja
added
in: oauth2
type: enhancement
and removed
status: waiting-for-triage
labels
jgrandja
suggested changes
May 4, 2020
| * @param authentication The authentication | ||
| * @return The re-authenticated authentication. | ||
| */ | ||
| protected OAuth2Authentication reauthenticateUserAuthentication(OAuth2Authentication authentication) { |
There was a problem hiding this comment.
Please remove reauthenticateUserAuthentication() and createUserPreAuthenticatedToken() as it's not needed. The only change required is to setDetails() on the PreAuthenticatedAuthenticationToken before it's passed to AuthenticationManager.
There was a problem hiding this comment.
Thanks for the PR @klieber. If you're still looking to get this merged, please see my review comment and apply the change and we'll get this merged.
Sure I will take a look.
There was a problem hiding this comment.
@jgrandja I've made the changes requested. Let me know if there is anything else needed.
During a token refresh in the `DefaultTokenServices` the user authentication will be re-authenticated if an `AuthenticationManager` was provided. A `PreAuthenticatedAuthenticationToken` is created based on the user authentication and then passed to the `AuthenticationManager`. However, if there were any details on the user authentication those details are lost because they are not copied to the `PreAuthenticatedAuthenticationToken`. If the `AuthenticationManager` is not provided then this logic is skipped over and the details are correctly preserved. The fix is simply to set the details on the `PreAuthenticatedAuthenticationToken` before passing it to the `AuthenticationManager`. Finally, I added two new tests to `DefaultTokenServicesTests` to validate that the user authentication is built correctly on a refresh. There is one test for the scenario when there is no re-authentication which passes even before these changes and then the other tests the re-authentication scenario which requires this change to pass.
klieber
force-pushed
the
bugfix/preserve-details-on-reauth
branch
from
fd08072 to
cfd1c3d
Compare
jgrandja
pushed a commit
that referenced
this pull request
May 21, 2020
During a token refresh in the `DefaultTokenServices` the user authentication will be re-authenticated if an `AuthenticationManager` was provided. A `PreAuthenticatedAuthenticationToken` is created based on the user authentication and then passed to the `AuthenticationManager`. However, if there were any details on the user authentication those details are lost because they are not copied to the `PreAuthenticatedAuthenticationToken`. If the `AuthenticationManager` is not provided then this logic is skipped over and the details are correctly preserved. The fix is simply to set the details on the `PreAuthenticatedAuthenticationToken` before passing it to the `AuthenticationManager`. Finally, I added two new tests to `DefaultTokenServicesTests` to validate that the user authentication is built correctly on a refresh. There is one test for the scenario when there is no re-authentication which passes even before these changes and then the other tests the re-authentication scenario which requires this change to pass. Fixes gh-823
|
Thanks for the update @klieber. This is now in master and backported to 2.4.x. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During a token refresh in the
DefaultTokenServicesthe userauthentication will be re-authenticated if an
AuthenticationManagerwas provided. A
PreAuthenticatedAuthenticationTokenis createdbased on the user authentication and then passed to the
AuthenticationManager. However, if there were any details onthe user authentication those details are lost because they are
not copied to the
PreAuthenticatedAuthenticationToken. If theAuthenticationManageris not provided then this logic is skippedover and the details are correctly preserved.
The fix is simply to set the details on the
PreAuthenticatedAuthenticationTokenbefore passing it to theAuthenticationManager. I've moved the re-authentication code intoa new protected method named
reauthenticateUserAuthenticationso thatit can be overridden if needed.
Finally, I added two new tests to
DefaultTokenServicesTeststovalidate that the user authentication is built correctly on a refresh.
There is one test for the scenario when there is no re-authentication
which passes even before these changes and then the other tests the
re-authentication scenario which requires this change to pass.