Skip to content

Commit 90d38ec

Browse files
nfebenfebe
authored
Merge pull request #56177 from nextcloud/backport/55251/stable32
[stable32] fix(sharing): Allow reasonable control for 'Hide download' on fed shares
2 parents 413c9a2 + 299662e commit 90d38ec

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)