Repository: box/box-python-sdk

Issue:
BoxSession._renew_session does not set previously refreshed tokens
I am using `CooperativelyManagedOAuth2` so that I can use the same oauth tokens for multiple clients but it seems like refreshed tokens do not get updated in the session if the tokens were refreshed by another client. I have two instances of the oauth class using the same tokens and two clients using one of the instances.

For example, say there are two clients initialized with the same tokens but different `CooperativelyManagedOAuth2` instances. The first client makes a request, sees the access token is expired and refreshes the tokens. The tokens in the first client's oauth instance are updated with the new tokens. The second client then makes a request, sees it's access token is expired and tries to refresh the token. The token is already refreshed so it does not try to refresh them again but then does not replace the old tokens so when client two retries the request it will raise a `BoxAPIException`.

Discussion:

Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index ec26cb8..e7ca71f 100644
--- a/HISTORY.rst
+++ b/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)
 ++++++++++++++++++
 
diff --git a/boxsdk/auth/oauth2.py b/boxsdk/auth/oauth2.py
index a810e79..6b0e9d5 100644
--- a/boxsdk/auth/oauth2.py
+++ b/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
diff --git a/boxsdk/auth/redis_managed_oauth2.py b/boxsdk/auth/redis_managed_oauth2.py
index f333849..a8d85ac 100644
--- a/boxsdk/auth/redis_managed_oauth2.py
+++ b/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):
         """
diff --git a/boxsdk/version.py b/boxsdk/version.py
index c64a173..9f6ca5f 100644
--- a/boxsdk/version.py
+++ b/boxsdk/version.py
@@ -3,4 +3,4 @@
 from __future__ import unicode_literals, absolute_import
 
 
-__version__ = '1.5.1'
+__version__ = '1.5.2'
diff --git a/requirements-dev.txt b/requirements-dev.txt
index f962a37..66fa4cd 100644
--- a/requirements-dev.txt
+++ b/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 patch:
diff --git a/test/conftest.py b/test/conftest.py
index c5e61dd..a5958da 100644
--- a/test/conftest.py
+++ b/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'
diff --git a/test/unit/auth/test_jwt_auth.py b/test/unit/auth/test_jwt_auth.py
index c4d5f11..a117850 100644
--- a/test/unit/auth/test_jwt_auth.py
+++ b/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(),
             )
diff --git a/test/unit/auth/test_oauth2.py b/test/unit/auth/test_oauth2.py
index af4d6ed..8fdf7b2 100644
--- a/test/unit/auth/test_oauth2.py
+++ b/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
diff --git a/test/unit/auth/test_redis_managed_oauth2.py b/test/unit/auth/test_redis_managed_oauth2.py
index c03eec1..dc67097 100644
--- a/test/unit/auth/test_redis_managed_oauth2.py
+++ b/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)
 
 
diff --git a/test/unit/network/test_logging_network.py b/test/unit/network/test_logging_network.py
index 3bd5eb0..58a8c61 100644
--- a/test/unit/network/test_logging_network.py
+++ b/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