Provided uncached way to use (Reactive)OidcIdTokenDecoderFactory

[iigolovko]

Hello.

Sorry for ignoring issue creating.

If ClientRegistration is managed by db, it's unreal to update jwsAlgorithm or jwksUri without restarting the app.

So I added possibility not to use ConcurrentHashMap on decoder creation to build new one everytime. It will be useful in some multi-integration cases.

Fixes gh-12816

[franticticktick]

Hey @iigolovko thanks for your valuable suggestion, but it would be better to create a ticket for this feature. It is quite possible that as a result of discussion we will find another solution if necessary.

[mipo256]

fixes #16655

[orchestr7]

Hey @iigolovko thanks for your valuable suggestion, but it would be better to create a ticket for this feature. It is quite possible that as a result of discussion we will find another solution if necessary.

Looks like #16655 affects a lot. Any chance that can be reviewed and handled somehow? @franticticktick

[mipo256]

My 2 cents.

I think that caching in general is a functionality that needs to be extracted from here. Maybe the core maintainers can consider introducing the CachingOidcIdTokenDecoderFactory that extend OidcIdTokenDecoderFactory and adds caching functionality. Or the alternative solution via decorator/caching delegate is better.

[jgrandja]

@mipo256 I agree that caching could be removed all together as the simplest solution and one that makes sense since caching is a separate concern that the application can implement using a delegation based strategy.

I'd like to hear feedback from others before we decide to remove the caching.

@unb @iigolovko @franticticktick @orchestr7

[franticticktick]

I agree with @mipo256 , in this case it is better to extract caching to a separate implementation. I don't really like the idea of ​​moving this logic to a delegation based strategy. It can complicate the API and make it less obvious than an explicit class with caching.

[iigolovko]

I don't think caching is necessary at all. But if you decide to keep it, we need to choose a default factory implementation. For me, the non-caching option better meets end user expectations.

[mipo256]

For me, the non-caching option better meets end user expectations.

I can see the motivation here. However, I think in order to stay backward compatible at least, and at most in order to omit the construction of NimbusJwtDecoder (which, at least from the first rough touch, seems to be quite expensive), the default behavior, IMVHO, should remain the cache usage.

But there must be an option to configure it for certain use cases nevertheless

[jgrandja]

Thanks all for the feedback.

I'm leaning towards removing the caching behaviour. This would be a breaking change so it would go into 7.0.

Users who want the caching behaviour could easily restore it using a delegation-based strategy, for example:

public class CachingOidcIdTokenDecoderFactory implements JwtDecoderFactory<ClientRegistration> {
	private final Map<String, JwtDecoder> jwtDecoders = new ConcurrentHashMap<>();
	
	private final OidcIdTokenDecoderFactory delegate = new OidcIdTokenDecoderFactory();

	@Override
	public JwtDecoder createDecoder(ClientRegistration clientRegistration) {
		Assert.notNull(clientRegistration, "clientRegistration cannot be null");
		return this.jwtDecoders.computeIfAbsent(clientRegistration.getRegistrationId(), 
				(key) -> this.delegate.createDecoder(clientRegistration));
	}

}

Thoughts?

[iigolovko]

@jgrandja , I removed it. Please check my PR again.