Django-oauth-toolkit: Data consistency issue when revoking access tokens

[gbataille]

Opening a new issue from #638 investigation.

Context:

AccessToken expose an API to revoke them: revoke().
I use that when a user does an explicit logout to make sure that the token is invalidated (we can argue whether this is really necessary from a security point of view, but...)

The issue:

revoke() actually just deletes the AccessToken in the db.
Yet, we might still have an associated refresh token.
When trying to use it, it will look for the linked access token to find the scopes associated with the "connection" and will crash when it does not find anything (DoesNotExist AccessToken)

Proposition:

• do not delete the token from the db, rather just update the "expires" attribute to invalidate it (and let the clear_token() function deal with the deletion when it's safe only)
• do not delete the token from the db, mark it as revoked (and change the token validation code to recognize a new revoked attribute)
• delete the RefreshToken at the same time. From my point of view, if the user voluntarily logs out, it's ok to kill his RefreshToken but I'm not sure whether there are other more complex scenari to consider.

Do you agree? which do you prefer? I'm happy to take a crack at it and provide a PR

[cleder]

+1 for "delete the RefreshToken at the same time."
The other propositions seem just to work around the problem at some level.
If you really need to keep track of token creations, deletions this should be done by a 3rd party product.

[gbataille]

To give a bit more context, in my db today I have 10,000 RefreshToken

• that are not expired (expires is null or in the future)
• that are not revoked (revoked is null)
• that don't have an access token (access_token_id is null, meaning I guess that the AccessToken has been deleted from the DB
• that have not been used (i.e. there are no AccessToken with a source_refresh_token pointing to it)

--> those RefreshToken therefore cannot get access to the scopes they were generated for (which is stored on the AccessToken) and when you try to use them, they do a 500

I'm trying to figure out if I'm doing something wrong somewhere and/or if there is a flaw in the library, and the mentioned AccessToken.revoke() seems a likely candidate to create the issue I'm experiencing

[gbataille]

Wow, so my investigation continued, now a few weeks later.

Both changes (the one here mentioned and the one from issue #638

1. Nothing bad to report due to those 2 changes.
2. But my issues continued. I checked on those RefreshToken that are as described above and I found something interesting. They were all created before Sept 22nd which is the date at which I upgraded from 0.11 to 1.0 which introduced the source_refresh_token column. My new assumption is now that the migration leaves the old data inconsistent (at least the way I upgraded).

So I just brutally removed all the tokens that are in a bad state. Since then (12h only), the issue that was happening very regularly has not shown itself once.

So I might be all done with this multiple source issue on RefreshTokens

[gbataille]

By the way, I'll submit a PR for the refresh token clean up mentioned at the start of this issue, but maybe only in a couple of weeks :(

[synasius]

I was reading RFC7009 spec (OAuth2.0 Token Revocation) and near the end of Section 2.1 we can read:

If the token passed to the request
is an access token, the server MAY revoke the respective refresh
token as well.

So the spec does not enforce to revoke refresh tokens related to a revoked access token. I think DOT should not enforce this behavior as well, but we could provide a setting to change the refresh token revocation policy.

Of course, accessing original scopes is still a problem. Sure, marking the access tokens as expired or using a revoked flag is a viable solution but we should provide a way (maybe a command) to clean revoked access tokens with no active refresh tokens...
Another approach could be to store the original scopes into the RefreshToken model and that would relieve us from keeping the revoked access tokens stored in the db. But this is a bigger change.

What do you think?

[ravangen]

It would be appreciated if DOT was to not "delete the RefreshToken at the same time.", or at least provide a setting to toggle this functionality.

One use case to consider is if an application wants to revoke an access token but still retain the ability to use their refresh token when necessary.

[IvanAnishchuk]

My guess is that removing expired/revoked access tokens is a performance optimization, even if expired ones are cleared periodically the access token table is bound to have quite a few of them accumulated at all times for any loaded service and that table is queried against for every single client request.

But it would be indeed better to bring some order here. One more thing to consider: tokens probably don't need created, expires, and revoked at the same time considering we always have expiration seconds in the settings if there's any doubt. created and expires fields would be enough for both access and refresh tokens to implement any required logic. On token issue/reissue set expire to now plus appropriate timedelta. On revocation, just set expired = now. On rotation it could be set to now plus grace period. This way simple comparison with now should be enough for all use cases, I think. revoked could still be used for future audit or something, to compare it with expires if we, say, wanted to know who and when revoked their tokens before their natural expiration but unless we allow variable expiration periods I don't see much difference with using value from the settings and created.

Also note that theoretically refresh tokens can have expiration period of their own, it's not currently working, AFAIK, but settings are available and documented. It might be a good idea to set revoked or hypothetical expires RefreshToken field to now + REFRESH_TOKEN_EXPIRE_SECONDS if the latter is set to simplify further validation.