misc: improve error handling when reading/writing roles summary cache (#84096)

* use atomicwriter to replace existing cache
* in case of failure to read or write the new cache, log the error and
  report the problem in the web interface.

src/authentic2/manager/role_views.py

@@ -857,8 +857,10 @@ class RoleSummaryView(RoleViewMixin, views.MediaMixin, DetailView):
             F('ou').asc(nulls_first=True), 'name'
         )
         ctx['admin_roles'] = list(self.object.get_admin_role().children(include_self=False))
-        summary_data = get_roles_summary_cache().get(self.context.uuid, {})
+        roles_summary_cache = get_roles_summary_cache()
+        summary_data = roles_summary_cache.get(self.context.uuid, {})
         ctx['summary_data'] = summary_data or {'type_objects': [], 'parents_type_objects': []}
+        ctx['summary_data_error'] = roles_summary_cache.get('error')
         return ctx
 
 

src/authentic2/manager/templates/authentic2/manager/role_summary.html

@@ -12,6 +12,11 @@
 {% endblock %}
 
 {% block main %}
+  {% if summary_data_error %}
+    <div class="errornotice">
+      <p>{{ summary_data_error }}</p>
+    </div>
+  {% endif %}
   <div class="fx-grid--d2">
     <div>
       <div class="section" id="parents">

src/authentic2/role_summary.py

@@ -20,8 +20,10 @@ import os.path
 import urllib
 
 import requests
+from atomicwrites import AtomicWriter
 from django.conf import settings
 from django.db import connection
+from django.utils.translation import gettext as _
 
 from .a2_rbac.models import Role
 
@@ -33,7 +35,7 @@ except ImportError:  # fallback on python requests, no Publik signature
 logger = logging.getLogger(__name__)
 
 
-def build_role_summary_cache():
+def build_roles_summary_cache():
     def _requests(url):
         logger.debug('role-summary: retrieving url %s', url)
         try:
@@ -107,7 +109,7 @@ def build_role_summary_cache():
     return data
 
 
-def get_role_summary_cache_path():
+def get_roles_summary_cache_path():
     try:
         tenant_dir = connection.tenant.get_directory()
     except AttributeError:
@@ -116,16 +118,31 @@ def get_role_summary_cache_path():
 
 
 def get_roles_summary_cache():
-    path = get_role_summary_cache_path()
-    if not path or not os.path.exists(path):
-        return {}
-    with open(path) as fd:
-        return json.load(fd)
+    try:
+        path = get_roles_summary_cache_path()
+        if not path or not os.path.exists(path):
+            return {}
+        with open(path) as fd:
+            roles_summary_cache = json.load(fd)
+    except Exception as e:
+        logger.exception('failed to load role summary cache')
+        roles_summary_cache = {'error': _('Loading of roles summary cache failed: %s') % e}
+    return roles_summary_cache
 
 
 def write_roles_summary_cache():
-    path = get_role_summary_cache_path()
-    if not path:
-        return
-    with open(path, 'w') as fd:
-        json.dump(build_role_summary_cache(), fd, indent=2)
+    try:
+        path = get_roles_summary_cache_path()
+        if not path:
+            return
+        roles_summary_cache = build_roles_summary_cache()
+    except Exception as e:
+        logger.exception('failed to build role summary cache')
+        roles_summary_cache = get_roles_summary_cache()
+        roles_summary_cache['error'] = _('Building of roles summary cache failed: %s') % e
+
+    try:
+        with AtomicWriter(path, mode='w', overwrite=True).open() as fd:
+            json.dump(roles_summary_cache, fd, indent=2)
+    except Exception:
+        logger.exception('failed to build role summary cache')

tests/test_role_manager.py

