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.