Merge pull request #53671 from nextcloud/fix/read-only-share-download

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) {

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);

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();
 				}
 			}

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())

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);
 		}
 

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'))

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'));

apps/settings/src/components/AdminSettingsSharingForm.vue

@@ -27,6 +27,15 @@
 					:label="t('settings', 'Ignore the following groups when checking group membership')"
 					style="width: 100%" />
 			</div>
+			<NcCheckboxRadioSwitch :checked.sync="settings.allowViewWithoutDownload">
+				{{ t('settings', 'Allow users to preview files even if download is disabled') }}
+			</NcCheckboxRadioSwitch>
+			<NcNoteCard v-show="settings.allowViewWithoutDownload"
+				id="settings-sharing-api-view-without-download-hint"
+				class="sharing__note"
+				type="warning">
+				{{ t('settings', 'Users will still be able to screenshot or record the screen. This does not provide any definitive protection.') }}
+			</NcNoteCard>
 		</div>
 
 		<div v-show="settings.enabled" id="settings-sharing-api" class="sharing__section">
@@ -258,6 +267,7 @@ interface IShareSettings {
 	remoteExpireAfterNDays: string
 	enforceRemoteExpireDate: boolean
 	allowCustomTokens: boolean
+	allowViewWithoutDownload: boolean
 }
 
 export default defineComponent({

build/integration/features/bootstrap/SharingContext.php

@@ -29,6 +29,7 @@ class SharingContext implements Context, SnippetAcceptingContext {
 		$this->deleteServerConfig('core', 'shareapi_expire_after_n_days');
 		$this->deleteServerConfig('core', 'link_defaultExpDays');
 		$this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled');
+		$this->deleteServerConfig('core', 'shareapi_allow_view_without_download');
 
 		$this->runOcc(['config:system:delete', 'share_folder']);
 	}

build/integration/sharing_features/sharing-v1-part2.feature

@@ -1265,7 +1265,9 @@ Feature: sharing
       |{http://open-collaboration-services.org/ns}share-permissions |
     Then the single response should contain a property "{http://open-collaboration-services.org/ns}share-permissions" with value "19"
 
-  Scenario: Cannot download a file when it's shared view-only
+  Scenario: Cannot download a file when it's shared view-only without shareapi_allow_view_without_download
+    Given As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
     Given user "user0" exists
     And user "user1" exists
     And User "user0" moves file "/textfile0.txt" to "/document.odt"
@@ -1274,8 +1276,15 @@ Feature: sharing
     When As an "user1"
     And Downloading file "/document.odt"
     Then the HTTP status code should be "403"
+    Then As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
+    Then As an "user1"
+    And Downloading file "/document.odt"
+    Then the HTTP status code should be "200"
 
-  Scenario: Cannot download a file when its parent is shared view-only
+  Scenario: Cannot download a file when its parent is shared view-only without shareapi_allow_view_without_download
+    Given As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
     Given user "user0" exists
     And user "user1" exists
     And User "user0" created a folder "/sharedviewonly"
@@ -1285,8 +1294,15 @@ Feature: sharing
     When As an "user1"
     And Downloading file "/sharedviewonly/document.odt"
     Then the HTTP status code should be "403"
+    Then As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
+    Then As an "user1"
+    And Downloading file "/sharedviewonly/document.odt"
+    Then the HTTP status code should be "200"
 
-  Scenario: Cannot copy a file when it's shared view-only
+  Scenario: Cannot copy a file when it's shared view-only even with shareapi_allow_view_without_download enabled
+    Given As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
     Given user "user0" exists
     And user "user1" exists
     And User "user0" moves file "/textfile0.txt" to "/document.odt"
@@ -1294,8 +1310,15 @@ Feature: sharing
     And user "user1" accepts last share
     When User "user1" copies file "/document.odt" to "/copyforbidden.odt"
     Then the HTTP status code should be "403"
+    Then As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
+    Then As an "user1"
+    And User "user1" copies file "/document.odt" to "/copyforbidden.odt"
+    Then the HTTP status code should be "403"
 
   Scenario: Cannot copy a file when its parent is shared view-only
+    Given As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "no"
     Given user "user0" exists
     And user "user1" exists
     And User "user0" created a folder "/sharedviewonly"
@@ -1304,5 +1327,10 @@ Feature: sharing
     And user "user1" accepts last share
     When User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
     Then the HTTP status code should be "403"
+    Then As an "admin"
+    And parameter "shareapi_allow_view_without_download" of app "core" is set to "yes"
+    Then As an "user1"
+    And User "user1" copies file "/sharedviewonly/document.odt" to "/copyforbidden.odt"
+    Then the HTTP status code should be "403"
 
 # See sharing-v1-part3.feature

core/Controller/PreviewController.php

@@ -157,10 +157,7 @@ class PreviewController extends Controller {
 		if ($isNextcloudPreview === false && $storage->instanceOfStorage(ISharedStorage::class)) {
 			/** @var ISharedStorage $storage */
 			$share = $storage->getShare();
-			$attributes = $share->getAttributes();
-			// No "allow preview" header set, so we must check if
-			// the share has not explicitly disabled download permissions
-			if ($attributes?->getAttribute('permissions', 'download') === false) {
+			if (!$share->canSeeContent()) {
 				return new DataResponse([], Http::STATUS_FORBIDDEN);
 			}
 		}

cypress/e2e/files_sharing/files-download.cy.ts

@@ -0,0 +1,83 @@
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+import type { User } from '@nextcloud/cypress'
+import { createShare } from './FilesSharingUtils.ts'
+import {
+	getActionEntryForFile,
+	getRowForFile,
+} from '../files/FilesUtils.ts'
+
+describe('files_sharing: Download forbidden', { testIsolation: true }, () => {
+	let user: User
+	let sharee: User
+
+	beforeEach(() => {
+		cy.runOccCommand('config:app:set --value yes core shareapi_allow_view_without_download')
+		cy.createRandomUser().then(($user) => {
+			user = $user
+		})
+		cy.createRandomUser().then(($user) => {
+			sharee = $user
+		})
+	})
+
+	after(() => {
+		cy.runOccCommand('config:app:delete core shareapi_allow_view_without_download')
+	})
+
+	it('cannot download a folder if disabled', () => {
+		// share the folder
+		cy.mkdir(user, '/folder')
+		cy.login(user)
+		cy.visit('/apps/files')
+		createShare('folder', sharee.userId, { read: true, download: false })
+		cy.logout()
+
+		// Now for the sharee
+		cy.login(sharee)
+
+		// visit shared files view
+		cy.visit('/apps/files')
+		// see the shared folder
+		getRowForFile('folder').should('be.visible')
+		getActionEntryForFile('folder', 'download').should('not.exist')
+
+		// Disable view without download option
+		cy.runOccCommand('config:app:set --value no core shareapi_allow_view_without_download')
+
+		// visit shared files view
+		cy.visit('/apps/files')
+		// see the shared folder
+		getRowForFile('folder').should('be.visible')
+		getActionEntryForFile('folder', 'download').should('not.exist')
+	})
+
+	it('cannot download a file if disabled', () => {
+		// share the folder
+		cy.uploadContent(user, new Blob([]), 'text/plain', '/file.txt')
+		cy.login(user)
+		cy.visit('/apps/files')
+		createShare('file.txt', sharee.userId, { read: true, download: false })
+		cy.logout()
+
+		// Now for the sharee
+		cy.login(sharee)
+
+		// visit shared files view
+		cy.visit('/apps/files')
+		// see the shared folder
+		getRowForFile('file.txt').should('be.visible')
+		getActionEntryForFile('file.txt', 'download').should('not.exist')
+
+		// Disable view without download option
+		cy.runOccCommand('config:app:set --value no core shareapi_allow_view_without_download')
+
+		// visit shared files view
+		cy.visit('/apps/files')
+		// see the shared folder
+		getRowForFile('file.txt').should('be.visible')
+		getActionEntryForFile('file.txt', 'download').should('not.exist')
+	})
+})

cypress/e2e/files_versions/version_download.cy.ts

@@ -14,6 +14,7 @@ describe('Versions download', () => {
 	before(() => {
 		randomFileName = Math.random().toString(36).replace(/[^a-z]+/g, '').substring(0, 10) + '.txt'
 
+		cy.runOccCommand('config:app:set --value no core shareapi_allow_view_without_download')
 		cy.createRandomUser()
 			.then((_user) => {
 				user = _user
@@ -24,6 +25,10 @@ describe('Versions download', () => {
 			})
 	})
 
+	after(() => {
+		cy.runOccCommand('config:app:delete core shareapi_allow_view_without_download')
+	})
+
 	it('Download versions and assert their content', () => {
 		assertVersionContent(0, 'v3')
 		assertVersionContent(1, 'v2')

lib/private/Share20/Manager.php

@@ -1940,6 +1940,10 @@ class Manager implements IManager {
 		return $this->appConfig->getValueBool('core', 'shareapi_allow_custom_tokens', false);
 	}
 
+	public function allowViewWithoutDownload(): bool {
+		return $this->appConfig->getValueBool('core', 'shareapi_allow_view_without_download', true);
+	}
+
 	public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $targetUser): bool {
 		if ($this->allowEnumerationFullMatch()) {
 			return true;

lib/private/Share20/Share.php

@@ -14,8 +14,10 @@ use OCP\Files\IRootFolder;
 use OCP\Files\Node;
 use OCP\Files\NotFoundException;
 use OCP\IUserManager;
+use OCP\Server;
 use OCP\Share\Exceptions\IllegalIDChangeException;
 use OCP\Share\IAttributes;
+use OCP\Share\IManager;
 use OCP\Share\IShare;
 
 class Share implements IShare {
@@ -622,4 +624,23 @@ class Share implements IShare {
 	public function getReminderSent(): bool {
 		return $this->reminderSent;
 	}
+
+	public function canSeeContent(): bool {
+		$shareManager = Server::get(IManager::class);
+
+		$allowViewWithoutDownload = $shareManager->allowViewWithoutDownload();
+		// If the share manager allows viewing without download, we can always see the content.
+		if ($allowViewWithoutDownload) {
+			return true;
+		}
+		
+		// No "allow preview" header set, so we must check if
+		// the share has not explicitly disabled download permissions
+		$attributes = $this->getAttributes();
+		if ($attributes?->getAttribute('permissions', 'download') === false) {
+			return false;
+		}
+
+		return true;
+	}
 }

lib/public/Share/IManager.php

@@ -472,6 +472,14 @@ interface IManager {
 	 */
 	public function allowCustomTokens(): bool;
 
+	/**
+	 * Check if the current user can view the share
+	 * even if the download is disabled.
+	 *
+	 * @since 32.0.0
+	 */
+	public function allowViewWithoutDownload(): bool;
+
 	/**
 	 * Check if the current user can enumerate the target user
 	 *

lib/public/Share/IShare.php

@@ -633,4 +633,11 @@ interface IShare {
 	 * @since 31.0.0
 	 */
 	public function getReminderSent(): bool;
+
+	/**
+	 * Check if the current user can see this share files contents.
+	 * This will check the download permissions as well as the global
+	 * admin setting to allow viewing files without downloading.
+	 */
+	public function canSeeContent(): bool;
 }

tests/Core/Controller/PreviewControllerTest.php

@@ -19,7 +19,6 @@ 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;
 
@@ -196,15 +195,9 @@ class PreviewControllerTest extends \Test\TestCase {
 			->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);
+		$share->method('canSeeContent')
+			->willReturn(false);
 
 		$storage = $this->createMock(ISharedStorage::class);
 		$storage->method('instanceOfStorage')
@@ -242,14 +235,9 @@ class PreviewControllerTest extends \Test\TestCase {
 			->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);
+		$share->method('canSeeContent')
+			->willReturn(false);
 
 		$storage = $this->createMock(ISharedStorage::class);
 		$storage->method('instanceOfStorage')
@@ -341,8 +329,8 @@ class PreviewControllerTest extends \Test\TestCase {
 
 		// No attributes set -> download permitted
 		$share = $this->createMock(IShare::class);
-		$share->method('getAttributes')
-			->willReturn(null);
+		$share->method('canSeeContent')
+			->willReturn(true);
 
 		$storage = $this->createMock(ISharedStorage::class);
 		$storage->method('instanceOfStorage')