From ac0ed0def8ed1b7f6df0138598098e31538dc78d Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 27 Jun 2024 13:33:39 -0700 Subject: [PATCH] Fix: handle errors for trash actions and only show documents user can restore or delete (#7119) --- src-ui/messages.xlf | 57 +++++++++++++----- .../admin/trash/trash.component.spec.ts | 45 ++++++++++++-- .../components/admin/trash/trash.component.ts | 60 ++++++++++++++----- src/documents/filters.py | 14 +++++ src/documents/tests/test_api_trash.py | 57 ++++++++++++++++-- src/documents/views.py | 5 +- 6 files changed, 195 insertions(+), 43 deletions(-) diff --git a/src-ui/messages.xlf b/src-ui/messages.xlf index 492c160c9..60ef72870 100644 --- a/src-ui/messages.xlf +++ b/src-ui/messages.xlf @@ -1437,7 +1437,7 @@ src/app/components/admin/trash/trash.component.ts - 80 + 86 src/app/components/admin/users-groups/users-groups.component.html @@ -2153,7 +2153,7 @@ src/app/components/admin/trash/trash.component.ts - 74 + 80 src/app/components/manage/management-list/management-list.component.ts @@ -2179,7 +2179,7 @@ src/app/components/admin/trash/trash.component.ts - 78 + 84 src/app/components/admin/users-groups/users-groups.component.ts @@ -2214,42 +2214,74 @@ Document deleted src/app/components/admin/trash/trash.component.ts - 63 + 64 + + + + Error deleting document + + src/app/components/admin/trash/trash.component.ts + 69 + + + src/app/components/document-detail/document-detail.component.ts + 799 This operation will permanently delete the selected documents. src/app/components/admin/trash/trash.component.ts - 76 + 82 This operation will permanently delete all documents in the trash. src/app/components/admin/trash/trash.component.ts - 77 + 83 Document(s) deleted src/app/components/admin/trash/trash.component.ts - 87 + 94 + + + + Error deleting document(s) + + src/app/components/admin/trash/trash.component.ts + 101 Document restored src/app/components/admin/trash/trash.component.ts - 97 + 113 + + + + Error restoring document + + src/app/components/admin/trash/trash.component.ts + 117 Document(s) restored src/app/components/admin/trash/trash.component.ts - 106 + 127 + + + + Error restoring document(s) + + src/app/components/admin/trash/trash.component.ts + 133 @@ -6073,13 +6105,6 @@ 716 - - Error deleting document - - src/app/components/document-detail/document-detail.component.ts - 799 - - Reprocess confirm diff --git a/src-ui/src/app/components/admin/trash/trash.component.spec.ts b/src-ui/src/app/components/admin/trash/trash.component.spec.ts index 57d4b4237..c9e797a1f 100644 --- 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 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() diff --git a/src-ui/src/app/components/admin/trash/trash.component.ts b/src-ui/src/app/components/admin/trash/trash.component.ts index b867f1706..cf807e831 100644 --- 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 = 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 + ) + }, }) } diff --git a/src/documents/filters.py b/src/documents/filters.py index c548cfa22..2c8baa62f 100644 --- 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 diff --git a/src/documents/tests/test_api_trash.py b/src/documents/tests/test_api_trash.py index 90d78610f..ab4e96773 100644 --- 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", diff --git a/src/documents/views.py b/src/documents/views.py index 7457ce12f..2862809f9 100644 --- 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):