From a4a35026ea7cfaa96c55957ae6827337b28318f0 Mon Sep 17 00:00:00 2001
From: Paul Marillonnet <pmarillonnet@entrouvert.com>
Date: Tue, 28 Apr 2020 14:01:19 +0200
Subject: [PATCH] a2_rbac: do not break unicity when get-or-creating admin role
 (#42178)

---
 src/authentic2/a2_rbac/management.py | 32 +++++++++++++++++---------
 src/authentic2/a2_rbac/managers.py   | 34 +++++++++++++---------------
 src/authentic2/a2_rbac/models.py     |  2 +-
 tests/test_a2_rbac.py                |  6 ++---
 4 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/src/authentic2/a2_rbac/management.py b/src/authentic2/a2_rbac/management.py
index 627aa9afc..2a9a49bd4 100644
--- a/src/authentic2/a2_rbac/management.py
+++ b/src/authentic2/a2_rbac/management.py
@@ -20,7 +20,9 @@ from django.utils.text import slugify
 from django.contrib.contenttypes.models import ContentType
 
 from django_rbac.models import ADMIN_OP
-from django_rbac.utils import get_role_model, get_ou_model
+from django_rbac.utils import get_ou_model
+from django_rbac.utils import get_permission_model
+from django_rbac.utils import get_role_model
 
 from ..utils import get_fk_model
 from . import utils, app_settings
@@ -138,7 +140,9 @@ def update_content_types_roles():
             continue
         ct_admin_role = Role.objects.get_admin_role(instance=ct, name=name,
                                                     slug=slug,
-                                                    update_name=True)
+                                                    update_name=True,
+                                                    update_slug=True,
+                                                    create=True)
         if MANAGED_CT[ct_tuple].get('must_view_user'):
             ct_admin_role.permissions.add(view_user_perm)
         ct_admin_role.permissions.add(search_ou_perm)
@@ -146,16 +150,22 @@ def update_content_types_roles():
 
 
 def update_user_admin_roles_permission():
-    roles = get_role_model().objects.filter(slug__startswith='_a2-managers-of-role',
-                                            permissions__operation__slug=ADMIN_OP.slug)
-    for role in roles:
-        old_perm = role.permissions.get(operation__slug=ADMIN_OP.slug)
-        administered_role = old_perm.target
+    role_ct = ContentType.objects.get_for_model(get_role_model())
+    permissions = get_permission_model().objects.filter(target_ct=role_ct,
+            operation__slug=ADMIN_OP.slug)
+    for perm in permissions:
+        administered_role = perm.target
         admin_role = administered_role.get_admin_role()
-        new_perm = admin_role.permissions.get(operation__slug=MANAGE_MEMBERS_OP.slug)
+        assert admin_role.slug.startswith('_a2-managers-of-role')
+        new_perm = admin_role.permissions.get(
+                operation__slug=MANAGE_MEMBERS_OP.slug)
+        assert new_perm.ou is None
+
         admin_role.delete()
+        assert len(perm.roles.all()) == 1
+        role = perm.roles.first()
         role.admin_scope_id = new_perm.pk
-        role.save()
-        role.permissions.remove(old_perm)
+        role.save(update_fields=['admin_scope_id'])
+        role.permissions.remove(perm)
         role.permissions.add(new_perm)
-        assert role.pk == administered_role.get_admin_role().pk
+        assert role.pk == administered_role.get_admin_role(create=False).pk
diff --git a/src/authentic2/a2_rbac/managers.py b/src/authentic2/a2_rbac/managers.py
index 0ef6a8cc9..8c441954b 100644
--- a/src/authentic2/a2_rbac/managers.py
+++ b/src/authentic2/a2_rbac/managers.py
@@ -41,7 +41,7 @@ class RoleManager(BaseRoleManager):
         op = get_operation(operation)
         Permission = rbac_utils.get_permission_model()
         if create:
