Add option to use jwksCache in NimbusJwtDecoder

[nmz]

Added option provide jwksCache so that the ttl can be configured

[pivotal-issuemaster]

@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.

[pivotal-issuemaster]

@nmz Thank you for signing the Contributor License Agreement!

[jzheaux]

Thanks for the pull request, @nmz! I've left some feedback inline.

[jzheaux]

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.

[20fps]

@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

[jzheaux]

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.

[jzheaux]

Please also add a unit test that confirms that the provided cache gets used.

[jzheaux]

@nmz are you able to apply the changes requested?

[andersflemmen]

@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.

[jzheaux]

Good questions, @andersflemmen. Let's see if I can address a couple of them.

Why shouldn't it be possible to set the cache time directly?

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 RestOperations. This keeps the builder from getting cluttered with several small configuration options while at the same time giving the application a great deal of configuration power.

That said, another route could be to introduce a more generic builder - NimbusJwtDecoder.withJwkSource(JWKSource) - similar to the one that NimbusReactiveJwtDecoder already has. In that case, the application could instantiate its own JWKSource:

@Bean 
JwtDecoder jwtDecoder() {
    JWKSource<SecurityContext> jwkSource = new RemoteJWKSet(url, retriever, cache);
    return NimbusJwtDecoder.withJwkSource(jwkSource).build();
}

NimbusJwtDecoder.withJwkSetUri(String) could be enhanced to delegate to this builder.

5 minutes is a pretty short default cache lifespan.

This would be something to take up with the Nimbus folks. Perhaps they'd accept a pull request to lengthen the default ttl.

[andersflemmen]

@jzheaux Thank you for clarifying, that makes sense. Would be nice to see either this pull request completed, or an withJwkSource option.

[jzheaux]

Closed in favor of #8332