Simplify adding an authentication validator

[dlehammer]

Hi spring-authorization-server gurus,

I'm probably holding it wrong, but here goes :)

Expected Behavior

Appending additional validation supplementing the default validation should be a 1st class citizen, preferably via the composite pattern or similar abstraction.
I suggest that appending custom validation should be as easy as invoking a add-method during configuration.

Current Behavior

The documentation outlines a way to override some default validation, while simultaneously being rather verbose.

SNIP..

private Consumer<List<AuthenticationProvider>> configureAuthenticationValidator() {
	return (authenticationProviders) ->
		authenticationProviders.forEach((authenticationProvider) -> {
			if (authenticationProvider instanceof OAuth2AuthorizationCodeRequestAuthenticationProvider) {
				Consumer<OAuth2AuthorizationCodeRequestAuthenticationContext> authenticationValidator =
					// Override default redirect_uri validator
					new CustomRedirectUriValidator()
						// Reuse default scope validator
						.andThen(OAuth2AuthorizationCodeRequestAuthenticationValidator.DEFAULT_SCOPE_VALIDATOR);

				((OAuth2AuthorizationCodeRequestAuthenticationProvider) authenticationProvider)
					.setAuthenticationValidator(authenticationValidator);
			}
		});
}

SNIP..

While the current support as outlined in the documentation is cumbersome and maintainer unfriendly, it's sufficient for overriding some default validation, it's also brittle as the overrider must explicit maintain the validation chain ( ...andThen(.. ) going forward.

Context

How has this issue affected you?

In-order to append custom validation, I've accumulated an undesired responsibility for maintaining a mirror of the default validation-chain.

What are you trying to accomplish?

Append custom validation, while preserving the default validation as-is.

What other alternatives have you considered?

The OAuth2AuthorizationEndpointConfigurer.addAuthorizationCodeRequestAuthenticationValidator(..), unfortunately it's unreachable outside the package and prepends the validator.

Are you aware of any workarounds?

Unfortunately no :/

[jgrandja]

@dlehammer

Appending additional validation supplementing the default validation should be a 1st class citizen

We don't want to expose OAuth2AuthorizationEndpointConfigurer.addAuthenticationValidator() at this point for the following reasons:

1. Not all AuthenticationProvider's have an associated "authentication validator".
2. OAuth2AuthorizationEndpointConfigurer initializes 2x AuthenticationProvider's and OAuth2TokenEndpointConfigurer initializes 3x AuthenticationProvider's, so we would need a generic API that would allow setting (or adding) an authentication validator to a specific AuthenticationProvider.
3. Some AuthenticationProvider's have additional setters for overriding behaviour, for example, OAuth2AuthorizationCodeRequestAuthenticationProvider.setAuthorizationCodeGenerator() and it doesn't make sense to expose this in OAuth2AuthorizationEndpointConfigurer since this specific configuration is an implementation detail and not a generic API that can be applied across the various OAuth2*EndpointConfigurer's.

The documentation outlines a way to override some default validation, while simultaneously being rather verbose.

I wouldn't qualify it as verbose as configureAuthenticationValidator() is a minimal custom configuration IMO.

it's also brittle as the overrider must explicit maintain the validation chain ( ...andThen(.. ) going forward.

andThen() is only necessary is you want to preserve the defaults, however, some applications may want to completely override the defaults and therefore andThen() would not be required. The flexibility is there and it's really up to the application requirements.

FYI, the general API OAuth2*EndpointConfigurer.authenticationProviders() provides the flexibility to customize any AuthenticationProvider that is initialized by an OAuth2*EndpointConfigurer. I understand you would prefer a finer grained API but I'm not sure how we would address that from a design standpoint given the reasons as explained above.

For now, I'll leave this issue open and we may address this at a later point.

[dlehammer]

Hi @jgrandja,

Thanks for taking the time to reply.

Perhaps an initial compromise could be access to the default validation chain currently defined by OAuth2AuthorizationCodeRequestAuthenticationValidator#authenticationValidator.

So appending instead would look something like

OAuth2AuthorizationCodeRequestAuthenticationValidator
    .authenticationValidator
        .andThen(new MyAdditionalValidator());

That way it would be possible to utilize the default validation as-is while it evolves over time, eliminating the need for users of additional validation to manually attempt to remember to check/determine if a new version of the spring-authorization-server warrants the need for changes in a particular code-base 🤓

[jgrandja]

@dlehammer I'm not sure I want to expose OAuth2AuthorizationCodeRequestAuthenticationValidator.authenticationValidator but I like the idea of containing the enhancement to this class to help users customize the authentication validator while preserving the defaults.

I think a static factory method could work, where the provided validator(s) are simply appended to the defaults.

Would you be interested in submitting a PR for this?

[dlehammer]

Hi @jgrandja,

Thanks for taking the time to reply.

Regarding

Would you be interested in submitting a PR for this?

I've thought about it, and the only thing preventing me is that I'm unsure what would be an acceptable approach.
So if it's still relevant I would be grateful if you could point me towards an "static factory method" spring code example.

Also i noticed #884 that seems like a closely related issue :)

[jgrandja]

@dlehammer We just released 1.1 and will soon be planning 1.2, which could potentially include this enhancement. Please give me a couple of weeks until we finalize our 1.2 plan and we can go from there. Thanks.

[Dogacel]

Hi, @jgrandja @dlehammer are there any updates to this issue? I am simply trying to support wildcard supports such that I will grant read/* to a client and it can automatically assume all scopes read/foo, read/bar etc.

I figured out how to do it for client credentials grant,

    @Bean
    fun clientAuthenticationProvider(
        authorizationService: OAuth2AuthorizationService,
        tokenGenerator: OAuth2TokenGenerator<*>
    ): OAuth2ClientCredentialsAuthenticationProvider {
        val authenticationProvider = OAuth2ClientCredentialsAuthenticationProvider(
            authorizationService,
            tokenGenerator,
        )

        authenticationProvider.setAuthenticationValidator { context ->
            val registeredClient = context.registeredClient
            val authorizedClient: OAuth2ClientCredentialsAuthenticationToken = context.getAuthentication()

            val regexifiedScopes = registeredClient.scopes.map {
                it.toRegex()
            }

            val unauthorized = authorizedClient.scopes.all { authorizedScope ->
                regexifiedScopes.any { it.matches(authorizedScope) }
            }.not()

            if (unauthorized) {
                throw OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_SCOPE)
            }
        }

        return authenticationProvider
    }

And later on,

        http
            .getConfigurer(OAuth2AuthorizationServerConfigurer::class.java)
            .oidc(Customizer.withDefaults())
            .clientAuthentication { clientAuthentication ->
                clientAuthentication.authenticationProvider(clientAuthenticationProvider)
            }

However, it seemed pretty though to just go ahead and update every one of the authentication flows like so one by one. Maybe because I am currently on exploration stage of spring security, it seems complex however it might be just regular configuration that we need to do anyway before the production release.

However, I wonder, if it is possible to do this by simply providing beans to Consumer<OAuth2ClientCredentialsAuthenticationContext> type?

So we can directly inject our own authentication validators without needing to override other stuff like I have done,

    @Bean
    fun authorizationService(): OAuth2AuthorizationService {
        return InMemoryOAuth2AuthorizationService()
    }


    @Bean
    fun tokenGenerator(jwtEncoder: JwtEncoder): OAuth2TokenGenerator<*> {
        return DelegatingOAuth2TokenGenerator(
            JwtGenerator(jwtEncoder),
            OAuth2AccessTokenGenerator(),
            OAuth2RefreshTokenGenerator(),
        )
    }

and this includes custom jwtEncoder etc.

[dlehammer]

Hi @Dogacel ,

Regarding

are there any updates to this issue?

Not that I'm aware of, ie.
OAuth2AuthorizationCodeRequestAuthenticationValidator#authenticationValidator is still unreachable as discussed earlier.

However, it seemed pretty though to just go ahead and update every one of the authentication flows like so one by one.

That's by design, in-order to ensure predictable default behaviour and readability.

Regarding your .. authenticationProvider.setAuthenticationValidator .. example, it seems sensible.
My only suggestion is encapsulation of your implementation in a class to allow for reuse in your desired flows 🤓

[OrangeDog]

A major issue I'm finding with the validation is that the error handling is baked into OAuth2AuthorizationCodeRequestAuthenticationValidator's private static methods. There's quite a lot of work there in order to return the error safely and correctly to the client/user.

IMO, that concern is entirely separate to actual validation, which should only need to check for errors.
Any custom validation has to duplicate all that code otherwise the exceptions it throws will not be handled correctly.

Surely that code should be moved out to a higher/later level (e.g. a failure handler)?
At the very least it should be public so custom validators can also call it.