Merge pull request #47831 from nextcloud/fix/view-only-preview

fix: Adjust preview for view-only shares
nextcloud · Oct 28, 2024 · d4acae6 · d4acae6
2 parents 8e6fd4d + c84c256
commit d4acae6
Show file tree

Hide file tree

Showing 5 changed files with 311 additions and 43 deletions.
diff --git 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
@@ -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
@@ -19,29 +19,28 @@
 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;
 use Test\TestCase;
 
 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();
 
 		$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
@@ -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);
 			}
 		}