wohali / oauth2-discord-new

New Discord Provider for the OAuth 2.0 Client
MIT License
118 stars 22 forks source link

Revoke Token #34

Open pablomayobre opened 3 years ago

pablomayobre commented 3 years ago

Hi, I was thinking that maybe having a revoke method on the Discord provider, to revoke an AccessToken.

Note that the AbstractProvider has no such method, but it could prove useful anyways.

I found a way to do this, but I haven't properly tested it yet:

/** Revoke an AccessToken
 * @param League\OAuth2\Client\Token\AccessTokenInterface $token The AccessToken to revoke
 * @return string One of 'success', 'retry' or 'error'
 *   'success': The token was successfully revoked.
 *   'retry': The token couldn't be revoked, the operation should be retried later.
 *   'error': There was some sort of error or bad response. Assume the token hasn't been revoked.
 */
public function revoke ($token) {
  $request = $this->getAuthenticatedRequest(
    'POST',
    $this->apiDomain . '/oauth2/token/revoke',
    $token,
    [
      'headers' => ['Content-Type' => 'application/x-www-form-urlencoded'],
      'body' => http_build_query([
        'token' => $token->getToken(),
        'token_type_hint' => 'access_token'
      ], '', '&')
    ]
  );

  $response = $this->getResponse($request);

  switch ($response->getStatusCode()){
    case 200:
      return 'success';
    case 503:
      return 'retry';
    default:
      return 'error';
  }
}

This code is based on RFC7009.

Errors are further discussed in RFC6749 Section 5.2

Also this code doesn't revoke the Refresh Token and it probably should.

pablomayobre commented 3 years ago

Related to #30

Also check https://github.com/thephpleague/oauth2-client/issues/479 for more info

wohali commented 3 years ago

If they're OK with it in thephpleague/oauth2-client, I'd accept a PR. Want to write one (with test case)?

pablomayobre commented 3 years ago

My questions would be:

How would you specify what token to revoke?

You can revoke either the access or the refresh token. I wouldn't do both just in case an error occurs and we need to report it to the user.

Also the access token is needed for authentication.

What should the return value look like?

A 200 status code is considered a success, 503 should be taken as "retry later", neither of these specify a Body for the response so it should be ignored.

Errors should be reported with a 400 (Bad Request) status code. A JSON Body will be attached with data for the errors. I still need to check these against Discord.

Our options would be:

Some other parts of this provider throw errors as custom Exceptions, so that may be our best option?

ShawnCZek commented 1 year ago

@pablomayobre What error may occur when revoking a token? I have just played around with the Discord OAuth2 server, and even if the passed token is wrong, it does not return any error (as expected, to be fair). The only possible errors are:

But then it, of course, depends on the specific implementation. In other words, if you want to provide an interface that revokes both tokens at once, then yes, there might be additional pitfalls. If not, it can be handled by the developer in the same way as retrieving the access token is being handled right now. That is to say, they handle any invalid response on their own.

Either way, have you made any prototype? I have tried to implement this similarly to retrieving the access token. However, my implementation is incomplete as it depends on the getAccessTokenOptions method of OptionProviderInterface.

A proper solution would have to provide an own optionProvider collaborator for League/OAuth2's AbstractProvider. With that, however, the complexity of this package rapidly grows.


Btw. I wonder if revoking the access token alone is enough. The RFC7009 proposal states the following:

A revocation request will invalidate the actual token and, if applicable, other tokens based on the same authorization grant and the authorization grant itself.

If I understand it correctly, revoking the access token should automatically revoke the refresh token, too. Which is exactly what I experienced with Discord. Once the access token was revoked, the application disappeared from Authorised Apps. On the other hand, as this is not documented anywhere, it is undefined behavior.

pablomayobre commented 1 year ago

I haven't worked with this API nor with the implementation since I posted the original issue, basically I did exactly what I proposed with a few modifications.

I invalidate both tokens just to be sure, and I have a catch for errors just in case. Even if they don't appear in the wild it may happen at some point due to rate limits or API changes on the Discord side or bad tokens in our DBs.

The revoke API I implemented just takes the token and the type of token and does the HTTP request, and that was a good enough solution for my needs.

Hopefully that's enough information but as I mentioned I don't have any strong opinions on this since I haven't been actively working on this in a long time (a year and a half)