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.

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

Allow for multiple Jwt to GrantedAuthorizies converters #7596 #7597

Closed
wants to merge 1 commit into from

Conversation

rolaca11
Copy link

@rolaca11 rolaca11 commented Nov 2, 2019

Resolution of gh-7596.

In this PR, I introduce a way to have multiple granted authority converters inspect the incoming Jwt token and return the union of authorities returned by those converters.

This PR also refactors the OAuth2ResourceServerConfigurer to make better use of the newly introduced way of managing aforementioned converters.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 2, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Dec 9, 2019

@rolaca11 would you be able to adjust this PR to conform to #7596 (comment)

@jzheaux jzheaux self-assigned this Dec 9, 2019
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2019
@jzheaux jzheaux added this to the 5.3.x milestone Dec 9, 2019
@rolaca11
Copy link
Author

Yes, I'll make time for it this evening.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 10, 2019
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, @rolaca11! Will you please take a look at https://github.com/spring-projects/spring-security/blob/master/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/DelegatingOAuth2TokenValidator.java and emulate that contract, for consistency?

Namely, please have a Collection constructor and varargs constructor, please remove the mutator methods, and please add some JavaDoc, including an @author tag, if you'd like.

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, @rolaca11! I've left just a bit more feedback inline.

@rolaca11
Copy link
Author

Of course, sorry for the lot of feedback iterations, I'll do my best

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.

Sorry this fell off my radar, @rolaca11. I just left a bit more feedback.

If you like, I'd be happy to make the remaining changes listed.

Or, if you still are able to complete the PR, please also squash your commits down to one commit that includes Closes gh-7596 at the end of the commit message.

@rolaca11
Copy link
Author

This is my first time squashing commits of a branch, so I might have messed up somewhere along the way. Can you please help me make sure everything is correct?

@jzheaux
Copy link
Contributor

jzheaux commented Nov 24, 2020

Thanks, @rolaca11! This is now merged into master.

I've also contributed a polish commit of b0d4e50 to bring the contribution in line with Spring Security conventions.

@jzheaux jzheaux closed this Nov 24, 2020
@NeluAkejelu
Copy link

Great! Thanks @jzheaux @rolaca11 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants