diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index a777751e4b..b5f0b13c95 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -964,11 +964,6 @@ public function insertSubmission(int $formId, array $answers, string $shareHash throw new OCSForbiddenException('This form is no longer taking answers'); } - // Does the user have permissions to submit - if (!$this->formsService->canSubmit($form)) { - throw new OCSForbiddenException('Already submitted'); - } - // Is the submission valid if (!$this->submissionService->validateSubmission($questions, $answers)) { throw new OCSBadRequestException('At least one submitted answer is not valid'); @@ -987,10 +982,23 @@ public function insertSubmission(int $formId, array $answers, string $shareHash $submission->setUserId($this->currentUser->getUID()); } + // Does the user have permissions to submit + // This is done right before insert so we minimize race conditions for submitting on unique-submission forms + if (!$this->formsService->canSubmit($form)) { + throw new OCSForbiddenException('Already submitted'); + } + // Insert new submission $this->submissionMapper->insert($submission); $submissionId = $submission->getId(); + // Ensure the form is unique if needed. + // If we can not submit anymore then the submission must be unique + if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) { + $this->submissionMapper->delete($submission); + throw new OCSForbiddenException('Already submitted'); + } + // Process Answers foreach ($answers as $questionId => $answerArray) { // Search corresponding Question, skip processing if not found diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index 2f61184f94..4cd91501aa 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -37,6 +37,7 @@ use OCA\Forms\Db\AnswerMapper; use OCA\Forms\Db\FormMapper; use OCA\Forms\Db\QuestionMapper; +use OCA\Forms\Db\Submission; use OCA\Forms\Db\SubmissionMapper; use OCP\AppFramework\Db\DoesNotExistException; use OCP\Files\File; @@ -153,6 +154,19 @@ public function getSubmissions(int $formId): array { } } + /** + * Validate the new submission is unique + */ + public function isUniqueSubmission(Submission $newSubmission): bool { + $submissions = $this->submissionMapper->findByForm($newSubmission->getFormId()); + foreach ($submissions as $submission) { + if ($submission->getUserId() === $newSubmission->getUserId() && $submission->getId() !== $newSubmission->getId()) { + return false; + } + } + return true; + } + /** * Export Submissions to Cloud-Filesystem * @param string $hash of the form diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index cc8f571ca5..97d213f022 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -128,6 +128,71 @@ public function setUp(): void { ); } + /** + * @dataProvider dataIsUniqueSubmission + */ + public function testIsUniqueSubmission(array $submissionData, array $otherSubmissionsData, bool $expected) { + $this->submissionMapper->method('findByForm') + ->willReturn(array_map(fn($data) => Submission::fromParams($data), $otherSubmissionsData)); + + $submission = Submission::fromParams($submissionData); + $this->assertEquals($expected, $this->submissionService->isUniqueSubmission($submission)); + } + + public function dataIsUniqueSubmission() { + return [ + [ + 'submissionData' => [ + 'id' => 3, + 'userId' => 'user', + 'formId' => 1, + ], + 'otherSubmissionData' => [ + [ + 'id' => 1, + 'userId' => 'other', + 'formId' => 1, + ], + [ + 'id' => 2, + 'userId' => 'yetAnother', + 'formId' => 1, + ], + ], + 'expected' => true, + ], + [ + 'submissionData' => [ + 'id' => 1, + 'userId' => 'user', + 'formId' => 1, + ], + 'otherSubmissionData' => [], + 'expected' => true, + ], + [ + 'submissionData' => [ + 'id' => 3, + 'userId' => 'user', + 'formId' => 1, + ], + 'otherSubmissionData' => [ + [ + 'id' => 1, + 'userId' => 'other', + 'formId' => 1, + ], + [ + 'id' => 2, + 'formId' => 1, + 'userId' => 'user', // conflict + ], + ], + 'expected' => false, + ], + ]; + } + public function testGetSubmissions() { $submission_1 = new Submission(); $submission_1->setId(42);