-            perm, created = Permission.objects.get_or_create(
+            perm, _ = Permission.objects.get_or_create(
                 operation=op,
                 target_ct=ContentType.objects.get_for_model(instance),
                 target_id=instance.pk,
@@ -55,7 +55,6 @@ class RoleManager(BaseRoleManager):
                     **kwargs)
             except Permission.DoesNotExist:
                 return None
-            created = False
 
         admin_role = self.get_mirror_role(perm, name, slug, ou=ou,
                                           update_name=update_name,
@@ -76,23 +75,27 @@ class RoleManager(BaseRoleManager):
 
     def get_mirror_role(self, instance, name, slug, ou=None,
                         update_name=False, update_slug=False, create=True):
-        '''Get or create a role which mirror another model, for example a
+        '''Get or create a role which mirrors another model, for example a
            permission.
         '''
         ct = ContentType.objects.get_for_model(instance)
+        update_fields = {}
         kwargs = {}
-        if ou or getattr(instance, 'ou', None):
-            kwargs['ou'] = ou or instance.ou
+        ou = ou or getattr(instance, 'ou', None)
+        if ou:
+            update_fields['ou'] = ou
         else:
             kwargs['ou__isnull'] = True
+        if update_name:
+            update_fields['name'] = name
+        if update_slug:
+            update_fields['slug'] = slug
+
         if create:
-            role, created = self.prefetch_related('permissions').get_or_create(
+            role, _ = self.prefetch_related('permissions').update_or_create(
                 admin_scope_ct=ct,
                 admin_scope_id=instance.pk,
-                defaults={
-                    'name': name,
-                    'slug': slug,
-                },
+                defaults=update_fields,
                 **kwargs)
         else:
             try:
@@ -102,14 +105,9 @@ class RoleManager(BaseRoleManager):
                     **kwargs)
             except self.model.DoesNotExist:
                 return None
-            created = False
-
-        if update_name and not created and role.name != name:
-            role.name = name
-            role.save()
-        if update_slug and not created and role.slug != slug:
-            role.slug = slug
-            role.save()
+            for field, value in update_fields.items():
+                setattr(role, field, value)
+            role.save(update_fields=update_fields)
         return role
 
     def get_by_natural_key(self, slug, ou_natural_key, service_natural_key):
diff --git a/src/authentic2/a2_rbac/models.py b/src/authentic2/a2_rbac/models.py
index 26d11873f..a5048f2a9 100644
--- a/src/authentic2/a2_rbac/models.py
+++ b/src/authentic2/a2_rbac/models.py
@@ -151,7 +151,7 @@ class OrganizationalUnit(OrganizationalUnitAbstractBase):
         slug = '_a2-managers-of-{ou.slug}'.format(ou=self)
         return Role.objects.get_admin_role(
             instance=self, name=name, slug=slug, operation=VIEW_OP,
-            update_name=True, update_slug=True)
+            update_name=True, update_slug=True, create=True)
 
     def delete(self, *args, **kwargs):
         Permission.objects.filter(ou=self).delete()
diff --git a/tests/test_a2_rbac.py b/tests/test_a2_rbac.py
index a52a27ee5..f34fd24bd 100644
--- a/tests/test_a2_rbac.py
+++ b/tests/test_a2_rbac.py
@@ -108,7 +108,7 @@ def test_admin_roles_update_slug(db):
     user = User.objects.create(username='john.doe')
     name1 = 'Can manage john.doe'
     slug1 = 'can-manage-john-doe'
-    admin_role1 = Role.objects.get_admin_role(user, name1, slug1)
+    admin_role1 = Role.objects.get_admin_role(user, name1, slug1, update_name=True, update_slug=True)
     assert admin_role1.name == name1
     assert admin_role1.slug == slug1
     name2 = 'Should manage john.doe'
@@ -403,7 +403,7 @@ def test_role_rename(db):
     assert ar1.slug == '_a2-managers-of-role-r1ter'
 
 
-def test_admin_role_user_view(settings, app, admin, simple_user, ou1, user_ou1, role_ou1):
+def test_admin_role_user_view(db, settings, app, admin, simple_user, ou1, user_ou1, role_ou1):
     role_ou1.get_admin_role().members.add(simple_user)
 
     # Default: all users are visible
@@ -493,7 +493,7 @@ def test_manager_roles_multi_ou(db, ou1):
 @pytest.mark.parametrize(
     'alert,deletion', [(-1, 31), (31, -1), (0, 31), (31, 0), (None, 31), (31, None), (32, 31)]
 )
-def test_unused_account_settings_validation(ou1, alert, deletion):
+def test_unused_account_settings_validation(db, ou1, alert, deletion):
     ou1.clean_unused_accounts_alert = alert
     ou1.clean_unused_accounts_deletion = deletion
     with pytest.raises(ValidationError):
-- 
2.28.0

