Merge pull request #76 from oslokommune/DP-2022_fix_token_refresh

DP-2022 Fix token refresh on inactive session

Author: fredriv

CHANGELOG.md

@@ -6,6 +6,9 @@
   read errors, and redirects). The `retry` parameter now only controls the
   maximum number of retries to perform on bad HTTP status codes.
 
+* Fix refresh of Keycloak access token when refresh token is invalid, e.g.
+  due to inactive session because Keycloak server restarted.
+
 ## 0.7.0
 
 * `Dataset.update_dataset` now supports partial metadata updates when the

okdata/sdk/auth/auth.py

@@ -2,7 +2,10 @@
 import logging
 
 from okdata.sdk.auth.credentials.client_credentials import ClientCredentialsProvider
-from okdata.sdk.auth.credentials.common import TokenProviderNotInitialized
+from okdata.sdk.auth.credentials.common import (
+    TokenProviderNotInitialized,
+    TokenRefreshError,
+)
 from okdata.sdk.auth.credentials.password_grant import TokenServiceProvider
 from okdata.sdk.auth.util import is_token_expired
 from okdata.sdk.exceptions import ApiAuthenticateError
@@ -55,16 +58,6 @@ def access_token(self):
             self.refresh_access_token()
         return self._access_token
 
-    # read only
-    @property
-    def refresh_token(self):
-        if not self.token_provider:
-            return None
-        # If expired, relog
-        if is_token_expired(self._refresh_token):
-            self.token_provider.new_token()
-        return self._refresh_token
-
     def login(self, force=False):
         if not self.token_provider:
             return
@@ -77,21 +70,26 @@ def login(self, force=False):
         if self._access_token and not is_token_expired(self._access_token):
             log.info("Token not expired, skipping")
             return
-        tokens = self.token_provider.new_token()
-        if "access_token" not in tokens:
-            raise ApiAuthenticateError
-        self._access_token = tokens["access_token"]
-        self._refresh_token = tokens["refresh_token"]
-        self.file_cache.write_credentials(credentials=self)
+        self.refresh_access_token()
 
     def refresh_access_token(self):
         if not self.token_provider:
             return
 
-        if is_token_expired(self._refresh_token):
+        tokens = None
+
+        if self._refresh_token and not is_token_expired(self._refresh_token):
+            try:
+                tokens = self.token_provider.refresh_token(self._refresh_token)
+            except TokenRefreshError as e:
+                log.warn(f"Error refreshing token: {e}")
+
+        if not tokens:
             tokens = self.token_provider.new_token()
-        else:
-            tokens = self.token_provider.refresh_token(self.refresh_token)
+            if "access_token" not in tokens:
+                raise ApiAuthenticateError
+            self._refresh_token = tokens["refresh_token"]
+
         self._access_token = tokens["access_token"]
         self.file_cache.write_credentials(credentials=self)
 

okdata/sdk/auth/credentials/client_credentials.py

@@ -1,9 +1,11 @@
 from typing import Optional
+from keycloak.exceptions import KeycloakGetError  # type: ignore
 from keycloak.keycloak_openid import KeycloakOpenID  # type: ignore
 
 from okdata.sdk.auth.credentials.common import (
     TokenProvider,
     TokenProviderNotInitialized,
+    TokenRefreshError,
 )
 from okdata.sdk.config import Config
 
@@ -39,7 +41,10 @@ def __post_init__(self):
         )
 
     def refresh_token(self, refresh_token):
-        return self.client.refresh_token(refresh_token=refresh_token)
+        try:
+            return self.client.refresh_token(refresh_token=refresh_token)
+        except KeycloakGetError as e:
+            raise TokenRefreshError(str(e))
 
     def new_token(self):
         return self.client.token(grant_type=["client_credentials"])

okdata/sdk/auth/credentials/common.py

@@ -5,6 +5,10 @@ class TokenProviderNotInitialized(Exception):
     pass
 
 
+class TokenRefreshError(Exception):
+    pass
+
+
 class TokenProvider:
     config: Config
 

tests/auth/auth_test.py

@@ -20,8 +20,8 @@
 logging.basicConfig(level=logging.INFO)
 
 
-config = Config()
-token_endpoint = "https://login-test.oslo.kommune.no/auth/realms/api-catalog/protocol/openid-connect/token"
+config = Config(env="prod")
+token_endpoint = "https://login.oslo.kommune.no/auth/realms/api-catalog/protocol/openid-connect/token"
 
 
 @pytest.fixture(scope="function")
@@ -39,11 +39,11 @@ def test_authenticate_cache_disabled(self, requests_mock, mock_home_dir):
 
         response = json.dumps(client_credentials_response)
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         auth.login()
-        assert auth.access_token == client_credentials_response["access_token"]
-        assert auth.refresh_token == client_credentials_response["refresh_token"]
+        assert auth._access_token == client_credentials_response["access_token"]
+        assert auth._refresh_token == client_credentials_response["refresh_token"]
 
     def test_authenticat_no_cache(self, requests_mock, mock_home_dir):
 
