Fix client bugs: refresh retry, error parsing, clone user-agent, scope validation (#535)

- Retry the token-refresh POST on 5xx (InternalServerError) using the same
  exponential backoff as request_json_string_with_retry, honoring
  max_retries_on_error. Previously a 5xx during refresh bypassed retries
  entirely (#518).
- raise_dropbox_error_for_resp: use res.json().get('error') and catch
  AttributeError so a 400 body lacking the "error" key raises BadInputError
  instead of KeyError (#527).
- clone(): pass self._raw_user_agent instead of self._user_agent so cloned
  clients don't get the OfficialDropboxPythonSDKv2 suffix appended twice (#526).
- Reorder scope validation to check isinstance before len(), so a non-list
  scope raises BadInputException instead of TypeError, in the client
  constructor, refresh_access_token, and DropboxOAuth2FlowBase (#526).

Adds unit tests for each fix (reproduced as failing tests first).

Author: AndreyVMarkelov

dropbox/dropbox_client.py

@@ -195,7 +195,7 @@ def __init__(self,
         if oauth2_refresh_token and not app_key:
             raise BadInputException("app_key is required to refresh tokens")
 
-        if scope is not None and (len(scope) == 0 or not isinstance(scope, list)):
+        if scope is not None and (not isinstance(scope, list) or len(scope) == 0):
             raise BadInputException("Scope list must be of type list")
 
         self._oauth2_access_token = oauth2_access_token
@@ -261,7 +261,7 @@ def clone(
             oauth2_access_token or self._oauth2_access_token,
             max_retries_on_error or self._max_retries_on_error,
             max_retries_on_rate_limit or self._max_retries_on_rate_limit,
-            user_agent or self._user_agent,
+            user_agent or self._raw_user_agent,
             session or self._session,
             headers or self._headers,
             timeout or self._timeout,
@@ -378,7 +378,7 @@ def refresh_access_token(self, host=API_HOST, scope=None):
         :param scope: list of permission scopes for access token
         :return:
         """
-        if scope is not None and (len(scope) == 0 or not isinstance(scope, list)):
+        if scope is not None and (not isinstance(scope, list) or len(scope) == 0):
             raise BadInputException("Scope list must be of type list")
 
         if not (self._oauth2_refresh_token and self._app_key):
@@ -401,8 +401,25 @@ def refresh_access_token(self, host=API_HOST, scope=None):
         timeout = DEFAULT_TIMEOUT
         if self._timeout:
             timeout = self._timeout
-        res = self._session.post(url, data=body, timeout=timeout)
-        self.raise_dropbox_error_for_resp(res)
+
+        attempt = 0
+        while True:
+            res = self._session.post(url, data=body, timeout=timeout)
+            try:
+                self.raise_dropbox_error_for_resp(res)
+                break
+            except InternalServerError as e:
+                attempt += 1
+                if attempt <= self._max_retries_on_error:
+                    # Use exponential backoff, matching request_json_string_with_retry.
+                    backoff = 2**attempt * random.random()
+                    self._logger.info(
+                        'HttpError status_code=%s while refreshing access token: '
+                        'Retrying in %.1f seconds',
+                        e.status_code, backoff)
+                    time.sleep(backoff)
+                else:
+                    raise
 
         token_content = res.json()
         self._oauth2_access_token = token_content["access_token"]
@@ -619,14 +636,14 @@ def raise_dropbox_error_for_resp(self, res):
             raise InternalServerError(request_id, res.status_code, res.text)
         elif res.status_code == 400:
             try:
-                if res.json()['error'] == 'invalid_grant':
+                if res.json().get('error') == 'invalid_grant':
                     request_id = res.headers.get('x-dropbox-request-id')
                     err = stone_serializers.json_compat_obj_decode(
                         AuthError_validator, 'invalid_access_token')
                     raise AuthError(request_id, err)
                 else:
                     raise BadInputError(request_id, res.text)
-            except ValueError:
+            except (ValueError, AttributeError):
                 raise BadInputError(request_id, res.text)
         elif res.status_code == 401:
             assert res.headers.get('content-type') == 'application/json', (

dropbox/oauth.py

@@ -116,7 +116,7 @@ class DropboxOAuth2FlowBase(object):
     def __init__(self, consumer_key, consumer_secret=None, locale=None, token_access_type=None,
                  scope=None, include_granted_scopes=None, use_pkce=False, timeout=DEFAULT_TIMEOUT,
                  ca_certs=None):
-        if scope is not None and (len(scope) == 0 or not isinstance(scope, list)):
+        if scope is not None and (not isinstance(scope, list) or len(scope) == 0):
             raise BadInputException("Scope list must be of type list")
         if token_access_type is not None and token_access_type not in TOKEN_ACCESS_TYPES:
             raise BadInputException("Token access type must be from the following enum: {}".format(

test/unit/test_dropbox_unit.py

@@ -8,7 +8,7 @@
 # Tests OAuth Flow
 from dropbox import DropboxOAuth2Flow, session, Dropbox, create_session
 from dropbox.dropbox_client import BadInputException, DropboxTeam
-from dropbox.exceptions import AuthError
+from dropbox.exceptions import AuthError, BadInputError
 from dropbox.oauth import OAuth2FlowNoRedirectResult, DropboxOAuth2FlowNoRedirect
 from datetime import datetime, timedelta
 
@@ -255,6 +255,37 @@ def invalid_grant_session_instance(self, mocker):
         mocker.patch.object(session_obj, 'post', return_value=post_response)
         return session_obj
 
+    @pytest.fixture(scope='function')
+    def bad_request_no_error_key_session_instance(self, mocker):
+        # A 400 response whose JSON body does not contain an "error" key.
+        session_obj = create_session()
+        post_response = mock.MagicMock(status_code=400)
+        post_response.json.return_value = {"error_description": "some other problem"}
+        post_response.text = '{"error_description": "some other problem"}'
+        mocker.patch.object(session_obj, 'post', return_value=post_response)
+        return session_obj
+
+    @pytest.fixture(scope='function')
+    def server_error_then_ok_session_instance(self, mocker):
+        # First refresh POST returns 500, second returns 200 with a valid token.
+        session_obj = create_session()
+        error_response = mock.MagicMock(status_code=500)
+        error_response.text = 'internal server error'
+        ok_response = mock.MagicMock(status_code=200)
+        ok_response.json.return_value = {"access_token": ACCESS_TOKEN, "expires_in": EXPIRES_IN}
+        mocker.patch.object(session_obj, 'post',
+                            side_effect=[error_response, ok_response])
+        return session_obj
+
+    @pytest.fixture(scope='function')
+    def server_error_session_instance(self, mocker):
+        # Every refresh POST returns 500.
+        session_obj = create_session()
+        error_response = mock.MagicMock(status_code=500)
+        error_response.text = 'internal server error'
+        mocker.patch.object(session_obj, 'post', return_value=error_response)
+        return session_obj
+
     def test_default_Dropbox_raises_assertion_error(self):
         with pytest.raises(BadInputException):
             # Requires either access token or refresh token
@@ -406,6 +437,55 @@ def test_team_client_as_user(self, session_instance):
                       session=session_instance)
         dbx.as_user(TEAM_MEMBER_ID)
 
+    def test_non_list_scope_raises_bad_input(self, session_instance):
+        # A non-list scope with no len() must raise BadInputException, not
+        # TypeError from calling len() before the isinstance check.
+        with pytest.raises(BadInputException):
+            Dropbox(oauth2_access_token=ACCESS_TOKEN,
+                    scope=12345,
+                    session=session_instance)
+
+    def test_clone_does_not_double_user_agent(self, session_instance):
+        dbx = Dropbox(oauth2_access_token=ACCESS_TOKEN, session=session_instance,
+                      user_agent='myapp')
+        cloned = dbx.clone()
+        # The base suffix must appear exactly once after cloning.
+        assert cloned._user_agent.count('OfficialDropboxPythonSDKv2/') == 1
+        assert cloned._user_agent == dbx._user_agent
+
+    def test_refresh_400_without_error_key(self, bad_request_no_error_key_session_instance):
+        # A 400 body lacking the "error" key must raise BadInputError, not KeyError.
+        dbx = Dropbox(oauth2_refresh_token=REFRESH_TOKEN,
+                      app_key=APP_KEY,
+                      app_secret=APP_SECRET,
+                      session=bad_request_no_error_key_session_instance)
+        with pytest.raises(BadInputError):
+            dbx.check_and_refresh_access_token()
+
+    def test_refresh_retries_on_server_error(self, server_error_then_ok_session_instance):
+        # A 5xx on the refresh POST must be retried, then succeed.
+        dbx = Dropbox(oauth2_refresh_token=REFRESH_TOKEN,
+                      app_key=APP_KEY,
+                      app_secret=APP_SECRET,
+                      max_retries_on_error=2,
+                      session=server_error_then_ok_session_instance)
+        dbx.check_and_refresh_access_token()
+        assert server_error_then_ok_session_instance.post.call_count == 2
+        assert dbx._oauth2_access_token == ACCESS_TOKEN
+
+    def test_refresh_raises_after_exhausting_retries(self, server_error_session_instance):
+        # Once retries are exhausted, the 5xx propagates as InternalServerError.
+        from dropbox.exceptions import InternalServerError
+        dbx = Dropbox(oauth2_refresh_token=REFRESH_TOKEN,
+                      app_key=APP_KEY,
+                      app_secret=APP_SECRET,
+                      max_retries_on_error=2,
+                      session=server_error_session_instance)
+        with pytest.raises(InternalServerError):
+            dbx.check_and_refresh_access_token()
+        # Initial attempt + 2 retries.
+        assert server_error_session_instance.post.call_count == 3
+
 
 class TestSession:
     def test_pickle_session(self):