Client.parse_response misinterprets success as failure

[dawid2193487]

I am attempting to do an access token request:

    resp = openid.client.do_access_token_request(
      state=aresp["state"],
      request_args={
        "code": aresp["code"],
      },
      authn_method="client_secret_basic"
    )

The request proceeds, gets to the openid server, and gets back this response from Keycloak. This is a successful response - and what I expect to receive back

{
    "access_token": "<snip>",
    "error": null,
    "error_description": null,
    "error_uri": null,
    "id_token": "<snip>",
    "not-before-policy": 0,
    "scope": "openid email profile",
    "session_state": "<snip>",
    "token_type": "Bearer"
}

Once parse_response is executed:

parse_response (__init__.py:655)
parse_request_response (__init__.py:764)
request_and_return (__init__.py:823)
do_access_token_request (__init__.py:928)
do_access_token_request (__init__.py:704)
finalize_route (<snip>\routes\openid_api.py:28)
<flask stack>

It does these steps:

1. L640: Deserialize the response
2. L646: Check if there's a "error" key in resp and if resp is NOT an ErrorResponse type
   Both of these evaluate to true. There's an "error" key that's equal to None, and resp is a perfectly valid AccessTokenResponse.
3. L647: Therefore, set resp to None
4. L657: Try casting a valid AccessTokenResponse into an ErrorResponse
5. L658: Raise an exception, since a non-error is not an error.
6. L691: Raise ResponseError("Missing or faulty response") despite a non-missing, non-faulty response

Looking at the RFC, it seems like Keycloak isn't doing anything wrong. https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2

[tpazderka]

Eh, this looks like a grey area to me...

The response you have posted is definitely not a valid ErrorResponse, but to me, it also doesn't look like a valid AccessTokenResponse either...

The specs do say to ignore extra fields that are not recognised, but error is/should be recognised in this context.

Also, the specs suggest (I think) to omit fields that are empty (in this case the error*).

@schlenk Your opinion?

[schlenk]

The OAuth2 RFC 6749 is clear in this case, at least for the code flow, it gets murky for the implicit flow:
5.1 states:
"The client MUST ignore unrecognized value names in the response."
So one could check if "null" is a valid JSON value, which it is, according to RFC 8259 (as well as the older RFC 4627).

As the standard also requires error responses to have HTTP status code 400, those can be distinguished from successful responses with the HTTP 200 status, even if both include all required fields for both case.

So the code should use the HTTP status code to distinguish if this is a success or error response and handle them accordingly.

In the implicit flow i think the case is ambiguous. The reading in 4.2.2 and 4.2.21 make it indistingushable. A response could be either a failure or a success message, as there is no HTTP status code to make the final decision from. But if it contains a non null access_token, and a null error code, i would still think it is a success. If both are set, i would opt for error, as something is obviously broken.

So i think this should work and should result in a valid AccessTokenResponse, if it is sent with a HTTP status code of 200 and as a broken ErrorResponse if received with a status code 400.

@tpazderka
The OpenID connect standard recommends (SHOULD) to omit null value claims from UserInfo responses in 5.3.2, but does not make any comments about the ErrorResponses or AccessTokenResponses, maybe thats the part you remembered.

The implementation decision of Keycloak is still weird, as it just wastes bandwith to include useless null values.

[tpazderka]

Yeah, you are correct about the omitting of empty claims... That's what I remember...

We could test for presence of non-empty error instead of simple presence, which should solve this...

[schlenk]

Should work good enough, yes.