From 2c2592c214ac6584fc034846a5c5c074f85fc0c6 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Tue, 27 Aug 2024 10:53:42 -0400 Subject: [PATCH 01/12] refactor assign_perms() to support bulk assignment --- kpi/mixins/object_permission.py | 228 +++++++++++------- .../v2/asset_permission_assignment.py | 19 +- 2 files changed, 149 insertions(+), 98 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 61d05eba0a..bcc78b4370 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -445,115 +445,157 @@ def get_all_implied_perms(cls, for_instance=None): def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, skip_kc=False, partial_perms=None): r""" - Assign `user_obj` the given `perm` on this object, or break - inheritance from a parent object. By default, recalculate - descendant objects' permissions and apply any applicable KC - permissions. - :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` - :param perm: str. The `codename` of the `Permission` - :param deny: bool. When `True`, break inheritance from parent object - :param defer_recalc: bool. When `True`, skip recalculating - descendants - :param skip_kc: bool. When `True`, skip assignment of applicable KC - permissions - :param partial_perms: dict. Filters used to narrow down query for - partial permissions - """ - app_label, codename = perm_parse(perm, self) + Assign one or more of the given permissions (`perm`) to the + `user_obj`, or break inheritance from a parent object. By default, + recalculate descendant objects' permissions and apply any applicable + KC permissions. + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` + :param perm: str or list. The `codename(s)` of the `Permission(s)` + :param deny: bool. When `True`, break inheritance from parent object + :param defer_recalc: bool. When `True`, skip recalculating + descendants + :param skip_kc: bool. When `True`, skip assignment of applicable KC + permissions + :param partial_perms: dict. Filters used to narrow down query for + partial permissions + """ + # Ensure `perm` is a list to handle multiple permissions + if isinstance(perm, str): + perm = [perm] + assignable_permissions = self.get_assignable_permissions() - if codename not in assignable_permissions: - # Some permissions are calculated and not stored in the database - raise serializers.ValidationError({ - 'permission': f'{codename} cannot be assigned explicitly to {self}' - }) - is_anonymous = is_user_anonymous(user_obj) user_obj = get_database_user(user_obj) - if is_anonymous: - # Is an anonymous user allowed to have this permission? - fq_permission = f'{app_label}.{codename}' - if ( - not deny - and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS - ): - raise serializers.ValidationError({ - 'permission': f'Anonymous users cannot be granted the permission {codename}.' - }) - perm_model = Permission.objects.get( - content_type__app_label=app_label, - codename=codename - ) - existing_perms = self.permissions.filter(user=user_obj) - identical_existing_perm = existing_perms.filter( - inherited=False, - permission_id=perm_model.pk, - deny=deny, - ) - if identical_existing_perm.exists(): - # We need to always update partial permissions because - # they may have changed even if `perm` is the same. - self._update_partial_permissions(user_obj, perm, - partial_perms=partial_perms) - # The user already has this permission directly applied - return identical_existing_perm.first() - - # Remove any explicitly-defined contradictory grants or denials - contradictory_filters = models.Q( - user=user_obj, - permission_id=perm_model.pk, - deny=not deny, - inherited=False - ) - if not deny and perm in self.CONTRADICTORY_PERMISSIONS.keys(): - contradictory_filters |= models.Q( + + if is_user_anonymous(user_obj): + for p in perm: + # Is an anonymous user allowed to have this permission? + fq_permission = ( + f'{perm_parse(p, self)[0]}.{perm_parse(p, self)[1]}' + ) + if ( + not deny + and fq_permission + not in settings.ALLOWED_ANONYMOUS_PERMISSIONS + ): + raise serializers.ValidationError( + { + 'permission': f'Anonymous users cannot be granted the permission {p}.' + } + ) + + new_permissions = [] + implied_permissions = set() + + for p in perm: + app_label, codename = perm_parse(p, self) + if codename not in assignable_permissions: + # Some permissions are calculated and not stored in the database + raise serializers.ValidationError( + { + 'permission': f'{codename} cannot be assigned explicitly to {self}' + } + ) + + perm_model = Permission.objects.get( + content_type__app_label=app_label, codename=codename + ) + existing_perms = self.permissions.filter(user=user_obj) + identical_existing_perm = existing_perms.filter( user=user_obj, - permission__codename__in=self.CONTRADICTORY_PERMISSIONS.get(perm), + inherited=False, + permission_id=perm_model.pk, + deny=deny, ) - contradictory_perms = existing_perms.filter(contradictory_filters) - contradictory_codenames = list(contradictory_perms.values_list( - 'permission__codename', flat=True)) - - contradictory_perms.delete() - # Check if any KC permissions should be removed as well - if deny and not skip_kc: - remove_applicable_kc_permissions( - self, user_obj, contradictory_codenames) - # Create the new permission - new_permission = ObjectPermission.objects.create( - asset=self, - user=user_obj, - permission_id=perm_model.pk, - deny=deny, - inherited=False - ) - # Assign any applicable KC permissions - if not deny and not skip_kc: - assign_applicable_kc_permissions(self, user_obj, codename) - # Resolve implied permissions, e.g. granting change implies granting - # view - implied_perms = self.get_implied_perms( - codename, reverse=deny, for_instance=self - ).intersection(assignable_permissions) - for implied_perm in implied_perms: - self.assign_perm( - user_obj, implied_perm, deny=deny, defer_recalc=True) + if identical_existing_perm.exists(): + # We need to always update partial permissions because + # they may have changed even if `perm` is the same. + self._update_partial_permissions( + user_obj, p, partial_perms=partial_perms + ) + # The user already has this permission directly applied + continue + + # Remove any explicitly-defined contradictory grants or denials + contradictory_filters = models.Q( + user=user_obj, + permission_id=perm_model.pk, + deny=not deny, + inherited=False, + ) + if not deny and p in self.CONTRADICTORY_PERMISSIONS.keys(): + contradictory_filters |= models.Q( + user=user_obj, + permission__codename__in=self.CONTRADICTORY_PERMISSIONS.get( + p + ), + ) + contradictory_perms = existing_perms.filter(contradictory_filters) + contradictory_codenames = list( + contradictory_perms.values_list( + 'permission__codename', flat=True + ) + ) + + contradictory_perms.delete() + # Check if any KC permissions should be removed as well + if deny and not skip_kc: + remove_applicable_kc_permissions( + self, user_obj, contradictory_codenames + ) + # Create the new permission and add to the list of permissions + new_permissions.append( + ObjectPermission( + asset=self, + user=user_obj, + permission_id=perm_model.pk, + deny=deny, + inherited=False, + ) + ) + # Assign any applicable KC permissions + if not deny and not skip_kc: + assign_applicable_kc_permissions(self, user_obj, codename) + # Resolve implied permissions, e.g. granting change implies granting + # view + implied_permissions.update( + self.get_implied_perms( + codename, reverse=deny, for_instance=self + ).intersection(assignable_permissions) + ) + + if new_permissions: + ObjectPermission.objects.bulk_create(new_permissions) + if len(new_permissions) == 1: + new_permissions = new_permissions[0] + if implied_permissions: + if implied_permissions: + self.assign_perm( + user_obj=user_obj, + perm=list(implied_permissions), + deny=deny, + defer_recalc=True, + skip_kc=skip_kc, + partial_perms=partial_perms, + ) # We might have been called by ourselves to assign a related # permission. In that case, don't recalculate here. if defer_recalc: - return new_permission + return new_permissions - self._update_partial_permissions( - user_obj, perm, partial_perms=partial_perms - ) + for p in perm: + self._update_partial_permissions( + user_obj, p, partial_perms=partial_perms + ) post_assign_perm.send( sender=self.__class__, instance=self, user=user_obj, - codename=codename, + codename=', '.join(perm), ) # Recalculate all descendants self.recalculate_descendants_perms() - return new_permission + return new_permissions def get_perms(self, user_obj: settings.AUTH_USER_MODEL) -> list[str]: """ diff --git a/kpi/serializers/v2/asset_permission_assignment.py b/kpi/serializers/v2/asset_permission_assignment.py index 4ccee9694a..eb7d8f8d64 100644 --- a/kpi/serializers/v2/asset_permission_assignment.py +++ b/kpi/serializers/v2/asset_permission_assignment.py @@ -352,20 +352,29 @@ def create(self, validated_data): removal.permission_codename, ) + user_permissions = defaultdict(list) + user_partial_perms = defaultdict(dict) + # Perform the new assignments for addition in incoming_assignments.difference(existing_assignments): if asset.owner_id == addition.user_pk: raise serializers.ValidationError( {'user': t(ASSIGN_OWNER_ERROR_MESSAGE)} ) + + # Group permissions by user + user_permissions[addition.user_pk].append(addition.permission_codename) + if addition.partial_permissions_json: partial_perms = json.loads(addition.partial_permissions_json) - else: - partial_perms = None + user_partial_perms[addition.user_pk].update(partial_perms) + + # Assign the permissions for each user + for user_pk, permissions in user_permissions.items(): asset.assign_perm( - user_obj=user_pk_to_obj_cache[addition.user_pk], - perm=addition.permission_codename, - partial_perms=partial_perms, + user_obj=user_pk_to_obj_cache[user_pk], + perm=permissions, + partial_perms=user_partial_perms[user_pk] if user_partial_perms[user_pk] else None ) # Return nothing, in a nice way, because the view is responsible for From c3b10c88a08ce2be20aa8d098458e1a03ef53385 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 28 Aug 2024 18:10:34 -0400 Subject: [PATCH 02/12] refactor remove_perms() to support bulk assignment --- kpi/mixins/object_permission.py | 132 ++++++++++-------- .../v2/asset_permission_assignment.py | 11 +- 2 files changed, 84 insertions(+), 59 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index bcc78b4370..5a67c92594 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -693,71 +693,91 @@ def has_perms(self, user_obj: User, perms: list, all_: bool = True) -> bool: @kc_transaction_atomic def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): """ - Revoke the given `perm` on this object from `user_obj`. By default, - recalculate descendant objects' permissions and remove any - applicable KC permissions. May delete granted permissions or add - deny permissions as appropriate: - Current access Action - ============== ====== - None None - Direct Remove direct permission - Inherited Add deny permission - Direct & Inherited Remove direct permission; add deny permission - :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` - :param perm str: The `codename` of the `Permission` - :param defer_recalc bool: When `True`, skip recalculating - descendants - :param skip_kc bool: When `True`, skip assignment of applicable KC - permissions + Revoke the given `perm` or list of permissions on this object from + `user_obj`. By default, recalculate descendant objects' + permissions and remove any applicable KC permissions. May delete + granted permissions or add deny permissions as appropriate: + Current access Action + ============== ====== + None None + Direct Remove direct permission + Inherited Add deny permission + Direct & Inherited Remove direct permission; add deny permission + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` + :param perm str or list: The `codename(s)` of the `Permission(s)` + :param defer_recalc bool: When `True`, skip recalculating + descendants + :param skip_kc bool: When `True`, skip assignment of applicable KC + permissions """ + # Ensure `perm` is a list to handle multiple permissions + if isinstance(perm, str): + perm = [perm] + user_obj = get_database_user(user_obj) - app_label, codename = perm_parse(perm, self) - # Get all assignable permissions, regardless of asset type. That way, - # we can allow invalid permissions to be removed - if codename not in self.get_assignable_permissions(ignore_type=True): - # Some permissions are calculated and not stored in the database - raise serializers.ValidationError({ - 'permission': f'{codename} cannot be removed explicitly.' - }) - all_permissions = self.permissions.filter( - user=user_obj, - permission__codename=codename, - deny=False - ) - direct_permissions = all_permissions.filter(inherited=False) - inherited_permissions = all_permissions.filter(inherited=True) - # Resolve implied permissions, e.g. revoking view implies revoking - # change - implied_perms = self.get_implied_perms( - codename, reverse=True, for_instance=self - ) - for implied_perm in implied_perms: - self.remove_perm( - user_obj, implied_perm, defer_recalc=True) - # Delete directly assigned permissions, if any - direct_permissions.delete() - if inherited_permissions.exists(): - # Delete inherited permissions - inherited_permissions.delete() - # Add a deny permission to block future inheritance - self.assign_perm(user_obj, perm, deny=True, defer_recalc=True) - # Remove any applicable KC permissions - if not skip_kc: - remove_applicable_kc_permissions(self, user_obj, codename) + permissions_to_delete = self.permissions.none() + all_perms_to_remove = perm.copy() + + while all_perms_to_remove: + current_perm = all_perms_to_remove.pop(0) + app_label, codename = perm_parse(current_perm, self) + # Get all assignable permissions, regardless of asset type. That way, + # we can allow invalid permissions to be removed + if codename not in self.get_assignable_permissions( + ignore_type=True + ): + # Some permissions are calculated and not stored in the database + raise serializers.ValidationError( + {'permission': f'{codename} cannot be removed explicitly.'} + ) + all_permissions = self.permissions.filter( + user=user_obj, permission__codename=codename, deny=False + ) + direct_permissions = all_permissions.filter(inherited=False) + inherited_permissions = all_permissions.filter(inherited=True) + # Resolve implied permissions, e.g. revoking view implies revoking + # change + implied_perms = self.get_implied_perms( + codename, reverse=True, for_instance=self + ) + all_perms_to_remove.extend(implied_perms) + # Add permissions to the list for deletion + permissions_to_delete = permissions_to_delete | direct_permissions + if inherited_permissions.exists(): + permissions_to_delete = ( + permissions_to_delete | inherited_permissions + ) + # Add a deny permission to block future inheritance + self.assign_perm( + user_obj, current_perm, deny=True, defer_recalc=True + ) + # Remove any applicable KC permissions + if not skip_kc: + remove_applicable_kc_permissions(self, user_obj, codename) + + # Perform bulk delete of all permissions at once + if permissions_to_delete.exists(): + permissions_to_delete.delete() # We might have been called by ourself to assign a related # permission. In that case, don't recalculate here. if defer_recalc: return - self._update_partial_permissions(user_obj, perm, remove=True) + # Ensure partial permissions are removed + if 'partial_submissions' in perm: + self._update_partial_permissions( + user_obj, 'partial_submissions', remove=True + ) - post_remove_perm.send( - sender=self.__class__, - instance=self, - user=user_obj, - codename=codename, - ) + for p in perm: + app_label, codename = perm_parse(p, self) + post_remove_perm.send( + sender=self.__class__, + instance=self, + user=user_obj, + codename=codename, + ) # Recalculate all descendants self.recalculate_descendants_perms() diff --git a/kpi/serializers/v2/asset_permission_assignment.py b/kpi/serializers/v2/asset_permission_assignment.py index eb7d8f8d64..00c2cdbb41 100644 --- a/kpi/serializers/v2/asset_permission_assignment.py +++ b/kpi/serializers/v2/asset_permission_assignment.py @@ -345,11 +345,16 @@ def create(self, validated_data): asset, user_pk_to_obj_cache ) - # Perform the removals + # Identify the removals and group by user + user_permissions_to_remove = defaultdict(list) for removal in existing_assignments.difference(incoming_assignments): + user_permissions_to_remove[removal.user_pk].append(removal.permission_codename) + + # Perform the removals for each user + for user_pk, permissions in user_permissions_to_remove.items(): asset.remove_perm( - user_pk_to_obj_cache[removal.user_pk], - removal.permission_codename, + user_obj=user_pk_to_obj_cache[user_pk], + perm=permissions ) user_permissions = defaultdict(list) From 9c4591bd1edef17db26b5beca32f723cb7fa5e77 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Thu, 29 Aug 2024 11:04:03 -0400 Subject: [PATCH 03/12] add tests for assign_perm() and remove_perm() bulk refactor --- .../test_api_asset_permission_assignment.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/kpi/tests/api/v2/test_api_asset_permission_assignment.py b/kpi/tests/api/v2/test_api_asset_permission_assignment.py index 3477c8b4ac..3e7864ba95 100644 --- a/kpi/tests/api/v2/test_api_asset_permission_assignment.py +++ b/kpi/tests/api/v2/test_api_asset_permission_assignment.py @@ -2,6 +2,8 @@ from copy import deepcopy from django.contrib.auth.models import Permission +from django.db import connection +from django.test.utils import CaptureQueriesContext from django.urls import reverse from rest_framework import status @@ -938,3 +940,48 @@ def test_partial_permission_invalid(self): ) response = self.client.post(bulk_endpoint, assignments, format='json') assert response.status_code == status.HTTP_400_BAD_REQUEST + + def test_bulk_assign_perm_reduces_queries(self): + permissions = [PERM_VIEW_ASSET, PERM_CHANGE_ASSET] + + # Assign permissions one by one + with CaptureQueriesContext(connection) as old_context: + for user in [self.someuser, self.anotheruser]: + for perm in permissions: + self.asset.assign_perm(user, perm) + old_method_query_count = len(old_context.captured_queries) + + # Bulk assign permissions + with CaptureQueriesContext(connection) as new_context: + for user in [self.someuser, self.anotheruser]: + self.asset.assign_perm(user, permissions) + new_method_query_count = len(new_context.captured_queries) + + # Ensure the new method reduces the number of queries + self.assertTrue(new_method_query_count < old_method_query_count) + + def test_bulk_remove_perm_reduces_queries(self): + permissions = [ + PERM_VIEW_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + PERM_DELETE_SUBMISSIONS, + ] + + for user in [self.someuser, self.anotheruser]: + self.asset.assign_perm(user, permissions) + + # Remove permissions one by one + with CaptureQueriesContext(connection) as old_context: + for user in [self.someuser, self.anotheruser]: + for perm in permissions: + self.asset.remove_perm(user, perm) + old_method_query_count = len(old_context.captured_queries) + + # Bulk remove permissions + with CaptureQueriesContext(connection) as new_context: + for user in [self.someuser, self.anotheruser]: + self.asset.remove_perm(user, permissions) + new_method_query_count = len(new_context.captured_queries) + + # Ensure the new method reduces the number of queries + self.assertTrue(new_method_query_count < old_method_query_count) From 216fa68758eb7b7e070f08fbaa0c2b1d84a97d13 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Thu, 29 Aug 2024 13:52:05 -0400 Subject: [PATCH 04/12] styling fixes --- kpi/mixins/object_permission.py | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 5a67c92594..00aafccabe 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -469,13 +469,12 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, if is_user_anonymous(user_obj): for p in perm: # Is an anonymous user allowed to have this permission? - fq_permission = ( - f'{perm_parse(p, self)[0]}.{perm_parse(p, self)[1]}' - ) + parsed_perm = perm_parse(p, self) + fq_permission = f'{parsed_perm[0]}.{parsed_perm[1]}' if ( - not deny - and fq_permission - not in settings.ALLOWED_ANONYMOUS_PERMISSIONS + # fmt: off + not deny and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS + # fmt: on ): raise serializers.ValidationError( { @@ -568,15 +567,14 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, if len(new_permissions) == 1: new_permissions = new_permissions[0] if implied_permissions: - if implied_permissions: - self.assign_perm( - user_obj=user_obj, - perm=list(implied_permissions), - deny=deny, - defer_recalc=True, - skip_kc=skip_kc, - partial_perms=partial_perms, - ) + self.assign_perm( + user_obj=user_obj, + perm=list(implied_permissions), + deny=deny, + defer_recalc=True, + skip_kc=skip_kc, + partial_perms=partial_perms, + ) # We might have been called by ourselves to assign a related # permission. In that case, don't recalculate here. if defer_recalc: From adeda44b20741c0c1bb8f599f289d37a536bfbe5 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Thu, 29 Aug 2024 14:31:33 -0400 Subject: [PATCH 05/12] refactor tests using self.assertNumQueries --- .../test_api_asset_permission_assignment.py | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/kpi/tests/api/v2/test_api_asset_permission_assignment.py b/kpi/tests/api/v2/test_api_asset_permission_assignment.py index 3e7864ba95..3b9d853cef 100644 --- a/kpi/tests/api/v2/test_api_asset_permission_assignment.py +++ b/kpi/tests/api/v2/test_api_asset_permission_assignment.py @@ -2,8 +2,6 @@ from copy import deepcopy from django.contrib.auth.models import Permission -from django.db import connection -from django.test.utils import CaptureQueriesContext from django.urls import reverse from rest_framework import status @@ -945,20 +943,15 @@ def test_bulk_assign_perm_reduces_queries(self): permissions = [PERM_VIEW_ASSET, PERM_CHANGE_ASSET] # Assign permissions one by one - with CaptureQueriesContext(connection) as old_context: + with self.assertNumQueries(36): for user in [self.someuser, self.anotheruser]: for perm in permissions: self.asset.assign_perm(user, perm) - old_method_query_count = len(old_context.captured_queries) # Bulk assign permissions - with CaptureQueriesContext(connection) as new_context: + with self.assertNumQueries(12): for user in [self.someuser, self.anotheruser]: self.asset.assign_perm(user, permissions) - new_method_query_count = len(new_context.captured_queries) - - # Ensure the new method reduces the number of queries - self.assertTrue(new_method_query_count < old_method_query_count) def test_bulk_remove_perm_reduces_queries(self): permissions = [ @@ -971,17 +964,12 @@ def test_bulk_remove_perm_reduces_queries(self): self.asset.assign_perm(user, permissions) # Remove permissions one by one - with CaptureQueriesContext(connection) as old_context: + with self.assertNumQueries(56): for user in [self.someuser, self.anotheruser]: for perm in permissions: self.asset.remove_perm(user, perm) - old_method_query_count = len(old_context.captured_queries) # Bulk remove permissions - with CaptureQueriesContext(connection) as new_context: + with self.assertNumQueries(42): for user in [self.someuser, self.anotheruser]: self.asset.remove_perm(user, permissions) - new_method_query_count = len(new_context.captured_queries) - - # Ensure the new method reduces the number of queries - self.assertTrue(new_method_query_count < old_method_query_count) From f4e22d03bf72381588c063963c76c18e7a38261d Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 18 Sep 2024 08:57:36 -0400 Subject: [PATCH 06/12] add tests for post_assign_asset_perm in signals.py --- kpi/tests/test_signals.py | 61 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 kpi/tests/test_signals.py diff --git a/kpi/tests/test_signals.py b/kpi/tests/test_signals.py new file mode 100644 index 0000000000..ca36832c21 --- /dev/null +++ b/kpi/tests/test_signals.py @@ -0,0 +1,61 @@ +from unittest.mock import MagicMock + +from django.contrib.auth.models import AnonymousUser, User +from django.test import TestCase + +from kpi.models import Asset +from kpi.signals import post_assign_asset_perm + + +class AssetPermissionSignalTest(TestCase): + def setUp(self): + self.asset = Asset.objects.create() + self.asset.connect_deployment(backend='mock') + self.asset.deployment.set_enketo_open_rosa_server = MagicMock() + self.anonymous_user = AnonymousUser() + self.user = User() + + def test_enketo_server_updated_for_anonymous_add_submissions(self): + """ + Test that the Enketo server URL is updated correctly when an anonymous + user is granted the 'add_submissions' permission. The Enketo URL should + be updated to reflect anonymous access by setting `require_auth=False` + """ + post_assign_asset_perm( + sender=Asset, + instance=self.asset, + user=self.anonymous_user, + codenames='add_submissions', + ) + + self.asset.deployment.set_enketo_open_rosa_server.assert_called_with( + require_auth=False + ) + + def test_enketo_server_not_updated_for_other_permissions(self): + """ + Test that the Enketo server URL is not updated when an authenticated + user is granted any type of permission. + """ + post_assign_asset_perm( + sender=Asset, + instance=self.asset, + user=self.user, + codenames='view_asset', + ) + + self.asset.deployment.set_enketo_open_rosa_server.assert_not_called() + + def test_enketo_server_not_updated_for_multiple_permissions(self): + """ + Test that the Enketo server URL is not updated when a user has multiple + permissions assigned. + """ + post_assign_asset_perm( + sender=Asset, + instance=self.asset, + user=self.user, + codenames=['view_asset', 'change_asset', 'delete_submissions'], + ) + + self.asset.deployment.set_enketo_open_rosa_server.assert_not_called() From affc2618e6af73d326b5889be0d8afe75740a2d8 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Fri, 20 Sep 2024 15:42:36 -0400 Subject: [PATCH 07/12] add back assign_perm() and remove_perm(), rename perm --> perms, refactor post_assign_asset_perm to take in lists of perms, add more tests --- kpi/mixins/object_permission.py | 272 +++++++++++++++--- .../v2/asset_permission_assignment.py | 8 +- kpi/signals.py | 23 +- .../test_api_asset_permission_assignment.py | 38 ++- 4 files changed, 282 insertions(+), 59 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 00aafccabe..3f2bc2697d 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -3,7 +3,7 @@ import copy from collections import defaultdict -from typing import Optional +from typing import Dict, List, Optional, Union from django.conf import settings from django.contrib.auth.models import AnonymousUser, Permission @@ -19,6 +19,7 @@ ASSET_TYPES_WITH_CHILDREN, ASSET_TYPE_SURVEY, PERM_FROM_KC_ONLY, + PERM_PARTIAL_SUBMISSIONS, PREFIX_PARTIAL_PERMS, ) from kpi.deployment_backends.kc_access.utils import ( @@ -445,36 +446,153 @@ def get_all_implied_perms(cls, for_instance=None): def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, skip_kc=False, partial_perms=None): r""" - Assign one or more of the given permissions (`perm`) to the - `user_obj`, or break inheritance from a parent object. By default, - recalculate descendant objects' permissions and apply any applicable - KC permissions. + Assign `user_obj` the given `perm` on this object, or break + inheritance from a parent object. By default, recalculate + descendant objects' permissions and apply any applicable KC + permissions. + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` + :param perm: str. The `codename` of the `Permission` + :param deny: bool. When `True`, break inheritance from parent object + :param defer_recalc: bool. When `True`, skip recalculating + descendants + :param skip_kc: bool. When `True`, skip assignment of applicable KC + permissions + :param partial_perms: dict. Filters used to narrow down query for + partial permissions + """ + app_label, codename = perm_parse(perm, self) + assignable_permissions = self.get_assignable_permissions() + if codename not in assignable_permissions: + # Some permissions are calculated and not stored in the database + raise serializers.ValidationError({ + 'permission': f'{codename} cannot be assigned explicitly to {self}' + }) + is_anonymous = is_user_anonymous(user_obj) + user_obj = get_database_user(user_obj) + if is_anonymous: + # Is an anonymous user allowed to have this permission? + fq_permission = f'{app_label}.{codename}' + if ( + not deny + and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS + ): + raise serializers.ValidationError({ + 'permission': f'Anonymous users cannot be granted the permission {codename}.' + }) + perm_model = Permission.objects.get( + content_type__app_label=app_label, + codename=codename + ) + existing_perms = self.permissions.filter(user=user_obj) + identical_existing_perm = existing_perms.filter( + inherited=False, + permission_id=perm_model.pk, + deny=deny, + ) + if identical_existing_perm.exists(): + # We need to always update partial permissions because + # they may have changed even if `perm` is the same. + self._update_partial_permissions(user_obj, perm, + partial_perms=partial_perms) + # The user already has this permission directly applied + return identical_existing_perm.first() + + # Remove any explicitly-defined contradictory grants or denials + contradictory_filters = models.Q( + user=user_obj, + permission_id=perm_model.pk, + deny=not deny, + inherited=False + ) + if not deny and perm in self.CONTRADICTORY_PERMISSIONS.keys(): + contradictory_filters |= models.Q( + user=user_obj, + permission__codename__in=self.CONTRADICTORY_PERMISSIONS.get(perm), + ) + contradictory_perms = existing_perms.filter(contradictory_filters) + contradictory_codenames = list(contradictory_perms.values_list( + 'permission__codename', flat=True)) + + contradictory_perms.delete() + # Check if any KC permissions should be removed as well + if deny and not skip_kc: + remove_applicable_kc_permissions( + self, user_obj, contradictory_codenames) + # Create the new permission + new_permission = ObjectPermission.objects.create( + asset=self, + user=user_obj, + permission_id=perm_model.pk, + deny=deny, + inherited=False + ) + # Assign any applicable KC permissions + if not deny and not skip_kc: + assign_applicable_kc_permissions(self, user_obj, codename) + # Resolve implied permissions, e.g. granting change implies granting + # view + implied_perms = self.get_implied_perms( + codename, reverse=deny, for_instance=self + ).intersection(assignable_permissions) + for implied_perm in implied_perms: + self.assign_perm( + user_obj, implied_perm, deny=deny, defer_recalc=True) + # We might have been called by ourselves to assign a related + # permission. In that case, don't recalculate here. + if defer_recalc: + return new_permission + + self._update_partial_permissions( + user_obj, perm, partial_perms=partial_perms + ) + post_assign_perm.send( + sender=self.__class__, + instance=self, + user=user_obj, + codenames=codename, + ) + + # Recalculate all descendants + self.recalculate_descendants_perms() + return new_permission + + @transaction.atomic + @kc_transaction_atomic + def assign_perms( + self, + user_obj: 'User', + perms: list[str], + deny: bool = False, + defer_recalc: bool = False, + skip_kc: bool = False, + partial_perms: Optional[Dict] = None, + ) -> Union[List['ObjectPermission'], None]: + r""" + Assign `user_obj` a list of permissions (`perms`) on this object, + or break inheritance from a parent object. This method allows bulk + assignment of multiple permissions at once. + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` - :param perm: str or list. The `codename(s)` of the `Permission(s)` + :param perms: list[str]. A list of `codenames` of the `Permissions` :param deny: bool. When `True`, break inheritance from parent object - :param defer_recalc: bool. When `True`, skip recalculating - descendants + :param defer_recalc: bool. When `True`, skip recalculating descendants :param skip_kc: bool. When `True`, skip assignment of applicable KC permissions :param partial_perms: dict. Filters used to narrow down query for - partial permissions + partial permissions + :return: A list of `ObjectPermission` instances or `None` """ - # Ensure `perm` is a list to handle multiple permissions - if isinstance(perm, str): - perm = [perm] - assignable_permissions = self.get_assignable_permissions() user_obj = get_database_user(user_obj) if is_user_anonymous(user_obj): - for p in perm: + for p in perms: # Is an anonymous user allowed to have this permission? parsed_perm = perm_parse(p, self) fq_permission = f'{parsed_perm[0]}.{parsed_perm[1]}' if ( - # fmt: off - not deny and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS - # fmt: on + not deny + and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS ): raise serializers.ValidationError( { @@ -485,7 +603,7 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, new_permissions = [] implied_permissions = set() - for p in perm: + for p in perms: app_label, codename = perm_parse(p, self) if codename not in assignable_permissions: # Some permissions are calculated and not stored in the database @@ -541,7 +659,7 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, remove_applicable_kc_permissions( self, user_obj, contradictory_codenames ) - # Create the new permission and add to the list of permissions + # Add permission to the list of permissions new_permissions.append( ObjectPermission( asset=self, @@ -563,13 +681,11 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, ) if new_permissions: - ObjectPermission.objects.bulk_create(new_permissions) - if len(new_permissions) == 1: - new_permissions = new_permissions[0] + ObjectPermission.objects.bulk_create(new_permissions, ignore_conflicts=True) if implied_permissions: - self.assign_perm( + self.assign_perms( user_obj=user_obj, - perm=list(implied_permissions), + perms=list(implied_permissions), deny=deny, defer_recalc=True, skip_kc=skip_kc, @@ -580,15 +696,18 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, if defer_recalc: return new_permissions - for p in perm: - self._update_partial_permissions( - user_obj, p, partial_perms=partial_perms - ) + for p in perms: + if p in self.CONTRADICTORY_PERMISSIONS.get(PERM_PARTIAL_SUBMISSIONS): + continue + else: + self._update_partial_permissions( + user_obj, p, partial_perms=partial_perms + ) post_assign_perm.send( sender=self.__class__, instance=self, user=user_obj, - codename=', '.join(perm), + codenames=perms, ) # Recalculate all descendants @@ -691,7 +810,87 @@ def has_perms(self, user_obj: User, perms: list, all_: bool = True) -> bool: @kc_transaction_atomic def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): """ - Revoke the given `perm` or list of permissions on this object from + Revoke the given `perm` on this object from `user_obj`. By default, + recalculate descendant objects' permissions and remove any + applicable KC permissions. May delete granted permissions or add + deny permissions as appropriate: + Current access Action + ============== ====== + None None + Direct Remove direct permission + Inherited Add deny permission + Direct & Inherited Remove direct permission; add deny permission + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` + :param perm str: The `codename` of the `Permission` + :param defer_recalc bool: When `True`, skip recalculating + descendants + :param skip_kc bool: When `True`, skip assignment of applicable KC + permissions + """ + user_obj = get_database_user(user_obj) + app_label, codename = perm_parse(perm, self) + # Get all assignable permissions, regardless of asset type. That way, + # we can allow invalid permissions to be removed + if codename not in self.get_assignable_permissions(ignore_type=True): + # Some permissions are calculated and not stored in the database + raise serializers.ValidationError({ + 'permission': f'{codename} cannot be removed explicitly.' + }) + all_permissions = self.permissions.filter( + user=user_obj, + permission__codename=codename, + deny=False + ) + direct_permissions = all_permissions.filter(inherited=False) + inherited_permissions = all_permissions.filter(inherited=True) + # Resolve implied permissions, e.g. revoking view implies revoking + # change + implied_perms = self.get_implied_perms( + codename, reverse=True, for_instance=self + ) + for implied_perm in implied_perms: + self.remove_perm( + user_obj, implied_perm, defer_recalc=True) + # Delete directly assigned permissions, if any + direct_permissions.delete() + if inherited_permissions.exists(): + # Delete inherited permissions + inherited_permissions.delete() + # Add a deny permission to block future inheritance + self.assign_perm(user_obj, perm, deny=True, defer_recalc=True) + # Remove any applicable KC permissions + if not skip_kc: + remove_applicable_kc_permissions(self, user_obj, codename) + + # We might have been called by ourself to assign a related + # permission. In that case, don't recalculate here. + if defer_recalc: + return + + self._update_partial_permissions(user_obj, perm, remove=True) + + post_remove_perm.send( + sender=self.__class__, + instance=self, + user=user_obj, + codename=codename, + ) + + # Recalculate all descendants + self.recalculate_descendants_perms() + + + @transaction.atomic + @kc_transaction_atomic + def remove_perms( + self, + user_obj: 'User', + perms: List[str], + defer_recalc: bool = False, + skip_kc: bool = False, + ) -> None: + """ + Bulk remove the list of permissions on this object from `user_obj`. By default, recalculate descendant objects' permissions and remove any applicable KC permissions. May delete granted permissions or add deny permissions as appropriate: @@ -702,19 +901,16 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): Inherited Add deny permission Direct & Inherited Remove direct permission; add deny permission :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` - :param perm str or list: The `codename(s)` of the `Permission(s)` + :param perm list[str]: The `codenames` of the `Permissions` :param defer_recalc bool: When `True`, skip recalculating descendants :param skip_kc bool: When `True`, skip assignment of applicable KC permissions + :return: None """ - # Ensure `perm` is a list to handle multiple permissions - if isinstance(perm, str): - perm = [perm] - user_obj = get_database_user(user_obj) permissions_to_delete = self.permissions.none() - all_perms_to_remove = perm.copy() + all_perms_to_remove = perms.copy() while all_perms_to_remove: current_perm = all_perms_to_remove.pop(0) @@ -763,12 +959,12 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): return # Ensure partial permissions are removed - if 'partial_submissions' in perm: + if 'partial_submissions' in perms: self._update_partial_permissions( user_obj, 'partial_submissions', remove=True ) - for p in perm: + for p in perms: app_label, codename = perm_parse(p, self) post_remove_perm.send( sender=self.__class__, diff --git a/kpi/serializers/v2/asset_permission_assignment.py b/kpi/serializers/v2/asset_permission_assignment.py index 00c2cdbb41..4938e8009b 100644 --- a/kpi/serializers/v2/asset_permission_assignment.py +++ b/kpi/serializers/v2/asset_permission_assignment.py @@ -352,9 +352,9 @@ def create(self, validated_data): # Perform the removals for each user for user_pk, permissions in user_permissions_to_remove.items(): - asset.remove_perm( + asset.remove_perms( user_obj=user_pk_to_obj_cache[user_pk], - perm=permissions + perms=permissions ) user_permissions = defaultdict(list) @@ -376,9 +376,9 @@ def create(self, validated_data): # Assign the permissions for each user for user_pk, permissions in user_permissions.items(): - asset.assign_perm( + asset.assign_perms( user_obj=user_pk_to_obj_cache[user_pk], - perm=permissions, + perms=permissions, partial_perms=user_partial_perms[user_pk] if user_partial_perms[user_pk] else None ) diff --git a/kpi/signals.py b/kpi/signals.py index bf25e01eb8..c6735464ed 100644 --- a/kpi/signals.py +++ b/kpi/signals.py @@ -1,4 +1,4 @@ -from typing import Union +from typing import List, Union from django.conf import settings from django.contrib.auth.models import AnonymousUser @@ -103,17 +103,20 @@ def post_assign_asset_perm( sender, instance, user: Union[settings.AUTH_USER_MODEL, 'AnonymousUser'], - codename: str, + codenames: Union[str, List[str]], **kwargs ): - - if not (is_user_anonymous(user) and codename == PERM_ADD_SUBMISSIONS): - return - - try: - instance.deployment.set_enketo_open_rosa_server(require_auth=False) - except DeploymentNotFound: - return + # If codenames is a string, convert it to a list + if isinstance(codenames, str): + codenames = [codenames] + + for codename in codenames: + if not (is_user_anonymous(user) and codename == PERM_ADD_SUBMISSIONS): + continue + try: + instance.deployment.set_enketo_open_rosa_server(require_auth=False) + except DeploymentNotFound: + return @receiver(post_remove_perm, sender=Asset) diff --git a/kpi/tests/api/v2/test_api_asset_permission_assignment.py b/kpi/tests/api/v2/test_api_asset_permission_assignment.py index 3b9d853cef..17625d1792 100644 --- a/kpi/tests/api/v2/test_api_asset_permission_assignment.py +++ b/kpi/tests/api/v2/test_api_asset_permission_assignment.py @@ -939,11 +939,11 @@ def test_partial_permission_invalid(self): response = self.client.post(bulk_endpoint, assignments, format='json') assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_bulk_assign_perm_reduces_queries(self): + def test_bulk_assign_perms_reduces_queries(self): permissions = [PERM_VIEW_ASSET, PERM_CHANGE_ASSET] # Assign permissions one by one - with self.assertNumQueries(36): + with self.assertNumQueries(46): for user in [self.someuser, self.anotheruser]: for perm in permissions: self.asset.assign_perm(user, perm) @@ -951,9 +951,9 @@ def test_bulk_assign_perm_reduces_queries(self): # Bulk assign permissions with self.assertNumQueries(12): for user in [self.someuser, self.anotheruser]: - self.asset.assign_perm(user, permissions) + self.asset.assign_perms(user, permissions) - def test_bulk_remove_perm_reduces_queries(self): + def test_bulk_remove_perms_reduces_queries(self): permissions = [ PERM_VIEW_SUBMISSIONS, PERM_CHANGE_SUBMISSIONS, @@ -961,10 +961,10 @@ def test_bulk_remove_perm_reduces_queries(self): ] for user in [self.someuser, self.anotheruser]: - self.asset.assign_perm(user, permissions) + self.asset.assign_perms(user, permissions) # Remove permissions one by one - with self.assertNumQueries(56): + with self.assertNumQueries(114): for user in [self.someuser, self.anotheruser]: for perm in permissions: self.asset.remove_perm(user, perm) @@ -972,4 +972,28 @@ def test_bulk_remove_perm_reduces_queries(self): # Bulk remove permissions with self.assertNumQueries(42): for user in [self.someuser, self.anotheruser]: - self.asset.remove_perm(user, permissions) + self.asset.remove_perms(user, permissions) + + def test_assign_perms_with_multiple_permissions(self): + # Assign multiple permissions at once + permissions = [ + PERM_VIEW_ASSET, + PERM_CHANGE_ASSET, + PERM_VIEW_SUBMISSIONS, + PERM_ADD_SUBMISSIONS, + PERM_CHANGE_SUBMISSIONS, + ] + self.asset.assign_perms(self.someuser, permissions) + + # Assert the user has been assigned the correct permissions + self.assertTrue(self.asset.has_perm(self.someuser, PERM_VIEW_ASSET)) + self.assertTrue(self.asset.has_perm(self.someuser, PERM_CHANGE_ASSET)) + self.assertTrue( + self.asset.has_perm(self.someuser, PERM_VIEW_SUBMISSIONS) + ) + self.assertTrue( + self.asset.has_perm(self.someuser, PERM_ADD_SUBMISSIONS) + ) + self.assertTrue( + self.asset.has_perm(self.someuser, PERM_CHANGE_SUBMISSIONS) + ) From 692a38d7a8e12a9c6655cb652d935626ac8ee0dd Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Fri, 20 Sep 2024 16:03:25 -0400 Subject: [PATCH 08/12] fix linter issues --- kpi/mixins/object_permission.py | 39 ++++++++++--------- .../v2/asset_permission_assignment.py | 10 ++++- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 3f2bc2697d..afae0ad742 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -596,7 +596,8 @@ def assign_perms( ): raise serializers.ValidationError( { - 'permission': f'Anonymous users cannot be granted the permission {p}.' + 'permission': f'Anonymous users cannot be granted the + permission {p}.' } ) @@ -609,7 +610,8 @@ def assign_perms( # Some permissions are calculated and not stored in the database raise serializers.ValidationError( { - 'permission': f'{codename} cannot be assigned explicitly to {self}' + 'permission': f'{codename} cannot be assigned explicitly to + {self}' } ) @@ -810,22 +812,22 @@ def has_perms(self, user_obj: User, perms: list, all_: bool = True) -> bool: @kc_transaction_atomic def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): """ - Revoke the given `perm` on this object from `user_obj`. By default, - recalculate descendant objects' permissions and remove any - applicable KC permissions. May delete granted permissions or add - deny permissions as appropriate: - Current access Action - ============== ====== - None None - Direct Remove direct permission - Inherited Add deny permission - Direct & Inherited Remove direct permission; add deny permission - :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` - :param perm str: The `codename` of the `Permission` - :param defer_recalc bool: When `True`, skip recalculating - descendants - :param skip_kc bool: When `True`, skip assignment of applicable KC - permissions + Revoke the given `perm` on this object from `user_obj`. By default, + recalculate descendant objects' permissions and remove any + applicable KC permissions. May delete granted permissions or add + deny permissions as appropriate: + Current access Action + ============== ====== + None None + Direct Remove direct permission + Inherited Add deny permission + Direct & Inherited Remove direct permission; add deny permission + :type user_obj: :py:class:`User` or :py:class:`AnonymousUser` + :param perm str: The `codename` of the `Permission` + :param defer_recalc bool: When `True`, skip recalculating + descendants + :param skip_kc bool: When `True`, skip assignment of applicable KC + permissions """ user_obj = get_database_user(user_obj) app_label, codename = perm_parse(perm, self) @@ -879,7 +881,6 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): # Recalculate all descendants self.recalculate_descendants_perms() - @transaction.atomic @kc_transaction_atomic def remove_perms( diff --git a/kpi/serializers/v2/asset_permission_assignment.py b/kpi/serializers/v2/asset_permission_assignment.py index 4938e8009b..30edcd0157 100644 --- a/kpi/serializers/v2/asset_permission_assignment.py +++ b/kpi/serializers/v2/asset_permission_assignment.py @@ -348,7 +348,9 @@ def create(self, validated_data): # Identify the removals and group by user user_permissions_to_remove = defaultdict(list) for removal in existing_assignments.difference(incoming_assignments): - user_permissions_to_remove[removal.user_pk].append(removal.permission_codename) + user_permissions_to_remove[removal.user_pk].append( + removal.permission_codename + ) # Perform the removals for each user for user_pk, permissions in user_permissions_to_remove.items(): @@ -379,7 +381,11 @@ def create(self, validated_data): asset.assign_perms( user_obj=user_pk_to_obj_cache[user_pk], perms=permissions, - partial_perms=user_partial_perms[user_pk] if user_partial_perms[user_pk] else None + partial_perms=( + user_partial_perms[user_pk] + if user_partial_perms[user_pk] + else None + ), ) # Return nothing, in a nice way, because the view is responsible for From 872ee18c2e003e06fd3f1e1a7d86449e91bf638a Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Fri, 20 Sep 2024 16:15:41 -0400 Subject: [PATCH 09/12] fix string line breaks --- kpi/mixins/object_permission.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index afae0ad742..36808e7301 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -596,8 +596,10 @@ def assign_perms( ): raise serializers.ValidationError( { - 'permission': f'Anonymous users cannot be granted the - permission {p}.' + 'permission': ( + f'Anonymous users cannot be granted the ' + f'permission {p}.' + ) } ) @@ -610,8 +612,9 @@ def assign_perms( # Some permissions are calculated and not stored in the database raise serializers.ValidationError( { - 'permission': f'{codename} cannot be assigned explicitly to - {self}' + 'permission': ( + f'{codename} cannot be assigned explicitly to {self}' + ) } ) From d92ecd96b01fc1dd8354645eedeea07d9f57fed5 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Mon, 23 Sep 2024 09:27:42 -0400 Subject: [PATCH 10/12] update darker.yml from beta-refactored branch --- .github/workflows/darker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/darker.yml b/.github/workflows/darker.yml index 18dd2763d1..4b6ee01246 100644 --- a/.github/workflows/darker.yml +++ b/.github/workflows/darker.yml @@ -24,6 +24,6 @@ jobs: # darker still exit with code 1 even with no errors on changes - name: Run Darker with base commit run: | - output=$(darker --check --isort -L "flake8 --max-line-length=88 --ignore=F821" kpi kobo hub -r ${{ github.event.pull_request.base.sha }}) + output=$(darker --check --isort -L "flake8 --max-line-length=88 --extend-ignore=F821" kpi kobo hub -r ${{ github.event.pull_request.base.sha }}) [[ -n "$output" ]] && echo "$output" && exit 1 || exit 0 shell: /usr/bin/bash {0} From ee876453a302b6ad45651ff8d6c98f1c463ef342 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Tue, 1 Oct 2024 11:13:04 -0400 Subject: [PATCH 11/12] remove deprecated typing imports --- kpi/mixins/object_permission.py | 8 ++++---- kpi/signals.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/kpi/mixins/object_permission.py b/kpi/mixins/object_permission.py index 36808e7301..03c76ed46a 100644 --- a/kpi/mixins/object_permission.py +++ b/kpi/mixins/object_permission.py @@ -3,7 +3,7 @@ import copy from collections import defaultdict -from typing import Dict, List, Optional, Union +from typing import Optional, Union from django.conf import settings from django.contrib.auth.models import AnonymousUser, Permission @@ -565,8 +565,8 @@ def assign_perms( deny: bool = False, defer_recalc: bool = False, skip_kc: bool = False, - partial_perms: Optional[Dict] = None, - ) -> Union[List['ObjectPermission'], None]: + partial_perms: Optional[dict] = None, + ) -> Union[list['ObjectPermission'], None]: r""" Assign `user_obj` a list of permissions (`perms`) on this object, or break inheritance from a parent object. This method allows bulk @@ -889,7 +889,7 @@ def remove_perm(self, user_obj, perm, defer_recalc=False, skip_kc=False): def remove_perms( self, user_obj: 'User', - perms: List[str], + perms: list[str], defer_recalc: bool = False, skip_kc: bool = False, ) -> None: diff --git a/kpi/signals.py b/kpi/signals.py index c6735464ed..9314ed1887 100644 --- a/kpi/signals.py +++ b/kpi/signals.py @@ -1,4 +1,4 @@ -from typing import List, Union +from typing import Union from django.conf import settings from django.contrib.auth.models import AnonymousUser @@ -103,7 +103,7 @@ def post_assign_asset_perm( sender, instance, user: Union[settings.AUTH_USER_MODEL, 'AnonymousUser'], - codenames: Union[str, List[str]], + codenames: Union[str, list[str]], **kwargs ): # If codenames is a string, convert it to a list From 4e57e37ae67ced02e10ac29c5a872b7976bd9939 Mon Sep 17 00:00:00 2001 From: RuthShryock Date: Wed, 2 Oct 2024 12:00:44 -0400 Subject: [PATCH 12/12] add missing import --- kpi/signals.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kpi/signals.py b/kpi/signals.py index 9314ed1887..603c437e94 100644 --- a/kpi/signals.py +++ b/kpi/signals.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from typing import Union from django.conf import settings