Skip to content

Commit 8725129

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 e323881 commit 8725129

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
@@ -2095,14 +2095,15 @@ 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());
21012102
foreach ($nodes as $node) {
2102-
// Owner always can download it - so allow it and break
2103+
// Owner always can download it - so allow it, but respect their explicit choice about hiding downloads
21032104
if ($node->getOwner()?->getUID() === $share->getSharedBy()) {
21042105
$canDownload = true;
2105-
$hideDownload = false;
2106+
$hideDownload = $userExplicitlySetHideDownload;
21062107
break;
21072108
}
21082109

@@ -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
@@ -6,8 +6,11 @@
66
*/
77
namespace OCA\Files_Sharing\Tests\Controller;
88

9+
use OC\Files\Storage\Wrapper\Wrapper;
910
use OCA\Federation\TrustedServers;
1011
use OCA\Files_Sharing\Controller\ShareAPIController;
12+
use OCA\Files_Sharing\External\Storage;
13+
use OCA\Files_Sharing\SharedStorage;
1114
use OCP\App\IAppManager;
1215
use OCP\AppFramework\Http\DataResponse;
1316
use OCP\AppFramework\OCS\OCSBadRequestException;
@@ -5299,4 +5302,217 @@ public function testFormatShareWithFederatedShareWithAtInUsername(): void {
52995302

53005303
$this->assertTrue($result['is_trusted_server']);
53015304
}
5305+
5306+
public function testOwnerCanAlwaysDownload(): void {
5307+
$ocs = $this->mockFormatShare();
5308+
5309+
$share = $this->createMock(IShare::class);
5310+
$node = $this->createMock(File::class);
5311+
$userFolder = $this->createMock(Folder::class);
5312+
$owner = $this->createMock(IUser::class);
5313+
5314+
$share->method('getSharedBy')->willReturn('sharedByUser');
5315+
$share->method('getNodeId')->willReturn(42);
5316+
$node->method('getOwner')->willReturn($owner);
5317+
$owner->method('getUID')->willReturn('sharedByUser');
5318+
5319+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5320+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5321+
5322+
// Expect hideDownload to be set to false since owner can always download
5323+
$share->expects($this->once())->method('setHideDownload')->with(false);
5324+
5325+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5326+
}
5327+
5328+
public function testParentHideDownloadEnforcedOnChild(): void {
5329+
$ocs = $this->mockFormatShare();
5330+
5331+
$share = $this->createMock(IShare::class);
5332+
$node = $this->createMock(File::class);
5333+
$userFolder = $this->createMock(Folder::class);
5334+
$owner = $this->createMock(IUser::class);
5335+
$storage = $this->createMock(SharedStorage::class);
5336+
$originalShare = $this->createMock(IShare::class);
5337+
5338+
$share->method('getSharedBy')->willReturn('sharedByUser');
5339+
$share->method('getNodeId')->willReturn(42);
5340+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5341+
$node->method('getOwner')->willReturn($owner);
5342+
$owner->method('getUID')->willReturn('differentOwner');
5343+
$node->method('getStorage')->willReturn($storage);
5344+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5345+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5346+
$storage->method('getShare')->willReturn($originalShare);
5347+
$originalShare->method('getHideDownload')->willReturn(true); // Parent hides download
5348+
$originalShare->method('getAttributes')->willReturn(null);
5349+
5350+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5351+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5352+
5353+
// Should be forced to hide download due to parent restriction
5354+
$share->expects($this->once())->method('setHideDownload')->with(true);
5355+
5356+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5357+
}
5358+
5359+
public function testUserCanHideWhenParentAllows(): void {
5360+
$ocs = $this->mockFormatShare();
5361+
5362+
$share = $this->createMock(IShare::class);
5363+
$node = $this->createMock(File::class);
5364+
$userFolder = $this->createMock(Folder::class);
5365+
$owner = $this->createMock(IUser::class);
5366+
$storage = $this->createMock(SharedStorage::class);
5367+
$originalShare = $this->createMock(IShare::class);
5368+
5369+
$share->method('getSharedBy')->willReturn('sharedByUser');
5370+
$share->method('getNodeId')->willReturn(42);
5371+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5372+
$node->method('getOwner')->willReturn($owner);
5373+
$owner->method('getUID')->willReturn('differentOwner');
5374+
$node->method('getStorage')->willReturn($storage);
5375+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5376+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5377+
$storage->method('getShare')->willReturn($originalShare);
5378+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5379+
$originalShare->method('getAttributes')->willReturn(null);
5380+
5381+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5382+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5383+
5384+
// Should respect user's choice to hide downloads
5385+
$share->expects($this->once())->method('setHideDownload')->with(true);
5386+
5387+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5388+
}
5389+
5390+
public function testParentDownloadAttributeInherited(): void {
5391+
$ocs = $this->mockFormatShare();
5392+
5393+
$share = $this->createMock(IShare::class);
5394+
$node = $this->createMock(File::class);
5395+
$userFolder = $this->createMock(Folder::class);
5396+
$owner = $this->createMock(IUser::class);
5397+
$storage = $this->createMock(SharedStorage::class);
5398+
$originalShare = $this->createMock(IShare::class);
5399+
$attributes = $this->createMock(\OCP\Share\IAttributes::class);
5400+
$shareAttributes = $this->createMock(\OCP\Share\IAttributes::class);
5401+
5402+
$share->method('getSharedBy')->willReturn('sharedByUser');
5403+
$share->method('getNodeId')->willReturn(42);
5404+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5405+
$share->method('getAttributes')->willReturn($shareAttributes);
5406+
$share->method('newAttributes')->willReturn($shareAttributes);
5407+
$node->method('getOwner')->willReturn($owner);
5408+
$owner->method('getUID')->willReturn('differentOwner');
5409+
$node->method('getStorage')->willReturn($storage);
5410+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5411+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5412+
$storage->method('getShare')->willReturn($originalShare);
5413+
$originalShare->method('getHideDownload')->willReturn(false);
5414+
$originalShare->method('getAttributes')->willReturn($attributes);
5415+
$attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download
5416+
5417+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5418+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5419+
5420+
// Should be forced to hide download and set download attribute to false
5421+
$share->expects($this->once())->method('setHideDownload')->with(true);
5422+
$shareAttributes->expects($this->once())->method('setAttribute')->with('permissions', 'download', false);
5423+
$share->expects($this->once())->method('setAttributes')->with($shareAttributes);
5424+
5425+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5426+
}
5427+
5428+
public function testFederatedStorageRespectsUserChoice(): void {
5429+
$ocs = $this->mockFormatShare();
5430+
5431+
$share = $this->createMock(IShare::class);
5432+
$node = $this->createMock(File::class);
5433+
$userFolder = $this->createMock(Folder::class);
5434+
$owner = $this->createMock(IUser::class);
5435+
$storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class);
5436+
5437+
$share->method('getSharedBy')->willReturn('sharedByUser');
5438+
$share->method('getNodeId')->willReturn(42);
5439+
$share->method('getHideDownload')->willReturn(true); // User chooses to hide downloads
5440+
$node->method('getOwner')->willReturn($owner);
5441+
$owner->method('getUID')->willReturn('differentOwner');
5442+
$node->method('getStorage')->willReturn($storage);
5443+
$storage->method('instanceOfStorage')->willReturnMap([
5444+
[SharedStorage::class, false],
5445+
[\OCA\Files_Sharing\External\Storage::class, true]
5446+
]);
5447+
5448+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5449+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5450+
5451+
// For federated storage, should respect user's choice
5452+
$share->expects($this->once())->method('setHideDownload')->with(true);
5453+
5454+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5455+
}
5456+
5457+
public function testUserAllowsDownloadWhenParentPermits(): void {
5458+
$ocs = $this->mockFormatShare();
5459+
5460+
$share = $this->createMock(IShare::class);
5461+
$node = $this->createMock(File::class);
5462+
$userFolder = $this->createMock(Folder::class);
5463+
$owner = $this->createMock(IUser::class);
5464+
$storage = $this->createMock(SharedStorage::class);
5465+
$originalShare = $this->createMock(IShare::class);
5466+
5467+
$share->method('getSharedBy')->willReturn('sharedByUser');
5468+
$share->method('getNodeId')->willReturn(42);
5469+
$share->method('getHideDownload')->willReturn(false); // User wants to allow downloads
5470+
$node->method('getOwner')->willReturn($owner);
5471+
$owner->method('getUID')->willReturn('differentOwner');
5472+
$node->method('getStorage')->willReturn($storage);
5473+
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5474+
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
5475+
$storage->method('getShare')->willReturn($originalShare);
5476+
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
5477+
$originalShare->method('getAttributes')->willReturn(null);
5478+
5479+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5480+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5481+
5482+
// Should allow downloads as both user and parent permit it
5483+
$share->expects($this->once())->method('setHideDownload')->with(false);
5484+
5485+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5486+
}
5487+
5488+
public function testWrapperStorageUnwrapped(): void {
5489+
$ocs = $this->mockFormatShare();
5490+
5491+
$share = $this->createMock(IShare::class);
5492+
$node = $this->createMock(File::class);
5493+
$userFolder = $this->createMock(Folder::class);
5494+
$owner = $this->createMock(IUser::class);
5495+
$wrapperStorage = $this->createMock(Wrapper::class);
5496+
$innerStorage = $this->createMock(SharedStorage::class);
5497+
$originalShare = $this->createMock(IShare::class);
5498+
5499+
$share->method('getSharedBy')->willReturn('sharedByUser');
5500+
$share->method('getNodeId')->willReturn(42);
5501+
$share->method('getHideDownload')->willReturn(false);
5502+
$node->method('getOwner')->willReturn($owner);
5503+
$owner->method('getUID')->willReturn('differentOwner');
5504+
$node->method('getStorage')->willReturn($wrapperStorage);
5505+
$wrapperStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
5506+
$wrapperStorage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($innerStorage);
5507+
$innerStorage->method('getShare')->willReturn($originalShare);
5508+
$originalShare->method('getHideDownload')->willReturn(false);
5509+
$originalShare->method('getAttributes')->willReturn(null);
5510+
5511+
$userFolder->method('getById')->with(42)->willReturn([$node]);
5512+
$this->rootFolder->method('getUserFolder')->with('sharedByUser')->willReturn($userFolder);
5513+
5514+
$share->expects($this->once())->method('setHideDownload')->with(false);
5515+
5516+
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
5517+
}
53025518
}

0 commit comments

Comments
 (0)