diff --git a/CHANGELOG.md b/CHANGELOG.md index 83141f5c20fb83881a9a154c36cfddcb05d0b156..9e5f9cbc1b6f2cf20a31e6a816ff6c5ccc3ceb0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,18 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.0.2] - 2022-03-05 +### Fixed + - Fix client credentials access token not being refreshed. + + ## [1.0.1] - 2022-01-09 ### Added - Incorporate new Card API card-rfid-data-config endpoint. ## [1.0.0] - 2022-01-20 ### Added - - Refactor to use openapi-generator to generate the card_client and photo_client. + - Refactor to use openapi-generator to generate the card_client and photo_client. ### Removed - Removed previous card_client and photo_client modules (breaking change). diff --git a/identitylib/api_client_mixin.py b/identitylib/api_client_mixin.py index 528900404bd8c2f0c62b89c2f4e07e2df5a17bf4..4ee5c2ea63576e11a0463aeaa72a61e75ba84ea2 100644 --- a/identitylib/api_client_mixin.py +++ b/identitylib/api_client_mixin.py @@ -1,35 +1,73 @@ import requests +from typing import Optional from logging import getLogger +from datetime import datetime, timedelta LOG = getLogger(__name__) -class ClientCredentialsConfigurationMixin(object): +class ClientCredentialsConfigurationMixin: + """ + Mixin to support client_credentials authentication flow. - """ mixin to support client_credentials authentication flow """ - - def __init__(self, client_key, client_secret, access_token_url): + """ + def __init__(self, client_key, client_secret, access_token_url=None): self.client_key = client_key self.client_secret = client_secret self.access_token_url = access_token_url - self.access_token = None - self.refresh_api_key_hook = self._refresh_access_token() + # a holder for our cached access token + self._access_token: Optional[str] = None + # a holder for the timestamp at which our access token expires + self._access_token_expires_at: Optional[datetime] = None + + def auth_settings(self): + """ + Overide the auth settings method to ensure that the access token is updated + before being used in an API request. + + + """ + self.access_token = self._get_api_gateway_access_token() + return super().auth_settings() - def _refresh_access_token(self): - """ refresh the access token """ + def _get_api_gateway_access_token(self): + """ + Returns either a recently refreshed access token or the cached access token + if still valid. - LOG.debug("Refreshing API access token") + """ + now = datetime.now() + if ( + self._access_token is not None and + self._access_token_expires_at is not None and + # ensure that our access token is still valid - with a leeway of 20 seconds + now < (self._access_token_expires_at - timedelta(seconds=20)) + ): + return self._access_token + + LOG.info("Refreshing API Gateway access token") auth = requests.auth.HTTPBasicAuth(self.client_key, self.client_secret) data = {"grant_type": "client_credentials"} response = requests.post(self.access_token_url, data=data, auth=auth) - - if response.status_code != 200 or not response.json().get('access_token'): + if ( + response.status_code >= 300 or + response.status_code < 200 or + not response.json().get('access_token') + ): raise RuntimeError( f'Access token request failed: {response.status_code}, {response.text}' ) - self.access_token = response.json()['access_token'] + response_data = response.json() + self._access_token = response_data['access_token'] + # the `expires_in` value is in seconds rather than milliseconds, if we don't have + # an `expires_in` use 1 minute as a reasonable default. + self._access_token_expires_at = now + timedelta( + seconds=response_data.get('expires_in', 60) + ) + + return self._access_token diff --git a/identitylib/card_client_configuration.py b/identitylib/card_client_configuration.py index b1648304566dd171755e9a27c7971ea45fbdaf3b..623caad19e066d73c53e09e032ff4a7b69df70ac 100644 --- a/identitylib/card_client_configuration.py +++ b/identitylib/card_client_configuration.py @@ -2,11 +2,17 @@ from identitylib.card_client.configuration import Configuration from identitylib.api_client_mixin import ClientCredentialsConfigurationMixin -class CardClientConfiguration(Configuration, ClientCredentialsConfigurationMixin): +class CardClientConfiguration(ClientCredentialsConfigurationMixin, Configuration): - def __init__(self, client_key, client_secret, access_token_url, *args, **kwargs): + def __init__( + self, + client_key, + client_secret, + access_token_url="https://api.apps.cam.ac.uk/oauth2/v1/token", + base_url="https://api.apps.cam.ac.uk/card" + ): - Configuration.__init__(self, *args, **kwargs) + Configuration.__init__(self, host=base_url) ClientCredentialsConfigurationMixin.__init__( self, client_key, client_secret, access_token_url ) diff --git a/identitylib/photo_client_configuration.py b/identitylib/photo_client_configuration.py index cbe63cc11c3109e83130e51037c6a82b8920e0e5..2801aea3ef84974fd7096eccb1c4b28e5b30ad6a 100644 --- a/identitylib/photo_client_configuration.py +++ b/identitylib/photo_client_configuration.py @@ -2,11 +2,17 @@ from identitylib.photo_client.configuration import Configuration from identitylib.api_client_mixin import ClientCredentialsConfigurationMixin -class PhotoClientConfiguration(Configuration, ClientCredentialsConfigurationMixin): +class PhotoClientConfiguration(ClientCredentialsConfigurationMixin, Configuration): - def __init__(self, client_key, client_secret, access_token_url, *args, **kwargs): + def __init__( + self, + client_key, + client_secret, + access_token_url="https://api.apps.cam.ac.uk/oauth2/v1/token", + base_url="https://api.apps.cam.ac.uk/photo" + ): - Configuration.__init__(self, *args, **kwargs) ClientCredentialsConfigurationMixin.__init__( self, client_key, client_secret, access_token_url ) + Configuration.__init__(self, host=base_url) diff --git a/requirements-tests.txt b/requirements-tests.txt index 3111445df6ce2371099f0d29458bd6bcefaae6d6..044fb568afc01117283384a35572354e505545c7 100644 --- a/requirements-tests.txt +++ b/requirements-tests.txt @@ -2,3 +2,5 @@ coverage pytest pytest-cov requests_mock +urllib3-mock +freezegun \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 183d55152eeac90d0039653de6c0e13718a404f8..5716c92a97a0289f14d3feb5df1bd7f375fb3d2b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,5 @@ dataclasses frozendict -python-dateutil \ No newline at end of file +python-dateutil +urllib3 +requests \ No newline at end of file diff --git a/setup.py b/setup.py index 81eee42077710fc69297f90702ee33d86f8f2196..250d3f467f31264ad81ecf10de351f58a54db6c9 100644 --- a/setup.py +++ b/setup.py @@ -8,7 +8,7 @@ PACKAGE_DESCRIPTION = ( "A module containing helpers and shared code related to identity systems within UIS, " "University of Cambridge." ) -PACKAGE_VERSION = "1.0.1" +PACKAGE_VERSION = "1.0.2" PACKAGE_URL = "https://gitlab.developers.cam.ac.uk/uis/devops/iam/identity-lib" diff --git a/tests/test_api_client_mixin.py b/tests/test_api_client_mixin.py index 2652b5a1cc493010cdf09e3fca2a58bebaa7d06d..807049ecded0d3de909a2f9f9c8c6439584f3df3 100644 --- a/tests/test_api_client_mixin.py +++ b/tests/test_api_client_mixin.py @@ -1,60 +1,140 @@ -from unittest import TestCase, mock -from identitylib.api_client_mixin import ClientCredentialsConfigurationMixin +from unittest import TestCase +from urllib3_mock import Responses +from base64 import b64encode +from datetime import datetime, timedelta +from freezegun import freeze_time +from identitylib.card_client_configuration import CardClientConfiguration +from identitylib.card_client import ApiClient +from identitylib.card_client.api.v1beta1_api import V1beta1Api as CardApi -class TestApiClientConfigurationMixin(TestCase): - class MockResponse: - """ mock the api post response """ - def __init__(self, response_status_code, response_text, response_data): - self.status_code = response_status_code - self.text = response_text - self.response_data = response_data +responses = Responses('requests.packages.urllib3') - def __iter__(self): - return self - def __next__(self): - return self +class TestApiClientConfigurationMixin(TestCase): - def json(self): - return self.response_data + @responses.activate + def test_access_token_is_fetched(self): + """ check the mixin requests an access token """ + def token_callback(request): + self.assertEqual( + request.headers.get('Authorization'), + f'Basic {b64encode(b"client_key_test:client_secret_test").decode("utf-8")}' + ) + return 201, {}, '{"access_token": "test_access_token"}' - @mock.patch('identitylib.api_client_mixin.requests.post', side_effect=MockResponse( - 200, "api responds 200", {'access_token': 'new_access_token'} - )) - def test_refresh_access_token(self, mock_post): - """ check the mixin refreshes access token """ + # mock the response from the token endpoint + responses.add_callback('POST', '/oauth2/v1/token', token_callback) - configuration = ClientCredentialsConfigurationMixin( - 'client_key', 'client_secret', 'access_token_url' + # mock a successful request for the available barcodes + def available_barcodes_callback(request): + self.assertEqual(request.headers.get('Authorization'), 'Bearer test_access_token') + return 200, {}, '{"results": []}' + responses.add_callback( + 'GET', + '/card/v1beta1/available-barcodes', + available_barcodes_callback ) - configuration._refresh_access_token() - self.assertEqual('new_access_token', configuration.access_token) + configuration = CardClientConfiguration('client_key_test', 'client_secret_test') + card_api_instance = CardApi(ApiClient(configuration)) + + card_api_instance.v1beta1_available_barcodes_list() - @mock.patch('identitylib.api_client_mixin.requests.post', side_effect=MockResponse( - 401, "api responds 401", - {"fault": {"faultstring": "Invalid access token", - "detail": {"errorcode": "oauth.v2.InvalidAccessToken"}}} - )) - def test_refresh_access_token_fails(self, mock_post): + @responses.activate + def test_refresh_access_token_fails(self): """ check the mixin correctly raises exception when server responds 401 """ + # mock a 401 response code for the token + responses.add('POST', '/oauth2/v1/token', status=401) + + configuration = CardClientConfiguration('client_key', 'client_secret') + card_api_instance = CardApi(ApiClient(configuration)) + with self.assertRaises(RuntimeError): - configuration = ClientCredentialsConfigurationMixin( - 'client_key', 'client_secret', 'access_token_url' - ) - configuration._refresh_access_token() + card_api_instance.v1beta1_available_barcodes_list() - @mock.patch('identitylib.api_client_mixin.requests.post', side_effect=MockResponse( - 200, "no access_token", {'no_access_token': 'not_an_access_token'} - )) - def test_refresh_access_token_no_token(self, mock_post): + @responses.activate + def test_refresh_access_token_no_token(self): """ check the mixin correctly raises exception when no access_token in response """ + responses.add('POST', '/oauth2/v1/token', body='{}') + + configuration = CardClientConfiguration('client_key', 'client_secret') + card_api_instance = CardApi(ApiClient(configuration)) + with self.assertRaises(RuntimeError): - configuration = ClientCredentialsConfigurationMixin( - 'client_key', 'client_secret', 'access_token_url' - ) - configuration._refresh_access_token() + card_api_instance.v1beta1_available_barcodes_list() + + @responses.activate + def test_will_refresh_the_token(self): + token_calls = 0 + + # record requests for an access token + def token_callback(request): + nonlocal token_calls + token_calls = token_calls + 1 + + # respond with no 'expires_in' which means we use the default of 1 minute + return 200, {}, '{"access_token": "new_access_token"}' + + responses.add_callback('POST', '/oauth2/v1/token', token_callback) + + # mock a successful request for the available barcodes + responses.add('GET', '/card/v1beta1/available-barcodes', body='{"results": []}') + + configuration = CardClientConfiguration('client_key_test', 'client_secret_test') + card_api_instance = CardApi(ApiClient(configuration)) + + now = datetime.now() + with freeze_time(now) as frozen_datetime: + card_api_instance.v1beta1_available_barcodes_list() + # an access token should have been requested + self.assertEqual(token_calls, 1) + + frozen_datetime.move_to(now + timedelta(seconds=60)) + card_api_instance.v1beta1_available_barcodes_list() + + # another access token should have been requested + self.assertEqual(token_calls, 2) + + card_api_instance.v1beta1_available_barcodes_list() + # our access token is still valid so we shouldn't have requested another + self.assertEqual(token_calls, 2) + + @responses.activate + def test_will_use_cached_token_until_expired(self): + token_calls = 0 + + # record requests for an access token + def token_callback(request): + nonlocal token_calls + token_calls = token_calls + 1 + + # respond with an 'expires_in' which indicates the token expires in 10 minutes + return 200, {}, '{"access_token": "test_access_token", "expires_in": 600}' + + responses.add_callback('POST', '/oauth2/v1/token', token_callback) + + # mock a successful request for the available barcodes + responses.add('GET', '/card/v1beta1/available-barcodes', body='{"results": []}') + + configuration = CardClientConfiguration('client_key_test', 'client_secret_test') + + card_api_instance = CardApi(ApiClient(configuration)) + + now = datetime.now() + with freeze_time(now) as frozen_datetime: + # first two calls should use the existing API token + card_api_instance.v1beta1_available_barcodes_list() + card_api_instance.v1beta1_available_barcodes_list() + + self.assertEqual(token_calls, 1) + + # travel forward in time + frozen_datetime.move_to(now + timedelta(seconds=600)) + + card_api_instance.v1beta1_available_barcodes_list() + # a new access token should have been requested, so we now have two calls + self.assertEqual(token_calls, 2) diff --git a/tox.ini b/tox.ini index fbbe9958c2675c944107aba209ff0b97069516b1..a8914464d9459fe56a2a0ce26a36461acf10c43d 100644 --- a/tox.ini +++ b/tox.ini @@ -39,9 +39,10 @@ passenv= commands= pytest \ --cov={toxinidir} \ + -p no:warnings \ --junitxml={[_vars]build_root}/{envname}/junit.xml \ --ignore-glob=identitylib/card_client/* \ - --ignore-glob=identitylib/photo_client/* + --ignore-glob=identitylib/photo_client/* {posargs} coverage html --directory {[_vars]build_root}/{envname}/htmlcov/ coverage xml -o {env:COVERAGE_XML_FILE:{[_vars]build_root}/{envname}/coverage.xml}