misc: implement soft-delete on RoleParenting (#57500)

src/authentic2/a2_rbac/migrations/0026_add_roleparenting_soft_delete.py

@@ -0,0 +1,27 @@
+# Generated by Django 2.2.19 on 2021-10-06 10:30
+
+import django.utils.timezone
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('a2_rbac', '0028_ou_home_url'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='roleparenting',
+            name='created',
+            field=models.DateTimeField(
+                auto_now_add=True, default=django.utils.timezone.now, verbose_name='Creation date'
+            ),
+            preserve_default=False,
+        ),
+        migrations.AddField(
+            model_name='roleparenting',
+            name='deleted',
+            field=models.DateTimeField(null=True, verbose_name='Deletion date'),
+        ),
+    ]

src/authentic2/custom_user/models.py

@@ -226,7 +226,10 @@ class User(AbstractBaseUser, PermissionMixin):
 
     def roles_and_parents(self):
         qs1 = self.roles.all()
-        qs2 = qs1.model.objects.filter(child_relation__child__in=qs1)
+        qs2 = qs1.model.objects.filter(
+            child_relation__deleted__isnull=True,
+            child_relation__child__in=qs1,
+        )
         qs = (qs1 | qs2).order_by('name').distinct()
         rp_qs = RoleParenting.objects.filter(child__in=qs1)
         qs = qs.prefetch_related(models.Prefetch('child_relation', queryset=rp_qs), 'child_relation__parent')

src/authentic2/management/commands/check-and-repair.py

@@ -388,7 +388,9 @@ class Command(BaseCommand):
                     direct_members = manager_role.members.all()
                     direct_members_count = direct_members.count()
                     direct_children = Role.objects.filter(
-                        parent_relation__parent=manager_role, parent_relation__direct=True
+                        parent_relation__deleted__isnull=True,
+                        parent_relation__parent=manager_role,
+                        parent_relation__direct=True,
                     )
                     direct_children_count = direct_children.count()
                     show = members_count or self.verbosity > 1
@@ -398,7 +400,9 @@ class Command(BaseCommand):
                             self.notice('- "%s" has problematic manager roles', role)
                         self.warning('  - %s', manager_role, ending=' ')
                     direct_parents = Role.objects.filter(
-                        child_relation__child=manager_role, child_relation__direct=True
+                        child_relation__deleted__isnull=True,
+                        child_relation__child=manager_role,
+                        child_relation__direct=True,
                     )
                     if show:
                         self.warning('DELETE', ending=' ')

src/authentic2/manager/resources.py

@@ -36,12 +36,10 @@ class UserResource(ModelResource):
     roles = Field()
 
     def dehydrate_roles(self, instance):
-        result = set()
+        roles = {role for role in instance.roles.all()}
-        for role in instance.roles.all():
+        # optimization as parent_relation is prefetched, filter deleted__isnull=True using python
-            result.add(role)
+        parents = {rp.parent for role in roles for rp in role.parent_relation.all() if not rp.deleted}
-            for pr in role.parent_relation.all():
+        return ', '.join(str(x) for x in roles | parents)
-                result.add(pr.parent)
-        return ', '.join(str(x) for x in result)
 
     class Meta:
         model = User

src/authentic2/manager/role_views.py

@@ -432,7 +432,7 @@ class RoleChildrenView(RoleViewMixin, views.HideOUColumnMixin, views.BaseSubTabl
                 Q(pk__in=children.filter(is_direct=False)), output_field=BooleanField()
             )
         )
-        rp_qs = RoleParenting.objects.filter(parent__in=children).annotate(name=F('parent__name'))
+        rp_qs = RoleParenting.alive.filter(parent__in=children).annotate(name=F('parent__name'))
         qs = qs.prefetch_related(Prefetch('parent_relation', queryset=rp_qs, to_attr='via'))
         return qs
 
@@ -494,7 +494,7 @@ class RoleParentsView(RoleViewMixin, views.HideOUColumnMixin, views.BaseSubTable
                 Q(pk__in=parents.filter(is_direct=False)), output_field=BooleanField()
             )
         )
-        rp_qs = RoleParenting.objects.filter(child__in=parents).annotate(name=F('child__name'))
+        rp_qs = RoleParenting.alive.filter(child__in=parents).annotate(name=F('child__name'))
         qs = qs.prefetch_related(Prefetch('child_relation', queryset=rp_qs, to_attr='via'))
         return qs
 

