fix: Prevent race condition on unique-submission forms

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
nextcloud · Dec 21, 2023 · a44fcdd · a44fcdd
1 parent 9d065c1
commit a44fcdd
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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);