From b5bf60f2231186e9155679482dec32aa65c94fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 6 Jul 2021 09:08:07 +0200 Subject: [PATCH 1/4] Validate reshare permissions against actual path that the user tries to share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise this could lead to taking the wrong user mount in case there are multiple ones with different permissions that the user could use to reshare Signed-off-by: Julius Härtl --- lib/private/Share20/Manager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index ccc2d454a94de..0d22aa290fad7 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -292,10 +292,10 @@ protected function generalCreateChecks(IShare $share) { $permissions = 0; if (!$isFederatedShare && $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { - $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) { + $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) use ($share) { // We need to filter since there might be other mountpoints that contain the file // e.g. if the user has access to the same external storage that the file is originating from - return $mount->getStorage()->instanceOfStorage(ISharedStorage::class); + return $mount->getStorage()->instanceOfStorage(ISharedStorage::class) && $mount->getPath() === $share->getNode()->getPath(); }); $userMount = array_shift($userMounts); if ($userMount === null) { From 70845cfbd3d7f17712d115d7e8beb990ebce04b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 14 Sep 2021 19:00:18 +0200 Subject: [PATCH 2/4] Fix tests to use the proper node of the user creating the share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/files_sharing/tests/ApiTest.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index f3a3157851137..e76567e0ac7c7 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -510,8 +510,9 @@ public function testGetShareFromSourceWithReshares() { ->setPermissions(19); $share1 = $this->shareManager->createShare($share1); + $node2 = \OC::$server->getUserFolder(self::TEST_FILES_SHARING_API_USER2)->get($this->filename); $share2 = $this->shareManager->newShare(); - $share2->setNode($node) + $share2->setNode($node2) ->setSharedBy(self::TEST_FILES_SHARING_API_USER2) ->setSharedWith(self::TEST_FILES_SHARING_API_USER3) ->setShareType(IShare::TYPE_USER) @@ -633,7 +634,7 @@ public function testGetShareFromFolderReshares() { $share1->setStatus(IShare::STATUS_ACCEPTED); $this->shareManager->updateShare($share1); - $node2 = $this->userFolder->get($this->folder.'/'.$this->filename); + $node2 = \OC::$server->getUserFolder(self::TEST_FILES_SHARING_API_USER2)->get($this->folder.'/'.$this->filename); $share2 = $this->shareManager->newShare(); $share2->setNode($node2) ->setSharedBy(self::TEST_FILES_SHARING_API_USER2) @@ -643,7 +644,7 @@ public function testGetShareFromFolderReshares() { $share2->setStatus(IShare::STATUS_ACCEPTED); $this->shareManager->updateShare($share2); - $node3 = $this->userFolder->get($this->folder.'/'.$this->subfolder.'/'.$this->filename); + $node3 = \OC::$server->getUserFolder(self::TEST_FILES_SHARING_API_USER2)->get($this->folder.'/'.$this->subfolder.'/'.$this->filename); $share3 = $this->shareManager->newShare(); $share3->setNode($node3) ->setSharedBy(self::TEST_FILES_SHARING_API_USER2) @@ -824,8 +825,9 @@ public function testGetShareMultipleSharedFolder() { $share2->setStatus(IShare::STATUS_ACCEPTED); $this->shareManager->updateShare($share2); + $node2 = \OC::$server->getUserFolder(self::TEST_FILES_SHARING_API_USER2)->get($this->folder . $this->subfolder); $share3 = $this->shareManager->newShare(); - $share3->setNode($node1) + $share3->setNode($node2) ->setSharedBy(self::TEST_FILES_SHARING_API_USER2) ->setShareType(IShare::TYPE_LINK) ->setPermissions(1); From 62f507ec2ef1fbb2badd09dd51feea104043d30a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 20 Oct 2021 16:13:30 +0200 Subject: [PATCH 3/4] Add test case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../sharing_features/sharing-v1-part3.feature | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/build/integration/sharing_features/sharing-v1-part3.feature b/build/integration/sharing_features/sharing-v1-part3.feature index 42e0f1b6add04..00324093b0e35 100644 --- a/build/integration/sharing_features/sharing-v1-part3.feature +++ b/build/integration/sharing_features/sharing-v1-part3.feature @@ -472,6 +472,36 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: do allow to create links on a shared folder with two mountpoints + Given As an "admin" + And user "user0" exists + And user "user1" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "/TMP" + And user "user0" created a folder "/TMP/SUB" + And As an "user0" + And creating a share with + | path | TMP | + | shareType | 1 | + | shareWith | group1 | + | permissions | 15 | + When As an "user1" + And accepting last share + And As an "user0" + And creating a share with + | path | TMP/SUB | + | shareType | 0 | + | shareWith | user1 | + | permissions | 31 | + When As an "user1" + And accepting last share + And creating a share with + | path | TMP/SUB | + | shareType | 3 | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + Scenario: deleting file out of a share as recipient creates a backup for the owner Given As an "admin" And user "user0" exists From a6c557cc79e5b6d3f824f3187a55ffb38b264742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 28 Oct 2021 12:49:49 +0200 Subject: [PATCH 4/4] Match against share target and node path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/private/Share20/Manager.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 0d22aa290fad7..0deae776caeea 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -52,6 +52,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\HintException; use OCP\IConfig; use OCP\IGroupManager; @@ -292,10 +293,18 @@ protected function generalCreateChecks(IShare $share) { $permissions = 0; if (!$isFederatedShare && $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { - $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) use ($share) { + try { + $targetNode = $share->getTarget() ? $userFolder->get($share->getTarget()) : null; + } catch (NotFoundException $e) { + $targetNode = null; + } + $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) use ($share, $targetNode) { // We need to filter since there might be other mountpoints that contain the file // e.g. if the user has access to the same external storage that the file is originating from - return $mount->getStorage()->instanceOfStorage(ISharedStorage::class) && $mount->getPath() === $share->getNode()->getPath(); + return $mount->getStorage()->instanceOfStorage(ISharedStorage::class) && ( + $targetNode ? $targetNode->getPath() === $mount->getPath() : + $mount->getPath() === $share->getNode()->getPath() + ); }); $userMount = array_shift($userMounts); if ($userMount === null) {