Added Support for RFC 8414 OAuth 2.0 Authorization Server Metadata

[rhamedy]

Fixes: gh-6500

[rhamedy]

@jzheaux
As per our conversations. Please see the first version of my changes. A few things

• I have repurposed the usage of ClientType enum and I am open to alternative suggestions (if any)
• Refactored the existing code to come up with reusable utility methods
• The existing tests are not updated and runs ✅ ensuring that existing functionality is not broken
• Added a matching unit test for OAuth2 (matching with OpenID)
• Added tests using RequestDispatcher to simulate the compatibility section of RFC 8414

As you might have noticed, the refactored/utility methods in JwtDecoders.java and ClientRegistrations.java are very similar. Any suggestion on how we could avoid this duplication? Would be nice to port some of the common methods where it's accessible by both classes.

Lastly, this is not the final change and the javadocs/comments require some more work. Once we agree on a optimal solution then I will proceed with addressing the comments and anything else that's necessary 👍

[jzheaux]

Thanks, @rhamedy! I've left some feedback inline - specifically, I'd like to see if we can't reduce a lot of the branching logic here.

[rhamedy]

Hey @jzheaux

Thanks for the review. The branching logic did feel a little messy. I have made some changes (please see the recent commit)

• Removed the validateIssuer and validateIssuer methods
• Removed the ClientType enum
• Created getOpenIdConfiguration and getOAuth2Configuration methods that also contain logic of how to construct the url
• I was thinking of refactoring the parse method to something as shown at the bottom. For now, created dedicated methods.
• Refactored the getScopes method to make it reusable for both OpenId and OAuth by passing the Scope instead of Metadata

private static <T> T parse(String body, Class<T> clazz) {
		Assert.isTrue((clazz.equals(OIDCProviderMetadata.class) || clazz.equals(AuthorizationServerMetadata.class)),
				"clazz allowed types are OIDCProviderMetadata and AuthorizationServerMetadata.");
		try {
			return clazz.equals(OIDCProviderMetadata.class)
					? clazz.cast(OIDCProviderMetadata.parse(body))
					: clazz.cast(AuthorizationServerMetadata.parse(body));
		}
		catch (ParseException e) {
			throw new RuntimeException(e);
		}
	}

Note: Once we are 👍 with the implementation details, then I will do the same for JwtDecoders

[jzheaux]

@rhamedy, thanks, this cleaned up nicely!

I've left a bit more feedback inline.

[rhamedy]

@jzheaux pushed the recent changes to both ClientRegistrations and JwtDecoders. I have not touched the tests, please let me know if you have any ideas to make them more effective/useful. I also added some javadoc and once again, feel free to leave some comments.

Because of our refactoring, the error messages as mentioned above is changed. Not sure if this is going to be an issue in regards to how it affects the users of this feature 🤔

[jzheaux]

@rhamedy this looks great. I've left just a bit more feedback inline.

[jzheaux]

We should be careful about changing error messages, but these changes appear reasonable to me.

[rhamedy]

Hi @jzheaux, appreciate the feedback. There is always something new to learn.

I have pushed my recent changes. Please let me know if you there is more room for improvement. I have added some javadoc and once feel free to point out areas that are confusing and need further clarification 👍

[jzheaux]

@rwinch @jgrandja Because we initially discussed fromOidcIssuerLocation together to establish its name, and we are introducing an additional method of a similar nature, I'd like to get your feedback on this contract before proceeding further.

The contract this PR proposes is:

• fromOidcIssuerLocation - no contract changes here, but it will now include the compatibility measures recommended in RFC 8414
• fromOAuth2IssuerLocation - a new method that will query the standard endpoint outlined in RFC 8414 to obtain the same information

[rhamedy]

@jzheaux based on our recent conversation on gh-6500, I have updated the ClientRegistrations.java and ClientRegistrationsTest.java for now. Please let me know if you wish any changes to be made before I go ahead and update the JWTDecoder its unit test and update the javadocs.

[jzheaux]

Yes, @rhamedy, this looks about right. Thank you, again!

It looks like there may be some cleanup items that you will still do, regarding error handling, but the contracts look right, which is I think what you were asking for in this review.

[rhamedy]

