-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Data consistency issue when revoking access tokens #645
Comments
+1 for "delete the RefreshToken at the same time." |
To give a bit more context, in my db today I have 10,000 RefreshToken
--> those 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 |
Wow, so my investigation continued, now a few weeks later. Both changes (the one here mentioned and the one from issue #638
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 |
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 :( |
I was reading RFC7009 spec (OAuth2.0 Token Revocation) and near the end of Section 2.1 we can read:
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 What do you think? |
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. |
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 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 |
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 theAccessToken
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:
clear_token()
function deal with the deletion when it's safe only)revoked
attribute)RefreshToken
at the same time. From my point of view, if the user voluntarily logs out, it's ok to kill hisRefreshToken
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
The text was updated successfully, but these errors were encountered: