Accept predicate in constructor for JwtIssuerAuthenticationManagerRes… #10002
Conversation
…olver Add a constructor to JwtIssuerAuthenticationManagerResolver to allow it to accept a Predicate<String> to determine whether an issuer should be trusted or not. This allows for cases where the trusted issuers are not necessarily known at application startup. Since JwtIssuerAuthenticationManagerResolver is final and internal classes are private, this is not possible to extend it to support this use case without duplicating the whole class.
|
@barrypitman Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@barrypitman Thank you for signing the Contributor License Agreement! |
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks, @barrypitman! I've left some feedback inline.
In addition to that feedback, will you please format your commit message?
| @@ -86,6 +86,16 @@ public JwtIssuerAuthenticationManagerResolver(Collection<String> trustedIssuers) | |||
| new TrustedIssuerJwtAuthenticationManagerResolver( | |||
There was a problem hiding this comment.
Will you please update the copyright message to now include 2021?
| * @param trustedIssuer a predicate to determine whether the issuer should be trusted or not | ||
| */ | ||
| public JwtIssuerAuthenticationManagerResolver(Predicate<String> trustedIssuer) { | ||
| this.authenticationManager = new ResolvingAuthenticationManager( |
There was a problem hiding this comment.
Please check for null constructor parameters. You can look at the other constructors as examples.
| * Construct a {@link JwtIssuerAuthenticationManagerResolver} using the provided | ||
| * parameters | ||
| * @param trustedIssuer a predicate to determine whether the issuer should be trusted or not | ||
| */ |
| public JwtIssuerAuthenticationManagerResolver(Predicate<String> trustedIssuer) { | ||
| this.authenticationManager = new ResolvingAuthenticationManager( | ||
| new TrustedIssuerJwtAuthenticationManagerResolver(trustedIssuer)); | ||
| } |
There was a problem hiding this comment.
Please add unit tests in JwtIssuerAuthenticationManagerResolverTests.
| public JwtIssuerAuthenticationManagerResolver(Predicate<String> trustedIssuer) { | ||
| this.authenticationManager = new ResolvingAuthenticationManager( | ||
| new TrustedIssuerJwtAuthenticationManagerResolver(trustedIssuer)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Are you able to add this same functionality for JwtIssuerReactiveAuthenticationManagerResolver?
jzheaux
added
in: oauth2
|
Hi, @barrypitman! Are you able to apply the requested changes? |
|
@barrypitman, thinking about this a bit more in conjunction with #10476, I think that the best constructor for these use cases is The reason is that introducing every constructor to allow for the myriad configurations applications want to do will become hard to maintain. In isolation, each seems reasonable and small, but the small changes start to add up. Instead, I believe we can simplify composing a custom For example, an public class MyAuthenticationManagerResolver implements AuthenticationManagerResolver<String> {
Predicate<String> myPredicate = // ...
@Cacheable(unless="#result==null")
public AuthenticationManager resolve(String issuer) {
if (!myPredicate.test(issuer)) {
return null;
}
return new JwtAuthenticationProvider(JwtDecoders.fromIssuerLocation(issuer))::authenticate;
}
}
// ...
@Bean
AuthenticationManagerResolver<String> multitenantResolver() {
return new JwtIssuerAuthenticationManagerResolver(new MyAuthenticationManagerResolver());
}which, while already quite simple, could be further simplified by Spring Security exposing a default resolver like so: public static AuthenticationManager fromIssuerLocation(String issuer) {
return new JwtAuthenticationProvider(JwtDecoders.fromIssuerLocation(issuer))::authenticate;
}As such, I'm going to close this PR. Please let me know if I've missed something that makes it more complex than I've described, and you and I can take another look at simplifying your use case. |
jzheaux
added
the
status: declined
|
No problem, thanks for the explanation Josh |
Add a constructor to JwtIssuerAuthenticationManagerResolver to allow it to accept a Predicate to determine whether an issuer should be trusted or not. This allows for cases where the trusted issuers are not necessarily known at application startup. Since JwtIssuerAuthenticationManagerResolver is final and internal classes are private, this is not possible to extend it to support this use case without duplicating the whole class.