Describe the bug OidcIdTokenDecoderFactory caches JwtDecoder instances on ClientRegistration.getRegistrationId(). The cached instance holds a reference to the ClientRegistration. If a new ClientRegistration is created with a different clientId but the same registrationId, the cached JwtDecoder is still returned. This causes validation errors when parsing tokens: "[invalid_id_token] The ID Token contains invalid claims..."
``` @Override
public JwtDecoder createDecoder(ClientRegistration clientRegistration) {
Assert.notNull(clientRegistration, "clientRegistration cannot be null");
return this.jwtDecoders.computeIfAbsent(clientRegistration.getRegistrationId(), (key) -> {
NimbusJwtDecoder jwtDecoder = buildDecoder(clientRegistration);
jwtDecoder.setJwtValidator(this.jwtValidatorFactory.apply(clientRegistration));
Converter
**To Reproduce**
ClientRegistration registration1 = CommonOAuth2Provider.GOOGLE.getBuilder("google").clientId("1").....build(); ClientRegistration registration2 = CommonOAuth2Provider.GOOGLE.getBuilder("google").clientId("2").....build();
OidcIdTokenDecoderFactory factory = new OidcIdTokenDecoderFactory(); JwtDecoder decoder1 = factory.createDecoder(registration1); Jwt jwt1 = decoder1.decode(token1); // token1 associated with clientId "1" JwtDecoder decoder2 = factory.createDecoder(registration2); Jwt jwt2 = decoder2.decode(token2); // token2 associated with clientId "2".
// The second call to decode() fails with "[invalid_id_token] The ID Token contains invalid claims..." as decoder2 == decoder1, which points to registration1
Expected behavior The OidcIdTokenDecoderFactory should discard the cached instance if the cached ClientRegistration has a different clientId to that supplied.
**Comment From: sjohnr**
Hi @unb, thanks for the report.
> If a new ClientRegistration is created with a different clientId but the same registrationId, the cached JwtDecoder is still returned.
Can you help me understand why this would happen? What is the use case for this behavior?
**Comment From: unb**
I have a ClientRegistrationRepository implementation that stores ClientRegistrations in a database; these may change at runtime.
In my case, I ran decode() with one version of a ClientRegistration, changed it, then ran it again. It failed on the second call with the [invalid_id_token] error, as it had the original Client Id.
As a workaround, I now create the OidcIdTokenDecoderFactory each time:
`JwtDecoder decoder = new OidcIdTokenDecoderFactory().createDecoder(clientRegistration);`
**Comment From: sjohnr**
> these may change at runtime
Apologies, but I still don’t understand. Why would a clientId change at runtime?
**Comment From: unb**
The client id is user configurable at runtime, in order to support integration with multiple email providers via oauth2.
I was documenting the process and found that when I used a new client id, OidcIdTokenDecoderFactory failed with the `[invalid_id_token]` error. It could also occur if a user has entered the wrong id and wants to correct it.
We can't hardcode the client id & secret, as its an opensource project.
**Comment From: sjohnr**
Thanks @unb. The javadoc of `OidcIdTokenDecoderFactory` states:
> A factory that provides a `JwtDecoder` used for `OidcIdToken` signature verification. The provided `JwtDecoder` is associated to a **specific** `ClientRegistration`.
(emphasis added)
Since the `ClientRegistration` is identified by `registrationId`, this is the only field used to make the determination that a cached instance can be used.
> **Expected behavior**
>
> The OidcIdTokenDecoderFactory should discard the cached instance if the cached ClientRegistration has a different clientId to that supplied.
I'm not confident that we can generalize the problem you described this way. In general, I think you'd have to check nearly _every_ field on the `ClientRegistration`. The `OidcIdTokenDecoderFactory` is not really designed for this case, so I don't feel the complexity would fit well in this implementation of `JwtDecoderFactory`. It is really just a cache, and caches can/should be managed by the application when required.
One potential improvement might be to add a `remove()` or `evict()` type method to `OidcIdTokenDecoderFactory`. This would allow the application to determine when to evict an entry in the cache. What do you think of this suggestion?
<!--
In some cases, you may also be able to replace `OidcIdTokenDecoderFactory` with `NimbusJwtDecoder.with*` methods directly. If you want/need to continue using `OidcIdTokenDecoderFactory` in the meantime, the simplest way to handle this would be to re-create the instance any time you save a `ClientRegistration` to the database.
Another possibility is to always delete the old `ClientRegistration` in the database and create a new one with a different `registrationId`. However, this will not clean up after itself and could result in `JwtDecoder` instances being left in memory. In this case, re-creating the instance of `OidcIdTokenDecoderFactory` occasionally would still be required.
-->
<!--
If you feel that detecting a change in `clientId` is sufficient to throw away the previously cached instance, you could build a delegating implementation that uses a different key, like so:
```java
However, this will not clean up after itself and could result in JwtDecoder instances being left in memory.
-->
Comment From: unb
That might work. In order to use it, the caller would need to keep their own cache of ClientRegistration instances and synchronise access e.g.:
public class MyTokenDecoderFactory extends OidcIdTokenDecoderFactory {
private final Map<String, ClientRegistration> registrations = new HashMap<>();
@Override
public synchronized JwtDecoder createDecoder(ClientRegistration clientRegistration) {
if (registrationChanged(clientRegistration)) {
evict(clientRegistration.getRegistrationId());
}
return super.createDecoder(clientRegistration);
}
private boolean registrationChanged(ClientRegistration registration) {
ClientRegistration existing = registrations.get(registration.getRegistrationId());
return (existing != null && !existing.getClientId().equals(registration.getClientId());
}
}
Alternatively, have createDecoder(ClientRegistration clientRegistration) delegate to a new OidcIdTokenDecoderFactory JwtDecoder createDecoder(ClientRegistration registration, boolean replace) method. If replace is true, it always replaces any existing instance, otherwise it uses the cached instance.
The default OidcIdTokenDecoderFactory implementation would be return createDecoder(clientRegistration, false);
Subclasses would override createDecoder(ClientRegistration clientRegistration) and invoke createDecoder(ClientRegistration registration, boolean replace) with set replace to true if the registration had changed.
Comment From: sjohnr
the caller would need to keep their own cache of ClientRegistration instances and synchronise access
I was thinking about the caller simply calling evict when the database operation (save) is performed. Regardless, the application would be able to decide when to evict however it wants to.
I don’t think making major changes to the existing method is necessary, nor is the creation of an entirely new implementation in the framework.
Comment From: unb
To be honest, I don't think it should cache at all; a JwtDecoderFactory wrapper could be used instead to provide caching. I don't feel strongly about it however, and your solution would support it.
Comment From: jgrandja
Related gh-16647