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

[FEATURE REQUEST] AadJwtBearerTokenAuthenticationConverter Change In Public Methods #28665

Closed
3 tasks done
azizabah opened this issue May 5, 2022 · 7 comments · Fixed by #32335
Closed
3 tasks done
Assignees
Labels
azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@azizabah
Copy link

azizabah commented May 5, 2022

Describe the bug
When upgrading from Azure Spring BOM 3.6.0 to 4.0.0, the exposed methods on AadJwtBearerTokenAuthenticationConverter (in addition to the AAD to Aad in the name) have changed. Specifically, we used this method historically to set a custom implementation that extended AadJwtGrantedAuthoritiesConverter.

 public void setJwtGrantedAuthoritiesConverter(
        Converter<Jwt, Collection<GrantedAuthority>> jwtGrantedAuthoritiesConverter) {
        this.jwtGrantedAuthoritiesConverter = jwtGrantedAuthoritiesConverter;
    }

To the best of my knowledge, I am not seeing an equivalent in the exposed constructors or existing methods here: https://github.com/Azure/azure-sdk-for-java/blob/c95ee077372865664baf2f9eec7e55397ec09b84/sdk/spring/spring-cloud-azure-autoconfigure/src/main/java/com/azure/spring/cloud/autoconfigure/aad/AadJwtBearerTokenAuthenticationConverter.java

Expected behavior
We should be able to set a custom JwtGrantedAuthoritiesConverter. Right now it is hardcoded by this line of code:

If you suspect a dependency version mismatch (e.g. you see NoClassDefFoundError, NoSuchMethodError or similar), please check out Troubleshoot dependency version conflict article first. If it doesn't provide solution for the problem, please provide:

  • verbose dependency tree (mvn dependency:tree -Dverbose)
  • exception message, full stack trace, and any available logs

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • Bug Description Added
  • Repro Steps Added
  • Setup information Added
@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels May 5, 2022
@saragluna saragluna modified the milestones: Backlog, [2022] May May 6, 2022
@saragluna saragluna added Client This issue points to a problem in the data-plane of the library. azure-spring All azure-spring related issues azure-spring-aad Spring active directory related issues. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels May 6, 2022
@chenrujun
Copy link

Hi, @azizabah . Thanks for reaching out.

  1. What is your customized jwtGrantedAuthoritiesConverter like?
  2. Is it possible to use AadJwtBearerTokenAuthenticationConverter(String principalClaimName, Map<String, String> claimToAuthorityPrefixMap) instead of setJwtGrantedAuthoritiesConverter(...) to satisfy your requirement?
  3. Maybe you can try this workaround before this issue resolved: Write your own JwtBearerTokenAuthenticationConverter by extending AadJwtBearerTokenAuthenticationConverter or Converter<Jwt, AbstractAuthenticationToken>. Then use the converter in your WebSecurityConfigurerAdapter just like AadResourceServerWebSecurityConfigurerAdapter did.

@azizabah
Copy link
Author

azizabah commented May 6, 2022

Thanks for taking a look, @chenrujun .

  1. Effectively it converts the JWT using AadJwtGrantedAuthoritiesConverter. It then has some business logic to add additional grants based on internal business logic to determine the userPrincipal and then a call to an internal service for users' permissions. It then concatenates the original Collection from the super.convert(jwt) with our added ones from the internal service call.
  2. Not in any way that's readily obvious to me.
  3. That's exactly what we did before the contract was broken. We have a CustomJwtAuthenticationConverter that extends AadJwtBearerTokenAuthenticationConverter and a CustomGrantedAuthoritiesConverter that extends AadJwtGrantedAuthoritiesConverter and we set the JwtGrantedAuthoritiesConverter on the CustomJwtAuthenticationConverter to the CustomGrantedAuthoritiesConverter. We then set that as the jwtAuthenticationConverter on a class that extends AadResourceServerWebSecurityConfigurerAdapter.

Looking at AadJwtBearerTokenAuthenticationConverter, I will have to effectively copy + paste the convert method into our class, override it, and then add in the missing logic. This is bad because now we need to be aware of any changes in that class' method and incorporate those into our duplicate code just because the contract was broken.

@chenrujun
Copy link

@azizabah Your request make sense. We will consider to add this feature back.

@saragluna saragluna modified the milestones: [2022] May, [2022] June, Backlog May 9, 2022
@saragluna saragluna changed the title [BUG] AadJwtBearerTokenAuthenticationConverter Change In Public Methods [FEATURE REQUEST] AadJwtBearerTokenAuthenticationConverter Change In Public Methods Sep 6, 2022
@stliu stliu modified the milestones: Backlog, 2022-12, 2023-01 Nov 8, 2022
@azizabah
Copy link
Author

@saragluna @stliu @chenrujun - Any chance of getting this prioritized? Also the renaming to a feature feels inaccurate because this is a loss of functionality do to it contracts being broken by Azure SDK, not something new needing to be added.

We would like to be able to upgrade this sometime soon given the support timing for 3.6.0.

@saragluna
Copy link
Member

@azizabah thanks for reaching out, we will discuss it and come back to you later. @moarychan could you help take a look at this?

@moarychan
Copy link
Member

moarychan commented Nov 17, 2022

I checked the changes after the 3.7.0, it was brought in this PR #23444.

I propose the below changes on the 2 major development branches:
main branch on Spring Boot 2.7:

feature/spring-boot-3 branch on Spring Boot 3:

@stliu
Copy link
Member

stliu commented Nov 22, 2022

@moarychan could you ping me offline and let's discuss this

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. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved.
Projects
Archived in project
5 participants