Allow for customization of IssuerResolver #9005
Conversation
…rter Customization of IssuerConverter and JwtAuthenticationConverter will make it easier to customize the way JWTs are handled, especially in a multi-tenant env, with the use of the default JwtAuthenticationProvider. This eliminates the need to write a complex implementation for different tasks, that ideally should be quick and easy, such as custom GranthedAuthorities conversion. Closes gh-8535
|
@AbstractConcept Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
spring-projects-issues
added
the
status: waiting-for-triage
|
@AbstractConcept Thank you for signing the Contributor License Agreement! |
…rter Customization of IssuerConverter and JwtAuthenticationConverter will make it easier to customize the way JWTs are handled, especially in a multi-tenant env, with the use of the default JwtAuthenticationProvider. This eliminates the need to write a complex implementation for different tasks, that ideally should be quick and easy, such as custom GranthedAuthorities conversion. Closes gh-8535
|
@jzheaux Could you take a look? |
There was a problem hiding this comment.
Thanks for the PR, @AbstractConcept! I've left some feedback inline.
...k/security/oauth2/server/resource/authentication/JwtIssuerAuthenticationManagerResolver.java
Show resolved
Hide resolved
...k/security/oauth2/server/resource/authentication/JwtIssuerAuthenticationManagerResolver.java
Outdated
Show resolved
Hide resolved
jzheaux
added
in: oauth2
|
@AbstractConcept I would also be really interested in having this enhancement 👍 Do you find time to complete the work on your PR or would you mind if I finish this according to @jzheaux feedback? |
|
@arvidOtt I'll get to this, hopefully will be done by tomorrow. |
…arate ticket needed + setter instead of constructor
There was a problem hiding this comment.
Thanks, @AbstractConcept, for the updates! I've left some additional feedback inline.
| */ | ||
| public JwtIssuerAuthenticationManagerResolver( | ||
| AuthenticationManagerResolver<String> issuerAuthenticationManagerResolver, | ||
| Converter<HttpServletRequest, String> issuerConverter) { |
There was a problem hiding this comment.
Since we are using a setter for the issuer converter, this constructor is unnecessary.
| @@ -130,6 +165,10 @@ public AuthenticationManager resolve(HttpServletRequest request) { | |||
| return authenticationManager; | |||
| } | |||
|
|
|||
| public void setIssuerConverter(Converter<HttpServletRequest, String> issuerConverter) { | |||
| this.issuerConverter = issuerConverter; | |||
There was a problem hiding this comment.
Please check for null here using Assert.notNull, as you did earlier in the constructor.
There was a problem hiding this comment.
Also, I believe it would be a bit more consistent with the rest of Spring Security to call this setIssuerResolver. It's certainly reminiscent of BearerTokenResolver in its role in the request.
There was a problem hiding this comment.
Also, out of curiosity, do you have a use case where you need the more generic contract of Converter<HttpServletRequest, String>? If not, setBearerTokenResolver may be better since that's likely the most common use case.
There was a problem hiding this comment.
@jzheaux the problem is that you cannot set the BearerTokenResolver on the issuerConverter as it is of type Converter<HttpServletRequest,String>. So either you replace the entire thing or you would need to add a setter on that one as well and change the type of the issuerConverter attribute directly to JwtClaimIssuerConverter. What do you think?
There was a problem hiding this comment.
I agree with you that adding a setter to JwtClaimIssuerConverter would be one way to do it, though it would remain private.
One nice thing about using BearerTokenResolver is that it simplifies the API and focuses on the most common use cases. It seems like a very uncommon use case to pull the issuer from somewhere else in the request other than the token itself.
| @@ -130,6 +165,10 @@ public AuthenticationManager resolve(HttpServletRequest request) { | |||
| return authenticationManager; | |||
| } | |||
|
|
|||
| public void setIssuerConverter(Converter<HttpServletRequest, String> issuerConverter) { | |||
There was a problem hiding this comment.
Please add JavaDoc, including a @since 5.5 so it's clear when the method became available.
| @@ -160,8 +199,16 @@ public String convert(@NonNull HttpServletRequest request) { | |||
|
|
|||
| private final Predicate<String> trustedIssuer; | |||
|
|
|||
| private final JwtAuthenticationConverter jwtAuthenticationConverter; | |||
There was a problem hiding this comment.
Let's leave these changes regarding JwtAuthenticationConverter for later.
jzheaux
changed the title
|
Thanks for your work on this @AbstractConcept. Another contributor had time to pick up where you left off, so I'm going to close this in favor of #9168 |
jzheaux
added
status: duplicate
Customization of IssuerConverter and JwtAuthenticationConverter will make it easier to customize the way JWTs are handled, especially in a multi-tenant env, with the use of the default JwtAuthenticationProvider. This eliminates the need to write a complex implementation for different tasks, that ideally should be quick and easy, such as custom GranthedAuthorities conversion.
Closes gh-8535