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.

Sign up for GitHub

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

Jump to bottom

Support nested username attribute in DefaultOAuth2User #14265

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Support nested username attribute in DefaultOAuth2User #14265

merged 3 commits into from
Jan 30, 2024

Conversation

ahmd-nabil
Copy link
Contributor

ISSUE gh-14186

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 8, 2023
@ahmd-nabil ahmd-nabil mentioned this pull request Dec 11, 2023
Merged
jzheaux
jzheaux requested changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @ahmd-nabil! I've left some feedback inline.

.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java Outdated Show resolved Hide resolved
.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java Outdated Show resolved Hide resolved
.../java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserServiceTests.java Show resolved Hide resolved
...uth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java Outdated Show resolved Hide resolved
...uth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java Outdated Show resolved Hide resolved
...org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationTokenMixinTests.java Outdated Show resolved Hide resolved
...uth2-core/src/main/java/org/springframework/security/oauth2/core/user/DefaultOAuth2User.java Outdated Show resolved Hide resolved
.../main/java/org/springframework/security/oauth2/client/userinfo/DefaultOAuth2UserService.java Show resolved Hide resolved
@ahmd-nabil
Copy link
Contributor Author

Hi @jzheaux. sorry for rushing you. But, I 've been waiting for a review for a while, so I am just making sure if PR is still needed. whenever you have the time I would appreciate your review :)

@jzheaux
Copy link
Contributor

jzheaux commented Jan 19, 2024

Thanks for the updates, @ahmd-nabil. Some of your recent changes caused me to see that there might not be a way to remain backward compatible. For example, an application using user-name-attribute: user-name will break when upgrading.

Further, the JWT spec does not restrict characters for custom claim names, meaning that it is technically possible for user.username to be a valid claim name by itself. This makes it rather tricky to know the user's intent by reading the property value.

Given that, I think we might need to take a different route, and hopefully one that is less invasive. To that end, I've pushed a change to my fork that takes a different approach. If you like it, you are welcome to merge it to your branch.

@ahmd-nabil
Copy link
Contributor Author

thanks @jzheaux merged. and as always thanks for your guidance :)

@jzheaux
Copy link
Contributor

jzheaux commented Jan 19, 2024

Great, thanks @ahmd-nabil. Are you able to also do the same for DefaultReactiveOAuth2UserService and the associated tests?

If not, I can add that as a polish. Either way, I think we're ready to separate the existing code cleanup into a separate commit and squash the other changes into a single commit.

@jzheaux jzheaux self-assigned this Jan 19, 2024
@jzheaux jzheaux added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 19, 2024
@ahmd-nabil
Copy link
Contributor Author

TY @jzheaux, I've separated the commit into 2 of them (I had to copy paste a lot to do that, I wonder if there is a better way).
I am not that good with project reactor but I will give it a try.

@ahmd-nabil
Add support for nested username attribute in DefaultOAuth2User
9e7ee78 
Closes spring-projectsgh-14186

Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
@ahmd-nabil
Polish DefaultOAuth2UserService
c6623a1 
Signed-off-by: ahmd-nabil <ahm3dnabil99@gmail.com>
@jzheaux jzheaux mentioned this pull request Jan 30, 2024
Closed
@jzheaux jzheaux removed the status: duplicate A duplicate of another issue label Jan 30, 2024
@jzheaux jzheaux added this to the 6.3.0-M2 milestone Jan 30, 2024
@jzheaux
Polish setAttributesConverter
e3b5da6 
- Add Tests
- Add Reactive Support

Issue spring-projectsgh-14186
@jzheaux jzheaux merged commit d7599ab into spring-projects:main Jan 30, 2024
3 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Jan 30, 2024

Thanks, @ahmd-nabil! This is now merged into main.

@jzheaux jzheaux changed the title Nested username attribute in DefaultOAuth2User Support nested username attribute in DefaultOAuth2User Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux Awaiting requested review from jzheaux

Assignees

@jzheaux jzheaux

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
Spring Security Team
Status: Done
Milestone
6.3.0-M2
Development

Successfully merging this pull request may close these issues.
3 participants
@ahmd-nabil @jzheaux @spring-projects-issues