Merge pull request #17 from DataCrunch-io/feature/CLOUD-26-inspect-potential-python-sdk-bug

Feature/cloud 26 inspect potential python sdk bug

Author: tamirse

CHANGELOG.rst

@@ -1,6 +1,14 @@
 Changelog
 =========
 
+v1.4.1 (2023-06-20)
+-------------------
+
+* Fixed a bug where token refresh failed
+
+v1.4.0 (2023-06-14)
+-------------------
+
 * Added support for permanent deletion of volumes
 * Added a Volume class method that inits a new Volume instance from a dict
 * Added a few integration tests for permanent deletion of volumes

datacrunch/__version__.py

@@ -1 +1 @@
-VERSION = '1.4.0'
+VERSION = '1.4.1'

datacrunch/authentication/authentication.py

@@ -78,7 +78,12 @@ def refresh(self) -> dict:
 
         response = requests.post(
             url, json=payload, headers=self._generate_headers())
-        handle_error(response)
+
+        # if refresh token is also expired, authenticate again:
+        if response.status_code == 401 or response.status_code == 400:
+            return self.authenticate()
+        else:
+            handle_error(response)
 
         auth_data = response.json()
 
@@ -104,4 +109,4 @@ def is_expired(self) -> bool:
         :return: True if the access token is expired, otherwise False.
         :rtype: bool
         """
-        return time.time() > self._expires_at
+        return time.time() >= self._expires_at

datacrunch/http_client/http_client.py

@@ -49,11 +49,11 @@ def post(self, url: str, json: dict = None, **kwargs) -> requests.Response:
         :return: Response object
         :rtype: requests.Response
         """
+        self._refresh_token_if_expired()
+
         url = self._add_base_url(url)
         headers = self._generate_headers()
 
-        self._refresh_token_if_expired()
-
         response = requests.post(url, json=json, headers=headers, **kwargs)
         handle_error(response)
 
@@ -76,11 +76,11 @@ def put(self, url: str, json: dict = None, **kwargs) -> requests.Response:
         :return: Response object
         :rtype: requests.Response
         """
+        self._refresh_token_if_expired()
+
         url = self._add_base_url(url)
         headers = self._generate_headers()
 
-        self._refresh_token_if_expired()
-
         response = requests.put(url, json=json, headers=headers, **kwargs)
         handle_error(response)
 
@@ -103,11 +103,11 @@ def get(self, url: str, params: dict = None, **kwargs) -> requests.Response:
         :return: Response object
         :rtype: requests.Response
         """
+        self._refresh_token_if_expired()
+
         url = self._add_base_url(url)
         headers = self._generate_headers()
 
-        self._refresh_token_if_expired()
-
         response = requests.get(url, params=params, headers=headers, **kwargs)
         handle_error(response)
 
@@ -130,11 +130,11 @@ def delete(self, url: str, json: dict = None, **kwargs) -> requests.Response:
         :return: Response object
         :rtype: requests.Response
         """
+        self._refresh_token_if_expired()
+
         url = self._add_base_url(url)
         headers = self._generate_headers()
 
-        self._refresh_token_if_expired()
-
         response = requests.delete(url, headers=headers, json=json, **kwargs)
         handle_error(response)
 
@@ -147,7 +147,7 @@ def _refresh_token_if_expired(self) -> None:
 
         :raises APIException: an api exception with message and error type code
         """
-        if(self._auth_service.is_expired()):
+        if (self._auth_service.is_expired()):
             # try to refresh. if refresh token has expired, reauthenticate
             try:
                 self._auth_service.refresh()
