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

[Crain-32]

Closes #11440 , includes a new test that covers both the URL encoding and URL non-encoding.

Force Push was to keep this at one commit, and I forgot to follow the formatting. 🤦🏻‍♂️

I am really bad at following this formatter, I apologize for any notifications this sends.

[Crain-32]

I have no idea why the formatter is failing on the imports in the DefaultOAuth2TokenRequestHeadersConverter. I'll need someone else to review that.

[sjohnr]

I have no idea why the formatter is failing on the imports in the DefaultOAuth2TokenRequestHeadersConverter. I'll need someone else to review that.

I checked the diff, and it looks like the java.nio.charset.StandardCharsets import is not correctly ordered within the import list. You can simply move it back to its original position.

[Crain-32]

That didn't fix it, I'm begin to think it's the function order?

Honestly since this is a small PR I'm going to let it sit for a bit and dig into #11157 and the IntelliJ formatting to see if we can get IntelliJ to auto format correctly.

[sjohnr]

@Crain-32 according to the build output, you can simply run ./gradlew format to fix formatting issues, and try the build again.

[Crain-32]

My brain was reading that as using format to check the formatting 🤦🏻‍♂️

Fixed.

[sjohnr]

Thanks for the PR and work on this so far @Crain-32! I have some comments inline below.

Additionally, I feel that we should provide equivalent support for the reactive side (AbstractWebClientReactiveOAuth2AccessTokenResponseClient). It is already set up to allow customizing the headers converter. I've added a comment below regarding the charset=UTF-8 in the Content-Type which is the only discrepancy I noticed between the two implementations. If that bit is made configurable, would you be able to add this to AbstractWebClientReactiveOAuth2AccessTokenResponseClient as well (and add corresponding tests)? If it's too much, I can help out with that as a follow up.

Thanks!

[sjohnr]

Please make this class final.

[sjohnr]

Also, I don't see any unit tests for this class? If so, please add tests (DefaultOAuth2TokenRequestHeadersConverterTests) specifically for this class.

[sjohnr]

Please add basic javadoc for this method.

[sjohnr]

I'd like to be consistent with the use of var. I don't see it being used anywhere else in this class (or in the new test) so for now I think we should use the normal syntax.

[sjohnr]

I wonder if we can get rid of this DEFAULT_TOKEN_HEADERS and simply add the relevant default headers directly in the convert method? I think it would nicely simplify the class and bring it into alignment with AbstractWebClientReactiveOAuth2AccessTokenResponseClient#populateTokenRequestHeaders. (see opening comment)

[Crain-32]

I've swapped it to also having a setter and no longer being static final.
This also meant I felt a lot better adjusting to using just Accept: application/json, since if there is a real need to go back to application/json;charset=UTF-8 or application/x-www-form-urlencoded;charset=UTF-8 for consumers, they can configure that the same way they can configure the encodeClientCredentialsIfRequired

[sjohnr]

I don't think I prefer to add setDefaultHeaders(...). Instead, we can add fields:

• private MediaType accept (defaults to MediaType.APPLICATION_JSON)
• private MediaType contentType (defaults to MediaType.APPLICATION_FORM_URLENCODED)

and private methods:

• setAccept(MediaType)
• setContentType(MediaType)

These methods will only be called by the new package-private static factory method (see below conversation). The static final and getDefaultTokenRequestHeaders() can be removed.

[sjohnr]

