Auto-configure JwtDecoder via OpenId Configuration

[jzheaux]

Adding OpenIdConfigurationJwtDecoderProvider which takes an issuer
and derives from it the jwk set uri via a call to
.well-known/openid-configuration

Fixes: gh-5523

[jgrandja]

This class needs to be in the config module just like OidcConfigurationProvider is.

Also, the name of this class restricts it to JwtDecoder configuration only. I think we need to rename it to be more generic so it makes sense when we need other provider configuration and associated static factory methods.

The common naming convention is to prefix with Oidc not OpenId

[jzheaux]

Makes sense to be consistent. Can you clarify why the existing OidcConfigurationProvider is in the config module, so I better understand this for the future? My guess is so that it can have broader access to security components across multiple modules.

As for the name, the only generic name that makes sense to me is OidcConfigurationProvider, just in a different package (since the existing one is in a client package). This is because they have the same object in mind: Use OIDC to discover and configure some security component. What are your thoughts on two classes of the same name, in different packages? Or what rules do we have for moving the existing OidcConfigurationProvider into a more generic package like o.s.s.config.oauth2.oidc.OidcConfigurationProvider which could then house both methods?

I we can combine the two, then I might like something named OidcDiscoveryConfigurationProvider because it clarifies the strategy being employed. Thoughts?

I also have a concern about the fact that, should these be combined (or future methods be added to either), the current method naming convention won't suffice. Could something like the following work instead:

public final class OidcConfigurationProvider {
    
    private OidcConfigurationProvider(OIDCProviderMetadata metadata) { ... }

    public static OidcConfigurationProvider issuer(String issuer) {
        String openidConfiguration = getOpenidConfiguration(issuer);
        OIDCProviderMetadata metadata = parse(openidConfiguration);
        // ... verify metadata
        return new OidcConfigurationProvider(metadata);
    }

    public JwtDecoder getJwtDecoder() {
        // ... derive from metadata
    }

    public ClientRegistration.Builder getClientRegistrationBuilder() {
        // ... derive from metadata
    }
}

The benefit of this approach is that users could create many security components without invoking the endpoint and doing the subsequent parsing and verification work more than once:

OidcConfigurationProvider.issuer("https://issuer").getClientRegistrationBuilder()

// or

OidcConfigurationProvider provider = OidcConfigurationProvider.issuer("https://issuer");
ClientRegistration.Builder builder = provider.getClientRegistrationBuilder();
JwtDecoder decoder = provider.getJwtDecoder();

[jgrandja]

Can you clarify why the existing OidcConfigurationProvider is in the config module

This is considered Java Config, just like OAuth2ResourceServerConfigurer and OAuth2ClientConfigurer, hence it belonging in config module

the only generic name that makes sense to me is OidcConfigurationProvider

Yeah same for me. Although we have this ticket to rename to OidcProviderConfiguration

What are your thoughts on two classes of the same name, in different packages

I was thinking the same and initially it makes sense but at the same time having users (or Boot) to fully qualify the class name when both are used (Client and Resource Server) would be a pain. I'm not sure what other name we can use at the moment.

Or what rules do we have for moving the existing OidcConfigurationProvider into a more generic package like o.s.s.config.oauth2.oidc.OidcConfigurationProvider which could then house both methods?

Housing both methods in OidcConfigurationProvider would require both dependencies - client and resource server - which is not good especially if the user is only using client for example.

However, I believe @rwinch did suggest this strategy (housing in one config class) when we spoke? Maybe he can clarify this further as I'm not sure if I understood him correctly.

I we can combine the two, then I might like something named OidcDiscoveryConfigurationProvider

I think we should use the name OidcProviderConfiguration to align with the spec. Or at least have that in the class name.

I do like your implementation approach on caching the OIDCProviderMetadata as per

public static OidcConfigurationProvider issuer(String issuer)

[jzheaux]

I think we should use the name OidcProviderConfiguration to align with the spec. Or at least have that in the class name.

My bad, I had misread the spec when I saw "Discovery" in the title [1], forgetting that we are just focused on the configuration section.

[1] - https://openid.net/specs/openid-connect-discovery-1_0.html#IssuerDiscovery

[jgrandja]

The code in here is duplicated from OidcConfigurationProvider, the getOpenidConfiguration and parse methods. We should try to reuse.

[jgrandja]

This dependency does not make sense. However, this will be removed when moving OpenIdConfigurationJwtDecoderProvider to the config module

[jzheaux]

For the record, @jgrandja and I discussed, and this will stay here in oauth2-jose since JwtDecoder, the security component being instantiated by JwtDecoders, is in oauth2-jose.

[jgrandja]

This is not used anywhere. We should remove it.

[jzheaux]

@jgrandja please consult one of the unit tests, which confirms that the configuration took effect. The two alternatives are to use reflection to confirm this setting or to perform some heavier configuration to mock this url endpoint in addition to the openid-configuration endpoint already mocked.

I'm okay with the first option, though not precisely ideal, though the second option I believe increases the scope of the test too much.

What would you do in this situation?

[jzheaux]

Also, I believe this situation would be simplified if we could expose RestOperations as configurable here as well as in NimbusJwtDecoderJwkSupport. Then, this test could provide a mock RestOperations and alleviate the need to mock an underlying server.

That being the case, I'm also okay with leaving the unit test in a suboptimal state (calling reflectively a private method) in anticipation of the class under test being able to more easily configure mock endpoints.

[jgrandja]

I must be missing something? I don't see any references to getJwkSetUrl() after searching for usages within IDE.

I believe this situation would be simplified if we could expose RestOperations as configurable here as well as in NimbusJwtDecoderJwkSupport

This is coming in #5603

[jgrandja]

@jzheaux Also, please consider this comment when re-working this PR

NOTE: We may have to re-factor the existing OidcConfigurationProvider to support both configurations of ClientRegistration and JwtDecoder. However, we're also debating on keeping these in separate classes - client and resource server specific configs. It's difficult to tell at this point until the work get's started so any suggestions you have let's start that dialogue in a PR.

[jgrandja]

@jzheaux Looks like you included the commit Jwt Claim Validation in this PR. Can you please remove that and re-submit.

[jzheaux]

Regarding including the Jwt Claim Validation commit, that is only temporary. It's a little messy, but since this is dependent on that, I rebased off of it so that the PR would compile. Once we merge the validation PR, I'll remove that commit here.

[jzheaux]

@rwinch This is ready for your review.

[rwinch]

Thanks this is merged into master