From 7a8139f54c069971b574d151223ac16016d044e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= Date: Thu, 4 Apr 2024 14:59:26 +0200 Subject: [PATCH] Don't rely on the `exp` fields of unsigned JWTs Don't decode access token/refresh token JWTs unsigned to read out the `exp` field, but rely on the `expires_at`/`refresh_expires_at` fields received together with the tokens instead. --- CHANGELOG.md | 1 + okdata/sdk/auth/auth.py | 40 +++++++++++++++++++++++++++------- okdata/sdk/auth/util.py | 26 ---------------------- requirements.txt | 2 -- setup.py | 1 - tests/auth/auth_test.py | 15 ++++++++++++- tests/auth/token_utils_test.py | 22 ------------------- tox.ini | 3 ++- 8 files changed, 49 insertions(+), 61 deletions(-) delete mode 100644 okdata/sdk/auth/util.py delete mode 100644 tests/auth/token_utils_test.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a520003..a2ff52f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * Removed dependency on the vulnerable (and seemingly abandoned) python-jose library. +* PyJWT is no longer a dependency. ## 3.1.0 - 2024-01-10 diff --git a/okdata/sdk/auth/auth.py b/okdata/sdk/auth/auth.py index e461563..9539133 100644 --- a/okdata/sdk/auth/auth.py +++ b/okdata/sdk/auth/auth.py @@ -1,5 +1,6 @@ import json import logging +from datetime import datetime, timedelta, timezone from okdata.sdk.auth.credentials.client_credentials import ClientCredentialsProvider from okdata.sdk.auth.credentials.common import ( @@ -7,14 +8,23 @@ 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 from okdata.sdk.file_cache import FileCache log = logging.getLogger() -class Authenticate(object): +def _is_expired(timestamp): + """Return true if `timestamp` has expired (or is just about to expire).""" + return timestamp and (timestamp - datetime.now(timezone.utc)).total_seconds() < 10 + + +class Authenticate: + _access_token = None + _refresh_token = None + _expires_at = None + _refresh_expires_at = None + def __init__(self, config, token_provider=None, file_cache=None): self.token_provider = token_provider if not self.token_provider: @@ -30,9 +40,6 @@ def __init__(self, config, token_provider=None, file_cache=None): if not self.file_cache: self.file_cache = FileCache(config) - self._access_token = None - self._refresh_token = None - def _resolve_token_provider(self, config): # Add more TokenProviders to accept different login methods strategies = [ClientCredentialsProvider, TokenServiceProvider] @@ -54,7 +61,7 @@ def access_token(self): if not self._access_token: self.login() # If expired, refresh - if is_token_expired(self._access_token): + if _is_expired(self._expires_at): self.refresh_access_token() return self._access_token @@ -66,8 +73,12 @@ def login(self, force=False): if cached: self._access_token = cached["access_token"] self._refresh_token = cached.get("refresh_token") + if expires_at := cached.get("expires_at"): + self._expires_at = datetime.fromisoformat(expires_at) + if refresh_expires_at := cached.get("refresh_expires_at"): + self._refresh_expires_at = datetime.fromisoformat(refresh_expires_at) - if self._access_token and not is_token_expired(self._access_token): + if self._access_token and not _is_expired(self._expires_at): log.info("Token not expired, skipping") return self.refresh_access_token() @@ -78,7 +89,7 @@ def refresh_access_token(self): tokens = None - if self._refresh_token and not is_token_expired(self._refresh_token): + if self._refresh_token and not _is_expired(self._refresh_expires_at): try: tokens = self.token_provider.refresh_token(self._refresh_token) except TokenRefreshError as e: @@ -89,6 +100,13 @@ def refresh_access_token(self): if "access_token" not in tokens: raise ApiAuthenticateError self._refresh_token = tokens.get("refresh_token") + self._expires_at = datetime.now(timezone.utc) + timedelta( + seconds=tokens.get("expires_in") + ) + if refresh_expires_in := tokens.get("refresh_expires_in"): + self._refresh_expires_at = datetime.now(timezone.utc) + timedelta( + seconds=refresh_expires_in + ) self._access_token = tokens["access_token"] self.file_cache.write_credentials(credentials=self) @@ -99,6 +117,12 @@ def __repr__(self): "provider": self.token_provider.__class__.__name__, "access_token": self._access_token, "refresh_token": self._refresh_token, + "expires_at": self._expires_at.isoformat() if self._expires_at else "", + "refresh_expires_at": ( + self._refresh_expires_at.isoformat() + if self._refresh_expires_at + else "" + ), } ) diff --git a/okdata/sdk/auth/util.py b/okdata/sdk/auth/util.py deleted file mode 100644 index 98a238b..0000000 --- a/okdata/sdk/auth/util.py +++ /dev/null @@ -1,26 +0,0 @@ -from datetime import datetime - -import jwt - - -def is_token_expired(token): - return is_expired(get_expired_timestamp(token)) - - -def is_expired(timestamp): - expires_dt = datetime.utcfromtimestamp(timestamp) - - difference = expires_dt - datetime.utcnow() - - if difference.total_seconds() < 10: - return True - - return False - - -def decode_token(token): - return jwt.decode(token, options={"verify_signature": False}) - - -def get_expired_timestamp(token): - return decode_token(token)["exp"] diff --git a/requirements.txt b/requirements.txt index ba5bd82..4a81767 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,8 +26,6 @@ packaging==24.0 # via deprecation pycparser==2.22 # via cffi -pyjwt==2.4.0 - # via okdata-sdk (setup.py) pyrsistent==0.18.1 # via jsonschema python-keycloak==3.11.1 diff --git a/setup.py b/setup.py index ad9aef0..ac2a54e 100644 --- a/setup.py +++ b/setup.py @@ -19,7 +19,6 @@ namespace_packages=["okdata"], install_requires=[ "jsonschema", - "PyJWT>=2.0.0", # Versions prior to 3.9.1 depends on the vulnerable (and seemingly # abandoned) python-jose library. "python-keycloak>=3.9.1,<4", diff --git a/tests/auth/auth_test.py b/tests/auth/auth_test.py index 57261a8..176db73 100644 --- a/tests/auth/auth_test.py +++ b/tests/auth/auth_test.py @@ -11,8 +11,10 @@ from freezegun import freeze_time from tests.auth.client_credentials_test_utils import ( - from_cache_not_expired_token, + expired_time, from_cache_expired_token, + from_cache_not_expired_token, + not_expired_time, utc_now, ) from tests.test_utils import ( @@ -71,6 +73,8 @@ def test_authenticate_cached_credentials(self, mock_home_dir): "provider": "ClientCredentialsProvider", "access_token": from_cache_not_expired_token, "refresh_token": from_cache_not_expired_token, + "expires_at": not_expired_time.isoformat(), + "refresh_expires_at": not_expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) @@ -88,6 +92,8 @@ def test_authenticate_refresh_credentials(self, requests_mock, mock_home_dir): "provider": "ClientCredentialsProvider", "access_token": from_cache_not_expired_token, "refresh_token": from_cache_not_expired_token, + "expires_at": not_expired_time.isoformat(), + "refresh_expires_at": not_expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) @@ -110,6 +116,8 @@ def test_authenticate_expired_tokens(self, requests_mock, mock_home_dir): "provider": "TokenServiceProvider", "access_token": from_cache_expired_token, "refresh_token": from_cache_expired_token, + "expires_at": expired_time.isoformat(), + "refresh_expires_at": expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) @@ -134,6 +142,8 @@ def test_authenticate_expired_access_token(self, requests_mock, mock_home_dir): "provider": "TokenServiceProvider", "access_token": from_cache_expired_token, "refresh_token": from_cache_not_expired_token, + "expires_at": expired_time.isoformat(), + "refresh_expires_at": not_expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) @@ -173,6 +183,8 @@ def test_refresh_inactive_session(self, requests_mock, mock_home_dir): "provider": "TokenServiceProvider", "access_token": from_cache_expired_token, "refresh_token": from_cache_not_expired_token, + "expires_at": expired_time.isoformat(), + "refresh_expires_at": not_expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) @@ -203,6 +215,7 @@ def test_refresh_no_refresh_token(self, requests_mock, mock_home_dir): cached_credentials = { "provider": "TokenServiceProvider", "access_token": from_cache_expired_token, + "expires_at": expired_time.isoformat(), } auth.file_cache.write_credentials(json.dumps(cached_credentials)) diff --git a/tests/auth/token_utils_test.py b/tests/auth/token_utils_test.py deleted file mode 100644 index ec1f026..0000000 --- a/tests/auth/token_utils_test.py +++ /dev/null @@ -1,22 +0,0 @@ -from freezegun import freeze_time - -from okdata.sdk.auth.util import is_token_expired -from tests.auth.client_credentials_test_utils import ( - not_expired_token, - expired_token, - utc_now, -) - - -class TestClientCredentials: - @freeze_time(utc_now) - def test_token_expiery(self): - cc1 = {"access_token": expired_token, "refresh_token": expired_token} - - cc2 = {"access_token": not_expired_token, "refresh_token": not_expired_token} - - assert is_token_expired(cc1["access_token"]) - assert is_token_expired(cc1["refresh_token"]) - - assert not is_token_expired(cc2["access_token"]) - assert not is_token_expired(cc2["refresh_token"]) diff --git a/tox.ini b/tox.ini index b29743a..0eeac64 100644 --- a/tox.ini +++ b/tox.ini @@ -4,8 +4,9 @@ envlist = py38,py39,py310,py311,py312,flake8,black,mypy [testenv] deps= freezegun==0.3.12 - pytest==6.2.5 + PyJWT==2.8.0 pytest-mock==1.12.1 + pytest==6.2.5 python-dateutil==2.8.0 requests-mock==1.6.0 -rrequirements.txt