JwtDecoders and ClientRegistrations should support RFC 8414

[jzheaux]

Related to #5543

JwtDecoders and ClientRegistrations can already retrieve configuration via an OIDC Provider Configuration Endpoint:

JwtDecoder decoder = JwtDecoders.withOidcIssuerLocation(oidcIssuerLocation);
ClientRegistration.Builder builder = ClientRegistrations.withOidcIssuerLocation(oidcIssuerLocation);

RFC 8414 defines a metadata endpoint not tied directly to OIDC, meaning that we should not use the OIDC-specific method here but instead introduce a new one.

One possibility is:

JwtDecoder decoder = JwtDecoders.withIssuer(issuer);
ClientRegistration.Builder builder = ClientRegistrations.withIssuer(issuer);

This more generic name is important since it may still perform the OIDC call as a compatibility measure in addition to the OAuth 2.0 discovery endpoint.

[rhamedy]

Hi @jzheaux
Can I work on this feature? I don't know what is the complexity but, seems like doable. I have some knowledge of JWT and OAuth2 and might require some guidance on what to keep in mind.

[jzheaux]

Certainly, that would be awesome!

Please double check the compatibility section of the spec as we'll want to follow it carefully.

Let me know where I can help out.

[rhamedy]

Hi @jzheaux ,

I am looking into this issue right now. I read some of RFC 8414 specification as well as looked inside JWTDecoder and ClientRegistration.Builder classes. In regards to the possibility you mentioned above

JwtDecoder decoder = JwtDecoders.withIssuer(issuer);
ClientRegistration.Builder builder = ClientRegistrations.withIssuer(issuer);

We might have to provide a mechanism to accept both issuer along with configuration i.e. openid-configuration OR oauth-authorization-server OR etc. Any thoughts?

The RFC 8414 metadata lists a ton of metadata fields. Are all those fields available already and where? if not what and what not should be added? Some pointers would be helpful.

The RFC 8414 also talks about some endpoints that are related token, registration, revocation, etc. and where can I look for these?

Also I noticed that with OpenId in JWTDecoder we have hardcoded the openid-configuration and what about scenarios when the user would like to provide a customized name/url? Maybe this is something of future/down the road.

Also, this the the PR for this go against master or there is a separate branch/tree where these features are tracked?

Lastly, just to be sure that I start on the right path. Can you provide me with some general (high-level) pointers that you think would help me.

Thanks for your time and useful feedback as always 👍

[jzheaux]

@rhamedy Great questions.

We might have to provide a mechanism to accept both issuer along with configuration
what about scenarios when the user would like to provide a customized name/url? Maybe this is something of future/down the road.

Yes, this sounds like a separate ticket, though agreed that RFC 8414 suggests that authorization servers may expose custom endpoints.

The RFC 8414 metadata lists a ton of metadata fields. Are all those fields available already and where? if not what and what not should be added?

We rely on the Nimbus API to parse those fields for us. Anything that isn't yet clearly needed is placed in the configurationMetadata property in ClientRegistration#ProviderDetails.

The RFC 8414 also talks about some endpoints that are related token, registration, revocation, etc. and where can I look for these?

These should be in ClientRegistration#ProviderDetails

Also, this the the PR for this go against master or there is a separate branch/tree where these features are tracked?

I like creating my own branch in my own fork for things like this. I usually call it gh-issuenumber. So, here, I'd call it gh-6500. Then, I do a PR from my fork/branch to origin/master.

(high-level) pointers that you think would help me.

• A small public API is typically the goal for the initial PR. It's easier to tweak a private method than a public method.
• Read the spec carefully, use tests to confirm that you've matched the spec

[rhamedy]

Hi @jzheaux, I have made some progress with this issue and I wanted to briefly described it here before creating a PR to ensure that I have not invented something new.

Assumption:
I assume we cannot drop the existing public method fromOidcIssuerLocation(String issuer) as it is probably because used and the least we can do is Deprecate it (or leave it as is). I have made the modifications in a way ensure that both the existing (fromOidcIssuerLocation(String issuer)) and the new one (shown below) can coexist and none of the existing functionalities are broken. Please correct me if I am wrong?

Modifications:
Focusing on ClientRegistrations and ClientRegistrationsTests for now. I have made the following changes

