Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revoke Token #34

Open
pablomayobre opened this issue Jul 5, 2021 · 5 comments · May be fixed by #45
Open

Revoke Token #34

pablomayobre opened this issue Jul 5, 2021 · 5 comments · May be fixed by #45

Comments

@pablomayobre
Copy link
Contributor

pablomayobre commented Jul 5, 2021

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
Copy link
Contributor Author

Related to #30

Also check thephpleague/oauth2-client#479 for more info

@wohali
Copy link
Owner

wohali commented Jul 6, 2021

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

@pablomayobre
Copy link
Contributor Author

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:

  • No return, throw when an error occurs (either custom or normal Exception).
  • Return a boolean, true for success, false for failure (both retry and errors are considered failures)
  • Return a boolean, true for success, false for retry, throw for errors (either custom or normal Exceptions)
  • Return a custom object or an array which can be used to check if it was a success, or get the error otherwise.

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

@ShawnCZek
Copy link
Contributor

@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:

  • If the client ID/secret is wrong (the developer's fault)
  • If the token_type_hint is wrong (the developer's fault again, however, it can be validated before sending the request)
  • Rate limit. Not sure what the limit here is, though. The Discord documentation is not very clear in this regard

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
Copy link
Contributor Author

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)

@ShawnCZek ShawnCZek linked a pull request Dec 29, 2022 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants