Merge pull request #5274 from nextcloud/bugfix/noid/load-error-messag…

…e-26

[stable26] fix: Proper error message based on file permissions

lib/Service/ApiService.php

@@ -91,8 +91,9 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 			} 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);

lib/Service/DocumentService.php

@@ -461,6 +461,7 @@ public function getFileForSession(Session $session, ?string $shareToken = null):
 
 	/**
 	 * @throws NotFoundException
+	 * @throws NotPermittedException
 	 */
 	public function getFileById($fileId, $userId = null): File {
 		$userId = $userId ?? $this->userId;
@@ -494,7 +495,17 @@ public function getFileById($fileId, $userId = null): File {
 			return ($b->getPermissions() & Constants::PERMISSION_UPDATE) <=> ($a->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 NotPermittedException();
+		}
+
+		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\NotPermittedException;
+use OCP\ICacheFactory;
+use OCP\IRequest;
+use OCP\Lock\ILockingProvider;
+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 ILockingProvider $lockingProvider;
+	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->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(NotPermittedException::class);
+		$actual = $this->documentService->getFileById(1234);
+	}
+}