Skip to content

Commit 32f92af

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 <[email protected]>
1 parent 30b5f00 commit 32f92af

File tree

2 files changed

+249
-11
lines changed

2 files changed

+249
-11
lines changed

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

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2095,6 +2095,7 @@ private function checkInheritedAttributes(IShare $share): void {
20952095

20962096
$canDownload = false;
20972097
$hideDownload = true;
2098+
$userExplicitlySetHideDownload = $share->getHideDownload(); // Capture user's explicit choice
20982099

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

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

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

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@
88

99
namespace OCA\Files_Sharing\Tests\Controller;
1010

11+
use OC\Files\Storage\Wrapper\Wrapper;
1112
use OCA\Federation\TrustedServers;
1213
use OCA\Files_Sharing\Controller\ShareAPIController;
14+
use OCA\Files_Sharing\External\Storage;
15+
use OCA\Files_Sharing\SharedStorage;
1316
use OCP\App\IAppManager;
1417
use OCP\AppFramework\Http\DataResponse;
1518
use OCP\AppFramework\OCS\OCSBadRequestException;
@@ -5432,4 +5435,217 @@ public function testFormatShareWithFederatedShareWithAtInUsername(): void {
54325435

54335436
$this->assertTrue($result['is_trusted_server']);
54345437
}
5438+
5439+
public function testOwnerCanAlwaysDownload(): void {
5440+
$ocs = $this->mockFormatShare();
5441+
5442+
$share = $this->createMock(IShare::class);
5443+
$node = $this->createMock(File::class);
5444+
$userFolder = $this->createMock(Folder::class);
5445+
$owner = $this->createMock(IUser::class);
5446+
5447+
$share->method('getSharedBy')->willReturn('sharedByUser');
5448+
$share->method('getNodeId')->willReturn(42);
5449+
$node->method('getOwner')->willReturn($owner);
5450+
$owner->method('getUID')->willReturn('sharedByUser');
5451+
5452+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5453+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5454+
5455+
// Expect hideDownload to be set to false since owner can always download
5456+
$share->expects($this->once())->method('setHideDownload')->with(false);
5457+
5458+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5459+
}
5460+
5461+
public function testParentHideDownloadEnforcedOnChild(): void {
5462+
$ocs = $this->mockFormatShare();
5463+
5464+
$share = $this->createMock(IShare::class);
5465+
$node = $this->createMock(File::class);
5466+
$userFolder = $this->createMock(Folder::class);
5467+
$owner = $this->createMock(IUser::class);
5468+
$storage = $this->createMock(SharedStorage::class);
5469+
$originalShare = $this->createMock(IShare::class);
5470+
5471+
$share->method('getSharedBy')->willReturn('sharedByUser');
5472+
$share->method('getNodeId')->willReturn(42);
5473+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5474+
$node->method('getOwner')->willReturn($owner);
5475+
$owner->method('getUID')->willReturn('differentOwner');
5476+
$node->method('getStorage')->willReturn($storage);
5477+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5478+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5479+
$storage->method('getShare')->willReturn($originalShare);
5480+
$originalShare->method('getHideDownload')->willReturn(true); // Parent hides download
5481+
$originalShare->method('getAttributes')->willReturn(null);
5482+
5483+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5484+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5485+
5486+
// Should be forced to hide download due to parent restriction
5487+
$share->expects($this->once())->method('setHideDownload')->with(true);
5488+
5489+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5490+
}
5491+
5492+
public function testUserCanHideWhenParentAllows(): void {
5493+
$ocs = $this->mockFormatShare();
5494+
5495+
$share = $this->createMock(IShare::class);
5496+
$node = $this->createMock(File::class);
5497+
$userFolder = $this->createMock(Folder::class);
5498+
$owner = $this->createMock(IUser::class);
5499+
$storage = $this->createMock(SharedStorage::class);
5500+
$originalShare = $this->createMock(IShare::class);
5501+
5502+
$share->method('getSharedBy')->willReturn('sharedByUser');
5503+
$share->method('getNodeId')->willReturn(42);
5504+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5505+
$node->method('getOwner')->willReturn($owner);
5506+
$owner->method('getUID')->willReturn('differentOwner');
5507+
$node->method('getStorage')->willReturn($storage);
5508+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5509+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5510+
$storage->method('getShare')->willReturn($originalShare);
5511+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5512+
$originalShare->method('getAttributes')->willReturn(null);
5513+
5514+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5515+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5516+
5517+
// Should respect user's choice to hide downloads
5518+
$share->expects($this->once())->method('setHideDownload')->with(true);
5519+
5520+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5521+
}
5522+
5523+
public function testParentDownloadAttributeInherited(): void {
5524+
$ocs = $this->mockFormatShare();
5525+
5526+
$share = $this->createMock(IShare::class);
5527+
$node = $this->createMock(File::class);
5528+
$userFolder = $this->createMock(Folder::class);
5529+
$owner = $this->createMock(IUser::class);
5530+
$storage = $this->createMock(SharedStorage::class);
5531+
$originalShare = $this->createMock(IShare::class);
5532+
$attributes = $this->createMock(\OCP\Share\IAttributes::class);
5533+
$shareAttributes = $this->createMock(\OCP\Share\IAttributes::class);
5534+
5535+
$share->method('getSharedBy')->willReturn('sharedByUser');
5536+
$share->method('getNodeId')->willReturn(42);
5537+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5538+
$share->method('getAttributes')->willReturn($shareAttributes);
5539+
$share->method('newAttributes')->willReturn($shareAttributes);
5540+
$node->method('getOwner')->willReturn($owner);
5541+
$owner->method('getUID')->willReturn('differentOwner');
5542+
$node->method('getStorage')->willReturn($storage);
5543+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5544+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5545+
$storage->method('getShare')->willReturn($originalShare);
5546+
$originalShare->method('getHideDownload')->willReturn(false);
5547+
$originalShare->method('getAttributes')->willReturn($attributes);
5548+
$attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download
5549+
5550+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5551+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5552+
5553+
// Should be forced to hide download and set download attribute to false
5554+
$share->expects($this->once())->method('setHideDownload')->with(true);
5555+
$shareAttributes->expects($this->once())->method('setAttribute')->with('permissions', 'download', false);
5556+
$share->expects($this->once())->method('setAttributes')->with($shareAttributes);
5557+
5558+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5559+
}
5560+
5561+
public function testFederatedStorageRespectsUserChoice(): void {
5562+
$ocs = $this->mockFormatShare();
5563+
5564+
$share = $this->createMock(IShare::class);
5565+
$node = $this->createMock(File::class);
5566+
$userFolder = $this->createMock(Folder::class);
5567+
$owner = $this->createMock(IUser::class);
5568+
$storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class);
5569+
5570+
$share->method('getSharedBy')->willReturn('sharedByUser');
5571+
$share->method('getNodeId')->willReturn(42);
5572+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5573+
$node->method('getOwner')->willReturn($owner);
5574+
$owner->method('getUID')->willReturn('differentOwner');
5575+
$node->method('getStorage')->willReturn($storage);
5576+
$storage->method('instanceOfStorage')->willReturnMap([
5577+
[SharedStorage::class, false],
5578+
[\OCA\Files_Sharing\External\Storage::class, true]
5579+
]);
5580+
5581+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5582+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5583+
5584+
// For federated storage, should respect user's choice
5585+
$share->expects($this->once())->method('setHideDownload')->with(true);
5586+
5587+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5588+
}
5589+
5590+
public function testUserAllowsDownloadWhenParentPermits(): void {
5591+
$ocs = $this->mockFormatShare();
5592+
5593+
$share = $this->createMock(IShare::class);
5594+
$node = $this->createMock(File::class);
5595+
$userFolder = $this->createMock(Folder::class);
5596+
$owner = $this->createMock(IUser::class);
5597+
$storage = $this->createMock(SharedStorage::class);
5598+
$originalShare = $this->createMock(IShare::class);
5599+
5600+
$share->method('getSharedBy')->willReturn('sharedByUser');
5601+
$share->method('getNodeId')->willReturn(42);
5602+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5603+
$node->method('getOwner')->willReturn($owner);
5604+
$owner->method('getUID')->willReturn('differentOwner');
5605+
$node->method('getStorage')->willReturn($storage);
5606+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5607+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5608+
$storage->method('getShare')->willReturn($originalShare);
5609+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5610+
$originalShare->method('getAttributes')->willReturn(null);
5611+
5612+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5613+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5614+
5615+
// Should allow downloads as both user and parent permit it
5616+
$share->expects($this->once())->method('setHideDownload')->with(false);
5617+
5618+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5619+
}
5620+
5621+
public function testWrapperStorageUnwrapped(): void {
5622+
$ocs = $this->mockFormatShare();
5623+
5624+
$share = $this->createMock(IShare::class);
5625+
$node = $this->createMock(File::class);
5626+
$userFolder = $this->createMock(Folder::class);
5627+
$owner = $this->createMock(IUser::class);
5628+
$wrapperStorage = $this->createMock(Wrapper::class);
5629+
$innerStorage = $this->createMock(SharedStorage::class);
5630+
$originalShare = $this->createMock(IShare::class);
5631+
5632+
$share->method('getSharedBy')->willReturn('sharedByUser');
5633+
$share->method('getNodeId')->willReturn(42);
5634+
$share->method('getHideDownload')->willReturn(false);
5635+
$node->method('getOwner')->willReturn($owner);
5636+
$owner->method('getUID')->willReturn('differentOwner');
5637+
$node->method('getStorage')->willReturn($wrapperStorage);
5638+
$wrapperStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5639+
$wrapperStorage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($innerStorage);
5640+
$innerStorage->method('getShare')->willReturn($originalShare);
5641+
$originalShare->method('getHideDownload')->willReturn(false);
5642+
$originalShare->method('getAttributes')->willReturn(null);
5643+
5644+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5645+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5646+
5647+
$share->expects($this->once())->method('setHideDownload')->with(false);
5648+
5649+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5650+
}
54355651
}

0 commit comments

Comments
 (0)