Skip to content

Commit

Permalink
fix: Adjust preview for view-only shares
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@susnux
susnux committed Sep 8, 2024
1 parent 926bd29 commit 73dad8a
Show file tree
Hide file tree
Showing 5 changed files with 310 additions and 42 deletions.
16 changes: 14 additions & 2 deletions apps/files_sharing/lib/Controller/PublicPreviewController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,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);
}
Expand All @@ -100,7 +102,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);
}

Expand All @@ -114,7 +126,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);
Expand Down
13 changes: 3 additions & 10 deletions apps/files_sharing/lib/ViewOnly.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,10 @@ private function checkFileInfo(Node $fileInfo): bool {
/** @var \OCA\Files_Sharing\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;
}
}
119 changes: 110 additions & 9 deletions apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -104,6 +103,108 @@ public function testShareNotAccessable() {
$this->assertEquals($expected, $res);
}

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')
Expand Down
13 changes: 9 additions & 4 deletions core/Controller/PreviewController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down
Loading

0 comments on commit 73dad8a

Please sign in to comment.