Merge pull request #6019 from nextcloud/fix/return_codes

fix(ApiService): Catch NotPermittedException and return 404
nextcloud · Jul 9, 2024 · cfb9236 · cfb9236
2 parents 2e84899 + e178b63
commit cfb9236
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 15 deletions.
diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php
@@ -58,14 +58,16 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b
 				} catch (NotFoundException $e) {
 					return new DataResponse([], Http::STATUS_NOT_FOUND);
 				} catch (NotPermittedException $e) {
-					return new DataResponse(['error' => $this->l10n->t('This file cannot be displayed as download is disabled by the share')], 404);
+					return new DataResponse(['error' => $this->l10n->t('This file cannot be displayed as download is disabled by the share')], Http::STATUS_NOT_FOUND);
 				}
 			} elseif ($fileId !== null) {
 				try {
 					$file = $this->documentService->getFileById($fileId, $this->userId);
 				} catch (NotFoundException|NotPermittedException $e) {
 					$this->logger->error('No permission to access this file', [ 'exception' => $e ]);
-					return new DataResponse(['error' => $this->l10n->t('No permission to access this file.')], Http::STATUS_NOT_FOUND);
+					return new DataResponse([
+						'error' => $this->l10n->t('File not found')
+					], Http::STATUS_NOT_FOUND);
 				}
 			} else {
 				return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED);
@@ -79,7 +81,7 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b
 				$share = $storage->getShare();
 				$shareAttribtues = $share->getAttributes();
 				if ($shareAttribtues !== null && $shareAttribtues->getAttribute('permissions', 'download') === false) {
-					return new DataResponse(['error' => $this->l10n->t('This file cannot be displayed as download is disabled by the share')], 403);
+					return new DataResponse(['error' => $this->l10n->t('This file cannot be displayed as download is disabled by the share')], Http::STATUS_FORBIDDEN);
 				}
 			}
 
@@ -105,7 +107,7 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b
 			}
 		} catch (Exception $e) {
 			$this->logger->error($e->getMessage(), ['exception' => $e]);
-			return new DataResponse(['error' => 'Failed to create the document session'], 500);
+			return new DataResponse(['error' => 'Failed to create the document session'], Http::STATUS_INTERNAL_SERVER_ERROR);
 		}
 
 		/** @var Document $document */
@@ -180,7 +182,7 @@ public function push(Session $session, Document $document, int $version, array $
 		try {
 			$result = $this->documentService->addStep($document, $session, $steps, $version, $token);
 		} catch (InvalidArgumentException $e) {
-			return new DataResponse(['error' => $e->getMessage()], 422);
+			return new DataResponse(['error' => $e->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY);
 		} catch (DoesNotExistException|NotPermittedException) {
 			// Either no write access or session was removed in the meantime (#3875).
 			return new DataResponse(['error' => $this->l10n->t('Editing session has expired. Please reload the page.')], Http::STATUS_PRECONDITION_FAILED);
@@ -201,16 +203,16 @@ public function sync(Session $session, Document $document, int $version = 0, ?st
 			// ensure file is still present and accessible
 			$file = $this->documentService->getFileForSession($session, $shareToken);
 			$this->documentService->assertNoOutsideConflict($document, $file);
-		} catch (NotFoundException|InvalidPathException $e) {
+		} catch (NotPermittedException|NotFoundException|InvalidPathException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'File not found'
-			], 404);
+			], Http::STATUS_NOT_FOUND);
 		} catch (DoesNotExistException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'Document no longer exists'
-			], 404);
+			], Http::STATUS_NOT_FOUND);
 		} catch (DocumentSaveConflictException) {
 			try {
 				/** @psalm-suppress PossiblyUndefinedVariable */
@@ -220,22 +222,22 @@ public function sync(Session $session, Document $document, int $version = 0, ?st
 			}
 		}
 
-		return new DataResponse($result, isset($result['outsideChange']) ? 409 : 200);
+		return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK);
 	}
 
 	public function save(Session $session, Document $document, int $version = 0, ?string $autosaveContent = null, ?string $documentState = null, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse {
 		try {
 			$file = $this->documentService->getFileForSession($session, $shareToken);
-		} catch (NotFoundException $e) {
+		} catch (NotPermittedException|NotFoundException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'File not found'
-			], 404);
+			], Http::STATUS_NOT_FOUND);
 		} catch (DoesNotExistException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'Document no longer exists'
-			], 404);
+			], Http::STATUS_NOT_FOUND);
 		}
 
 		$result = [];
@@ -248,15 +250,15 @@ public function save(Session $session, Document $document, int $version = 0, ?st
 				// Ignore locked exception since it might happen due to an autosave action happening at the same time
 			}
 		} catch (NotFoundException) {
-			return new DataResponse([], 404);
+			return new DataResponse([], Http::STATUS_NOT_FOUND);
 		} catch (Exception $e) {
 			$this->logger->error($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
 				'message' => 'Failed to autosave document'
-			], 500);
+			], Http::STATUS_INTERNAL_SERVER_ERROR);
 		}
 
-		return new DataResponse($result, isset($result['outsideChange']) ? 409 : 200);
+		return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK);
 	}
 
 	public function updateSession(Session $session, string $guestName): DataResponse {

diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
@@ -436,6 +436,7 @@ public function getAll(): \Generator {
 	}
 
 	/**
+	 * @throws NotPermittedException
 	 * @throws NotFoundException
 	 */
 	public function getFileForSession(Session $session, ?string $shareToken = null): File {