Closes gh-14352 #14369
Closed
Closes gh-14352 #14369
Conversation
This file contains 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
Crain-32
commented
Dec 22, 2023
| * @throws IllegalArgumentException if {@code principal.getAuthorities()} is null, or | ||
| * any Authorities in it are null. | ||
| */ | ||
| private UsernamePasswordTypedAuthenticationToken(UserDetails principal, String credentials, boolean authenticated) { |
There was a problem hiding this comment.
Another thing to note. I was very torn on if this should be a public or private constructor. In a review of how UsernamePasswordAuthenticationToken is handled, I arrived to the conclusion the maximum visibility should be protected, however with no sub-classes or other reasons to expose it, I left it as private.
spring-projects-issues
added
the
status: waiting-for-triage
|
Closing this PR for now per this comment. |
sjohnr
added
type: enhancement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The goal of #14352 is to begin introducing a more strongly typed
Authenticationobject into the Spring Security Ecosystem. This should hopefully (My fingers are crossed) help slowly reduce some overhead for developers across the board as the general flow of User -> Authentication Object can be made more clear with people being able to see they should put aUserDetailsObject in the Principal to align with Spring Security's Testing Support, along with being able to see the flow of Types through Spring Security, where now you just seeObjectpassed everywhere.For this PR I left it at adding in the interface and replicating the simple
UsernamePasswordAuthenticationTokenwith the new interface, along with creating a newAbstractUserDetailsAuthentication<?, ?>which mirrors theAbstractAuthenticationToken, but requires the Principal object to be aUserDetailsimplementation. I believe swapping theUsernamePasswordAuthenticationTokenfor theUsernamePasswordTypedAuthenticationToken<?>to be a breaking change, and will leave that level of integration to the hands of the larger Spring Team.Additionally I swapped the
AbstractAuthenticationToken.eraseSecret(String)'s instance check to be a more modern capture group, as I was already reviewing that entire class.Last thing to note is at the time of this PR #14352 is still in "waiting-for-triage", but I had a free afternoon and felt like trying this out, figured that might also help triage effort as at least for me, seeing code makes life a lot easier.