Add support to ClientRegistrations for using a metadata resolver

[sjohnr]

Issue gh-8882

[jgrandja]

Thanks @sjohnr ! This is looking good to me. I left a couple of minor feedback.

Let's also gather feedback from @jzheaux

[jzheaux]

Thanks, @sjohnr! I've left some feedback inline.

[jzheaux]

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.

[jgrandja]

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?

[jzheaux]

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.

[jzheaux]

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.

[jgrandja]

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

[jgrandja]

Let's consider metadataRetriever instead of metadataResolver

[sjohnr]

After discussion, it has been determined that we won't be able to fully resolve the issue of allowing a custom RestOperations when using Spring Boot's configuration properties without changes in Boot itself. Further, the better option is to introduce a discovery client API. See this branch for a possible starting point. This will be revisited after the 5.8/6.0 release.