Reuse refresh tokens

Reuse refresh tokens if it's enabled. This is based on a code sample
in #304 from #304 (comment)

For us, this has cleaned up a number of race conditions deleting and recreating
tokens.

Author: jimgraham

oauth2_provider/oauth2_validators.py

@@ -10,9 +10,11 @@
 from django.conf import settings
 from django.contrib.auth import authenticate
 from django.core.exceptions import ObjectDoesNotExist
+from django.db import transaction
 from oauthlib.oauth2 import RequestValidator
 
 from .compat import unquote_plus
+from .exceptions import FatalClientError
 from .models import Grant, AccessToken, RefreshToken, get_application_model, AbstractApplication
 from .settings import oauth2_settings
 
@@ -86,6 +88,9 @@ def _authenticate_basic_auth(self, request):
         if self._load_application(client_id, request) is None:
             log.debug("Failed basic auth: Application %s does not exist" % client_id)
             return False
+        elif request.client.client_id != client_id:
+            log.debug("Failed basic auth: wrong client id %s" % client_id)
+            return False
         elif request.client.client_secret != client_secret:
             log.debug("Failed basic auth: wrong client secret %s" % client_secret)
             return False
@@ -292,41 +297,88 @@ def save_authorization_code(self, client_id, code, request, *args, **kwargs):
                   scope=' '.join(request.scopes))
         g.save()
 
+    @transaction.atomic
     def save_bearer_token(self, token, request, *args, **kwargs):
         """
-        Save access and refresh token, If refresh token is issued, remove old refresh tokens as
-        in rfc:`6`
+        Save access and refresh token, If refresh token is issued, remove or
+        reuse old refresh token as in rfc:`6`
+
+        @see: https://tools.ietf.org/html/draft-ietf-oauth-v2-31#page-43
         """
-        if request.refresh_token:
-            # remove used refresh token
-            try:
-                RefreshToken.objects.get(token=request.refresh_token).revoke()
-            except RefreshToken.DoesNotExist:
-                assert()  # TODO though being here would be very strange, at least log the error
+
+        if 'scope' not in token:
+            raise FatalClientError(u"Failed to renew access token: missing scope")
 
         expires = timezone.now() + timedelta(seconds=oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS)
+
         if request.grant_type == 'client_credentials':
             request.user = None
 
+        # This comes from OAuthLib:
+        # https://github.com/idan/oauthlib/blob/1.0.3/oauthlib/oauth2/rfc6749/tokens.py#L267
+        # Its value is either a new random code; or if we are reusing
+        # refresh tokens, then it is the same value that the request passed in
+        # (stored in `request.refresh_token`)
+        refresh_token_code = token.get('refresh_token', None)
+
+        if refresh_token_code:
+            # an instance of `RefreshToken` that matches the old refresh code.
+            # Set on the request in `validate_refresh_token`
+            refresh_token_instance = getattr(request, 'refresh_token_instance', None)
+
+            # If we are to reuse tokens, and we can: do so
+            if not self.rotate_refresh_token(request) and \
+                isinstance(refresh_token_instance, RefreshToken) and \
+                    refresh_token_instance.access_token:
+
+                access_token = AccessToken.objects.select_for_update().get(
+                    pk=refresh_token_instance.access_token.pk
+                )
+                access_token.user = request.user
+                access_token.scope = token['scope']
+                access_token.expires = expires
+                access_token.token = token['access_token']
+                access_token.application = request.client
+                access_token.save()
+
+            # else create fresh with access & refresh tokens
+            else:
+                # revoke existing tokens if possible
+                if isinstance(refresh_token_instance, RefreshToken):
+                    try:
+                        refresh_token_instance.revoke()
+                    except (AccessToken.DoesNotExist, RefreshToken.DoesNotExist):
+                        pass
+                    else:
+                        setattr(request, 'refresh_token_instance', None)
+
+                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()
+
+        # No refresh token should be created, just access token
+        else:
+            self._create_access_token(expires, request, token)
+
+        # 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):
         access_token = AccessToken(
             user=request.user,
             scope=token['scope'],
             expires=expires,
             token=token['access_token'],
-            application=request.client)
+            application=request.client
+        )
         access_token.save()