Hi @jzheaux, I wanted to pick your brain with regards to unit testing. Now that we have decided to make the added contract fromIssuerLocation query all the endpoints (i.e. original oidc path, recommended oidc path by RFC 8414 and Oauth2, that's 3) then it means that for each existing test we need to add another 3 tests to test all the paths? Does that make sense?

For example, the following test was there from before

@Test
	public void issuerWhenResponseIsTypicalThenReturnedDecoderValidatesIssuer() {
		prepareConfigurationResponse();
		this.server.enqueue(new MockResponse().setBody(JWK_SET));

		JwtDecoder decoder = JwtDecoders.fromOidcIssuerLocation(this.issuer);

		assertThatCode(() -> decoder.decode(ISSUER_MISMATCH))
				.isInstanceOf(JwtValidationException.class)
				.hasMessageContaining("This iss claim is not equal to the configured issuer");
	}

So we keep the existing test as is since it's enforcing the existing fromOidcIssuerLocation contract. For the newly introduced contract fromIssuerLocation we need the following tests

1. Test the original oidc v1 (i.e. same as above test) ensuring that fromIssuerLocation can be used in place of fromOidcIssuerLocation
2. Test the recommended oidc by RFC 8414 i.e. /.well-known/openid-configuration/issuer
3. Test the oauth2 by RFC 8414

The 3 test cases would look as follow (refactored assertions) and using separate mock responses to simulate each case

        @Test
	public void issuerWhenOidcV1ResponseIsTypicalThenReturnedDecoderValidatesIssuer() {
		prepareConfigurationResponseForOidcV1("issuer1");
		JwtDecoder decoder = JwtDecoders.fromIssuerLocation(this.issuer);
		assertResponseIsTypicalThenReturnedDecoderValidatesIssuer(decoder);
	}

	@Test
	public void issuerWhenOidcResponseIsTypicalThenReturnedDecoderValidatesIssuer() {
		prepareConfigurationResponseForOidc("issuer1");
		JwtDecoder decoder = JwtDecoders.fromIssuerLocation(this.issuer);
		assertResponseIsTypicalThenReturnedDecoderValidatesIssuer(decoder);
	}

	@Test
	public void issuerWhenOauth2ResponseIsTypicalThenReturnedDecoderValidatesIssuer() {
		prepareConfigurationResponseForOauth2("issuer1");
		JwtDecoder decoder = JwtDecoders.fromIssuerLocation(this.issuer);
		assertResponseIsTypicalThenReturnedDecoderValidatesIssuer(decoder);
	}

	private void assertResponseIsTypicalThenReturnedDecoderValidatesIssuer(JwtDecoder decoder) {
		assertThatCode(() -> decoder.decode(ISSUER_MISMATCH))
		.isInstanceOf(JwtValidationException.class)
		.hasMessageContaining("This iss claim is not equal to the configured issuer");
	}

If you notice the first line of each of the above 3 test is different and that's because we would like the server to respond with 200 depending on which case we are testing otherwise respond with 404. Here is a sample

private void prepareConfigurationResponseForOauth2(String path) {
		this.issuer = this.server.url(path).toString();
		String body = String.format(DEFAULT_RESPONSE_TEMPLATE, this.issuer, this.issuer);

		final Dispatcher dispatcher = new Dispatcher() {
			@Override
			public MockResponse dispatch(RecordedRequest request) throws InterruptedException {
				switch(request.getPath()) {
					case "/.well-known/oauth-authorization-server/issuer1":
						return buildSuccessMockResponse(body);
					case "/issuer1/.well-known/jwks.json":
						return buildSuccessMockResponse(JWK_SET);
				}
				return new MockResponse().setResponseCode(404);
			}
		};
		this.server.setDispatcher(dispatcher);
	}

The switch case for prepareConfigurationResponseForOidc and prepareConfigurationResponseForOidcV1 are different.

We do the same for all of the existing tests for both JwtDecodersTest and ClientRegistrationsTest. Does this make sense? It kinda feels like a huge volume of test and I wanted to run it by you in case I have overlooked something and we do not have to do all that?

[jzheaux]

@rhamedy, it's a good question - addressing all permutations can get pretty hard to keep up with for really not that much benefit.

I think the critical logic to verify is

1. Does the endpoint resolution logic work?, e.g. does it correctly fall back, and
2. Does it correctly parse the response into a ClientRegistrations.Builder.

I think these can be tested independently for the most part - I wouldn't imagine there being 3x tests, it'd probably be one (or some) tests to verify the fallback logic and another set of tests confirming the parsing logic against the oauth2 endpoint.

[rhamedy]

Hi @jzheaux

Please see the recent changes. I have refactored the JwtDecoers, added unit tests as well as javadoc. I am sure there will be changes to be made, please feel free to let me know.

As far as ClientRegistrations is concerned, since we changed the scope of fromIssuerLocation to query all 3 endpoint, before we invoke withProviderConfiguration we need to know whether this is Oidc or Oauth2 and that is because we will have to include .userInfoUri(metadata.getUserInfoEndpointURI().toASCIIString()); and that means we have to either parse AuthorizationServerMetadata or OidcProviderMetadata depending on which endpoint responded successfully.
I have implemented a solution for now (my recent changes) but, feel free to purpose a different one comes in mind.

I have also added javadoc please feel free to point out any that need to be updated.
Thanks for these reviews/feedback btw.

[rhamedy]

Might have to change that message to paths cannot be null or empty.

[rhamedy]

The use of Map<ProviderType, String> adds quite a bit of branching logic that might make the code not easy to read, would be open to suggestions on ways if we could avoid the if-checks and still be able to make fromIssuerLocation query all three endpoints.

[jzheaux]

@rhamedy, this is now merged into master via f5b7706. Thanks for all your efforts on this, I think it turned out well.

I squashed your commits (which is why you see a different commit hash) and then simplifed some of the branching logic that you called out - see the polish commit of 1739ef8 for details.

[rhamedy]

Whoa! 😮 That's a massive polish! Thanks for that. This PR definitely went through some ups and downs and change of requirements but, at the end of the day like you said turned out pretty good and thanks to your polish commit, things looks much clearer.