@@ -727,69 +727,71 @@ def test_role_summary_other_services(app, superuser, settings, simple_role, monk
     parent_role = Role.objects.create(name='parent role', slug='parent-role', ou=get_default_ou())
     simple_role.add_parent(parent_role)
 
+    roles_summary_cache = {
+        simple_role.uuid: {
+            'name': 'simple role',
+            'slug': 'simple-role',
+            'parents': [parent_role.uuid],
+            'type_objects': [
+                {
+                    'hit': [
+                        {
+                            'id': 'foo',
+                            'text': 'Foo',
+                            'type': 'forms',
+                            'urls': {
+                                'dependencies': 'http://example.org/api/export-import/forms/foo/dependencies/',
+                                'redirect': 'http://example.org/api/export-import/forms/foo/redirect/',
+                            },
+                        }
+                    ],
+                    'id': 'forms',
+                    'singular': 'Formulaire',
+                    'text': 'Formulaires',
+                    'urls': {'list': 'http://example.org/api/export-import/forms/'},
+                },
+                {
+                    'hit': [
+                        {
+                            'id': 'test',
+                            'text': 'Test',
+                            'type': 'workflows',
+                            'urls': {
+                                'dependencies': 'http://example.org/api/export-import/workflows/test/dependencies/',
+                                'redirect': 'http://example.org/api/export-import/workflows/test/redirect/',
+                            },
+                        }
+                    ],
+                    'id': 'workflows',
+                    'singular': 'Workflow',
+                    'text': 'Workflows',
+                    'urls': {'list': 'http://example.org/api/export-import/workflows/'},
+                },
+            ],
+            'parents_type_objects': [
+                {
+                    'hit': [
+                        {
+                            'id': 'bar',
+                            'text': 'Bar',
+                            'type': 'forms',
+                            'urls': {
+                                'dependencies': 'http://example.org/api/export-import/forms/bar/dependencies/',
+                                'redirect': 'http://example.org/api/export-import/forms/bar/redirect/',
+                            },
+                        }
+                    ],
+                    'id': 'forms',
+                    'singular': 'Formulaire',
+                    'text': 'Formulaires',
+                    'urls': {'list': 'http://example.org/api/export-import/forms/'},
+                }
+            ],
+        },
+    }
+
     def mock_get_roles_summary_cache():
-        return {
-            simple_role.uuid: {
-                'name': 'simple role',
-                'slug': 'simple-role',
-                'parents': [parent_role.uuid],
-                'type_objects': [
-                    {
-                        'hit': [
-                            {
-                                'id': 'foo',
-                                'text': 'Foo',
-                                'type': 'forms',
-                                'urls': {
-                                    'dependencies': 'http://example.org/api/export-import/forms/foo/dependencies/',
-                                    'redirect': 'http://example.org/api/export-import/forms/foo/redirect/',
-                                },
-                            }
-                        ],
-                        'id': 'forms',
-                        'singular': 'Formulaire',
-                        'text': 'Formulaires',
-                        'urls': {'list': 'http://example.org/api/export-import/forms/'},
-                    },
-                    {
-                        'hit': [
-                            {
-                                'id': 'test',
-                                'text': 'Test',
-                                'type': 'workflows',
-                                'urls': {
-                                    'dependencies': 'http://example.org/api/export-import/workflows/test/dependencies/',
-                                    'redirect': 'http://example.org/api/export-import/workflows/test/redirect/',
-                                },
-                            }
-                        ],
-                        'id': 'workflows',
-                        'singular': 'Workflow',
-                        'text': 'Workflows',
-                        'urls': {'list': 'http://example.org/api/export-import/workflows/'},
-                    },
-                ],
-                'parents_type_objects': [
-                    {
-                        'hit': [
-                            {
-                                'id': 'bar',
-                                'text': 'Bar',
-                                'type': 'forms',
-                                'urls': {
-                                    'dependencies': 'http://example.org/api/export-import/forms/bar/dependencies/',
-                                    'redirect': 'http://example.org/api/export-import/forms/bar/redirect/',
-                                },
-                            }
-                        ],
-                        'id': 'forms',
-                        'singular': 'Formulaire',
-                        'text': 'Formulaires',
-                        'urls': {'list': 'http://example.org/api/export-import/forms/'},
-                    }
-                ],
-            },
-        }
+        return roles_summary_cache
 
     import authentic2.manager.role_views
 
@@ -822,6 +824,12 @@ def test_role_summary_other_services(app, superuser, settings, simple_role, monk
     assert form_anchor.text == 'Bar'
     assert form_anchor.attrib['href'] == 'http://example.org/api/export-import/forms/bar/redirect/'
 
+    assert not resp.pyquery.find('div.errornotice')
+
+    roles_summary_cache['error'] = 'BOOM!'
+    resp = app.get(url)
+    assert resp.pyquery.find('div.errornotice').text() == 'BOOM!'
+
 
 def test_role_summary_in_nav(app, superuser, simple_role):
     url = reverse('a2-manager-role-members', kwargs={'pk': simple_role.pk})

tests/test_role_summary.py

@@ -1,3 +1,5 @@
+from unittest import mock
+
 import responses
 
 from authentic2 import role_summary
@@ -136,7 +138,7 @@ def test_role_summary(db, simple_role, superuser, tmpdir, monkeypatch, caplog):
             },
         )
 
