Merge pull request #134 from box/issue_128

Always update `OAuth2` object with new tokens

Author: jmoldow

HISTORY.rst

@@ -6,6 +6,11 @@ Release History
 Upcoming
 ++++++++
 
+1.5.2
+++++++++++++++++++
+
+- Bugfix so that ``OAuth2`` always has the correct tokens after a call to ``refresh()``.
+
 1.5.1 (2016-03-23)
 ++++++++++++++++++
 

boxsdk/auth/oauth2.py

@@ -167,10 +167,17 @@ def _get_tokens(self):
         """
         Get the current access and refresh tokens.
 
+        This is a protected method that can be overridden to look up tokens
+        from an external source (the inverse of the `store_tokens` callback).
+
+        This method does not need to update this object's private token
+        attributes. Its caller in :class:`OAuth2` is responsible for that.
+
         :return:
             Tuple containing the current access token and refresh token.
+            One or both of them may be `None`, if they aren't set.
         :rtype:
-            `tuple` of (`unicode`, `unicode`)
+            `tuple` of ((`unicode` or `None`), (`unicode` or `None`))
         """
         return self._access_token, self._refresh_token
 
@@ -181,16 +188,24 @@ def refresh(self, access_token_to_refresh):
 
         :param access_token_to_refresh:
             The expired access token, which needs to be refreshed.
+            Pass `None` if you don't have the access token.
         :type access_token_to_refresh:
-            `unicode`
+            `unicode` or `None`
+        :return:
+            Tuple containing the new access token and refresh token.
+            The refresh token may be `None`, if the authentication scheme
+            doesn't use one, or keeps it hidden from this client.
+        :rtype:
+            `tuple` of (`unicode`, (`unicode` or `None`))
         """
         with self._refresh_lock:
-            access_token, refresh_token = self._get_tokens()
+            access_token, refresh_token = self._get_and_update_current_tokens()
             # The lock here is for handling that case that multiple requests fail, due to access token expired, at the
             # same time to avoid multiple session renewals.
-            if access_token_to_refresh == access_token:
-                # If the active access token is the same as the token needs to be refreshed, we make the request to
-                # refresh the token.
+            if (access_token is None) or (access_token_to_refresh == access_token):
+                # If the active access token is the same as the token needs to
+                # be refreshed, or if we don't currently have any active access
+                # token, we make the request to refresh the token.
                 return self._refresh(access_token_to_refresh)
             else:
                 # If the active access token (self._access_token) is not the same as the token needs to be refreshed,
@@ -213,11 +228,37 @@ def _get_state_csrf_token():
         return 'box_csrf_token_' + ''.join(ascii_alphabet[int(system_random.random() * ascii_len)] for _ in range(16))
 
     def _store_tokens(self, access_token, refresh_token):
-        self._access_token = access_token
-        self._refresh_token = refresh_token
+        self._update_current_tokens(access_token, refresh_token)
         if self._store_tokens_callback is not None:
             self._store_tokens_callback(access_token, refresh_token)
 
+    def _get_and_update_current_tokens(self):
+        """Get the current access and refresh tokens, while also storing them in this object's private attributes.
+
+        :return:
+            Same as for :meth:`_get_tokens()`.
+        """
+        tokens = self._get_tokens()
+        self._update_current_tokens(*tokens)
+        return tokens
+
+    def _update_current_tokens(self, access_token, refresh_token):
+        """Store the latest tokens in this object's private attributes.
+
+        :param access_token:
+            The latest access token.
+            May be `None`, if it hasn't been provided.
+        :type access_token:
+            `unicode` or `None`
+        :param refresh_token:
+            The latest refresh token.
+            May be `None`, if the authentication scheme doesn't use one, or if
+            it hasn't been provided.
+        :type refresh_token:
+            `unicode` or `None`
+        """
+        self._access_token, self._refresh_token = access_token, refresh_token
+
     def send_token_request(self, data, access_token, expect_refresh_token=True):
         """
         Send the request to acquire or refresh an access token.
