fix: Adjust preview for view-only shares

Previously there was a different behavior for public shares (link-shares) and internal shares,
if the user disabled the view permission.
The legacy UI for public shares simply "disabled" the context menu and hided all download actions.
With Nextcloud 31 all share types use the consistent permissions attributes,
which simplifies code, but caused a regression: Images can no longer been viewed.

Because on 30 and before the attribute was not set, previews for view-only files
were still allowed. Now with 31 we need a new way to allow "viewing" shares.

So this is allowing previews for those files, but only for internal usage.
This is done by settin a special header, which only works with custom requests,
and not by opening the URL directly.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

apps/files_sharing/lib/Controller/PublicPreviewController.php

@@ -78,6 +78,8 @@ class PublicPreviewController extends PublicShareController {
 		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 @@ class PublicPreviewController extends PublicShareController {
 		}
 
 		$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 @@ class PublicPreviewController extends PublicShareController {
 
 			$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);

apps/files_sharing/lib/ViewOnly.php

@@ -89,17 +89,10 @@ class ViewOnly {
 		/** @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;
 	}
 }

apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php

@@ -19,6 +19,7 @@ use OCP\IPreview;
 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 @@ 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();
@@ -42,6 +40,7 @@ class PublicPreviewControllerTest extends TestCase {
 		$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 @@ class PublicPreviewControllerTest extends TestCase {
 
 		$this->controller = new PublicPreviewController(
 			'files_sharing',
-			$this->createMock(IRequest::class),
+			$this->request,
 			$this->shareManager,
 			$this->createMock(ISession::class),
 			$this->previewManager
@@ -104,7 +103,109 @@ class PublicPreviewControllerTest extends TestCase {
 		$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'))

core/Controller/PreviewController.php

@@ -8,7 +8,6 @@ declare(strict_types=1);
  */
 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\File;
 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 @@ class PreviewController extends Controller {
 			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);
 			}
 		}

tests/Core/Controller/PreviewControllerTest.php

@@ -14,34 +14,35 @@ use OCP\Files\Folder;
 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,31 +125,7 @@ class PreviewControllerTest extends \Test\TestCase {
 		$this->assertEquals($expected, $res);
 	}
 
-	public function testForbiddenFile(): void {
-		$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);
-
-		$this->previewManager->method('isAvailable')
-			->with($this->equalTo($file))
-			->willReturn(true);
-
-		$file->method('isReadable')
-			->willReturn(false);
-
-		$res = $this->controller->getPreview('file', 10, 10, true, true);
-		$expected = new DataResponse([], Http::STATUS_FORBIDDEN);
-
-		$this->assertEquals($expected, $res);
-	}
-
-	public function testNoPreview(): void {
+	public function testNoPreview() {
 		$userFolder = $this->createMock(Folder::class);
 		$this->rootFolder->method('getUserFolder')
 			->with($this->equalTo($this->userId))
@@ -179,6 +156,133 @@ class PreviewControllerTest extends \Test\TestCase {
 
 		$this->assertEquals($expected, $res);
 	}
+	public function testFileWithoutReadPermission() {
+		$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);
+
+		$this->previewManager->method('isAvailable')
+			->with($this->equalTo($file))
+			->willReturn(true);
+
+		$file->method('isReadable')
+			->willReturn(false);
+
+		$res = $this->controller->getPreview('file', 10, 10, true, true);
+		$expected = new DataResponse([], Http::STATUS_FORBIDDEN);
+
+		$this->assertEquals($expected, $res);
+	}
+
+	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);
+
+		$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'))
+			->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'));
+	}
 
 	public function testValidPreview(): void {
 		$userFolder = $this->createMock(Folder::class);
@@ -218,4 +322,57 @@ class PreviewControllerTest extends \Test\TestCase {
 		$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'));
+	}
 }