From 34ba64af3f0ca51fcb34ece742f89b3c1bac97fe Mon Sep 17 00:00:00 2001 From: Monty Dawson <wgd23@cam.ac.uk> Date: Thu, 3 Mar 2022 13:42:41 +0000 Subject: [PATCH 1/4] Add a failing test for token refresh --- identitylib/api_client_mixin.py | 2 +- requirements-tests.txt | 1 + tests/test_api_client_mixin.py | 28 ++++++++++++++++++++++++++++ tox.ini | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/identitylib/api_client_mixin.py b/identitylib/api_client_mixin.py index 5289004..944ab1c 100644 --- a/identitylib/api_client_mixin.py +++ b/identitylib/api_client_mixin.py @@ -4,7 +4,7 @@ from logging import getLogger LOG = getLogger(__name__) -class ClientCredentialsConfigurationMixin(object): +class ClientCredentialsConfigurationMixin: """ mixin to support client_credentials authentication flow """ diff --git a/requirements-tests.txt b/requirements-tests.txt index 3111445..c614aa8 100644 --- a/requirements-tests.txt +++ b/requirements-tests.txt @@ -2,3 +2,4 @@ coverage pytest pytest-cov requests_mock +urllib3-mock \ No newline at end of file diff --git a/tests/test_api_client_mixin.py b/tests/test_api_client_mixin.py index 2652b5a..f1156e8 100644 --- a/tests/test_api_client_mixin.py +++ b/tests/test_api_client_mixin.py @@ -1,5 +1,13 @@ from unittest import TestCase, mock +from urllib3_mock import Responses + from identitylib.api_client_mixin import ClientCredentialsConfigurationMixin +from identitylib.card_client_configuration import CardClientConfiguration +from identitylib.card_client import ApiClient +from identitylib.card_client.api.v1beta1_api import V1beta1Api as CardApi + + +responses = Responses('requests.packages.urllib3') class TestApiClientConfigurationMixin(TestCase): @@ -58,3 +66,23 @@ class TestApiClientConfigurationMixin(TestCase): 'client_key', 'client_secret', 'access_token_url' ) configuration._refresh_access_token() + + @responses.activate + def test_refresh_works_when_api_responds_with_403(self, *args, **kwargs): + # mock the initial request for an access token + responses.add('POST', '/oauth/v1/test', body='{"access_token": "new_access_token"}') + # mock a successful request for the available barcodes + responses.add('GET', '/card/v1beta1/available-barcodes', body='{"results": []}') + # mock a failed request to the card logos endpoint + responses.add('GET', '/card/v1beta1/card-logos', status=401) + + configuration = CardClientConfiguration( + 'client_key_test', + 'client_secret_test', + 'https://api.apps.cam.ac.uk/oauth/v1/test' + ) + + card_api_instance = CardApi(ApiClient(configuration)) + + card_api_instance.v1beta1_available_barcodes_list() + card_api_instance.v1beta1_card_logos_list() diff --git a/tox.ini b/tox.ini index fbbe995..7255b40 100644 --- a/tox.ini +++ b/tox.ini @@ -41,7 +41,7 @@ commands= --cov={toxinidir} \ --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} -- GitLab From 89e097dc514d12d5a578c9a278890e7c51b45feb Mon Sep 17 00:00:00 2001 From: Monty Dawson <wgd23@cam.ac.uk> Date: Sat, 5 Mar 2022 20:47:15 +0000 Subject: [PATCH 2/4] Ensure that access token is refreshed before use --- identitylib/api_client_mixin.py | 60 ++++++-- identitylib/card_client_configuration.py | 12 +- identitylib/photo_client_configuration.py | 12 +- requirements-tests.txt | 3 +- tests/test_api_client_mixin.py | 162 ++++++++++++++-------- tox.ini | 1 + 6 files changed, 177 insertions(+), 73 deletions(-) diff --git a/identitylib/api_client_mixin.py b/identitylib/api_client_mixin.py index 944ab1c..4ee5c2e 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: + """ + 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 b164830..623caad 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 cbe63cc..2801aea 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 c614aa8..044fb56 100644 --- a/requirements-tests.txt +++ b/requirements-tests.txt @@ -2,4 +2,5 @@ coverage pytest pytest-cov requests_mock -urllib3-mock \ No newline at end of file +urllib3-mock +freezegun \ No newline at end of file diff --git a/tests/test_api_client_mixin.py b/tests/test_api_client_mixin.py index f1156e8..807049e 100644 --- a/tests/test_api_client_mixin.py +++ b/tests/test_api_client_mixin.py @@ -1,7 +1,9 @@ -from unittest import TestCase, mock +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.api_client_mixin import ClientCredentialsConfigurationMixin from identitylib.card_client_configuration import CardClientConfiguration from identitylib.card_client import ApiClient from identitylib.card_client.api.v1beta1_api import V1beta1Api as CardApi @@ -11,78 +13,128 @@ responses = Responses('requests.packages.urllib3') 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.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"}' - def __iter__(self): - return self + # mock the response from the token endpoint + responses.add_callback('POST', '/oauth2/v1/token', token_callback) - def __next__(self): - return self + # 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 + ) - def json(self): - return self.response_data + configuration = CardClientConfiguration('client_key_test', 'client_secret_test') + card_api_instance = CardApi(ApiClient(configuration)) - @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 """ + card_api_instance.v1beta1_available_barcodes_list() - configuration = ClientCredentialsConfigurationMixin( - 'client_key', 'client_secret', 'access_token_url' - ) - configuration._refresh_access_token() + @responses.activate + def test_refresh_access_token_fails(self): + """ check the mixin correctly raises exception when server responds 401 """ - self.assertEqual('new_access_token', configuration.access_token) + # mock a 401 response code for the token + responses.add('POST', '/oauth2/v1/token', status=401) - @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): - """ check the mixin correctly raises exception when server responds 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_refresh_works_when_api_responds_with_403(self, *args, **kwargs): - # mock the initial request for an access token - responses.add('POST', '/oauth/v1/test', body='{"access_token": "new_access_token"}') + 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": []}') - # mock a failed request to the card logos endpoint - responses.add('GET', '/card/v1beta1/card-logos', status=401) - configuration = CardClientConfiguration( - 'client_key_test', - 'client_secret_test', - 'https://api.apps.cam.ac.uk/oauth/v1/test' - ) + configuration = CardClientConfiguration('client_key_test', 'client_secret_test') card_api_instance = CardApi(ApiClient(configuration)) - card_api_instance.v1beta1_available_barcodes_list() - card_api_instance.v1beta1_card_logos_list() + 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 7255b40..a891446 100644 --- a/tox.ini +++ b/tox.ini @@ -39,6 +39,7 @@ 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/* {posargs} -- GitLab From dc2e7db871fd054d756d71a891ad764b9cdde986 Mon Sep 17 00:00:00 2001 From: Monty Dawson <wgd23@cam.ac.uk> Date: Sat, 5 Mar 2022 21:30:53 +0000 Subject: [PATCH 3/4] Up-version for 1.0.2 release --- CHANGELOG.md | 7 ++++++- setup.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83141f5..9e5f9cb 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/setup.py b/setup.py index 81eee42..250d3f4 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" -- GitLab From 9b5c21f1f9da94a4248ad4cce94750c293cdd7fa Mon Sep 17 00:00:00 2001 From: Monty Dawson <wgd23@cam.ac.uk> Date: Mon, 7 Mar 2022 12:10:30 +0000 Subject: [PATCH 4/4] Add missing requirements --- requirements.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 183d551..5716c92 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 -- GitLab