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

Return most /authorize errors as a redirect #71

Merged
merged 3 commits into from
Feb 7, 2023

Conversation

mgorven
Copy link
Contributor

@mgorven mgorven commented Feb 2, 2023

The spec defines that most errors to /authorize should be returned to the redirect URI with error and error_description query params:

If the resource owner denies the access request or if the request
fails for reasons other than a missing or invalid redirection URI,
the authorization server informs the client by adding the following
parameters to the query component of the redirection URI using the
"application/x-www-form-urlencoded" format, per Appendix B:

Add InvalidRedirectURIError and use InvalidClientError for all client_id errors since these should not redirect. Update
catch_errors_and_unavailability to optionally return errors as redirects.

The spec defines that most errors to `/authorize` should be returned to
the redirect URI with `error` and `error_description` query params:

    If the resource owner denies the access request or if the request
    fails for reasons other than a missing or invalid redirection URI,
    the authorization server informs the client by adding the following
    parameters to the query component of the redirection URI using the
    "application/x-www-form-urlencoded" format, per Appendix B:

Add `InvalidRedirectURIError` and use `InvalidClientError` for all
`client_id` errors since these should not redirect. Update
`catch_errors_and_unavailability` to optionally return errors as
redirects.
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #71 (54682ab) into master (ac311d9) will decrease coverage by 0.29%.
The diff coverage is 95.12%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
- Coverage   99.66%   99.38%   -0.29%     
==========================================
  Files          14       14              
  Lines         602      650      +48     
  Branches       86       96      +10     
==========================================
+ Hits          600      646      +46     
- Misses          1        2       +1     
- Partials        1        2       +1     
Impacted Files Coverage Δ
aioauth/utils.py 97.77% <93.10%> (-2.23%) ⬇️
aioauth/errors.py 100.00% <100.00%> (ø)
aioauth/grant_type.py 97.50% <100.00%> (ø)
aioauth/response_type.py 100.00% <100.00%> (ø)
aioauth/server.py 100.00% <100.00%> (ø)
aioauth/config.py 100.00% <0.00%> (ø)
aioauth/models.py 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aliev
Copy link
Owner

aliev commented Feb 3, 2023

Hi @mgorven ! Thank you for your great catch and your PRs. I'll take a look at them today.

@aliev aliev merged commit 27c3715 into aliev:master Feb 7, 2023
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.

None yet

3 participants