Skip to content

Add client_credentials grant type support #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add client_credentials grant type support #88

wants to merge 1 commit into from

Conversation

@alek-sys
Copy link
Contributor

@alek-sys alek-sys commented Jun 11, 2020 •
edited by jgrandja
Loading

Fixes gh-51
@jgrandja jgrandja self-assigned this Jun 11, 2020
@jgrandja jgrandja added the type: enhancement A general enhancement label Jun 11, 2020
@jgrandja jgrandja added this to the 0.0.1 milestone Jun 11, 2020
@alek-sys
Copy link
Contributor Author

alek-sys commented Jun 15, 2020 •
edited
Loading

Hey @jgrandja, I still have a couple of to-dos:

  • Support basic auth for client id / secret
  • Update sample app to use JWK to sign the token. This should allow to put together an end-to-end demo

With that in mind, feel free to provide any initial feedback you have.
jgrandja
jgrandja requested changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @alek-sys. Please see review comments.

In addition to the feedback, please revert all changes in the samples/* as we can update the existing samples after we get this merged.
@jgrandja
Copy link
Collaborator

jgrandja commented Jun 15, 2020

@alek-sys

Regarding

Support basic auth for client id / secret

This is already implemented in ClientSecretBasicAuthenticationConverter
@alek-sys
Copy link
Contributor Author

alek-sys commented Jun 16, 2020

Thanks for feedback @jgrandja! I'm now re-working this PR based on your comments.
@alek-sys alek-sys force-pushed the master branch 4 times, most recently from acb8bea to 0fa4376 Compare June 18, 2020 13:07
@alek-sys
Copy link
Contributor Author

alek-sys commented Jun 18, 2020

Updated based on your comments.

Also, the spec states:

If the issued access token scope is different from the one requested by the client, the authorization server MUST include the "scope" response parameter to inform the client of the actual scope granted.

But currently Token Endpoint always returns scope in the response. Shoud l I fix it in this PR or create a separate story for this (unless there is one already)?
@jgrandja
Copy link
Collaborator

jgrandja commented Jun 18, 2020

The returned scope will come into play in #42 at a later point. No need to address it in this PR. But feel free to add notes to #42.
@alek-sys alek-sys marked this pull request as ready for review June 18, 2020 22:28
jgrandja
jgrandja requested changes Jun 22, 2020
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @alek-sys. Please see review comments.
@alek-sys
Copy link
Contributor Author

alek-sys commented Jun 23, 2020

Thanks @jgrandja, I've updated this PR. Now there is much better split between converted and provider.
@jgrandja
Copy link
Collaborator

jgrandja commented Jun 23, 2020

Great thanks @alek-sys. I will review this tomorrow.
@alek-sys
Copy link
Contributor Author

alek-sys commented Jun 26, 2020

@jgrandja updated based on your latest comments. Also, see my comment regarding OAuth2ScopeParser - I think it has some value as it validates scope parameter to be compliant with RFC.
@jgrandja
Copy link
Collaborator

jgrandja commented Jul 1, 2020 •
edited
Loading

@alek-sys Can you please update the test methods to following the naming convention methodNameWhenConditionThenResult. Take a look at OAuth2TokenEndpointFilterTests and OAuth2AuthorizationCodeAuthenticationProviderTests for examples.

Also, please add integration tests in OAuth2ClientCredentailsGrantTests (new). See OAuth2AuthorizationCodeGrantTests for example.
@alek-sys alek-sys force-pushed the master branch 4 times, most recently from 359ebd2 to 23ffd28 Compare July 2, 2020 11:22
@alek-sys
Copy link
Contributor Author

alek-sys commented Jul 2, 2020

@jgrandja done. Actually there was OAuth2ClientCredentialsGrantTests class with an integration test already. I updated it a bit to make it more like OAuth2AuthorizationCodeGrantTests. I also changed security configuration for it to be more realistic by overriding authentication entry point to return 401 when client is not authenticated and client_credentials grant is used.
jgrandja
jgrandja requested changes Jul 3, 2020
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @alek-sys! After the next update we should be ready to merge.
FYI, I'm out on PTO next Mon-Wed and back Thursday so no rush for the updates.
Thanks.
@alek-sys
Copy link
Contributor Author

alek-sys commented Jul 8, 2020

All updated based on your review @jgrandja. I only didn't add @Nullable since scope field is never null. Also, I noticed there is no logging anywhere in the project. Is that intentional or should I add at least debug logging to some of the components?
@jgrandja jgrandja added the status: duplicate A duplicate of another issue label Jul 13, 2020
jgrandja added a commit that referenced this pull request Jul 13, 2020
@jgrandja
Polish gh-88
ea828fb
@jgrandja
Copy link
Collaborator

jgrandja commented Jul 13, 2020

Thanks for adding this feature @alek-sys !

FYI, I followed up with a polish commit as some changes were needed and I wanted to get this merged.
Let me know if you have any questions regarding the changes in the polish commit.
@jgrandja jgrandja closed this Jul 13, 2020
jgrandja added a commit that referenced this pull request Oct 9, 2020
@jgrandja
Add @nullable to Delegating*Converter.convert()
f0013fc 
Issue gh-88, gh-45
mohammedBalhaddad pushed a commit to mohammedBalhaddad/spring-authorization-server that referenced this pull request Oct 12, 2020
@jgrandja @mohammedBalhaddad
Add @nullable to Delegating*Converter.convert()
531fb37 
Issue spring-projectsgh-88, spring-projectsgh-45
jgrandja added a commit that referenced this pull request Nov 3, 2020
@jgrandja
Polish gh-88
6c74864
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
@jgrandja
Polish spring-projectsgh-88
2aa0dfe
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
@jgrandja
Add @nullable to Delegating*Converter.convert()
d9b9c4f 
Issue spring-projectsgh-88, spring-projectsgh-45
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
@jgrandja
Polish spring-projectsgh-88
cf43f47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: duplicate A duplicate of another issue type: enhancement A general enhancement

2 participants

@alek-sys @jgrandja