OidcIdTokenDecoderFactory caching of ClientRegistration fails to decode tokens if clientId changes

[unb]

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

	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<Map<String, Object>, Map<String, Object>> claimTypeConverter = this.claimTypeConverterFactory
					.apply(clientRegistration);
			if (claimTypeConverter != null) {
				jwtDecoder.setClaimSetConverter(claimTypeConverter);
			}
			return jwtDecoder;
		});
	}

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.

[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?

[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);

[sjohnr]

these may change at runtime

Apologies, but I still don’t understand. Why would a clientId change at runtime?

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

[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?