Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow AbstractWebClientReactiveOAuth2AccessTokenResponseClient to be extended #14657

Open
Tracked by #15299
kkondratov opened this issue Feb 28, 2024 · 6 comments · May be fixed by #15884
Open
Tracked by #15299

Allow AbstractWebClientReactiveOAuth2AccessTokenResponseClient to be extended #14657

kkondratov opened this issue Feb 28, 2024 · 6 comments · May be fixed by #15884
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@kkondratov
Copy link

kkondratov commented Feb 28, 2024

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. #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.

@kkondratov kkondratov added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Feb 28, 2024
@sjohnr
Copy link
Member

sjohnr commented Mar 4, 2024

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?

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 4, 2024
@kkondratov
Copy link
Author

kkondratov commented Mar 5, 2024

@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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 5, 2024
@sjohnr
Copy link
Member

sjohnr commented Mar 5, 2024

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.

@kkondratov
Copy link
Author

@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.

@sjohnr
Copy link
Member

sjohnr commented Mar 7, 2024

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. 😉

@sjohnr
Copy link
Member

sjohnr commented Apr 19, 2024

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

sjohnr added a commit to sjohnr/spring-security that referenced this issue May 1, 2024
sjohnr added a commit to sjohnr/spring-security that referenced this issue May 21, 2024
@sjohnr sjohnr moved this to In Progress in Spring Security Team Jun 25, 2024
sjohnr added a commit to sjohnr/spring-security that referenced this issue Oct 7, 2024
@sjohnr sjohnr linked a pull request Oct 7, 2024 that will close this issue
sjohnr added a commit to sjohnr/spring-security that referenced this issue Oct 23, 2024
@sjohnr sjohnr removed the status in Spring Security Team Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants