Implement Client Credentials Authentication #72

pkostrzewa · 2020-04-27T15:30:25Z

The Filter should process requests for the (default) path /oauth2/token and check if credentials are available in the request.
The OAuth2ClientAuthenticationToken should be passed to the AuthenticationManager.
The AuthenticationManager should be composed of OAuth2ClientAuthenticationProvider.
The OAuth2ClientAuthenticationProvider should use the RegisteredClientRepository.
The RegisteredClient should be returned in a new OAuth2ClientAuthenticationToken if the authentication succeeds.
The Filter should save the OAuth2ClientAuthenticationToken in the SecurityContext.
Add javadoc for class and public methods.
Unit tests.

The ClientAuthenticationConverter is based on BasicAuthenticationFilter.

Fixes #39 .

pkostrzewa · 2020-04-27T17:02:33Z

@jgrandja Few questions:

Should AuthenticationSuccessHandler and AuthenticationFailureHandler be used in early implementation?
Should RequestMatcher be used to check if request matches or simply checking the request URI is currently enough?

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java

.../springframework/security/oauth2/server/authorization/web/ClientAuthenticationConverter.java

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java

...ringframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java

pkostrzewa · 2020-04-28T21:43:00Z

@jgrandja Could you please elaborate?

The AuthenticationManager should be composed of OAuth2ClientAuthenticationProvider

Isn't OAuth2ClientAuthenticationProvider automatically registered in AuthenticationManager? If not, what should be done to register a provider?

jgrandja

Thanks for the PR @pkostrzewa. Please see my review comments.

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java

...ork/security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationToken.java

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java

...ringframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java

.../springframework/security/oauth2/server/authorization/web/ClientAuthenticationConverter.java

jgrandja · 2020-04-29T19:46:23Z

@pkostrzewa

Isn't OAuth2ClientAuthenticationProvider automatically registered in AuthenticationManager?

Yes, it will be in a later story. Nothing to do as far as this issue goes. I was simply stating the runtime expectations. I updated the main issue to be clear.

pkostrzewa · 2020-04-30T08:37:51Z

@jgrandja

Thanks for the review! I will update the PR with your comments later on today.

pkostrzewa · 2020-05-01T13:55:26Z

@jgrandja I've updated the PR. There are still missing tests and documentation and I will provide both as soon as I can.

jgrandja

Thanks for the updates @pkostrzewa. Please see my review comments.

For the next update, can you please rebase on master and avoid merge commits. And after you add all the tests please change the Draft to Ready and I think we'll be close to merging.

...ringframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java

...gframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationConverter.java

...th2/server/authorization/authentication/DefaultOAuth2ClientAuthenticationSuccessHandler.java

...th2/server/authorization/authentication/DefaultOAuth2ClientAuthenticationFailureHandler.java

...ework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationConverterTests.java

pkostrzewa · 2020-05-07T11:38:43Z

Thanks for the review @jgrandja. I will update the PR soon.

pkostrzewa · 2020-05-20T18:09:54Z

For the next update, can you please rebase on master and avoid merge commits. And after you add all the tests please change the Draft to Ready and I think we'll be close to merging.

Do you mean I should squash all commits from this PR into single one?

jgrandja · 2020-05-20T19:50:54Z

@pkostrzewa Yes please squash to 1 commit and rebase on latest master.

pkostrzewa · 2020-05-24T17:32:33Z

I've added missing tests and rebased on top of the master branch.

If there will be any changes requested I can address them next wednesday at earliest.

anoopgarlapati · 2020-05-26T07:18:51Z

...ork/security/oauth2/server/authorization/web/DefaultOAuth2ClientAuthenticationConverter.java

+		if (credentials.length != 2) {
+			throw new OAuth2AuthenticationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_TOKEN));
+		}
+		return new OAuth2ClientAuthenticationToken(credentials[0], credentials[1]);


@pkostrzewa cc: @jgrandja
As per RFC 6749 - section 2.3.1, the client identifier and client password are further encoded using application/x-www-form-urlencoded encoding algorithm and then used as username and password in HTTP Basic Authentication Scheme.

The client identifier is encoded using the
"application/x-www-form-urlencoded" encoding algorithm per
Appendix B, and the encoded value is used as the username; the client
password is encoded using the same algorithm and used as the
password.

There is an issue already logged in legacy oauth2 project spring-attic/spring-security-oauth#1826 and we should implement this according to specification in this project.

@anoopgarlapati
Although RFC 6749 - section 2.3.1, indeed does say:

The client identifier is encoded using the
"application/x-www-form-urlencoded" encoding algorithm per
Appendix B, and the encoded value is used as the username; the client
password is encoded using the same algorithm and used as the
password.

It further states:

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 (or other password-based HTTP authentication schemes). the parameters can only be transmitted in the request-body and MUST NOT be included in the request URI.

