Merge pull request #2945 from kobotoolbox/2940-allow-non-owners-to-re…

…move-their-own-permissions

Allow non-owners to remove themselves from a shared asset

kpi/permissions.py

@@ -121,7 +121,7 @@ class AssetNestedObjectPermission(BaseAssetNestedObjectPermission):
     perms_map['HEAD'] = perms_map['GET']
     perms_map['PUT'] = perms_map['POST']
     perms_map['PATCH'] = perms_map['POST']
-    perms_map['DELETE'] = perms_map['POST']
+    perms_map['DELETE'] = perms_map['GET']
 
     def has_permission(self, request, view):
         if not request.user:

kpi/tests/api/v2/test_api_permissions.py

@@ -244,6 +244,116 @@ def test_inherited_viewable_asset_not_deletable(self):
         response = self.client.delete(url)
         self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
+    def test_shared_asset_remove_own_permissions_allowed(self):
+        """
+        Ensuring that a non-owner who has been shared an asset is able to remove
+        themselves from that asset if they want.
+        """
+        self.client.login(
+            username=self.someuser.username,
+            password=self.someuser_password,
+        )
+        new_asset = self.create_asset(
+            name='a new asset',
+            owner=self.someuser,
+        )
+        perm = new_asset.assign_perm(self.anotheruser, 'view_asset')
+        kwargs = {
+            'parent_lookup_asset': new_asset.uid,
+            'uid': perm.uid,
+        }
+        url = reverse(
+            'api_v2:asset-permission-assignment-detail', kwargs=kwargs
+        )
+        self.client.logout()
+        self.client.login(
+            username=self.anotheruser.username,
+            password=self.anotheruser_password,
+        )
+        assert self.anotheruser.has_perm(PERM_VIEW_ASSET, new_asset)
+
+        # `anotheruser` attempting to remove themselves from the asset
+        res = self.client.delete(url)
+        assert res.status_code == status.HTTP_204_NO_CONTENT
+        assert not self.anotheruser.has_perm(PERM_VIEW_ASSET, new_asset)
+        assert len(new_asset.get_perms(self.anotheruser)) == 0
+
+    def test_shared_asset_non_owner_remove_owners_permissions_not_allowed(self):
+        """
+        Ensuring that a non-owner who has been shared an asset is not able to
+        remove permissions from the owner of that asset
+        """
+        self.client.login(
+            username=self.someuser.username,
+            password=self.someuser_password,
+        )
+        new_asset = self.create_asset(
+            name='a new asset',
+            owner=self.someuser,
+        )
+        # Getting existing permission for the owner of the asset
+        perm = ObjectPermission.objects.filter(asset=new_asset).get(
+            user=self.someuser, permission__codename=PERM_VIEW_ASSET
+        )
+        new_asset.assign_perm(self.anotheruser, PERM_VIEW_ASSET)
+        kwargs = {
+            'parent_lookup_asset': new_asset.uid,
+            'uid': perm.uid,
+        }
+        url = reverse(
+            'api_v2:asset-permission-assignment-detail', kwargs=kwargs
+        )
+        self.client.logout()
+        self.client.login(
+            username=self.anotheruser.username,
+            password=self.anotheruser_password,
+        )
+        assert self.someuser.has_perm(PERM_VIEW_ASSET, new_asset)
+
+        # `anotheruser` attempting to remove `someuser` from the asset
+        res = self.client.delete(url)
+        assert res.status_code == status.HTTP_403_FORBIDDEN
+        assert self.someuser.has_perm(PERM_VIEW_ASSET, new_asset)
+
+    def test_shared_asset_non_owner_remove_another_non_owners_permissions_not_allowed(self):
+        """
+        Ensuring that a non-owner who has an asset shared with them cannot
+        remove permissions from another non-owner with that same asset shared
+        with them.
+        """
+        yetanotheruser = User.objects.create(
+            username='yetanotheruser',
+        )
+        self.client.login(
+            username=self.someuser.username,
+            password=self.someuser_password,
+        )
+        new_asset = self.create_asset(
+            name='a new asset',
+            owner=self.someuser,
+            owner_password=self.someuser_password,
+        )
+        new_asset.assign_perm(self.anotheruser, PERM_VIEW_ASSET)
+        perm = new_asset.assign_perm(yetanotheruser, PERM_VIEW_ASSET)
+        kwargs = {
+            'parent_lookup_asset': new_asset.uid,
+            'uid': perm.uid,
+        }
+        url = reverse(
+            'api_v2:asset-permission-assignment-detail', kwargs=kwargs
+        )
+        self.client.logout()
+        self.client.login(
+            username=self.anotheruser.username,
+            password=self.anotheruser_password,
+        )
+        assert yetanotheruser.has_perm(PERM_VIEW_ASSET, new_asset)
+
+        # `anotheruser` attempting to remove `yetanotheruser` from the asset
+        res = self.client.delete(url)
+        assert res.status_code == status.HTTP_404_NOT_FOUND
+        assert yetanotheruser.has_perm(PERM_VIEW_ASSET, new_asset)
+
     def test_copy_permissions_between_assets(self):
         # Give "someuser" edit permissions on an asset owned by "admin"
         self.add_perm(self.admin_asset, self.someuser, 'change_')

kpi/views/v2/asset_permission_assignment.py

@@ -234,7 +234,15 @@ def clone(self, request, *args, **kwargs):
     def destroy(self, request, *args, **kwargs):
         object_permission = self.get_object()
         user = object_permission.user
-        if user.pk == self.asset.owner_id:
+        # If the user is not the owner of the asset, but trying to delete the
+        # owner's permissions, raise permission denied error. However, if they
+        # are the owner of the asset, they should also be prevented from
+        # deleting their own permissions, but given a more appropriate response
+        if (self.request.user.pk != self.asset.owner_id) and (
+            self.request.user.pk != user.pk
+        ):
+            raise exceptions.PermissionDenied()
+        elif user.pk == self.asset.owner_id:
             return Response({
                 'detail': _("Owner's permissions cannot be deleted")
             }, status=status.HTTP_409_CONFLICT)