-
-        if 'refresh_token' in token:
-            refresh_token = RefreshToken(
-                user=request.user,
-                token=token['refresh_token'],
-                application=request.client,
-                access_token=access_token
-            )
-            refresh_token.save()
-
-        # TODO check out a more reliable way to communicate expire time to oauthlib
-        token['expires_in'] = oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS
+        return access_token
 
     def revoke_token(self, token, token_type_hint, request, *args, **kwargs):
         """

oauth2_provider/tests/test_oauth2_validators.py

@@ -1,25 +1,31 @@
+from datetime import timedelta
+
 from django.contrib.auth import get_user_model
-from django.test import TestCase
+from django.test import TransactionTestCase
+from django.utils import timezone
 
 import mock
 from oauthlib.common import Request
 
+from ..exceptions import FatalClientError
 from ..oauth2_validators import OAuth2Validator
-from ..models import get_application_model
+from ..models import get_application_model, AccessToken, RefreshToken
 
 UserModel = get_user_model()
 AppModel = get_application_model()
 
 
-class TestOAuth2Validator(TestCase):
+class TestOAuth2Validator(TransactionTestCase):
     def setUp(self):
         self.user = UserModel.objects.create_user("user", "test@user.com", "123456")
         self.request = mock.MagicMock(wraps=Request)
-        self.request.client = None
+        self.request.user = self.user
+        self.request.grant_type = "not client"
         self.validator = OAuth2Validator()
         self.application = AppModel.objects.create(
             client_id='client_id', client_secret='client_secret', user=self.user,
             client_type=AppModel.CLIENT_PUBLIC, authorization_grant_type=AppModel.GRANT_PASSWORD)
+        self.request.client = self.application
 
     def tearDown(self):
         self.application.delete()
@@ -108,3 +114,129 @@ def test_client_authentication_required(self):
 
     def test_load_application_fails_when_request_has_no_client(self):
         self.assertRaises(AssertionError, self.validator.authenticate_client_id, 'client_id', {})
+
+    def test_rotate_refresh_token__is_true(self):
+        self.assertTrue(self.validator.rotate_refresh_token(mock.MagicMock()))
+
+    def test_save_bearer_token__without_user__raises_fatal_client(self):
+        token = {}
+
+        with self.assertRaises(FatalClientError):
+            self.validator.save_bearer_token(token, mock.MagicMock())
+
+    def test_save_bearer_token__with_existing_tokens__does_not_create_new_tokens(self):
+
+        rotate_token_function = mock.MagicMock()
+        rotate_token_function.return_value = False
+        self.validator.rotate_refresh_token = rotate_token_function
+
+        access_token = AccessToken.objects.create(
+            token="123",
+            user=self.user,
+            expires=timezone.now() + timedelta(seconds=60),
+            application=self.application
+        )
+        refresh_token = RefreshToken.objects.create(
+            access_token=access_token,
+            token="abc",
+            user=self.user,
+            application=self.application
+        )
+        self.request.refresh_token_instance = refresh_token
+        token = {
+            "scope": "foo bar",
+            "refresh_token": "abc",
+            "access_token": "123",
+        }
+
+        self.assertEqual(1, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())
+
+        self.validator.save_bearer_token(token, self.request)
+
+        self.assertEqual(1, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())
+
+    def test_save_bearer_token__checks_to_rotate_tokens(self):
+
+        rotate_token_function = mock.MagicMock()
+        rotate_token_function.return_value = False
+        self.validator.rotate_refresh_token = rotate_token_function
+
+        access_token = AccessToken.objects.create(
+            token="123",
+            user=self.user,
+            expires=timezone.now() + timedelta(seconds=60),
+            application=self.application
+        )
+        refresh_token = RefreshToken.objects.create(
+            access_token=access_token,
+            token="abc",
+            user=self.user,
+            application=self.application
+        )
+        self.request.refresh_token_instance = refresh_token
+        token = {
+            "scope": "foo bar",
+            "refresh_token": "abc",
+            "access_token": "123",
+        }
+
+        self.validator.save_bearer_token(token, self.request)
+        rotate_token_function.assert_called_once_with(self.request)
+
+    def test_save_bearer_token__with_new_token__creates_new_tokens(self):
+        token = {
+            "scope": "foo bar",
+            "refresh_token": "abc",
+            "access_token": "123",
+        }
+
+        self.assertEqual(0, RefreshToken.objects.count())
+        self.assertEqual(0, AccessToken.objects.count())
+
+        self.validator.save_bearer_token(token, self.request)
+
+        self.assertEqual(1, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())
+
+    def test_save_bearer_token__with_new_token_equal_to_existing_token__revokes_old_tokens(self):
+        access_token = AccessToken.objects.create(
+            token="123",
+            user=self.user,
+            expires=timezone.now() + timedelta(seconds=60),
+            application=self.application
+        )
+        refresh_token = RefreshToken.objects.create(
+            access_token=access_token,
+            token="abc",
+            user=self.user,
+            application=self.application
+        )
+
+        self.request.refresh_token_instance = refresh_token
+
+        token = {
+            "scope": "foo bar",
+            "refresh_token": "abc",
+            "access_token": "123",
+        }
+
+        self.assertEqual(1, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())
+
+        self.validator.save_bearer_token(token, self.request)
+
+        self.assertEqual(1, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())
+
+    def test_save_bearer_token__with_no_refresh_token__creates_new_access_token_only(self):
+        token = {
+            "scope": "foo bar",
+            "access_token": "123",
+        }
+
+        self.validator.save_bearer_token(token, self.request)
+
+        self.assertEqual(0, RefreshToken.objects.count())
+        self.assertEqual(1, AccessToken.objects.count())