The authorization server MUST require the of TLS as described in Section 1.6 when sending requests using password authentication.

Since this client authentication method involves a password, the authorization server MUST protect any endpoint utilizing it against brute force attacks.

Those sections do not contradict each other. The encoding needs to be applied when using Basic authentication.

jgrandja · 2020-06-02T03:10:26Z

Thank you for all the updates @pkostrzewa !

Overall, the implementation looked good, however, I did add a polish commit in order to get this merged.

Please take a look at the changes applied in the polish commit and let me know if you have any questions.

Thank you again and I look forward to another contribution 👍

krisztian-toth reviewed Apr 28, 2020

View reviewed changes

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java Outdated Show resolved Hide resolved

krisztian-toth reviewed Apr 28, 2020

View reviewed changes

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java Show resolved Hide resolved

.../springframework/security/oauth2/server/authorization/web/ClientAuthenticationConverter.java Outdated Show resolved Hide resolved

Captain-P-Goldfish suggested changes Apr 28, 2020

View reviewed changes

.../security/oauth2/server/authorization/authentication/OAuth2ClientAuthenticationProvider.java Outdated Show resolved Hide resolved

...ringframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java Outdated Show resolved Hide resolved

jgrandja requested changes Apr 29, 2020

View reviewed changes

jgrandja self-assigned this Apr 29, 2020

jgrandja added the type: enhancement A general enhancement label Apr 29, 2020

pkostrzewa requested a review from jgrandja May 1, 2020 13:55

jgrandja requested changes May 6, 2020

View reviewed changes

jgrandja mentioned this pull request May 8, 2020

Added token endpoint implementation #79

Closed

9 tasks

pkostrzewa marked this pull request as ready for review May 24, 2020 17:27

Implement Client Credentials Authentication

1084611

pkostrzewa requested a review from jgrandja May 25, 2020 11:20

anoopgarlapati reviewed May 26, 2020

View reviewed changes

jgrandja added this to the 0.0.1 milestone Jun 2, 2020

jgrandja added the status: duplicate A duplicate of another issue label Jun 2, 2020

jgrandja added a commit that referenced this pull request Jun 2, 2020

Polish gh-72

7a5585f

jgrandja closed this Jun 2, 2020

pkostrzewa deleted the gh-39 branch June 2, 2020 13:29

jgrandja mentioned this pull request Jun 15, 2020

Add client_credentials grant type support #88

Closed

jzheaux mentioned this pull request Apr 16, 2021

Client credentials not correctly encoded in Basic Auth spring-projects/spring-security#9610

Closed

petergphillips mentioned this pull request Jul 5, 2021

Regression with URL encode client credentials spring-projects/spring-security#10018

Closed

doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023

Polish spring-projectsgh-72

0ec5b21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Client Credentials Authentication #72

Implement Client Credentials Authentication #72

pkostrzewa commented Apr 27, 2020 •

edited

Loading

pkostrzewa commented Apr 27, 2020 •

edited

Loading

pkostrzewa commented Apr 28, 2020 •

edited

Loading

jgrandja left a comment

jgrandja commented Apr 29, 2020

pkostrzewa commented Apr 30, 2020

pkostrzewa commented May 1, 2020

jgrandja left a comment

pkostrzewa commented May 7, 2020

pkostrzewa commented May 20, 2020

jgrandja commented May 20, 2020

pkostrzewa commented May 24, 2020

anoopgarlapati May 26, 2020 •

edited

Loading

dfcoffin May 26, 2020

OrangeDog Jan 18, 2021

jgrandja commented Jun 2, 2020

Implement Client Credentials Authentication #72

Implement Client Credentials Authentication #72

Conversation

pkostrzewa commented Apr 27, 2020 • edited Loading

pkostrzewa commented Apr 27, 2020 • edited Loading

pkostrzewa commented Apr 28, 2020 • edited Loading

jgrandja left a comment

Choose a reason for hiding this comment

jgrandja commented Apr 29, 2020

pkostrzewa commented Apr 30, 2020

pkostrzewa commented May 1, 2020

jgrandja left a comment

Choose a reason for hiding this comment

pkostrzewa commented May 7, 2020

pkostrzewa commented May 20, 2020

jgrandja commented May 20, 2020

pkostrzewa commented May 24, 2020

anoopgarlapati May 26, 2020 • edited Loading

Choose a reason for hiding this comment

dfcoffin May 26, 2020

Choose a reason for hiding this comment

OrangeDog Jan 18, 2021

Choose a reason for hiding this comment

jgrandja commented Jun 2, 2020

pkostrzewa commented Apr 27, 2020 •

edited

Loading

pkostrzewa commented Apr 27, 2020 •

edited

Loading

pkostrzewa commented Apr 28, 2020 •

edited

Loading

anoopgarlapati May 26, 2020 •

edited

Loading