Skip to content

Commit

Permalink
Merge pull request #5274 from nextcloud/bugfix/noid/load-error-messag…
Browse files Browse the repository at this point in the history
…e-26

[stable26] fix: Proper error message based on file permissions
  • Loading branch information
blizzz authored Jan 17, 2024
2 parents 2dd9267 + dd39277 commit ae96ca8
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 3 deletions.
5 changes: 3 additions & 2 deletions lib/Service/ApiService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 12 additions & 1 deletion lib/Service/DocumentService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand Down
108 changes: 108 additions & 0 deletions tests/unit/Service/DocumentServiceTest.php
Original file line number Diff line number Diff line change
@@ -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);
}
}

0 comments on commit ae96ca8

Please sign in to comment.