diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 4df9116bc..5c3e8a801 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1055,11 +1055,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'); @@ -1078,10 +1073,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/Db/SubmissionMapper.php b/lib/Db/SubmissionMapper.php index ed5d0c03d..d5e5d2a76 100644 --- a/lib/Db/SubmissionMapper.php +++ b/lib/Db/SubmissionMapper.php @@ -110,16 +110,22 @@ public function findParticipantsByForm(int $formId): array { } /** + * Count submissions by form and optionally also by userId + * @param int $formId ID of the form to count submissions for + * @param string|null $userId optionally limit submissions to the one of that user * @throws \Exception */ - public function countSubmissions(int $formId): int { + public function countSubmissions(int $formId, ?string $userId = null): int { $qb = $this->db->getQueryBuilder(); - $qb->select($qb->func()->count('*', 'num_submissions')) + $query = $qb->select($qb->func()->count('*', 'num_submissions')) ->from($this->getTableName()) ->where($qb->expr()->eq('form_id', $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT))); + if (!is_null($userId)) { + $query->andWhere($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))); + } - $result = $qb->executeQuery(); + $result = $query->executeQuery(); $row = $result->fetch(); $result->closeCursor(); diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index 2f61184f9..538211bec 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,13 @@ public function getSubmissions(int $formId): array { } } + /** + * Validate the new submission is unique + */ + public function isUniqueSubmission(Submission $newSubmission): bool { + return $this->submissionMapper->countSubmissions($newSubmission->getFormId(), $newSubmission->getUserId()) === 1; + } + /** * Export Submissions to Cloud-Filesystem * @param string $hash of the form diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 47de63b8c..d6d6ec443 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -713,6 +713,10 @@ public function testInsertSubmission_forbiddenException($hasUserAccess, $hasForm ->with(1) ->willReturn($form); + $this->submissionService + ->method('validateSubmission') + ->willReturn(true); + $this->formAccess($hasUserAccess, $hasFormExpired, $canSubmit); $this->expectException(OCSForbiddenException::class); diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index cc8f571ca..582dd28e5 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -128,6 +128,41 @@ public function setUp(): void { ); } + /** + * @dataProvider dataIsUniqueSubmission + */ + public function testIsUniqueSubmission(array $submissionData, int $numberOfSubmissions, bool $expected) { + $this->submissionMapper->method('countSubmissions') + ->with($submissionData['formId'], $submissionData['userId']) + ->willReturn($numberOfSubmissions); + + $submission = Submission::fromParams($submissionData); + $this->assertEquals($expected, $this->submissionService->isUniqueSubmission($submission)); + } + + public function dataIsUniqueSubmission() { + return [ + [ + 'submissionData' => [ + 'id' => 1, + 'userId' => 'user', + 'formId' => 1, + ], + 'numberOfSubmissions' => 1, + 'expected' => true, + ], + [ + 'submissionData' => [ + 'id' => 3, + 'userId' => 'user', + 'formId' => 1, + ], + 'numberOfSubmissions' => 2, + 'expected' => false, + ], + ]; + } + public function testGetSubmissions() { $submission_1 = new Submission(); $submission_1->setId(42);