jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.16k stars 794 forks source link

revoking AccessToken only and then using RefreshToken token to generate new AccessToken breaks. #839

Open adityakrgupta25 opened 4 years ago

adityakrgupta25 commented 4 years ago

Describe the bug Revoking AccessTokens does not revoke the corresponding RefreshTokens. So in case, AccessToken is revoked, its deleted from the dB. If a request for a new token on TokenViewcomes with the existing RefreshToken, it would cause the flow to break at OAuth2Validator.get_original_scopes() due to lack of existence of AccessToken.

  File "/usr/local/lib/python3.7/site-packages/oauth2_provider/oauth2_backends.py", line 138, in create_token_response
     headers, extra_credentials)
   File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/endpoints/base.py", line 64, in wrapper
     return f(endpoint, uri, *args, **kwargs)
   File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/endpoints/token.py", line 117, in create_token_response
     request, self.default_token_type)
  File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py", line 59, in create_token_response
     self.validate_token_request(request)
   File "/usr/local/lib/python3.7/site-packages/oauthlib/oauth2/rfc6749/grant_types/refresh_token.py", line 117, in validate_token_request
 request.refresh_token, request))
   File "/usr/local/lib/python3.7/site-packages/oauth2_provider/oauth2_validators.py", line 605, in get_original_scopes
    return AccessToken.objects.get(source_refresh_token_id=rt.id).scope
   File "/usr/local/lib/python3.7/site-packages/django/db/models/manager.py", line 82, in manager_method
  return getattr(self.get_queryset(), name)(*args, **kwargs)
line 408, in get
 self.model._meta.object_name
oauth2_provider.models.AccessToken.DoesNotExist: AccessToken matching query does not exist.

To Reproduce

  1. Generate access token/ refresh token pair.
  2. Revoke only access token by hitting: /o/revoke_token/ endpoint with a body that would look like:
    { 'token': <your access token>,
            'token_type_hint': 'access_token',
            'client_id': ...,
            'client_secret': ...}
  3. Try regenerating access/refresh token pair using refresh_token as your grant_type.

Expected behaviour If revoking (aka deleting) only AccessTokens is allowed without revoking refresh tokens, this should not crash when generating new token with existing valid refresh token.

Version django-oauth-toolkit==1.2.0

Should be reproducible on the newer version.

Additional context

It came with this merging of this issue: https://github.com/jazzband/django-oauth-toolkit/issues/497 wherein it was intended to provide grace period window by not deleting the Refresh token if the Access Token. This has lead to above unintended consequences.

phroiland commented 4 years ago

Is that causing this?

Screen Shot 2020-05-16 at 4 56 04 PM

Version django-oauth-toolkit==1.2.0 Django==1.11.29

adityakrgupta25 commented 4 years ago

Is that causing this?

Screen Shot 2020-05-16 at 4 56 04 PM

Version django-oauth-toolkit==1.2.0 Django==1.11.29

No, this seems very different from what we have been experiencing.

SamSchelfhout commented 4 years ago

This issue still exists in the latest version. I`m not sure if it is an oauthlib or django-oauth-toolkit issue. In the oauthlib refreh token function the following function is called:

File: oauth2_validators.py

def get_original_scopes(self, refresh_token, request, *args, **kwargs):
        # Avoid second query for RefreshToken since this method is invoked *after*
        # validate_refresh_token.
        rt = request.refresh_token_instance
        if not rt.access_token_id:
            return AccessToken.objects.get(source_refresh_token_id=rt.id).scope

        return rt.access_token.scope

When the access code was revoked, it was removed from the database. Thus it is impossible to get the original scope based on the original access code.

This issue seems to break a crucial part of the OAuth2 process. Is there a solution coming?

MattBlack85 commented 4 years ago

Revoking seems to me like something very destructive and somethingvvery different from an expiration,like you don't want anybody else to use your app no matter what. I have to read more, but to me it looks like this code provide some guard when the access token is expired,if the access one is revoked, it seems to me that both access and refresh one should go?

SamSchelfhout commented 4 years ago

This issue causes a 500 HTTP error. The RFC https://tools.ietf.org/html/rfc7009 states the following:

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.

How did the django-oauth-toolkit intended this to work? Should the refresh token be invalidated too or should it still be usable?

n2ygk commented 4 years ago

@SamSchelfhout I think that the more conservative approach is to revoke the respective refresh token as well. What does it mean to revoke access and then expect the refresh token -- which exists solely to renew access -- to let one use it to get access again? This feels like a "pass the hash" exposure. I think the code as implemented is right but the 500 error should be fixed.

MattBlack85 commented 4 years ago

The problem is here https://github.com/jazzband/django-oauth-toolkit/blob/master/oauth2_provider/models.py#L281

We do not delete the refresh token on cascade on revocation but we set the relation to Null, that's why end in that situation

Il dom 1 nov 2020, 15:02 Alan Crosswell notifications@github.com ha scritto:

@SamSchelfhout https://github.com/SamSchelfhout I think that the more conservative approach is to revoke the respective refresh token as well. What does it mean to revoke access and then expect the refresh token -- which exists solely to renew access -- to let one use it to get access again? This feels like a "pass the hash" exposure. I think the code as implemented is right but the 500 error should be fixed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jazzband/django-oauth-toolkit/issues/839#issuecomment-720093331, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7YNFV5DPTUQZQDUOVCA6TSNVS6FANCNFSM4M6WDBMA .

krzysieqq commented 3 years ago

still in 1.4.0, any updates?

petros94 commented 1 year ago

Any news? still in 2.2.0