django-oauth-toolkit
django-oauth-toolkit copied to clipboard
jazzband
Refreshing a revoked access token throws an error 500
When you revoke an access token and try to refresh it afterwards, you end up with an error 500.
This seems to be related to the get_original_scopes method of oauth2_provider/oauth2_validators.py which tries to retrieve the access token related to the refresh token. However, since it was revoked, it no longer exists and Django throws a DoesNotExists error which isn't catch.
When we revoke an access token, maybe we should revoke refresh tokens too ? Or we could add a clause to the queryset on validate_refresh_token method to exclude refresh token without access_token.
As a stopgap, could you do something like the following:
token_model = oauth2_provider.models.get_refresh_token_model()
tokens = oauth2_provider.models.RefreshToken.objects.all()
for token in tokens:
token.delete()
This would of course be for revoking every refresh token (I haven't tested it but it works for access tokens at least, if you replace the "refresh"s with "access")
I've also ran into this. It looks like this cannot be mitigated without adding the scope to the RefreshToken model as well?
We're mitigating like this on our access token model currently:
def revoke(self):
self.expires = now() - timedelta(hours=1)
self.save(update_fields=['expires'])
I've implemented a fix for this: https://github.com/jazzband/django-oauth-toolkit/pull/620
FYI: My fix turned out to not be a fix, because I just replicated a bug in 0.12.0 that would return a 401.
I think the real fix would be to allow the refresh token, which itself was not revoked, to continue to work. I'm not sure of the correct fix, but it might be to store the refresh token's scope, and not rely on an associated access token to determine the original scope.
I think the real fix would be to allow the refresh token, which itself was not revoked, to continue to work. I'm not sure of the correct fix, but it might be to store the refresh token's scope, and not rely on an associated access token to determine the original scope.
That would be option A, option B would be adding a revoked attribute to access tokens and revoke them by setting that instead of deleting them. Not sure which one is better.
Agreed. Not sure which approach is better. I need to move on, but hopefully this is more clear for someone to pick up.
Any progress on this? I have just run into this myself upgrading from 1.0.0 to 1.2.0. I'd be happy to try and make a PR if someone can point me in the right direction.
No progress that I am aware of.
The options continue to be: Option A: Redundantly store the original scope with the revoke token and use this rather than trying to reference the related access token. Option B: Store "revoked" status with the access token and update its status, rather than deleting, when revoked.
See my previous PR if you want much more detail around the bug with code references: https://github.com/jazzband/django-oauth-toolkit/pull/620.
Good luck @keattang.
@robrap Is there a consensus on which to go with? I'd prefer not to write something and then it be decided that it's not the correct approach
Not sure if @jleclanche could give thumbs-up on an approach before you start. I agree with wanting to know which way to go first.
I don't have an ultimate say on DOT, but option B sounds better to me.
Assuming option B is taken, can I take it that the cascade would be changed from null-on-cascade to delete-on-cascade?
