Skip to content

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

Merged
merged 5 commits into from
Dec 13, 2018

Conversation

JonathanHuot
Copy link
Member

@JonathanHuot JonathanHuot commented Dec 4, 2018

OAuth2 Provider errors access_denied/unauthorized_client/consent_required/login_required MUST be 400, and not 401.

Also, the remaining 401 (invalid_client and invalid_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.

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.
@JonathanHuot JonathanHuot added this to the 3.0.0 milestone Dec 4, 2018
@JonathanHuot JonathanHuot added Breaking Breaking change, to go in the next major release. OAuth2-Provider This impact the provider part of OAuth2 labels Dec 4, 2018
@JonathanHuot JonathanHuot mentioned this pull request Dec 4, 2018
23 tasks
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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

@JonathanHuot JonathanHuot Dec 13, 2018

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 ? :-)

Copy link
Member

@skion skion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@JonathanHuot JonathanHuot merged commit 5d9a9c9 into master Dec 13, 2018
@JonathanHuot JonathanHuot deleted the 264-status401 branch December 13, 2018 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Breaking change, to go in the next major release. OAuth2-Provider This impact the provider part of OAuth2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants