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

For AAD resource-server, create grantedAuthority by both "roles" and "claims" by default. #19412

Merged
merged 8 commits into from
Mar 5, 2021
Merged

For AAD resource-server, create grantedAuthority by both "roles" and "claims" by default. #19412

merged 8 commits into from
Mar 5, 2021

Conversation

ZhuXiaoBing-cn
Copy link
Contributor

For AAD resource-server, create grantedAuthority by both "roles" and "claims" by default.

@ghost ghost added the azure-spring All azure-spring related issues label Feb 24, 2021
backwind1233
backwind1233 reviewed Feb 24, 2021
View reviewed changes
...spring-boot/src/main/java/com/azure/spring/aad/webapi/AADJwtGrantedAuthoritiesConverter.java Outdated
Comment on lines 37 to 40
for (String claimName : WELL_KNOWN_AUTHORITIES_CLAIM_NAMES) {
if (jwt.containsClaim(claimName)) {
Object authorities = jwt.getClaim(claimName);
if (authorities instanceof String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about

if ( jwt.getClaim(claimName) instanceof String)

@ZhuXiaoBing-cn
Update the code:
50cf55d 
* Simplify the code, remove unnecessary variables.
@saragluna saragluna added the azure-spring-aad Spring active directory related issues. label Mar 3, 2021
@ZhuXiaoBing-cn
* Avoid breaking change.
e82614d 
* Update unit tests.
@ZhuXiaoBing-cn
* Modify the no-argument constructor.
ff0fd55 
* Remove setJwtGrantedAuthoritiesConverter method.
@ZhuXiaoBing-cn
Merge branch 'master' into create_grantedAuthority_by_both_roles_and_…
1c7ff30 
…claims_by_default
@ZhuXiaoBing-cn ZhuXiaoBing-cn requested a review from stliu as a code owner March 4, 2021 08:18
saragluna
saragluna requested changes Mar 4, 2021
View reviewed changes
.../src/test/java/com/azure/spring/aad/webapi/AADJwtBearerTokenAuthenticationConverterTest.java Outdated
when(jwt.containsClaim("roles")).thenReturn(true);
AADJwtBearerTokenAuthenticationConverter converter = new AADJwtBearerTokenAuthenticationConverter("roles", "ROLE_");
AADJwtBearerTokenAuthenticationConverter converter = new AADJwtBearerTokenAuthenticationConverter("roles",
Copy link
Member

Choose a reason for hiding this comment

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

We use roles + "APPROLE_" as a default, maybe we should use another combination here.

@saragluna
Copy link
Member

/azp run java - spring - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@saragluna
Copy link
Member

IT failed for port already in use error, so merging this PR now.

https://dev.azure.com/azure-sdk/internal/_build/results?buildId=767263&view=logs&j=33a2a0fe-e3d6-52cb-a715-0d6e82684ee6&t=61962a86-9832-5837-a509-be34c34373e6&l=291

@saragluna
Copy link
Member

/check-enforcer override

@saragluna saragluna merged commit 0373b50 into Azure:master Mar 5, 2021
@saragluna saragluna added this to the [2021] April milestone Mar 22, 2021
@mattstrain
Copy link

mattstrain commented Jun 7, 2021 •
edited
Loading

Hi just wondering why you chose
private static final String DEFAULT_ROLES_AUTHORITY_PREFIX = "APPROLE_";
and not
private static final String DEFAULT_ROLES_AUTHORITY_PREFIX = "ROLE_";

The affect of this is that you now MUST use @PreAuthorize("hasAuthority(‘APPROLE_ADMIN')") and NOT @PreAuthorize("hasRole('ADMIN')") as it will no longer work. This means we have to refactor a lot of our codebase :-(!

From https://www.baeldung.com/spring-security-expressions

Roles and authorities are similar in Spring.

The main difference is that, roles have special semantics – starting with Spring Security 4, the ‘ROLE_‘ prefix is automatically added (if it's not already there) by any role related method.

So hasAuthority(‘ROLE_ADMIN') is similar to hasRole(‘ADMIN') because the ‘ROLE_‘ prefix gets added automatically.

@mattstrain
Copy link

ok no bigggie so i worked out we can pass our own converter in AADResourceServerWebSecurityConfigurerAdapter like this http.oauth2ResourceServer().jwt().jwtAuthenticationConverter(tokenConverter); and then created a converter which maps to ROLE_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@backwind1233 backwind1233 backwind1233 left review comments

@saragluna saragluna saragluna approved these changes

@chenrujun chenrujun Awaiting requested review from chenrujun

@jialindai jialindai Awaiting requested review from jialindai

@yiliuTo yiliuTo Awaiting requested review from yiliuTo

@stliu stliu Awaiting requested review from stliu

Assignees

@backwind1233 backwind1233

@saragluna saragluna

Labels
azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues.
Projects
None yet
Milestone
[2021] April
Development

Successfully merging this pull request may close these issues.
4 participants
@ZhuXiaoBing-cn @saragluna @mattstrain @backwind1233