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

Replace UnauthorizedRequestError with InvalidRequestError #69

Open
Uzlopak opened this issue Nov 15, 2021 · 6 comments
Open

Replace UnauthorizedRequestError with InvalidRequestError #69

Uzlopak opened this issue Nov 15, 2021 · 6 comments
Labels
compliance 📜 OAuth 2.0 standard compliance hacktoberfest tests 🧪 Relates to tests

Comments

@Uzlopak
Copy link
Collaborator

Uzlopak commented Nov 15, 2021

The UnauthorizedRequestError is not a standard error code. According to the reference in the comment https://datatracker.ietf.org/doc/html/rfc6750#section-3.1 there is no unauthorized_request error.

UnauthorizedRequestError is used in the AuthenticateHandler for indicating that there was no token in body and header.
According to the Spec it should be an InvalidRequestError, as the token is clearly a missing parameter.

The request is missing a required parameter, includes an
unsupported parameter or parameter value, repeats the same
parameter, uses more than one method for including an access
token, or is otherwise malformed. The resource server SHOULD
respond with the HTTP 400 (Bad Request) status code.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 15, 2021

Thus, the correct ErrorCode for missing token is 400 and not 401

@jankapunkt
Copy link
Member

Hm but there is also

invalid_token
The access token provided is expired, revoked, malformed, or
invalid for other reasons. The resource SHOULD respond with
the HTTP 401 (Unauthorized) status code. The client MAY
request a new access token and retry the protected resource
request.

I think the part invalid for other reason is interpreted as "missing". However, I agree that simply missing the token is rather a bad request than the token being invalid. Especially from a machine-perspective the distinciton has to be precise. A token that does not exist cannot be valid or invalid.

Is there any discussion on this in the original repo? This would be a breaking change, right?

Also we need to identify the places in the code and tests to be updated accordingly.

// cc @jwerre @HappyZombies

@jankapunkt jankapunkt added compliance 📜 OAuth 2.0 standard compliance tests 🧪 Relates to tests labels Nov 16, 2021
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 16, 2021

Good argument, but also this would mean, that we would not need that UnauthorizedRequestError but invalid_token with 401.

@jwerre
Copy link
Contributor

jwerre commented Nov 16, 2021

Both are good points. Just out of curiosity I've hit a couple of well know APIs without an access token:

curl -I https://api.github.com/user
HTTP/2 401
curl -I https://api.stripe.com/v1/charges
HTTP/2 401
www-authenticate: Basic realm="Stripe"
curl -I https://api.twilio.com/2010-04-01/Accounts
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Basic realm="Twilio API"

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Nov 16, 2021

I tested the three above mentioned resource servers and only stripe made a RFC6750 conform response.

Return Value of the resource server is:

{
  "error": {
    "message": "You did not provide an API key. You need to provide your API key in the Authorization header, using Bearer auth (e.g. 'Authorization: Bearer YOUR_SECRET_KEY'). See https://stripe.com/docs/api#authentication for details, or we can help at https://support.stripe.com/.",
    "type": "invalid_request_error"
  }
}

InvalidRequestError ^^

@jwerre
Copy link
Contributor

jwerre commented Nov 16, 2021

So it should be 401:invalid_request_error. Do you concur @jankapunkt @HappyZombies ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compliance 📜 OAuth 2.0 standard compliance hacktoberfest tests 🧪 Relates to tests
Projects
None yet
Development

No branches or pull requests

3 participants