Error Response is not OAuth2 compliant

[thuethe]

On Issuing an Access Token the OAuth 2.0 Server produces an error response like:

{
    "error": "invalid_client",
    "message": "Client authentication failed"
}

with optional hint in some cases.

Specs compliant would be an error response like:

{
    "error": "invalid_client",
    "error_description": "Client authentication failed",
    "error_uri": "..."
}

error_uri and error_description are optional.

From the specs (https://tools.ietf.org/html/rfc6749#section-5.2):

The authorization server responds with an HTTP 400 (Bad Request) status code (unless specified otherwise) and includes the following parameters with the response: error REQUIRED. [...] error_description OPTIONAL. Human-readable ASCII [USASCII] text providing additional information, used to assist the client developer in understanding the error that occurred. Values for the "error_description" parameter MUST NOT include characters outside the set %x20-21 / %x23-5B / %x5D-7E. error_uri OPTIONAL. A URI identifying a human-readable web page with information about the error, used to provide the client developer with additional information about the error. Values for the "error_uri" parameter MUST conform to the URI-reference syntax and thus MUST NOT include characters outside the set %x21 / %x23-5B / %x5D-7E.

[temp]

Also, the state query parameter ist missing in error responses. The RFC states:

state
         REQUIRED if a "state" parameter was present in the client
         authorization request.  The exact value received from the
         client.

[Sephster]

Thanks both. I will aim to look at this this evening.

[lordrhodos]

Also, the state query parameter ist missing in error responses. The RFC states:

state
        REQUIRED if a "state" parameter was present in the client
       authorization request.  The exact value received from the
        client.

just to clarify, this seems to apply only to the following grant types:

• Authorization Code Grant (https://tools.ietf.org/html/rfc6749#section-4.1.2.1)
• Implicit Grant (https://tools.ietf.org/html/rfc6749#section-4.2.2.1)

The error responses for the following grant types do not require the stateparameter:

• Resource Owner Password Credentials Grant
• Client Credentials Grant

(both link to section 5.2 in the spec for the error response)

[Spomky]

@lordrhodos tha’s right. Broadly speaking, the state parameter is should be part of any response (success or failure) from the authorization endpoint. This is true for all response types ; not only the Authorization Code Grant and Implicit Grant but also the ones defined by other specification (e.g. OpenID Connect and its id_token or none response types)

[lordrhodos]

@Spomky speaking spec language is an own skill I guess 😉

From what I understand this library does not support OIDC (and its erlated response types), nevertheless this should be fixed. What I wonder is how the inclusion of thestate query parameter should be handled for this library?

@Sephster do you have any opinion about it? Currently the OAuthServerException does not know about a state parameter or AuthorizationRequest. Should a PR add an optional $state parameter to \OAuth2\Server\Exception\OAuthServerException::__constructdefaulting tonull` or is there another favored approach?

[Sephster]

Yeah you are right that we don't currently implement OIDC so this shouldn't be a concern for us and we should just adhere to the already implemented RFCs. At the moment the exceptions are static so I think it would be best if we just pass state in as an argument to the relevant method when applicable and make this argument optional.

[Sephster]

@marc-mabe had submitted a PR to fix the error_description which I had initially accepted but I think it will likely be reverted. Adding my comment here so others can see and generate some discussion:

@marc-mabe I was going to create a release for this but I'm now thinking this isn't the right solution.

The error_description is applicable for authorization requests from the implicit grant and auth code grant.

At the moment I think we do need to have a way to optionally specify the error_description but if we want to make this spec compliant, I don't think moving to replace the existing message is the way to go about this.

On the surface, it looked like someone had put the property in as a wrong name but on closer inspection, I think this is something custom to this library to assist developers.

I would appreciate your thoughts on this but given this, I will likely revert this PR.

[marc-mabe]

@Sephster I don't fully get your concerns

From your comment:

I think this is something custom to this library to assist developers.

spec

error_description OPTIONAL. Human-readable ASCII [USASCII] text providing additional information, used to assist the client developer in understanding the error that occurred. Values for the "error_description" parameter MUST NOT include characters outside the set %x20-21 / %x23-5B / %x5D-7E.

What does the current message be different then the error_description ?

[Sephster]

Sorry @marc-mabe I need to do a bit more reading on this. I initially thought that error_description was only used by error responses from the implicit grant and auth code grant as it is explicitly mentioned there but I can see in section 5.2 it is also listed as being used in the password grant etc.

I need to take a step back and check all the places this change will apply but now expect I won't revert this as my previous assertion wasn't correct.

Please bear with me while I look into this further :+1:

[Sephster]

Looks good to me. It is being used by all grants so happy to leave in. I will roll out a release shortly. Thank you

[Sephster]

Need to force error_description for version 9 and add in state response to the exception if present to close this issue off.