Skip to content

Fix: sharing attribute resolution #66375

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

Merged
GhassenKefi merged 3 commits into from
Nov 6, 2026
+500 −84
Merged

Fix: sharing attribute resolution #66375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9074d87
refactor(files_sharing): resolve download permission of received shar…
GhassenKefi Nov 4, 2026
27e9d2a
fix(sharing): keep reshare attributes consistent with received shares
GhassenKefi Nov 4, 2026
279ede5
fix(files): consider all shares of the target when moving mount points
GhassenKefi Nov 4, 2026
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
4 changes: 2 additions & 2 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 @@ -23,6 +23,7 @@
use OCA\Files_Sharing\ResponseDefinitions;
use OCA\Files_Sharing\SharedStorage;
use OCA\Files_Sharing\TokenScopes as SharingTokenScopes;
use OCA\Files_Sharing\ViewOnly;
use OCA\GlobalSiteSelector\Service\SlaveService;
use OCP\App\IAppManager;
use OCP\AppFramework\Http;
Expand Down Expand Up @@ -2112,14 +2113,13 @@ private function checkInheritedAttributes(IShare $share): void {

/** @var SharedStorage $storage */
$originalShare = $storage->getShare();
$inheritedAttributes = $originalShare->getAttributes();

// 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;
$parentAllowsDownload = ViewOnly::canDownloadThroughMount($node);

// Apply inheritance rules:
// 1. If parent hides download, child must hide download
Expand Down
5 changes: 5 additions & 0 deletions apps/files_sharing/lib/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OC\Files\Cache\FileAccess;
use OC\Files\Filesystem;
use OC\Files\Mount\MountPoint;
use OC\Share20\Manager;
use OCP\Constants;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
Expand Down Expand Up @@ -66,6 +67,10 @@ private static function moveShareInOrOutOfShare($path): void {

$shareManager = Server::get(\OCP\Share\IManager::class);

if ($shareManager instanceof Manager) {
$shareManager->restrictDownloadOfReshares($src);
}

// We intentionally include invalid shares, as they have been automatically invalidated due to the node no longer
// being accessible for the user. Only in this case where we adjust the share after it was moved we want to ignore
// this to be able to still adjust it.
Expand Down
35 changes: 20 additions & 15 deletions apps/files_sharing/lib/ViewOnly.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use OCP\Files\Folder;
use OCP\Files\Node;
use OCP\Files\NotFoundException;
use OCP\Share\IShare;

/**
* Handles restricting for download of files
Expand Down Expand Up @@ -84,30 +85,34 @@ private function dirRecursiveCheck(Folder $dirInfo): bool {
return true;
}

/**
* @param Node $fileInfo
* @return bool
*/
public function isDownloadable(Node $fileInfo): bool {
// Restrict view-only to nodes which are shared
try {
$storage = $fileInfo->getStorage();
return self::canDownloadThroughMount($fileInfo);
} catch (NotFoundException) {
return true;
}
}

if (!$storage->instanceOfStorage(SharedStorage::class)) {
/**
* Whether the node can be downloaded through the mount it is accessed by,
* which for share mounts requires one of the received shares to allow it.
*
* @param ?callable(IShare): bool $allowsDownload whether a received share allows downloads, defaults to its download attribute
* @throws NotFoundException
*/
public static function canDownloadThroughMount(Node $node, ?callable $allowsDownload = null): bool {
$mount = $node->getMountPoint();
if (!$mount instanceof SharedMount) {
return true;
}

// Extract extra permissions
/** @var SharedStorage $storage */
$share = $storage->getShare();

// Check whether download-permission was denied (granted if not set)
$attributes = $share->getAttributes();
$canDownload = $attributes?->getAttribute('permissions', 'download');
$allowsDownload ??= static fn (IShare $share): bool => $share->getAttributes()?->getAttribute('permissions', 'download') !== false;
foreach ($mount->getGroupedShares() as $share) {
if ($allowsDownload($share)) {
return true;
}
}

return $canDownload !== false;
return false;
}
}
77 changes: 37 additions & 40 deletions apps/files_sharing/tests/ApplicationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@
use OCA\Files_Sharing\AppInfo\Application;
use OCA\Files_Sharing\Listener\BeforeDirectFileDownloadListener;
use OCA\Files_Sharing\Listener\BeforeZipCreatedListener;
use OCA\Files_Sharing\SharedStorage;
use OCA\Files_Sharing\SharedMount;
use OCA\Files_Sharing\ViewOnly;
use OCP\Files\Events\BeforeDirectFileDownloadEvent;
use OCP\Files\Events\BeforeZipCreatedEvent;
use OCP\Files\File;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\Files\IUserFolder;
use OCP\Files\Storage\IStorage;
use OCP\Files\Mount\IMountPoint;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\IAttributes;
Expand All @@ -47,38 +47,37 @@ protected function setUp(): void {
public static function providesDataForCanGet(): array {
return [
'normal file (sender)' => [
'/bar.txt', IStorage::class, false, null, true
'/bar.txt', null, true
],
'shared file (receiver) with attribute secure-view-enabled set false' => [
'/share-bar.txt', SharedStorage::class, true, true, true
'/share-bar.txt', true, true
],
'shared file (receiver) with attribute secure-view-enabled set true' => [
'/secure-share-bar.txt', SharedStorage::class, true, false, false
'/secure-share-bar.txt', false, false
],
];
}

#[DataProvider(methodName: 'providesDataForCanGet')]
public function testCheckDirectCanBeDownloaded(
string $path,
string $storageClass,
bool $instanceOfSharedStorage,
?bool $storageShareDownloadPermission,
?bool $shareDownloadPermission,
bool $canDownloadDirectly,
): void {
$fileStorage = $this->createMock($storageClass);
$fileStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn($instanceOfSharedStorage);
if ($storageShareDownloadPermission !== null) {
if ($shareDownloadPermission === null) {
$mount = $this->createMock(IMountPoint::class);
} else {
$fileShareAttributes = $this->createMock(IAttributes::class);
$fileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn($storageShareDownloadPermission);
$fileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn($shareDownloadPermission);
$fileShare = $this->createMock(IShare::class);
$fileShare->method('getAttributes')->willReturn($fileShareAttributes);

$fileStorage->method('getShare')->willReturn($fileShare);
$mount = $this->createMock(SharedMount::class);
$mount->method('getGroupedShares')->willReturn([$fileShare]);
}

$file = $this->createMock(File::class);
$file->method('getStorage')->willReturn($fileStorage);
$file->method('getMountPoint')->willReturn($mount);

$userFolder = $this->createMock(IUserFolder::class);
$userFolder->method('get')->willReturn($file);
Expand All @@ -105,16 +104,16 @@ public function testCheckDirectCanBeDownloaded(
public static function providesDataForCanZip(): array {
return [
'can download zipped 2 non-shared files inside non-shared folder' => [
'/folder', ['bar1.txt', 'bar2.txt'], 'nonSharedStorage', ['nonSharedStorage','nonSharedStorage'], true
'/folder', ['bar1.txt', 'bar2.txt'], 'nonSharedMount', ['nonSharedMount','nonSharedMount'], true
],
'can download zipped non-shared folder' => [
'/', ['folder'], 'nonSharedStorage', ['nonSharedStorage','nonSharedStorage'], true
'/', ['folder'], 'nonSharedMount', ['nonSharedMount','nonSharedMount'], true
],
'cannot download zipped 1 non-shared file and 1 secure-shared inside non-shared folder' => [
'/folder', ['secured-bar1.txt', 'bar2.txt'], 'nonSharedStorage', ['nonSharedStorage','secureSharedStorage'], false,
'/folder', ['secured-bar1.txt', 'bar2.txt'], 'nonSharedMount', ['nonSharedMount','secureSharedMount'], false,
],
'cannot download zipped secure-shared folder' => [
'/', ['secured-folder'], 'secureSharedStorage', [], false,
'/', ['secured-folder'], 'secureSharedMount', [], false,
],
];
}
Expand All @@ -123,42 +122,40 @@ public static function providesDataForCanZip(): array {
public function testCheckZipCanBeDownloaded(
string $dir,
array $files,
string $folderStorage,
string $folderMount,
array $directoryListing,
bool $downloadSuccessful,
): void {
// Mock: Normal file/folder storage
$nonSharedStorage = $this->createMock(IStorage::class);
$nonSharedStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(false);
// Mock: Normal file/folder mount
$nonSharedMount = $this->createMock(IMountPoint::class);

// Mock: Secure-view file/folder shared storage
$secureReceiverFileShareAttributes = $this->createMock(IAttributes::class);
$secureReceiverFileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn(false);
$secureReceiverFileShare = $this->createMock(IShare::class);
$secureReceiverFileShare->method('getAttributes')->willReturn($secureReceiverFileShareAttributes);
$secureSharedStorage = $this->createMock(SharedStorage::class);
$secureSharedStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
$secureSharedStorage->method('getShare')->willReturn($secureReceiverFileShare);
$secureSharedMount = $this->createMock(SharedMount::class);
$secureSharedMount->method('getGroupedShares')->willReturn([$secureReceiverFileShare]);

$folder = $this->createMock(Folder::class);
$folder->method('getPath')->willReturn($dir);
if ($folderStorage === 'nonSharedStorage') {
$folder->method('getStorage')->willReturn($nonSharedStorage);
} elseif ($folderStorage === 'secureSharedStorage') {
$folder->method('getStorage')->willReturn($secureSharedStorage);
if ($folderMount === 'nonSharedMount') {
$folder->method('getMountPoint')->willReturn($nonSharedMount);
} elseif ($folderMount === 'secureSharedMount') {
$folder->method('getMountPoint')->willReturn($secureSharedMount);
} else {
throw new \Exception('Unknown storage ' . $folderStorage);
throw new \Exception('Unknown mount ' . $folderMount);
}
if (count($directoryListing) > 0) {
$directoryListing = array_map(
function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) {
function (string $fileMount) use ($nonSharedMount, $secureSharedMount) {
$file = $this->createMock(File::class);
if ($fileStorage === 'nonSharedStorage') {
$file->method('getStorage')->willReturn($nonSharedStorage);
} elseif ($fileStorage === 'secureSharedStorage') {
$file->method('getStorage')->willReturn($secureSharedStorage);
if ($fileMount === 'nonSharedMount') {
$file->method('getMountPoint')->willReturn($nonSharedMount);
} elseif ($fileMount === 'secureSharedMount') {
$file->method('getMountPoint')->willReturn($secureSharedMount);
} else {
throw new \Exception('Unknown storage ' . $fileStorage);
throw new \Exception('Unknown mount ' . $fileMount);
}
return $file;
},
Expand All @@ -177,13 +174,13 @@ function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) {

// If the folder contains any secure-shared files, make it appear as a secure-shared folder
// so that ViewOnly::isDownloadable() will return false
$containsSecureSharedFiles = in_array('secureSharedStorage', $directoryListing);
if ($containsSecureSharedFiles && $folderStorage === 'nonSharedStorage') {
$folder->method('getStorage')->willReturn($secureSharedStorage);
$containsSecureSharedFiles = in_array('secureSharedMount', $directoryListing);
if ($containsSecureSharedFiles && $folderMount === 'nonSharedMount') {
$folder->method('getMountPoint')->willReturn($secureSharedMount);
}

$rootFolder = $this->createMock(Folder::class);
$rootFolder->method('getStorage')->willReturn($nonSharedStorage);
$rootFolder->method('getMountPoint')->willReturn($nonSharedMount);
$rootFolder->method('getDirectoryListing')->willReturn([$folder]);

$userFolder = $this->createMock(IUserFolder::class);
Expand Down
11 changes: 11 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 @@ -14,6 +14,7 @@
use OCA\Federation\TrustedServers;
use OCA\Files_Sharing\Controller\ShareAPIController;
use OCA\Files_Sharing\External\Storage;
use OCA\Files_Sharing\SharedMount;
use OCA\Files_Sharing\SharedStorage;
use OCP\App\IAppManager;
use OCP\AppFramework\Http\DataResponse;
Expand Down Expand Up @@ -4654,6 +4655,12 @@ public function testOwnerCanAlwaysDownload(): void {
$this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]);
}

private function createSharedMount(IShare ...$shares): SharedMount&MockObject {
$mount = $this->createMock(SharedMount::class);
$mount->method('getGroupedShares')->willReturn($shares);
return $mount;
}

public function testParentHideDownloadEnforcedOnChild(): void {
$ocs = $this->mockFormatShare();

Expand All @@ -4673,6 +4680,7 @@ public function testParentHideDownloadEnforcedOnChild(): void {
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
$storage->method('getShare')->willReturn($originalShare);
$node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare));
$originalShare->method('getHideDownload')->willReturn(true); // Parent hides download
$originalShare->method('getAttributes')->willReturn(null);

Expand Down Expand Up @@ -4704,6 +4712,7 @@ public function testUserCanHideWhenParentAllows(): void {
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
$storage->method('getShare')->willReturn($originalShare);
$node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare));
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
$originalShare->method('getAttributes')->willReturn(null);

Expand Down Expand Up @@ -4739,6 +4748,7 @@ public function testParentDownloadAttributeInherited(): void {
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
$storage->method('getShare')->willReturn($originalShare);
$node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare));
$originalShare->method('getHideDownload')->willReturn(false);
$originalShare->method('getAttributes')->willReturn($attributes);
$attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download
Expand Down Expand Up @@ -4802,6 +4812,7 @@ public function testUserAllowsDownloadWhenParentPermits(): void {
$storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true);
$storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage);
$storage->method('getShare')->willReturn($originalShare);
$node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare));
$originalShare->method('getHideDownload')->willReturn(false); // Parent allows download
$originalShare->method('getAttributes')->willReturn(null);

Expand Down
Loading
Loading