From b27f9d57c1d68fa166500a5381ffa74b2ed8f0bd Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 9 Jul 2024 11:07:35 +0200 Subject: [PATCH 1/2] chore(ApiService): Use constants for http status codes everywhere Signed-off-by: Jonas --- lib/Service/ApiService.php | 24 ++++++++++++------------ lib/Service/DocumentService.php | 1 + 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index be2aee01751..3fb9a3a2777 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -85,7 +85,7 @@ 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) { try { @@ -106,7 +106,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); } } @@ -132,7 +132,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 */ @@ -207,7 +207,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); @@ -232,12 +232,12 @@ public function sync(Session $session, Document $document, int $version = 0, ?st $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 */ @@ -247,7 +247,7 @@ 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 { @@ -257,12 +257,12 @@ public function save(Session $session, Document $document, int $version = 0, ?st $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 = []; @@ -275,15 +275,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 index 976039cbeec..32e37b97fdf 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -452,6 +452,7 @@ public function getAll(): \Generator { } /** + * @throws NotPermittedException * @throws NotFoundException */ public function getFileForSession(Session $session, ?string $shareToken = null): File { From c952501767f3f701fb8e65f428286b072381a27f Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 9 Jul 2024 11:15:52 +0200 Subject: [PATCH 2/2] fix(ApiService): Catch NotPermittedException and return 404 Also adjust 404 error message in create api function in case of NotPermittedException. We don't want to distinguish between missing permissions and nonexisting files to not reveal that the file exists. Signed-off-by: Jonas --- lib/Service/ApiService.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 3fb9a3a2777..39e8ffe1d4a 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -92,7 +92,9 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b $file = $this->documentService->getFileById($fileId); } 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); @@ -228,7 +230,7 @@ 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' @@ -253,7 +255,7 @@ public function sync(Session $session, Document $document, int $version = 0, ?st 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'