-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Enhance the token authentication converter to accept the custom jwt granted authorities converter #32335
Enhance the token authentication converter to accept the custom jwt granted authorities converter #32335
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
…/support-accept-jwt-granted-authority-converter-4.x
…/support-accept-jwt-granted-authority-converter-4.x
@stliu , please help review the API changes in this PR, thanks~ |
public AadResourceServerWebSecurityConfigurerAdapter(AadResourceServerProperties properties, | ||
Converter<Jwt, Collection<GrantedAuthority>> jwtGrantedAuthorityConverter) { | ||
this.properties = properties; | ||
this.jwtGrantedAuthorityConverter = jwtGrantedAuthorityConverter; |
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're checking NotNull at here https://github.com/Azure/azure-sdk-for-java/pull/32335/files#diff-ce112d381fe58ed8f9480c52baed5571933351148ab89b75987aaf0dc8658214R94 but why not here?
converter.setJwtGrantedAuthoritiesConverter(jwtGrantedAuthorityConverter); | ||
} else { | ||
converter.setJwtGrantedAuthoritiesConverter( | ||
new AadJwtGrantedAuthoritiesConverter(properties.getClaimToAuthorityPrefixMap())); |
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 a little wired, why this new AadJwtGrantedAuthoritiesConverter(properties.getClaimToAuthorityPrefixMap())
here instead of in the constructor?
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.
It's required a Map to construct the granted authorities converter, the AadJwtGrantedAuthoritiesConverter
only accepts a constructor parameter.
/check-enforcer override |
Description
Fixes #28665
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines