Skip to content

Commit d237027

Browse files
pringelmannpringelmann
authored
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 <peter.ringelmann@nextcloud.com> * PR feedback -e Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com> --------- Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
1 parent f8567a5 commit d237027

2 files changed

Lines changed: 126 additions & 0 deletions

File tree

‎lib/Controller/ApiController.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1322,6 +1322,14 @@ public function getSubmission(int $formId, int $submissionId): DataResponse|Data
13221322
public function deleteAllSubmissions(int $formId): DataResponse {
13231323
$form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE);
13241324

1325+
// Require explicit results_delete permission for bulk deletion.
1326+
// canDeleteResults() also returns true for submitters with allowEditSubmissions,
1327+
// but those users should only be able to delete their own submissions individually.
1328+
if (!in_array(Constants::PERMISSION_RESULTS_DELETE, $this->formsService->getPermissions($form))) {
1329+
$this->logger->debug('User lacks results_delete permission for bulk deletion');
1330+
throw new OCSForbiddenException('No permission to delete all submissions');
1331+
}
1332+
13251333
// Delete all submissions (incl. Answers)
13261334
$this->submissionMapper->deleteByForm($formId);
13271335
$this->formMapper->update($form);

‎tests/Unit/Controller/ApiControllerTest.php‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,124 @@ public function dataTestDeletePermission() {
10311031
];
10321032
}
10331033

1034+
/**
1035+
* Test that a submitter with allowEditSubmissions but without
1036+
* PERMISSION_RESULTS_DELETE cannot bulk-delete all submissions.
1037+
*/
1038+
public function testDeleteAllSubmissionsNoPermission(): void {
1039+
$form = Form::fromParams([
1040+
'id' => 1,
1041+
'title' => 'Test Form',
1042+
'hash' => 'hash',
1043+
'access' => [
1044+
'permitAllUsers' => false,
1045+
'showToAllUsers' => false,
1046+
],
1047+
'ownerId' => 'otherUser',
1048+
'description' => '',
1049+
'expires' => 0,
1050+
'isAnonymous' => false,
1051+
'submitMultiple' => false,
1052+
'showExpiration' => false,
1053+
]);
1054+
1055+
// getFormIfAllowed passes (canDeleteResults returns true due to allowEditSubmissions)
1056+
$this->formsService
1057+
->method('getFormIfAllowed')
1058+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
1059+
->willReturn($form);
1060+
1061+
// But user only has submit permission, not results_delete
1062+
$this->formsService
1063+
->method('getPermissions')
1064+
->with($form)
1065+
->willReturn([Constants::PERMISSION_SUBMIT]);
1066+
1067+
// Bulk delete must NOT be called
1068+
$this->submissionMapper
1069+
->expects($this->never())
1070+
->method('deleteByForm');
1071+
1072+
$this->expectException(OCSForbiddenException::class);
1073+
$this->apiController->deleteAllSubmissions(1);
1074+
}
1075+
1076+
/**
1077+
* Test that the form owner can bulk-delete all submissions.
1078+
*/
1079+
public function testDeleteAllSubmissionsAsOwner(): void {
1080+
$form = Form::fromParams([
1081+
'id' => 1,
1082+
'title' => 'Test Form',
1083+
'hash' => 'hash',
1084+
'access' => [
1085+
'permitAllUsers' => false,
1086+
'showToAllUsers' => false,
1087+
],
1088+
'ownerId' => 'currentUser',
1089+
'description' => '',
1090+
'expires' => 0,
1091+
'isAnonymous' => false,
1092+
'submitMultiple' => false,
1093+
'showExpiration' => false,
1094+
]);
1095+
1096+
$this->formsService
1097+
->method('getFormIfAllowed')
1098+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
1099+
->willReturn($form);
1100+
1101+
$this->formsService
1102+
->method('getPermissions')
1103+
->with($form)
1104+
->willReturn(Constants::PERMISSION_ALL);
1105+
1106+
$this->submissionMapper
1107+
->expects($this->once())
1108+
->method('deleteByForm')
1109+
->with(1);
1110+
1111+
$this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1));
1112+
}
1113+
1114+
/**
1115+
* Test that a collaborator with PERMISSION_RESULTS_DELETE can bulk-delete.
1116+
*/
1117+
public function testDeleteAllSubmissionsWithResultsDeletePermission(): void {
1118+
$form = Form::fromParams([
1119+
'id' => 1,
1120+
'title' => 'Test Form',
1121+
'hash' => 'hash',
1122+
'access' => [
1123+
'permitAllUsers' => false,
1124+
'showToAllUsers' => false,
1125+
],
1126+
'ownerId' => 'otherUser',
1127+
'description' => '',
1128+
'expires' => 0,
1129+
'isAnonymous' => false,
1130+
'submitMultiple' => false,
1131+
'showExpiration' => false,
1132+
]);
1133+
1134+
$this->formsService
1135+
->method('getFormIfAllowed')
1136+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
1137+
->willReturn($form);
1138+
1139+
$this->formsService
1140+
->method('getPermissions')
1141+
->with($form)
1142+
->willReturn([Constants::PERMISSION_RESULTS_DELETE, Constants::PERMISSION_SUBMIT]);
1143+
1144+
$this->submissionMapper
1145+
->expects($this->once())
1146+
->method('deleteByForm')
1147+
->with(1);
1148+
1149+
$this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1));
1150+
}
1151+
10341152
public function testTransferOwnerNotOwner() {
10351153
$form = new Form();
10361154
$form->setId(1);

0 commit comments

Comments
 (0)