Add support for Resource Owner Password Credentials grant #7013
Conversation
jgrandja
added
in: oauth2
jgrandja
force-pushed
the
gh-6003-pwd-grant
branch
from
a8a5f16 to
a916391
Compare
jgrandja
force-pushed
the
gh-6003-pwd-grant
branch
from
a916391 to
7f81fc6
Compare
jgrandja
force-pushed
the
gh-6003-pwd-grant
branch
from
7f81fc6 to
efaa1e1
Compare
jgrandja
force-pushed
the
gh-6003-pwd-grant
branch
from
efaa1e1 to
42f1e20
Compare
|
|
||
| ClientRegistration clientRegistration = context.getClientRegistration(); | ||
| OAuth2AuthorizedClient authorizedClient = context.getAuthorizedClient(); | ||
| if (!AuthorizationGrantType.PASSWORD.equals(clientRegistration.getAuthorizationGrantType()) || |
There was a problem hiding this comment.
Should it retrieve the OAuth2AuthorizedClient again if the token is expired?
There was a problem hiding this comment.
The RefreshTokenOAuth2AuthorizedClientProvider will take care of this as long as it's configured/composed within the DefaultOAuth2AuthorizedClientManager - which by default it is configured.
There was a problem hiding this comment.
If the grant type is PASSWORD doesn't it need to use a OAuth2PasswordGrantRequest to request a new token?
There was a problem hiding this comment.
It does use a OAuth2PasswordGrantRequest when calling the OAuth2AccessTokenResponseClient- see code just below this logic.
There was a problem hiding this comment.
Sorry that was in context of my previous comment. If the token expires shouldn't a new token be requested with a OAuth2PasswordGrantRequest
There was a problem hiding this comment.
If the token expires shouldn't a new token be requested with a OAuth2PasswordGrantRequest
No. For expired access tokens, the RefreshTokenOAuth2AuthorizedClientProvider handles this. The PasswordOAuth2AuthorizedClientProvider only handles getting a new access token (if client is not authorized)
There was a problem hiding this comment.
Actually, looks like I missed a condition here. I'll update shortly.
There was a problem hiding this comment.
@rwinch Thanks for catching this. I've pushed the updates.
| contextAttributes.put(OAuth2AuthorizationContext.REQUEST_SCOPE_ATTRIBUTE_NAME, | ||
| StringUtils.delimitedListToStringArray(scope, " ")); | ||
| } | ||
| String username = authorizeRequest.getServerWebExchange().getRequest().getQueryParams().getFirst(OAuth2ParameterNames.USERNAME); |
There was a problem hiding this comment.
I think this will only get the username if the username is a query parameter, but not if it's a form parameter.
There was a problem hiding this comment.
@jzheaux Yes you are right. Thanks for catching this! Form parameters are read from ServerWebExchange().getFormData(), which returns a Mono<MultiValueMap<String, String>>. So I had to change the API for DefaultServerOAuth2AuthorizedClientManager.contextAttributesMapper to Function<ServerOAuth2AuthorizeRequest, Mono<Map<String, Object>>> (return a Mono).
I don't think it's a good idea to read the request body for each request mapping by default, so I removed that code. The default behaviour for DefaultContextAttributesMapper should read supported query parameters only.
NOTE: I did add a test that shows how a custom contextAttributesMapper can map form parameters.
| contextAttributes.put(OAuth2AuthorizationContext.REQUEST_SCOPE_ATTRIBUTE_NAME, | ||
| StringUtils.delimitedListToStringArray(scope, " ")); | ||
| } | ||
| String username = authorizeRequest.getServletRequest().getParameter(OAuth2ParameterNames.USERNAME); |
There was a problem hiding this comment.
I figured because of this comment: https://github.com/spring-projects/spring-security/pull/7013/files#diff-b1268bb315785679ff599fe121c6a23aR142 this mapping would be removed from both the servlet bits as well as the reactive bits. This PR appears to remove it from reactive but not servlet.
|
Merged via dcd997e |
Fixes #6003