@@ -182,7 +182,7 @@ def _generate_user_agent(self) -> str:
         :rtype: str
         """
         # get the first 10 chars of the client id
-        client_id_truncated = self._auth_service._client_id[:10]  
+        client_id_truncated = self._auth_service._client_id[:10]
 
         return f'datacrunch-python-v{self._version}-{client_id_truncated}'
 

tests/unit_tests/authentication/test_authentication.py

@@ -1,5 +1,5 @@
 import pytest
-import responses # https://github.com/getsentry/responses
+import responses  # https://github.com/getsentry/responses
 from responses import matchers
 import time
 
@@ -22,6 +22,7 @@
 ACCESS_TOKEN2 = 'access2'
 REFRESH_TOKEN2 = 'refresh2'
 
+
 class TestAuthenticationService:
 
     @pytest.fixture
@@ -78,7 +79,8 @@ def test_authenticate_failed(self, authentication_service, endpoint):
         assert excinfo.value.code == INVALID_REQUEST
         assert excinfo.value.message == INVALID_REQUEST_MESSAGE
         assert responses.assert_call_count(endpoint, 1) is True
-        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode()
+        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode(
+        )
 
     def test_refresh_successful(self, authentication_service, endpoint):
         # add a response for the client credentials grant
@@ -92,7 +94,8 @@ def test_refresh_successful(self, authentication_service, endpoint):
                 'token_type': TOKEN_TYPE,
                 'expires_in': EXPIRES_IN
             },
-            match=[matchers.json_params_matcher({"grant_type":"client_credentials", "client_id": CLIENT_ID, "client_secret": CLIENT_SECRET})],
+            match=[matchers.json_params_matcher(
+                {"grant_type": "client_credentials", "client_id": CLIENT_ID, "client_secret": CLIENT_SECRET})],
             status=200
         )
 
@@ -107,12 +110,13 @@ def test_refresh_successful(self, authentication_service, endpoint):
                 'token_type': TOKEN_TYPE,
                 'expires_in': EXPIRES_IN
             },
-            match=[matchers.json_params_matcher({"grant_type":"refresh_token", "refresh_token": REFRESH_TOKEN})],
+            match=[matchers.json_params_matcher(
+                {"grant_type": "refresh_token", "refresh_token": REFRESH_TOKEN})],
             status=200
         )
 
         # act
-        auth_data = authentication_service.authenticate() # authenticate first
+        auth_data = authentication_service.authenticate()  # authenticate first
 
         # assert
         assert type(auth_data) == dict
@@ -121,17 +125,19 @@ def test_refresh_successful(self, authentication_service, endpoint):
         assert authentication_service._scope == SCOPE
         assert authentication_service._token_type == TOKEN_TYPE
         assert authentication_service._expires_at != None
-        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode()
+        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode(
+        )
 
-        auth_data2 = authentication_service.refresh() # refresh
+        auth_data2 = authentication_service.refresh()  # refresh
 
         assert type(auth_data2) == dict
         assert authentication_service._access_token == ACCESS_TOKEN2
         assert authentication_service._refresh_token == REFRESH_TOKEN2
         assert authentication_service._scope == SCOPE
         assert authentication_service._token_type == TOKEN_TYPE
         assert authentication_service._expires_at != None
-        assert responses.calls[1].request.body == f'{{"grant_type": "refresh_token", "refresh_token": "{REFRESH_TOKEN}"}}'.encode()
+        assert responses.calls[1].request.body == f'{{"grant_type": "refresh_token", "refresh_token": "{REFRESH_TOKEN}"}}'.encode(
+        )
         assert responses.assert_call_count(endpoint, 2) is True
 
     def test_refresh_failed(self, authentication_service, endpoint):
@@ -147,7 +153,8 @@ def test_refresh_failed(self, authentication_service, endpoint):
                 'token_type': TOKEN_TYPE,
                 'expires_in': EXPIRES_IN
             },
-            match=[matchers.json_params_matcher({"grant_type":"client_credentials", "client_id": CLIENT_ID, "client_secret": CLIENT_SECRET})],
+            match=[matchers.json_params_matcher(
+                {"grant_type": "client_credentials", "client_id": CLIENT_ID, "client_secret": CLIENT_SECRET})],
             status=200
         )
 
@@ -156,12 +163,13 @@ def test_refresh_failed(self, authentication_service, endpoint):
             responses.POST,
             endpoint,
             json={"code": INVALID_REQUEST, "message": INVALID_REQUEST_MESSAGE},
-            match=[matchers.json_params_matcher({"grant_type":"refresh_token", "refresh_token": REFRESH_TOKEN})],
-            status=400
+            match=[matchers.json_params_matcher(
+                {"grant_type": "refresh_token", "refresh_token": REFRESH_TOKEN})],
+            status=500
         )
 
         # act
-        authentication_service.authenticate() # authenticate first
+        authentication_service.authenticate()  # authenticate first
 
         with pytest.raises(APIException) as excinfo:
             authentication_service.refresh()
@@ -170,21 +178,25 @@ def test_refresh_failed(self, authentication_service, endpoint):
         assert excinfo.value.code == INVALID_REQUEST
         assert excinfo.value.message == INVALID_REQUEST_MESSAGE
         assert responses.assert_call_count(endpoint, 2) is True
-        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode()
-        assert responses.calls[1].request.body == f'{{"grant_type": "refresh_token", "refresh_token": "{REFRESH_TOKEN}"}}'.encode()
+        assert responses.calls[0].request.body == f'{{"grant_type": "client_credentials", "client_id": "{CLIENT_ID}", "client_secret": "{CLIENT_SECRET}"}}'.encode(
+        )
+        assert responses.calls[1].request.body == f'{{"grant_type": "refresh_token", "refresh_token": "{REFRESH_TOKEN}"}}'.encode(
+        )
 
     def test_is_expired(self, authentication_service, endpoint):
         # arrange
         current_time = time.time()
         future_time = current_time + 3600
 
         # act
-        authentication_service._expires_at = current_time # set the expired_at as current time
+        # set the expired_at as current time
+        authentication_service._expires_at = current_time
         is_expired_current = authentication_service.is_expired()
 
-        authentication_service._expires_at = future_time # set the expired_at as future time
+        # set the expired_at as future time
+        authentication_service._expires_at = future_time
         is_expired_future = authentication_service.is_expired()
 
         # assert
         assert is_expired_current == True
-        assert is_expired_future == False
+        assert is_expired_future == False