@@ -54,11 +54,11 @@ def test_authenticat_no_cache(self, requests_mock, mock_home_dir):
 
         response = json.dumps(client_credentials_response)
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         auth.login()
-        assert auth.access_token == client_credentials_response["access_token"]
-        assert auth.refresh_token == client_credentials_response["refresh_token"]
+        assert auth._access_token == client_credentials_response["access_token"]
+        assert auth._refresh_token == client_credentials_response["refresh_token"]
 
     def test_authenticate_cached_credentials(self, mock_home_dir):
         client_credentials_provider = ClientCredentialsProvider(config)
@@ -73,8 +73,8 @@ def test_authenticate_cached_credentials(self, mock_home_dir):
 
         auth.file_cache.write_credentials(json.dumps(cached_credentials))
         auth.login()
-        assert auth.access_token == cached_credentials["access_token"]
-        assert auth.refresh_token == cached_credentials["refresh_token"]
+        assert auth._access_token == cached_credentials["access_token"]
+        assert auth._refresh_token == cached_credentials["refresh_token"]
 
     def test_authenticate_refresh_credentials(self, requests_mock, mock_home_dir):
 
@@ -93,11 +93,11 @@ def test_authenticate_refresh_credentials(self, requests_mock, mock_home_dir):
 
         response = json.dumps(client_credentials_response)
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         auth.login()
-        assert auth.access_token == cached_credentials["access_token"]
-        assert auth.refresh_token == cached_credentials["refresh_token"]
+        assert auth._access_token == cached_credentials["access_token"]
+        assert auth._refresh_token == cached_credentials["refresh_token"]
 
     def test_authenticate_expired_tokens(self, requests_mock, mock_home_dir):
         client_credentials_provider = ClientCredentialsProvider(config)
@@ -115,13 +115,13 @@ def test_authenticate_expired_tokens(self, requests_mock, mock_home_dir):
 
         response = json.dumps(client_credentials_response)
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         auth.login()
         print(from_cache_not_expired_token)
         print(from_cache_expired_token)
-        assert auth.access_token == client_credentials_response["access_token"]
-        assert auth.refresh_token == client_credentials_response["access_token"]
+        assert auth._access_token == client_credentials_response["access_token"]
+        assert auth._refresh_token == client_credentials_response["access_token"]
 
     def test_authenticate_expired_access_token(self, requests_mock, mock_home_dir):
         client_credentials_provider = ClientCredentialsProvider(config)
@@ -139,11 +139,11 @@ def test_authenticate_expired_access_token(self, requests_mock, mock_home_dir):
 
         response = json.dumps(client_credentials_response)
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         auth.login()
-        assert auth.access_token == from_cache_not_expired_token
-        assert auth.refresh_token == cached_credentials["refresh_token"]
+        assert auth._access_token == from_cache_not_expired_token
+        assert auth._refresh_token == cached_credentials["refresh_token"]
 
     def test_authenticate_fail(self, requests_mock, mock_home_dir):
         client_credentials_provider = ClientCredentialsProvider(
@@ -152,12 +152,43 @@ def test_authenticate_fail(self, requests_mock, mock_home_dir):
         auth = Authenticate(config=config, token_provider=client_credentials_provider)
 
         response = json.dumps(
-            {"error": "authenitcation error", "error_description": "No such client"}
+            {"error": "authentication error", "error_description": "No such client"}
         )
         matcher = re.compile(token_endpoint)
-        requests_mock.register_uri("POST", matcher, text=response, status_code=204)
+        requests_mock.register_uri("POST", matcher, text=response, status_code=200)
 
         try:
             auth.login()
         except ApiAuthenticateError:
             assert True
+
+    def test_refresh_inactive_session(self, requests_mock, mock_home_dir):
+        client_credentials_provider = ClientCredentialsProvider(config)
+        auth = Authenticate(config=config, token_provider=client_credentials_provider)
+
+        auth.file_cache.credentials_cache_enabled = True
+
+        cached_credentials = {
+            "provider": "TokenServiceProvider",
+            "access_token": from_cache_expired_token,
+            "refresh_token": from_cache_not_expired_token,
+        }
+
+        auth.file_cache.write_credentials(json.dumps(cached_credentials))
+
+        error_msg = {
+            "error": "invalid_grant",
+            "error_description": "Session not active",
+        }
+        refresh_response = {"text": json.dumps(error_msg), "status_code": 400}
+        login_response = {
+            "text": json.dumps(client_credentials_response),
+            "status_code": 200,
+        }
+        matcher = re.compile(token_endpoint)
+        requests_mock.register_uri("POST", matcher, [refresh_response, login_response])
+
+        auth.login()
+
+        assert auth._access_token == from_cache_not_expired_token
+        assert auth._refresh_token == cached_credentials["refresh_token"]