Skip to content

Commit f23a8cb

Browse files
Chartman123Chartman123
committed
fixup! refactor: streamline form update logic and enhance permission checks for archiving, locking, and ownership transfer
Signed-off-by: Christian Hartmann <chris-hartmann@gmx.de>
1 parent c46650a commit f23a8cb

2 files changed

Lines changed: 92 additions & 86 deletions

File tree

‎lib/Controller/ApiController.php‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -340,16 +340,22 @@ private function checkArchivePermission(Form $form, string $currentUserId, array
340340
$isArchived = $this->formsService->isFormArchived($form);
341341
$owner = $currentUserId === $form->getOwnerId();
342342
$onlyState = sizeof($keyValuePairs) === 1 && key_exists('state', $keyValuePairs);
343-
$archiving = $onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_ARCHIVED;
344-
$unarchiving = $onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED;
345343

346-
if (
347-
($isArchived && !($owner && $unarchiving)) ||
348-
(!$isArchived && !($owner && $archiving))
349-
) {
350-
$this->logger->debug('Only the form owner can set/unset the `archived` state');
351-
throw new OCSForbiddenException('Only the form owner can set/unset the `archived` state');
344+
// Only check if the request is trying to change the archived state
345+
if ($onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_ARCHIVED) {
346+
// Trying to archive
347+
if (!$owner || $isArchived) {
348+
$this->logger->debug('Only the form owner can archive the form, and only if it is not already archived');
349+
throw new OCSForbiddenException('Only the form owner can archive the form, and only if it is not already archived');
350+
}
351+
} elseif ($onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED) {
352+
// Trying to unarchive
353+
if (!$owner || !$isArchived) {
354+
$this->logger->debug('Only the form owner can unarchive the form, and only if it is currently archived');
355+
throw new OCSForbiddenException('Only the form owner can unarchive the form, and only if it is currently archived');
356+
}
352357
}
358+
// All other updates are allowed (including updates that do not touch the state)
353359
}
354360

355361
private function isLockingRequest(array $keyValuePairs): bool {

‎tests/Unit/Controller/ApiControllerTest.php‎

Lines changed: 78 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -468,9 +468,9 @@ public function testCloneForm_exceptions(bool $canCreate, $callback, string $exc
468468
$this->configService->expects($this->once())
469469
->method('canCreateForms')
470470
->willReturn($canCreate);
471-
$this->formMapper->expects($canCreate ? $this->once() : $this->never())
472-
->method('findById')
473-
->with(7)
471+
$this->formsService->expects($canCreate ? $this->once() : $this->never())
472+
->method('getFormIfAllowed')
473+
->with(7, Constants::PERMISSION_EDIT)
474474
->willReturnCallback($callback);
475475
$this->expectException($exception);
476476
$this->apiController->newForm(7);
@@ -686,8 +686,8 @@ public function testNewSubmission_answers() {
686686
5 => ['ignore unknown question'],
687687
];
688688

689-
$this->formMapper->expects($this->once())
690-
->method('findById')
689+
$this->formsService->expects($this->once())
690+
->method('loadFormForSubmission')
691691
->with(1)
692692
->willReturn($form);
693693

@@ -767,8 +767,8 @@ public function testNewSubmission_answers() {
767767

768768
public function testNewSubmission_formNotFound() {
769769
$exception = $this->createMock(MapperException::class);
770-
$this->formMapper->expects($this->once())
771-
->method('findById')
770+
$this->formsService->expects($this->once())
771+
->method('loadFormForSubmission')
772772
->with(1)
773773
->willThrowException($exception);
774774
$this->expectException(NoSuchFormException::class);
@@ -794,8 +794,8 @@ public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExp
794794
$form->setId(1);
795795
$form->setOwnerId('admin');
796796

797-
$this->formMapper->expects($this->once())
798-
->method('findById')
797+
$this->formsService->expects($this->once())
798+
->method('loadFormForSubmission')
799799
->with(1)
800800
->willReturn($form);
801801

@@ -811,8 +811,8 @@ public function testNewSubmission_validateSubmission() {
811811
$form->setId(1);
812812
$form->setOwnerId('admin');
813813

814-
$this->formMapper->expects($this->once())
815-
->method('findById')
814+
$this->formsService->expects($this->once())
815+
->method('loadFormForSubmission')
816816
->with(1)
817817
->willReturn($form);
818818

@@ -840,9 +840,9 @@ public function testDeleteSubmissionNotFound() {
840840
$form->setId(1);
841841
$form->setOwnerId('currentUser');
842842

843-
$this->formMapper->expects(self::once())
844-
->method('findById')
845-
->with(1)
843+
$this->formsService->expects(self::once())
844+
->method('getFormIfAllowed')
845+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
846846
->willReturn($form);
847847

848848
$this->formsService->expects(self::once())
@@ -873,9 +873,9 @@ public function testDeleteSubmissionNoPermission($submissionData, $formData) {
873873
->with(42)
874874
->willReturn($submission);
875875

876-
$this->formMapper
877-
->method('findById')
878-
->with(1)
876+
$this->formsService
877+
->method('getFormIfAllowed')
878+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
879879
->willReturn($form);
880880

881881
$this->formsService
@@ -900,9 +900,9 @@ public function testDeleteSubmission($submissionData, $formData) {
900900
->with(42)
901901
->willReturn($submission);
902902

903-
$this->formMapper
904-
->method('findById')
905-
->with(1)
903+
$this->formsService
904+
->method('getFormIfAllowed')
905+
->with(1, Constants::PERMISSION_RESULTS_DELETE)
906906
->willReturn($form);
907907

908908
$this->formsService
@@ -956,12 +956,12 @@ public function testTransferOwnerNotOwner() {
956956
$form->setHash('hash');
957957
$form->setOwnerId('otherUser');
958958

959-
$this->formMapper
960-
->method('findById')
961-
->with(1)
959+
$this->formsService
960+
->method('getFormIfAllowed')
961+
->with(1, Constants::PERMISSION_EDIT)
962962
->willReturn($form);
963963

964-
$this->expectException(NoSuchFormException::class);
964+
$this->expectException(OCSForbiddenException::class);
965965
$this->apiController->updateForm(1, ['ownerId' => 'newOwner']);
966966
}
967967

@@ -971,9 +971,9 @@ public function testTransferNewOwnerNotFound() {
971971
$form->setHash('hash');
972972
$form->setOwnerId('currentUser');
973973

974-
$this->formMapper
975-
->method('findById')
976-
->with(1)
974+
$this->formsService
975+
->method('getFormIfAllowed')
976+
->with(1, Constants::PERMISSION_EDIT)
977977
->willReturn($form);
978978

979979
$this->formsService
@@ -996,9 +996,9 @@ public function testTransferOwner() {
996996
$form->setHash('hash');
997997
$form->setOwnerId('currentUser');
998998

999-
$this->formMapper
1000-
->method('findById')
1001-
->with(1)
999+
$this->formsService
1000+
->method('getFormIfAllowed')
1001+
->with(1, Constants::PERMISSION_EDIT)
10021002
->willReturn($form);
10031003

10041004
$this->formsService
@@ -1018,9 +1018,9 @@ public function testTransferOwner() {
10181018

10191019
public function testGetSubmission_invalidForm() {
10201020
$exception = $this->createMock(MapperException::class);
1021-
$this->formMapper->expects($this->once())
1022-
->method('findById')
1023-
->with(1)
1021+
$this->formsService->expects($this->once())
1022+
->method('getFormIfAllowed')
1023+
->with(1, Constants::PERMISSION_RESULTS)
10241024
->willThrowException($exception);
10251025
$this->expectException(NoSuchFormException::class);
10261026
$this->apiController->getSubmission(1, 42);
@@ -1031,9 +1031,9 @@ public function testGetSubmission_noPermissions() {
10311031
$form->setId(1);
10321032
$form->setOwnerId('currentUser');
10331033

1034-
$this->formMapper->expects($this->once())
1035-
->method('findById')
1036-
->with(1)
1034+
$this->formsService->expects($this->once())
1035+
->method('getFormIfAllowed')
1036+
->with(1, Constants::PERMISSION_RESULTS)
10371037
->willReturn($form);
10381038

10391039
$this->formsService->expects($this->once())
@@ -1050,9 +1050,9 @@ public function testGetSubmission_notFound() {
10501050
$form->setId(1);
10511051
$form->setOwnerId('currentUser');
10521052

1053-
$this->formMapper->expects($this->once())
1054-
->method('findById')
1055-
->with(1)
1053+
$this->formsService->expects($this->once())
1054+
->method('getFormIfAllowed')
1055+
->with(1, Constants::PERMISSION_RESULTS)
10561056
->willReturn($form);
10571057

10581058
$this->formsService->expects($this->once())
@@ -1084,9 +1084,9 @@ public function testGetSubmission_success() {
10841084
'timestamp' => 1234567890
10851085
];
10861086

1087-
$this->formMapper->expects($this->once())
1088-
->method('findById')
1089-
->with(1)
1087+
$this->formsService->expects($this->once())
1088+
->method('getFormIfAllowed')
1089+
->with(1, Constants::PERMISSION_RESULTS)
10901090
->willReturn($form);
10911091

10921092
$this->formsService->expects($this->once())
@@ -1124,9 +1124,9 @@ public function testGetSubmission_mismatchedFormId() {
11241124
'timestamp' => 1234567890
11251125
];
11261126

1127-
$this->formMapper->expects($this->once())
1128-
->method('findById')
1129-
->with(1)
1127+
$this->formsService->expects($this->once())
1128+
->method('getFormIfAllowed')
1129+
->with(1, Constants::PERMISSION_RESULTS)
11301130
->willReturn($form);
11311131

11321132
$this->formsService->expects($this->once())
@@ -1157,9 +1157,9 @@ public function testGetSubmission_anonymousUser() {
11571157
'timestamp' => 1234567890
11581158
];
11591159

1160-
$this->formMapper->expects($this->once())
1161-
->method('findById')
1162-
->with(1)
1160+
$this->formsService->expects($this->once())
1161+
->method('getFormIfAllowed')
1162+
->with(1, Constants::PERMISSION_RESULTS)
11631163
->willReturn($form);
11641164

11651165
$this->formsService->expects($this->once())
@@ -1195,9 +1195,9 @@ public function testGetSubmission_userNotFound() {
11951195
'timestamp' => 1234567890
11961196
];
11971197

1198-
$this->formMapper->expects($this->once())
1199-
->method('findById')
1200-
->with(1)
1198+
$this->formsService->expects($this->once())
1199+
->method('getFormIfAllowed')
1200+
->with(1, Constants::PERMISSION_RESULTS)
12011201
->willReturn($form);
12021202

12031203
$this->formsService->expects($this->once())
@@ -1236,9 +1236,9 @@ public function testUpdateSubmission_success() {
12361236
$submission->setFormId($formId);
12371237
$submission->setUserId($userId);
12381238

1239-
$this->formMapper->expects($this->once())
1240-
->method('findById')
1241-
->with($formId)
1239+
$this->formsService->expects($this->once())
1240+
->method('loadFormForSubmission')
1241+
->with(1)
12421242
->willReturn($form);
12431243

12441244
$this->formsService->expects($this->once())
@@ -1293,9 +1293,9 @@ public function testUpdateSubmission_formNotEditable() {
12931293
$form->setOwnerId('formOwner');
12941294
$form->setAllowEditSubmissions(false); // Form does not allow edits
12951295

1296-
$this->formMapper->expects($this->once())
1297-
->method('findById')
1298-
->with($formId)
1296+
$this->formsService->expects($this->once())
1297+
->method('loadFormForSubmission')
1298+
->with(1)
12991299
->willReturn($form);
13001300

13011301
$this->formsService->expects($this->once())
@@ -1320,9 +1320,9 @@ public function testUpdateSubmission_invalidAnswers() {
13201320
$form->setOwnerId('formOwner');
13211321
$form->setAllowEditSubmissions(true);
13221322

1323-
$this->formMapper->expects($this->once())
1324-
->method('findById')
1325-
->with($formId)
1323+
$this->formsService->expects($this->once())
1324+
->method('loadFormForSubmission')
1325+
->with(1)
13261326
->willReturn($form);
13271327

13281328
$this->formsService->expects($this->once())
@@ -1356,9 +1356,9 @@ public function testUpdateSubmission_submissionNotFound() {
13561356
$form->setOwnerId('formOwner');
13571357
$form->setAllowEditSubmissions(true);
13581358

1359-
$this->formMapper->expects($this->once())
1360-
->method('findById')
1361-
->with($formId)
1359+
$this->formsService->expects($this->once())
1360+
->method('loadFormForSubmission')
1361+
->with(1)
13621362
->willReturn($form);
13631363

13641364
$this->formsService->expects($this->once())
@@ -1402,9 +1402,9 @@ public function testUpdateSubmission_mismatchedFormId() {
14021402
$submission->setFormId(2); // Belongs to a different form
14031403
$submission->setUserId('currentUser');
14041404

1405-
$this->formMapper->expects($this->once())
1406-
->method('findById')
1407-
->with($formId)
1405+
$this->formsService->expects($this->once())
1406+
->method('loadFormForSubmission')
1407+
->with(1)
14081408
->willReturn($form);
14091409

14101410
$this->formsService->expects($this->once())
@@ -1448,9 +1448,9 @@ public function testUpdateSubmission_notOwnSubmission() {
14481448
$submission->setFormId($formId);
14491449
$submission->setUserId('anotherUser'); // Submission belongs to another user
14501450

1451-
$this->formMapper->expects($this->once())
1452-
->method('findById')
1453-
->with($formId)
1451+
$this->formsService->expects($this->once())
1452+
->method('loadFormForSubmission')
1453+
->with(1)
14541454
->willReturn($form);
14551455

14561456
$this->formsService->expects($this->once())
@@ -1484,9 +1484,9 @@ public function testUpdateSubmission_loadForm_notFound() {
14841484
$submissionId = 42;
14851485
$answers = ['q1' => ['answer1']];
14861486

1487-
$this->formMapper->expects($this->once())
1488-
->method('findById')
1489-
->with($formId)
1487+
$this->formsService->expects($this->once())
1488+
->method('loadFormForSubmission')
1489+
->with(1)
14901490
->willThrowException(new DoesNotExistException('Form not found by mapper')); // Corrected Exception
14911491

14921492
$this->expectException(NoSuchFormException::class);
@@ -1502,9 +1502,9 @@ public function testUpdateSubmission_loadForm_noAccess_noShare() {
15021502
$form = new Form();
15031503
$form->setId($formId);
15041504

1505-
$this->formMapper->expects($this->once())
1506-
->method('findById')
1507-
->with($formId)
1505+
$this->formsService->expects($this->once())
1506+
->method('loadFormForSubmission')
1507+
->with(1)
15081508
->willReturn($form);
15091509

15101510
// No share hash provided, so hasUserAccess will be checked
@@ -1526,9 +1526,9 @@ public function testUpdateSubmission_loadForm_formExpired() {
15261526
$form = new Form();
15271527
$form->setId($formId);
15281528

1529-
$this->formMapper->expects($this->once())
1530-
->method('findById')
1531-
->with($formId)
1529+
$this->formsService->expects($this->once())
1530+
->method('loadFormForSubmission')
1531+
->with(1)
15321532
->willReturn($form);
15331533

15341534
// Assuming access is granted (e.g., via user access or a valid share)

0 commit comments

Comments
 (0)