Returning forbidden when an unauthorised user tries to revoke a token

[cristofer]

Summary

When digging into an issue we are currently facing with Revoked tokens, I found out that the test should not revoke the token as its unauthorized leads me to see that if the user is not authorised to perform the revoke, then it was not receiving the correct status for the operation, which is forbidden

[cristofer]

Something weird with the errors in Travis, I will take a look. However in the meantime I would appreciate if you see this PR and can confirm that the new logic is correct.

[nbulaj]

Hi @cristofer . I took a look at RFC 7009 (OAuth 2.0 Token Revocation), section 2.1. Revocation Request:

The authorization server first validates the client credentials (in
case of a confidential client) and then verifies whether the token
was issued to the client making the revocation request. If this
validation fails, the request is refused and the client is informed
of the error by the authorization server as described below.

Actually I didn't find what is meant by this described below, so we consider it's the error response from base RFC (6749).

Then I checked section 5. Security Considerations and didn't find that we must return 200 in case client unauthorized. So seems like this PR is totally reasonable :+1. But I think it must be improved and response must complain RFC in a way we do it in other places - with error codes like "unauthorized_client" and so one (I mean mean-full error responses, not just 403 with empty body).
/cc @felipeelias

Also I think we need to change this line:

#   token.revoke if token.accessible?
#   to:
token.revoke if token&.accessible? # in case invalid access token value was passed and we didn't find the token

because of:

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

If client sends invalid token we have nil object for @token and I think it will return NoMethodError when try to check for token.accessible?. So we need a guard clause. And in any cases we just return 200 status code that is OK with the RFC.

Also this point I think we don't check for token revocation:

A malicious client may attempt to guess valid tokens on this endpoint
by making revocation requests against potential token strings.
According to this specification, a client's request must contain a
valid client_id, in the case of a public client, or valid client
credentials, in the case of a confidential client.

Could you please add it too? If no - it's OK, I'll do it myself when I have time. Just LMK.
Thanks for your contribution!

[nbulaj]

Well, seems like your changes broke the specs :) You need to double-check them and fix I think

[cristofer]

thank you @nbulaj ! I will take care of what you mentioned before as soon as I have time. And about the travis errors, any clue about this error? uninitialized constant RSpec::Rails::ChannelExampleGroup::ActionCable it looks super weird :-/

[nbulaj]

Oh, seems like test app (spec/dummy) loads all Rails engines including ActionCable. Wondering how we didn't face it before 🤔 Need to check it out

[nbulaj]

I fixed the specs, so you can rebase with current master

[cristofer]

@nbulaj please let me know if the changes I pushed in 72f84d1 are good enough (added the error description and the guard, also fixed the tests). I was not sure how to add the logic here

doorkeeper/app/controllers/doorkeeper/tokens_controller.rb

Line 74 in 72f84d1

# Client is public, authentication unnecessary

for the valid client_id, should I compare it with the server application? thanks!

[nbulaj]

See comments please

[nbulaj]

. I was not sure how to add the logic here

@cristofer you don't need to add anything here. This method checks if the token (that must be revoked) is present in the HTTP request. If not - we have an error (and it's OK). It token value present in the request, then server checks if this token issued to some application. In case it is, and application is confidential, then server must (and do it) check if authorized client is equal to the client that token was issued for. In case application isn't confidential (i.e public) - it's OK, token can be revoked by anybody.

[cristofer]

@nbulaj I hope f221f12 includes what is needed in order to merge this PR :) if not let me know and I will do the necessary changes.

Thank you!

[nbulaj]

Don't forget please to squash commits as bot suggested and we can merge it
Thanks!

[cristofer]

All good now @nbulaj ! Thanks for the time you took to give feedback, I am very excited about contributing to this amazing gem :)

Chao!

[nbulaj]

LGTM
🚝

[nbulaj]

Thanks @cristofer for your patience and work! 🎉