Skip to content

OAuth2AuthorizationCodeGrantWebFilter should handle OAuth2AuthorizationException #8609 #8616

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

Closed
wants to merge 1 commit into from

Conversation

shazin
Copy link
Contributor

@shazin shazin commented May 30, 2020

The issue is in OAuth2AuthorizationCodeGrantWebFilter as it does not handle OAuth2AuthorizationException, which can be thrown by ServerOAuth2AuthorizationCodeAuthenticationTokenConverter.

The fix should be in OAuth2AuthorizationCodeGrantWebFilter to handle OAuth2AuthorizationException and delegate to authenticationFailureHandler on errors.

Fixes gh-8609

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 30, 2020
Copy link
Contributor

@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 @shazin. Please see review comment.

Also, please add test(s) as this is required for all changes.

@@ -184,6 +186,8 @@ private void updateDefaultAuthenticationConverter() {
return this.authenticationManager.authenticate(token)
.switchIfEmpty(Mono.defer(() -> Mono.error(new IllegalStateException("No provider found for " + token.getClass()))))
.flatMap(authentication -> onAuthenticationSuccess(authentication, webFilterExchange))
.onErrorResume(OAuth2AuthorizationException.class, e -> this.authenticationFailureHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the correct place for the fix. OAuth2AuthorizationException is not thrown by AuthenticationManager, instead it's thrown by authenticationConverter.convert(exchange) (ServerOAuth2AuthorizationCodeAuthenticationTokenConverter)

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 2, 2020
@jgrandja jgrandja removed the type: bug A general bug label Jun 9, 2020
@jgrandja
Copy link
Contributor

jgrandja commented Jun 9, 2020

@shazin I'm closing this PR as I just pushed the fix in these commits 4c902bb da4b626

@jgrandja jgrandja closed this Jun 9, 2020
@jgrandja jgrandja added the status: declined A suggestion or change that we don't feel we should currently apply label Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth2AuthorizationCodeGrantWebFilter should handle OAuth2AuthorizationException
3 participants