src/authentic2/manager/tables.py

@@ -264,8 +264,8 @@ class UserRolesTable(Table):
     )
     ou = tables.Column()
     via = tables.TemplateColumn(
-        '{% if not record.member %}{% for rel in record.child_relation.all %}{{ rel.child }} {% if not'
+        '{% if not record.member %}{% for rel in record.child_relation.all %}'
-        ' forloop.last %}, {% endif %}{% endfor %}{% endif %}',
+        '{{ rel.child }} {% if not forloop.last %}, {% endif %}{% endfor %}{% endif %}',
         verbose_name=_('Inherited from'),
         orderable=False,
         attrs={"td": {"class": "via"}},

src/authentic2/manager/user_views.py

@@ -640,7 +640,7 @@ class UserRolesView(HideOUColumnMixin, BaseSubTableView):
         if self.is_ou_specified():
             roles = self.object.roles.all()
             User = get_user_model()
-            rp_qs = RoleParenting.objects.filter(child__in=roles)
+            rp_qs = RoleParenting.alive.filter(child__in=roles)
             qs = Role.objects.all()
             qs = qs.prefetch_related(models.Prefetch('child_relation', queryset=rp_qs, to_attr='via'))
             qs = qs.prefetch_related(

src/django_rbac/apps.py

@@ -8,7 +8,7 @@ class DjangoRBACConfig(AppConfig):
     def ready(self):
         from django.db.models.signals import post_delete, post_migrate, post_save
 
-        from . import signal_handlers, utils
+        from . import signal_handlers, signals, utils
 
         # update role parenting when new role parenting is created
         post_save.connect(signal_handlers.role_parenting_post_save, sender=utils.get_role_parenting_model())
@@ -16,6 +16,14 @@ class DjangoRBACConfig(AppConfig):
         post_delete.connect(
             signal_handlers.role_parenting_post_delete, sender=utils.get_role_parenting_model()
         )
+        # or soft-created
+        signals.post_soft_create.connect(
+            signal_handlers.role_parenting_post_soft_delete, sender=utils.get_role_parenting_model()
+        )
+        # or soft-deleted
+        signals.post_soft_delete.connect(
+            signal_handlers.role_parenting_post_soft_delete, sender=utils.get_role_parenting_model()
+        )
         # create CRUD operations and admin
         post_migrate.connect(signal_handlers.create_base_operations, sender=self)
         # update role parenting in post migrate

src/django_rbac/managers.py

@@ -1,4 +1,5 @@
 import contextlib
+import datetime
 import threading
 
 from django.contrib.auth import get_user_model
@@ -8,7 +9,7 @@ from django.db.models import query
 from django.db.models.query import Prefetch, Q
 from django.db.transaction import atomic
 
-from . import utils
+from . import signals, utils
 
 
 class AbstractBaseManager(models.Manager):
@@ -107,7 +108,10 @@ class RoleQuerySet(query.QuerySet):
         return self.filter(members=user).parents().distinct()
 
     def parents(self, include_self=True, annotate=False):
-        qs = self.model.objects.filter(child_relation__child__in=self)
+        qs = self.model.objects.filter(
+            child_relation__deleted__isnull=True,
+            child_relation__child__in=self,
+        )
         if include_self:
             qs = self | qs
         qs = qs.distinct()
@@ -116,7 +120,10 @@ class RoleQuerySet(query.QuerySet):
         return qs
 
     def children(self, include_self=True, annotate=False):
-        qs = self.model.objects.filter(parent_relation__parent__in=self)
+        qs = self.model.objects.filter(
+            parent_relation__deleted__isnull=True,
+            parent_relation__parent__in=self,
+        )
         if include_self:
             qs = self | qs
         qs = qs.distinct()
@@ -128,7 +135,10 @@ class RoleQuerySet(query.QuerySet):
         User = get_user_model()
         prefetch = Prefetch('roles', queryset=self, to_attr='direct')
         return (
-            User.objects.filter(Q(roles__in=self) | Q(roles__parent_relation__parent__in=self))
+            User.objects.filter(
+                Q(roles__in=self)
+                | Q(roles__parent_relation__parent__in=self, roles__parent_relation__deleted__isnull=True)
+            )
             .distinct()
             .prefetch_related(prefetch)
         )
@@ -168,6 +178,29 @@ class RoleParentingManager(models.Manager):
             raise self.model.DoesNotExist
         return self.get(parent=parent, child=child, direct=direct)
 
+    def soft_create(self, parent, child):
+        with atomic(savepoint=False):
+            rp, created = self.get_or_create(parent=parent, child=child, direct=True)
+            new = created or rp.deleted
+            if not created and rp.deleted:
+                rp.created = datetime.datetime.now()
+                rp.deleted = None
+                rp.save(update_fields=['created', 'deleted'])
+            if new:
+                signals.post_soft_create.send(sender=self.model, instance=rp)
+
+    def soft_delete(self, parent, child):
+        from . import signals
+
+        qs = self.filter(parent=parent, child=child, deleted__isnull=True, direct=True)
+        with atomic(savepoint=False):
+            rp = qs.first()
+            if rp:
+                count = qs.update(deleted=datetime.datetime.now())
+                # read-commited, view of tables can change during transaction
+                if count:
+                    signals.post_soft_delete.send(sender=self.model, instance=rp)
+
     def update_transitive_closure(self):
         """Recompute the transitive closure of the inheritance relation
         from scratch. Add missing indirect relations and delete
@@ -179,8 +212,10 @@ class RoleParentingManager(models.Manager):
 
         with atomic(savepoint=False):
             # existing direct paths
-            direct = set(self.filter(direct=True).values_list('parent_id', 'child_id'))
+            direct = set(self.filter(direct=True, deleted__isnull=True).values_list('parent_id', 'child_id'))
-            old_indirects = set(self.filter(direct=False).values_list('parent_id', 'child_id'))
+            old_indirects = set(
+                self.filter(direct=False, deleted__isnull=True).values_list('parent_id', 'child_id')
+            )
             indirects = set(direct)
 
             while True:
@@ -197,22 +232,26 @@ class RoleParentingManager(models.Manager):
                 # Delete old ones
                 obsolete = old_indirects - indirects - direct
                 if obsolete:
-                    obsolete_values = ', '.join('(%s, %s)' % (a, b) for a, b in obsolete)
+                    sql = '''UPDATE "%s" AS relation \
-                    sql = '''DELETE FROM "%s" AS relation \
+SET deleted = now()\
-USING (VALUES %s) AS dead(parent_id, child_id) \
+FROM (VALUES %s) AS dead(parent_id, child_id) \
 WHERE relation.direct = 'false' AND relation.parent_id = dead.parent_id \
-AND relation.child_id = dead.child_id''' % (
+AND relation.child_id = dead.child_id AND deleted IS NULL''' % (
                         self.model._meta.db_table,
-                        obsolete_values,
+                        ', '.join('(%s, %s)' % (a, b) for a, b in obsolete),
                     )
                     cur.execute(sql)
                 # Create new indirect relations
                 new = indirects - old_indirects - direct
                 if new:
                     new_values = ', '.join(
-                        ("(%s, %s, 'false')" % (parent_id, child_id) for parent_id, child_id in new)
+                        (
+                            "(%s, %s, 'false', now(), NULL)" % (parent_id, child_id)
+                            for parent_id, child_id in new
+                        )
                     )
-                    sql = '''INSERT INTO "%s" (parent_id, child_id, direct) VALUES %s''' % (
+                    sql = '''INSERT INTO "%s" (parent_id, child_id, direct, created, deleted) VALUES %s \
+ON CONFLICT (parent_id, child_id, direct) DO UPDATE SET created = EXCLUDED.created, deleted = NULL''' % (
                         self.model._meta.db_table,
                         new_values,
                     )

src/django_rbac/migrations/0008_add_roleparenting_soft_delete.py

@@ -0,0 +1,27 @@
+# Generated by Django 2.2.19 on 2021-10-06 10:34
+
+import django.utils.timezone
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('django_rbac', '0007_add_unique_constraints'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='roleparenting',
+            name='created',
+            field=models.DateTimeField(
+                auto_now_add=True, default=django.utils.timezone.now, verbose_name='Creation date'
+            ),
+            preserve_default=False,
+        ),
+        migrations.AddField(
+            model_name='roleparenting',
+            name='deleted',
+            field=models.DateTimeField(null=True, verbose_name='Deletion date'),
+        ),
+    ]

src/django_rbac/models.py

@@ -14,6 +14,7 @@ from django.db import models
 from django.db.models.query import Prefetch, Q
 from django.utils.translation import gettext
 from django.utils.translation import ugettext_lazy as _
+from model_utils.managers import QueryManager
 
 from . import backends, constants, managers, utils
 
@@ -186,19 +187,19 @@ class RoleAbstractBase(AbstractOrganizationalUnitScopedBase, AbstractBase):
 
     def add_child(self, child):
         RoleParenting = utils.get_role_parenting_model()
-        RoleParenting.objects.get_or_create(parent=self, child=child, direct=True)
+        RoleParenting.objects.soft_create(self, child)
 
     def remove_child(self, child):
         RoleParenting = utils.get_role_parenting_model()
-        RoleParenting.objects.filter(parent=self, child=child, direct=True).delete()
+        RoleParenting.objects.soft_delete(self, child)
 
     def add_parent(self, parent):
         RoleParenting = utils.get_role_parenting_model()
-        RoleParenting.objects.get_or_create(parent=parent, child=self, direct=True)
+        RoleParenting.objects.soft_create(parent, self)
 
     def remove_parent(self, parent):
         RoleParenting = utils.get_role_parenting_model()
-        RoleParenting.objects.filter(child=self, parent=parent, direct=True).delete()
+        RoleParenting.objects.soft_delete(parent, self)
 
     def parents(self, include_self=True, annotate=False):
         return self.__class__.objects.filter(pk=self.pk).parents(include_self=include_self, annotate=annotate)
@@ -211,8 +212,12 @@ class RoleAbstractBase(AbstractOrganizationalUnitScopedBase, AbstractBase):
     def all_members(self):
         User = get_user_model()
         prefetch = Prefetch('roles', queryset=self.__class__.objects.filter(pk=self.pk), to_attr='direct')
+
         return (
-            User.objects.filter(Q(roles=self) | Q(roles__parent_relation__parent=self))
+            User.objects.filter(
+                Q(roles=self)
+                | Q(roles__parent_relation__parent=self) & Q(roles__parent_relation__deleted__isnull=True)
+            )
             .distinct()
             .prefetch_related(prefetch)
         )
@@ -249,8 +254,11 @@ class RoleParentingAbstractBase(models.Model):
         on_delete=models.CASCADE,
     )
     direct = models.BooleanField(default=True, blank=True)
+    created = models.DateTimeField(verbose_name=_('Creation date'), auto_now_add=True)
+    deleted = models.DateTimeField(verbose_name=_('Deletion date'), null=True)
 
     objects = managers.RoleParentingManager()
+    alive = QueryManager(deleted__isnull=True)
 
     def natural_key(self):
         return [self.parent.natural_key(), self.child.natural_key(), self.direct]

src/django_rbac/signal_handlers.py

@@ -19,6 +19,11 @@ def role_parenting_post_delete(sender, instance, **kwargs):
     sender.objects.update_transitive_closure()
 
 
+def role_parenting_post_soft_delete(sender, instance, **kwargs):
+    '''Close the role parenting relation after instance soft-deletion'''
+    sender.objects.update_transitive_closure()
+
+
 def create_base_operations(app_config, verbosity=2, interactive=True, using=DEFAULT_DB_ALIAS, **kwargs):
     '''Create some basic operations, matching permissions from Django'''
     if not router.allow_migrate(using, models.Operation):

src/django_rbac/signals.py

@@ -0,0 +1,5 @@
+from django import dispatch
+
+# update role parenting transitive closure when role parenting is deleted
+post_soft_create = dispatch.Signal(providing_args=['instance'])
+post_soft_delete = dispatch.Signal(providing_args=['instance'])

tests_rbac/test_rbac.py

@@ -41,7 +41,7 @@ def test_role_parenting(db):
     assert Role.objects.count() == 10
     assert RoleParenting.objects.count() == 0
     for i in range(1, 3):
-        RoleParenting.objects.create(parent=roles[i - 1], child=roles[i])
+        RoleParenting.objects.soft_create(parent=roles[i - 1], child=roles[i])
     assert RoleParenting.objects.filter(direct=True).count() == 2
     assert RoleParenting.objects.filter(direct=False).count() == 1
     for i, role in enumerate(roles[:3]):
@@ -59,7 +59,7 @@ def test_role_parenting(db):
         assert role.parents().count() == i + 1
         assert role.children(False).count() == 3 - i - 1
         assert role.parents(False).count() == i
-    RoleParenting.objects.create(parent=roles[2], child=roles[3])
+    RoleParenting.objects.soft_create(parent=roles[2], child=roles[3])
     assert RoleParenting.objects.filter(direct=True).count() == 5
     assert RoleParenting.objects.filter(direct=False).count() == 10
     for i in range(6):
@@ -69,17 +69,79 @@ def test_role_parenting(db):
         assert role.parents().count() == i + 1
         assert role.children(False).count() == 6 - i - 1
         assert role.parents(False).count() == i
-    RoleParenting.objects.filter(parent=roles[2], child=roles[3], direct=True).delete()
+    RoleParenting.objects.soft_delete(roles[2], roles[3])
-    assert RoleParenting.objects.filter(direct=True).count() == 4
+    assert (
-    assert RoleParenting.objects.filter(direct=False).count() == 2
+        RoleParenting.objects.filter(
+            direct=True,
+            deleted__isnull=True,
+        ).count()
+        == 4
+    )
+    assert (
+        RoleParenting.objects.filter(
+            direct=False,
+            deleted__isnull=True,
+        ).count()
+        == 2
+    )
     # test that it works with cycles
-    RoleParenting.objects.create(parent=roles[2], child=roles[3])
+    RoleParenting.objects.soft_create(parent=roles[2], child=roles[3])
-    RoleParenting.objects.create(parent=roles[5], child=roles[0])
+    RoleParenting.objects.soft_create(parent=roles[5], child=roles[0])
     for role in roles[:6]:
         assert role.children().count() == 6
         assert role.parents().count() == 6
 
 
+def test_role_parenting_soft_delete_children(db):
+    OrganizationalUnit = utils.get_ou_model()
+    Role = utils.get_role_model()
+    RoleParenting = utils.get_role_parenting_model()
+
+    ou = OrganizationalUnit.objects.create(name='ou')
+    roles = []
+    for i in range(10):
+        roles.append(Role.objects.create(name='r%d' % i, ou=ou))
+    assert not len(RoleParenting.objects.all())
+
+    rps = []
+    for i in range(5):
+        rps.append(RoleParenting.objects.soft_create(parent=roles[9 - i], child=roles[i]))
+    assert len(RoleParenting.objects.all()) == 5
+    for i in range(5):
+        roles[9 - i].remove_child(roles[i])
+        assert len(RoleParenting.objects.all()) == 5
+        assert len(RoleParenting.objects.filter(deleted__isnull=True).all()) == 4 - i
+    for i in range(5):
+        roles[9 - i].add_child(roles[i])
+        assert len(RoleParenting.objects.all()) == 5
+        assert len(RoleParenting.objects.filter(deleted__isnull=True).all()) == i + 1
+
+
+def test_role_parenting_soft_delete_parents(db):
+    OrganizationalUnit = utils.get_ou_model()
+    Role = utils.get_role_model()
+    RoleParenting = utils.get_role_parenting_model()
+
+    ou = OrganizationalUnit.objects.create(name='ou')
+    roles = []
+    for i in range(10):
+        roles.append(Role.objects.create(name='r%d' % i, ou=ou))
+    assert not len(RoleParenting.objects.all())
+
+    rps = []
+    for i in range(5):
+        rps.append(RoleParenting.objects.soft_create(child=roles[9 - i], parent=roles[i]))
+    assert len(RoleParenting.objects.all()) == 5
+    for i in range(5):
+        roles[9 - i].remove_parent(roles[i])
+        assert len(RoleParenting.objects.all()) == 5
+        assert len(RoleParenting.objects.filter(deleted__isnull=True).all()) == 4 - i
+    for i in range(5):
+        roles[9 - i].add_parent(roles[i])
+        assert len(RoleParenting.objects.all()) == 5
+        assert len(RoleParenting.objects.filter(deleted__isnull=True).all()) == i + 1
+
+
 SIZE = 1000
 SPAN = 50
 
@@ -255,7 +317,9 @@ def test_random_role_parenting(db):
                 break
             z = new_z
         real = np.zeros((c, c), dtype=bool)
-        for parent_id, child_id in RoleParenting.objects.values_list('parent_id', 'child_id'):
+        for parent_id, child_id in RoleParenting.objects.filter(deleted__isnull=True).values_list(
+            'parent_id', 'child_id'
+        ):
             real[parent_id][child_id] = True
         assert np.array_equal(real, z & ~one)