Add option to use jwksCache in NimbusJwtDecoder#7639
Add option to use jwksCache in NimbusJwtDecoder#7639nmz wants to merge 1 commit intospring-projects:masterfrom
Conversation
|
@nmz Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@nmz Thank you for signing the Contributor License Agreement! |
| this.jwkSetUri = jwkSetUri; | ||
| } | ||
|
|
||
| public JwkSetUriJwtDecoderBuilder jwkSetCache(JWKSetCache jwkSetCache) { |
There was a problem hiding this comment.
I'd prefer exposing an interface that is more powerful than the JWKSetCache interface. If an application really wants to supply a JWKSetCache, then it's quite simple to construct a DefaultJWTProcessor directly and pass that to the NimbusJwtDecoder constructor.
What if this took a org.springframework.cache.Cache instead? Then this builder could internally wrap that in a CacheJWKSetCache implementation, similar to the design of RestOperationsResourceRetreiver.
The nice thing about doing that is that an application can then use whatever caching mechanism they wish, be it Caffeine, Hazelcast, or something else.
Another nice this is that using Cache allows the entries to be key-value based which is better suited for a multi-tenant resource server that is caching keys from multiple issuers.
There was a problem hiding this comment.
@jzheaux Sry, but how should it help to multi-tenant resource server? I mean in multi-tenant environment we will have a separate AuthenticationManager for each tenant, that means for each tenant we will have: unique jwks-uri -> unique decoder -> so unique jwkSetCache, I don't see possible way for multiple keys to be stored in JWKSetCache with the current implementation
There was a problem hiding this comment.
Good question, @20fps. I guess it comes down to what parts of your infra are multi-tenant. If your cache is multi-tenant, then each AuthenticationManager could have a JwtDecoder that shares the same cache - each JWK Set keyed, for example, by that tenant's JWK Set Uri.
If each tenant has its own cache, then that point is moot. My main point is that by introducing a key to the cache grants additional flexibility - multi-tenancy is one way that flexibility could be leveraged.
| Assertions.assertThatCode(() -> builder.jwsAlgorithm(null)).isInstanceOf(IllegalArgumentException.class); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Please also add a unit test that confirms that the provided cache gets used.
|
@nmz are you able to apply the changes requested? |
|
@jzheaux if you want the same setup as with the NimbusJwtDecoder, but be able to set the cache time, it's not really just as simple as using the DefaultJwtProcessor though? Why shouldn't it be possible to set the cache time directly? 5 minutes is a pretty short default cache lifespan. |
|
Good questions, @andersflemmen. Let's see if I can address a couple of them.
Spring Security favors exposing components over individual properties. Consider, for example, a related use case of changing the socket timeout for the JWK Set request. Instead of exposing a method for changing the socket timeout, Spring Security exposes a method for supplying a custom That said, another route could be to introduce a more generic builder - @Bean
JwtDecoder jwtDecoder() {
JWKSource<SecurityContext> jwkSource = new RemoteJWKSet(url, retriever, cache);
return NimbusJwtDecoder.withJwkSource(jwkSource).build();
}
This would be something to take up with the Nimbus folks. Perhaps they'd accept a pull request to lengthen the default ttl. |
|
@jzheaux Thank you for clarifying, that makes sense. Would be nice to see either this pull request completed, or an withJwkSource option. |
|
Closed in favor of #8332 |
Added option provide jwksCache so that the ttl can be configured