-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add SpEL support for nested username extraction in OAuth2 user info #16857
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR @yybmion! I wonder if this might be better implemented using SpEL to provide more powerful options for resolving the username. What are your thoughts? |
Hi @rwinch , Thank you for your guidance on this. I initially chose the dot notation approach because it offers a simple and intuitive solution specifically for the nested user-name-attribute issue. However, I can see the value in using SpEL as you suggested. While I think it may be slightly more complex, SpEL provides much greater extensibility for future use cases beyond simple nested structures. The consistency with other parts of the Spring Security framework is also a advantage. If you confirm that SpEL is the preferred direction, I'd be happy to update the PR accordingly. |
Yes. Please provide an implementation that uses SpEL. |
Hello @rwinch, I'd like to clarify your feedback on my PR about supporting nested properties in the user-name-attribute. Did you mean that I should implement support for expressions like Thank you for your guidance! |
I think an outline would be: Allow Injecting the Principal Name into
|
This comment was marked as resolved.
This comment was marked as resolved.
c5aa6d9
to
972afda
Compare
Hi @rwinch thank you for the detailed guide. I've implemented the SpEL-based approach as outlined in the original issue. The solution provides
Please see the PR description for detailed changes. |
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.
Thank you for the updates. This is taking shape. I've provided feedback inline.
} | ||
else { | ||
try { | ||
ExpressionParser parser = new SpelExpressionParser(); |
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 ExpressionParser should be reused as it is thread safe. You can safely make this a final member variable.
else { | ||
try { | ||
ExpressionParser parser = new SpelExpressionParser(); | ||
StandardEvaluationContext context = new StandardEvaluationContext(); |
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.
Please use SimpleEvaluationContext
instead since StandardEvaluationContext
is less safe and unnecessary.
private String evaluateUsername(Map<String, Object> attributes, String usernameExpression) { | ||
Object value = null; | ||
|
||
if (attributes.containsKey(usernameExpression)) { |
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.
You should be able to remove this if statement and always use the expression if it is set as described on the builder.
return this.usernameExpression; | ||
} | ||
|
||
return this.userNameAttributeName; |
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.
Please just return the usernameExpression
without defaulting here. Instead ensure that the userNameAttributeName
setter sets the usernameExpression
.
@@ -672,7 +700,14 @@ private ProviderDetails createProviderDetails(ClientRegistration clientRegistrat | |||
providerDetails.tokenUri = this.tokenUri; | |||
providerDetails.userInfoEndpoint.uri = this.userInfoUri; | |||
providerDetails.userInfoEndpoint.authenticationMethod = this.userInfoAuthenticationMethod; | |||
if (this.usernameExpression != null) { |
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.
If you update the builder to set the expression when setting the userNameAttributeName
, then the if statement can be removed
return new DefaultOAuth2User(authorities, attributes, userNameAttributeName); | ||
|
||
String evaluatedUsername = evaluateUsername(attributes, usernameExpression); | ||
Collection<GrantedAuthority> authorities = getAuthorities(token, attributes, usernameExpression); |
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 is now passing in usernameExpression
to getAuthorities
which means that the usernameExpression
is now passed into OAuth2UserAuthority
in place of userNameAttributeName
.
I think when users use more complex expressions it will break functionality introduced gh-15012 because the OAuth2UserAuthority
only has an attribute name and not a property. We will need to figure out how to integrate cleanly with gh-15012 From the PR it sounds like we could just expose the username rather than the attribute, but I'm not certain how the integration is being used cc @filiphr
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.
Thanks for highlighting this important integration issue with gh-15012!
I've addressed this by following your suggestion to expose the username directly rather than the attribute name.
- Added
username
field to OAuth2UserAuthority withgetUsername()
method - Builder pattern:
OAuth2UserAuthority.withUsername(evaluatedUsername)
- Pass the evaluated username result (not the SpEL expression) to OAuth2UserAuthority
I think this way, OAuth2UserAuthority always receives the final evaluated username string, preserving compatibility with gh-15012 regardless of expression complexity.
*/ | ||
@Deprecated | ||
public Builder userNameAttributeName(String userNameAttributeName) { |
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 is the place where I think you should default the usernameExpression to "['" + userNameAttributeName +
"']"`.
*/ | ||
@Deprecated |
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 think that you can remove the Deprecated as this simplifies setting the expression. Just update the Javadoc.
public Builder userNameAttributeName(String userNameAttributeName) { | ||
this.userNameAttributeName = userNameAttributeName; | ||
return this; | ||
} | ||
|
||
/** | ||
* Sets the SpEL expression used to extract the username from user info response. |
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.
Please provide a few examples for the value.
* @param username the user's name | ||
* @return a new {@code DefaultOAuth2User} | ||
*/ | ||
public static DefaultOAuth2User withUsername(Collection<? extends GrantedAuthority> authorities, |
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 think mentioned a factory method previously, but I do not like this with multiple arguments. Please create a public static Builder withUsername(String)
method instead.
- Add username field to DefaultOAuth2User for direct name injection - Add Builder pattern with DefaultOAuth2User.withUsername(String) static factory method - Deprecate constructor that uses nameAttributeKey lookup in favor of Builder pattern - Update Jackson mixins to support username field serialization/deserialization This change prepares for SpEL support in the next commit. Signed-off-by: yybmion <yunyubin54@gmail.com>
0f6d3ce
to
5798e87
Compare
Thanks for the feedback @rwinch ! I've addressed all the review comments and updated the code accordingly. But the build failure appears to be related to Kotlin version. What would you recommend I do? |
- Add usernameExpression property with SpEL evaluation support - Auto-convert userNameAttributeName to SpEL for backward compatibility - Use SimpleEvaluationContext for secure expression evaluation - Pass evaluated username to OAuth2UserAuthority for spring-projectsgh-15012 compatibility - Add Builder pattern to DefaultOAuth2User - Support nested property access (e.g., "data.username") Fixes spring-projectsgh-16390 Signed-off-by: yybmion <yunyubin54@gmail.com>
This PR adds support for extracting usernames from nested properties in OAuth2 user info responses using SpEL expressions, addressing the limitation where providers wrap user data in nested objects.
Fixes #16390
Problem
OAuth2 providers (like X/Twitter) wrap user data in nested objects, requiring complex workarounds to extract usernames.
Solution
Commit 1: Allow injecting principal name into DefaultOAuth2User
Commit 2: Add SpEL support for nested username extraction
OAuth2UserAuthority
#15012 compatibilityBackward Compatibility