Introduce customizer for SpringOpaqueTokenIntrospector

[quaff]

I want to customize authenticationConverter of SpringOpaqueTokenIntrospector, currently I need to redefine my own bean to back off Spring Boot's auto-configured one, it would be nice if I can customize the auto-configured one.

I'd like to contribute if the proposal is accepted.

spring-boot/module/spring-boot-security-oauth2-resource-server/src/main/java/org/springframework/boot/security/oauth2/server/resource/autoconfigure/servlet/OAuth2ResourceServerOpaqueTokenConfiguration.java

Lines 50 to 62 in 450eb48

	SpringOpaqueTokenIntrospector opaqueTokenIntrospector(OAuth2ResourceServerProperties properties) {
	OAuth2ResourceServerProperties.Opaquetoken opaquetoken = properties.getOpaquetoken();
	String introspectionUri = opaquetoken.getIntrospectionUri();
	Assert.state(introspectionUri != null, "'introspectionUri' must not be null");
	String clientId = opaquetoken.getClientId();
	Assert.state(clientId != null, "'clientId' must not be null");
	String clientSecret = opaquetoken.getClientSecret();
	Assert.state(clientSecret != null, "'clientSecret' must not be null");
	return SpringOpaqueTokenIntrospector.withIntrospectionUri(introspectionUri)
	.clientId(clientId)
	.clientSecret(clientSecret)
	.build();
	}

[wilkinsona]

I wonder why it's a setter on the introspector itself rather than a method on the builder. If it were to move, perhaps with a period of being supported in both places for backwards compatibility, we may want to provide a customizer for SpringReactiveOpaqueTokenIntrospector.Builder and SpringOpaqueTokenIntrospector.Builder rather than the introspectors that they build.

@jzheaux is there any chance that the authentication converter configuration would move to the builders and the introspectors would become immutable?

[philwebb]

@jzheaux Any thoughts on this?

[jzheaux]

There aren't any plans to move it from being a setter. Generally, Spring Security beans are designed with optional parameters as setters and required parameters in the constructor. If possible, I'd prefer to not alter that convention here.

One thing that may help is adding a postProcess method that operates on the constructed object. Something like postProcess(Consumer<SpringOpaqueTokenIntrospector>) which would allow the setter to be called by a customizer.

What would you think of the following:

@Bean 
SpringOpaqueTokenIntrospectorBuilderCustomizer customizer() {
    return (builder) -> builder.postProcessor((introspector) -> introspector
        .setAuthenticationConverter(myConverter)
    );
}

This would allow us to still consider moving the setter at another time should it prove necessary.

[wilkinsona]

Thanks, @jzheaux.

Generally, Spring Security beans are designed with optional parameters as setters and required parameters in the constructor. If possible, I'd prefer to not alter that convention here.

I wonder if you'd reconsider this as it feels inconsistent with the rest of the portfolio. For example, Framework's WebClient.Builder and Spring Data Neo4's Neo4jClient.Builder have many methods for optional settings. There are several other examples, these are just the first two that I stumbled upon.

I think this approach aids discoverability and makes the API easier to use. You only have one place to look when trying to find a method to configure something. That's particularly important when using auto-complete in an IDE as you don't have to modify your code to go back and forth between two different locations.

[jzheaux]

It's certainly worth considering. It's a change that initially seems larger to me than this ticket itself; NimbusJwtDecoder, AllRequiredFactorsAuthorizationManager and DefaultAuthorizationManagerFactory are all examples of a common pattern that Security uses to simplify constructors.

That said, perhaps it would be reasonable to add an authenticationConverter method to the builder with build ultimately calling the setter method, at least for now.

Could you help me understand if that is necessary to facilitate Boot's efforts, that is, in comparison to the customize method I described? It seems to me that the Boot API change would be the same regardless. I don't mean to state a preference one way or the other; I just want to make sure I'm not missing something.

[wilkinsona]

It's certainly worth considering. It's a change that initially seems larger to me than this ticket itself; NimbusJwtDecoder, AllRequiredFactorsAuthorizationManager and DefaultAuthorizationManagerFactory are all examples of a common pattern that Security uses to simplify constructors.

I'm mostly interested in the consistency of Builder APIs that are themselves a way to simplify constructors. As far as I can see, DefaultAuthorizationManagerFactory doesn't have a builder so it could be ignored for now.

I'd welcome a method on AllRequiredFactorsAuthorizationManager.Builder for configuring the Clock so that the Builder provides a complete API for creating a fully configured AllRequiredFactorsAuthorizationManager instance.

Could you help me understand if that is necessary to facilitate Boot's efforts, that is, in comparison to the customize method I described?

In terms of making the customization possible, I don't think it makes a difference if it's a postProcessor method or an authenticationConverter method that's added to the builder. However, in terms of consistency in builder APIs across the portfolio, I'm very much in favor of the latter over the former. In addition to improving consistency, it also makes things more discoverable as I described above.

[jzheaux]

Sounds good, @wilkinsona, I've created spring-projects/spring-security#18623 so it can get prioritized