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

[BUG]the return value of customized convert should follow Spring secuity standard #20388

Closed
backwind1233 opened this issue Apr 6, 2021 · 4 comments · Fixed by #20445
Closed
Assignees
Labels
azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues. bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@backwind1233
Copy link
Contributor

Describe the bug
by default, a bearer token in request will be convert into an AbstractAuthenticationToken object through spring security framwork, and the AbstractAuthenticationToken object's getName will get subject claim in the bearer token.
but our customized AADB2CJwtBearerTokenAuthenticationConverter’s convert method does not follow the standard, which will cause some unexpected problem.

Exception or Stack Trace

To Reproduce

Code Snippet

 @Override
    public AbstractAuthenticationToken convert(Jwt jwt) {
        OAuth2AccessToken accessToken = new OAuth2AccessToken(
            OAuth2AccessToken.TokenType.BEARER, jwt.getTokenValue(), jwt.getIssuedAt(), jwt.getExpiresAt());
        Collection<GrantedAuthority> authorities = extractAuthorities(jwt);
        AADB2COAuth2AuthenticatedPrincipal principal = new AADB2COAuth2AuthenticatedPrincipal(
            jwt.getHeaders(), jwt.getClaims(), authorities, jwt.getTokenValue());
        return new BearerTokenAuthentication(principal, accessToken, authorities);
    }

Expected behavior

Screenshots

Setup (please complete the following information):

Additional context

Information Checklist

@backwind1233 backwind1233 added the azure-spring All azure-spring related issues label Apr 6, 2021
@backwind1233 backwind1233 self-assigned this Apr 6, 2021
@ZhuXiaoBing-cn
Copy link
Contributor

@backwind1233, You are right. AbstractAuthenticationToken object's getName will get a subject claim in the bearer token for the spring security framework.
But in our AADB2CJwtBearerTokenAuthenticationConverter, Jwt is converted into an implementation class BearerTokenAuthentication of AbstractAuthenticationToken, which has more flexible scalability.
However, an OAuth2AuthenticatedPrincipal object needs to be passed when constructing BearerTokenAuthentication, which will implement the getName method. I think what we need to change is the getName method in OAuth2AuthenticatedPrincipal.

@saragluna saragluna added the azure-spring-aad Spring active directory related issues. label Apr 7, 2021
@backwind1233 backwind1233 added Client This issue points to a problem in the data-plane of the library. bug This issue requires a change to an existing behavior in the product in order to be resolved. labels Apr 7, 2021
@backwind1233
Copy link
Contributor Author

@ZhuXiaoBing-cn I suggest don't change the OAuth2AuthenticatedPrincipal's getName method, we can replace BearerTokenAuthentication with our own class

@backwind1233 backwind1233 added this to the [2021] May milestone Apr 7, 2021
@saragluna
Copy link
Member

@backwind1233 @ZhuXiaoBing-cn will this change in #20340 also affect the Prinicipal we're talking in this thread?

@saragluna
Copy link
Member

Okay, even though they're all implementing the OAuth2AuthenticatedPrincipal, OAuth2User and DefaultOAuth2AuthenticatedPrincipal. But only the OAuth2User could leverage the usernameAttributeName property. For this reason https://github.com/spring-projects/spring-security/blob/7ef3f619242816683a72b35a1f8b4fb4f32d5203/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/user/OAuth2User.java#L33.

azure-sdk pushed a commit to azure-sdk/azure-sdk-for-java that referenced this issue Sep 13, 2022
Review request for Microsoft.ContainerService to add version 2022-08-02-preview (Azure#20487)

* Adds base for updating Microsoft.ContainerService from version preview/2022-07-02-preview to version 2022-08-02-preview

* Updates readme

* Updates API version in new specs and examples

* sync changes from PR#19592 & PR#20145 (Azure#20402)

* update readme & remove fleets (Azure#20388)

* AKS agent pool properties add AgentPoolWindowsProfile and DisableOutboundNAT (Azure#20407)

* AKS agent pool properties add AgentPoolWindowsProfile and DisableOutboundNAT

* Change disableOutboundNAT to disableOutboundNat because of ARM format; Improve description.

* Improve description

* Added guardrails profile to specs (Azure#20171)

* added guardrails profile to specs

* linter fix

* added x-ms-enum

* added guardrails definitions for versions route

* linter fix, added custom word

* fix

* typo fix

* fix for example

* added object type

* added object type

* removed data field

* changed descriptions

* *

* add "Mariner" for AKS os sku (Azure#20420)

* add KubeProxyConfig to AKS NetworkProfile (Azure#20446)

* add KubeProxyConfig to AKS NetworkProfile

* remove extra comma, type: integer for *Seconds

* add custom words

* prettier

* follow camel case ARM guidance

* 2nd camel case standard

* feat: add rest api spec for ip-based lb (Azure#20392)

* fix LroLocationHeader (Azure#20491)

* fix ProvisioningStateValidation (Azure#20490)

* Removed guardrailsVersions route, added guardrails profice to MC properties (Azure#20625)

* removed guardrails-versions route, added guardrails profice to MC properties

* removed unsued file for guardrails versions, removed unused custom word

Co-authored-by: Shiqian Tao <62196586+ShiqianTao@users.noreply.github.com>
Co-authored-by: rsamigullin <89222124+rsamigullin@users.noreply.github.com>
Co-authored-by: Ace Eldeib <aleldeib@microsoft.com>
Co-authored-by: Matt Stam <mattstam@live.com>
Co-authored-by: Qi Ni <pomelonicky@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues. bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library.
Projects
None yet
3 participants