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 a6ad70a7f4..66e39bb071 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -975,10 +975,20 @@ class ShareAPIController extends OCSController { } 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 6ab7cfdf9a..9ef5768253 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 e90d44d1a6..f6532ea564 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 9ef5768253..db31107964 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 67130c01eb..f00b5c424b 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -31,6 +31,7 @@ use OCP\AppFramework\Http\DataResponse; 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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { }) )->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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { }) )->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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { }) )->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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { }) )->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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $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 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2239,6 +2403,21 @@ class ShareAPIControllerTest extends TestCase { ['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 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2285,6 +2466,21 @@ class ShareAPIControllerTest extends TestCase { ['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 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2341,6 +2539,21 @@ class ShareAPIControllerTest extends TestCase { } )); + $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 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2400,6 +2615,21 @@ class ShareAPIControllerTest extends TestCase { ->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); }