Skip to content

Commit 299662e

Browse files
nfebenfebe
committed
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 <fenn25.fn@gmail.com>
1 parent 72eb064 commit 299662e

2 files changed

Lines changed: 251 additions & 13 deletions

File tree

‎apps/files_sharing/lib/Controller/ShareAPIController.php‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2096,14 +2096,15 @@ private function checkInheritedAttributes(IShare $share): void {
20962096

20972097
$canDownload = false;
20982098
$hideDownload = true;
2099+
$userExplicitlySetHideDownload = $share->getHideDownload(); // Capture user's explicit choice
20992100

21002101
$userFolder = $this->rootFolder->getUserFolder($share->getSharedBy());
21012102
$nodes = $userFolder->getById($share->getNodeId());
21022103
foreach ($nodes as $node) {
2103-
// Owner always can download it - so allow it and break
2104+
// Owner always can download it - so allow it, but respect their explicit choice about hiding downloads
21042105
if ($node->getOwner()?->getUID() === $share->getSharedBy()) {
21052106
$canDownload = true;
2106-
$hideDownload = false;
2107+
$hideDownload = $userExplicitlySetHideDownload;
21072108
break;
21082109
}
21092110

@@ -2121,23 +2122,44 @@ private function checkInheritedAttributes(IShare $share): void {
21212122
/** @var SharedStorage $storage */
21222123
$originalShare = $storage->getShare();
21232124
$inheritedAttributes = $originalShare->getAttributes();
2124-
// hide if hidden and also the current share enforces hide (can only be false if one share is false or user is owner)
2125-
$hideDownload = $hideDownload && $originalShare->getHideDownload();
2126-
// allow download if already allowed by previous share or when the current share allows downloading
2127-
$canDownload = $canDownload || $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false;
2125+
2126+
// For federated shares: users can only be MORE restrictive, never LESS restrictive
2127+
// If parent has hideDownload=true, child MUST have hideDownload=true
2128+
$parentHidesDownload = $originalShare->getHideDownload();
2129+
2130+
// Check if download permission is available from parent
2131+
$parentAllowsDownload = $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false;
2132+
2133+
// Apply inheritance rules:
2134+
// 1. If parent hides download, child must hide download
2135+
// 2. If parent allows download, child can choose to hide or allow
2136+
// 3. If parent forbids download, child cannot allow download
2137+
$hideDownload = $parentHidesDownload || $userExplicitlySetHideDownload;
2138+
2139+
$canDownload = $canDownload || $parentAllowsDownload;
2140+
21282141
} elseif ($node->getStorage()->instanceOfStorage(Storage::class)) {
21292142
$canDownload = true; // in case of federation storage, we can expect the download to be activated by default
2143+
// For external federation storage, respect user's choice if downloads are available
2144+
$hideDownload = $userExplicitlySetHideDownload;
21302145
}
21312146
}
21322147

2133-
if ($hideDownload || !$canDownload) {
2148+
// Apply the final restrictions:
2149+
// 1. If parent doesn't allow downloads at all, force hide and disable download attribute
2150+
// 2. If parent allows downloads, respect user's hideDownload choice
2151+
if (!$canDownload) {
2152+
// Parent completely forbids downloads - must enforce this restriction
21342153
$share->setHideDownload(true);
2135-
2136-
if (!$canDownload) {
2137-
$attributes = $share->getAttributes() ?? $share->newAttributes();
2138-
$attributes->setAttribute('permissions', 'download', false);
2139-
$share->setAttributes($attributes);
2140-
}
2154+
$attributes = $share->getAttributes() ?? $share->newAttributes();
2155+
$attributes->setAttribute('permissions', 'download', false);
2156+
$share->setAttributes($attributes);
2157+
} elseif ($hideDownload) {
2158+
// Either parent forces hide, or user chooses to hide - respect this
2159+
$share->setHideDownload(true);
2160+
} else {
2161+
// User explicitly wants to allow downloads and parent permits it
2162+
$share->setHideDownload(false);
21412163
}
21422164
}
21432165

‎apps/files_sharing/tests/Controller/ShareAPIControllerTest.php‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,11 @@
77
*/
88
namespace OCA\Files_Sharing\Tests\Controller;
99

10+
use OC\Files\Storage\Wrapper\Wrapper;
1011
use OCA\Federation\TrustedServers;
1112
use OCA\Files_Sharing\Controller\ShareAPIController;
13+
use OCA\Files_Sharing\External\Storage;
14+
use OCA\Files_Sharing\SharedStorage;
1215
use OCP\App\IAppManager;
1316
use OCP\AppFramework\Http\DataResponse;
1417
use OCP\AppFramework\OCS\OCSBadRequestException;
@@ -5378,4 +5381,217 @@ public function testFormatShareWithFederatedShareWithAtInUsername(): void {
53785381

53795382
$this->assertTrue($result['is_trusted_server']);
53805383
}
5384+
5385+
public function testOwnerCanAlwaysDownload(): void {
5386+
$ocs = $this->mockFormatShare();
5387+
5388+
$share = $this->createMock(IShare::class);
5389+
$node = $this->createMock(File::class);
5390+
$userFolder = $this->createMock(Folder::class);
5391+
$owner = $this->createMock(IUser::class);
5392+
5393+
$share->method('getSharedBy')->willReturn('sharedByUser');
5394+
$share->method('getNodeId')->willReturn(42);
5395+
$node->method('getOwner')->willReturn($owner);
5396+
$owner->method('getUID')->willReturn('sharedByUser');
5397+
5398+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5399+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5400+
5401+
// Expect hideDownload to be set to false since owner can always download
5402+
$share->expects($this->once())->method('setHideDownload')->with(false);
5403+
5404+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5405+
}
5406+
5407+
public function testParentHideDownloadEnforcedOnChild(): void {
5408+
$ocs = $this->mockFormatShare();
5409+
5410+
$share = $this->createMock(IShare::class);
5411+
$node = $this->createMock(File::class);
5412+
$userFolder = $this->createMock(Folder::class);
5413+
$owner = $this->createMock(IUser::class);
5414+
$storage = $this->createMock(SharedStorage::class);
5415+
$originalShare = $this->createMock(IShare::class);
5416+
5417+
$share->method('getSharedBy')->willReturn('sharedByUser');
5418+
$share->method('getNodeId')->willReturn(42);
5419+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5420+
$node->method('getOwner')->willReturn($owner);
5421+
$owner->method('getUID')->willReturn('differentOwner');
5422+
$node->method('getStorage')->willReturn($storage);
5423+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5424+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5425+
$storage->method('getShare')->willReturn($originalShare);
5426+
$originalShare->method('getHideDownload')->willReturn(true); // Parent hides download
5427+
$originalShare->method('getAttributes')->willReturn(null);
5428+
5429+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5430+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5431+
5432+
// Should be forced to hide download due to parent restriction
5433+
$share->expects($this->once())->method('setHideDownload')->with(true);
5434+
5435+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5436+
}
5437+
5438+
public function testUserCanHideWhenParentAllows(): void {
5439+
$ocs = $this->mockFormatShare();
5440+
5441+
$share = $this->createMock(IShare::class);
5442+
$node = $this->createMock(File::class);
5443+
$userFolder = $this->createMock(Folder::class);
5444+
$owner = $this->createMock(IUser::class);
5445+
$storage = $this->createMock(SharedStorage::class);
5446+
$originalShare = $this->createMock(IShare::class);
5447+
5448+
$share->method('getSharedBy')->willReturn('sharedByUser');
5449+
$share->method('getNodeId')->willReturn(42);
5450+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5451+
$node->method('getOwner')->willReturn($owner);
5452+
$owner->method('getUID')->willReturn('differentOwner');
5453+
$node->method('getStorage')->willReturn($storage);
5454+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5455+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5456+
$storage->method('getShare')->willReturn($originalShare);
5457+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5458+
$originalShare->method('getAttributes')->willReturn(null);
5459+
5460+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5461+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5462+
5463+
// Should respect user's choice to hide downloads
5464+
$share->expects($this->once())->method('setHideDownload')->with(true);
5465+
5466+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5467+
}
5468+
5469+
public function testParentDownloadAttributeInherited(): void {
5470+
$ocs = $this->mockFormatShare();
5471+
5472+
$share = $this->createMock(IShare::class);
5473+
$node = $this->createMock(File::class);
5474+
$userFolder = $this->createMock(Folder::class);
5475+
$owner = $this->createMock(IUser::class);
5476+
$storage = $this->createMock(SharedStorage::class);
5477+
$originalShare = $this->createMock(IShare::class);
5478+
$attributes = $this->createMock(\OCP\Share\IAttributes::class);
5479+
$shareAttributes = $this->createMock(\OCP\Share\IAttributes::class);
5480+
5481+
$share->method('getSharedBy')->willReturn('sharedByUser');
5482+
$share->method('getNodeId')->willReturn(42);
5483+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5484+
$share->method('getAttributes')->willReturn($shareAttributes);
5485+
$share->method('newAttributes')->willReturn($shareAttributes);
5486+
$node->method('getOwner')->willReturn($owner);
5487+
$owner->method('getUID')->willReturn('differentOwner');
5488+
$node->method('getStorage')->willReturn($storage);
5489+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5490+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5491+
$storage->method('getShare')->willReturn($originalShare);
5492+
$originalShare->method('getHideDownload')->willReturn(false);
5493+
$originalShare->method('getAttributes')->willReturn($attributes);
5494+
$attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download
5495+
5496+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5497+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5498+
5499+
// Should be forced to hide download and set download attribute to false
5500+
$share->expects($this->once())->method('setHideDownload')->with(true);
5501+
$shareAttributes->expects($this->once())->method('setAttribute')->with('permissions', 'download', false);
5502+
$share->expects($this->once())->method('setAttributes')->with($shareAttributes);
5503+
5504+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5505+
}
5506+
5507+
public function testFederatedStorageRespectsUserChoice(): void {
5508+
$ocs = $this->mockFormatShare();
5509+
5510+
$share = $this->createMock(IShare::class);
5511+
$node = $this->createMock(File::class);
5512+
$userFolder = $this->createMock(Folder::class);
5513+
$owner = $this->createMock(IUser::class);
5514+
$storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class);
5515+
5516+
$share->method('getSharedBy')->willReturn('sharedByUser');
5517+
$share->method('getNodeId')->willReturn(42);
5518+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5519+
$node->method('getOwner')->willReturn($owner);
5520+
$owner->method('getUID')->willReturn('differentOwner');
5521+
$node->method('getStorage')->willReturn($storage);
5522+
$storage->method('instanceOfStorage')->willReturnMap([
5523+
[SharedStorage::class, false],
5524+
[\OCA\Files_Sharing\External\Storage::class, true]
5525+
]);
5526+
5527+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5528+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5529+
5530+
// For federated storage, should respect user's choice
5531+
$share->expects($this->once())->method('setHideDownload')->with(true);
5532+
5533+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5534+
}
5535+
5536+
public function testUserAllowsDownloadWhenParentPermits(): void {
5537+
$ocs = $this->mockFormatShare();
5538+
5539+
$share = $this->createMock(IShare::class);
5540+
$node = $this->createMock(File::class);
5541+
$userFolder = $this->createMock(Folder::class);
5542+
$owner = $this->createMock(IUser::class);
5543+
$storage = $this->createMock(SharedStorage::class);
5544+
$originalShare = $this->createMock(IShare::class);
5545+
5546+
$share->method('getSharedBy')->willReturn('sharedByUser');
5547+
$share->method('getNodeId')->willReturn(42);
5548+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5549+
$node->method('getOwner')->willReturn($owner);
5550+
$owner->method('getUID')->willReturn('differentOwner');
5551+
$node->method('getStorage')->willReturn($storage);
5552+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5553+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5554+
$storage->method('getShare')->willReturn($originalShare);
5555+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5556+
$originalShare->method('getAttributes')->willReturn(null);
5557+
5558+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5559+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5560+
5561+
// Should allow downloads as both user and parent permit it
5562+
$share->expects($this->once())->method('setHideDownload')->with(false);
5563+
5564+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5565+
}
5566+
5567+
public function testWrapperStorageUnwrapped(): void {
5568+
$ocs = $this->mockFormatShare();
5569+
5570+
$share = $this->createMock(IShare::class);
5571+
$node = $this->createMock(File::class);
5572+
$userFolder = $this->createMock(Folder::class);
5573+
$owner = $this->createMock(IUser::class);
5574+
$wrapperStorage = $this->createMock(Wrapper::class);
5575+
$innerStorage = $this->createMock(SharedStorage::class);
5576+
$originalShare = $this->createMock(IShare::class);
5577+
5578+
$share->method('getSharedBy')->willReturn('sharedByUser');
5579+
$share->method('getNodeId')->willReturn(42);
5580+
$share->method('getHideDownload')->willReturn(false);
5581+
$node->method('getOwner')->willReturn($owner);
5582+
$owner->method('getUID')->willReturn('differentOwner');
5583+
$node->method('getStorage')->willReturn($wrapperStorage);
5584+
$wrapperStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5585+
$wrapperStorage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($innerStorage);
5586+
$innerStorage->method('getShare')->willReturn($originalShare);
5587+
$originalShare->method('getHideDownload')->willReturn(false);
5588+
$originalShare->method('getAttributes')->willReturn(null);
5589+
5590+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5591+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5592+
5593+
$share->expects($this->once())->method('setHideDownload')->with(false);
5594+
5595+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5596+
}
53815597
}

0 commit comments

Comments
 (0)