fix(backend): conflicts were not detected on sync anymore

Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Jul 16, 2023 · 75bdd5c · 75bdd5c
1 parent f7107a5
commit 75bdd5c
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 16 deletions.
diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php
@@ -37,6 +37,7 @@
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\Constants;
+use OCP\Files\InvalidPathException;
 use OCP\Files\Lock\ILock;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
@@ -220,8 +221,9 @@ public function sync(Session $session, Document $document, $version = 0, ?string
 			];
 
 			// ensure file is still present and accessible
-			$this->documentService->getFileForSession($session, $shareToken);
-		} catch (NotFoundException $e) {
+			$file = $this->documentService->getFileForSession($session, $shareToken);
+			$this->documentService->assertNoOutsideConflict($document, $file);
+		} catch (NotFoundException|InvalidPathException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'File not found'
@@ -231,9 +233,15 @@ public function sync(Session $session, Document $document, $version = 0, ?string
 			return new DataResponse([
 				'message' => 'Document no longer exists'
 			], 404);
+		} catch (DocumentSaveConflictException) {
+			try {
+				$result['outsideChange'] = $file->getContent();
+			} catch (LockedException) {
+				// Ignore locked exception since it might happen due to an autosave action happening at the same time
+			}
 		}
 
-		return new DataResponse($result, 200);
+		return new DataResponse($result, isset($result['outsideChange']) ? 409 : 200);
 	}
 
 	public function save(Session $session, Document $document, $version = 0, $autosaveContent = null, $documentState = null, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse {
@@ -252,7 +260,7 @@ public function save(Session $session, Document $document, $version = 0, $autosa
 		}
 
 		try {
-			$result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken, $this->request->getParam('filePath'));
+			$result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken);
 		} catch (DocumentSaveConflictException) {
 			try {
 				$result['outsideChange'] = $file->getContent();

diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
@@ -295,6 +295,27 @@ public function getSteps($documentId, $lastVersion): array {
 		return $this->stepMapper->find($documentId, $lastVersion);
 	}
 
+	/**
+	 * @throws DocumentSaveConflictException
+	 * @throws InvalidPathException
+	 * @throws NotFoundException
+	 */
+	public function assertNoOutsideConflict(Document $document, File $file, bool $force = false, ?string $shareToken = null): void {
+		$documentId = $document->getId();
+		$savedEtag = $file->getEtag();
+		$lastMTime = $document->getLastSavedVersionTime();
+
+		if ($lastMTime > 0
+			&& $force === false
+			&& $this->isReadOnly($file, $shareToken)
+			&& $savedEtag !== $document->getLastSavedVersionEtag()
+			&& $lastMTime !== $file->getMtime()
+			&& !$this->cache->get('document-save-lock-' . $documentId)
+		) {
+			throw new DocumentSaveConflictException('File changed in the meantime from outside');
+		}
+	}
+
 	/**
 	 * @throws DocumentSaveConflictException
 	 * @throws DoesNotExistException
@@ -303,7 +324,7 @@ public function getSteps($documentId, $lastVersion): array {
 	 * @throws NotPermittedException
 	 * @throws Exception
 	 */
-	public function autosave(Document $document, ?File $file, int $version, ?string $autoSaveDocument, ?string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null, ?string $filePath = null): Document {
+	public function autosave(Document $document, ?File $file, int $version, ?string $autoSaveDocument, ?string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): Document {
 		$documentId = $document->getId();
 		if ($file === null) {
 			throw new NotFoundException();
@@ -313,17 +334,7 @@ public function autosave(Document $document, ?File $file, int $version, ?string
 			return $document;
 		}
 
-		$savedEtag = $file->getEtag();
-		$lastMTime = $document->getLastSavedVersionTime();
-
-		if ($lastMTime > 0 && $savedEtag !== $document->getLastSavedVersionEtag() && $lastMTime !== $file->getMtime() && $force === false) {
-			if (!$this->cache->get('document-save-lock-' . $documentId)) {
-				throw new DocumentSaveConflictException('File changed in the meantime from outside');
-			} else {
-				// Only return here if the document is locked, otherwise we can continue to save
-				return $document;
-			}
-		}
+		$this->assertNoOutsideConflict($document, $file, $force);
 
 		if ($autoSaveDocument === null) {
 			return $document;
@@ -339,6 +350,7 @@ public function autosave(Document $document, ?File $file, int $version, ?string
 		}
 
 		// Only save once every AUTOSAVE_MINIMUM_DELAY seconds
+		$lastMTime = $document->getLastSavedVersionTime();
 		if ($file->getMTime() === $lastMTime && $lastMTime > time() - self::AUTOSAVE_MINIMUM_DELAY && $manualSave === false) {
 			return $document;
 		}