From 6892ae56733a82955163b2b09c2507c59a5b90a2 Mon Sep 17 00:00:00 2001 From: "Peter R." Date: Tue, 14 Apr 2026 09:47:00 +0200 Subject: [PATCH] fix: require explicit share permission for bulk submission endpoints (#3290) * fix: require explicit share permission for bulk submission actions Add permission guard to deleteAllSubmissions() to verify the user holds the required share-level permission, matching the pattern already used by the single-item endpoint. -e Signed-off-by: Peter Ringelmann * PR feedback -e Signed-off-by: Peter Ringelmann --------- Signed-off-by: Peter Ringelmann --- lib/Controller/ApiController.php | 8 ++ tests/Unit/Controller/ApiControllerTest.php | 118 ++++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 09bc3cb9c..bb1b1b555 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1316,6 +1316,14 @@ public function getSubmission(int $formId, int $submissionId): DataResponse|Data public function deleteAllSubmissions(int $formId): DataResponse { $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); + // Require explicit results_delete permission for bulk deletion. + // canDeleteResults() also returns true for submitters with allowEditSubmissions, + // but those users should only be able to delete their own submissions individually. + if (!in_array(Constants::PERMISSION_RESULTS_DELETE, $this->formsService->getPermissions($form))) { + $this->logger->debug('User lacks results_delete permission for bulk deletion'); + throw new OCSForbiddenException('No permission to delete all submissions'); + } + // Delete all submissions (incl. Answers) $this->submissionMapper->deleteByForm($formId); $this->formMapper->update($form); diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 453e81adc..eed2cb106 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -1030,6 +1030,124 @@ public function dataTestDeletePermission() { ]; } + /** + * Test that a submitter with allowEditSubmissions but without + * PERMISSION_RESULTS_DELETE cannot bulk-delete all submissions. + */ + public function testDeleteAllSubmissionsNoPermission(): void { + $form = Form::fromParams([ + 'id' => 1, + 'title' => 'Test Form', + 'hash' => 'hash', + 'access' => [ + 'permitAllUsers' => false, + 'showToAllUsers' => false, + ], + 'ownerId' => 'otherUser', + 'description' => '', + 'expires' => 0, + 'isAnonymous' => false, + 'submitMultiple' => false, + 'showExpiration' => false, + ]); + + // getFormIfAllowed passes (canDeleteResults returns true due to allowEditSubmissions) + $this->formsService + ->method('getFormIfAllowed') + ->with(1, Constants::PERMISSION_RESULTS_DELETE) + ->willReturn($form); + + // But user only has submit permission, not results_delete + $this->formsService + ->method('getPermissions') + ->with($form) + ->willReturn([Constants::PERMISSION_SUBMIT]); + + // Bulk delete must NOT be called + $this->submissionMapper + ->expects($this->never()) + ->method('deleteByForm'); + + $this->expectException(OCSForbiddenException::class); + $this->apiController->deleteAllSubmissions(1); + } + + /** + * Test that the form owner can bulk-delete all submissions. + */ + public function testDeleteAllSubmissionsAsOwner(): void { + $form = Form::fromParams([ + 'id' => 1, + 'title' => 'Test Form', + 'hash' => 'hash', + 'access' => [ + 'permitAllUsers' => false, + 'showToAllUsers' => false, + ], + 'ownerId' => 'currentUser', + 'description' => '', + 'expires' => 0, + 'isAnonymous' => false, + 'submitMultiple' => false, + 'showExpiration' => false, + ]); + + $this->formsService + ->method('getFormIfAllowed') + ->with(1, Constants::PERMISSION_RESULTS_DELETE) + ->willReturn($form); + + $this->formsService + ->method('getPermissions') + ->with($form) + ->willReturn(Constants::PERMISSION_ALL); + + $this->submissionMapper + ->expects($this->once()) + ->method('deleteByForm') + ->with(1); + + $this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1)); + } + + /** + * Test that a collaborator with PERMISSION_RESULTS_DELETE can bulk-delete. + */ + public function testDeleteAllSubmissionsWithResultsDeletePermission(): void { + $form = Form::fromParams([ + 'id' => 1, + 'title' => 'Test Form', + 'hash' => 'hash', + 'access' => [ + 'permitAllUsers' => false, + 'showToAllUsers' => false, + ], + 'ownerId' => 'otherUser', + 'description' => '', + 'expires' => 0, + 'isAnonymous' => false, + 'submitMultiple' => false, + 'showExpiration' => false, + ]); + + $this->formsService + ->method('getFormIfAllowed') + ->with(1, Constants::PERMISSION_RESULTS_DELETE) + ->willReturn($form); + + $this->formsService + ->method('getPermissions') + ->with($form) + ->willReturn([Constants::PERMISSION_RESULTS_DELETE, Constants::PERMISSION_SUBMIT]); + + $this->submissionMapper + ->expects($this->once()) + ->method('deleteByForm') + ->with(1); + + $this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1)); + } + public function testTransferOwnerNotOwner() { $form = new Form(); $form->setId(1);