-
-
Notifications
You must be signed in to change notification settings - Fork 495
Handle 401 with WWW-Authenticate. Moved wrong 401 into 400. #623
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
Conversation
access_denied/unauthorized_client/consent_required/login_required MUST be 400, and not 401. Also, 401 MUST have WWW-Authenticate when set. It could have an impact of processing those in webframeworks.
It misses the possibility to add scope= and realm= at the moment, but it should be a step forward into the right direction.
@@ -63,7 +63,7 @@ def create_introspect_response(self, uri, http_method='POST', body=None, | |||
log.debug('Token introspect valid for %r.', request) | |||
except OAuth2Error as e: | |||
log.debug('Client error during validation of %r. %r.', request, e) | |||
return {}, e.json, e.status_code | |||
return e.headers, e.json, e.status_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be headers.update(e.headers)
instead and include e.g. Content-Type
?
Also, just checking that this should return the error
and error_description
now in both the response header and body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, actually we could take this PR opportinity to add the Content-Type to the headers. It's also applicable for revocation endpoint. It was missing for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For error
in both WWW-Authenticate
and HTTP body, I have not seen any hard requirements, but the RFC specifies that 401 can be used, and error
must be in the response's body. It doesn't said explicitly that both can be used in same time, though..
Introspect RFC:
If the protected resource uses OAuth 2.0 client credentials to
authenticate to the introspection endpoint and its credentials are
invalid, the authorization server responds with an HTTP 401
(Unauthorized) as described in Section 5.2 of OAuth 2.0 [RFC6749].
OAuth2 5.2 section:
The authorization server responds with an HTTP 400 (Bad Request)
status code (unless specified otherwise) and includes the following
parameters with the response:
error
invalid_client
(..). The authorization server MAY
return an HTTP 401 (Unauthorized) status code to indicate
which HTTP authentication schemes are supported. If the
client attempted to authenticate via the "Authorization"
request header field, the authorization server MUST
respond with an HTTP 401 (Unauthorized) status code and
include the "WWW-Authenticate" response header field
matching the authentication scheme used by the client.
(..)
The parameters are included in the entity-body of the HTTP response
using the "application/json" media type as defined by [RFC4627]. (..)
And then Bearer RFC6750:
3. The WWW-Authenticate Response Header Field
(..)
All challenges defined by this specification MUST use the auth-scheme
value "Bearer". This scheme MUST be followed by one or more
auth-param values.
(..)
If the protected resource request included an access token and failed
authentication, the resource server SHOULD include the "error"
attribute to provide the client with the reason why the access
request was declined.
I think this 401/Authenticate topic deserves changes more in-depth, and this PR could be a short term solution to progress forward.
In the future, we should handle all areas of the (at least) three RFCs, mostly because we don't support everything (bearer authentication vs client authentication; sending the authentication scheme used by the client
; scope
/realm
; and so on and so forth)
To summarize, the examples in Bearer's RFC shows no HTTP body, the OAuth2's RFC
requires the error in the HTTP body, the Introspect's RFC mentions both Bearer and OAuth2 RFCs.
Current OAuthlib-2 send errors in the HTTP body, what should do the next-gen OAuthlib-3 ? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
OAuth2 Provider errors
access_denied
/unauthorized_client
/consent_required
/login_required
MUST be 400, and not 401.Also, the remaining 401 (
invalid_client
andinvalid_token
) MUST have WWW-Authenticate when set. It could have an impact of processing those in webframeworks.Require an understanding of web frameworks (django, flask, ...) but also how themselves are implemented. Any feedback on this is more than welcome.