From 029ecb7966a5b38ca98cf770fca88c243684f8ad Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 2 Jul 2019 10:22:30 +0200 Subject: [PATCH] Better check reshare permissions when creating a share Signed-off-by: Joas Schilling --- .../features/sharing-v1-part2.feature | 60 +++++++++++++++++++ lib/private/Share20/Manager.php | 25 +++++++- tests/lib/Share20/ManagerTest.php | 6 ++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature index f6532ea564d..9fbb4cda947 100644 --- a/build/integration/features/sharing-v1-part2.feature +++ b/build/integration/features/sharing-v1-part2.feature @@ -251,6 +251,66 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: User is not allowed to reshare file with additional delete permissions + As an "admin" + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And As an "user0" + And creating a share with + | path | /PARENT | + | shareType | 0 | + | shareWith | user1 | + | permissions | 16 | + And As an "user1" + When creating a share with + | path | /PARENT (2) | + | shareType | 0 | + | shareWith | user2 | + | permissions | 25 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + + Scenario: User is not allowed to reshare file with additional delete permissions for files + As an "admin" + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 16 | + And As an "user1" + When creating a share with + | path | /textfile0 (2).txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 25 | + 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 + | id | A_NUMBER | + | item_type | file | + | item_source | A_NUMBER | + | share_type | 0 | + | share_with | user2 | + | file_source | A_NUMBER | + | file_target | /textfile0 (2).txt | + | path | /textfile0 (2).txt | + | permissions | 17 | + | stime | A_NUMBER | + | storage | A_NUMBER | + | mail_send | 0 | + | uid_owner | user1 | + | storage_id | shared::/textfile0 (2).txt | + | file_parent | A_NUMBER | + | share_with_displayname | user2 | + | displayname_owner | user1 | + | mimetype | text/plain | + Scenario: Get a share with a user which didn't received the share Given user "user0" exists And user "user1" exists diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 8ca6185d08c..7d9a0380b25 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -269,11 +269,13 @@ class Manager implements IManager { // And you can't share your rootfolder if ($this->userManager->userExists($share->getSharedBy())) { - $sharedPath = $this->rootFolder->getUserFolder($share->getSharedBy())->getPath(); + $userFolder = $this->rootFolder->getUserFolder($share->getSharedBy()); + $userFolderPath = $userFolder->getPath(); } else { - $sharedPath = $this->rootFolder->getUserFolder($share->getShareOwner())->getPath(); + $userFolder = $this->rootFolder->getUserFolder($share->getShareOwner()); + $userFolderPath = $userFolder->getPath(); } - if ($sharedPath === $share->getNode()->getPath()) { + if ($userFolderPath === $share->getNode()->getPath()) { throw new \InvalidArgumentException('You can’t share your root folder'); } @@ -297,6 +299,23 @@ class Manager implements IManager { $mount = $share->getNode()->getMountPoint(); if (!($mount instanceof MoveableMount)) { $permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE; + } else if ($share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { + $userMountPointId = $mount->getStorageRootId(); + $userMountPoints = $userFolder->getById($userMountPointId); + $userMountPoint = array_shift($userMountPoints); + + /* Check if this is an incoming share */ + $incomingShares = $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_USER, $userMountPoint, -1, 0); + $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0)); + $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0)); + + /** @var \OCP\Share\IShare[] $incomingShares */ + if (!empty($incomingShares)) { + $permissions = 0; + foreach ($incomingShares as $incomingShare) { + $permissions |= $incomingShare->getPermissions(); + } + } } // Check that we do not share with more permissions than we have diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 3387c6b7cdd..c1dbf1cdcb5 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -590,6 +590,12 @@ class ManagerTest extends \Test\TestCase { $limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ); $limitedPermssions->method('getPath')->willReturn('path'); + $owner = $this->createMock(IUser::class); + $owner->method('getUID') + ->willReturn($user0); + $limitedPermssions->method('getOwner') + ->willReturn($owner); + $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $limitedPermssions, $user2, $user0, $user0, null, null, null), 'A share requires permissions', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null), 'A share requires permissions', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $limitedPermssions, null, $user0, $user0, null, null, null), 'A share requires permissions', true]; -- 2.39.5