Skip to content

WebFlux oauth2Login returns 500 when bad client credentials #5562

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
jgrandja opened this issue Jul 24, 2018 · 7 comments
Closed

WebFlux oauth2Login returns 500 when bad client credentials #5562

jgrandja opened this issue Jul 24, 2018 · 7 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@jgrandja
Copy link
Contributor

jgrandja commented Jul 24, 2018

When WebFlux oauth2Login is configured with 1 ClientRegistration that has bad client credentials, a 500 response will occur during the processing of the Authorization Response when attempting to exchange the code for the access_token. The parameters from the authorization response are also viewable in the browser location bar.

We should ensure a redirect to the default login page to display the error message, for example, [invalid_client] Unauthorized.

@jgrandja jgrandja added type: bug A general bug Reactive in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Jul 24, 2018
@jgrandja jgrandja added this to the 5.1.0.M2 milestone Jul 24, 2018
@rwinch rwinch modified the milestones: 5.1.0.M2, 5.1.0.RC1, 5.1.0 Jul 26, 2018
@rwinch
Copy link
Member

rwinch commented Aug 21, 2018

In my opinion this is a this is a configuration error that should result in a 500. Why should we do this?

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Aug 21, 2018
@jgrandja
Copy link
Contributor Author

See invalid_client

@rwinch
Copy link
Member

rwinch commented Aug 21, 2018

@jgrandja I'm not sure I understand. All I see is what the authorization server should do. This code is discussing changes to the client.

@jgrandja
Copy link
Contributor Author

@rwinch I agree that it's a configuration error but we should at least help the user by displaying the error message coming back from the Authorization Server so they have some indication on where to resolve the issue.

Here are the results when I tried the Servlet-based oauth2Login sample with an invalid client_secret configured:

Google

google-token-response-1

google-token-response-2

Okta

okta-token-response-1

okta-token-response-2

@rwinch
Copy link
Member

rwinch commented Aug 22, 2018

Thanks for the response. This does seem like a reasonable improvement, but I don't see this as broken. I'd guess that URL includes the same error information in it so the user should be able to figure it out.

@rwinch rwinch modified the milestones: 5.1.0, 5.1.1 Sep 17, 2018
@rwinch rwinch modified the milestones: 5.1.1, 5.1.2 Oct 12, 2018
@jzheaux
Copy link
Contributor

jzheaux commented Nov 26, 2018

I think that there is value in bringing the reactive experience into parity with the servlet experience. It would be nice if both redirected to "/login?error" (servlet-based oauth2Login already does this).

To achieve this, though, there are some more basic features, as I see it. LoginPageGeneratedWebFilter does not yet support any way to pull an error message from the session or the request. Also, RedirectServerAuthenticationFailureHandler does not save any error message to the session or request.

@rwinch @jgrandja do these two features seem reasonable for Reactive applications? If so, I'll create tickets for those.

@rwinch rwinch modified the milestones: 5.1.2, 5.1.3 Nov 28, 2018
@rwinch
Copy link
Member

rwinch commented Nov 29, 2018

It is tricky because we don't want to reveal too much information about the log in failure. In general, explaining too much information (i.e. the username does not exist, the password is invalid, etc) is not recommended.

If there is a failure in the systems, then I think the general error page should be used rather than our default log in page trying to handle it.

@jzheaux jzheaux modified the milestones: 5.1.3, 5.1.4 Jan 11, 2019
@jgrandja jgrandja modified the milestones: 5.1.4, 5.1.5 Feb 13, 2019
@jgrandja jgrandja modified the milestones: 5.1.5, 5.1.6 Apr 2, 2019
@rwinch rwinch removed the Reactive label May 6, 2019
@jgrandja jgrandja modified the milestones: 5.1.6, 5.1.x May 21, 2019
@jgrandja jgrandja removed the status: waiting-for-feedback We need additional information before we can continue label Jul 10, 2019
@rwinch rwinch removed their assignment Jul 29, 2019
@jgrandja jgrandja self-assigned this Nov 26, 2019
@jgrandja jgrandja modified the milestones: 5.1.x, 5.3.0.M1 Dec 5, 2019
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x labels Dec 5, 2019
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: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants