Skip to content

Add support to ClientRegistrations for using a metadata resolver #11321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add support to ClientRegistrations for using a metadata resolver #11321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ea863
Add support to ClientRegistrations for using a metadata resolver
Jun 1, 2022
3c8a21f
Polish ClientRegistrations
Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 171 additions & 8 deletions ...ain/java/org/springframework/security/oauth2/client/registration/ClientRegistrations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -21,6 +21,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Supplier;

import com.nimbusds.oauth2.sdk.ParseException;
Expand Down Expand Up @@ -48,6 +49,7 @@
* @author Rob Winch
* @author Josh Cummings
* @author Rafiullah Hamedy
* @author Steve Riesenberg
* @since 5.1
*/
public final class ClientRegistrations {
Expand All @@ -61,6 +63,11 @@ public final class ClientRegistrations {
private static final ParameterizedTypeReference<Map<String, Object>> typeReference = new ParameterizedTypeReference<Map<String, Object>>() {
};

private static final Function<URI, Map<String, Object>> DEFAULT_METADATA_RESOLVER = (metadataUri) -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, "resolver" is a conventional word in Spring Security that is reserved for constructing an object based on the HttpServletRequest. There have been exceptions, however I'd personally like that we try and be more consistent with this convention. @rwinch and @jgrandja are free to correct me here as I'm drawing from a conversation that the three of us had a couple of years ago and maybe I'm remembering it incorrectly.

It may make more sense to call this DEFAULT_METADATA_CONVERTER and use Spring's Converter class. Another possibility is to call it DEFAULT_METADATA_FETCHER or similar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll consider changing the name if it makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider metadataRetriever instead of metadataResolver

RequestEntity<Void> request = RequestEntity.get(metadataUri).build();
return rest.exchange(request, typeReference).getBody();
};

private ClientRegistrations() {
}

Expand Down Expand Up @@ -100,6 +107,66 @@ public static ClientRegistration.Builder fromOidcIssuerLocation(String issuer) {
return getBuilder(issuer, oidc(URI.create(issuer)));
}

/**
* Creates a {@link ClientRegistration.Builder} using the provided <a href=
* "https://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
* and {@link Function metadata resolver} to obtain the values used to initialize the
* {@link ClientRegistration.Builder}.
*
* <p>
* This method differs from {@link #fromOidcIssuerLocation(String)} in that the
* <a href=
* "https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse">OpenID
* Provider Configuration Response</a> is provided by the metadata resolver, which
* allows flexibility in determining how the metadata is obtained. For example, you
* can use any HTTP client configured with user-defined proxy settings, obtain
* metadata from a file on the filesystem, etc.
* </p>
*
* <p>
* The default metadata resolver used by {@link #fromOidcIssuerLocation(String)} uses
* a {@code RestTemplate} to make an <a href=
* "https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest">OpenID
* Provider Configuration Request</a> to the metadata endpoint. For example, if the
* issuer provided is "https://example.com", then an "OpenID Provider Configuration
* Request" will be made to "https://example.com/.well-known/openid-configuration".
* The result is expected to be an "OpenID Provider Configuration Response". The
* default metadata resolver is as follows:
* </p>
* <pre>
* RestTemplate rest = new RestTemplate();
* ParameterizedTypeReference&lt;Map&lt;String, Object&gt;&gt; typeReference = new ParameterizedTypeReference&lt;Map&lt;String, Object&gt;&gt;() {
* };
* Function&lt;URI, Map&lt;String, Object&gt;&gt; metadataResolver = (metadataUri) -> {
* RequestEntity&lt;Void&gt; request = RequestEntity.get(metadataUri).build();
* return rest.exchange(request, typeReference).getBody();
* };
* </pre>
*
* <p>
* Example usage:
* </p>
* <pre>
* ClientRegistration registration =
* ClientRegistrations.fromOidcIssuerMetadata("https://example.com", metadataResolver)
* .clientId("client-id")
* .clientSecret("client-secret")
* .build();
* </pre>
* @param issuer the <a href=
* "https://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
* @param metadataResolver the {@code Function} used to resolve metadata in an
* application-specific way
* @return a {@link ClientRegistration.Builder} that was initialized by the OpenID
* Provider Configuration.
* @since 5.8
*/
public static ClientRegistration.Builder fromOidcIssuerMetadata(String issuer,
Function<URI, Map<String, Object>> metadataResolver) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer a new non-static component. What happens when there is another behavioral aspect that someone wants to customize? It seems to me that optional configuration like this is meant for setters, not constructors/static factory methods.

It also feels a little odd passing behavior as a parameter. Consider, hypothetically, introducing a method that takes a parameters object ClientRegistrationsParameters, and it holds the issuer and the metadata resolver. To me, this feels strange to place behavior in the parameters object and, by association the parameter list.

This is just my preference, though, and you may choose to disregard it. If you want to address this comment, here are a couple of ways that come to mind:

