From 8725129312bd6ab81188506b354be7d072eb4288 Mon Sep 17 00:00:00 2001 From: nfebe Date: Thu, 18 Sep 2025 18:45:40 +0100 Subject: [PATCH] fix(sharing): Allow reasonable control 4 'Hide download' on fed shares When creating public links from federated shares, users should be able to set the 'Hide download' option independently as long as they are more restrictive than the original share permissions. Previously, the `checkInheritedAttributes` method was ignoring user preferences and always overriding the hideDownload setting based solely on inherited permissions, preventing users from disabling downloads even when the parent share allowed them. This fix implements some sort of inheritance logic: - Users can only be MORE restrictive than parent shares, never LESS restrictive - If parent hides downloads -> child MUST hide downloads (enforced) - If parent allows downloads -> child can CHOOSE to hide or allow downloads - If parent forbids downloads entirely -> child cannot enable downloads Signed-off-by: nfebe --- .../lib/Controller/ShareAPIController.php | 48 ++-- .../Controller/ShareAPIControllerTest.php | 216 ++++++++++++++++++ 2 files changed, 251 insertions(+), 13 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 6511135a510..bca92e095ed 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -2095,14 +2095,15 @@ class ShareAPIController extends OCSController { $canDownload = false; $hideDownload = true; + $userExplicitlySetHideDownload = $share->getHideDownload(); // Capture user's explicit choice $userFolder = $this->rootFolder->getUserFolder($share->getSharedBy()); $nodes = $userFolder->getById($share->getNodeId()); foreach ($nodes as $node) { - // Owner always can download it - so allow it and break + // Owner always can download it - so allow it, but respect their explicit choice about hiding downloads if ($node->getOwner()?->getUID() === $share->getSharedBy()) { $canDownload = true; - $hideDownload = false; + $hideDownload = $userExplicitlySetHideDownload; break; } @@ -2120,23 +2121,44 @@ class ShareAPIController extends OCSController { /** @var SharedStorage $storage */ $originalShare = $storage->getShare(); $inheritedAttributes = $originalShare->getAttributes(); - // hide if hidden and also the current share enforces hide (can only be false if one share is false or user is owner) - $hideDownload = $hideDownload && $originalShare->getHideDownload(); - // allow download if already allowed by previous share or when the current share allows downloading - $canDownload = $canDownload || $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false; + + // For federated shares: users can only be MORE restrictive, never LESS restrictive + // If parent has hideDownload=true, child MUST have hideDownload=true + $parentHidesDownload = $originalShare->getHideDownload(); + + // Check if download permission is available from parent + $parentAllowsDownload = $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false; + + // Apply inheritance rules: + // 1. If parent hides download, child must hide download + // 2. If parent allows download, child can choose to hide or allow + // 3. If parent forbids download, child cannot allow download + $hideDownload = $parentHidesDownload || $userExplicitlySetHideDownload; + + $canDownload = $canDownload || $parentAllowsDownload; + } elseif ($node->getStorage()->instanceOfStorage(Storage::class)) { $canDownload = true; // in case of federation storage, we can expect the download to be activated by default + // For external federation storage, respect user's choice if downloads are available + $hideDownload = $userExplicitlySetHideDownload; } } - if ($hideDownload || !$canDownload) { + // Apply the final restrictions: + // 1. If parent doesn't allow downloads at all, force hide and disable download attribute + // 2. If parent allows downloads, respect user's hideDownload choice + if (!$canDownload) { + // Parent completely forbids downloads - must enforce this restriction $share->setHideDownload(true); - - if (!$canDownload) { - $attributes = $share->getAttributes() ?? $share->newAttributes(); - $attributes->setAttribute('permissions', 'download', false); - $share->setAttributes($attributes); - } + $attributes = $share->getAttributes() ?? $share->newAttributes(); + $attributes->setAttribute('permissions', 'download', false); + $share->setAttributes($attributes); + } elseif ($hideDownload) { + // Either parent forces hide, or user chooses to hide - respect this + $share->setHideDownload(true); + } else { + // User explicitly wants to allow downloads and parent permits it + $share->setHideDownload(false); } } diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 4ced55ab7c9..1030285463b 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -6,8 +6,11 @@ */ namespace OCA\Files_Sharing\Tests\Controller; +use OC\Files\Storage\Wrapper\Wrapper; use OCA\Federation\TrustedServers; use OCA\Files_Sharing\Controller\ShareAPIController; +use OCA\Files_Sharing\External\Storage; +use OCA\Files_Sharing\SharedStorage; use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSBadRequestException; @@ -5299,4 +5302,217 @@ class ShareAPIControllerTest extends TestCase { $this->assertTrue($result['is_trusted_server']); } + + public function testOwnerCanAlwaysDownload(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('sharedByUser'); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // Expect hideDownload to be set to false since owner can always download + $share->expects($this->once())->method('setHideDownload')->with(false); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testParentHideDownloadEnforcedOnChild(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $storage = $this->createMock(SharedStorage::class); + $originalShare = $this->createMock(IShare::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(false); // User wants to allow downloads + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($storage); + $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); + $storage->method('getShare')->willReturn($originalShare); + $originalShare->method('getHideDownload')->willReturn(true); // Parent hides download + $originalShare->method('getAttributes')->willReturn(null); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // Should be forced to hide download due to parent restriction + $share->expects($this->once())->method('setHideDownload')->with(true); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testUserCanHideWhenParentAllows(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $storage = $this->createMock(SharedStorage::class); + $originalShare = $this->createMock(IShare::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($storage); + $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); + $storage->method('getShare')->willReturn($originalShare); + $originalShare->method('getHideDownload')->willReturn(false); // Parent allows download + $originalShare->method('getAttributes')->willReturn(null); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // Should respect user's choice to hide downloads + $share->expects($this->once())->method('setHideDownload')->with(true); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testParentDownloadAttributeInherited(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $storage = $this->createMock(SharedStorage::class); + $originalShare = $this->createMock(IShare::class); + $attributes = $this->createMock(\OCP\Share\IAttributes::class); + $shareAttributes = $this->createMock(\OCP\Share\IAttributes::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(false); // User wants to allow downloads + $share->method('getAttributes')->willReturn($shareAttributes); + $share->method('newAttributes')->willReturn($shareAttributes); + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($storage); + $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); + $storage->method('getShare')->willReturn($originalShare); + $originalShare->method('getHideDownload')->willReturn(false); + $originalShare->method('getAttributes')->willReturn($attributes); + $attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // Should be forced to hide download and set download attribute to false + $share->expects($this->once())->method('setHideDownload')->with(true); + $shareAttributes->expects($this->once())->method('setAttribute')->with('permissions', 'download', false); + $share->expects($this->once())->method('setAttributes')->with($shareAttributes); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testFederatedStorageRespectsUserChoice(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($storage); + $storage->method('instanceOfStorage')->willReturnMap([ + [SharedStorage::class, false], + [\OCA\Files_Sharing\External\Storage::class, true] + ]); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // For federated storage, should respect user's choice + $share->expects($this->once())->method('setHideDownload')->with(true); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testUserAllowsDownloadWhenParentPermits(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $storage = $this->createMock(SharedStorage::class); + $originalShare = $this->createMock(IShare::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(false); // User wants to allow downloads + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($storage); + $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); + $storage->method('getShare')->willReturn($originalShare); + $originalShare->method('getHideDownload')->willReturn(false); // Parent allows download + $originalShare->method('getAttributes')->willReturn(null); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + // Should allow downloads as both user and parent permit it + $share->expects($this->once())->method('setHideDownload')->with(false); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } + + public function testWrapperStorageUnwrapped(): void { + $ocs = $this->mockFormatShare(); + + $share = $this->createMock(IShare::class); + $node = $this->createMock(File::class); + $userFolder = $this->createMock(Folder::class); + $owner = $this->createMock(IUser::class); + $wrapperStorage = $this->createMock(Wrapper::class); + $innerStorage = $this->createMock(SharedStorage::class); + $originalShare = $this->createMock(IShare::class); + + $share->method('getSharedBy')->willReturn('sharedByUser'); + $share->method('getNodeId')->willReturn(42); + $share->method('getHideDownload')->willReturn(false); + $node->method('getOwner')->willReturn($owner); + $owner->method('getUID')->willReturn('differentOwner'); + $node->method('getStorage')->willReturn($wrapperStorage); + $wrapperStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $wrapperStorage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($innerStorage); + $innerStorage->method('getShare')->willReturn($originalShare); + $originalShare->method('getHideDownload')->willReturn(false); + $originalShare->method('getAttributes')->willReturn(null); + + $userFolder->method('getById')->with(42)->willReturn([$node]); + $this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder); + + $share->expects($this->once())->method('setHideDownload')->with(false); + + $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); + } }