@@ -262,7 +303,7 @@ def revoke(self):
         Revoke the authorization for the current access/refresh token pair.
         """
         with self._refresh_lock:
-            access_token, refresh_token = self._get_tokens()
+            access_token, refresh_token = self._get_and_update_current_tokens()
             token_to_revoke = access_token or refresh_token
             if token_to_revoke is None:
                 return

boxsdk/auth/redis_managed_oauth2.py

@@ -30,13 +30,7 @@ def __init__(self, unique_id=uuid4(), redis_server=None, *args, **kwargs):
         refresh_lock = Lock(redis=self._redis_server, name='{0}_lock'.format(self._unique_id))
         super(RedisManagedOAuth2Mixin, self).__init__(*args, refresh_lock=refresh_lock, **kwargs)
         if self._access_token is None:
-            self._update_current_tokens()
-
-    def _update_current_tokens(self):
-        """
-        Get the latest tokens from redis and store them.
-        """
-        self._access_token, self._refresh_token = self._redis_server.hvals(self._unique_id) or (None, None)
+            self._get_and_update_current_tokens()
 
     @property
     def unique_id(self):
@@ -51,8 +45,7 @@ def _get_tokens(self):
         Base class override.
         Gets the latest tokens from redis before returning them.
         """
-        self._update_current_tokens()
-        return super(RedisManagedOAuth2Mixin, self)._get_tokens()
+        return self._redis_server.hvals(self._unique_id) or (None, None)
 
     def _store_tokens(self, access_token, refresh_token):
         """

boxsdk/version.py

@@ -3,4 +3,4 @@
 from __future__ import unicode_literals, absolute_import
 
 
-__version__ = '1.5.1'
+__version__ = '1.5.2'

requirements-dev.txt

@@ -1,17 +1,10 @@
 -r requirements.txt
 bottle
 jsonpatch
-mock<=1.0.1
+mock>=2.0.0
 pep8
 pylint
-
-# Temporary version exclusion of the 2.8 release line.
-# <https://github.com/pytest-dev/pytest/issues/1085> breaks pytest on Python 2, only in 2.8.1. Fixed in upcoming 2.8.2.
-# <https://github.com/pytest-dev/pytest/issues/1035> breaks pytest on Python 2.6, on all currently existing 2.8.*
-# releases. Has not yet been fixed in the master branch, so there isn't a guarantee that it will work in the upcoming
-# 2.8.2 release.
-pytest<2.8
-
+pytest>=2.8.3
 pytest-cov
 pytest-xdist
 sphinx

test/conftest.py

@@ -102,6 +102,12 @@ def access_token():
     return 'T9cE5asGnuyYCCqIZFoWjFHvNbvVqHjl'
 
 
+@pytest.fixture(scope='session')
+def new_access_token():
+    # Must be distinct from access_token.
+    return 'ZFoWjFHvNbvVqHjlT9cE5asGnuyYCCqI'
+
+
 @pytest.fixture(scope='session')
 def refresh_token():
     return 'J7rxTiWOHMoSC1isKZKBZWizoRXjkQzig5C6jFgCVJ9bUnsUfGMinKBDLZWP9BgRb'

test/unit/auth/test_jwt_auth.py

@@ -73,8 +73,8 @@ def jwt_auth_init_mocks(
     }
 
     mock_network_layer.request.return_value = successful_token_response
-    key_file = mock_open()
-    with patch('boxsdk.auth.jwt_auth.open', key_file, create=True) as jwt_auth_open:
+    key_file_read_data = 'key_file_read_data'
+    with patch('boxsdk.auth.jwt_auth.open', mock_open(read_data=key_file_read_data), create=True) as jwt_auth_open:
         with patch('cryptography.hazmat.primitives.serialization.load_pem_private_key') as load_pem_private_key:
             oauth = JWTAuth(
                 client_id=fake_client_id,
@@ -89,9 +89,9 @@ def jwt_auth_init_mocks(
             )
 
             jwt_auth_open.assert_called_once_with(sentinel.rsa_path)
-            key_file.return_value.read.assert_called_once_with()  # pylint:disable=no-member
+            jwt_auth_open.return_value.read.assert_called_once_with()  # pylint:disable=no-member
             load_pem_private_key.assert_called_once_with(
-                key_file.return_value.read.return_value,  # pylint:disable=no-member
+                key_file_read_data,
                 password=rsa_passphrase,
                 backend=default_backend(),
             )

test/unit/auth/test_oauth2.py

@@ -5,9 +5,11 @@
 from functools import partial
 import re
 from threading import Thread
+import uuid
 
 from mock import Mock
 import pytest
+from six.moves import range   # pylint:disable=redefined-builtin
 from six.moves.urllib import parse as urlparse  # pylint:disable=import-error,no-name-in-module,wrong-import-order
 
 from boxsdk.exception import BoxOAuthException
@@ -314,3 +316,41 @@ def test_revoke_sends_revoke_request(
         access_token=access_token,
     )
     assert oauth.access_token is None
+
+
+def test_tokens_get_updated_after_noop_refresh(client_id, client_secret, access_token, new_access_token, refresh_token, mock_network_layer):
+    """`OAuth2` object should update its state with new tokens, after no-op refresh.
+
+    If the protected method `_get_tokens()` returns new tokens, refresh is
+    skipped, and those tokens are used.
+
+    This is a regression test for issue #128 [1]. We would return the new
+    tokens without updating the object state. Subsequent uses of the `OAuth2`
+    object would use the old tokens.
+
+    [1] <https://github.com/box/box-python-sdk/issues/128>
+    """
+    new_refresh_token = uuid.uuid4().hex
+    new_tokens = (new_access_token, new_refresh_token)
+
+    class GetTokensOAuth2(OAuth2):
+        def _get_tokens(self):
+            """Return a new set of tokens, without updating any state.
+
+            In order for the test to pass, the `OAuth2` object must be
+            correctly programmed to take this return value and use it to update
+            its state.
+            """
+            return new_tokens
+
+    oauth = GetTokensOAuth2(
+        client_id=client_id,
+        client_secret=client_secret,
+        access_token=access_token,
+        refresh_token=refresh_token,
+        network_layer=mock_network_layer,
+    )
+    assert oauth.access_token == access_token
+
+    assert oauth.refresh(access_token) == new_tokens
+    assert oauth.access_token == new_access_token

test/unit/auth/test_redis_managed_oauth2.py

@@ -2,6 +2,8 @@
 
 from __future__ import unicode_literals, absolute_import
 
+import uuid
+
 from mock import Mock, patch
 
 from boxsdk.auth import redis_managed_oauth2
@@ -21,19 +23,24 @@ def test_redis_managed_oauth2_gets_tokens_from_redis_on_init(access_token, refre
     assert oauth2.unique_id is unique_id
 
 
-def test_redis_managed_oauth2_gets_tokens_from_redis_during_refresh(access_token, refresh_token):
+def test_redis_managed_oauth2_gets_tokens_from_redis_during_refresh(access_token, refresh_token, new_access_token):
+    new_refresh_token = uuid.uuid4().hex
     redis_server = Mock(redis_managed_oauth2.StrictRedis)
-    redis_server.hvals.return_value = access_token, refresh_token
+    redis_server.hvals.return_value = new_access_token, new_refresh_token
     unique_id = Mock()
-    with patch.object(redis_managed_oauth2.RedisManagedOAuth2Mixin, '_update_current_tokens'):
-        oauth2 = redis_managed_oauth2.RedisManagedOAuth2(
-            client_id=None,
-            client_secret=None,
-            unique_id=unique_id,
-            redis_server=redis_server,
-        )
+    oauth2 = redis_managed_oauth2.RedisManagedOAuth2(
+        access_token=access_token,
+        refresh_token=refresh_token,
+        client_id=None,
+        client_secret=None,
+        unique_id=unique_id,
+        redis_server=redis_server,
+    )
+    assert oauth2.access_token == access_token
+    redis_server.hvals.assert_not_called()
 
-    assert oauth2.refresh('bogus_access_token') == (access_token, refresh_token)
+    assert oauth2.refresh('bogus_access_token') == (new_access_token, new_refresh_token)
+    assert oauth2.access_token == new_access_token
     redis_server.hvals.assert_called_once_with(unique_id)
 
 

test/unit/network/test_logging_network.py

@@ -28,7 +28,7 @@ def test_logging_network_does_not_call_setup_logging_if_logger_is_not_none():
     logger = Mock(Logger)
     with patch.object(logging_network, 'setup_logging') as setup_logging:
         network = LoggingNetwork(logger)
-        setup_logging.assert_never_called()
+        setup_logging.assert_not_called()
         assert network.logger is logger