• Come up with a new public method
  public static ClientRegistration.Builder fromIssuer(String issuer, ClientType type) where ClientType is a Enum 😐as follow
  Note: This won't support custom urls should we decide to introduce that in the future, curious if we should add a third parameter to this method 🤔to avoid birth of another public API in the future

public enum ClientType {	
	OIDC("openid-configuration"),
	OAUTH("oauth-authorization-server");

	private String issuerIdentifier;

	ClientType(String issuerIdentifier) {
		this.issuerIdentifier = issuerIdentifier;
	}

	public String getIssuerIdentifier() {
	        return this.issuerIdentifier;
        }
} 

Enum is my way of making the fromIssuer Generic, open to better suggestions (I suppose down the road when we allow custom url, then we could create a OTHER entry as well)

• Refactored the existing private String getConfiguration to dynamically pass ClientType and based on that decide whether to use openid-configuration or oauth-authorization-server as well as to make it reusable between existing api (fromOidcIssuerLocation) and new one (fromIssuer)

private static String getConfiguration(String issuer, ClientType type) {
	RestTemplate rest = new RestTemplate();
	try {
		return rest.getForObject(composeServerMetadataUrl(issuer, type, true), String.class);
	} catch(HttpClientErrorException ex) {
		 if(ex.getStatusCode() == HttpStatus.NOT_FOUND) {
			 return rest.getForObject(composeServerMetadataUrl(issuer, type, false), String.class);
		 } else {
			 throw new IllegalArgumentException();
		 }
	} catch(RuntimeException e) {
		throw new IllegalArgumentException("Unable to resolve the " + getTypeName(type) + " with the provided Issuer of \"" + issuer + "\"", e);
	}
}

• Further in order to support for the Compatibility with legacy application, I ported the hardcoded url from the getConfiguration to its own method by calling composeServerMetadataUrl(issuer, type, true) where the last argument states that first try url suggested by RF8414, if that resulted in 404 then try older version

• Lastly, I ensured that the existing unit tests are ✅ regardless of the refactoring as well as duplicated all the unit tests to user fromIssuer(String issuer, ClientType type) where type is OIDC and OAUTH and they all pass 👍 In order to test the Compatibility I have made use of Dispatcher of MockWebServer to simulate 404 on first attempt for /.well-known/openid-configuration/issuer1 and 200 OK for /issuer1/.well-known/openid-configuration and it works as per the specification.

If I understand correctly, the point of this issue is to come up with a public api that basically allows for server metadata data to be retrieved for both openid and oauth taking into account the compatibility. Is that right?

Sorry for so too much details, I might be suffering from too much details syndrome, if that's a thing 😆

Thanks.

[jzheaux]

@rhamedy You are correct that fromOidcIssuerLocation will remain. It's a nice optimization for users who specifically want to do OIDC-based discovery.

My thoughts on the method are that it would just be fromIssuer(String issuer) for now, leaving the ClientType out until the API has more time to bake. As you said, this might change if and when the API supports custom discovery endpoints. The semantics of fromIssuer(String issuer) would be to do discovery after the RFC 8414 fashion, leaving folks to call fromOidcIssuerLocation if they already know they are doing OIDC-based discovery. Do you see any problems with that?

The rest of what you are saying sounds reasonable, though it will probably become clearer once you submit a PR.

Sorry for so too much details, I might be suffering from too much details syndrome, if that's a thing

I think it's a good level of detail. :) Writing things down often helps the writer think things through as well. (That's the case with me, anyway.)

[rhamedy]

If I understand correctly, the new fromIssuer(String issuer) method will only do oauth2-authorization-server related request? I do not see a problem with that, except a slight confusion if the new api is used for the purposes of OIDC and I suppose we can clarify that with the help of some javadocs.

I will raise a PR next week! Thanks for clarifying my confusions 🙂

[jzheaux]

Correct, fromIssuer would only do oauth2-authorization-server, though we may add more sophisticated support later. There might be some value in calling it fromOAuth2IssuerLocation if you think that clarifies its usage.

This also means that fromOidcIssuerLocation would need an update in order to comply with Section 5.

[rhamedy]

I will go with fromOAuth2IssuerLocation option as it aligns well with fromOidcIssuerLocation and reduces the confusion. I will see how much of the existing code I can refactor to make it reusable by both 👍 taking into account the Section 5