From a13d1c9c05c370926125a28e33aa999fa0b82046 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Mon, 31 Jan 2022 20:52:46 +0100 Subject: [PATCH 6/6] misc: implement soft-delete on RoleParenting (#57500) --- .../0026_add_roleparenting_soft_delete.py | 27 +++++++ src/authentic2/custom_user/models.py | 5 +- .../management/commands/check-and-repair.py | 8 +- src/authentic2/manager/resources.py | 10 +-- src/authentic2/manager/role_views.py | 4 +- src/authentic2/manager/tables.py | 4 +- src/authentic2/manager/user_views.py | 2 +- src/django_rbac/apps.py | 10 ++- src/django_rbac/managers.py | 65 ++++++++++++--- .../0008_add_roleparenting_soft_delete.py | 27 +++++++ src/django_rbac/models.py | 18 +++-- src/django_rbac/signal_handlers.py | 5 ++ src/django_rbac/signals.py | 7 ++ tests_rbac/test_rbac.py | 80 +++++++++++++++++-- 14 files changed, 231 insertions(+), 41 deletions(-) create mode 100644 src/authentic2/a2_rbac/migrations/0026_add_roleparenting_soft_delete.py create mode 100644 src/django_rbac/migrations/0008_add_roleparenting_soft_delete.py create mode 100644 src/django_rbac/signals.py diff --git a/src/authentic2/a2_rbac/migrations/0026_add_roleparenting_soft_delete.py b/src/authentic2/a2_rbac/migrations/0026_add_roleparenting_soft_delete.py new file mode 100644 index 00000000..fcf5c55d --- /dev/null +++ b/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'), + ), + ] diff --git a/src/authentic2/custom_user/models.py b/src/authentic2/custom_user/models.py index 366e42af..5206922a 100644 --- a/src/authentic2/custom_user/models.py +++ b/src/authentic2/custom_user/models.py @@ -220,7 +220,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') diff --git a/src/authentic2/management/commands/check-and-repair.py b/src/authentic2/management/commands/check-and-repair.py index fbe4e65f..cc36a0c3 100644 --- a/src/authentic2/management/commands/check-and-repair.py +++ b/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=' ') diff --git a/src/authentic2/manager/resources.py b/src/authentic2/manager/resources.py index 3885a4bd..f3f543a9 100644 --- a/src/authentic2/manager/resources.py +++ b/src/authentic2/manager/resources.py @@ -36,12 +36,10 @@ class UserResource(ModelResource): roles = Field() def dehydrate_roles(self, instance): - result = set() - for role in instance.roles.all(): - result.add(role) - for pr in role.parent_relation.all(): - result.add(pr.parent) - return ', '.join(str(x) for x in result) + roles = {role for role in instance.roles.all()} + # optimization as parent_relation is prefetched, filter deleted__isnull=True using python + parents = {rp.parent for role in roles for rp in role.parent_relation.all() if not rp.deleted} + return ', '.join(str(x) for x in roles | parents) class Meta: model = User diff --git a/src/authentic2/manager/role_views.py b/src/authentic2/manager/role_views.py index 28e37d5d..0e88c45c 100644 --- a/src/authentic2/manager/role_views.py +++ b/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 diff --git a/src/authentic2/manager/tables.py b/src/authentic2/manager/tables.py index 0e919623..d1017fc7 100644 --- a/src/authentic2/manager/tables.py +++ b/src/authentic2/manager/tables.py @@ -202,8 +202,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' - ' forloop.last %}, {% endif %}{% endfor %}{% endif %}', + '{% if not record.member %}{% for rel in record.child_relation.all %}' + '{{ rel.child }} {% if not forloop.last %}, {% endif %}{% endfor %}{% endif %}', verbose_name=_('Inherited from'), orderable=False, ) diff --git a/src/authentic2/manager/user_views.py b/src/authentic2/manager/user_views.py index 96155556..14fb2923 100644 --- a/src/authentic2/manager/user_views.py +++ b/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( diff --git a/src/django_rbac/apps.py b/src/django_rbac/apps.py index 511b9da6..cde0be3f 100644 --- a/src/django_rbac/apps.py +++ b/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 diff --git a/src/django_rbac/managers.py b/src/django_rbac/managers.py index 6dfe88fc..7aaf85f8 100644 --- a/src/django_rbac/managers.py +++ b/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')) - old_indirects = set(self.filter(direct=False).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, 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 = '''DELETE FROM "%s" AS relation \ -USING (VALUES %s) AS dead(parent_id, child_id) \ + sql = '''UPDATE "%s" AS relation \ +SET deleted = now()\ +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, ) diff --git a/src/django_rbac/migrations/0008_add_roleparenting_soft_delete.py b/src/django_rbac/migrations/0008_add_roleparenting_soft_delete.py new file mode 100644 index 00000000..2474d2e6 --- /dev/null +++ b/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'), + ), + ] diff --git a/src/django_rbac/models.py b/src/django_rbac/models.py index e3228c24..18a77c0a 100644 --- a/src/django_rbac/models.py +++ b/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] diff --git a/src/django_rbac/signal_handlers.py b/src/django_rbac/signal_handlers.py index 24dc25b7..15facf21 100644 --- a/src/django_rbac/signal_handlers.py +++ b/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): diff --git a/src/django_rbac/signals.py b/src/django_rbac/signals.py new file mode 100644 index 00000000..01aca919 --- /dev/null +++ b/src/django_rbac/signals.py @@ -0,0 +1,7 @@ +from django import dispatch + +from . import signal_handlers, utils + +# 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']) diff --git a/tests_rbac/test_rbac.py b/tests_rbac/test_rbac.py index 44fb189e..bf91ffe5 100644 --- a/tests_rbac/test_rbac.py +++ b/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() - assert RoleParenting.objects.filter(direct=True).count() == 4 - assert RoleParenting.objects.filter(direct=False).count() == 2 + RoleParenting.objects.soft_delete(roles[2], roles[3]) + assert ( + 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.create(parent=roles[5], child=roles[0]) + RoleParenting.objects.soft_create(parent=roles[2], child=roles[3]) + 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) -- 2.34.1