-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix empty code parameter in CodeVerifierAuthenticator #1680
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.
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
Conversation
There was a problem hiding this 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 @aijazkeerio. Please see review comments.
@@ -140,7 +140,8 @@ private static boolean authorizationCodeGrant(Map<String, Object> parameters) { | |||
// @formatter:off | |||
return AuthorizationGrantType.AUTHORIZATION_CODE.getValue().equals( | |||
parameters.get(OAuth2ParameterNames.GRANT_TYPE)) && | |||
parameters.get(OAuth2ParameterNames.CODE) != null; | |||
parameters.get(OAuth2ParameterNames.CODE) != null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the grant type is authorization_code
and the code
is empty, it should not return false
and instead should error.
Try this instead:
if (!AuthorizationGrantType.AUTHORIZATION_CODE.getValue().equals(parameters.get(OAuth2ParameterNames.GRANT_TYPE))) {
return false;
}
if (!StringUtils.hasText((String) parameters.get(OAuth2ParameterNames.CODE))) {
throwInvalidGrant(OAuth2ParameterNames.CODE);
}
return true;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review new changes.
I have removed the formatter:on off, let me know if I should add it back.
@@ -282,6 +283,29 @@ public void authenticateWhenAuthorizationCodeGrantAndValidCredentialsThenAuthent | |||
assertThat(authenticationResult.getRegisteredClient()).isEqualTo(registeredClient); | |||
} | |||
|
|||
@Test | |||
public void authenticateWhenAuthorizationCodeGrantAndPkceAndValidCodeVerifierAndMissingCodeThenAuthenticated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this test and add OAuth2AuthorizationCodeGrantTests.requestWhenPublicClientWithPkceAndEmptyCodeThenBadRequest()
. You can use requestWhenPublicClientWithPkceThenReturnAccessTokenResponse()
as a template and adjust it.
Thanks for the updates @aijazkeerio ! This is now merged. FYI, I added a polish commit that was mostly formatting changes. |
Fixes #1671