search

wohali / oauth2-discord-new

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

Token revocation #45

Open ShawnCZek opened 1 year ago

ShawnCZek commented 1 year ago

This is an implementation of the token revocation based on RFC7009.

Introduction

Revoking a token using the OAuth 2.0 Client is a bit more complex. This action is not a part of the traditional RFC6749 standard, and therefore, contributors of The League of Extraordinary Packages refused to provide any interface for it. See thephpleague/oauth2-client#479 for more information.

As a result, we are left to implement the token revocation fully. This is, however, a considerable problem in many cases. We obviously cannot break the public API and cannot extend existing interfaces or abstractions. Moreover, the implementation of the base provider does not allow any reasonable abstraction for the token revocation due to everything being based on access tokens only.

This also raises the question of how to properly open the token revocation for an extension.

Implementation

Considering everything mentioned above, I have kept everything as simple as possible. When you compare my implementation to the access token methods, you will find the connection between them:

The method that the user is interested in, revokeToken(), has the same interface as getAccessToken(). This allows the user to specify any other parameter or override existing options.

I had the idea of creating methods such as revokeAccessToken() or revokeRefreshToken(). This would, however, not be correct according to RFC7009. This standard allows the developer to send any token, and the authorization server is supposed to deal with that in the best possible manner.

Undefined Behavior

Another concern of this whole pull request is the lack of documentation. Except for specifying the URL, the official documentation does not describe the token revocation in any way. This raises many questions as the standard has a few recommendations and is sometimes open for interpretation.

Revocation Request

Let's take a look at section 2.1. of RFC7009:

Depending on the authorization server's revocation policy, the revocation of a particular token may cause the revocation of related tokens and the underlying authorization grant. If the particular token is a refresh token and the authorization server supports the revocation of access tokens, then the authorization server SHOULD also invalidate all access tokens based on the same authorization grant (see Implementation Note). If the token passed to the request is an access token, the server MAY revoke the respective refresh token as well.

Refer to RFC2119 for the explanation of SHOULD and MAY keywords.

As of right now, sending either a refresh token or an access token invalidates the full authorization grant. This is now also documented in the official Discord documentation (see discord/discord-api-docs#6356 for more details).

Error Response

Error responses are unclear. Let me quote section 2.2.1. of RFC7009:

If the server responds with HTTP status code 503, the client must assume the token still exists and may retry after a reasonable delay. The server may include a "Retry-After" header in the response to indicate how long the service is expected to be unavailable to the requesting client.

This status code, however, is not discussed anywhere in the Discord documentation. As this rather seems to be a rare error and may occur in the OAuth2 authorization flow, too, I have left it unhandled. Or, well, it is currently handled by checkResponse().

Sources

When I was implementing this, I went through external sources (other libraries, guides, ...). These are my observations:

Consideration

I mentioned these sources for a reason. Token revocation seems to be an ignored topic in general. Therefore, this implementation is only based on information from the standard and my observations.

This can obviously become a problem. Things might change between versions without notice and cause issues for the users of this library. On the other hand, this must not be a reason why we should ignore this topic. Token revocation is extremely useful as it fully logs out the Discord user and deletes all data connected to the respective authorization grant (e.g., application metadata).

All in all, this pull request follows the contributing guidelines of this repository:

This resolves #34.

wohali commented 1 year ago

Hey @ShawnCZek , thanks for this and your extensive writeup.

Do you feel like taking a crack at a separate PR to replace Travis with GHA? I don't have the time right now and that'd be convenient. It'd make checking the functionality in this PR a bit easier, too.

I can then do a bit more detailed of a review other than my skimming of the code.

ShawnCZek commented 1 year ago

I have noticed that the Travis integration is a bit iffy but did not pay too much attention to it 😅

Sure, I can take a look at migrating from Travis to GitHub Actions and will draw a new pull request for it by the end of this week.

ShawnCZek commented 5 months ago

I have made a few changes to the documentation based on the recent development of the Discord documentation.

Moreover, I removed the dependency on the option provider, which was an incorrect assumption on my part. Please take a look at the commit message (c20fe196aa622e02e2c9d958c7915a09ba5a1961) for more details on why I did this.