Repository: globus/globus-sdk-python
Issue: RenewingAuthorizer implementation is in violation of OAuth2 spec

This is not very high priority due to the implementation of Globus Auth. In our case, this violation makes no difference. However, future changes which expect this to be correct would be prone to issues.

While reading the OAuth2 spec (again) for other work, I noticed this in [section 6](https://tools.ietf.org/html/rfc6749#section-6):
> The authorization server MAY issue a new refresh token, in which case
> the client MUST discard the old refresh token and replace it with the
> new refresh token.

Our refresh code does not treat the refresh token at all:

https://github.com/globus/globus-sdk-python/blob/85f09d5ec979bd606f908e1001bbaa55e61980aa/globus_sdk/authorizers/renewing.py#L89-L107

No change to `on_refresh` is necessary, as we pass complete token data.

I'll open a CLI issue to fix its `on_refresh` handler.

Changed files:
globus_sdk/authorizers/refresh_token.py, tests/unit/authorizers/test_refresh_token_authorizer.py

diff --git a/globus_sdk/authorizers/refresh_token.py b/globus_sdk/authorizers/refresh_token.py
index 87015b3..cbff251 100644
--- a/globus_sdk/authorizers/refresh_token.py
+++ b/globus_sdk/authorizers/refresh_token.py
@@ -87,6 +87,9 @@ class RefreshTokenAuthorizer(RenewingAuthorizer):
         """
         Get the tokens .by_resource_server,
         Ensure that only one token was gotten, and return that token.
+
+        If the token_data includes a "refresh_token" field, update self.refresh_token to
+        that value.
         """
         token_data = res.by_resource_server.values()
         if len(token_data) != 1:
@@ -95,4 +98,11 @@ class RefreshTokenAuthorizer(RenewingAuthorizer):
                 "didn't return exactly one token. Possible service error."
             )
 
-        return next(iter(token_data))
+        token_data = next(iter(token_data))
+
+        # handle refresh_token being present
+        # mandated by OAuth2: https://tools.ietf.org/html/rfc6749#section-6
+        if "refresh_token" in token_data:
+            self.refresh_token = token_data["refresh_token"]
+
+        return token_data
diff --git a/tests/unit/authorizers/test_refresh_token_authorizer.py b/tests/unit/authorizers/test_refresh_token_authorizer.py
index ab7cef1..2363151 100644
--- a/tests/unit/authorizers/test_refresh_token_authorizer.py
+++ b/tests/unit/authorizers/test_refresh_token_authorizer.py
@@ -13,12 +13,19 @@ ACCESS_TOKEN = "access_token_1"
 EXPIRES_AT = -1
 
 
-@pytest.fixture
-def response():
+@pytest.fixture(params=["simple", "with_new_refresh_token"])
+def response(request):
     r = mock.Mock()
     r.by_resource_server = {
-        "rs1": {"expires_at_seconds": -1, "access_token": "access_token_2"}
-    }
+        "simple": {"rs1": {"expires_at_seconds": -1, "access_token": "access_token_2"}},
+        "with_new_refresh_token": {
+            "rs1": {
+                "expires_at_seconds": -1,
+                "access_token": "access_token_2",
+                "refresh_token": "refresh_token_2",
+            }
+        },
+    }[request.param]
     return r
 
 
@@ -50,9 +57,8 @@ def test_get_token_response(authorizer, client, response):
 
 def test_multiple_resource_servers(authorizer, response):
     """
-    Sets the mock ConfidentialAppAuthClient to return multiple resource
-    servers. Confirms GlobusError is raised when _extract_token_data is
-    called.
+    Sets the mock client to return multiple resource servers.
+    Confirms GlobusError is raised when _extract_token_data is called.
     """
     response.by_resource_server["rs2"] = {
         "expires_at_seconds": -1,
@@ -61,3 +67,19 @@ def test_multiple_resource_servers(authorizer, response):
     with pytest.raises(ValueError) as excinfo:
         authorizer._extract_token_data(response)
     assert "didn't return exactly one token" in str(excinfo.value)
+
+
+def test_conditional_refresh_token_update(authorizer, response):
+    """
+    Call check_expiration_time (triggering a refresh)
+    Confirm that the authorizer always udpates its access token and only updates
+    refresh_token if one was present in the response
+    """
+    authorizer.check_expiration_time()  # trigger refresh
+    token_data = response.by_resource_server["rs1"]
+    if "refresh_token" in token_data:  # if present, confirm refresh token was updated
+        assert authorizer.access_token == "access_token_2"
+        assert authorizer.refresh_token == "refresh_token_2"
+    else:  # otherwise, confirm no change
+        assert authorizer.access_token == "access_token_2"
+        assert authorizer.refresh_token == "refresh_token_1"