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

Grant Individual Authorities From Claims #7351

Merged
merged 2 commits into from
Sep 4, 2019

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Sep 4, 2019

Fixes gh-7339

@jzheaux jzheaux requested a review from jgrandja September 4, 2019 10:37
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks @jzheaux. I left some comments inline. In addition to those, we also need to update the Reactive counterparts: DefaultReactiveOAuth2UserService and OidcReactiveOAuth2UserService

new OidcUserAuthority(userRequest.getIdToken(), userInfo));
Set<GrantedAuthority> authorities = new LinkedHashSet<>();
authorities.add(new OidcUserAuthority(userRequest.getIdToken(), userInfo));
authorities.addAll(oauth2UserAuthorities);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right as authorities will now contain OAuth2UserAuthority and OidcUserAuthority instances - it should only contain OidcUserAuthority. Please add a test to enforce this.

Copy link
Contributor Author

@jzheaux jzheaux Sep 4, 2019

Choose a reason for hiding this comment

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

The tests already enforce this -- see that they check the size of the returned authorities as well as the types and ordering of the elements.

Because of the DefaultOAuth2User#sortAuthorities method, the duplicate is eliminated.

Is there anything additional you'd like to see to handle this?

Set<GrantedAuthority> authorities = new LinkedHashSet<>();
authorities.add(new OAuth2UserAuthority(userAttributes));
for (String authority : getAuthorities(() -> userAttributes)) {
authorities.add(new SimpleGrantedAuthority("SCOPE_" + authority));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the logic from JwtGrantedAuthoritiesConverter should be replicated here. I believe the additional default authorities can be simplified like this:

Current authority: OAuth2UserAuthority -> ROLE_USER

Additional authorities: OAuth2UserRequest.OAuth2AccessToken.scopes e.g. SCOPE_profile, SCOPE_email

** The assumption is that OAuth2UserRequest.OAuth2AccessToken.scopes contains profile and email, meaning the user authorized the client to access those scopes during the authorization approval step. NOTE: These scopes would be initially configured with the ClientRegistration.scopes.

@jzheaux jzheaux self-assigned this Sep 4, 2019
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Sep 4, 2019
@jzheaux jzheaux added this to the 5.2.0.RC1 milestone Sep 4, 2019
@jzheaux jzheaux merged commit 833bfd0 into spring-projects:master Sep 4, 2019
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: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultOAuth2UserService should extract authorities
2 participants