Allow adding an AuthenticationProvider and AuthenticationConverter

[GrmpfNarf]

Expected Behavior IT would be nice if we just can add a single (or multiple) AuthenticationProviders/AuthenticationConverters and not override the default ones. Current Behavior Currently I'm just able to use the default ones OR custom ones but I cannot mix. When I want to use the default ones and just want to add a custom one I have to copy the initalisation of the default ones: Example:

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
        return customizer -> customizer
                .accessTokenRequestConverter(authenticationConverter())
                .authenticationProvider(resourceOwnerPasswordAuthenticationProvider())
                .authenticationProvider(oAuth2AuthorizationCodeAuthenticationProvider())
                .authenticationProvider(oAuth2ClientCredentialsAuthenticationProvider())
                .authenticationProvider(oAuth2RefreshTokenAuthenticationProvider());
    }

private AuthenticationConverter authenticationConverter() {
        return new DelegatingAuthenticationConverter(
                Arrays.asList(
                        new OAuth2AuthorizationCodeAuthenticationConverter(),
                        new OAuth2RefreshTokenAuthenticationConverter(),
                        new OAuth2ClientCredentialsAuthenticationConverter(),
                        new OAuth2ResourceOwnerPasswordAuthenticationConverter()
                )
        );
    }

The oauth2**AuthenticationProvider() methods are a copy of the content of the OAuth2TokenEndpointConfigurer.createDefaultAuthenticationProviders method.

Context I'm trying to add a Password grant type so I have to add a custom AuthenticationProvider and AuthenticationConverter.

[opcooc]

https://github.com/spring-projects/spring-authorization-server/pull/343

[sjohnr]

Hi @GrmpfNarf, thanks for the suggestion. I've thought about this a bit, and I feel that focusing on just changing the authenticationProvider(...) method would make other customizations harder. For example, if we changed it to "add" instead of "replace" providers, we would have the opposite problem, e.g. "How do I replace them with only my own custom one?" etc.

Instead, one idea is that we could provide focused helper methods in the configurer to explicitly add support for built-in grant types that the token endpoint can handle. For example:

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
    return customizer -> customizer
        .accessTokenRequestConverter(authenticationConverter())
        .authenticationProvider(resourceOwnerPasswordAuthenticationProvider())
        .authorizationCode()
        .clientCredentials()
        .refreshToken();
}

The last three would be defaults if custom providers are not specified, but specifying your own turns off the defaults (as it does currently), so you can easily re-enable them as above. Any thoughts on that idea?

[sjohnr]

Hi @GrmpfNarf, did you have a chance to review my comment above?

[GrmpfNarf]

Hi @GrmpfNarf, did you have a chance to review my comment above?

Hi @sjohnr. First sorry for the very late answer I just forgot and the project was frozen in my company. But now it is again under development. And yes this solution will be acceptable and it whould be nice when the methods

.authorizationCode()
.clientCredentials()
.refreshToken();

Will not only add the AuthenticationProvider but also the AuthenticationConverter, or there are similiar methods for the converter.

[sjohnr]

Thanks @GrmpfNarf for the feedback. Based on more recent discussion, a better idea for how to approach this is to add a Consumer-based method. For example:

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
    return tokenEndpoint -> tokenEndpoint
            .authenticationProviders(authenticationProviders ->
                        authenticationProviders.add(resourceOwnerPasswordAuthenticationProvider())
            );
}

This would give you access to the list of defaults from the configurer, and the ability add to or completely override the defaults.

This does not address the converters. However, I think the converters are not a problem, because they all have no-arg constructors, so it's fairly easy to add to the defaults as in your example from the description of this issue.

[GrmpfNarf]

Thanks @GrmpfNarf for the feedback. Based on more recent discussion, a better idea for how to approach this is to add a Consumer-based method. For example:

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
    return tokenEndpoint -> tokenEndpoint
            .authenticationProviders(authenticationProviders ->
                        authenticationProviders.add(resourceOwnerPasswordAuthenticationProvider())
            );
}

This would give you access to the list of defaults from the configurer, and the ability add to or completely override the defaults.

This does not address the converters. However, I think the converters are not a problem, because they all have no-arg constructors, so it's fairly easy to add to the defaults as in your example from the description of this issue.

I'm fine with this solution also. I can add the converter by my solution above, so having them addable by the same way like the providers is a nice to have. In my opinion it is more consitent by adding them the same way like the providers because without them the providers didn't work.

[ghost]

hi @jgrandja. I can start working on this issue.

[jacko9et]

Thanks @GrmpfNarf for the feedback. Based on more recent discussion, a better idea for how to approach this is to add a Consumer-based method. For example:

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
    return tokenEndpoint -> tokenEndpoint
            .authenticationProviders(authenticationProviders ->
                        authenticationProviders.add(resourceOwnerPasswordAuthenticationProvider())
            );
}

This would give you access to the list of defaults from the configurer, and the ability add to or completely override the defaults.

This does not address the converters. However, I think the converters are not a problem, because they all have no-arg constructors, so it's fairly easy to add to the defaults as in your example from the description of this issue.

I think adding an authentication method requires setting two parameters at the same time, Converter and Provider, because they come in pairs, and setting the Provider alone may cause other problems.

public interface AuthenticationMethodProvider {
    void add(AuthenticationConverter converter, AuthenticationProvider provider);
}

private Customizer<OAuth2TokenEndpointConfigurer> customizeTokenEndpoint() {
    return tokenEndpoint -> tokenEndpoint
            .authenticationMethodProviders(
                    authenticationMethodProviders.add(ownerAuthenticationConverter, ownerAuthenticationProvider())
            );
}

[bob-walters]

Hi, I noticed that this issue has not seen work lately, and may have stalled. I had the same need and had originally opened https://github.com/spring-projects/spring-authorization-server/issues/702, which was a duplicate given this issue. I would be happy to work on this over the next week and submit a PR, per the recommended implementation stated by @lu-cheng . Let me know if that would help / be welcome.

[bob-walters]

@jgrandja - wanted to draw your attention to the above offer. I have read https://github.com/spring-projects/spring-authorization-server/blob/main/CONTRIBUTING.adoc and signed the individual contributor agreement. I can pick this up if nobody is currently working it.

[bob-walters]

@ovidiupopa91 - same offer as above. (I see this assigned to you, but assume you have other priorities currently.) Plan was to follow @lu-cheng 's design

[jgrandja]

@bob-walters Thanks for getting in touch. We're planning on a few improvements to the configuration model including this ticket. However, the design will likely be different than what is discussed in this ticket. I'm planning on circling back to this soon and will touch base at that point.