-        data = role_summary.build_role_summary_cache()
+        data = role_summary.build_roles_summary_cache()
         assert data == {
             simple_role.uuid: {
                 'name': 'simple role',
@@ -225,23 +227,47 @@ def test_role_summary(db, simple_role, superuser, tmpdir, monkeypatch, caplog):
             },
         }
         assert (
-            caplog.text
+            '\n'.join(caplog.messages)  # do not use caplog.text, it contains line of code numbers
             == '''\
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'forms', 'text': 'Formulaires', 'singular': 'Formulaire', 'urls': {'list': 'http://example.org/api/export-import/forms/'}}, {'id': 'workflows', 'text': 'Workflows', 'singular': 'Workflow', 'urls': {'list': 'http://example.org/api/export-import/workflows/'}}, {'id': 'roles', 'text': 'Roles', 'singular': 'Role', 'urls': {'list': 'http://example.org/api/export-import/roles/'}}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/forms/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'foo', 'text': 'Foo', 'type': 'forms', 'urls': {'dependencies': 'http://example.org/api/export-import/forms/foo/dependencies/', 'redirect': 'http://example.org/api/export-import/forms/foo/redirect/'}}, {'id': 'bar', 'text': 'Bar', 'type': 'forms', 'urls': {'dependencies': 'http://example.org/api/export-import/forms/bar/dependencies/', 'redirect': 'http://example.org/api/export-import/forms/bar/redirect/'}}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/forms/foo/dependencies/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'simple-role', 'text': 'simple role', 'uuid': '6115a844a91840f6a83f942c0180f80f', 'type': 'roles'}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/forms/bar/dependencies/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'parent-role', 'text': 'parent role', 'uuid': '73b3d397fbcf4252aecedb195bab8281', 'type': 'roles'}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/workflows/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'test', 'text': 'Test', 'type': 'workflows', 'urls': {'dependencies': 'http://example.org/api/export-import/workflows/test/dependencies/', 'redirect': 'http://example.org/api/export-import/workflows/test/redirect/'}}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/workflows/test/dependencies/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'simple-role', 'text': 'simple role', 'uuid': '6115a844a91840f6a83f942c0180f80f', 'type': 'roles'}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url http://example.org/api/export-import/roles/
-DEBUG    authentic2.role_summary:role_summary.py:53 role-summary: response {'data': [{'id': 'agent', 'text': 'Agent', 'type': 'roles', 'urls': {}}]}
-DEBUG    authentic2.role_summary:role_summary.py:38 role-summary: retrieving url https://foo.whatever.none/api/export-import/
-DEBUG    authentic2.role_summary:role_summary.py:43 role-summary: error 404 Client Error: Not Found for url: https://foo.whatever.none/api/export-import/
-'''
+role-summary: retrieving url http://example.org/api/export-import/
+role-summary: response {'data': [{'id': 'forms', 'text': 'Formulaires', 'singular': 'Formulaire', 'urls': {'list': 'http://example.org/api/export-import/forms/'}}, {'id': 'workflows', 'text': 'Workflows', 'singular': 'Workflow', 'urls': {'list': 'http://example.org/api/export-import/workflows/'}}, {'id': 'roles', 'text': 'Roles', 'singular': 'Role', 'urls': {'list': 'http://example.org/api/export-import/roles/'}}]}
+role-summary: retrieving url http://example.org/api/export-import/forms/
+role-summary: response {'data': [{'id': 'foo', 'text': 'Foo', 'type': 'forms', 'urls': {'dependencies': 'http://example.org/api/export-import/forms/foo/dependencies/', 'redirect': 'http://example.org/api/export-import/forms/foo/redirect/'}}, {'id': 'bar', 'text': 'Bar', 'type': 'forms', 'urls': {'dependencies': 'http://example.org/api/export-import/forms/bar/dependencies/', 'redirect': 'http://example.org/api/export-import/forms/bar/redirect/'}}]}
+role-summary: retrieving url http://example.org/api/export-import/forms/foo/dependencies/
+role-summary: response {'data': [{'id': 'simple-role', 'text': 'simple role', 'uuid': '6115a844a91840f6a83f942c0180f80f', 'type': 'roles'}]}
+role-summary: retrieving url http://example.org/api/export-import/forms/bar/dependencies/
+role-summary: response {'data': [{'id': 'parent-role', 'text': 'parent role', 'uuid': '73b3d397fbcf4252aecedb195bab8281', 'type': 'roles'}]}
+role-summary: retrieving url http://example.org/api/export-import/workflows/
+role-summary: response {'data': [{'id': 'test', 'text': 'Test', 'type': 'workflows', 'urls': {'dependencies': 'http://example.org/api/export-import/workflows/test/dependencies/', 'redirect': 'http://example.org/api/export-import/workflows/test/redirect/'}}]}
+role-summary: retrieving url http://example.org/api/export-import/workflows/test/dependencies/
+role-summary: response {'data': [{'id': 'simple-role', 'text': 'simple role', 'uuid': '6115a844a91840f6a83f942c0180f80f', 'type': 'roles'}]}
+role-summary: retrieving url http://example.org/api/export-import/roles/
+role-summary: response {'data': [{'id': 'agent', 'text': 'Agent', 'type': 'roles', 'urls': {}}]}
+role-summary: retrieving url https://foo.whatever.none/api/export-import/
+role-summary: error 404 Client Error: Not Found for url: https://foo.whatever.none/api/export-import/'''
         )
+
+
+@mock.patch('authentic2.role_summary.get_roles_summary_cache_path')
+@mock.patch('authentic2.role_summary.build_roles_summary_cache')
+def test_write_roles_summary_cache_error(build_roles_summary_cache, get_roles_summary_cache_path, tmp_path):
+    cache_path = tmp_path / 'cache.json'
+    get_roles_summary_cache_path.return_value = str(cache_path)
+    build_roles_summary_cache.side_effect = Exception('Boom!')
+
+    role_summary.write_roles_summary_cache()
+
+    assert role_summary.get_roles_summary_cache() == {
+        'error': 'Building of roles summary cache failed: Boom!'
+    }
+
+
+@mock.patch('authentic2.role_summary.get_roles_summary_cache_path')
+def test_read_roles_summary_cache_error(get_roles_summary_cache_path, tmp_path):
+    cache_path = tmp_path / 'cache.json'
+    get_roles_summary_cache_path.return_value = str(cache_path)
+    with cache_path.open('w') as fd:
+        fd.write('x')
+    assert role_summary.get_roles_summary_cache() == {
+        'error': 'Loading of roles summary cache failed: Expecting value: line 1 column 1 (char 0)'
+    }