Repository: globus/globus-sdk-python
Pull request #173 (2017-04-05 20:33:03)

Fix the broken contract with on_refresh
The RenewingAuthorizer broke our contract with supplied `on_refresh` callbacks. Those are supposed to be called on the raw response -- to do whatever manipulation they would like. Instead, they were being called on the `by_resource_server` dict property.

Fix this by separating the steps of getting a token (and response) from processing it. This might not be the cleanest imaginable fix, but it works.


Patch:
diff --git a/globus_sdk/authorizers/client_credentials.py b/globus_sdk/authorizers/client_credentials.py
index f087c675..225ea426 100644
--- a/globus_sdk/authorizers/client_credentials.py
+++ b/globus_sdk/authorizers/client_credentials.py
@@ -64,14 +64,18 @@ class ClientCredentialsAuthorizer(RenewingAuthorizer):
         super(ClientCredentialsAuthorizer, self).__init__(
             access_token, expires_at, on_refresh)
 
-    def _get_token_data(self):
+    def _get_token_response(self):
         """
-        Make a client credentials grant, get the tokens .by_resource_server,
-        Ensure that only one token was gotten, and return that token.
+        Make a client credentials grant
         """
-        res = self.confidential_client.oauth2_client_credentials_tokens(
+        return self.confidential_client.oauth2_client_credentials_tokens(
             requested_scopes=self.scopes)
 
+    def _extract_token_data(self, res):
+        """
+        Get the tokens .by_resource_server,
+        Ensure that only one token was gotten, and return that token.
+        """
         token_data = res.by_resource_server.values()
         if len(token_data) != 1:
             raise ValueError(
diff --git a/globus_sdk/authorizers/refresh_token.py b/globus_sdk/authorizers/refresh_token.py
index ab21b7c8..6ca6e458 100644
--- a/globus_sdk/authorizers/refresh_token.py
+++ b/globus_sdk/authorizers/refresh_token.py
@@ -59,13 +59,17 @@ class RefreshTokenAuthorizer(RenewingAuthorizer):
         super(RefreshTokenAuthorizer, self).__init__(
             access_token, expires_at, on_refresh)
 
-    def _get_token_data(self):
+    def _get_token_response(self):
         """
-        Make a refresh token grant, get the tokens .by_resource_server,
-        Ensure that only one token was gotten, and return that token.
+        Make a refresh token grant
         """
-        res = self.auth_client.oauth2_refresh_token(self.refresh_token)
+        return self.auth_client.oauth2_refresh_token(self.refresh_token)
 
+    def _extract_token_data(self, res):
+        """
+        Get the tokens .by_resource_server,
+        Ensure that only one token was gotten, and return that token.
+        """
         token_data = res.by_resource_server.values()
         if len(token_data) != 1:
             raise ValueError(
diff --git a/globus_sdk/authorizers/renewing.py b/globus_sdk/authorizers/renewing.py
index 80f513c2..8a47f22e 100644
--- a/globus_sdk/authorizers/renewing.py
+++ b/globus_sdk/authorizers/renewing.py
@@ -25,7 +25,8 @@ class RenewingAuthorizer(GlobusAuthorizer):
     expiration time, callbacks on renewal, and 401 handling.
 
     To make an authorizer that implements this class implement
-    the _get_token_data method for that authorization type.
+    the _get_token_response and _extract_token_data methods for that
+    authorization type,
     """
 
     def __init__(self, access_token=None, expires_at=None, on_refresh=None):
@@ -61,11 +62,17 @@ class RenewingAuthorizer(GlobusAuthorizer):
             self._get_new_access_token()
 
     @abc.abstractmethod
-    def _get_token_data(self):
+    def _get_token_response(self):
         """
-        Get the first element of token_response.by_resource_server using
-        whatever flow or client or credentials the specific authorizer
-        implementing this class uses.
+        Using whatever method the specific authorizer implementing this class
+        does, get a new token response.
+        """
+
+    @abc.abstractmethod
+    def _extract_token_data(self, res):
+        """
+        Given a token response object, get the first element of
+        token_response.by_resource_server
         This method is expected to enforce that by_resource_server is only
         returning one access token, and return a ValueError otherwise.
         """
@@ -81,11 +88,12 @@ class RenewingAuthorizer(GlobusAuthorizer):
 
     def _get_new_access_token(self):
         """
-        Given token data from _get_token_data,
+        Given token data from _get_token_response and _extract_token_data,
         set the access token and expiration time, and call on_refresh
         """
-        # get the first (and only) item from this iterable
-        token_data = self._get_token_data()
+        # get the first (and only) token
+        res = self._get_token_response()
+        token_data = self._extract_token_data(res)
 
         self._set_expiration_time(token_data['expires_at_seconds'])
         self.access_token = token_data['access_token']
@@ -95,7 +103,7 @@ class RenewingAuthorizer(GlobusAuthorizer):
                     .format(self.access_token[-5:]))
 
         if callable(self.on_refresh):
-            self.on_refresh(token_data)
+            self.on_refresh(res)
             logger.debug("Invoked on_refresh callback")
 
     def _check_expiration_time(self):


Test patch:
diff --git a/tests/unit/test_client_credentials_authorizer.py b/tests/unit/test_client_credentials_authorizer.py
index 498d1ccf..06212b1a 100644
--- a/tests/unit/test_client_credentials_authorizer.py
+++ b/tests/unit/test_client_credentials_authorizer.py
@@ -40,15 +40,15 @@ class ClientCredentialsAuthorizerTests(CapturedIOTestCase):
             self.cac, self.scopes, access_token=self.access_token,
             expires_at=self.expires_at)
 
-    def test_get_token_data(self):
+    def test_get_token_response(self):
         """
-        Calls _get_token_data, confirms that the mock
+        Calls _get_token_response, confirms that the mock
         ConfidentialAppAuthClient is used and the known data was returned.
         """
         # get new_access_token
-        res = self.authorizer._get_token_data()
+        res = self.authorizer._get_token_response()
         # confirm expected response
-        self.assertEqual(res, self.rs_data)
+        self.assertEqual(res, self.response)
         # confirm mock ConfidentailAppAuthClient was used as expected
         self.cac.oauth2_client_credentials_tokens.assert_called_once_with(
             requested_scopes=self.scopes)
@@ -56,7 +56,8 @@ class ClientCredentialsAuthorizerTests(CapturedIOTestCase):
     def test_multiple_resource_servers(self):
         """
         Sets the mock ConfidentialAppAuthClient to return multiple resource
-        servers. Confirms GlobusError is raised when _get_token_data is called.
+        servers. Confirms GlobusError is raised when _extract_token_data is
+        called.
         """
         self.response.by_resource_server = {
             "rs1": {
@@ -69,7 +70,7 @@ class ClientCredentialsAuthorizerTests(CapturedIOTestCase):
             }
         }
         with self.assertRaises(ValueError) as err:
-            self.authorizer._get_token_data()
+            self.authorizer._extract_token_data(self.response)
 
         self.assertIn("didn't return exactly one token", str(err.exception))
         self.assertIn(self.scopes, str(err.exception))
diff --git a/tests/unit/test_refresh_token_authorizer.py b/tests/unit/test_refresh_token_authorizer.py
index e6978071..cbf720e5 100644
--- a/tests/unit/test_refresh_token_authorizer.py
+++ b/tests/unit/test_refresh_token_authorizer.py
@@ -38,15 +38,15 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
             self.refresh_token, self.ac, access_token=self.access_token,
             expires_at=self.expires_at)
 
-    def test_get_token_data(self):
+    def test_get_token_response(self):
         """
-        Calls _get_token_data, confirms that the mock
+        Calls _get_token_response, confirms that the mock
         AuthClient is used and the known data was returned.
         """
         # get new_access_token
-        res = self.authorizer._get_token_data()
+        res = self.authorizer._get_token_response()
         # confirm expected response
-        self.assertEqual(res, self.rs_data)
+        self.assertEqual(res, self.response)
         # confirm mock ConfidentailAppAuthClient was used as expected
         self.ac.oauth2_refresh_token.assert_called_once_with(
             self.refresh_token)
@@ -54,7 +54,8 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
     def test_multiple_resource_servers(self):
         """
         Sets the mock ConfidentialAppAuthClient to return multiple resource
-        servers. Confirms GlobusError is raised when _get_token_data is called.
+        servers. Confirms GlobusError is raised when _extract_token_data is
+        called.
         """
         self.response.by_resource_server = {
             "rs1": {
@@ -67,5 +68,5 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
             }
         }
         with self.assertRaises(ValueError) as err:
-            self.authorizer._get_token_data()
+            self.authorizer._extract_token_data(self.response)
         self.assertIn("didn't return exactly one token", str(err.exception))
diff --git a/tests/unit/test_renewing_authorizer.py b/tests/unit/test_renewing_authorizer.py
index 702165b2..c3146b93 100644
--- a/tests/unit/test_renewing_authorizer.py
+++ b/tests/unit/test_renewing_authorizer.py
@@ -11,14 +11,18 @@ from tests.framework import CapturedIOTestCase
 
 class MockRenewer(RenewingAuthorizer):
     """
-    Class that implements RenewingAuthorizer so that _get_token_data
-    can return known values for testing
+    Class that implements RenewingAuthorizer so that _get_token_response and
+    _extract_token_data can return known values for testing
     """
     def __init__(self, token_data, **kwargs):
         self.token_data = token_data
+        self.token_response = mock.Mock()
         super(MockRenewer, self).__init__(**kwargs)
 
-    def _get_token_data(self):
+    def _get_token_response(self):
+        return self.token_response
+
+    def _extract_token_data(self, res):
         return self.token_data