fix: Prevent race condition on unique-submission forms

lib/Controller/ApiController.php

@@ -1055,11 +1055,6 @@
 			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 @@
 			$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

lib/Db/SubmissionMapper.php

@@ -110,16 +110,22 @@
 	}
 
 	/**
+	 * 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();
 

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

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);

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);