From 2e4d15ee9372eb65289382dae0ee3c82a54cac06 Mon Sep 17 00:00:00 2001 From: Phillip Baker Date: Thu, 15 Feb 2018 19:16:52 -0500 Subject: [PATCH] Revoke refresh tokens instead of deleting them To handle the case where a refresh token is used to generate a new access token but the new access/refresh token is never received, allow a refresh token to be re-used within a grace period, returning the same access token (appearing as if the request/response was 'cached'). This now revokes the token with a timestamp instead of deleting it. Refs #497 --- docs/settings.rst | 10 ++ .../migrations/0006_auto_20171214_2232.py | 44 +++++++++ oauth2_provider/models.py | 24 +++-- oauth2_provider/oauth2_validators.py | 72 ++++++++++----- oauth2_provider/settings.py | 1 + tests/test_authorization_code.py | 91 ++++++++++++++++++- tests/test_oauth2_validators.py | 2 +- tests/test_token_revocation.py | 3 +- 8 files changed, 216 insertions(+), 31 deletions(-) create mode 100644 oauth2_provider/migrations/0006_auto_20171214_2232.py diff --git a/docs/settings.rst b/docs/settings.rst index fb567ebfa..d7d07c4fe 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -100,6 +100,16 @@ REFRESH_TOKEN_EXPIRE_SECONDS The number of seconds before a refresh token gets removed from the database by the ``cleartokens`` management command. Check :ref:`cleartokens` management command for further info. +REFRESH_TOKEN_GRACE_PERIOD_SECONDS +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +The number of seconds between when a refresh token is first used when it is +expired. The most common case of this for this is native mobile applications +that run into issues of network connectivity during the refresh cycle and are +unable to complete the full request/response life cycle. Without a grace +period the application, the app then has only a consumed refresh token and the +only recourse is to have the user re-authenticate. A suggested value, if this +is enabled, is 2 minutes. + REFRESH_TOKEN_MODEL ~~~~~~~~~~~~~~~~~~~ The import string of the class (model) representing your refresh tokens. Overwrite diff --git a/oauth2_provider/migrations/0006_auto_20171214_2232.py b/oauth2_provider/migrations/0006_auto_20171214_2232.py new file mode 100644 index 000000000..4622f096d --- /dev/null +++ b/oauth2_provider/migrations/0006_auto_20171214_2232.py @@ -0,0 +1,44 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.1 on 2017-05-14 11:41 +from __future__ import unicode_literals + +from oauth2_provider.settings import oauth2_settings +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('oauth2_provider', '0005_auto_20170514_1141'), + ] + + operations = [ + migrations.AddField( + model_name='accesstoken', + name='source_refresh_token', + field=models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=oauth2_settings.REFRESH_TOKEN_MODEL, related_name='refreshed_access_token'), + preserve_default=False, + ), + migrations.AddField( + model_name='refreshtoken', + name='revoked', + field=models.DateTimeField(null=True, default=django.utils.timezone.now), + preserve_default=False, + ), + migrations.AlterField( + model_name='refreshtoken', + name='token', + field=models.CharField(max_length=255), + ), + migrations.AlterField( + model_name='refreshtoken', + name='access_token', + field=models.OneToOneField(blank=True, null=True, related_name='refresh_token', to=oauth2_settings.ACCESS_TOKEN_MODEL, on_delete=models.SET_NULL), + ), + migrations.AlterUniqueTogether( + name='refreshtoken', + unique_together=set([('token', 'revoked')]), + ), + ] diff --git a/oauth2_provider/models.py b/oauth2_provider/models.py index 5f3b96e7d..6836fe35f 100644 --- a/oauth2_provider/models.py +++ b/oauth2_provider/models.py @@ -221,6 +221,7 @@ class AbstractAccessToken(models.Model): Fields: * :attr:`user` The Django user representing resources' owner + * :attr:`source_refresh_token` If from a refresh, the consumed RefeshToken * :attr:`token` Access token * :attr:`application` Application instance * :attr:`expires` Date and time of token expiration, in DateTime format @@ -231,6 +232,11 @@ class AbstractAccessToken(models.Model): settings.AUTH_USER_MODEL, on_delete=models.CASCADE, blank=True, null=True, related_name="%(app_label)s_%(class)s" ) + source_refresh_token = models.OneToOneField( + # unique=True implied by the OneToOneField + oauth2_settings.REFRESH_TOKEN_MODEL, on_delete=models.SET_NULL, blank=True, null=True, + related_name="refreshed_access_token" + ) token = models.CharField(max_length=255, unique=True, ) application = models.ForeignKey( oauth2_settings.APPLICATION_MODEL, on_delete=models.CASCADE, blank=True, null=True, @@ -313,38 +319,41 @@ class AbstractRefreshToken(models.Model): * :attr:`application` Application instance * :attr:`access_token` AccessToken instance this refresh token is bounded to + * :attr:`revoked` Timestamp of when this refresh token was revoked """ id = models.BigAutoField(primary_key=True) user = models.ForeignKey( settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="%(app_label)s_%(class)s" ) - token = models.CharField(max_length=255, unique=True) + token = models.CharField(max_length=255) application = models.ForeignKey( oauth2_settings.APPLICATION_MODEL, on_delete=models.CASCADE) access_token = models.OneToOneField( - oauth2_settings.ACCESS_TOKEN_MODEL, on_delete=models.CASCADE, + oauth2_settings.ACCESS_TOKEN_MODEL, on_delete=models.SET_NULL, blank=True, null=True, related_name="refresh_token" ) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) + revoked = models.DateTimeField(null=True) def revoke(self): """ - Delete this refresh token along with related access token + Mark this refresh token revoked and revoke related access token """ access_token_model = get_access_token_model() - token = access_token_model.objects.get(id=self.access_token.id) - # Avoid cascade by deleting self first. - self.delete() - token.revoke() + access_token_model.objects.get(id=self.access_token_id).revoke() + self.access_token = None + self.revoked = timezone.now() + self.save() def __str__(self): return self.token class Meta: abstract = True + unique_together = ("token", "revoked",) class RefreshToken(AbstractRefreshToken): @@ -390,6 +399,7 @@ def clear_expired(): with transaction.atomic(): if refresh_expire_at: + refresh_token_model.objects.filter(revoked__lt=refresh_expire_at).delete() refresh_token_model.objects.filter(access_token__expires__lt=refresh_expire_at).delete() access_token_model.objects.filter(refresh_token__isnull=True, expires__lt=now).delete() grant_model.objects.filter(expires__lt=now).delete() diff --git a/oauth2_provider/oauth2_validators.py b/oauth2_provider/oauth2_validators.py index 2cc4b4b20..dd0cef903 100644 --- a/oauth2_provider/oauth2_validators.py +++ b/oauth2_provider/oauth2_validators.py @@ -10,6 +10,7 @@ from django.contrib.auth import authenticate, get_user_model from django.core.exceptions import ObjectDoesNotExist from django.db import transaction +from django.db.models import Q from django.utils import timezone from django.utils.timezone import make_aware from oauthlib.oauth2 import RequestValidator @@ -449,24 +450,43 @@ def save_bearer_token(self, token, request, *args, **kwargs): # else create fresh with access & refresh tokens else: - # revoke existing tokens if possible + # revoke existing tokens if possible to allow reuse of grant if isinstance(refresh_token_instance, RefreshToken): + previous_access_token = AccessToken.objects.filter( + source_refresh_token=refresh_token_instance + ).first() try: refresh_token_instance.revoke() except (AccessToken.DoesNotExist, RefreshToken.DoesNotExist): pass else: setattr(request, "refresh_token_instance", None) + else: + previous_access_token = None + + # If the refresh token has already been used to create an + # access token (ie it's within the grace period), return that + # access token + if not previous_access_token: + access_token = self._create_access_token( + expires, + request, + token, + source_refresh_token=refresh_token_instance, + ) - access_token = self._create_access_token(expires, request, token) - - refresh_token = RefreshToken( - user=request.user, - token=refresh_token_code, - application=request.client, - access_token=access_token - ) - refresh_token.save() + refresh_token = RefreshToken( + user=request.user, + token=refresh_token_code, + application=request.client, + access_token=access_token + ) + refresh_token.save() + else: + # make sure that the token data we're returning matches + # the existing token + token["access_token"] = previous_access_token.token + token["scope"] = previous_access_token.scope # No refresh token should be created, just access token else: @@ -475,13 +495,14 @@ def save_bearer_token(self, token, request, *args, **kwargs): # TODO: check out a more reliable way to communicate expire time to oauthlib token["expires_in"] = oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS - def _create_access_token(self, expires, request, token): + def _create_access_token(self, expires, request, token, source_refresh_token=None): access_token = AccessToken( user=request.user, scope=token["scope"], expires=expires, token=token["access_token"], - application=request.client + application=request.client, + source_refresh_token=source_refresh_token, ) access_token.save() return access_token @@ -524,6 +545,9 @@ 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 def validate_refresh_token(self, refresh_token, client, request, *args, **kwargs): @@ -531,13 +555,19 @@ def validate_refresh_token(self, refresh_token, client, request, *args, **kwargs Check refresh_token exists and refers to the right client. Also attach User instance to the request object """ - try: - rt = RefreshToken.objects.get(token=refresh_token) - request.user = rt.user - request.refresh_token = rt.token - # Temporary store RefreshToken instance to be reused by get_original_scopes. - request.refresh_token_instance = rt - return rt.application == client - - except RefreshToken.DoesNotExist: + + null_or_recent = Q(revoked__isnull=True) | Q( + revoked__gt=timezone.now() - timedelta( + seconds=oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS + ) + ) + rt = RefreshToken.objects.filter(null_or_recent, token=refresh_token).first() + + if not rt: return False + + request.user = rt.user + request.refresh_token = rt.token + # Temporary store RefreshToken instance to be reused by get_original_scopes and save_bearer_token. + request.refresh_token_instance = rt + return rt.application == client diff --git a/oauth2_provider/settings.py b/oauth2_provider/settings.py index a3ef0b4b1..8799a3956 100644 --- a/oauth2_provider/settings.py +++ b/oauth2_provider/settings.py @@ -45,6 +45,7 @@ "AUTHORIZATION_CODE_EXPIRE_SECONDS": 60, "ACCESS_TOKEN_EXPIRE_SECONDS": 36000, "REFRESH_TOKEN_EXPIRE_SECONDS": None, + "REFRESH_TOKEN_GRACE_PERIOD_SECONDS": 0, "ROTATE_REFRESH_TOKEN": True, "ERROR_RESPONSE_WITH_SCOPES": False, "APPLICATION_MODEL": APPLICATION_MODEL, diff --git a/tests/test_authorization_code.py b/tests/test_authorization_code.py index 824818450..0a7f82c59 100644 --- a/tests/test_authorization_code.py +++ b/tests/test_authorization_code.py @@ -580,6 +580,54 @@ def test_refresh(self): content = json.loads(response.content.decode("utf-8")) self.assertTrue("invalid_grant" in content.values()) + def test_refresh_with_grace_period(self): + """ + Request an access token using a refresh token + """ + oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS = 120 + self.client.login(username="test_user", password="123456") + authorization_code = self.get_auth() + + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org" + } + auth_headers = get_basic_auth_header(self.application.client_id, self.application.client_secret) + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("refresh_token" in content) + + # make a second token request to be sure the previous refresh token remains valid, see #65 + authorization_code = self.get_auth() + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org" + } + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + + token_request_data = { + "grant_type": "refresh_token", + "refresh_token": content["refresh_token"], + "scope": content["scope"], + } + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("access_token" in content) + first_access_token = content["access_token"] + + # check refresh token returns same data if used twice, see #497 + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("access_token" in content) + self.assertEqual(content["access_token"], first_access_token) + oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS = 0 + def test_refresh_invalidates_old_tokens(self): """ Ensure existing refresh tokens are cleaned up when issuing new ones @@ -608,7 +656,8 @@ def test_refresh_invalidates_old_tokens(self): response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 200) - self.assertFalse(RefreshToken.objects.filter(token=rt).exists()) + refresh_token = RefreshToken.objects.filter(token=rt).first() + self.assertIsNotNone(refresh_token.revoked) self.assertFalse(AccessToken.objects.filter(token=at).exists()) def test_refresh_no_scopes(self): @@ -693,6 +742,46 @@ def test_refresh_fail_repeating_requests(self): response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) self.assertEqual(response.status_code, 401) + def test_refresh_repeating_requests(self): + """ + Trying to refresh an access token with the same refresh token more than + once succeeds in the grace period and fails outside + """ + oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS = 120 + self.client.login(username="test_user", password="123456") + authorization_code = self.get_auth() + + token_request_data = { + "grant_type": "authorization_code", + "code": authorization_code, + "redirect_uri": "http://example.org" + } + auth_headers = get_basic_auth_header(self.application.client_id, self.application.client_secret) + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + content = json.loads(response.content.decode("utf-8")) + self.assertTrue("refresh_token" in content) + + token_request_data = { + "grant_type": "refresh_token", + "refresh_token": content["refresh_token"], + "scope": content["scope"], + } + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 200) + + # try refreshing outside the refresh window, see #497 + rt = RefreshToken.objects.get(token=content["refresh_token"]) + self.assertIsNotNone(rt.revoked) + rt.revoked = timezone.now() - datetime.timedelta(minutes=10) # instead of mocking out datetime + rt.save() + + response = self.client.post(reverse("oauth2_provider:token"), data=token_request_data, **auth_headers) + self.assertEqual(response.status_code, 401) + oauth2_settings.REFRESH_TOKEN_GRACE_PERIOD_SECONDS = 0 + def test_refresh_repeating_requests_non_rotating_tokens(self): """ Try refreshing an access token with the same refresh token more than once when not rotating tokens. diff --git a/tests/test_oauth2_validators.py b/tests/test_oauth2_validators.py index 92f024f48..d8ab2349b 100644 --- a/tests/test_oauth2_validators.py +++ b/tests/test_oauth2_validators.py @@ -236,7 +236,7 @@ def test_save_bearer_token__with_new_token_equal_to_existing_token__revokes_old_ self.validator.save_bearer_token(token, self.request) - self.assertEqual(1, RefreshToken.objects.count()) + self.assertEqual(1, RefreshToken.objects.filter(revoked__isnull=True).count()) self.assertEqual(1, AccessToken.objects.count()) def test_save_bearer_token__with_no_refresh_token__creates_new_access_token_only(self): diff --git a/tests/test_token_revocation.py b/tests/test_token_revocation.py index 8870b9d7b..c7520641a 100644 --- a/tests/test_token_revocation.py +++ b/tests/test_token_revocation.py @@ -149,7 +149,8 @@ def test_revoke_refresh_token(self): url = "{url}?{qs}".format(url=reverse("oauth2_provider:revoke-token"), qs=query_string) response = self.client.post(url) self.assertEqual(response.status_code, 200) - self.assertFalse(RefreshToken.objects.filter(id=rtok.id).exists()) + refresh_token = RefreshToken.objects.filter(id=rtok.id).first() + self.assertIsNotNone(refresh_token.revoked) self.assertFalse(AccessToken.objects.filter(id=rtok.access_token.id).exists()) def test_revoke_token_with_wrong_hint(self):