fix: Proper error message based on file permissions

Signed-off-by: Julius Härtl <[email protected]>

lib/Service/ApiService.php

@@ -81,8 +81,13 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 				} catch (NotFoundException $e) {
 				}
 			} elseif ($fileId) {
-				$file = $this->documentService->getFileById($fileId);
-				$readOnly = !$file->isUpdateable();
+				try {
+					$file = $this->documentService->getFileById($fileId);
+					$readOnly = !$file->isUpdateable();
+				} catch (NotFoundException $e) {
+					$this->logger->error('No permission to access this file', [ 'exception' => $e ]);
+					return new DataResponse('No permission to access this file.', Http::STATUS_NOT_FOUND);
+				}
 			} else {
 				return new DataResponse('No valid file argument provided', 500);
 			}

lib/Service/DocumentService.php

@@ -33,7 +33,7 @@
 use OCP\IRequest;
 use OCP\Lock\ILockingProvider;
 use function json_encode;
-use OC\Files\Node\File;
+use OCP\Files\File;
 use OCA\Text\Db\Document;
 use OCA\Text\Db\DocumentMapper;
 use OCA\Text\Db\Step;
@@ -395,6 +395,7 @@ public function getFileForSession(Session $session, $shareToken) {
 
 	/**
 	 * @throws NotFoundException
+	 * @throws NotPermittedException
 	 */
 	public function getFileById($fileId, $userId = null): Node {
 		try {
@@ -415,7 +416,17 @@ public function getFileById($fileId, $userId = null): Node {
 			return ($a->getPermissions() & Constants::PERMISSION_UPDATE) < ($b->getPermissions() & Constants::PERMISSION_UPDATE);
 		});
 
-		return array_shift($files);
+		$file = array_shift($files);
+
+		if (!$file instanceof File) {
+			throw new NotFoundException();
+		}
+
+		if (($file->getPermissions() & Constants::PERMISSION_READ) !== Constants::PERMISSION_READ) {
+			throw new NotFoundException();
+		}
+
+		return $file;
 	}
 
 	/**

tests/unit/Service/DocumentServiceTest.php

@@ -0,0 +1,108 @@
+<?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\NotFoundException;
+use OCP\ICacheFactory;
+use OCP\ILogger;
+use OCP\IRequest;
+use OCP\Lock\ILockingProvider;
+use OCP\Share\IManager;
+
+class DocumentServiceTest extends \PHPUnit\Framework\TestCase {
+	private $documentService;
+
+	private $documentMapper;
+	private $setpMapper;
+	private $sessionMapper;
+	private $appData;
+	private $userId;
+	private $rootFolder;
+	private $cacheFactory;
+	private $loggerInterface;
+	private $shareManager;
+	private $request;
+	private $directManager;
+	private $lockingProvider;
+	private $lockManager;
+	private $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(ILogger::class);
+		$this->shareManager = $this->createMock(IManager::class);
+		$this->request = $this->createMock(IRequest::class);
+		$this->directManager = $this->createMock(\OCP\DirectEditing\IManager::class);
+		$this->lockingProvider = $this->createMock(ILockingProvider::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->lockingProvider,
+			$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(NotFoundException::class);
+		$actual = $this->documentService->getFileById(1234);
+	}
+}