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