Allow configurable JwtDecoder for ID Token verification

[jgrandja]

This PR resolves gh-5717

@rwinch There is an issue with the changes in this PR.

Given the following that allows for setting a custom JwtDecoder factory:

http
  .oauth2Login()
    .tokenEndpoint()
      .jwtDecoderFactory(Function<ClientRegistration, JwtDecoder> jwtDecoderFactory)
      ...         

This introduces the oauth2-jose dependency for oauth2Login which is not ideal.
I'm not sure what we can do at this point to not include oauth2-jose as a required dependency?
Maybe you have an idea?

[rwinch]

I agree that it isn't ideal to have the jose dependency here. Perhaps the better approach is allowing the user to configure the AuthenticationProvider instead.

[jzheaux]

Since you are using ConcurrentHashMap, is it better to use computeIfAbsent instead of get?

[jgrandja]

Thanks for that catch @jzheaux! Yes, we should use computeIfAbsent. I'll apply that update.

[jgrandja]

@rwinch

Perhaps the better approach is allowing the user to configure the AuthenticationProvider instead

Sure this is another option to consider. However, given where we are with the release, this won't make it into 5.1. We'll come back to this after 5.1 is out.

[jgrandja]

@rwinch I decided to go with the @Bean approach to allow users to configure a custom OidcIdToken signature verifier. The @Bean is expected to be of type Function<ClientRegistration, JwtDecoder>, as per our previous discussions.

I'm wondering if we should consider declaring a new type for this? It's just that Function is quite generic with the apply and maybe something like this would be more descriptive for the user:

public interface IdTokenDecoders {
	JwtDecoder getDecoder(ClientRegistration clientRegistration);
}

[rwinch]

I think you are right we need a custom interface. However, I think we should consider a different name. Perhaps JwtDecoderFactory?

[jgrandja]

I feel JwtDecoderFactory is too general given this factory is really meant for client only. The name JwtDecoderFactory can be applied to client and resource server whereas IdTokenDecoderFactory is meant for client as far as how a user may interpret the name.

The reason I'm thinking IdToken should be in the name is that it's only meant for ID Token verification. I'm not anticipating the use of a JwtDecoder for client in any other areas - at least at this moment.

I think IdTokenDecoderFactory is fine but I also like IdTokenDecoders as well.

[rwinch]

I see your point about it being client specific, but I still don't like the name. The Id Token is not part of the input or the output of the only method on the interface. Do we anticipate needing something similar for the resource server? If so, perhaps we make the input generic and stick with something like JwtDecoderFactory?

[jgrandja]

Do we anticipate needing something similar for the resource server?

Very likely, probably with the multi-tenancy feature we still need to get to

we make the input generic and stick with something like JwtDecoderFactory

I like this option and provides us the flexibility. I'll go with this

[jgrandja]

@rwinch I added the new JwtDecoderFactory and ReactiveJwtDecoderFactory.

[rwinch]

Did you verify this isn't going to break any existing test assumptions? We probably shouldn't modify this as we don't know what impact this is having on existing tests. It may invalidate a test that has different expectations of the scopes.

[jgrandja]

I did validate that this does not trigger any failed tests. However, it's probably safer to avoid this change and keep the changes isolated to the test that requires the openid scope. I'll make this change.

[jgrandja]

@rwinch I pushed the update for this