Skip to content
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

Respond with helpful and spec complient error on invalid user credentials #1230

Merged
merged 2 commits into from
Jul 10, 2021

Conversation

marc-mabe
Copy link
Contributor

@marc-mabe marc-mabe commented May 21, 2021

See #967 #1175 #1093

This PR tries to solve the non helpful error message if a user types in wrong credentials in a a much smaller scope then done in #1093 and hopefully can be merged and released much faster.

In #967 the error type should be invalid_grant (instead of invalid_credentials) and the HTTP status code should be 400 instead of 401 but this also had changed the error message from The user credentials were incorrect. into The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. which is not user friendly at all.

Instead of calling invalidGrant() (as done in #967) this PR calls invalidCredentials() but modifies the logic of invalidCredentials() to return invalid_grant with 400.

Interestingly invalidCredentials() wasn't used anymore since #967.

TODOs

  • Update tests

@Sephster
Copy link
Member

Hi @marc-mabe - seems like a good suggestion. Happy to approve this once the tests are fixed. Thank you

@marc-mabe
Copy link
Contributor Author

@Sephster done

@Sephster
Copy link
Member

LGTM. Thanks @marc-mabe

@Sephster Sephster merged commit 0c86312 into thephpleague:master Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants