From 47f495aaac074dd228098c4265c073c2be545c84 Mon Sep 17 00:00:00 2001
From: Adrian <adrian.moennich@cern.ch>
Date: Tue, 3 Dec 2024 16:15:27 +0100
Subject: [PATCH] Cache group member lists (#32)

* Add caching to CERNGroup.get_members
* Use group name directly instead of querying id first
---
 flask_multipass_cern.py     | 70 +++++++++++++++++++++++++------------
 tests/test_request_retry.py | 12 -------
 2 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/flask_multipass_cern.py b/flask_multipass_cern.py
index f42f2fc..eaa38da 100644
--- a/flask_multipass_cern.py
+++ b/flask_multipass_cern.py
@@ -139,12 +139,32 @@ def get_members(self):
         if self.provider.settings['cern_users_group'] and self.name.lower() == 'cern users':
             name = self.provider.settings['cern_users_group']
         assert '/' not in name
-        with self.provider._get_api_session() as api_session:
-            group_data = self.provider._get_group_data(name)
-            if group_data is None:
+
+        cache_key = f'flask-multipass-cern:{self.provider.name}:group-members:{name}'
+        cached_results = None
+        if (cached_data := self.provider.cache.get(cache_key)) is not None:
+            cached_results = []
+            for res in cached_data:
+                identifier = res.pop('upn')
+                extra_data = self.provider._extract_extra_data(res)
+                cached_results.append(IdentityInfo(self.provider, identifier, extra_data, **res))
+            if not self.provider.cache.should_refresh(cache_key):
+                yield from cached_results
                 return
-            gid = group_data['id']
 
+        try:
+            api_session = self.provider._get_api_session()
+        except RequestException:
+            self.provider.logger.warning('Refreshing members failed for group %s (could not get API token)', name)
+            if cached_results is not None:
+                self.provider.cache.delay_refresh(cache_key, CACHE_TTL)
+                yield from cached_results
+                return
+            else:
+                self.provider.logger.error('Getting members failed for group %s (could not get API token)', name)
+                raise
+
+        with api_session:
             params = {
                 'limit': 5000,
                 'field': [
@@ -157,14 +177,32 @@ def get_members(self):
                     'personId',
                 ],
             }
-            results = self.provider._fetch_all(api_session, f'/api/v1.0/Group/{gid}/memberidentities/precomputed',
-                                               params)[0]
+            try:
+                results = self.provider._fetch_all(api_session, f'/api/v1.0/Group/{name}/memberidentities/precomputed',
+                                                params)[0]
+            except RequestException:
+                self.provider.logger.warning('Refreshing members failed for group %s', name)
+                if cached_results is not None:
+                    self.provider.cache.delay_refresh(cache_key, CACHE_TTL)
+                    yield from cached_results
+                    return
+                else:
+                    self.provider.logger.error('Getting members failed for group %s', name)
+                    raise
+
+        cache_data = []
+        identities = []
         for res in results:
             del res['id']  # id is always included
             self.provider._fix_phone(res)
-            identifier = res.pop('upn')
-            extra_data = self.provider._extract_extra_data(res)
-            yield IdentityInfo(self.provider, identifier, extra_data, **res)
+            res_copy = dict(res)
+            identifier = res_copy.pop('upn')
+            extra_data = self.provider._extract_extra_data(res_copy)
+            identities.append(IdentityInfo(self.provider, identifier, extra_data, **res_copy))
+            cache_data.append(res)
+
+        self.provider.cache.set(cache_key, cache_data, CACHE_LONG_TTL, CACHE_TTL)
+        yield from identities
 
     def has_member(self, identifier):
         try:
@@ -509,20 +547,6 @@ def _fetch_all(self, api_session, endpoint, params, limit=None):
             results = results[:limit]
         return results, total
 
-    @memoize_request
-    def _get_group_data(self, name):
-        params = {
-            'filter': [f'groupIdentifier:eq:{name}'],
-            'field': ['id', 'groupIdentifier'],
-        }
-        with self._get_api_session() as api_session:
-            resp = api_session.get(f'{self.authz_api_base}/api/v1.0/Group', params=params)
-            resp.raise_for_status()
-            data = resp.json()
-        if len(data['data']) != 1:
-            return None
-        return data['data'][0]
-
     def _get_identity_data(self, identifier):
         params = {
             'field': [
diff --git a/tests/test_request_retry.py b/tests/test_request_retry.py
index da10057..15e9b81 100644
--- a/tests/test_request_retry.py
+++ b/tests/test_request_retry.py
@@ -34,18 +34,6 @@ def test_get_identity_data_retry(provider):
         assert len(httpretty.latest_requests()) == HTTP_RETRY_COUNT + 1
 
 
-@pytest.mark.usefixtures('httpretty_enabled', 'mock_get_api_session')
-def test_get_group_data_retry(provider):
-    authz_api = provider.settings.get('authz_api')
-    test_uri = f'{authz_api}/api/v1.0/Group'
-    httpretty.register_uri(httpretty.GET, test_uri, status=503)
-
-    try:
-        provider._get_group_data('mygroup')
-    except requests.exceptions.RequestException:
-        assert len(httpretty.latest_requests()) == HTTP_RETRY_COUNT + 1
-
-
 @pytest.mark.usefixtures('httpretty_enabled', 'mock_get_api_session')
 def test_fetch_all_retry(provider):
     authz_api_base = provider.settings.get('authz_api')