Repository: globus/globus-sdk-python
Pull request #162 (2017-03-23 18:15:46)

Remove confusing top-level attrs of Token Response
After some thought about #158, I've become convinced that these attributes should be removed.
The silent "promotion" of scopes by Auth, the lack of any public specification as to the method of determination for that top-level token, and the inability of users to change what that token is by any means mean that these properties offer very little value and a lot of confusion.

`OAuthTokenResponse` top-level properties for accessing the top-level token aren't the desired/canonical/safe way to access tokens. We're telling people to always use `by_resource_server`, so we should cut down the interface for this class to that and only that.

Minor change included: make the dependent token response a subclass of the typical token response, only overriding a helper used by `__init__`.

This will require that we bump to `0.7.0` in the next release.


Patch:
diff --git a/globus_sdk/auth/token_response.py b/globus_sdk/auth/token_response.py
index 04abee41..39d80fbc 100644
--- a/globus_sdk/auth/token_response.py
+++ b/globus_sdk/auth/token_response.py
@@ -34,15 +34,17 @@ class OAuthTokenResponse(GlobusHTTPResponse):
     """
     def __init__(self, *args, **kwargs):
         GlobusHTTPResponse.__init__(self, *args, **kwargs)
+        self._init_rs_dict()
 
+    def _init_rs_dict(self):
         # call the helper at the top level
         self._by_resource_server = {
-            self.resource_server: _convert_token_info_dict(self)}
+            self['resource_server']: _convert_token_info_dict(self)}
         # call the helper on everything in 'other_tokens'
         self._by_resource_server.update(dict(
             (unprocessed_item['resource_server'],
              _convert_token_info_dict(unprocessed_item))
-            for unprocessed_item in self.other_tokens))
+            for unprocessed_item in self['other_tokens']))
 
     @property
     def by_resource_server(self):
@@ -56,50 +58,6 @@ class OAuthTokenResponse(GlobusHTTPResponse):
         """
         return self._by_resource_server
 
-    @property
-    def expires_at_seconds(self):
-        """
-        A POSIX timestamp representation of the expiration
-        time for the top-level token in the response.
-        """
-        top_level_token = self._by_resource_server[self.resource_server]
-        return top_level_token['expires_at_seconds']
-
-    @property
-    def expires_in(self):
-        """
-        The ``expires_in`` value for the top-level token in the response.
-        """
-        return int(self.expires_at_seconds - time.time())
-
-    @property
-    def access_token(self):
-        """
-        The ``access_token`` value for the top-level token in the response.
-        """
-        return self['access_token']
-
-    @property
-    def refresh_token(self):
-        """
-        The ``refresh_token`` value for the top-level token in the response.
-        """
-        return self.get('refresh_token')
-
-    @property
-    def resource_server(self):
-        """
-        The ``resource_server`` value for the top-level token in the response.
-        """
-        return self['resource_server']
-
-    @property
-    def other_tokens(self):
-        """
-        The array of tokens other than the top-level one.
-        """
-        return self['other_tokens']
-
     def decode_id_token(self, auth_client):
         """
         A parsed ID Token (OIDC) as a dict.
@@ -121,7 +79,7 @@ class OAuthTokenResponse(GlobusHTTPResponse):
 
         return jwt.decode(
             self['id_token'], jwk_data,
-            access_token=self.access_token,
+            access_token=self['access_token'],
             audience=auth_client.client_id)
 
     def __str__(self):
@@ -130,30 +88,23 @@ class OAuthTokenResponse(GlobusHTTPResponse):
         return json.dumps(self.by_resource_server, indent=2)
 
 
-class OAuthDependentTokenResponse(GlobusHTTPResponse):
+class OAuthDependentTokenResponse(OAuthTokenResponse):
     """
     Class for responses from the OAuth2 code for tokens retrieved by the
     OAuth2 Dependent Token Extension Grant. For more complete docs, see
     :meth:`oauth2_get_dependent_tokens \
     <globus_sdk.ConfidentialAppAuthClient.oauth2_get_dependent_tokens>`
     """
-    def __init__(self, *args, **kwargs):
-        GlobusHTTPResponse.__init__(self, *args, **kwargs)
-
+    def _init_rs_dict(self):
         # call the helper on everything in the response array
         self._by_resource_server = dict(
             (unprocessed_item['resource_server'],
              _convert_token_info_dict(unprocessed_item))
             for unprocessed_item in self.data)
 
-    @property
-    def by_resource_server(self):
-        """
-        Representation of the token response in a ``dict`` indexed by resource
-        server.
-
-        Although ``OAuthDependentTokenResponse.data`` is still available and
-        valid, this representation is typically more desirable for applications
-        trying to use the resulting tokens.
-        """
-        return self._by_resource_server
+    def decode_id_token(self, auth_client):
+        # just in case
+        raise NotImplementedError(
+            ('OAuthDependentTokenResponse.decode_id_token() is not and cannot '
+             'be implemented. Dependent Tokens data does not include an '
+             'id_token'))
diff --git a/globus_sdk/authorizers/refresh_token.py b/globus_sdk/authorizers/refresh_token.py
index 1e9e082f..2128ca8b 100644
--- a/globus_sdk/authorizers/refresh_token.py
+++ b/globus_sdk/authorizers/refresh_token.py
@@ -110,8 +110,17 @@ class RefreshTokenAuthorizer(GlobusAuthorizer):
         """
         token_response = self.auth_client.oauth2_refresh_token(
             self.refresh_token)
-        self._set_expiration_time(token_response.expires_at_seconds)
-        self.access_token = token_response.access_token
+
+        token_data = token_response.by_resource_server.values()
+        if len(token_data) != 1:
+            raise ValueError(
+                "Attempting refresh for refresh token authorizer "
+                "didn't return exactly one value. Possible service error.")
+        # get the first (and only) item from this iterable
+        token_data = next(iter(token_data))
+
+        self._set_expiration_time(token_data['expires_at_seconds'])
+        self.access_token = token_data['access_token']
         logger.info(("RefreshTokenAuthorizer.access_token updated to "
                      '"...{}" (last 5 chars)')
                     .format(self.access_token[-5:]))


Test patch:
diff --git a/tests/integration/test_refresh_token_auth_client.py b/tests/integration/test_refresh_token_auth_client.py
index e0ef1fb7..4392b74d 100644
--- a/tests/integration/test_refresh_token_auth_client.py
+++ b/tests/integration/test_refresh_token_auth_client.py
@@ -25,9 +25,12 @@ class RefreshTokenAuthorizerIntegrationTests(CapturedIOTestCase):
                      'grant_type': 'refresh_token',
                      'client_id': client_id}
         token_res = globus_sdk.AuthClient().oauth2_token(form_data)
+        token_res = token_res.by_resource_server
 
-        self.access_token = token_res.access_token
-        self.expires_at = token_res.expires_at_seconds
+        self.access_token = token_res[
+            'transfer.api.globus.org']['access_token']
+        self.expires_at = token_res[
+            'transfer.api.globus.org']['expires_at_seconds']
 
         self.on_refresh = mock.Mock()
         self.nac = globus_sdk.NativeAppAuthClient(
diff --git a/tests/unit/test_refresh_token_authorizer.py b/tests/unit/test_refresh_token_authorizer.py
index bc056c2d..95076795 100644
--- a/tests/unit/test_refresh_token_authorizer.py
+++ b/tests/unit/test_refresh_token_authorizer.py
@@ -25,8 +25,13 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
 
         # mock response value for simulating a refresh token grant response
         self.response = mock.Mock()
-        self.response.expires_at_seconds = int(time.time()) + 1000
-        self.response.access_token = "access_token_2"
+        self.response.by_resource_server = {
+            "rs1": {
+                "expires_at_seconds": int(time.time()) + 1000,
+                "access_token": "access_token_2"
+            }
+        }
+        self.rs_data = self.response.by_resource_server['rs1']
 
         # mock AuthClient for testing
         self.ac = mock.Mock()
@@ -44,11 +49,12 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         """
         authorizer = RefreshTokenAuthorizer(
             self.refresh_token, self.ac, access_token=self.access_token)
-        self.assertEqual(authorizer.access_token, self.response.access_token)
+        self.assertEqual(authorizer.access_token, self.rs_data['access_token'])
 
         authorizer = RefreshTokenAuthorizer(
-            self.refresh_token, self.ac, expires_at=self.access_token)
-        self.assertEqual(authorizer.access_token, self.response.access_token)
+            self.refresh_token, self.ac,
+            expires_at=self.rs_data['expires_at_seconds'])
+        self.assertEqual(authorizer.access_token, self.rs_data['access_token'])
 
     def test_set_expiration_time(self):
         """
@@ -76,9 +82,9 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer._get_new_access_token()
         # confirm side effects
         self.assertEqual(self.authorizer.access_token,
-                         self.response.access_token)
+                         self.rs_data['access_token'])
         self.assertEqual(self.authorizer.expires_at,
-                         self.response.expires_at_seconds -
+                         self.rs_data['expires_at_seconds'] -
                          EXPIRES_ADJUST_SECONDS)
         self.ac.oauth2_refresh_token.assert_called_once_with(
             self.refresh_token)
@@ -98,9 +104,9 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         time.sleep(1)
         self.authorizer._check_expiration_time()
         self.assertEqual(self.authorizer.access_token,
-                         self.response.access_token)
+                         self.rs_data['access_token'])
         self.assertEqual(self.authorizer.expires_at,
-                         self.response.expires_at_seconds -
+                         self.rs_data['expires_at_seconds'] -
                          EXPIRES_ADJUST_SECONDS)
 
     def test_check_expiration_time_no_token(self):
@@ -110,9 +116,9 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer.access_token = None
         self.authorizer._check_expiration_time()
         self.assertEqual(self.authorizer.access_token,
-                         self.response.access_token)
+                         self.rs_data['access_token'])
         self.assertEqual(self.authorizer.expires_at,
-                         self.response.expires_at_seconds -
+                         self.rs_data['expires_at_seconds'] -
                          EXPIRES_ADJUST_SECONDS)
 
     def test_check_expiration_time_no_expiration(self):
@@ -122,9 +128,9 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer.expires_at = None
         self.authorizer._check_expiration_time()
         self.assertEqual(self.authorizer.access_token,
-                         self.response.access_token)
+                         self.rs_data['access_token'])
         self.assertEqual(self.authorizer.expires_at,
-                         self.response.expires_at_seconds -
+                         self.rs_data['expires_at_seconds'] -
                          EXPIRES_ADJUST_SECONDS)
 
     def test_set_authorization_header(self):
@@ -160,7 +166,7 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer.set_authorization_header(header_dict)
         # confirm value
         self.assertEqual(header_dict["Authorization"],
-                         "Bearer " + self.response.access_token)
+                         "Bearer " + self.rs_data['access_token'])
 
     def test_set_authorization_header_no_token(self):
         """
@@ -173,7 +179,7 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer.set_authorization_header(header_dict)
         # confirm value
         self.assertEqual(header_dict["Authorization"],
-                         "Bearer " + self.response.access_token)
+                         "Bearer " + self.rs_data['access_token'])
 
     def test_set_authorization_header_no_expires(self):
         """
@@ -186,7 +192,7 @@ class RefreshTokenAuthorizerTests(CapturedIOTestCase):
         self.authorizer.set_authorization_header(header_dict)
         # confirm value
         self.assertEqual(header_dict["Authorization"],
-                         "Bearer " + self.response.access_token)
+                         "Bearer " + self.rs_data['access_token'])
 
     def test_handle_missing_authorization(self):
         """
diff --git a/tests/unit/test_token_response.py b/tests/unit/test_token_response.py
index cd0c10f5..f8806c74 100644
--- a/tests/unit/test_token_response.py
+++ b/tests/unit/test_token_response.py
@@ -117,54 +117,6 @@ class OAuthTokenResponseTests(CapturedIOTestCase):
             self.assertIn(server_data["expires_at_seconds"],
                           (expected - 1, expected, expected + 1))
 
-    def test_expires_at_seconds(self):
-        """
-        Gets response's expires_at_seconds attribute, confirms expected value
-        """
-        # assumes test runs within 1 second range
-        expected = int(time.time()) + self.top_token["expires_in"]
-        self.assertIn(self.response.expires_at_seconds,
-                      (expected - 1, expected, expected + 1))
-
-    def test_expires_in(self):
-        """
-        Gets two measurements of response's expires_in attribute,
-        Sanity checks that expires_in decreases as time goes on
-        """
-        first_measure = self.response.expires_in
-        time.sleep(1.1)
-        second_measure = self.response.expires_in
-        self.assertTrue(first_measure <= self.top_token["expires_in"])
-        self.assertTrue(second_measure < first_measure)
-
-    def test_access_token(self):
-        """
-        Gets response's access_token attribute, confirms expected value
-        """
-        self.assertEqual(self.response.access_token,
-                         self.top_token["access_token"])
-
-    def test_refresh_token(self):
-        """
-        Gets response's refresh_token attribute, confirms expected value
-        """
-        self.assertEqual(self.response.refresh_token,
-                         self.top_token["refresh_token"])
-
-    def test_resource_server(self):
-        """
-        Gets response's resource_server attribute, confirms expected value
-        """
-        self.assertEqual(self.response.resource_server,
-                         self.top_token["resource_server"])
-
-    def test_other_tokens(self):
-        """
-        Gets response's other_tokens attribute, confirms expected value
-        """
-        self.assertEqual(self.response.other_tokens,
-                         self.top_token["other_tokens"])
-
     @unittest.skipIf(JOSE_FLAG, "python-jose successfully imported")
     def test_decode_id_token_no_jose(self):
         """