  1. Embed the behavior into the builder that is returned. The caller would need to perform a cast for backward compatibility, but one could imagine returning an IssuerClientRegistrationBuilder similar to how NimbusJwtDecoder's builders are written. The cast could be removed in Spring Security 6.
  2. Create a new and separate component that is not static and is naturally configurable with setters. ClientRegistrations could be changed to call this with its static RestTemplate. Folks wanting to configure the rest template would use the new component.

I realize that the team may have differing opinions about 1 and whether behavior should go into a builder, but for completeness, I'm listing it as an option that I personally like. I also realize 2 has been discussed in the past and rejected as not needed. That said, the request to customize the resolution strategy has come up so often that I think there's value in revisiting that decision.

One nice thing about these solutions is that they allow the configuration to be more conservative if that's desired. That is, it seems like the contract is as wide as it is because we are trying to make sure that we don't need to overload the method once again down the road. It would be nicer to have a method called setRestOperations since that matches the rest of Spring Security.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @jzheaux.

I see your point regarding...

It also feels a little odd passing behavior as a parameter

I agree that in certain scenarios it doesn't make sense from a design standpoint, however, there are exceptions to this principle. RestTemplate.execute() accepts RequestCallback and ResponseExtractor and Reactive streams accept Function, BiFunction, etc. that all provide behavioural logic.

Regardless, your points have swayed me away from the overloaded method strategy and it feels more that the behaviour should be replaced as a whole within the utility component by exposing a setter. So I'm proposing we expose a static setter instead of the overloaded methods:

private static final Function<URI, Map<String, Object>> DEFAULT_METADATA_RESOLVER = (metadataUri) -> {
	RequestEntity<Void> request = RequestEntity.get(metadataUri).build();
	return rest.exchange(request, typeReference).getBody();
};

private static Function<URI, Map<String, Object>> metadataResolver = DEFAULT_METADATA_RESOLVER;

public static void setMetadataResolver(Function<URI, Map<String, Object>> metadataResolver) {
	ClientRegistrations.metadataResolver = metadataResolver;
}

With this design, the consuming application would need to call setMetadataResolver() in a static initializing block in order to change the behaviour before ClientRegistrations.fromIssuerLocation() is called from another component, e.g. Spring Boot's OAuth2ClientPropertiesRegistrationAdapter.

I realize this may not be the "cleanest" design but it is by far the most simple compared to creating a new component or exposing a new builder. This simple design can address the Spring Boot issues that arise when the issuer-uri property is used and there are firewall/proxy related issues.

What are your thoughts on this approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue that might come up with this approach is integrating with Spring Boot. In the past, we've run into issues when trying to configure SecurityContextHolder#setContextHolderStrategy. As of right now, there are no plans (I suppose) to have Spring Boot apply a RestTemplate instance to ClientRegistrations#setMetadataResolver; however, I think we'd see the same resistance to the idea should that come up.

Also related to Boot's concern about configuring SecurityContextHolder, introducing static configurability to ClientRegistrations means that it can't be configured differently for multiple application contexts.

a static initializing block

My concern here is that it would deviate from the general guidance we've given so far about RestTemplate configuration being @Bean-based. In an initializing block, there would be no opportunity to pick up a Spring-managed RestTemplate instance and apply it.

If there is just one application context, the code can refer to ClientRegistrations#setMetadataResolver in an @Autowired-annotated method instead of being in a static initializing block.

setMetadataResolver

I don't have a strong opinion here, but my preference is setRestOperations since that's what is exposed in most other places in Spring Security. I understand that there may be additional use cases this PR is trying to address, though.

Assert.hasText(issuer, "issuer cannot be empty");
return getBuilder(issuer, oidc(URI.create(issuer), metadataResolver));
}

