♻️ Updating HTTP auth error codes to 401 while complying to RFC standards #12507
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added a default value for realm, to satisfy the requirements of basic. Also changed some of the 403 codes to 401 since 403 was not fitting for that scenario.
Changed some of the tests to match the more correct formating of the HTTP-schemas. Then matched the test cases for the new error messages and new error codes.
alejsdev
changed the title
Contributor
|
This pull request has a merge conflict that needs to be resolved. |
github-actions
bot
added
the
conflicts
Member
|
Hi @mrPaintMan, thanks for the detailed and thoughtful PR! We've had quite a few open PRs for this issue, but have recently decided that we'll likely go with #13786, which provides an additional fall-back mechanism for users that need more time to update (as this is a breaking change). As such, I'll go ahead and close this one. If you feel like it, always welcome to help review #13786 ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR This PR aims to change default error code for HTTPBearer and HTTPDigest to from 403_Forbidden to 401_Unauthorized.
Goal of this PR
This PR aims to address the issues of having status code 403_forbidden for the HTTPBearer and HTTPDigest classes, and change them to 401_Unauthorized. From what we understand, the reason for why this hasn't been fixed already is what @tiangolo states in his comment in PR #2120. This PR aims to comply to the error handling described in RFC standards 6750 (Bearer auth), 7616 (Digest auth) and 7617 (Basic auth).
This PR does not aim to provide a working implementation of the Bearer or the Digest authentication schemas, but rather to improve the existing error handling.
Changed behavior
In order to comply to these standards, we have introduced a default value (the value
global) to therealmchallenge of theWWW-Authenticateheader, since it MUST be present for Basic auth, and MAY be present for Bearer and Digest auth. On top of this, Bearer auth MUST have at least one challenge in theWWW-Authenticateheader, but no specific challenge is required, thus making the realm challenge suitable for this case as well.Also, the challenge
qop(quality of protection) MUST be present on an erroneous request to an endpoint protected by Digest auth. We solved this by defaulting this value toauthsince we interpreted that this is the default value according to the standard.Additional changes
qopandrealmto the openapi docs according to how other components are handledrealmfor Basic, Bearer and Digest authqopfor Digest authDisclosure
We have very little experience with contributing to open source projects, and are very open to suggestions and feedback on this work!