From 8f972785f7a7be350bc43e4428504af3550ff357 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 | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index a6ad70a7f4bf2..66e39bb0715ae 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -975,10 +975,20 @@ public function updateShare( } if ($permissions !== null && $share->getShareOwner() !== $this->currentUser) { + + // 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, $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)); + $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 9bb690d299085a9667918a8f540e3631ad0b2103 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 6ab7cfdf9a0fa..9ef57682534c4 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 824c57d508ec54dd90597e00619da101d09c0b38 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 9ef57682534c4..db31107964e48 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 808280b1a6d772329660763f7d7a1b2386669a96 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 27 Jun 2019 15:27:21 +0200 Subject: [PATCH 4/4] Fix sharing tests Signed-off-by: Roeland Jago Douma --- .../Controller/ShareAPIControllerTest.php | 230 ++++++++++++++++++ 1 file changed, 230 insertions(+) diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 67130c01eb509..f00b5c424bf15 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; @@ -1573,6 +1574,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) @@ -1607,6 +1610,21 @@ 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', '', '', '', 'false'); @@ -1618,6 +1636,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) @@ -1645,6 +1665,21 @@ 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', 'note', 'label', 'true'); @@ -1659,6 +1694,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) @@ -1679,6 +1716,21 @@ 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); @@ -1949,6 +2001,9 @@ public function testUpdateLinkShareDoNotSendPasswordByTalkWithTalkDisabledDoesNo $date->setTime(0,0,0); $node = $this->getMockBuilder(File::class)->getMock(); + $node->method('getId') + ->willReturn(42); + $share = $this->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) @@ -1982,6 +2037,27 @@ public function testUpdateLinkShareDoNotSendPasswordByTalkWithTalkDisabledDoesNo }) )->will($this->returnArgument(0)); + $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); + + $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', null, null, null, null, null); @@ -1993,6 +2069,9 @@ public function testUpdateLinkShareExpireDateDoesNotChangeOther() { $ocs = $this->mockFormatShare(); $node = $this->getMockBuilder(File::class)->getMock(); + $node->method('getId') + ->willReturn(42); + $share = $this->newShare(); $share->setPermissions(\OCP\Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) @@ -2027,6 +2106,21 @@ public function testUpdateLinkShareExpireDateDoesNotChangeOther() { }) )->will($this->returnArgument(0)); + $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, null, null, '2010-12-23', null, null, null); @@ -2040,6 +2134,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) @@ -2072,6 +2168,21 @@ 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, null, null, null); @@ -2085,6 +2196,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) @@ -2116,6 +2229,21 @@ 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, null, null, null); @@ -2129,6 +2257,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) @@ -2158,6 +2288,21 @@ public function testUpdateLinkSharePermissionsShare() { }) )->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); + $this->shareManager->method('getSharedWith')->willReturn([]); $expected = new DataResponse([]); @@ -2171,6 +2316,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) @@ -2189,6 +2336,21 @@ 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); @@ -2200,6 +2362,8 @@ public function testUpdateShareCannotIncreasePermissions() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2239,6 +2403,21 @@ 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 { @@ -2253,6 +2432,8 @@ public function testUpdateShareCannotIncreasePermissionsLinkShare() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2285,6 +2466,21 @@ 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); @@ -2300,6 +2496,8 @@ public function testUpdateShareCannotIncreasePermissionsRoomShare() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2341,6 +2539,21 @@ function ($id) use ($share) { } )); + $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->any()) ->method('getSharedWith') ->will($this->returnValueMap([ @@ -2363,6 +2576,8 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2400,6 +2615,21 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() { ->with($share) ->willReturn($share); + $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); + $result = $ocs->updateShare(42, 31); $this->assertInstanceOf(DataResponse::class, $result); }