From c5b3868c73349f86511f286aa00ed22ca399763f Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 19 Jan 2025 11:36:24 +0100 Subject: [PATCH] fix(sharing): Ensure download restrictions are not dropped 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 [skip ci] --- .../lib/Controller/ShareAPIController.php | 45 ++++--- .../Controller/ShareAPIControllerTest.php | 118 ++++++++++-------- .../sharing_features/sharing-v1-part2.feature | 73 +++++++++++ 3 files changed, 168 insertions(+), 68 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index f54d419d90b1c..a490d75eba06c 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -779,6 +779,7 @@ public function createShare( } $share->setShareType($shareType); + $this->checkInheritedAttributes($share); if ($note !== '') { $share->setNote($note); @@ -1261,7 +1262,6 @@ public function updateShare( if ($attributes !== null) { $share = $this->setShareAttributes($share, $attributes); } - $this->checkInheritedAttributes($share); // Handle mail send if ($sendMail === 'true' || $sendMail === 'false') { @@ -1345,6 +1345,7 @@ public function updateShare( } try { + $this->checkInheritedAttributes($share); $share = $this->shareManager->updateShare($share); } catch (HintException $e) { $code = $e->getCode() === 0 ? 403 : $e->getCode(); @@ -2047,20 +2048,18 @@ private function checkInheritedAttributes(IShare $share): void { 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 \OCA\Files_Sharing\SharedStorage $storage */ $inheritedAttributes = $storage->getShare()->getAttributes(); @@ -2071,6 +2070,24 @@ private function checkInheritedAttributes(IShare $share): void { $attributes->setAttribute('permissions', 'download', false); $share->setAttributes($attributes); } + + /** @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 e04b492925916..b6c5b585d9bfa 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -154,7 +154,7 @@ private function mockFormatShare() { $this->factory, $this->mailer, $this->currentUser, - ])->setMethods(['formatShare']) + ])->onlyMethods(['formatShare']) ->getMock(); } @@ -398,9 +398,9 @@ public function testDeleteSharedWithMyGroup() { ->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') @@ -461,9 +461,9 @@ public function testDeleteSharedWithGroupIDontBelongTo() { ->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'); @@ -793,7 +793,6 @@ public function testGetShare(IShare $share, array $result): void { $userFolder->method('getById') ->with($share->getNodeId()) ->willReturn([$share->getNode()]); - $userFolder->method('getFirstNodeById') ->with($share->getNodeId()) ->willReturn($share->getNode()); @@ -832,9 +831,8 @@ public function testGetShare(IShare $share, array $result): void { ]); $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); } @@ -1489,6 +1487,9 @@ public function testCanAccessShare() { $userFolder->method('getFirstNodeById') ->with($share->getNodeId()) ->willReturn($file); + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$file]); $file->method('getPermissions') ->will($this->onConsecutiveCalls(\OCP\Constants::PERMISSION_SHARE, \OCP\Constants::PERMISSION_READ)); @@ -1582,9 +1583,9 @@ public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share, ->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') @@ -1672,8 +1673,7 @@ public function testCreateShareUserNoShareWith(): void { $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); @@ -1700,8 +1700,7 @@ public function testCreateShareUserNoValidShareWith() { $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); @@ -1905,8 +1904,7 @@ public function testCreateShareGroupNotAllowed() { $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); @@ -2634,9 +2632,9 @@ public function testUpdateShareCantAccess() { ->with($this->currentUser) ->willReturn($userFolder); - $userFolder->method('getFirstNodeById') + $userFolder->method('getById') ->with($share->getNodeId()) - ->willReturn($share->getNode()); + ->willReturn([$share->getNode()]); $this->ocs->updateShare(42); } @@ -2727,6 +2725,9 @@ public function testUpdateLinkShareClear() { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$node]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($node); @@ -2781,9 +2782,9 @@ public function testUpdateLinkShareSet() { ->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') @@ -2831,9 +2832,9 @@ public function testUpdateLinkShareEnablePublicUpload($permissions, $publicUploa ->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') @@ -2889,9 +2890,9 @@ public function testUpdateLinkShareSetCRUDPermissions($permissions) { ->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') @@ -2947,9 +2948,9 @@ public function testUpdateLinkShareInvalidDate() { $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); @@ -2994,9 +2995,9 @@ public function testUpdateLinkSharePublicUploadNotAllowed($permissions, $publicU $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); @@ -3022,10 +3023,13 @@ public function testUpdateLinkSharePublicUploadOnFile() { $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); @@ -3059,9 +3063,9 @@ public function testUpdateLinkSharePasswordDoesNotChangeOther(): void { [$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); @@ -3110,9 +3114,9 @@ public function testUpdateLinkShareSendPasswordByTalkDoesNotChangeOther() { $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); @@ -3168,9 +3172,9 @@ public function testUpdateLinkShareSendPasswordByTalkWithTalkDisabledDoesNotChan $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); @@ -3208,9 +3212,9 @@ public function testUpdateLinkShareDoNotSendPasswordByTalkDoesNotChangeOther() { $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); @@ -3302,9 +3306,9 @@ public function testUpdateLinkShareDoNotSendPasswordByTalkWithTalkDisabledDoesNo ->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') @@ -3370,9 +3374,9 @@ public function testUpdateLinkShareExpireDateDoesNotChangeOther() { ->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') @@ -3431,9 +3435,9 @@ public function testUpdateLinkSharePublicUploadDoesNotChangeOther() { ->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') @@ -3491,9 +3495,9 @@ public function testUpdateLinkSharePermissions() { ->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') @@ -3551,9 +3555,9 @@ public function testUpdateLinkSharePermissionsShare() { ->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') @@ -3599,9 +3603,9 @@ public function testUpdateOtherPermissions() { ->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') @@ -3665,6 +3669,9 @@ public function testUpdateShareCannotIncreasePermissions() { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($folder); @@ -3735,9 +3742,9 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() { ->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') @@ -4981,6 +4988,9 @@ private function getNonSharedUserFolder(): array { $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 8cc97fe71ee58..e4da84365085c 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