Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Adjust preview for view-only shares #47831

Merged
merged 1 commit into from
Oct 28, 2024
+311 −43
Merged

fix: Adjust preview for view-only shares #47831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions apps/files_sharing/lib/Controller/PublicPreviewController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}

Expand All @@ -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);
Expand Down
13 changes: 3 additions & 10 deletions apps/files_sharing/lib/ViewOnly.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
121 changes: 111 additions & 10 deletions apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php
View file
Open in desktop
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,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'))
Expand Down
13 changes: 9 additions & 4 deletions core/Controller/PreviewController.php
View file
Open in desktop
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
Loading