Remove use of deprecated ClientAuthenticationMethod's

[bibibiu2017]

Fixes gh-346

[pivotal-cla]

@bibibiu2017 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@bibibiu2017 Thank you for signing the Contributor License Agreement!

[sjohnr]

Hi @bibibiu2017, thanks for your interest in the project!

Unfortunately, this change has broken quite a lot of tests. Any chance you can take a look at those? Looking at the tests may highlight whether this alone is the correct change to make or not. I'm still catching up on the issue myself.

Also, please refer to contributing guidelines when submitting pull requests. In particular, you will want to update the header of files updated with the year "2021" in the copyright, squash your commits, and include "Fixes gh-346" on a separate line in your commit comment.

[bibibiu2017]

I have taken a look at the tests, the configured test registered clients, and some assertions are using the deprecated POST and BASIC Client authentication methods.

[sjohnr]

Thanks for looking at the tests, @bibibiu2017! I've tested on your branch, and while the immediate issue is resolved with this change, the sample project no longer works. This is because we've flipped the problem so now the deprecated client authentication methods are not allowed.

This is a difficult issue to solve for without too many changes, due to the fact that the AuthenticationConverters being used aren't aware of the RegisteredClient which requests what method(s) should be supported, as that later runs into a conflict in OAuth2ClientAuthenticationProvider.java#L103.

Of the available options, I think we can either:

1. Automatically add CLIENT_SECRET_BASIC or CLIENT_SECRET_POST when a client is registered with BASIC or POST respectively, or
2. Automatically upgrade BASIC or POST to CLIENT_SECRET_BASIC or CLIENT_SECRET_POST respectively when a client is registered.

Both options could have an impact. Would you be able to attempt option 2? One approach could be limiting changes to RegisteredClient.Builder#clientAuthenticationMethod, RegisteredClient.Builder#clientAuthenticationMethods and tests.

[bibibiu2017]

I have attempted option 2. Though I could not localize impact to the recommended test files due to assertions that check equality with deprecated BASIC and POST. One recommendation would be to change overridden equals() method of ClientAuthenticationMethod making BASIC or POST equivalent to CLIENT_SECRET_BASIC or CLIENT_SECRET_POST respectively.

[sjohnr]

@bibibiu2017, sorry I should have been more clear, I meant "limiting additional changes outside of tests" and I think you have done that! I was just thinking of a slightly different approach than yours, but they work out to the same thing. So this seems like a great option!

See inline feedback below. Just a note I'm not going to merge this right away, as I'd like to have a discussion with our community maintainer first, but if we can get it ready for merge, then we'll be in really good shape.

[sjohnr]

Also, I forgot to mention, make sure your commit message contains on a separate line:

"Closes gh-346"

[bibibiu2017]

@sjohnr thanks for the feedback will be working on this shortly.

[jgrandja]

Thanks for all the updates @bibibiu2017 ! This is now in main.

I also added a polish commit that ensures all usages of ClientAuthenticationMethod.BASIC and ClientAuthenticationMethod.POST are no longer used.