Skip to content

[stable31] fix(sharing): Allow reasonable control for 'Hide download' on fed shares #56178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: stable31
Choose a base branch
from
+251 −13
Open

[stable31] fix(sharing): Allow reasonable control for 'Hide download' on fed shares #56178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 35 additions & 13 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2093,14 +2093,15 @@ private function checkInheritedAttributes(IShare $share): void {

$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;
}

Expand All @@ -2118,23 +2119,44 @@ private function checkInheritedAttributes(IShare $share): void {
/** @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);
}
}

Expand Down
216 changes: 216 additions & 0 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -5299,4 +5302,217 @@ public function testFormatShareWithFederatedShareWithAtInUsername(): void {

$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]);
}
}
Loading