diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 9678aa67888bc..da71aca904c26 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -78,6 +78,8 @@ public function getPreview( int $y = 32, $a = false, ) { + $cacheForSeconds = 60 * 60 * 24; // 1 day + if ($token === '' || $x === 0 || $y === 0) { return new DataResponse([], Http::STATUS_BAD_REQUEST); } @@ -93,7 +95,17 @@ public function getPreview( } $attributes = $share->getAttributes(); - if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) { + // Only explicitly set to false will forbid the download! + $downloadForbidden = $attributes?->getAttribute('permissions', 'download') === false; + // Is this header is set it means our UI is doing a preview for no-download shares + // we check a header so we at least prevent people from using the link directly (obfuscation) + $isPublicPreview = $this->request->getHeader('X-NC-Preview') === 'true'; + + if ($isPublicPreview && $downloadForbidden) { + // Only cache for 15 minutes on public preview requests to quickly remove from cache + $cacheForSeconds = 15 * 60; + } elseif ($downloadForbidden) { + // This is not a public share preview so we only allow a preview if download permissions are granted return new DataResponse([], Http::STATUS_FORBIDDEN); } @@ -107,7 +119,7 @@ public function getPreview( $f = $this->previewManager->getPreview($file, $x, $y, !$a); $response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]); - $response->cacheFor(3600 * 24); + $response->cacheFor($cacheForSeconds); return $response; } catch (NotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); diff --git a/apps/files_sharing/lib/ViewOnly.php b/apps/files_sharing/lib/ViewOnly.php index 9cd18f968f6e0..2204d26388bf3 100644 --- a/apps/files_sharing/lib/ViewOnly.php +++ b/apps/files_sharing/lib/ViewOnly.php @@ -89,17 +89,10 @@ private function checkFileInfo(Node $fileInfo): bool { /** @var SharedStorage $storage */ $share = $storage->getShare(); - $canDownload = true; - - // Check if read-only and on whether permission can download is both set and disabled. + // Check whether download-permission was denied (granted if not set) $attributes = $share->getAttributes(); - if ($attributes !== null) { - $canDownload = $attributes->getAttribute('permissions', 'download'); - } + $canDownload = $attributes?->getAttribute('permissions', 'download'); - if ($canDownload !== null && !$canDownload) { - return false; - } - return true; + return $canDownload !== false; } } diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index 700406c97ba17..bd64cfc60b4c9 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -19,6 +19,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\Share\Exceptions\ShareNotFound; +use OCP\Share\IAttributes; use OCP\Share\IManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -26,15 +27,12 @@ class PublicPreviewControllerTest extends TestCase { - /** @var IPreview|\PHPUnit\Framework\MockObject\MockObject */ - private $previewManager; - /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ - private $shareManager; - /** @var ITimeFactory|MockObject */ - private $timeFactory; + private IPreview&MockObject $previewManager; + private IManager&MockObject $shareManager; + private ITimeFactory&MockObject $timeFactory; + private IRequest&MockObject $request; - /** @var PublicPreviewController */ - private $controller; + private PublicPreviewController $controller; protected function setUp(): void { parent::setUp(); @@ -42,6 +40,7 @@ protected function setUp(): void { $this->previewManager = $this->createMock(IPreview::class); $this->shareManager = $this->createMock(IManager::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->request = $this->createMock(IRequest::class); $this->timeFactory->method('getTime') ->willReturn(1337); @@ -50,7 +49,7 @@ protected function setUp(): void { $this->controller = new PublicPreviewController( 'files_sharing', - $this->createMock(IRequest::class), + $this->request, $this->shareManager, $this->createMock(ISession::class), $this->previewManager @@ -104,7 +103,109 @@ public function testShareNotAccessable(): void { $this->assertEquals($expected, $res); } - public function testPreviewFile(): void { + public function testShareNoDownload() { + $share = $this->createMock(IShare::class); + $this->shareManager->method('getShareByToken') + ->with($this->equalTo('token')) + ->willReturn($share); + + $share->method('getPermissions') + ->willReturn(Constants::PERMISSION_READ); + + $attributes = $this->createMock(IAttributes::class); + $attributes->method('getAttribute') + ->with('permissions', 'download') + ->willReturn(false); + $share->method('getAttributes') + ->willReturn($attributes); + + $res = $this->controller->getPreview('token', 'file', 10, 10); + $expected = new DataResponse([], Http::STATUS_FORBIDDEN); + + $this->assertEquals($expected, $res); + } + + public function testShareNoDownloadButPreviewHeader() { + $share = $this->createMock(IShare::class); + $this->shareManager->method('getShareByToken') + ->with($this->equalTo('token')) + ->willReturn($share); + + $share->method('getPermissions') + ->willReturn(Constants::PERMISSION_READ); + + $attributes = $this->createMock(IAttributes::class); + $attributes->method('getAttribute') + ->with('permissions', 'download') + ->willReturn(false); + $share->method('getAttributes') + ->willReturn($attributes); + + $this->request->method('getHeader') + ->with('X-NC-Preview') + ->willReturn('true'); + + $file = $this->createMock(File::class); + $share->method('getNode') + ->willReturn($file); + + $preview = $this->createMock(ISimpleFile::class); + $preview->method('getName')->willReturn('name'); + $preview->method('getMTime')->willReturn(42); + $this->previewManager->method('getPreview') + ->with($this->equalTo($file), 10, 10, false) + ->willReturn($preview); + + $preview->method('getMimeType') + ->willReturn('myMime'); + + $res = $this->controller->getPreview('token', 'file', 10, 10, true); + $expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']); + $expected->cacheFor(15 * 60); + $this->assertEquals($expected, $res); + } + + public function testShareWithAttributes() { + $share = $this->createMock(IShare::class); + $this->shareManager->method('getShareByToken') + ->with($this->equalTo('token')) + ->willReturn($share); + + $share->method('getPermissions') + ->willReturn(Constants::PERMISSION_READ); + + $attributes = $this->createMock(IAttributes::class); + $attributes->method('getAttribute') + ->with('permissions', 'download') + ->willReturn(true); + $share->method('getAttributes') + ->willReturn($attributes); + + $this->request->method('getHeader') + ->with('X-NC-Preview') + ->willReturn('true'); + + $file = $this->createMock(File::class); + $share->method('getNode') + ->willReturn($file); + + $preview = $this->createMock(ISimpleFile::class); + $preview->method('getName')->willReturn('name'); + $preview->method('getMTime')->willReturn(42); + $this->previewManager->method('getPreview') + ->with($this->equalTo($file), 10, 10, false) + ->willReturn($preview); + + $preview->method('getMimeType') + ->willReturn('myMime'); + + $res = $this->controller->getPreview('token', 'file', 10, 10, true); + $expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']); + $expected->cacheFor(3600 * 24); + $this->assertEquals($expected, $res); + } + + public function testPreviewFile() { $share = $this->createMock(IShare::class); $this->shareManager->method('getShareByToken') ->with($this->equalTo('token')) diff --git a/core/Controller/PreviewController.php b/core/Controller/PreviewController.php index a3b826c19e64d..807df4a2ebcbd 100644 --- a/core/Controller/PreviewController.php +++ b/core/Controller/PreviewController.php @@ -8,7 +8,6 @@ */ namespace OC\Core\Controller; -use OCA\Files_Sharing\SharedStorage; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; @@ -21,6 +20,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\Files\Storage\ISharedStorage; use OCP\IPreview; use OCP\IRequest; use OCP\Preview\IMimeIconProvider; @@ -145,12 +145,17 @@ private function fetchPreview( return new DataResponse([], Http::STATUS_NOT_FOUND); } + // Is this header is set it means our UI is doing a preview for no-download shares + // we check a header so we at least prevent people from using the link directly (obfuscation) + $isNextcloudPreview = $this->request->getHeader('X-NC-Preview') === 'true'; $storage = $node->getStorage(); - if ($storage->instanceOfStorage(SharedStorage::class)) { - /** @var SharedStorage $storage */ + if ($isNextcloudPreview === false && $storage->instanceOfStorage(ISharedStorage::class)) { + /** @var ISharedStorage $storage */ $share = $storage->getShare(); $attributes = $share->getAttributes(); - if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) { + // No "allow preview" header set, so we must check if + // the share has not explicitly disabled download permissions + if ($attributes?->getAttribute('permissions', 'download') === false) { return new DataResponse([], Http::STATUS_FORBIDDEN); } } diff --git a/tests/Core/Controller/PreviewControllerTest.php b/tests/Core/Controller/PreviewControllerTest.php index 4274f15e8edc0..e7ecba27064db 100644 --- a/tests/Core/Controller/PreviewControllerTest.php +++ b/tests/Core/Controller/PreviewControllerTest.php @@ -14,34 +14,35 @@ use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; +use OCP\Files\Storage\ISharedStorage; use OCP\Files\Storage\IStorage; use OCP\IPreview; use OCP\IRequest; use OCP\Preview\IMimeIconProvider; +use OCP\Share\IAttributes; +use OCP\Share\IShare; +use PHPUnit\Framework\MockObject\MockObject; class PreviewControllerTest extends \Test\TestCase { - /** @var IRootFolder|\PHPUnit\Framework\MockObject\MockObject */ - private $rootFolder; - /** @var string */ - private $userId; + private string $userId; + private PreviewController $controller; - /** @var IPreview|\PHPUnit\Framework\MockObject\MockObject */ - private $previewManager; - - /** @var PreviewController|\PHPUnit\Framework\MockObject\MockObject */ - private $controller; + private IRootFolder&MockObject $rootFolder; + private IPreview&MockObject $previewManager; + private IRequest&MockObject $request; protected function setUp(): void { parent::setUp(); - $this->rootFolder = $this->createMock(IRootFolder::class); $this->userId = 'user'; + $this->rootFolder = $this->createMock(IRootFolder::class); $this->previewManager = $this->createMock(IPreview::class); + $this->request = $this->createMock(IRequest::class); $this->controller = new PreviewController( 'core', - $this->createMock(IRequest::class), + $this->request, $this->previewManager, $this->rootFolder, $this->userId, @@ -124,7 +125,38 @@ public function testNoPreviewAndNoIcon(): void { $this->assertEquals($expected, $res); } - public function testForbiddenFile(): void { + public function testNoPreview() { + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->equalTo($this->userId)) + ->willReturn($userFolder); + + $file = $this->createMock(File::class); + $userFolder->method('get') + ->with($this->equalTo('file')) + ->willReturn($file); + + $storage = $this->createMock(IStorage::class); + $file->method('getStorage') + ->willReturn($storage); + + $this->previewManager->method('isAvailable') + ->with($this->equalTo($file)) + ->willReturn(true); + + $file->method('isReadable') + ->willReturn(true); + + $this->previewManager->method('getPreview') + ->with($this->equalTo($file), 10, 10, false, $this->equalTo('myMode')) + ->willThrowException(new NotFoundException()); + + $res = $this->controller->getPreview('file', 10, 10, true, true, 'myMode'); + $expected = new DataResponse([], Http::STATUS_NOT_FOUND); + + $this->assertEquals($expected, $res); + } + public function testFileWithoutReadPermission() { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder') ->with($this->equalTo($this->userId)) @@ -148,36 +180,108 @@ public function testForbiddenFile(): void { $this->assertEquals($expected, $res); } - public function testNoPreview(): void { + public function testFileWithoutDownloadPermission() { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder') ->with($this->equalTo($this->userId)) ->willReturn($userFolder); $file = $this->createMock(File::class); + $file->method('getId')->willReturn(123); $userFolder->method('get') ->with($this->equalTo('file')) ->willReturn($file); - $storage = $this->createMock(IStorage::class); + $this->previewManager->method('isAvailable') + ->with($this->equalTo($file)) + ->willReturn(true); + + $shareAttributes = $this->createMock(IAttributes::class); + $shareAttributes->expects(self::atLeastOnce()) + ->method('getAttribute') + ->with('permissions', 'download') + ->willReturn(false); + + $share = $this->createMock(IShare::class); + $share->method('getAttributes') + ->willReturn($shareAttributes); + + $storage = $this->createMock(ISharedStorage::class); + $storage->method('instanceOfStorage') + ->with(ISharedStorage::class) + ->willReturn(true); + $storage->method('getShare') + ->willReturn($share); + $file->method('getStorage') ->willReturn($storage); + $file->method('isReadable') + ->willReturn(true); + + $this->request->method('getHeader')->willReturn(''); + + $res = $this->controller->getPreview('file', 10, 10, true, true); + $expected = new DataResponse([], Http::STATUS_FORBIDDEN); + + $this->assertEquals($expected, $res); + } + + public function testFileWithoutDownloadPermissionButHeader() { + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->equalTo($this->userId)) + ->willReturn($userFolder); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(123); + $userFolder->method('get') + ->with($this->equalTo('file')) + ->willReturn($file); $this->previewManager->method('isAvailable') ->with($this->equalTo($file)) ->willReturn(true); + $shareAttributes = $this->createMock(IAttributes::class); + $shareAttributes->method('getAttribute') + ->with('permissions', 'download') + ->willReturn(false); + + $share = $this->createMock(IShare::class); + $share->method('getAttributes') + ->willReturn($shareAttributes); + + $storage = $this->createMock(ISharedStorage::class); + $storage->method('instanceOfStorage') + ->with(ISharedStorage::class) + ->willReturn(true); + $storage->method('getShare') + ->willReturn($share); + + $file->method('getStorage') + ->willReturn($storage); $file->method('isReadable') ->willReturn(true); + $this->request + ->method('getHeader') + ->with('X-NC-Preview') + ->willReturn('true'); + + $preview = $this->createMock(ISimpleFile::class); + $preview->method('getName')->willReturn('my name'); + $preview->method('getMTime')->willReturn(42); $this->previewManager->method('getPreview') ->with($this->equalTo($file), 10, 10, false, $this->equalTo('myMode')) - ->willThrowException(new NotFoundException()); + ->willReturn($preview); + $preview->method('getMimeType') + ->willReturn('myMime'); $res = $this->controller->getPreview('file', 10, 10, true, true, 'myMode'); - $expected = new DataResponse([], Http::STATUS_NOT_FOUND); - $this->assertEquals($expected, $res); + $this->assertEquals('myMime', $res->getHeaders()['Content-Type']); + $this->assertEquals(Http::STATUS_OK, $res->getStatus()); + $this->assertEquals($preview, $this->invokePrivate($res, 'file')); } public function testValidPreview(): void { @@ -218,4 +322,57 @@ public function testValidPreview(): void { $this->assertEquals(Http::STATUS_OK, $res->getStatus()); $this->assertEquals($preview, $this->invokePrivate($res, 'file')); } + + public function testValidPreviewOfShare() { + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->equalTo($this->userId)) + ->willReturn($userFolder); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(123); + $userFolder->method('get') + ->with($this->equalTo('file')) + ->willReturn($file); + + $this->previewManager->method('isAvailable') + ->with($this->equalTo($file)) + ->willReturn(true); + + // No attributes set -> download permitted + $share = $this->createMock(IShare::class); + $share->method('getAttributes') + ->willReturn(null); + + $storage = $this->createMock(ISharedStorage::class); + $storage->method('instanceOfStorage') + ->with(ISharedStorage::class) + ->willReturn(true); + $storage->method('getShare') + ->willReturn($share); + + $file->method('getStorage') + ->willReturn($storage); + $file->method('isReadable') + ->willReturn(true); + + $this->request + ->method('getHeader') + ->willReturn(''); + + $preview = $this->createMock(ISimpleFile::class); + $preview->method('getName')->willReturn('my name'); + $preview->method('getMTime')->willReturn(42); + $this->previewManager->method('getPreview') + ->with($this->equalTo($file), 10, 10, false, $this->equalTo('myMode')) + ->willReturn($preview); + $preview->method('getMimeType') + ->willReturn('myMime'); + + $res = $this->controller->getPreview('file', 10, 10, true, true, 'myMode'); + + $this->assertEquals('myMime', $res->getHeaders()['Content-Type']); + $this->assertEquals(Http::STATUS_OK, $res->getStatus()); + $this->assertEquals($preview, $this->invokePrivate($res, 'file')); + } }