From 594a16440dd5a7787a5341b04bb5b1cb617eb8e8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julius=20H=C3=A4rtl?= <jus@bitgrid.net>
Date: Wed, 3 Jan 2024 14:45:50 +0100
Subject: [PATCH] fix: Proper error message based on file permissions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Julius Härtl <jus@bitgrid.net>
---
 lib/Service/ApiService.php                 |   5 +-
 lib/Service/DocumentService.php            |   5 +
 src/components/Editor.vue                  |   2 +-
 tests/unit/Service/DocumentServiceTest.php | 104 +++++++++++++++++++++
 4 files changed, 113 insertions(+), 3 deletions(-)
 create mode 100644 tests/unit/Service/DocumentServiceTest.php

diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php
index 6846b5c8a..288171640 100644
--- a/lib/Service/ApiService.php
+++ b/lib/Service/ApiService.php
@@ -89,8 +89,9 @@ class ApiService {
 			} elseif ($fileId) {
 				try {
 					$file = $this->documentService->getFileById($fileId);
-				} catch (NotFoundException $e) {
-					return new DataResponse([], Http::STATUS_NOT_FOUND);
+				} catch (NotFoundException|NotPermittedException $e) {
+					$this->logger->error('No permission to access this file', [ 'exception' => $e ]);
+					return new DataResponse($this->l10n->t('No permission to access this file.'), Http::STATUS_NOT_FOUND);
 				}
 			} else {
 				return new DataResponse('No valid file argument provided', Http::STATUS_PRECONDITION_FAILED);
diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
index 251b7c667..496200f2a 100644
--- a/lib/Service/DocumentService.php
+++ b/lib/Service/DocumentService.php
@@ -465,6 +465,7 @@ class DocumentService {
 
 	/**
 	 * @throws NotFoundException
+	 * @throws NotPermittedException
 	 */
 	public function getFileById(int $fileId, ?string $userId = null): File {
 		$userId = $userId ?? $this->userId;
@@ -504,6 +505,10 @@ class DocumentService {
 			throw new NotFoundException();
 		}
 
+		if (($file->getPermissions() & Constants::PERMISSION_READ) !== Constants::PERMISSION_READ) {
+			throw new NotPermittedException();
+		}
+
 		return $file;
 	}
 
diff --git a/src/components/Editor.vue b/src/components/Editor.vue
index 2ccfcb151..a6a49b0af 100644
--- a/src/components/Editor.vue
+++ b/src/components/Editor.vue
@@ -32,7 +32,7 @@
 			:has-connection-issue="hasConnectionIssue"
 			@reconnect="reconnect" />
 
-		<SkeletonLoading v-if="!contentLoaded" />
+		<SkeletonLoading v-if="!contentLoaded && !displayedStatus" />
 		<Wrapper v-if="displayed"
 			:sync-error="syncError"
 			:has-connection-issue="hasConnectionIssue"
diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php
new file mode 100644
index 000000000..7b886ebd4
--- /dev/null
+++ b/tests/unit/Service/DocumentServiceTest.php
@@ -0,0 +1,104 @@
+<?php
+
+namespace OCA\Text\Tests;
+
+use OCA\Text\Db\DocumentMapper;
+use OCA\Text\Db\SessionMapper;
+use OCA\Text\Db\StepMapper;
+use OCA\Text\Service\DocumentService;
+use OCP\Constants;
+use OCP\Files\Config\IUserMountCache;
+use OCP\Files\Folder;
+use OCP\Files\IAppData;
+use OCP\Files\IRootFolder;
+use OCP\Files\Lock\ILockManager;
+use OCP\Files\NotPermittedException;
+use OCP\ICacheFactory;
+use OCP\IRequest;
+use OCP\Share\IManager;
+use Psr\Log\LoggerInterface;
+
+class DocumentServiceTest extends \PHPUnit\Framework\TestCase {
+	private DocumentService $documentService;
+
+	private DocumentMapper $documentMapper;
+	private StepMapper $setpMapper;
+	private SessionMapper $sessionMapper;
+	private IAppData $appData;
+	private string $userId;
+	private IRootFolder $rootFolder;
+	private ICacheFactory $cacheFactory;
+	private LoggerInterface $loggerInterface;
+	private IManager $shareManager;
+	private IRequest $request;
+	private \OCP\DirectEditing\IManager $directManager;
+	private ILockManager $lockManager;
+	private IUserMountCache $userMountCache;
+
+	public function setUp(): void {
+		$this->documentMapper = $this->createMock(DocumentMapper::class);
+		$this->setpMapper = $this->createMock(StepMapper::class);
+		$this->sessionMapper = $this->createMock(SessionMapper::class);
+		$this->appData = $this->createMock(IAppData::class);
+		$this->userId = 'admin';
+		$this->rootFolder = $this->createMock(IRootFolder::class);
+		$this->cacheFactory = $this->createMock(ICacheFactory::class);
+		$this->loggerInterface = $this->createMock(LoggerInterface::class);
+		$this->shareManager = $this->createMock(IManager::class);
+		$this->request = $this->createMock(IRequest::class);
+		$this->directManager = $this->createMock(\OCP\DirectEditing\IManager::class);
+		$this->lockManager = $this->createMock(ILockManager::class);
+		$this->userMountCache = $this->createMock(IUserMountCache::class);
+
+		$this->documentService = new DocumentService(
+			$this->documentMapper,
+			$this->setpMapper,
+			$this->sessionMapper,
+			$this->appData,
+			$this->userId,
+			$this->rootFolder,
+			$this->cacheFactory,
+			$this->loggerInterface,
+			$this->shareManager,
+			$this->request,
+			$this->directManager,
+			$this->lockManager,
+			$this->userMountCache,
+		);
+	}
+
+	public function testGetFileById() {
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')->willReturn($userFolder);
+
+		$file = $this->createMock(\OCP\Files\File::class);
+		$file->method('getPermissions')->willReturn(Constants::PERMISSION_READ);
+		$userFolder->method('getById')->willReturn([$file]);
+		$actual = $this->documentService->getFileById(1234);
+		self::assertEquals($file, $actual);
+	}
+
+	public function testGetFileByIdSortUpdatableFirst() {
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')->willReturn($userFolder);
+
+		$file1 = $this->createMock(\OCP\Files\File::class);
+		$file1->method('getPermissions')->willReturn(Constants::PERMISSION_READ);
+		$file2 = $this->createMock(\OCP\Files\File::class);
+		$file2->method('getPermissions')->willReturn(Constants::PERMISSION_READ & Constants::PERMISSION_UPDATE);
+		$userFolder->method('getById')->willReturn([$file1, $file2]);
+		$actual = $this->documentService->getFileById(1234);
+		self::assertEquals($file2, $actual);
+	}
+
+	public function testGetFileByIdNoRead() {
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')->willReturn($userFolder);
+
+		$file = $this->createMock(\OCP\Files\File::class);
+		$file->method('getPermissions')->willReturn(Constants::PERMISSION_UPDATE);
+		$userFolder->method('getById')->willReturn([$file]);
+		$this->expectException(NotPermittedException::class);
+		$actual = $this->documentService->getFileById(1234);
+	}
+}