Spring Security Allow AbstractWebClientReactiveOAuth2AccessTokenResponseClient to be extended

Expected Behavior Be able to extend the AbstractWebClientReactiveOAuth2AccessTokenResponseClient for custom AuthorizationGrantType implementations not just the four default ones implemented in the spring security framework.

Current Behavior

The current implementation of the oauth2 AbstractWebClientReactiveOAuth2AccessTokenResponseClient uses the type T extends AbstractOAuth2AuthorizationGrantRequest which implies the ability to extend the AbstractOAuth2AuthorizationGrantRequest and extend the AbstractWebClientReactiveOAuth2AccessTokenResponseClient to implement a custom authorization grant. The AbstractWebClientReactiveOAuth2AccessTokenResponseClient however has a package private constructor which restricts the ability to extend the mentioned class unless one puts the class in the org.springframework.security.oauth2.client.endpoint in their codebase.

Context The OAuth2 spec allows for custom implementations of the OAuth2 grants by defining a grant type as specified in OAuth2 RFC 6749 Section 4.5: Extension Grants.

Current implementation of the AuthorizationGrantType allows for custom grant types to be defined and the extension of AbstractOAuth2AuthorizationGrantRequest allows that as well.

However the inability to extend the AbstractWebClientReactiveOAuth2AccessTokenResponseClient leaves one with only one choice is to either duplicate the implementation in the afformentioned class, or write ones own implementation. Which is quite annoying when the base is already present in the framework code.

The request to allow for extension of this class has be done before i.e. https://github.com/spring-projects/spring-security/issues/10836 but with a failed mention to provide for customisation it was declined. However there was no mention or thought of custom grant type support.

It would be great to be able to create a custom extension of the said class in our own package structure rather than having to either reimplement the internals of the AbstractWebClientReactiveOAuth2AccessTokenResponseClient or place the new client into the org.springframework.security.oauth2.client.endpoint package.

Comment From: sjohnr

Hi @kkondratov, thanks for reaching out!

I have reviewed the existing implementation(s) and agree that there is quite a bit of boilerplate code required to implement the ReactiveOAuth2AccessTokenResponseClient interface. Being able to extend AbstractWebClientReactiveOAuth2AccessTokenResponseClient is one possible solution.

However, I don't believe the abstract class was ever intended to be made public, and may have only been made public due to unavoidable circumstances during refactoring that I don't have full context on. The constructor remaining package-private seems to confirm that this was the intention. Further, reviewing the class I can tell you that I would not prefer to open up this class to extension, as it will continue to be beneficial to keep this class internal to the framework and not allow users to extend it. (I understand that users can circumvent this by using the package-collision workaround but this is clearly not intended either.)

Having said that, the existing abstract class is already suitable for a generic implementation, except that it is not possible to construct a plain instance and customize the headersConverter and parametersConverter like you can for any other subclass. So I propose we create a new public class like the following to support custom grant types:

public final class WebClientReactiveOAuth2AccessTokenResponseClient<T extends AbstractOAuth2AuthorizationGrantRequest>
        extends AbstractWebClientReactiveOAuth2AccessTokenResponseClient<T> {

    @Override
    ClientRegistration clientRegistration(T grantRequest) {
        return grantRequest.getClientRegistration();
    }

    @Override
    Set<String> scopes(T grantRequest) {
        return grantRequest.getClientRegistration().getScopes();
    }

}

which could then be used like so:

public class MyGrantRequest extends AbstractOAuth2AuthorizationGrantRequest {

    private final String assertion;

    protected MyGrantRequest(ClientRegistration clientRegistration, String assertion) {
        super(new AuthorizationGrantType("urn:ietf:params:oauth:grant-type:my-grant-type"), clientRegistration);
        this.assertion = assertion;
    }

    public String getAssertion() {
        return this.assertion;
    }

}

static ReactiveOAuth2AccessTokenResponseClient<MyGrantRequest> myTokenResponseClient() {

    WebClientReactiveOAuth2AccessTokenResponseClient<MyGrantRequest> tokenResponseClient =
            new WebClientReactiveOAuth2AccessTokenResponseClient<>();

    tokenResponseClient.addParametersConverter((grantRequest) -> {
        MultiValueMap<String, String> parameters = new LinkedMultiValueMap<>();
        parameters.add(OAuth2ParameterNames.ASSERTION, grantRequest.getAssertion());

        return parameters;
    });

    return tokenResponseClient;
}

What do you think of this proposal as an alternative to opening up the existing abstract class?

Comment From: kkondratov

@sjohnr Thank you for clarifying that the intent of the existing class was keeping it closed off for extension.

Indeed the current implementation AbstractWebClientReactiveOAuth2AccessTokenResponseClient contains a large boilerplate which can be reused, hence my question/suggestion.

Your proposal to add a class that contains the boilerplate and let the internals be customizable is exactly what we've done by using the package-collision workaround with the existing AbstractWebClientReactiveOAuth2AccessTokenResponseClient.

The proposed solution indeed solves the issue of implementing custom grant types and I agree with the solution.

Reason for edit: I misunderstood the proposed implementation initially hence the deletion of the other code.

Comment From: sjohnr

Thanks for confirming that this suggestion would work for you! Would you be interested in submitting a PR with the proposed WebClientReactiveOAuth2AccessTokenResponseClient and some tests? I'm happy to help guide you if you need help, as I'm currently adding an implementation for Token Exchange Grant right now.

Comment From: kkondratov

@sjohnr I'd be happy and interested in submitting a PR. Will need to read up on the guidelines and the procedure regarding the spring projects.

Comment From: sjohnr

Thanks @kkondratov! If you weren't aware, you can read about that here.

As far as adding tests, take a look at WebClientReactiveTokenExchangeTokenResponseClientTests which I added yesterday. I think it turned out fairly well, and looks similar to the test class for the non-reactive client. See also WebClientReactiveJwtBearerTokenResponseClientTests, though it looks slightly different than the corresponding non-reactive version.

Since jwt-bearer is a very simple grant type, my suggestion would be to re-use the existing JwtBearerGrantRequest but configure the generic client and test it as if you were implementing the jwt-bearer grant type as custom. Feel free to use or discard that suggestion. :wink:

Comment From: sjohnr

Hi @kkondratov, have you had a chance to work on this? If not, no problem.