From 0af0e354163f5a8598d67b074a9bd7e83c6c7a0e Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Fri, 17 Apr 2020 17:07:01 -0400 Subject: [PATCH 1/4] Add failing test case for denying anonymous perms See #2528 --- kpi/tests/api/v2/test_api_permissions.py | 30 ++++++++++++++++++++++++ kpi/tests/kpi_test_case.py | 20 ++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/kpi/tests/api/v2/test_api_permissions.py b/kpi/tests/api/v2/test_api_permissions.py index 2f2572ef76..895927d7c4 100644 --- a/kpi/tests/api/v2/test_api_permissions.py +++ b/kpi/tests/api/v2/test_api_permissions.py @@ -86,6 +86,36 @@ def test_public_asset_not_in_list_admin(self): self.assert_object_in_object_list(self.someusers_public_asset, self.admin, self.admin_password, in_list=False) + def test_revoke_anon_from_asset_in_public_collection(self): + self.login(self.someuser.username, self.someuser_password) + public_collection = self.create_collection('public_collection') + child_asset = self.create_asset('child_asset_in_public_collection') + self.add_to_collection(child_asset, public_collection) + child_asset.refresh_from_db() + + # Anon should have no access at this point + self.client.logout() + self.assert_viewable(child_asset, viewable=False) + + # Grant anon access to the parent collection + self.login(self.someuser.username, self.someuser_password) + self.add_perm(public_collection, self.anon, 'view_') + + # Verify anon can access the child asset + self.client.logout() + # Anon can only see a public asset by accessing the detail view + # directly; `assert_viewble()` will always fail because it expects the + # asset to be in the list view as well + self.assert_detail_viewable(child_asset) + + # Revoke anon's access to the child asset + self.login(self.someuser.username, self.someuser_password) + self.remove_perm_v2_api(child_asset, self.anon,'view_asset') + + # Make sure anon cannot access the child asset any longer + self.client.logout() + self.assert_viewable(child_asset, viewable=False) + class ApiPermissionsTestCase(KpiTestCase): fixtures = ['test_data'] diff --git a/kpi/tests/kpi_test_case.py b/kpi/tests/kpi_test_case.py index dd33e14b13..705416ec49 100644 --- a/kpi/tests/kpi_test_case.py +++ b/kpi/tests/kpi_test_case.py @@ -197,6 +197,26 @@ def remove_perm(self, obj, owner, owner_password, other_user, # assigned yet and fails when it has. self._test_remove_perm(obj, perm_name_prefix, other_user) + def remove_perm_v2_api(self, asset, user, perm_codename): + # TODO: replace `remove_perm()` with this method + + if not isinstance(asset, Asset): + raise NotImplementedError + + list_url = reverse( + 'api_v2:asset-permission-assignment-list', args=[asset.uid] + ) + perm_list = self.client.get(list_url).data + for perm in perm_list: + # Is there there no simple way to resolve a URL to an object? + user_detail = self.client.get(perm['user']).data + if user_detail['username'] == user.username: + perm_detail = self.client.get(perm['permission']).data + if perm_detail['codename'] == perm_codename: + break + response = self.client.delete(perm['url']) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + def assert_object_in_object_list(self, obj, user=None, password=None, in_list=True, msg=None): view_name = obj._meta.model_name + '-list' From b678858de6086dae10f034a8f9871e59d3b1c35c Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 4 Dec 2019 15:33:29 -0400 Subject: [PATCH 2/4] Do not expose denied permissions --- kpi/utils/object_permission_helper.py | 2 +- kpi/views/v2/asset.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/kpi/utils/object_permission_helper.py b/kpi/utils/object_permission_helper.py index 6d96caf168..fc9044f0ea 100644 --- a/kpi/utils/object_permission_helper.py +++ b/kpi/utils/object_permission_helper.py @@ -47,7 +47,7 @@ def get_user_permission_assignments_queryset(cls, affected_object, user): # `affected_object.permissions` is a `GenericRelation(ObjectPermission)` # Don't Prefetch `content_object`. # See `AssetPermissionAssignmentSerializer.to_representation()` - queryset = affected_object.permissions.select_related( + queryset = affected_object.permissions.filter(deny=False).select_related( 'permission', 'user' ).order_by( 'user__username', 'permission__codename' diff --git a/kpi/views/v2/asset.py b/kpi/views/v2/asset.py index 64876c54ec..37c0ad1179 100644 --- a/kpi/views/v2/asset.py +++ b/kpi/views/v2/asset.py @@ -231,7 +231,8 @@ def get_serializer_context(self): asset_ids = self.filter_queryset(queryset).values_list('id').distinct() object_permissions = ObjectPermission.objects.filter( content_type_id=asset_content_type.pk, - object_id__in=asset_ids + object_id__in=asset_ids, + deny=False, ).select_related( 'user', 'permission' ).order_by( From 5b615f9257862a731169b36a05618c9a8e6f2dd1 Mon Sep 17 00:00:00 2001 From: Olivier Leger Date: Wed, 4 Dec 2019 15:35:25 -0400 Subject: [PATCH 3/4] Allow to assign a denied permission to AnonymousUser even if it does not belong to anonymous user's allowed permissions --- kpi/models/object_permission.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kpi/models/object_permission.py b/kpi/models/object_permission.py index 914f54b111..a92124d8d7 100644 --- a/kpi/models/object_permission.py +++ b/kpi/models/object_permission.py @@ -728,8 +728,9 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, user_obj.pk == settings.ANONYMOUS_USER_ID ): # Is an anonymous user allowed to have this permission? - fq_permission = '{}.{}'.format(app_label, codename) - if fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS: + fq_permission = f'{app_label}.{codename}' + if deny is False and \ + fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS: raise ValidationError( 'Anonymous users cannot have the permission {}.'.format( codename) From 4186f0fc3807feff97ddbf32404af28243728305 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Fri, 17 Apr 2020 14:38:34 -0400 Subject: [PATCH 4/4] Prefer `not` to `is False` and clarify message --- kpi/models/object_permission.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/kpi/models/object_permission.py b/kpi/models/object_permission.py index a92124d8d7..9024143503 100644 --- a/kpi/models/object_permission.py +++ b/kpi/models/object_permission.py @@ -729,11 +729,14 @@ def assign_perm(self, user_obj, perm, deny=False, defer_recalc=False, ): # Is an anonymous user allowed to have this permission? fq_permission = f'{app_label}.{codename}' - if deny is False and \ - fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS: + if ( + not deny + and fq_permission not in settings.ALLOWED_ANONYMOUS_PERMISSIONS + ): raise ValidationError( - 'Anonymous users cannot have the permission {}.'.format( - codename) + 'Anonymous users cannot be granted the permission {}.'.format( + codename + ) ) # Get the User database representation for AnonymousUser user_obj = get_anonymous_user()