token_type_hint should be used as a hint only

[rozagerardo]

Describe the bug
Currently, if we pass a token_type_hint parameter that doesn't match the token we passed, the token is not revoked. the parameter should be just a hint to find the token, but as indicated in the specs, if it is not found using the token_type_hint value, then the rest of the supported types should be used:

If the server is unable to locate the token using the given hint, it MUST extend its search across all of its supported token types.

To Reproduce
1- Issue an Access Token
2- Send a request as follows:

curl --location --request POST 'http://localhost:9000/oauth2/revoke' \
--header 'Authorization: Basic bWVzc2FnaW5nLWNsaWVudDpzZWNyZXQ=' \
--header 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'token=<access_token>' \
--data-urlencode 'token_type_hint=refresh_token'

3- Server responds with 200, but the token is not revoked

Expected behavior
Server should still revoke the Access Token

[lspil]

Hm, this is an interesting one. I also think the hint should not restrict to a specific type. If everyone agrees, I can also make the change with #170 which is on my side on the same code.

[lspil]

Btw, isn't that very related also with #174 you registered earlier?

[jgrandja]

@rozagerardo The request you specified is not valid, since you're trying to revoke an access token but the token_type_hint
is refresh_token. This is a bug on the client side.

Furthermore, as per spec in 2.2. Revocation Response:

The authorization server responds with HTTP status code 200 if the
token has been revoked successfully or if the client submitted an
invalid token.

Given that the request is invalid, the expected response is still 200.

I'm going to close this as the current behaviour is inline with spec.

[rozagerardo]

Sorry @jgrandja I know you have a lot to go through for this project, but can we discuss this a little bit further? I'm pretty much sure I'm not misunderstanding the specs.

As I understand it, the token_type_hint is just a hint for the server to find the token easier, but it should affect in no way the outcome of the result. We could even ignore completely the request parameter if we want. I guess this behavior is because the token_type_hint parameter is optional. Note that what I posted as a quoted note above is also part of the spec:

token_type_hint OPTIONAL. A hint about the type of the token submitted for revocation. Clients MAY pass this parameter in order to help the authorization server to optimize the token lookup. If the server is unable to locate the token using the given hint, it MUST extend its search across all of its supported token types. An authorization server MAY ignore this parameter, particularly if it is able to detect the token type automatically.

An invalid token type hint value is ignored by the authorization server and does not influence the revocation response.

Regarding the note you mentioned:

The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token.

Note that here the spec mentions invalid token, but the validity of the token has nothing to do with the request we made, and the parameters we passed. Of course, in any case the response should be 200, but the outcome should be different (the token should be revoked).

Let me know what you think about this, if you are still not sure, I can explore some well-known solutions like Keycloak or Okta to see how they handle these scenarios.

[jgrandja]

@rozagerardo I see your point as well but revoking an access token when token_type_hint is refresh_token does not seem right to me. The request is invalid so revoking the access token may cause side affects on client code.

I would be interested to see what Keycloak and Okta do in these instances. If you can explore those providers and any other ones that would be very helpful.

[rozagerardo]

Cool @jgrandja. Yes, I agree this is an edge case (why would a Client explicitly indicate a wrong token type?), but if it's part of the spec we should respect that.
Sounds good, I'll take my time to analyze other providers and report back afterwards :) Thanks for the quick response.

---

And sorry @lspil , I didn't answer your last question. This is somewhat related to #174, but it's actually a different scenario.
I think it's clearer having a look at the code:
1- this issue would be resolved by changing how we obtain the token from the in-memory storage.
Currently, we're trying to find it using the provided token type hint, if it can't be found with that particular hint, then it retrieves null (here)
And here I worked on a solution using the token type only as a hint as part of #52. Here we retrieve null only if the token can't be found using any supported token type; the hint only determines with which token type we'll request first.

2- The issue in #174 on the other hand was solved here by not retrieving the unsupported_token_type response if the token_type_hint doesn't match any unknown token type, because this error response is not related to the token_type_hint param in any way.
As I understand the spec, this error response should be retrieved only if the server is asked to revoke a token, and it does not support revoking that particular type of token. I'm not sure if the possibility of revoking a particular token type should be configurable, but it's clear to me that as long as we support revoking any kind of token, this error response should not be a possible outcome.
Let me know if that was clear :)

[rozagerardo]

Reporting back about this @jgrandja (since you're out for the holidays and this is a closed issue I might ping you again a few days after you return).

Here are two videos demoing the Introspection and Revocation endpoints for Okta and Keycloak:
https://drive.google.com/drive/folders/1QIE0_j1YpHtuQeGh5Gs1lv7fg-jH-6Ed?usp=sharing

The highlights are:

1- They both seem to work according to the hypothesis I presented in this issue; both endpoints for both framework process the request normally regardless of the token_type_hint param value; if the param doesn't match the token type of the token, the token is introspected and revoked just fine.

2- Particularly, for just for Keycloak's Introspection endpoint, the service responds with a 400 Bad Request when an unknown token_type_hint value is used. The Introspection endpoint is not totally explicit about this as in the Revocation endpoint specs, but IMO I would follow the approach taken in Okta, keeping consistency between both endpoints regarding this aspect. I reported a bug to the Keycloak team (here) to see if they fix this or if they make me change my mind :)

3- I think we both agree with this point, but it's worth mentioning that, as shown in the video, we have a case of when to use the unsupported_token_type response error. It seems Keycloak doesn't support revocating an access_token (as per their codebase at all, and when we try to revoke an access_token, the framework retrieves a unsupported_token_type response (different from the invalid request retrieved in the scenario mentioned in point 2) and regardless of the token_type_hint parameter used. This aligns with the description of issue #174 I reported earlier 👍

Let me know if there is anything else I should explore here, if I should reopen this issue or create a PR with a solution similar to the one I presented for the Introspection Endpoint here.

[jgrandja]

@rozagerardo You got me on the same page as you 👍

Ok, let's align with:

If the server is unable to locate the token using
the given hint, it MUST extend its search across all of its
supported token types

Would you like to submit a PR for this fix?

Also, I agree with your statement:

The Introspection endpoint is not totally explicit about this as in the Revocation endpoint specs, but IMO I would follow the approach taken in Okta, keeping consistency between both endpoints regarding this aspect

[rozagerardo]

Sounds great @jgrandja, I have now prepared the following PR:
#188

Note the integration tests mock the method we're using here, I've validated the functionality by running the sample and revoking a token manually as well, just to be sure everything is working as expected 👍

Looking forward to the comments!

And happy new year BTW!