From 74aa530fb4ca922d3577b64cc01676d28c449988 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 2a23f01122..84c265a075 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -58,7 +58,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 !== null) { try { @@ -79,7 +79,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 +105,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 +180,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); @@ -205,12 +205,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 */ @@ -220,7 +220,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 { @@ -230,12 +230,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 = []; @@ -248,15 +248,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 48f5017cca..cd264c528b 100644 --- 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 { From e178b6308259d72a4a378f1ba2cba27363fb1595 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 84c265a075..772d8b8bcf 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -65,7 +65,9 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b $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); @@ -201,7 +203,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' @@ -226,7 +228,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'