Add support for client_credentials grant #5627
Conversation
jgrandja
force-pushed
the
gh-4982-client-creds-grant
branch
from
6ccf046 to
c2c054a
Compare
|
|
||
| ClientRegistration clientRegistration = clientCredentialsGrantRequest.getClientRegistration(); | ||
|
|
||
| // Headers |
There was a problem hiding this comment.
This is a pretty large method. I think each of these block with a comment above them make sense to be in a separate private method
| public OAuth2AuthorizedClientArgumentResolver(OAuth2AuthorizedClientRepository authorizedClientRepository) { | ||
| public OAuth2AuthorizedClientArgumentResolver(ClientRegistrationRepository clientRegistrationRepository, | ||
| OAuth2AuthorizedClientRepository authorizedClientRepository) { | ||
| Assert.notNull(clientRegistrationRepository, "clientRegistrationRepository cannot be null"); |
There was a problem hiding this comment.
Is it true that users will always need a ClientRegistrationRepository or is that only necessary for Client Credentials? If it is not always required, I prefer to overload the constructor with optional parameters last
There was a problem hiding this comment.
I'm even wondering if makes sense to have a separate resolver for the client credentials? If nothing else, the resolveArgument should probably be broken up into more than one method (and likely by the grant type)
There was a problem hiding this comment.
ClientRegistrationRepository is required as I need to determine the clientRegistration.getAuthorizationGrantType() before I can delegate to the correct logic.
There was a problem hiding this comment.
I'm even wondering if makes sense to have a separate resolver for the client credentials? If nothing else, the resolveArgument should probably be broken up into more than one method (and likely by the grant type)
The logic for authorization_code is this:
if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) {
throw new ClientAuthorizationRequiredException(clientRegistrationId);
}
I don't see much value in breaking this up into different methods? Do you find this unreadable?
There was a problem hiding this comment.
I do find it unreadable. As soon as you start adding block comments that should be an indicator that the method is too large. Instead change the block into a method that reads like the block comment.
There was a problem hiding this comment.
I think you meant this comment for the getTokenResponse() method? I am breaking that up.
|
This has been rebased and merged in via 9527432 |
rwinch
added
New Feature
in: oauth2
Fixes gh-4982