Client id inclusion for refresh token grant is not consistent between servlet and reactive stacks

[benba]

When a refresh token grant exchange occurs with a ClientAuthentication Method set to NONE

On a servlet appliction, the client_id field will be missing because it is only added if the ClientAuthenticationMethod is set to CLIENT_SECRET_POST: see OAuth2Refresh TokenGrantRequestEntityConverter

On a reactive applicatoin, the behavior is not consistent because the client_id field is always added except if ClientAuthenticationMethod is set to CLIENT_SECRET_BASIC: see AbstractWebClientReactiveOAuth2AccessTokenResponseClient

Our internal IDP use only public clients but client_id parameter is mandatory, so the reactive default seems better for us, but maybe there are other cases that would make the current servlet default better?
I don't see any clear answer for the specification, some implementations (like Auth0 or Curity) seems to always ask a client_id, but other don't (like Okta).

Maybe the client_id inclusion should depend from another property of the client registration instead of the ClientAuthenticationMethod ?

[sjohnr]

@benba thanks for reaching out!

This is a fairly nuanced topic, particularly because (as you pointed out) the specification doesn't paint a perfectly clear picture. A reading of the core OAuth 2.0 spec suggests that whether the client_id parameter is required or not depends entirely on the client authentication mechanism chosen. In all of the examples you provided, the client_id is included when using client_secret_post as the method. It's worth mentioning that the spec discourages its use:

Including the client credentials in the request-body using the two
parameters is NOT RECOMMENDED and SHOULD be limited to clients unable
to directly utilize the HTTP Basic authentication scheme

I'm not clear why Auth0 and Curity would use it as their default example, but I'm sure they both support client_secret_basic as well and don't require client_id in that case. Perhaps I'm wrong though.

In any case, I think this issue extends beyond just refresh tokens to other grant types as well. I believe the reactive side has it mostly correct. Of the currently supported client authentication methods (none, client_secret_basic, client_secret_post, private_key_jwt and client_secret_jwt), it seems to me that only client_secret_basic provides the client id without the need for the parameter. On the servlet side, the authorization code grant is already consistent with this, but the others seem not to be.

Before we go any farther, we would need to identify whether making a change here would be a breaking change and have an impact on users. It doesn't seem likely that clients would break if they start providing a client_id in cases where they were not before. However, changing this type of behavior often causes a lot of disruption for users because of the wide variety of providers. I'm not sure if we can determine how disruptive it will be, and I'd prefer not to move forward without more discussion and feedback on that.

In the meantime, you can of course work around the issue by customizing the token request parameters with the following configuration:

@Bean
public OAuth2AccessTokenResponseClient<OAuth2RefreshTokenGrantRequest> refreshTokenAccessTokenResponseClient() {
	var requestEntityConverter = new OAuth2RefreshTokenGrantRequestEntityConverter();
	requestEntityConverter.addParametersConverter(parametersConverter());

	var accessTokenResponseClient = new DefaultRefreshTokenTokenResponseClient();
	accessTokenResponseClient.setRequestEntityConverter(requestEntityConverter);

	return accessTokenResponseClient;
}

private static Converter<OAuth2RefreshTokenGrantRequest, MultiValueMap<String, String>> parametersConverter() {
	return (grantRequest) -> {
		var clientRegistration = grantRequest.getClientRegistration();
		var clientAuthenticationMethod = clientRegistration.getClientAuthenticationMethod();
		var parameters = new LinkedMultiValueMap<String, String>();
		if (!ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(clientAuthenticationMethod)) {
			parameters.set(OAuth2ParameterNames.CLIENT_ID, clientRegistration.getClientId());
		}

		return parameters;
	};
}

I'll spend some time thinking about this, but let me know if you have any additional thoughts.

[marbon87]

I had the same problem by using oauth2-login with a public-pkce client and keycloak as idp.

Thanks for the workaround @sjohnr

[sjohnr]

Apologies for the noise on this issue, I mixed up this issue # with gh-11298.

By way of an update, my goal is to partially address this issue via work on theme issue gh-15299. However, due to needing to remain backwards compatible, I don't anticipate directly addressing this issue with changes to the existing DefaultRefreshTokenTokenResponseClient (and corresponding OAuth2RefreshTokenGrantRequestEntityConverter). Instead, we can make it possible to customize behavior across both Servlet and Reactive similar to the above workaround.

[marbon87]

Hi @sjohnr,
the workaround is not working as expected for client-authentication-method=POST, because the client_id-Parameter is added twice (by the custom parametersConverter and in OAuth2RefreshTokenGrantRequestEntityConverter#createParameters) and then you get the following exception:

Caused by: org.springframework.security.oauth2.core.OAuth2AuthorizationException: [invalid_request] duplicated parameter
	at org.springframework.security.oauth2.client.http.OAuth2ErrorResponseErrorHandler.handleError(OAuth2ErrorResponseErrorHandler.java:66)
	at org.springframework.web.client.ResponseErrorHandler.handleError(ResponseErrorHandler.java:63)
	at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:942)
	at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:891)
	at org.springframework.web.client.RestTemplate.exchange(RestTemplate.java:730)
	at org.springframework.security.oauth2.client.endpoint.DefaultRefreshTokenTokenResponseClient.getResponse(DefaultRefreshTokenTokenResponseClient.java:98)
	at org.springframework.security.oauth2.client.endpoint.DefaultRefreshTokenTokenResponseClient.getTokenResponse(DefaultRefreshTokenTokenResponseClient.java:74)
	at org.springframework.security.oauth2.client.endpoint.DefaultRefreshTokenTokenResponseClient.getTokenResponse(DefaultRefreshTokenTokenResponseClient.java:53)
	at org.springframework.security.oauth2.client.RefreshTokenOAuth2AuthorizedClientProvider.getTokenResponse(RefreshTokenOAuth2AuthorizedClientProvider.java:101)

IMHO it has to be

private static Converter<OAuth2RefreshTokenGrantRequest, MultiValueMap<String, String>> parametersConverter() {
	return grantRequest -> {
		var clientRegistration = grantRequest.getClientRegistration();
		var clientAuthenticationMethod = clientRegistration.getClientAuthenticationMethod();
		var parameters = new LinkedMultiValueMap<String, String>();
		if (ClientAuthenticationMethod.NONE.equals(clientAuthenticationMethod)) {
			parameters.set(OAuth2ParameterNames.CLIENT_ID, clientRegistration.getClientId());
		}

		return parameters;
	};
}

[sjohnr]

Thanks for the update @marbon87. I think that makes sense for the workaround.

[sjohnr]

Hi everyone. I have merged gh-15337 which includes new implementations of OAuth2AccessTokenResponseClient for servlet applications, which are implemented much more consistently with the reactive counterparts. The goal is to make it possible to opt-in (instead of breaking changes) for refresh token grant (and other grant types) sending parameters in token requests in a consistent manner.

Please try out the changes in 6.4.0-SNAPSHOT and share your feedback related to this issue. If you don't have any other OAuth2 Client customizations, you can opt-in for the refresh token grant by simply publishing the following bean:

@Configuration
@EnableWebSecurity
public class SecurityConfiguration {

	// ...

	@Bean
	public OAuth2AccessTokenResponseClient<OAuth2RefreshTokenGrantRequest> refreshTokenAccessTokenResponseClient() {
		return new RestClientRefreshTokenTokenResponseClient();
	}

}

[marbon87]

Hey @sjohnr,

sorry, but how does this solve the root problem of this issue, that the client-id is missing in the refresh request?
There is stil a difference in reactive and servlet implementation, or not?

[sjohnr]

Hi @marbon87,

As mentioned in this comment, we cannot adjust the behavior in the DefaultRefreshTokenTokenResponseClient as it would not be backwards compatible. However, the new implementation RestClientRefreshTokenTokenResponseClient behaves the same as the reactive implementation. So if you'd like servlet to be consistent, you can opt-in as demonstrated in my previous comment. Does that make sense?

[marbon87]

Hey @sjohnr ,

sorry my unit-test had a smell. After i fixed that, everything is working fine with the new RestClientRefreshTokenTokenResponseClient

[sjohnr]

Thanks for the feedback @marbon87.

As mentioned in this comment, directly addressing this issue would break passivity. Instead, users on the servlet stack can opt-in to RestClient-based implementations that are consistent with reactive. I'm going to close this issue as a duplicate of (addressed by) gh-15298.