/**
* Creates a {@link ClientRegistration.Builder} using the provided <a href=
* "https://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
Expand Down Expand Up @@ -144,15 +211,100 @@ public static ClientRegistration.Builder fromIssuerLocation(String issuer) {
return getBuilder(issuer, oidc(uri), oidcRfc8414(uri), oauth(uri));
}

/**
* Creates a {@link ClientRegistration.Builder} using the provided <a href=
* "https://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
* and {@link Function metadata resolver} by querying three different discovery
* endpoints serially, using the values in the first successful response to
* initialize. If an endpoint returns anything other than a 200 or a 4xx, the method
* will exit without attempting subsequent endpoints.
*
* The three endpoints are computed as follows, given that the {@code issuer} is
* composed of a {@code host} and a {@code path}:
*
* <ol>
* <li>{@code host/.well-known/openid-configuration/path}, as defined in
* <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414's Compatibility
* Notes</a>.</li>
* <li>{@code issuer/.well-known/openid-configuration}, as defined in <a href=
* "https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest">
* OpenID Provider Configuration</a>.</li>
* <li>{@code host/.well-known/oauth-authorization-server/path}, as defined in
* <a href="https://tools.ietf.org/html/rfc8414#section-3.1">Authorization Server
* Metadata Request</a>.</li>
* </ol>
*
* Note that the second endpoint is the equivalent of calling
* {@link ClientRegistrations#fromOidcIssuerLocation(String)}.
*
* <p>
* This method differs from {@link ClientRegistrations#fromIssuerLocation(String)} in
* that the <a href=
* "https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse">OpenID
* Provider Configuration Response</a> or
* <a href="https://datatracker.ietf.org/doc/html/rfc8414#section-3.2">Authorization
* Server Metadata Response</a> is provided by the metadata resolver, which allows
* flexibility in determining how the metadata is obtained. For example, you can use
* any HTTP client configured with user-defined proxy settings, obtain metadata from a
* file on the filesystem, etc.
* </p>
*
* <p>
* The default metadata resolver used by {@link #fromIssuerLocation(String)} uses a
* {@code RestTemplate} to make an "OpenID Provider Configuration Request" or
* "Authorization Server Metadata Request" to the metadata endpoint. The default
* metadata resolver is as follows:
* </p>
* <pre>
* RestTemplate rest = new RestTemplate();
* ParameterizedTypeReference&lt;Map&lt;String, Object&gt;&gt; typeReference = new ParameterizedTypeReference&lt;Map&lt;String, Object&gt;&gt;() {
* };
* Function&lt;URI, Map&lt;String, Object&gt;&gt; metadataResolver = (metadataUri) -> {
* RequestEntity&lt;Void&gt; request = RequestEntity.get(metadataUri).build();
* return rest.exchange(request, typeReference).getBody();
* };
* </pre>
*
* <p>
* Example usage:
* </p>
* <pre>
* ClientRegistration registration =
* ClientRegistrations.fromIssuerMetadata("https://example.com", metadataResolver)
* .clientId("client-id")
* .clientSecret("client-secret")
* .build();
* </pre>
* @param issuer the <a href=
* "https://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
* @param metadataResolver the {@code Function} used to resolve metadata in an
* application-specific way
* @return a {@link ClientRegistration.Builder} that was initialized by one of the
* described endpoints
* @since 5.8
*/
public static ClientRegistration.Builder fromIssuerMetadata(String issuer,
Function<URI, Map<String, Object>> metadataResolver) {
Assert.hasText(issuer, "issuer cannot be empty");
URI uri = URI.create(issuer);
return getBuilder(issuer, oidc(uri, metadataResolver), oidcRfc8414(uri, metadataResolver),
oauth(uri, metadataResolver));
}

private static Supplier<ClientRegistration.Builder> oidc(URI issuer) {
return oidc(issuer, DEFAULT_METADATA_RESOLVER);
}

private static Supplier<ClientRegistration.Builder> oidc(URI issuer,
Function<URI, Map<String, Object>> metadataResolver) {
// @formatter:off
URI uri = UriComponentsBuilder.fromUri(issuer)
.replacePath(issuer.getPath() + OIDC_METADATA_PATH)
.build(Collections.emptyMap());
// @formatter:on
return () -> {
RequestEntity<Void> request = RequestEntity.get(uri).build();
Map<String, Object> configuration = rest.exchange(request, typeReference).getBody();
Map<String, Object> configuration = metadataResolver.apply(uri);

OIDCProviderMetadata metadata = parse(configuration, OIDCProviderMetadata::parse);
ClientRegistration.Builder builder = withProviderConfiguration(metadata, issuer.toASCIIString())
.jwkSetUri(metadata.getJWKSetURI().toASCIIString());
Expand All @@ -164,27 +316,38 @@ private static Supplier<ClientRegistration.Builder> oidc(URI issuer) {
}

private static Supplier<ClientRegistration.Builder> oidcRfc8414(URI issuer) {
return oidcRfc8414(issuer, DEFAULT_METADATA_RESOLVER);
}

private static Supplier<ClientRegistration.Builder> oidcRfc8414(URI issuer,
Function<URI, Map<String, Object>> metadataResolver) {
// @formatter:off
URI uri = UriComponentsBuilder.fromUri(issuer)
.replacePath(OIDC_METADATA_PATH + issuer.getPath())
.build(Collections.emptyMap());
// @formatter:on
return getRfc8414Builder(issuer, uri);
return getRfc8414Builder(issuer, uri, metadataResolver);
}

private static Supplier<ClientRegistration.Builder> oauth(URI issuer) {
return oauth(issuer, DEFAULT_METADATA_RESOLVER);
}

private static Supplier<ClientRegistration.Builder> oauth(URI issuer,
Function<URI, Map<String, Object>> metadataResolver) {
// @formatter:off
URI uri = UriComponentsBuilder.fromUri(issuer)
.replacePath(OAUTH_METADATA_PATH + issuer.getPath())
.build(Collections.emptyMap());
// @formatter:on
return getRfc8414Builder(issuer, uri);
return getRfc8414Builder(issuer, uri, metadataResolver);
}

private static Supplier<ClientRegistration.Builder> getRfc8414Builder(URI issuer, URI uri) {
private static Supplier<ClientRegistration.Builder> getRfc8414Builder(URI issuer, URI uri,
Function<URI, Map<String, Object>> metadataResolver) {
return () -> {
RequestEntity<Void> request = RequestEntity.get(uri).build();
Map<String, Object> configuration = rest.exchange(request, typeReference).getBody();
Map<String, Object> configuration = metadataResolver.apply(uri);

AuthorizationServerMetadata metadata = parse(configuration, AuthorizationServerMetadata::parse);
ClientRegistration.Builder builder = withProviderConfiguration(metadata, issuer.toASCIIString());
URI jwkSetUri = metadata.getJWKSetURI();
Expand Down
30 changes: 29 additions & 1 deletion ...ava/org/springframework/security/oauth2/client/registration/ClientRegistrationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -454,6 +454,34 @@ public void issuerWhenOAuth2ConfigurationDoesNotMatchThenMeaningfulErrorMessage(
// @formatter:on
}

@Test
public void fromOidcIssuerMetadataWhenAllInformationThenSuccess() {
this.issuer = "https://example.com";
// @formatter:off
ClientRegistration registration =
ClientRegistrations.fromOidcIssuerMetadata(this.issuer, (uri) -> this.response)
.clientId("client-id")
.clientSecret("client-secret")
.build();
// @formatter:on
ClientRegistration.ProviderDetails provider = registration.getProviderDetails();
assertIssuerMetadata(registration, provider);
}

@Test
public void fromIssuerMetadataWhenAllInformationThenSuccess() {
this.issuer = "https://example.com";
// @formatter:off
ClientRegistration registration =
ClientRegistrations.fromIssuerMetadata(this.issuer, (uri) -> this.response)
.clientId("client-id")
.clientSecret("client-secret")
.build();
// @formatter:on
ClientRegistration.ProviderDetails provider = registration.getProviderDetails();
assertIssuerMetadata(registration, provider);
}

private ClientRegistration.Builder registration(String path) throws Exception {
this.issuer = createIssuerFromServer(path);
this.response.put("issuer", this.issuer);
Expand Down