Fix: handle errors for trash actions and only show documents user can restore or delete (#7119)

2025-04-02 13:45:10 -05:00 · 2024-06-27 13:33:39 -07:00 · 2024-06-27 13:33:39 -07:00 · ac0ed0def8
commit ac0ed0def8
parent f01283c309
6 changed files with 195 additions and 43 deletions
--- a/src-ui/messages.xlf
+++ b/src-ui/messages.xlf
@ -1437,7 +1437,7 @@
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">80</context>
+          <context context-type="linenumber">86</context>
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/users-groups/users-groups.component.html</context>
@ -2153,7 +2153,7 @@
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">74</context>
+          <context context-type="linenumber">80</context>
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/manage/management-list/management-list.component.ts</context>
@ -2179,7 +2179,7 @@
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">78</context>
+          <context context-type="linenumber">84</context>
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/users-groups/users-groups.component.ts</context>
@ -2214,42 +2214,74 @@
        <source>Document deleted</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">63</context>
+          <context context-type="linenumber">64</context>
+        </context-group>
+      </trans-unit>
+      <trans-unit id="7295637485862454066" datatype="html">
+        <source>Error deleting document</source>
+        <context-group purpose="location">
+          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
+          <context context-type="linenumber">69</context>
+        </context-group>
+        <context-group purpose="location">
+          <context context-type="sourcefile">src/app/components/document-detail/document-detail.component.ts</context>
+          <context context-type="linenumber">799</context>
        </context-group>
      </trans-unit>
      <trans-unit id="7266264608936522311" datatype="html">
        <source>This operation will permanently delete the selected documents.</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">76</context>
+          <context context-type="linenumber">82</context>
        </context-group>
      </trans-unit>
      <trans-unit id="6804051092296228130" datatype="html">
        <source>This operation will permanently delete all documents in the trash.</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">77</context>
+          <context context-type="linenumber">83</context>
        </context-group>
      </trans-unit>
      <trans-unit id="6996183233986182894" datatype="html">
        <source>Document(s) deleted</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">87</context>
+          <context context-type="linenumber">94</context>
+        </context-group>
+      </trans-unit>
+      <trans-unit id="6962724852893361467" datatype="html">
+        <source>Error deleting document(s)</source>
+        <context-group purpose="location">
+          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
+          <context context-type="linenumber">101</context>
        </context-group>
      </trans-unit>
      <trans-unit id="7534569062269274401" datatype="html">
        <source>Document restored</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">97</context>
+          <context context-type="linenumber">113</context>
+        </context-group>
+      </trans-unit>
+      <trans-unit id="9136016619414048201" datatype="html">
+        <source>Error restoring document</source>
+        <context-group purpose="location">
+          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
+          <context context-type="linenumber">117</context>
        </context-group>
      </trans-unit>
      <trans-unit id="960063472770266304" datatype="html">
        <source>Document(s) restored</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
-          <context context-type="linenumber">106</context>
+          <context context-type="linenumber">127</context>
+        </context-group>
+      </trans-unit>
+      <trans-unit id="8405416976953346141" datatype="html">
+        <source>Error restoring document(s)</source>
+        <context-group purpose="location">
+          <context context-type="sourcefile">src/app/components/admin/trash/trash.component.ts</context>
+          <context context-type="linenumber">133</context>
        </context-group>
      </trans-unit>
      <trans-unit id="8119815638230251386" datatype="html">
@ -6073,13 +6105,6 @@
          <context context-type="linenumber">716</context>
        </context-group>
      </trans-unit>
-      <trans-unit id="7295637485862454066" datatype="html">
-        <source>Error deleting document</source>
-        <context-group purpose="location">
-          <context context-type="sourcefile">src/app/components/document-detail/document-detail.component.ts</context>
-          <context context-type="linenumber">799</context>
-        </context-group>
-      </trans-unit>
      <trans-unit id="619486176823357521" datatype="html">
        <source>Reprocess confirm</source>
        <context-group purpose="location">
--- a/src-ui/src/app/components/admin/trash/trash.component.spec.ts
+++ b/src-ui/src/app/components/admin/trash/trash.component.spec.ts
@ -11,10 +11,11 @@ import {
 import { NgxBootstrapIconsModule, allIcons } from 'ngx-bootstrap-icons'
 import { FormsModule, ReactiveFormsModule } from '@angular/forms'
 import { TrashService } from 'src/app/services/trash.service'
-import { of } from 'rxjs'
+import { of, throwError } from 'rxjs'
 import { ConfirmDialogComponent } from '../../common/confirm-dialog/confirm-dialog.component'
 import { By } from '@angular/platform-browser'
 import { SafeHtmlPipe } from 'src/app/pipes/safehtml.pipe'
+import { ToastService } from 'src/app/services/toast.service'

 const documentsInTrash = [
  {
@ -36,6 +37,7 @@ describe('TrashComponent', () => {
  let fixture: ComponentFixture<TrashComponent>
  let trashService: TrashService
  let modalService: NgbModal
+  let toastService: ToastService

  beforeEach(async () => {
    await TestBed.configureTestingModule({
@ -58,6 +60,7 @@ describe('TrashComponent', () => {
    fixture = TestBed.createComponent(TrashComponent)
    trashService = TestBed.inject(TrashService)
    modalService = TestBed.inject(NgbModal)
+    toastService = TestBed.inject(ToastService)
    component = fixture.componentInstance
    fixture.detectChanges()
  })
@ -76,12 +79,20 @@ describe('TrashComponent', () => {
    expect(component.documentsInTrash).toEqual(documentsInTrash)
  })

-  it('should support delete document', () => {
+  it('should support delete document, show error if needed', () => {
    const trashSpy = jest.spyOn(trashService, 'emptyTrash')
    let modal
    modalService.activeInstances.subscribe((instances) => {
      modal = instances[0]
    })
+    const toastErrorSpy = jest.spyOn(toastService, 'showError')
+
+    // fail first
+    trashSpy.mockReturnValue(throwError(() => 'Error'))
+    component.delete(documentsInTrash[0])
+    modal.componentInstance.confirmClicked.next()
+    expect(toastErrorSpy).toHaveBeenCalled()
+
    trashSpy.mockReturnValue(of('OK'))
    component.delete(documentsInTrash[0])
    expect(modal).toBeDefined()
@ -89,12 +100,20 @@ describe('TrashComponent', () => {
    expect(trashSpy).toHaveBeenCalled()
  })

-  it('should support empty trash', () => {
+  it('should support empty trash, show error if needed', () => {
    const trashSpy = jest.spyOn(trashService, 'emptyTrash')
    let modal
    modalService.activeInstances.subscribe((instances) => {
      modal = instances[instances.length - 1]
    })
+    const toastErrorSpy = jest.spyOn(toastService, 'showError')
+
+    // fail first
+    trashSpy.mockReturnValue(throwError(() => 'Error'))
+    component.emptyTrash()
+    modal.componentInstance.confirmClicked.next()
+    expect(toastErrorSpy).toHaveBeenCalled()
+
    trashSpy.mockReturnValue(of('OK'))
    component.emptyTrash()
    expect(modal).toBeDefined()
@ -106,18 +125,34 @@ describe('TrashComponent', () => {
    expect(trashSpy).toHaveBeenCalledWith([1, 2])
  })

-  it('should support restore document', () => {
+  it('should support restore document, show error if needed', () => {
    const restoreSpy = jest.spyOn(trashService, 'restoreDocuments')
    const reloadSpy = jest.spyOn(component, 'reload')
+    const toastErrorSpy = jest.spyOn(toastService, 'showError')
+
+    // fail first
+    restoreSpy.mockReturnValue(throwError(() => 'Error'))
+    component.restore(documentsInTrash[0])
+    expect(toastErrorSpy).toHaveBeenCalled()
+    expect(reloadSpy).not.toHaveBeenCalled()
+
    restoreSpy.mockReturnValue(of('OK'))
    component.restore(documentsInTrash[0])
    expect(restoreSpy).toHaveBeenCalledWith([documentsInTrash[0].id])
    expect(reloadSpy).toHaveBeenCalled()
  })

-  it('should support restore all documents', () => {
+  it('should support restore all documents, show error if needed', () => {
    const restoreSpy = jest.spyOn(trashService, 'restoreDocuments')
    const reloadSpy = jest.spyOn(component, 'reload')
+    const toastErrorSpy = jest.spyOn(toastService, 'showError')
+
+    // fail first
+    restoreSpy.mockReturnValue(throwError(() => 'Error'))
+    component.restoreAll()
+    expect(toastErrorSpy).toHaveBeenCalled()
+    expect(reloadSpy).not.toHaveBeenCalled()
+
    restoreSpy.mockReturnValue(of('OK'))
    component.restoreAll()
    expect(restoreSpy).toHaveBeenCalled()
--- a/src-ui/src/app/components/admin/trash/trash.component.ts
+++ b/src-ui/src/app/components/admin/trash/trash.component.ts
@ -59,10 +59,16 @@ export class TrashComponent implements OnDestroy {
      .pipe(takeUntil(this.unsubscribeNotifier))
      .subscribe(() => {
        modal.componentInstance.buttonsEnabled = false
-        this.trashService.emptyTrash([document.id]).subscribe(() => {
-          this.toastService.showInfo($localize`Document deleted`)
-          modal.close()
-          this.reload()
+        this.trashService.emptyTrash([document.id]).subscribe({
+          next: () => {
+            this.toastService.showInfo($localize`Document deleted`)
+            modal.close()
+            this.reload()
+          },
+          error: (err) => {
+            this.toastService.showError($localize`Error deleting document`, err)
+            modal.close()
+          },
        })
      })
  }
@ -83,29 +89,51 @@ export class TrashComponent implements OnDestroy {
      .subscribe(() => {
        this.trashService
          .emptyTrash(documents ? Array.from(documents) : null)
-          .subscribe(() => {
-            this.toastService.showInfo($localize`Document(s) deleted`)
-            this.allToggled = false
-            modal.close()
-            this.reload()
+          .subscribe({
+            next: () => {
+              this.toastService.showInfo($localize`Document(s) deleted`)
+              this.allToggled = false
+              modal.close()
+              this.reload()
+            },
+            error: (err) => {
+              this.toastService.showError(
+                $localize`Error deleting document(s)`,
+                err
+              )
+              modal.close()
+            },
          })
      })
  }

  restore(document: Document) {
-    this.trashService.restoreDocuments([document.id]).subscribe(() => {
-      this.toastService.showInfo($localize`Document restored`)
-      this.reload()
+    this.trashService.restoreDocuments([document.id]).subscribe({
+      next: () => {
+        this.toastService.showInfo($localize`Document restored`)
+        this.reload()
+      },
+      error: (err) => {
+        this.toastService.showError($localize`Error restoring document`, err)
+      },
    })
  }

  restoreAll(documents: Set<number> = null) {
    this.trashService
      .restoreDocuments(documents ? Array.from(documents) : null)
-      .subscribe(() => {
-        this.toastService.showInfo($localize`Document(s) restored`)
-        this.allToggled = false
-        this.reload()
+      .subscribe({
+        next: () => {
+          this.toastService.showInfo($localize`Document(s) restored`)
+          this.allToggled = false
+          this.reload()
+        },
+        error: (err) => {
+          this.toastService.showError(
+            $localize`Error restoring document(s)`,
+            err
+          )
+        },
      })
  }

--- a/src/documents/filters.py
+++ b/src/documents/filters.py
@ -276,3 +276,17 @@ class ObjectOwnedOrGrantedPermissionsFilter(ObjectPermissionsFilter):
        objects_owned = queryset.filter(owner=request.user)
        objects_unowned = queryset.filter(owner__isnull=True)
        return objects_with_perms | objects_owned | objects_unowned
+
+
+class ObjectOwnedPermissionsFilter(ObjectPermissionsFilter):
+    """
+    A filter backend that limits results to those where the requesting user
+    owns the objects or objects without an owner (for backwards compat)
+    """
+
+    def filter_queryset(self, request, queryset, view):
+        if request.user.is_superuser:
+            return queryset
+        objects_owned = queryset.filter(owner=request.user)
+        objects_unowned = queryset.filter(owner__isnull=True)
+        return objects_owned | objects_unowned
--- a/src/documents/tests/test_api_trash.py
+++ b/src/documents/tests/test_api_trash.py
@ -1,3 +1,4 @@
+from django.contrib.auth.models import Permission
 from django.contrib.auth.models import User
 from django.core.cache import cache
 from rest_framework import status
@ -10,7 +11,8 @@ class TestTrashAPI(APITestCase):
    def setUp(self):
        super().setUp()

-        self.user = User.objects.create_superuser(username="temp_admin")
+        self.user = User.objects.create_user(username="temp_admin")
+        self.user.user_permissions.add(*Permission.objects.all())
        self.client.force_authenticate(user=self.user)
        cache.clear()

@ -97,6 +99,56 @@ class TestTrashAPI(APITestCase):
        self.assertEqual(resp.status_code, status.HTTP_200_OK)
        self.assertEqual(Document.global_objects.count(), 0)

+    def test_api_trash_show_owned_only(self):
+        """
+        GIVEN:
+            - Existing documents in trash
+        WHEN:
+            - API request to show documents in trash for regular user
+            - API request to show documents in trash for superuser
+        THEN:
+            - Only owned documents are returned
+        """
+
+        document_u1 = Document.objects.create(
+            title="Title",
+            content="content",
+            checksum="checksum",
+            mime_type="application/pdf",
+            owner=self.user,
+        )
+        document_u1.delete()
+        document_not_owned = Document.objects.create(
+            title="Title2",
+            content="content2",
+            checksum="checksum2",
+            mime_type="application/pdf",
+        )
+        document_not_owned.delete()
+        user2 = User.objects.create_user(username="user2")
+        document_u2 = Document.objects.create(
+            title="Title3",
+            content="content3",
+            checksum="checksum3",
+            mime_type="application/pdf",
+            owner=user2,
+        )
+        document_u2.delete()
+
+        # user only sees their own documents or unowned documents
+        resp = self.client.get("/api/trash/")
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(resp.data["count"], 2)
+        self.assertEqual(resp.data["results"][0]["id"], document_not_owned.pk)
+        self.assertEqual(resp.data["results"][1]["id"], document_u1.pk)
+
+        # superuser sees all documents
+        superuser = User.objects.create_superuser(username="superuser")
+        self.client.force_authenticate(user=superuser)
+        resp = self.client.get("/api/trash/")
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(resp.data["count"], 3)
+
    def test_api_trash_insufficient_permissions(self):
        """
        GIVEN:
@ -107,9 +159,6 @@ class TestTrashAPI(APITestCase):
            - 403 Forbidden
        """

-        user1 = User.objects.create_user(username="user1")
-        self.client.force_authenticate(user=user1)
-        self.client.force_login(user=user1)
        user2 = User.objects.create_user(username="user2")
        document = Document.objects.create(
            title="Title",
--- a/src/documents/views.py
+++ b/src/documents/views.py
@ -96,6 +96,7 @@ from documents.filters import CustomFieldFilterSet
 from documents.filters import DocumentFilterSet
 from documents.filters import DocumentTypeFilterSet
 from documents.filters import ObjectOwnedOrGrantedPermissionsFilter
+from documents.filters import ObjectOwnedPermissionsFilter
 from documents.filters import ShareLinkFilterSet
 from documents.filters import StoragePathFilterSet
 from documents.filters import TagFilterSet
@ -2060,7 +2061,7 @@ class SystemStatusView(PassUserMixin):
 class TrashView(ListModelMixin, PassUserMixin):
    permission_classes = (IsAuthenticated,)
    serializer_class = TrashSerializer
-    filter_backends = (ObjectOwnedOrGrantedPermissionsFilter,)
+    filter_backends = (ObjectOwnedPermissionsFilter,)
    pagination_class = StandardPagination

    model = Document
@ -2079,7 +2080,7 @@ class TrashView(ListModelMixin, PassUserMixin):
        docs = (
            Document.global_objects.filter(id__in=doc_ids)
            if doc_ids is not None
-            else Document.deleted_objects.all()
+            else self.filter_queryset(self.get_queryset()).all()
        )
        for doc in docs:
            if not has_perms_owner_aware(request.user, "delete_document", doc):