In order to bring this into alignment with the corresponding logic on the reactive side (see AbstractWebClientReactiveOAuth2AccessTokenResponseClient#populateTokenRequestHeaders), I wonder if there's anything we can do about this charset=UTF-8? For backwards compatibility, it might be nice to make this additional param on the Content-Type configurable. If we do this, we can re-use this exact implementation for the reactive side as well. (see opening comment)

[Crain-32]

Removing the charset=UTF-8 breaks about 19 tests. Do you want me to remove that check from the tests? Or maintain a default of charset=UTF-8? (More information on a different conversation)

[sjohnr]

@Crain-32 We don't want to change the behavior for the servlet side (tests should not be changed). But charset=UTF-8 should not be the default for the new class either. Instead, the AbstractOAuth2AuthorizationGrantRequestEntityConverter should be able to easily create an instance of this new class with the addition of charset=UTF-8 enabled. Then the reactive version would just instantiate and use the class with defaults.

The best way to do this IMHO would be to create a package-private static factory method in this new class that instantiates the class and enables the customized charset, which is used by AbstractOAuth2AuthorizationGrantRequestEntityConverter. Make sense?

[Crain-32]

@sjohnr it does make sense to use a factory. Would the preferred naming be something like ....HeaderConverterCustomizer, or .....HeaderConverterFactory? The first feels more in line with the general idea of "Customizer" the JWT Client customizer does, where the second one feels more generic to the Java ecosystem.

Follow up question on that though, would it be better to expose the factory/builder within the public scope instead of the class?
Current plan for customization was just having the consumer use new Default.....(), but if we're having the internals use a factory, might as well expose the factory instead of the class.

[sjohnr]

Would the preferred naming be something like ....HeaderConverterCustomizer, or .....HeaderConverterFactory?

It should be a package-private static method, not a class. The name doesn't exactly matter because it will be hidden at the package level. However, withCharsetUtf8() would be my preferred name, as it tells the story fairly well.

Current plan for customization was just having the consumer use new Default.....(), but if we're having the internals use a factory, might as well expose the factory instead of the class.

I don't think that we should expose this. The charset customization is only being used to make this change non-breaking. The charset=UTF-8 variants in MediaType are deprecated and should not be used. We don't want to encourage their use, so keeping this customization internal and only used by default implementations provided by the framework is preferred. Said another way, this customization should stay hidden in the framework and only used to prevent breaking tests (e.g. backwards compatibility).

[sjohnr]

This note seems to be added in the "Authenticate using private_key_jwt" section? If so, this wouldn't be the best place to add this note. Perhaps it could be a [TIP] elsewhere? For example, it could be added to authorization-grants.adoc where we cover customizing the access token request for each grant type. But this would require adding the tip 5 times because we unfortunately have structured the content that way currently. Or we could (for now) add an "Authenticate using client_secret_basic" section on this page.

[Crain-32]

I'll format and push the code I have broken tests (See other comments) so I'll post the relevant stuff here, and we can discuss the documentation side after we feel good about the integration of the DefaultOAuthtokenREquestHeadersConverter into the Reactive and Non-Reactive side. Since I can see that impacting the documentation we set up.
I've swapped the AbstractWebClientReactiveOAuth2AccessTokenResponseClient to use the DefaultOAuth2TokenREquestHeadersConverter

private Converter<T, HttpHeaders> headersConverter = new DefaultOAuth2TokenRequestHeadersConverter<>();

As other comments state, I've also swapped the default headers to be done as follows.

private HttpHeaders initialTokenHeaders = getDefaultTokenRequestHeaders();

private static HttpHeaders getDefaultTokenRequestHeaders() {
HttpHeaders headers = new HttpHeaders();
headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON));
headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
return headers;
}
// There is a setter for the initialTokenHeaders as well

I do currently have the DefaultOAuth2TokenRequestHeadersConverter set to be public final, so that way consumers can customize it by just creating their own instance.

[sjohnr]

Let's fix things so that tests are not broken (see above recommendations). As mentioned above, let's also remove getDefaultTokenRequestHeaders().

[Crain-32]

Sorry, took this week off in general.
I'll keep to this tomorrow.

[testersen]

With this pull request, will there be a configuration we can tweak in the application properties/yaml file we can use to disable url encoding of the identifier? Something like spring.security.oauth2.client.provider.my-provider.urlencode-identifier = false?

[sjohnr]

@testersen configuration properties are a spring boot feature, so we won't be able to add anything like that here. I don't think that would be the preferred way to customize anyway, since it is authentication method specific.

We'll provide a docs update with this PR but this comment illustrates intended usage.

[testersen]

Understood! Thanks for making that clearer! :)

[Crain-32]

That last push should have covered all the code changes. I'm not sure if historicalConverter was the best name for the simple factory, but it sounded good, so I went with it.

Figured it would be best to get an okay on the code -> update docs -> rebase on master -> finalize.

*Formatting is currently broken, will be fixed on the Update Docs update

[sjohnr]

Thanks for the updates @Crain-32! Since we're wrapping up the release cycle and I felt like this could still fit in this release, I went ahead and merged this PR as d0adb2a with polish commit 2598bf8. I also updated the docs in 2dd908d.

[Crain-32]

@sjohnr Thanks for doing that! I know this PR/Issue was a little long lived, but I'm grateful for the feedback and finishing it out for the release. Ideally the next one will go smoother.