Simplify Disabling application/x-www-form-urlencoded Encoding Client ID and Secret

[rwinch]

There are quite a few authorization servers (see how many users have commented on gh-10018) that do not URL Encode the Client ID and Secret as required by the OAuth specification. There is a way to customize the configuration to support this use-case, but is quite verbose given the number of users experiencing the problem. We should provide an easier way to disable URL encoding.

[Crain-32]

@sjohnr I've encountered this issue myself now, and dug up this issue while researching the problem.

Since you're assigned to this issue, what sort of approach were you thinking for it? Since it appears that from the issues that it's either all Base64 encoded, or URL Escaped and then encoded, I think we can just put a property into the OAuth2 registration.

Something like,
spring.security.oauth2.client.registration.myRegistration.base64-encoder-escaped=false # or true

Since it's in the client registration, we only have to adjust the OAuth2AuthorizationGrantRequestEntityUtils::getTokenRequestHeaders to the following.

static HttpHeaders getTokenRequestHeaders(ClientRegistration clientRegistration) {
	HttpHeaders headers = new HttpHeaders();
	headers.addAll(DEFAULT_TOKEN_REQUEST_HEADERS);
        final boolean escapeEncoding = clientRegistration.escapeEncoding();
	if (ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(clientRegistration.getClientAuthenticationMethod())
			|| ClientAuthenticationMethod.BASIC.equals(clientRegistration.getClientAuthenticationMethod())) {
		String clientId = escapeEncoding ? clientId : encodeClientCredential(clientRegistration.getClientId());
		String clientSecret = escapeEncoding ? clientSecret : encodeClientCredential(clientRegistration.getClientSecret());
		headers.setBasicAuth(clientId, clientSecret);
	}
	return headers;
}

[sjohnr]

Hi @Crain-32, thanks for thinking about this! Sadly I have not been able to prioritize this issue, but it seems like it would be helpful to make a solution available. I think it would be fine for a contributor from the community to work on it as well. Would you like to?

I think we can just put a property into the OAuth2 registration.

I don't prefer this approach, and generally we don't want to introduce properties into ClientRegistration for specific client authentication methods. Instead, customization should be applied using provided hooks. In this case, AbstractOAuth2AuthorizationGrantRequestEntityConverter (and all subclasses) have setHeadersConverter(Converter<T, HttpHeaders> headersConverter).

So my preference would be to introduce a new public class with a setter containing a boolean flag to enable/disable this behavior. Something like:

public final class DefaultOAuth2TokenRequestHeadersConverter<T extends AbstractOAuth2AuthorizationGrantRequest>
        implements Converter<T, HttpHeaders> {

    private boolean encodeClientCredentials = true;

    // setter and implementation...

}

It's also possible we would want to introduce a factory method instead of a public class, so we'd need to think about that before committing to either approach. Any thoughts on this?

[Crain-32]

Thanks for the fast response @sjohnr! I'd love to work on this.

However, I will admit that I don't have the biggest understanding of all the OAuth2 Builders available, so I'll attempt to do a factory/class style, and copy some of what I see around it. I know there is the public OAuth2AuthorizedClientProviderBuilder password(Consumer<PasswordGrantBuilder> builderConsumer) for example. Which will probably be my starting point.

Just to make sure I got a solid grasp of what to look at, I'd assume the result should also include the following,

• Default Behavior is still Java's URLEncoder of the Values before passing them to the Basic Auth. (Existing Tests should cover this)
• Some sort of Note/Tip in the Spring Security OAuth2 Documentation about this.
• Some sort of small test.

[sjohnr]

I know there is the public OAuth2AuthorizedClientProviderBuilder password(Consumer<PasswordGrantBuilder> builderConsumer) for example. Which will probably be my starting point.

Take a look at OAuth2AuthorizationRequestCustomizers.withPkce() as this is more what I was thinking for a factory method. In our case, it would return a Converter<T, HttpHeaders>. But again I'm not sure we will go with a factory method, so don't spend too much time on it yet. I'll converse with @jgrandja this week and get back to you.

I'd assume the result should also include the following,

• Default Behavior is still Java's URLEncoder of the Values before passing them to the Basic Auth. (Existing Tests should cover this)

• Some sort of Note/Tip in the Spring Security OAuth2 Documentation about this.

• Some sort of small test.

I agree!

[sjohnr]

Hi @Crain-32. I discussed with @jgrandja and we think the first option (a public class as in my example) is the way to go. So don't worry about a factory method. Make sense? Let me know if you have any questions. Thanks!

[Crain-32]

@sjohnr hit a little stumbling block so I'll use this as an opportunity to get some feedback while I finish up the implementation.

I'm unsure what the best course of action is to let the Users set the flag to false. Since the AbstractOAuth2AuthorizationGantRequestEntityConverter doesn't expose a getter for the headersConverter, the only ways I can think of is to have them utilize the setHeadersConverter, and move the encodeClientCredentials into the Constructor of the DefaultOAuth2TokenRequestHeadersConverter instead of a setter.

Or add a setter to the Abstract Entity Converter that does a check like this.

public setHeaderCoverterUrlEncoding(boolean shouldUseUrlEncoding) {
     if (this.headersConverter instanceof DefaultOAuth2TokenRequestHeadersConverter converter) {
         converter.setEncodeClientCredentials(shouldUseUrlEncoding);
     }
}

I'm leaning towards the setter in the Abstract Entity Converter, since if you're modifying the Headers Converter that much, you likely aren't using the default anymore.

---

In terms of house keeping, as we're replacing the last usage of OAuth2AuthorizationGrantRequestEntityUtils with this class, I've marked the utils as Deprecated. Felt like the right move.

[sjohnr]

@Crain-32 thanks for the update. I'm glad you're making progress!

I'm unsure what the best course of action is to let the Users set the flag to false.

There is no need to expose a setter or getter on the AbstractOAuth2AuthorizationGantRequestEntityConverter since this is a customization that should use existing customization hooks. A special constructor isn't needed either. The usage pattern should be like the following:

@Bean
public OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> authorizationCodeTokenResponseClient() {
	var defaultHeadersConverter = new DefaultOAuth2TokenRequestHeadersConverter<OAuth2AuthorizationCodeGrantRequest>();
	defaultHeadersConverter.setEncodeClientCredentials(false);

	var requestEntityConverter = new OAuth2AuthorizationCodeGrantRequestEntityConverter();
	requestEntityConverter.setHeadersConverter(defaultHeadersConverter);

	var tokenResponseClient = new DefaultAuthorizationCodeTokenResponseClient();
	tokenResponseClient.setRequestEntityConverter(requestEntityConverter);

	return tokenResponseClient;
}

Does that clear it up for you?

In terms of house keeping, as we're replacing the last usage of OAuth2AuthorizationGrantRequestEntityUtils with this class, I've marked the utils as Deprecated. Felt like the right move.

OAuth2AuthorizationGrantRequestEntityUtils is not a public class, so it can be directly removed without deprecation.

[Crain-32]

That does! Thank you.

I hadn't even realized that wasn't a public class 🤦🏻‍♂️ , I'll remove it and attach the author onto my class Java doc, since I'm copying and pasting like 95% of that class.

Since this covers all the implementation, I'll keep out of your hair till next week, since I'm not going to get a PR ready till after this weekend, and I expect the testing/docs to be decently straightforward.