diff --git a/HISTORY.rst b/HISTORY.rst index ec26cb88c..e7ca71f03 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 a810e7940..6b0e9d587 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 f33384998..a8d85aca7 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 c64a17351..9f6ca5fed 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 f962a371b..66fa4cd6a 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. -# breaks pytest on Python 2, only in 2.8.1. Fixed in upcoming 2.8.2. -# 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 diff --git a/test/conftest.py b/test/conftest.py index c5e61dd61..a5958da65 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 c4d5f11dd..a1178506c 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 af4d6ed00..8fdf7b2cb 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] + """ + 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 c03eec174..dc6709772 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 3bd5eb07b..58a8c61da 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