From: shamoon <4887959+shamoon@users.noreply.github.com>
Date: Mon, 26 Feb 2024 00:59:59 +0000 (-0800)
Subject: Fix: bulk edit objects does not respect global permissions (#5888)
X-Git-Tag: v2.5.4~2^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=db0a2eb1a3e4658de7b3f2fea6e0087726d3976c;p=thirdparty%2Fpaperless-ngx.git

Fix: bulk edit objects does not respect global permissions (#5888)
---

diff --git a/src-ui/src/app/components/manage/management-list/management-list.component.html b/src-ui/src/app/components/manage/management-list/management-list.component.html
index 58101c388a..997ca00bd0 100644
--- a/src-ui/src/app/components/manage/management-list/management-list.component.html
+++ b/src-ui/src/app/components/manage/management-list/management-list.component.html
@@ -2,10 +2,10 @@
   <button class="btn btn-sm btn-outline-secondary me-2" (click)="clearSelection()" [hidden]="selectedObjects.size === 0">
     <i-bs  name="x"></i-bs>&nbsp;<ng-container i18n>Clear selection</ng-container>
     </button>
-    <button type="button" class="btn btn-sm btn-outline-primary me-2" (click)="setPermissions()" [disabled]="!userOwnsAll || selectedObjects.size === 0">
+    <button type="button" class="btn btn-sm btn-outline-primary me-2" (click)="setPermissions()" [disabled]="!userCanBulkEdit(PermissionAction.Change) || selectedObjects.size === 0">
       <i-bs name="person-fill-lock"></i-bs>&nbsp;<ng-container i18n>Permissions</ng-container>
     </button>
-    <button type="button" class="btn btn-sm btn-outline-danger me-5" (click)="delete()" [disabled]="!userOwnsAll || selectedObjects.size === 0">
+    <button type="button" class="btn btn-sm btn-outline-danger me-5" (click)="delete()" [disabled]="!userCanBulkEdit(PermissionAction.Delete) || selectedObjects.size === 0">
       <i-bs name="trash"></i-bs>&nbsp;<ng-container i18n>Delete</ng-container>
     </button>
     <button type="button" class="btn btn-sm btn-outline-primary" (click)="openCreateDialog()" *pngxIfPermissions="{ action: PermissionAction.Add, type: permissionType }">
diff --git a/src-ui/src/app/components/manage/management-list/management-list.component.spec.ts b/src-ui/src/app/components/manage/management-list/management-list.component.spec.ts
index 710d3018a9..c349fa935f 100644
--- a/src-ui/src/app/components/manage/management-list/management-list.component.spec.ts
+++ b/src-ui/src/app/components/manage/management-list/management-list.component.spec.ts
@@ -23,7 +23,10 @@ import { TagService } from 'src/app/services/rest/tag.service'
 import { PageHeaderComponent } from '../../common/page-header/page-header.component'
 import { TagListComponent } from '../tag-list/tag-list.component'
 import { ManagementListComponent } from './management-list.component'
-import { PermissionsService } from 'src/app/services/permissions.service'
+import {
+  PermissionAction,
+  PermissionsService,
+} from 'src/app/services/permissions.service'
 import { ToastService } from 'src/app/services/toast.service'
 import { EditDialogComponent } from '../../common/edit-dialog/edit-dialog.component'
 import { ConfirmDialogComponent } from '../../common/confirm-dialog/confirm-dialog.component'
@@ -65,6 +68,7 @@ describe('ManagementListComponent', () => {
   let modalService: NgbModal
   let toastService: ToastService
   let documentListViewService: DocumentListViewService
+  let permissionsService: PermissionsService
 
   beforeEach(async () => {
     TestBed.configureTestingModule({
@@ -77,18 +81,7 @@ describe('ManagementListComponent', () => {
         ConfirmDialogComponent,
         PermissionsDialogComponent,
       ],
-      providers: [
-        {
-          provide: PermissionsService,
-          useValue: {
-            currentUserCan: () => true,
-            currentUserHasObjectPermissions: () => true,
-            currentUserOwnsObject: () => true,
-          },
-        },
-        DatePipe,
-        PermissionsGuard,
-      ],
+      providers: [DatePipe, PermissionsGuard],
       imports: [
         HttpClientTestingModule,
         NgbPaginationModule,
@@ -115,6 +108,14 @@ describe('ManagementListComponent', () => {
           })
         }
       )
+    permissionsService = TestBed.inject(PermissionsService)
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(true)
+    jest
+      .spyOn(permissionsService, 'currentUserHasObjectPermissions')
+      .mockReturnValue(true)
+    jest
+      .spyOn(permissionsService, 'currentUserOwnsObject')
+      .mockReturnValue(true)
     modalService = TestBed.inject(NgbModal)
     toastService = TestBed.inject(ToastService)
     documentListViewService = TestBed.inject(DocumentListViewService)
@@ -312,4 +313,10 @@ describe('ManagementListComponent', () => {
     expect(bulkEditSpy).toHaveBeenCalled()
     expect(successToastSpy).toHaveBeenCalled()
   })
+
+  it('should disallow bulk permissions or delete objects if no global perms', () => {
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(false)
+    expect(component.userCanBulkEdit(PermissionAction.Delete)).toBeFalsy()
+    expect(component.userCanBulkEdit(PermissionAction.Change)).toBeFalsy()
+  })
 })
diff --git a/src-ui/src/app/components/manage/management-list/management-list.component.ts b/src-ui/src/app/components/manage/management-list/management-list.component.ts
index 0b0365f068..8837314882 100644
--- a/src-ui/src/app/components/manage/management-list/management-list.component.ts
+++ b/src-ui/src/app/components/manage/management-list/management-list.component.ts
@@ -15,16 +15,14 @@ import {
   MATCH_NONE,
 } from 'src/app/data/matching-model'
 import { ObjectWithId } from 'src/app/data/object-with-id'
-import {
-  ObjectWithPermissions,
-  PermissionsObject,
-} from 'src/app/data/object-with-permissions'
+import { ObjectWithPermissions } from 'src/app/data/object-with-permissions'
 import {
   SortableDirective,
   SortEvent,
 } from 'src/app/directives/sortable.directive'
 import { DocumentListViewService } from 'src/app/services/document-list-view.service'
 import {
+  PermissionAction,
   PermissionsService,
   PermissionType,
 } from 'src/app/services/permissions.service'
@@ -250,7 +248,9 @@ export abstract class ManagementListComponent<T extends ObjectWithId>
     )
   }
 
-  get userOwnsAll(): boolean {
+  userCanBulkEdit(action: PermissionAction): boolean {
+    if (!this.permissionsService.currentUserCan(action, this.permissionType))
+      return false
     let ownsAll: boolean = true
     const objects = this.data.filter((o) => this.selectedObjects.has(o.id))
     ownsAll = objects.every((o) =>
diff --git a/src/documents/tests/test_api_objects.py b/src/documents/tests/test_api_objects.py
index 3b38f2b5fb..9a0ccd5981 100644
--- a/src/documents/tests/test_api_objects.py
+++ b/src/documents/tests/test_api_objects.py
@@ -1,6 +1,7 @@
 import json
 from unittest import mock
 
+from django.contrib.auth.models import Permission
 from django.contrib.auth.models import User
 from rest_framework import status
 from rest_framework.test import APITestCase
@@ -310,7 +311,62 @@ class TestBulkEditObjects(APITestCase):
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(StoragePath.objects.count(), 0)
 
-    def test_bulk_edit_object_permissions_insufficient_perms(self):
+    def test_bulk_edit_object_permissions_insufficient_global_perms(self):
+        """
+        GIVEN:
+            - Existing objects, user does not have global delete permissions
+        WHEN:
+            - bulk_edit_objects API endpoint is called with delete operation
+        THEN:
+            - User is not able to delete objects
+        """
+        self.client.force_authenticate(user=self.user1)
+
+        response = self.client.post(
+            "/api/bulk_edit_objects/",
+            json.dumps(
+                {
+                    "objects": [self.t1.id, self.t2.id],
+                    "object_type": "tags",
+                    "operation": "delete",
+                },
+            ),
+            content_type="application/json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+        self.assertEqual(response.content, b"Insufficient permissions")
+
+    def test_bulk_edit_object_permissions_sufficient_global_perms(self):
+        """
+        GIVEN:
+            - Existing objects, user does have global delete permissions
+        WHEN:
+            - bulk_edit_objects API endpoint is called with delete operation
+        THEN:
+            - User is able to delete objects
+        """
+        self.user1.user_permissions.add(
+            *Permission.objects.filter(codename="delete_tag"),
+        )
+        self.user1.save()
+        self.client.force_authenticate(user=self.user1)
+
+        response = self.client.post(
+            "/api/bulk_edit_objects/",
+            json.dumps(
+                {
+                    "objects": [self.t1.id, self.t2.id],
+                    "object_type": "tags",
+                    "operation": "delete",
+                },
+            ),
+            content_type="application/json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+    def test_bulk_edit_object_permissions_insufficient_object_perms(self):
         """
         GIVEN:
             - Objects owned by user other than logged in user
@@ -319,8 +375,13 @@ class TestBulkEditObjects(APITestCase):
         THEN:
             - User is not able to delete objects
         """
-        self.t1.owner = User.objects.get(username="temp_admin")
-        self.t1.save()
+        self.t2.owner = User.objects.get(username="temp_admin")
+        self.t2.save()
+
+        self.user1.user_permissions.add(
+            *Permission.objects.filter(codename="delete_tag"),
+        )
+        self.user1.save()
         self.client.force_authenticate(user=self.user1)
 
         response = self.client.post(
diff --git a/src/documents/views.py b/src/documents/views.py
index c73a8050bc..6169ac5bb1 100644
--- a/src/documents/views.py
+++ b/src/documents/views.py
@@ -1419,7 +1419,15 @@ class BulkEditObjectsView(GenericAPIView, PassUserMixin):
         objs = object_class.objects.filter(pk__in=object_ids)
 
         if not user.is_superuser:
-            has_perms = all((obj.owner == user or obj.owner is None) for obj in objs)
+            model_name = object_class._meta.verbose_name
+            perm = (
+                f"documents.change_{model_name}"
+                if operation == "set_permissions"
+                else f"documents.delete_{model_name}"
+            )
+            has_perms = user.has_perm(perm) and all(
+                (obj.owner == user or obj.owner is None) for obj in objs
+            )
 
             if not has_perms:
                 return HttpResponseForbidden("Insufficient permissions")