Cache on URI not registration #56
Conversation
The cache should be based on the URI that we retrieve the JWK set from and not the client registration. That way if the JWK set URI changes we will start looking for a different value in the cache and the change will correctly be picked up. This also makes the cache much more efficient as if multiple tools (clientRegistrations) share the same JWK set then they will share the same JwtDecoder.
buckett
requested review from
Copilot,
nicholaswilson100 and
sebastianchristopher
There was a problem hiding this comment.
Pull request overview
This PR changes the caching strategy for JwtDecoder instances from being keyed by client registration ID to being keyed by JWK Set URI. This allows multiple client registrations that share the same JWK Set URI to reuse the same JwtDecoder, improving efficiency. It also ensures that if a JWK Set URI changes for a registration, the cache correctly picks up the change.
Key changes:
- Cache key changed from
clientRegistration.getRegistrationId()tojwkSetUri - JWK Set URI validation moved earlier in the method before cache lookup
- Error messages still reference registration ID for clarity when URI is missing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JwtDecoder jwtDecoder = this.jwtDecoders.get(jwkSetUri); | ||
| if (jwtDecoder == null) { | ||
| if (!StringUtils.hasText(clientRegistration.getProviderDetails().getJwkSetUri())) { | ||
| OAuth2Error oauth2Error = new OAuth2Error( | ||
| MISSING_SIGNATURE_VERIFIER_ERROR_CODE, | ||
| "Failed to find a Signature Verifier for Client Registration: '" + | ||
| clientRegistration.getRegistrationId() + "'. Check to ensure you have configured the JwkSet URI.", | ||
| null | ||
| ); | ||
| throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.toString()); | ||
| } | ||
| // TODO This should look at the Cache-Control header so to expire old jwtDecoders. | ||
| // Canvas looks to rotate it's keys monthly. | ||
| String jwkSetUri = clientRegistration.getProviderDetails().getJwkSetUri(); | ||
| NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder decoderBuilder = NimbusJwtDecoder.withJwkSetUri(jwkSetUri).jwsAlgorithm(SignatureAlgorithm.from(JwsAlgorithms.RS256)); | ||
| if (restOperations != null) { | ||
| decoderBuilder.restOperations(restOperations); | ||
| } | ||
| jwtDecoder = decoderBuilder.build(); | ||
| this.jwtDecoders.put(clientRegistration.getRegistrationId(), jwtDecoder); | ||
| this.jwtDecoders.put(jwkSetUri, jwtDecoder); | ||
| } |
There was a problem hiding this comment.
There is a potential race condition in the check-then-act pattern. If multiple threads call this method concurrently with the same jwkSetUri, they could all pass the null check at line 177 and create multiple JwtDecoder instances, with only the last one winning. While ConcurrentHashMap is thread-safe for individual operations, the composite operation (check if absent, then put) is not atomic.
Consider using computeIfAbsent to ensure atomic creation of JwtDecoder instances:
return this.jwtDecoders.computeIfAbsent(jwkSetUri, uri -> {
NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder decoderBuilder = NimbusJwtDecoder.withJwkSetUri(uri).jwsAlgorithm(SignatureAlgorithm.from(JwsAlgorithms.RS256));
if (restOperations != null) {
decoderBuilder.restOperations(restOperations);
}
return decoderBuilder.build();
});
This ensures only one JwtDecoder is created per unique jwkSetUri, even under concurrent access.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
* Initial plan * Fix race condition using computeIfAbsent for atomic JwtDecoder creation Co-authored-by: buckett <5921+buckett@users.noreply.github.com> * Fix grammar in TODO comment (it's -> its) Co-authored-by: buckett <5921+buckett@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: buckett <5921+buckett@users.noreply.github.com>
The cache should be based on the URI that we retrieve the JWK set from and not the client registration. That way if the JWK set URI changes we will start looking for a different value in the cache and the change will correctly be picked up.
This also makes the cache much more efficient as if multiple tools (clientRegistrations) share the same JWK set then they will share the same JwtDecoder.