diff options
author | Roeland Jago Douma <rullzer@users.noreply.github.com> | 2019-06-28 16:28:00 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-06-28 16:28:00 +0300 |
commit | 30455813534779cf7c59c39e9274665502e397c0 (patch) | |
tree | 23fd58366ebfba78b6dbb691b77e76c56f42f41c | |
parent | 538326b8ad0a54d05521b1115bccdc04bf172f0e (diff) | |
parent | be9d3fe3000bc397aa412d1fa91904374f0fdc1a (diff) |
Merge pull request #16143 from nextcloud/backport/16097/stable14
[stabel 14] Better check reshare permissions
4 files changed, 212 insertions, 7 deletions
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 21da5d72b10..ee4a67ed441 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -895,10 +895,19 @@ class ShareAPIController extends OCSController { } 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)) { diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 15c4071bc46..71af69f8779 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; @@ -1498,6 +1499,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) @@ -1524,6 +1527,19 @@ 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', ''); @@ -1535,6 +1551,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) @@ -1559,6 +1577,19 @@ 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'); @@ -1573,6 +1604,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) @@ -1593,6 +1626,19 @@ 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); @@ -1760,6 +1806,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) @@ -1784,6 +1832,19 @@ 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); @@ -1797,6 +1858,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) @@ -1820,6 +1883,19 @@ 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); @@ -1833,6 +1909,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) @@ -1856,6 +1934,19 @@ 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, 31, null, null, null, null); @@ -1867,6 +1958,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) @@ -1885,6 +1978,19 @@ 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); @@ -1896,6 +2002,8 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -1935,6 +2043,19 @@ 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 { @@ -1949,6 +2070,8 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -1981,6 +2104,19 @@ 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); @@ -1996,6 +2132,8 @@ class ShareAPIControllerTest extends TestCase { $ocs = $this->mockFormatShare(); $folder = $this->createMock(Folder::class); + $folder->method('getId') + ->willReturn(42); $share = \OC::$server->getShareManager()->newShare(); $share @@ -2045,6 +2183,19 @@ class ShareAPIControllerTest extends TestCase { ['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 { diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature index e90d44d1a61..f6532ea564d 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 44a41341a02..c559008c43c 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 | @@ -355,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 |