From 9806a9830c381d04c7c3379d6241f764b58e97e3 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Wed, 25 Jun 2025 08:49:14 +0200 Subject: [PATCH 1/2] feat(files_sharing): allow viewing files with download disabled Signed-off-by: skjnldsv --- apps/dav/lib/DAV/ViewOnlyPlugin.php | 13 ++- .../dav/tests/unit/DAV/ViewOnlyPluginTest.php | 28 +++++-- apps/files/lib/Controller/ApiController.php | 7 +- .../tests/Controller/ApiControllerTest.php | 15 +--- .../Controller/PublicPreviewController.php | 7 +- .../PublicPreviewControllerTest.php | 28 +++---- apps/settings/lib/Settings/Admin/Sharing.php | 1 + .../components/AdminSettingsSharingForm.vue | 10 +++ .../features/bootstrap/SharingContext.php | 1 + .../sharing_features/sharing-v1-part2.feature | 34 +++++++- core/Controller/PreviewController.php | 5 +- .../e2e/files_sharing/files-download.cy.ts | 83 +++++++++++++++++++ .../e2e/files_versions/version_download.cy.ts | 5 ++ dist/settings-vue-settings-admin-sharing.js | 4 +- ...settings-vue-settings-admin-sharing.js.map | 2 +- lib/private/Share20/Manager.php | 4 + lib/private/Share20/Share.php | 21 +++++ lib/public/Share/IManager.php | 9 ++ lib/public/Share/IShare.php | 7 ++ .../Core/Controller/PreviewControllerTest.php | 24 ++---- 20 files changed, 234 insertions(+), 74 deletions(-) create mode 100644 cypress/e2e/files_sharing/files-download.cy.ts diff --git a/apps/dav/lib/DAV/ViewOnlyPlugin.php b/apps/dav/lib/DAV/ViewOnlyPlugin.php index 4c3b49a45b0..9b9615b8063 100644 --- a/apps/dav/lib/DAV/ViewOnlyPlugin.php +++ b/apps/dav/lib/DAV/ViewOnlyPlugin.php @@ -84,18 +84,25 @@ class ViewOnlyPlugin extends ServerPlugin { if (!$storage->instanceOfStorage(ISharedStorage::class)) { return true; } + // Extract extra permissions /** @var ISharedStorage $storage */ $share = $storage->getShare(); - $attributes = $share->getAttributes(); if ($attributes === null) { return true; } - // Check if read-only and on whether permission can download is both set and disabled. + // We have two options here, if download is disabled, but viewing is allowed, + // we still allow the GET request to return the file content. $canDownload = $attributes->getAttribute('permissions', 'download'); - if ($canDownload !== null && !$canDownload) { + if (!$share->canSeeContent()) { + throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.'); + } + + // If download is disabled, we disable the COPY and MOVE methods even if the + // shareapi_allow_view_without_download is set to true. + if ($request->getMethod() !== 'GET' && ($canDownload !== null && !$canDownload)) { throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.'); } } catch (NotFound $e) { diff --git a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php index 9227d34b30f..742258f4fc6 100644 --- a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php +++ b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php @@ -74,24 +74,30 @@ class ViewOnlyPluginTest extends TestCase { public static function providesDataForCanGet(): array { return [ // has attribute permissions-download enabled - can get file - [false, true, true], + [false, true, true, true], // has no attribute permissions-download - can get file - [false, null, true], - // has attribute permissions-download disabled- cannot get the file - [false, false, false], + [false, null, true, true], // has attribute permissions-download enabled - can get file version - [true, true, true], + [true, true, true, true], // has no attribute permissions-download - can get file version - [true, null, true], - // has attribute permissions-download disabled- cannot get the file version - [true, false, false], + [true, null, true, true], + // has attribute permissions-download disabled - cannot get the file + [false, false, false, false], + // has attribute permissions-download disabled - cannot get the file version + [true, false, false, false], + + // Has global allowViewWithoutDownload option enabled + // has attribute permissions-download disabled - can get file + [false, false, false, true], + // has attribute permissions-download disabled - can get file version + [true, false, false, true], ]; } /** * @dataProvider providesDataForCanGet */ - public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile): void { + public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanDownloadFile, bool $allowViewWithoutDownload): void { $nodeInfo = $this->createMock(File::class); if ($isVersion) { $davPath = 'versions/alice/versions/117/123456'; @@ -150,6 +156,10 @@ class ViewOnlyPluginTest extends TestCase { ->method('getAttribute') ->with('permissions', 'download') ->willReturn($attrEnabled); + + $share->expects($this->once()) + ->method('canSeeContent') + ->willReturn($allowViewWithoutDownload); if (!$expectCanDownloadFile) { $this->expectException(Forbidden::class); diff --git a/apps/files/lib/Controller/ApiController.php b/apps/files/lib/Controller/ApiController.php index 9c683b7f41f..8bb024fb698 100644 --- a/apps/files/lib/Controller/ApiController.php +++ b/apps/files/lib/Controller/ApiController.php @@ -105,11 +105,12 @@ class ApiController extends Controller { } // Validate the user is allowed to download the file (preview is some kind of download) + /** @var ISharedStorage $storage */ $storage = $file->getStorage(); if ($storage->instanceOfStorage(ISharedStorage::class)) { - /** @var ISharedStorage $storage */ - $attributes = $storage->getShare()->getAttributes(); - if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) { + /** @var IShare $share */ + $share = $storage->getShare(); + if (!$share->canSeeContent()) { throw new NotFoundException(); } } diff --git a/apps/files/tests/Controller/ApiControllerTest.php b/apps/files/tests/Controller/ApiControllerTest.php index 0c9d7a4fa6e..d6d86e293fd 100644 --- a/apps/files/tests/Controller/ApiControllerTest.php +++ b/apps/files/tests/Controller/ApiControllerTest.php @@ -29,7 +29,6 @@ use OCP\IPreview; use OCP\IRequest; use OCP\IUser; use OCP\IUserSession; -use OCP\Share\IAttributes; use OCP\Share\IManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -183,16 +182,10 @@ class ApiControllerTest extends TestCase { } public function testGetThumbnailSharedNoDownload(): void { - $attributes = $this->createMock(IAttributes::class); - $attributes->expects(self::once()) - ->method('getAttribute') - ->with('permissions', 'download') - ->willReturn(false); - $share = $this->createMock(IShare::class); $share->expects(self::once()) - ->method('getAttributes') - ->willReturn($attributes); + ->method('canSeeContent') + ->willReturn(false); $storage = $this->createMock(ISharedStorage::class); $storage->expects(self::once()) @@ -221,8 +214,8 @@ class ApiControllerTest extends TestCase { public function testGetThumbnailShared(): void { $share = $this->createMock(IShare::class); $share->expects(self::once()) - ->method('getAttributes') - ->willReturn(null); + ->method('canSeeContent') + ->willReturn(true); $storage = $this->createMock(ISharedStorage::class); $storage->expects(self::once()) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index f275001f162..0bcc18f2fa8 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -102,9 +102,9 @@ class PublicPreviewController extends PublicShareController { return new DataResponse([], Http::STATUS_FORBIDDEN); } - $attributes = $share->getAttributes(); // Only explicitly set to false will forbid the download! - $downloadForbidden = $attributes?->getAttribute('permissions', 'download') === false; + $downloadForbidden = !$share->canSeeContent(); + // 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'; @@ -181,8 +181,7 @@ class PublicPreviewController extends PublicShareController { return new DataResponse([], Http::STATUS_FORBIDDEN); } - $attributes = $share->getAttributes(); - if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) { + if (!$share->canSeeContent()) { return new DataResponse([], Http::STATUS_FORBIDDEN); } diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index 41789cdd138..02f1081e93f 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -20,7 +20,6 @@ use OCP\IRequest; use OCP\ISession; use OCP\Preview\IMimeIconProvider; use OCP\Share\Exceptions\ShareNotFound; -use OCP\Share\IAttributes; use OCP\Share\IManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -114,12 +113,8 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getPermissions') ->willReturn(Constants::PERMISSION_READ); - $attributes = $this->createMock(IAttributes::class); - $attributes->method('getAttribute') - ->with('permissions', 'download') + $share->method('canSeeContent') ->willReturn(false); - $share->method('getAttributes') - ->willReturn($attributes); $res = $this->controller->getPreview('token', 'file', 10, 10); $expected = new DataResponse([], Http::STATUS_FORBIDDEN); @@ -136,12 +131,8 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getPermissions') ->willReturn(Constants::PERMISSION_READ); - $attributes = $this->createMock(IAttributes::class); - $attributes->method('getAttribute') - ->with('permissions', 'download') + $share->method('canSeeContent') ->willReturn(false); - $share->method('getAttributes') - ->willReturn($attributes); $this->request->method('getHeader') ->with('x-nc-preview') @@ -176,12 +167,8 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getPermissions') ->willReturn(Constants::PERMISSION_READ); - $attributes = $this->createMock(IAttributes::class); - $attributes->method('getAttribute') - ->with('permissions', 'download') + $share->method('canSeeContent') ->willReturn(true); - $share->method('getAttributes') - ->willReturn($attributes); $this->request->method('getHeader') ->with('x-nc-preview') @@ -220,6 +207,9 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getNode') ->willReturn($file); + $share->method('canSeeContent') + ->willReturn(true); + $preview = $this->createMock(ISimpleFile::class); $preview->method('getName')->willReturn('name'); $preview->method('getMTime')->willReturn(42); @@ -249,6 +239,9 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getNode') ->willReturn($folder); + $share->method('canSeeContent') + ->willReturn(true); + $folder->method('get') ->with($this->equalTo('file')) ->willThrowException(new NotFoundException()); @@ -272,6 +265,9 @@ class PublicPreviewControllerTest extends TestCase { $share->method('getNode') ->willReturn($folder); + $share->method('canSeeContent') + ->willReturn(true); + $file = $this->createMock(File::class); $folder->method('get') ->with($this->equalTo('file')) diff --git a/apps/settings/lib/Settings/Admin/Sharing.php b/apps/settings/lib/Settings/Admin/Sharing.php index e001a7d00ea..a3d35bacb25 100644 --- a/apps/settings/lib/Settings/Admin/Sharing.php +++ b/apps/settings/lib/Settings/Admin/Sharing.php @@ -72,6 +72,7 @@ class Sharing implements IDelegatedSettings { 'remoteExpireAfterNDays' => $this->config->getAppValue('core', 'shareapi_remote_expire_after_n_days', '7'), 'enforceRemoteExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_remote_expire_date'), 'allowCustomTokens' => $this->shareManager->allowCustomTokens(), + 'allowViewWithoutDownload' => $this->shareManager->allowViewWithoutDownload(), ]; $this->initialState->provideInitialState('sharingAppEnabled', $this->appManager->isEnabledForUser('files_sharing')); diff --git a/apps/settings/src/components/AdminSettingsSharingForm.vue b/apps/settings/src/components/AdminSettingsSharingForm.vue index 25dcbb7972d..1973781edee 100644 --- a/apps/settings/src/components/AdminSettingsSharingForm.vue +++ b/apps/settings/src/components/AdminSettingsSharingForm.vue @@ -27,6 +27,15 @@ :label="t('settings', 'Ignore the following groups when checking group membership')" style="width: 100%" /> + + {{ t('settings', 'Allow users to preview files even if download is disabled') }} + +