From df86882e8ed2ac1558c55cf783fda72e0ab32b15 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Tue, 7 Oct 2025 00:56:16 -0700 Subject: [PATCH] Fix: require only change permissions for task dismissal, add frontend error handling (#11023) --- .../admin/tasks/tasks.component.spec.ts | 40 +++++++++++++++++++ .../components/admin/tasks/tasks.component.ts | 14 ++++++- src-ui/src/app/services/tasks.service.spec.ts | 2 +- src-ui/src/app/services/tasks.service.ts | 15 ++++--- src/documents/permissions.py | 18 +++++++++ src/documents/tests/test_api_tasks.py | 38 ++++++++++++++++++ src/documents/views.py | 7 +++- 7 files changed, 124 insertions(+), 10 deletions(-) diff --git a/src-ui/src/app/components/admin/tasks/tasks.component.spec.ts b/src-ui/src/app/components/admin/tasks/tasks.component.spec.ts index 8158be7b2..1a085150e 100644 --- a/src-ui/src/app/components/admin/tasks/tasks.component.spec.ts +++ b/src-ui/src/app/components/admin/tasks/tasks.component.spec.ts @@ -16,6 +16,7 @@ import { NgbNavItem, } from '@ng-bootstrap/ng-bootstrap' import { allIcons, NgxBootstrapIconsModule } from 'ngx-bootstrap-icons' +import { throwError } from 'rxjs' import { routes } from 'src/app/app-routing.module' import { PaperlessTask, @@ -28,6 +29,7 @@ import { PermissionsGuard } from 'src/app/guards/permissions.guard' import { CustomDatePipe } from 'src/app/pipes/custom-date.pipe' import { PermissionsService } from 'src/app/services/permissions.service' import { TasksService } from 'src/app/services/tasks.service' +import { ToastService } from 'src/app/services/toast.service' import { environment } from 'src/environments/environment' import { ConfirmDialogComponent } from '../../common/confirm-dialog/confirm-dialog.component' import { PageHeaderComponent } from '../../common/page-header/page-header.component' @@ -123,6 +125,7 @@ describe('TasksComponent', () => { let router: Router let httpTestingController: HttpTestingController let reloadSpy + let toastService: ToastService beforeEach(async () => { TestBed.configureTestingModule({ @@ -157,6 +160,7 @@ describe('TasksComponent', () => { httpTestingController = TestBed.inject(HttpTestingController) modalService = TestBed.inject(NgbModal) router = TestBed.inject(Router) + toastService = TestBed.inject(ToastService) fixture = TestBed.createComponent(TasksComponent) component = fixture.componentInstance jest.useFakeTimers() @@ -249,6 +253,42 @@ describe('TasksComponent', () => { expect(dismissSpy).toHaveBeenCalledWith(selected) }) + it('should show an error and re-enable modal buttons when dismissing multiple tasks fails', () => { + component.selectedTasks = new Set([tasks[0].id, tasks[1].id]) + const error = new Error('dismiss failed') + const toastSpy = jest.spyOn(toastService, 'showError') + const dismissSpy = jest + .spyOn(tasksService, 'dismissTasks') + .mockReturnValue(throwError(() => error)) + + let modal: NgbModalRef + modalService.activeInstances.subscribe((m) => (modal = m[m.length - 1])) + + component.dismissTasks() + expect(modal).not.toBeUndefined() + + modal.componentInstance.confirmClicked.emit() + + expect(dismissSpy).toHaveBeenCalledWith(new Set([tasks[0].id, tasks[1].id])) + expect(toastSpy).toHaveBeenCalledWith('Error dismissing tasks', error) + expect(modal.componentInstance.buttonsEnabled).toBe(true) + expect(component.selectedTasks.size).toBe(0) + }) + + it('should show an error when dismissing a single task fails', () => { + const error = new Error('dismiss failed') + const toastSpy = jest.spyOn(toastService, 'showError') + const dismissSpy = jest + .spyOn(tasksService, 'dismissTasks') + .mockReturnValue(throwError(() => error)) + + component.dismissTask(tasks[0]) + + expect(dismissSpy).toHaveBeenCalledWith(new Set([tasks[0].id])) + expect(toastSpy).toHaveBeenCalledWith('Error dismissing task', error) + expect(component.selectedTasks.size).toBe(0) + }) + it('should support dismiss all tasks', () => { let modal: NgbModalRef modalService.activeInstances.subscribe((m) => (modal = m[m.length - 1])) diff --git a/src-ui/src/app/components/admin/tasks/tasks.component.ts b/src-ui/src/app/components/admin/tasks/tasks.component.ts index eb7263137..6f144c58c 100644 --- a/src-ui/src/app/components/admin/tasks/tasks.component.ts +++ b/src-ui/src/app/components/admin/tasks/tasks.component.ts @@ -24,6 +24,7 @@ import { PaperlessTask } from 'src/app/data/paperless-task' import { IfPermissionsDirective } from 'src/app/directives/if-permissions.directive' import { CustomDatePipe } from 'src/app/pipes/custom-date.pipe' import { TasksService } from 'src/app/services/tasks.service' +import { ToastService } from 'src/app/services/toast.service' import { ConfirmDialogComponent } from '../../common/confirm-dialog/confirm-dialog.component' import { PageHeaderComponent } from '../../common/page-header/page-header.component' import { LoadingComponentWithPermissions } from '../../loading-component/loading.component' @@ -72,6 +73,7 @@ export class TasksComponent tasksService = inject(TasksService) private modalService = inject(NgbModal) private readonly router = inject(Router) + private readonly toastService = inject(ToastService) public activeTab: TaskTab public selectedTasks: Set = new Set() @@ -154,11 +156,19 @@ export class TasksComponent modal.componentInstance.confirmClicked.pipe(first()).subscribe(() => { modal.componentInstance.buttonsEnabled = false modal.close() - this.tasksService.dismissTasks(tasks) + this.tasksService.dismissTasks(tasks).subscribe({ + error: (e) => { + this.toastService.showError($localize`Error dismissing tasks`, e) + modal.componentInstance.buttonsEnabled = true + }, + }) this.clearSelection() }) } else { - this.tasksService.dismissTasks(tasks) + this.tasksService.dismissTasks(tasks).subscribe({ + error: (e) => + this.toastService.showError($localize`Error dismissing task`, e), + }) this.clearSelection() } } diff --git a/src-ui/src/app/services/tasks.service.spec.ts b/src-ui/src/app/services/tasks.service.spec.ts index 0d4c8ee01..640f84587 100644 --- a/src-ui/src/app/services/tasks.service.spec.ts +++ b/src-ui/src/app/services/tasks.service.spec.ts @@ -51,7 +51,7 @@ describe('TasksService', () => { }) it('calls acknowledge_tasks api endpoint on dismiss and reloads', () => { - tasksService.dismissTasks(new Set([1, 2, 3])) + tasksService.dismissTasks(new Set([1, 2, 3])).subscribe() const req = httpTestingController.expectOne( `${environment.apiBaseUrl}tasks/acknowledge/` ) diff --git a/src-ui/src/app/services/tasks.service.ts b/src-ui/src/app/services/tasks.service.ts index d3a5224a1..305258d7b 100644 --- a/src-ui/src/app/services/tasks.service.ts +++ b/src-ui/src/app/services/tasks.service.ts @@ -1,7 +1,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable, inject } from '@angular/core' import { Observable, Subject } from 'rxjs' -import { first, takeUntil } from 'rxjs/operators' +import { first, takeUntil, tap } from 'rxjs/operators' import { PaperlessTask, PaperlessTaskName, @@ -68,14 +68,17 @@ export class TasksService { } public dismissTasks(task_ids: Set) { - this.http + return this.http .post(`${this.baseUrl}tasks/acknowledge/`, { tasks: [...task_ids], }) - .pipe(first()) - .subscribe((r) => { - this.reload() - }) + .pipe( + first(), + takeUntil(this.unsubscribeNotifer), + tap(() => { + this.reload() + }) + ) } public cancelPending(): void { diff --git a/src/documents/permissions.py b/src/documents/permissions.py index c5de34607..797d92ed9 100644 --- a/src/documents/permissions.py +++ b/src/documents/permissions.py @@ -161,3 +161,21 @@ class PaperlessNotePermissions(BasePermission): perms = self.perms_map[request.method] return request.user.has_perms(perms) + + +class AcknowledgeTasksPermissions(BasePermission): + """ + Permissions class that checks for model permissions for acknowledging tasks. + """ + + perms_map = { + "POST": ["documents.change_paperlesstask"], + } + + def has_permission(self, request, view): + if not request.user or not request.user.is_authenticated: # pragma: no cover + return False + + perms = self.perms_map.get(request.method, []) + + return request.user.has_perms(perms) diff --git a/src/documents/tests/test_api_tasks.py b/src/documents/tests/test_api_tasks.py index c139d05da..aa42577c4 100644 --- a/src/documents/tests/test_api_tasks.py +++ b/src/documents/tests/test_api_tasks.py @@ -135,6 +135,44 @@ class TestTasks(DirectoriesMixin, APITestCase): response = self.client.get(self.ENDPOINT + "?acknowledged=false") self.assertEqual(len(response.data), 0) + def test_acknowledge_tasks_requires_change_permission(self): + """ + GIVEN: + - A regular user initially without change permissions + - A regular user with change permissions + WHEN: + - API call is made to acknowledge tasks + THEN: + - The first user is forbidden from acknowledging tasks + - The second user is allowed to acknowledge tasks + """ + regular_user = User.objects.create_user(username="test") + self.client.force_authenticate(user=regular_user) + + task = PaperlessTask.objects.create( + task_id=str(uuid.uuid4()), + task_file_name="task_one.pdf", + ) + + response = self.client.post( + self.ENDPOINT + "acknowledge/", + {"tasks": [task.id]}, + ) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + regular_user2 = User.objects.create_user(username="test2") + regular_user2.user_permissions.add( + Permission.objects.get(codename="change_paperlesstask"), + ) + regular_user2.save() + self.client.force_authenticate(user=regular_user2) + + response = self.client.post( + self.ENDPOINT + "acknowledge/", + {"tasks": [task.id]}, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_tasks_owner_aware(self): """ GIVEN: diff --git a/src/documents/views.py b/src/documents/views.py index cf103f9a7..efc6896e7 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -136,6 +136,7 @@ from documents.models import WorkflowAction from documents.models import WorkflowTrigger from documents.parsers import get_parser_class_for_mime_type from documents.parsers import parse_date_generator +from documents.permissions import AcknowledgeTasksPermissions from documents.permissions import PaperlessAdminPermissions from documents.permissions import PaperlessNotePermissions from documents.permissions import PaperlessObjectPermissions @@ -2487,7 +2488,11 @@ class TasksViewSet(ReadOnlyModelViewSet): queryset = PaperlessTask.objects.filter(task_id=task_id) return queryset - @action(methods=["post"], detail=False) + @action( + methods=["post"], + detail=False, + permission_classes=[IsAuthenticated, AcknowledgeTasksPermissions], + ) def acknowledge(self, request): serializer = AcknowledgeTasksViewSerializer(data=request.data) serializer.is_valid(raise_exception=True)