From d4f423d20b240b13ed36beacacb40dbe38763343 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 21 Jun 2019 09:22:06 +0200 Subject: [PATCH 1/4] Correctly check share permissions when updating a re-sub-share Before this change the node you shared was checked for permissions. This works when you reshare the folder that was shared with you. However when you reshared a subfolder (e.g. as public link), you could afterwards update the permissions and grant create+update permissions although the share you receive was read-only. Signed-off-by: Joas Schilling --- .../lib/Controller/ShareAPIController.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 21da5d72b1054..ee4a67ed441bf 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -895,10 +895,19 @@ public function updateShare( } if ($permissions !== null && $share->getShareOwner() !== $this->currentUser) { - /* Check if this is an incomming share */ - $incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $share->getNode(), -1, 0); - $incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0)); - $incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0)); + // Get the root mount point for the user and check the share permissions there + $userFolder = $this->rootFolder->getUserFolder($this->currentUser); + $userNodes = $userFolder->getById($share->getNodeId()); + $userNode = array_shift($userNodes); + + $userMountPointId = $userNode->getMountPoint()->getStorageRootId(); + $userMountPoints = $userFolder->getById($userMountPointId); + $userMountPoint = array_shift($userMountPoints); + + /* Check if this is an incoming share */ + $incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $userMountPoint, -1, 0); + $incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0)); + $incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0)); /** @var \OCP\Share\IShare[] $incomingShares */ if (!empty($incomingShares)) { From 8fb7247aa538a2724cadb6104cf32bde73e45ab0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 21 Jun 2019 12:16:27 +0200 Subject: [PATCH 2/4] Do not create folders with admin user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin user is not deleted after each integration test is run, so folders created by the admin user in one test are still there when the next tests run; tests should be independent one from each other, so a regular user that is created and deleted for each test should be used instead. Signed-off-by: Daniel Calviño Sánchez --- build/integration/features/sharing-v1-part3.feature | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/build/integration/features/sharing-v1-part3.feature b/build/integration/features/sharing-v1-part3.feature index 44a41341a028c..4f9314677df37 100644 --- a/build/integration/features/sharing-v1-part3.feature +++ b/build/integration/features/sharing-v1-part3.feature @@ -339,14 +339,16 @@ Feature: sharing Scenario: do not allow to increase link share permissions on reshare Given As an "admin" - And user "admin" created a folder "/TMP" And user "user0" exists + And user "user1" exists + And user "user0" created a folder "/TMP" + And As an "user0" And creating a share with | path | TMP | | shareType | 0 | - | shareWith | user0 | + | shareWith | user1 | | permissions | 17 | - When As an "user0" + When As an "user1" And creating a share with | path | TMP | | shareType | 3 | From 61a9ff5d3880b09d4b8f872b117da31e4621647f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 21 Jun 2019 12:22:21 +0200 Subject: [PATCH 3/4] Add integration test for increasing sub reshare permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests check an user share and a link share; there is a slight difference in style between them as each one is based on the test above it, which tests increasing reshare permissions. Signed-off-by: Daniel Calviño Sánchez --- .../features/sharing-v1-part2.feature | 22 +++++++++++++++++++ .../features/sharing-v1-part3.feature | 21 ++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature index e90d44d1a6193..f6532ea564d8a 100644 --- a/build/integration/features/sharing-v1-part2.feature +++ b/build/integration/features/sharing-v1-part2.feature @@ -417,6 +417,28 @@ Feature: sharing | permissions | 31 | Then the OCS status code should be "404" + Scenario: Do not allow sub reshare to exceed permissions + Given user "user0" exists + And user "user1" exists + And user "user2" exists + 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 | 0 | + | shareWith | user1 | + | permissions | 21 | + And As an "user1" + And creating a share with + | path | /TMP/SUB | + | shareType | 0 | + | shareWith | user2 | + | permissions | 21 | + When Updating last share with + | permissions | 31 | + Then the OCS status code should be "404" + Scenario: Only allow 1 link share per file/folder Given user "user0" exists And As an "user0" diff --git a/build/integration/features/sharing-v1-part3.feature b/build/integration/features/sharing-v1-part3.feature index 4f9314677df37..c559008c43c3e 100644 --- a/build/integration/features/sharing-v1-part3.feature +++ b/build/integration/features/sharing-v1-part3.feature @@ -357,6 +357,27 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: do not allow to increase link share permissions on sub reshare + Given As an "admin" + And user "user0" exists + And user "user1" exists + 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 | 0 | + | shareWith | user1 | + | permissions | 17 | + When As an "user1" + And creating a share with + | path | TMP/SUB | + | shareType | 3 | + And Updating last share with + | publicUpload | true | + Then the OCS status code should be "404" + 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 be9d3fe3000bc397aa412d1fa91904374f0fdc1a Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 28 Jun 2019 10:40:52 +0200 Subject: [PATCH 4/4] Update tests Signed-off-by: Roeland Jago Douma --- .../Controller/ShareAPIControllerTest.php | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 15c4071bc46ae..71af69f8779a3 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -31,6 +31,7 @@ use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\Files\File; use OCP\Files\Folder; +use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage; use OCP\IConfig; use OCP\IL10N; @@ -1498,6 +1499,8 @@ public function testUpdateLinkShareClear() { $ocs = $this->mockFormatShare(); $node = $this->getMockBuilder(Folder::class)->getMock(); + $node->method('getId') + ->willReturn(42); $share = $this->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) @@ -1524,6 +1527,19 @@ public function testUpdateLinkShareClear() { $this->shareManager->method('getSharedWith') ->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$node]); + $mountPoint = $this->createMock(IMountPoint::class); + $node->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, null, '', null, 'false', ''); @@ -1535,6 +1551,8 @@ public function testUpdateLinkShareSet() { $ocs = $this->mockFormatShare(); $folder = $this->getMockBuilder(Folder::class)->getMock(); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1559,6 +1577,19 @@ public function testUpdateLinkShareSet() { $this->shareManager->method('getSharedWith') ->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, null, 'password', null, 'true', '2000-01-01'); @@ -1573,6 +1604,8 @@ public function testUpdateLinkShareEnablePublicUpload($permissions, $publicUploa $ocs = $this->mockFormatShare(); $folder = $this->getMockBuilder(Folder::class)->getMock(); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1593,6 +1626,19 @@ public function testUpdateLinkShareEnablePublicUpload($permissions, $publicUploa }) )->will($this->returnArgument(0)); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, $permissions, $password, null, $publicUpload, $expireDate); @@ -1760,6 +1806,8 @@ public function testUpdateLinkSharePublicUploadDoesNotChangeOther() { $date = new \DateTime('2000-01-01'); $folder = $this->getMockBuilder(Folder::class)->getMock(); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1784,6 +1832,19 @@ public function testUpdateLinkSharePublicUploadDoesNotChangeOther() { $this->shareManager->method('getSharedWith') ->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, null, null, null, 'true', null); @@ -1797,6 +1858,8 @@ public function testUpdateLinkSharePermissions() { $date = new \DateTime('2000-01-01'); $folder = $this->getMockBuilder(Folder::class)->getMock(); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1820,6 +1883,19 @@ public function testUpdateLinkSharePermissions() { $this->shareManager->method('getSharedWith')->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, 7, null, null, null, null); @@ -1833,6 +1909,8 @@ public function testUpdateLinkSharePermissionsShare() { $date = new \DateTime('2000-01-01'); $folder = $this->getMockBuilder(Folder::class)->getMock(); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1856,6 +1934,19 @@ public function testUpdateLinkSharePermissionsShare() { $this->shareManager->method('getSharedWith')->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, 31, null, null, null, null); @@ -1867,6 +1958,8 @@ public function testUpdateOtherPermissions() { $ocs = $this->mockFormatShare(); $file = $this->getMockBuilder(File::class)->getMock(); + $file->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) @@ -1885,6 +1978,19 @@ public function testUpdateOtherPermissions() { $this->shareManager->method('getSharedWith')->willReturn([]); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$file]); + $mountPoint = $this->createMock(IMountPoint::class); + $file->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $expected = new DataResponse([]); $result = $ocs->updateShare(42, 31, null, null, null, null); @@ -1896,6 +2002,8 @@ public function testUpdateShareCannotIncreasePermissions() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -1935,6 +2043,19 @@ public function testUpdateShareCannotIncreasePermissions() { ['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []] ])); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $this->shareManager->expects($this->never())->method('updateShare'); try { @@ -1949,6 +2070,8 @@ public function testUpdateShareCannotIncreasePermissionsLinkShare() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -1981,6 +2104,19 @@ public function testUpdateShareCannotIncreasePermissionsLinkShare() { ['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []] ])); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $this->shareManager->expects($this->never())->method('updateShare'); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -1996,6 +2132,8 @@ public function testUpdateShareCannotIncreasePermissionsRoomShare() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2045,6 +2183,19 @@ function ($id) use ($share) { ['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, [$incomingShare]] ])); + $userFolder = $this->createMock(Folder::class); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); + $mountPoint = $this->createMock(IMountPoint::class); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $mountPoint->method('getStorageRootId') + ->willReturn(42); + $this->shareManager->expects($this->never())->method('updateShare'); try {