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

Invalid HTTP statuses on error response #264

Closed
prudnikov opened this issue Sep 15, 2014 · 11 comments
Closed

Invalid HTTP statuses on error response #264

prudnikov opened this issue Sep 15, 2014 · 11 comments

Comments

@prudnikov
Copy link

RFC 6749 Section 5.2 (http://tools.ietf.org/html/rfc6749#section-5.2) describes error response. It says that server should respond with HTTP 400 (Bad request). But oauthlib uses HTTP 401 (Unauthorized) instead for errors such as InvalidClientError, InvalidGrantError, UnauthorizedClientError and other related errors.

For example https://github.com/idan/oauthlib/blob/master/oauthlib/oauth2/rfc6749/errors.py#L181

@ib-lundgren
Copy link
Collaborator

The spec mention that 401 may be used with invalid client errors and must be used when the authentication was done via the authorization header field. Although oauthlib currently does nothing to suggest that the 'WWW-Authenticate' header field should be included in the response as the spec require. I reckon keeping 401 for this error is ok but we should add a header suggestion to the exception.

Regarding the other the spec does not explicitly say that 401 may be used and thus we should probably consider changing to 400. However it does seem more appropriate with a 401 in both invalid grant and especially unauthorized client errors.

@asteinlein, @lepture, @masci thoughts?

@samskiter
Copy link

@ib-lundgren Hi - just stumbled into this very issue and would like to chime in. From reading the spec and looking around other answers, IMO a 400 is the correct response.

Firstly, the spec:

The authorization server responds with an HTTP 400 (Bad Request)
status code (unless specified otherwise) and includes the following
parameters with the response:

There's no MUST in the 400 requirements, but there's no MAY either.

Secondly, and the reason I stumbled into this was through a bug of sorts in the sun JDK on my client (see here: http://stackoverflow.com/a/7524681/1137254 )

Essentially, my client's HTTP stack would close the connection early for a 401, but the OAuth stack above (google-http-oauth-client) would attempt to continue reading and parse the results. I think this was written into OpenJDK per the definition and expectations around a 401 (UPDATE: it's actually to do with 'streaming' mode). IMO OpenJDK is wrong too and I think I'd hit this same issue if I were to get an "invalid_client" 401 during /token endpoint requests.

@jmbowman
Copy link

Adding another vote for 400 being the correct error code here. We used to use django-oauth2-provider for oauth2 authentication from mobile apps, and it returned 400 in this case. When switching to django-oauth-toolkit (which uses oauthlib) in order to upgrade to newer Django versions, we had to hack the token request view to return a 400 instead to keep the mobile apps working correctly.

Here's one of the authors of the spec specifically stating that a 400 is the intended response for invalid authentication credentials when requesting a token, and why: https://www.ietf.org/mail-archive/web/oauth/current/msg02127.html

And here's another example of a project that's having trouble coping with oauthlib using a 401 in these circumstances.

@LennyLip
Copy link

LennyLip commented Apr 2, 2016

+1 for 400

@rinatio
Copy link

rinatio commented Feb 9, 2018

Vote for 400

@JonathanHuot
Copy link
Member

I'm reviving this old thread & start a PR with the changes suggested above.

@JonathanHuot
Copy link
Member

Before making progress on this, I'd like to summarize the discussion which happen in the past.

  1. invalid_client, where it MUST be 401 if HTTP authentication is used, else it MAY be 401.
  2. invalid_token, where it SHOULD be 401.

In any cases, if 401 is used, WWW-Authenticate MUST be set (RFC2616).

Other OAuth2.0 implementations

A quick overview of the most common OAuth2.0 implementations are similar to the overall consensus:

Conlusion

We have to align with the RFC, however to not break any implementations, we must not include these changes in 2.x.x version, but wait until 3.x.x

Please, feel free to chim in.

@asteinlein
Copy link
Contributor

I cannot remember the reason/the client(s) that made me push PR #247, but I see and understand the reasons for reverting this. Thanks for the awesome research @JonathanHuot, and doing this in 3.x sounds like the best approach.

@JonathanHuot
Copy link
Member

Fixed in #561 / oauthlib>= 3.0.0

@esfandiaryfard
Copy link

according to this RFC that mentioned above invalid client also should return status 400 but in package still is 401

@JonathanHuot
Copy link
Member

Hi @esfandiaryfard, could you point the exact text of the RFC ? Thanks!

pomegranited added a commit to edx-olive/edx-platform-old that referenced this issue Jun 5, 2019
https://github.com/edx-olive/edx-platform/compare/38577488cb8c9335f73456939d480b19fdd92f3c...239cb23502a8616d826acdde50bc4dad70aa7b2b

And uses upstream (constrained) versions of requests-oauthlib and oauthlib

Bumping requests-oauthlib to 1.2 updates oauthlib, which changes a response
code in certain cases, and causes a test to fail.

See oauthlib/oauthlib#264 for more context
ushkarev added a commit to ministryofjustice/money-to-prisoners-api that referenced this issue Nov 10, 2021
Updated `oauthlib` now returns a 400 error instead of 401 for invalid grants (ref: oauthlib/oauthlib#264).
For our purposes, we don't care which type of error it is as long as it is a definitive error.
ushkarev added a commit to ministryofjustice/money-to-prisoners-api that referenced this issue Nov 10, 2021
Previously, a 401 was returned when invalid credentials were used with the Resource Owner Password Credentials Grant flow.
See oauthlib/oauthlib#264 and oauthlib/oauthlib#619
Versions 3+ return 400 making it tougher to distinguish between incorrect passwords and other errors.
This patch reverts the status code back to 401, but only in the case where credentials were incorrect.
ushkarev added a commit to ministryofjustice/money-to-prisoners-api that referenced this issue Nov 11, 2021
Previously, a 401 was returned when invalid credentials were used with the Resource Owner Password Credentials Grant flow.
See oauthlib/oauthlib#264 and oauthlib/oauthlib#619
Versions 3+ return 400 making it tougher to distinguish between incorrect passwords and other errors.
This patch reverts the status code back to 401, but only in the case where credentials were incorrect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants