diff options
author | Ferdinand Thiessen <opensource@fthiessen.de> | 2025-01-19 11:36:24 +0100 |
---|---|---|
committer | backportbot[bot] <backportbot[bot]@users.noreply.github.com> | 2025-02-04 15:06:11 +0000 |
commit | 8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1 (patch) | |
tree | deba33466ca11a4d8fc04fab80f3abd3a6292328 | |
parent | dd921969dafa64de916cd7f9daba6bf2e57f4525 (diff) | |
download | nextcloud-server-8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1.tar.gz nextcloud-server-8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1.zip |
fix(sharing): Ensure download restrictions are not droppedbackport/50642/stable31
When a user receives a share with share-permissions but also with
download restrictions (hide download or the modern download permission attribute),
then re-shares of that share must always also include those restrictions.
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
3 files changed, 193 insertions, 92 deletions
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 1fb62480049..9716e8f2772 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -654,7 +654,6 @@ class ShareAPIController extends OCSController { } $share->setSharedBy($this->userId); - $this->checkInheritedAttributes($share); // Handle mail send if (is_null($sendMail)) { @@ -787,6 +786,7 @@ class ShareAPIController extends OCSController { } $share->setShareType($shareType); + $this->checkInheritedAttributes($share); if ($note !== '') { $share->setNote($note); @@ -1272,7 +1272,6 @@ class ShareAPIController extends OCSController { if ($attributes !== null) { $share = $this->setShareAttributes($share, $attributes); } - $this->checkInheritedAttributes($share); // Handle mail send if ($sendMail === 'true' || $sendMail === 'false') { @@ -1359,6 +1358,7 @@ class ShareAPIController extends OCSController { } try { + $this->checkInheritedAttributes($share); $share = $this->shareManager->updateShare($share); } catch (HintException $e) { $code = $e->getCode() === 0 ? 403 : $e->getCode(); @@ -2083,30 +2083,48 @@ class ShareAPIController extends OCSController { if (!$share->getSharedBy()) { return; // Probably in a test } + + $canDownload = false; + $hideDownload = true; + $userFolder = $this->rootFolder->getUserFolder($share->getSharedBy()); - $node = $userFolder->getFirstNodeById($share->getNodeId()); - if (!$node) { - return; - } - if ($node->getStorage()->instanceOfStorage(SharedStorage::class)) { - $storage = $node->getStorage(); - if ($storage instanceof Wrapper) { - $storage = $storage->getInstanceOfStorage(SharedStorage::class); - if ($storage === null) { - throw new \RuntimeException('Should not happen, instanceOfStorage but getInstanceOfStorage return null'); - } - } else { - throw new \RuntimeException('Should not happen, instanceOfStorage but not a wrapper'); + $nodes = $userFolder->getById($share->getNodeId()); + foreach ($nodes as $node) { + // Owner always can download it - so allow it and break + if ($node->getOwner()?->getUID() === $share->getSharedBy()) { + $canDownload = true; + $hideDownload = false; + break; } - /** @var SharedStorage $storage */ - $inheritedAttributes = $storage->getShare()->getAttributes(); - if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) { - $share->setHideDownload(true); - $attributes = $share->getAttributes(); - if ($attributes) { - $attributes->setAttribute('permissions', 'download', false); - $share->setAttributes($attributes); + + if ($node->getStorage()->instanceOfStorage(SharedStorage::class)) { + $storage = $node->getStorage(); + if ($storage instanceof Wrapper) { + $storage = $storage->getInstanceOfStorage(SharedStorage::class); + if ($storage === null) { + throw new \RuntimeException('Should not happen, instanceOfStorage but getInstanceOfStorage return null'); + } + } else { + throw new \RuntimeException('Should not happen, instanceOfStorage but not a wrapper'); } + + /** @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; + } + } + + if ($hideDownload || !$canDownload) { + $share->setHideDownload(true); + + if (!$canDownload) { + $attributes = $share->getAttributes() ?? $share->newAttributes(); + $attributes->setAttribute('permissions', 'download', false); + $share->setAttributes($attributes); } } } diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index f8c316cd2e9..ac788dd6b28 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -157,7 +157,7 @@ class ShareAPIControllerTest extends TestCase { $this->factory, $this->mailer, $this->currentUser, - ])->setMethods(['formatShare']) + ])->onlyMethods(['formatShare']) ->getMock(); } @@ -246,9 +246,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($node); + ->willReturn([$node]); $this->shareManager ->expects($this->once()) @@ -411,9 +411,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($share->getNode()); + ->willReturn([$share->getNode()]); $this->shareManager->expects($this->once()) ->method('deleteFromSelf') @@ -474,9 +474,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($share->getNode()); + ->willReturn([$share->getNode()]); $this->shareManager->expects($this->never()) ->method('deleteFromSelf'); @@ -506,9 +506,10 @@ class ShareAPIControllerTest extends TestCase { ->willReturn($mount); $userFolder = $this->createMock(Folder::class); - $userFolder - ->expects($this->exactly(2)) - ->method('getFirstNodeById') + $userFolder->method('getById') + ->with(2) + ->willReturn([$file]); + $userFolder->method('getFirstNodeById') ->with(2) ->willReturn($file); @@ -841,7 +842,8 @@ class ShareAPIControllerTest extends TestCase { $this->mailer, $this->currentUser, - ])->setMethods(['canAccessShare']) + ]) + ->onlyMethods(['canAccessShare']) ->getMock(); $ocs->expects($this->any()) @@ -862,7 +864,6 @@ class ShareAPIControllerTest extends TestCase { $userFolder->method('getById') ->with($share->getNodeId()) ->willReturn([$share->getNode()]); - $userFolder->method('getFirstNodeById') ->with($share->getNodeId()) ->willReturn($share->getNode()); @@ -901,9 +902,8 @@ class ShareAPIControllerTest extends TestCase { ]); $this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC')); - $d = $ocs->getShare($share->getId())->getData()[0]; - - $this->assertEquals($result, $ocs->getShare($share->getId())->getData()[0]); + $data = $ocs->getShare($share->getId())->getData()[0]; + $this->assertEquals($result, $data); } @@ -1558,6 +1558,9 @@ class ShareAPIControllerTest extends TestCase { $userFolder->method('getFirstNodeById') ->with($share->getNodeId()) ->willReturn($file); + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$file]); $file->method('getPermissions') ->will($this->onConsecutiveCalls(Constants::PERMISSION_SHARE, Constants::PERMISSION_READ)); @@ -1651,9 +1654,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($share->getNode()); + ->willReturn([$share->getNode()]); if (!$helperAvailable) { $this->appManager->method('isEnabledForUser') @@ -1741,8 +1744,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -1769,8 +1771,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -1869,8 +1870,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('allowGroupSharing')->willReturn(true); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -1974,8 +1974,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFolder(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -2531,8 +2530,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFolder(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -2569,8 +2567,7 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') + $this->rootFolder->method('getUserFolder') ->with('currentUser') ->willReturn($userFolder); @@ -2703,9 +2700,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($share->getNode()); + ->willReturn([$share->getNode()]); $this->ocs->updateShare(42); } @@ -2796,6 +2793,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$node]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($node); @@ -2850,9 +2850,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -2900,9 +2900,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -2958,9 +2958,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -3016,9 +3016,9 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); [$userFolder, $folder] = $this->getNonSharedUserFolder(); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3063,9 +3063,9 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); [$userFolder, $folder] = $this->getNonSharedUserFolder(); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3091,10 +3091,13 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); - [$userFolder, $file] = $this->getNonSharedUserFile(); - $userFolder->method('getFirstNodeById') + $file = $this->getMockBuilder(File::class)->getMock(); + $file->method('getId') + ->willReturn(42); + [$userFolder, $folder] = $this->getNonSharedUserFolder(); + $userFolder->method('getById') ->with(42) - ->willReturn($file); + ->willReturn([$folder]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3128,9 +3131,9 @@ class ShareAPIControllerTest extends TestCase { [$userFolder, $node] = $this->getNonSharedUserFolder(); $node->method('getId')->willReturn(42); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3179,9 +3182,9 @@ class ShareAPIControllerTest extends TestCase { $date->setTime(0, 0, 0); [$userFolder, $node] = $this->getNonSharedUserFolder(); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3237,9 +3240,9 @@ class ShareAPIControllerTest extends TestCase { $date->setTime(0, 0, 0); [$userFolder, $node] = $this->getNonSharedUserFolder(); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3277,9 +3280,9 @@ class ShareAPIControllerTest extends TestCase { $date->setTime(0, 0, 0); [$userFolder, $node] = $this->getNonSharedUserFolder(); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); @@ -3371,9 +3374,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $mountPoint = $this->createMock(IMountPoint::class); $node->method('getMountPoint') @@ -3439,9 +3442,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($node); + ->willReturn([$node]); $mountPoint = $this->createMock(IMountPoint::class); $node->method('getMountPoint') @@ -3500,9 +3503,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -3560,9 +3563,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -3620,9 +3623,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -3668,9 +3671,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($file); + ->willReturn([$file]); $mountPoint = $this->createMock(IMountPoint::class); $file->method('getMountPoint') @@ -3734,6 +3737,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($folder); @@ -3804,9 +3810,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with(42) - ->willReturn($folder); + ->willReturn([$folder]); $mountPoint = $this->createMock(IMountPoint::class); $folder->method('getMountPoint') @@ -3834,9 +3840,10 @@ class ShareAPIControllerTest extends TestCase { ->willReturn($mount); $userFolder = $this->createMock(Folder::class); - $userFolder - ->expects($this->exactly(2)) - ->method('getFirstNodeById') + $userFolder->method('getById') + ->with(2) + ->willReturn([$file]); + $userFolder->method('getFirstNodeById') ->with(2) ->willReturn($file); @@ -5107,6 +5114,9 @@ class ShareAPIControllerTest extends TestCase { $userFolder->method('getStorage')->willReturn($storage); $node->method('getStorage')->willReturn($storage); $node->method('getId')->willReturn(42); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($this->currentUser); + $node->method('getOwner')->willReturn($user); return [$userFolder, $node]; } diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index 8cc97fe71ee..e4da8436508 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -701,6 +701,79 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: download restrictions can not be dropped + As an "admin" + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + And As an "user0" + And creating a share with + | path | /tmp.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + And As an "user1" + And accepting last share + When Getting info of last share + Then Share fields of last share match with + | uid_owner | user0 | + | uid_file_owner | user0 | + | permissions | 17 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + When creating a share with + | path | /tmp.txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 1 | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When As an "user2" + And accepting last share + And Getting info of last share + Then Share fields of last share match with + | share_type | 0 | + | permissions | 1 | + | uid_owner | user1 | + | uid_file_owner | user0 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + + Scenario: download restrictions can not be dropped when re-sharing even on link shares + As an "admin" + Given user "user0" exists + And user "user1" exists + And User "user0" uploads file with content "foo" to "/tmp.txt" + And As an "user0" + And creating a share with + | path | /tmp.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + And As an "user1" + And accepting last share + When Getting info of last share + Then Share fields of last share match with + | uid_owner | user0 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + When creating a share with + | path | /tmp.txt | + | shareType | 3 | + | permissions | 1 | + And Getting info of last share + And Updating last share with + | hideDownload | false | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When Getting info of last share + Then Share fields of last share match with + | share_type | 3 | + | uid_owner | user1 | + | uid_file_owner | user0 | + | hide_download | 1 | + | attributes | [{"scope":"permissions","key":"download","value":false}] | + Scenario: User is not allowed to reshare file with additional delete permissions As an "admin" Given user "user0" exists |