search

bshaffer / oauth2-server-php

A library for implementing an OAuth2 Server in php
http://bshaffer.github.io/oauth2-server-php-docs
MIT License
3.26k stars 950 forks source link

Modified response code 400 to 401 #962

Open whvandervelde opened 5 years ago

whvandervelde commented 5 years ago

When passing invalid client credentials (either client_id or client_secret) when requesting an access token, instead of returning a 400 response code, it should be returning a 401 response code for unauthorized. Limited it to the case where all conditions (public/non public etc) are passed and its only about the credentials. TokenControllerTest modified accordingly.

CameronGo commented 4 years ago

Just uncovered this in our environment today. Any idea when this PR may get merged or why not?

CameronGo commented 4 years ago

Just saw the note from another issue indicating that the 400 response is the desired and correct response code for this scenario. https://github.com/bshaffer/oauth2-server-php/pull/846

I'm not an expert in this, but I believe the RFC allows for a 401 response here based on this note: https://tools.ietf.org/html/rfc6749#section-6

Then here (which I realize is not the RFC) specifically states that an invalid client ID or client secret will return a 401: https://www.oauth.com/oauth2-servers/access-tokens/access-token-response/

whvandervelde commented 4 years ago

Hey, wow this has been a while ago ;-) Yes, this is specifically (only) done for Client Credentials where I think 401 would be more appropriate, as per your reference of the RFC. On a general note I think it just makes sense on what a 401 should be used for vs a 400 and to be able to provide better error handling for anybody using this.

There was some discussion where 400 was the preferred choice with User Credentials because 401 requires a challenge in the response, see #924. What you refer too with #846 is for a refresh_token, that probably also still would be a 400 preferred?

whvandervelde commented 4 years ago

The reason I prefer a 401 here is because we want to use a specific flow for a client in case of supplying invalid credentials and we don't want to tie that to a specific implementation's error message content.

Spomky commented 4 years ago

The error 400 is the correct error code as per the RFC. There is no reason to change it.

CameronGo commented 4 years ago

@Spomky - i'm not saying the 400 response is wrong per the RFC, but from the links provided it seems to be the RFC allows for a 401 as well. The additional link I provided shows an example from oauth.com where they in fact are returning a 401 in the specific scenario described here.

So while I don't think it has to be changed, it seems to me that it could be changed and still be correct and would be helpful for anyone attempting to code specifically for invalid credentials, which seems like a totally reasonable thing to do.