From bb9da2577298e967dcb36abf29dd1278da7d633f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Jun 2020 16:25:49 +0200 Subject: [PATCH 1/5] Use the correct mountpoint to calculate If we use the owners mount point this results in null. And then the rest of the checks get called with null. Which doesn't work. Signed-off-by: Roeland Jago Douma --- lib/private/Share20/Manager.php | 9 ++++++++- tests/lib/Share20/ManagerTest.php | 9 +++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 9e7f034add4..c8ba04a4636 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -299,13 +299,20 @@ class Manager implements IManager { $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); $permissions = 0; - $mount = $share->getNode()->getMountPoint(); + + $userMounts = $userFolder->getById($share->getNode()->getId()); + $userMount = array_shift($userMounts); + $mount = $userMount->getMountPoint(); if (!$isFederatedShare && $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { // When it's a reshare use the parent share permissions as maximum $userMountPointId = $mount->getStorageRootId(); $userMountPoints = $userFolder->getById($userMountPointId); $userMountPoint = array_shift($userMountPoints); + if ($userMountPoint === null) { + throw new GenericShareException('Could not get proper user mount for ' . $userMountPointId . '. Failing since else the next calls are called with null'); + } + /* Check if this is an incoming share */ $incomingShares = $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_USER, $userMountPoint, -1, 0); $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0)); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index ce35baf2ee4..0fbbbcbe2e5 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -614,6 +614,7 @@ class ManagerTest extends \Test\TestCase { $limitedPermssions = $this->createMock(File::class); $limitedPermssions->method('isShareable')->willReturn(true); $limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ); + $limitedPermssions->method('getId')->willReturn(108); $limitedPermssions->method('getPath')->willReturn('path'); $limitedPermssions->method('getOwner') ->willReturn($owner); @@ -635,6 +636,7 @@ class ManagerTest extends \Test\TestCase { $nonMoveableMountPermssions = $this->createMock(Folder::class); $nonMoveableMountPermssions->method('isShareable')->willReturn(true); $nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ); + $nonMoveableMountPermssions->method('getId')->willReturn(108); $nonMoveableMountPermssions->method('getPath')->willReturn('path'); $nonMoveableMountPermssions->method('getOwner') ->willReturn($owner); @@ -656,6 +658,7 @@ class ManagerTest extends \Test\TestCase { $allPermssions = $this->createMock(Folder::class); $allPermssions->method('isShareable')->willReturn(true); $allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); + $allPermssions->method('getId')->willReturn(108); $allPermssions->method('getOwner') ->willReturn($owner); $allPermssions->method('getStorage') @@ -676,6 +679,7 @@ class ManagerTest extends \Test\TestCase { $remoteFile = $this->createMock(Folder::class); $remoteFile->method('isShareable')->willReturn(true); $remoteFile->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ ^ \OCP\Constants::PERMISSION_UPDATE); + $remoteFile->method('getId')->willReturn(108); $remoteFile->method('getOwner') ->willReturn($owner); $remoteFile->method('getStorage') @@ -710,6 +714,11 @@ class ManagerTest extends \Test\TestCase { $userFolder->expects($this->any()) ->method('getId') ->willReturn(42); + // Id 108 is used in the data to refer to the node of the share. + $userFolder->expects($this->any()) + ->method('getById') + ->with(108) + ->willReturn([$share->getNode()]); $userFolder->expects($this->any()) ->method('getRelativePath') ->willReturnArgument(0); From 52aa0ef3dabffc3b3e921fa07b96f7f315b427dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 22 Jun 2020 19:40:11 +0200 Subject: [PATCH 2/5] Add more integration tests for resharing permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../sharing_features/sharing-v1-part2.feature | 244 ++++++++++++++++++ 1 file changed, 244 insertions(+) diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature index 52fbe73dc9c..8fc06fbddeb 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -562,6 +562,36 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: User is allowed to reshare file with more permissions if shares of same file to same user have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 1 | + | shareWith | group1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + And accepting last share + When creating a share with + | path | /textfile0 (2).txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 19 | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + Scenario: User is not allowed to reshare file with more permissions As an "admin" Given user "user0" exists @@ -583,6 +613,92 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: User is not allowed to reshare file with more permissions even if shares of same file to other users have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user3 | + | permissions | 15 | + And As an "user3" + And accepting last share + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + And accepting last share + When creating a share with + | path | /textfile0 (2).txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 19 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + + Scenario: User is not allowed to reshare file with more permissions even if shares of other files from same user have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And As an "user0" + And creating a share with + | path | /textfile1.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + And accepting last share + When creating a share with + | path | /textfile1 (2).txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 19 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + + Scenario: User is not allowed to reshare file with more permissions even if shares of other files from other users have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And As an "user3" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And As an "user0" + And creating a share with + | path | /textfile1.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + And accepting last share + When creating a share with + | path | /textfile1 (2).txt | + | shareType | 0 | + | shareWith | user2 | + | permissions | 19 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + Scenario: User is not allowed to reshare file with additional delete permissions As an "admin" Given user "user0" exists @@ -857,6 +973,39 @@ Feature: sharing Then the OCS status code should be "100" And the HTTP status code should be "200" + Scenario: Allow reshare to exceed permissions if shares of same file to same user have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And group "group1" exists + And user "user1" belongs to group "group1" + And user "user0" created a folder "/TMP" + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 1 | + | shareWith | group1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + And accepting last share + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user2 | + | permissions | 17 | + When Updating last share with + | permissions | 31 | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + Scenario: Do not allow reshare to exceed permissions Given user "user0" exists And user "user1" exists @@ -880,6 +1029,101 @@ Feature: sharing Then the OCS status code should be "404" And the HTTP status code should be "200" + Scenario: Do not allow reshare to exceed permissions even if shares of same file to other users have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And user "user0" created a folder "/TMP" + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user3 | + | permissions | 15 | + And As an "user3" + And accepting last share + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user1 | + | permissions | 21 | + And As an "user1" + And accepting last share + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user2 | + | permissions | 21 | + When Updating last share with + | permissions | 31 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + + Scenario: Do not allow reshare to exceed permissions even if shares of other files from same user have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And As an "user0" + And creating a share with + | path | /FOLDER | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And user "user0" created a folder "/TMP" + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user1 | + | permissions | 21 | + And As an "user1" + And accepting last share + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user2 | + | permissions | 21 | + When Updating last share with + | permissions | 31 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + + Scenario: Do not allow reshare to exceed permissions even if shares of other files from other users have them + Given user "user0" exists + And user "user1" exists + And user "user2" exists + And user "user3" exists + And As an "user3" + And creating a share with + | path | /FOLDER | + | shareType | 0 | + | shareWith | user1 | + | permissions | 15 | + And As an "user1" + And accepting last share + And user "user0" created a folder "/TMP" + And As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user1 | + | permissions | 21 | + And As an "user1" + And accepting last share + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user2 | + | permissions | 21 | + When Updating last share with + | permissions | 31 | + Then the OCS status code should be "404" + And the HTTP status code should be "200" + Scenario: Do not allow sub reshare to exceed permissions Given user "user0" exists And user "user1" exists From 2eb61b4111d4269239c68c4d057192c7bb857a56 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 13:58:04 +0200 Subject: [PATCH 3/5] ensure mounts are scanned during tests Signed-off-by: Robin Appelman --- tests/lib/TestCase.php | 2 +- tests/lib/Traits/MountProviderTrait.php | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php index b80e720efd3..40ba26a895c 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -441,7 +441,7 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase { } } - private function IsDatabaseAccessAllowed() { + protected function IsDatabaseAccessAllowed() { // on travis-ci.org we allow database access in any case - otherwise // this will break all apps right away if (true == getenv('TRAVIS')) { diff --git a/tests/lib/Traits/MountProviderTrait.php b/tests/lib/Traits/MountProviderTrait.php index 0437157e84f..379d33ea71c 100644 --- a/tests/lib/Traits/MountProviderTrait.php +++ b/tests/lib/Traits/MountProviderTrait.php @@ -33,6 +33,12 @@ trait MountProviderTrait { $this->mounts[$userId] = []; } $this->mounts[$userId][] = ['storage' => $storage, 'mountPoint' => $mountPoint, 'arguments' => $arguments]; + + if ($this->IsDatabaseAccessAllowed()) { + $mount = new MountPoint($storage, $mountPoint, $arguments, $this->storageFactory); + $storage = $mount->getStorage(); + $storage->getScanner()->scan(''); + } } protected function registerStorageWrapper($name, $wrapper) { From cc35da4d28ed4da9ad7b11a3a907cc99c535b10a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 13:58:27 +0200 Subject: [PATCH 4/5] ensure home storage is initialized on first setup Signed-off-by: Robin Appelman --- lib/private/Files/Filesystem.php | 4 ++++ lib/private/Files/Mount/LocalHomeMountProvider.php | 2 +- lib/private/Files/Mount/MountPoint.php | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 358c707f186..4b04656f49b 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -438,6 +438,10 @@ class Filesystem { // home mounts are handled seperate since we need to ensure this is mounted before we call the other mount providers $homeMount = $mountConfigManager->getHomeMountForUser($userObject); + if ($homeMount->getStorageRootId() === -1) { + $homeMount->getStorage()->mkdir(''); + $homeMount->getStorage()->getScanner()->scan(''); + } self::getMountManager()->addMount($homeMount); diff --git a/lib/private/Files/Mount/LocalHomeMountProvider.php b/lib/private/Files/Mount/LocalHomeMountProvider.php index 744a57981e2..68c69cc8cdc 100644 --- a/lib/private/Files/Mount/LocalHomeMountProvider.php +++ b/lib/private/Files/Mount/LocalHomeMountProvider.php @@ -35,7 +35,7 @@ class LocalHomeMountProvider implements IHomeMountProvider { * * @param IUser $user * @param IStorageFactory $loader - * @return \OCP\Files\Mount\IMountPoint[] + * @return \OCP\Files\Mount\IMountPoint|null */ public function getHomeMountForUser(IUser $user, IStorageFactory $loader) { $arguments = ['user' => $user]; diff --git a/lib/private/Files/Mount/MountPoint.php b/lib/private/Files/Mount/MountPoint.php index 2b5fa3f7199..d9d5117cd3f 100644 --- a/lib/private/Files/Mount/MountPoint.php +++ b/lib/private/Files/Mount/MountPoint.php @@ -267,7 +267,7 @@ class MountPoint implements IMountPoint { * @return int */ public function getStorageRootId() { - if (is_null($this->rootId)) { + if (is_null($this->rootId) || $this->rootId === -1) { $this->rootId = (int)$this->getStorage()->getCache()->getId(''); } return $this->rootId; From 498a1d029387518330484df72390ad50dc2b5bbe Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 15:32:11 +0200 Subject: [PATCH 5/5] better cleanup in share tests Signed-off-by: Robin Appelman --- apps/files_sharing/tests/EtagPropagationTest.php | 2 ++ apps/files_sharing/tests/TestCase.php | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/apps/files_sharing/tests/EtagPropagationTest.php b/apps/files_sharing/tests/EtagPropagationTest.php index bcab1a92bb8..6ab1051c558 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -126,6 +126,8 @@ class EtagPropagationTest extends PropagationTestCase { $view2 = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); $view2->mkdir('/sub1/sub2'); $view2->rename('/folder', '/sub1/sub2/folder'); + $this->loginAsUser(self::TEST_FILES_SHARING_API_USER2); + $insideInfo = $view2->getFileInfo('/sub1/sub2/folder/inside'); $this->assertInstanceOf('\OC\Files\FileInfo', $insideInfo); diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php index 8ea72943be4..b0c122b8fe6 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -125,6 +125,14 @@ abstract class TestCase extends \Test\TestCase { $qb->delete('share'); $qb->execute(); + $qb = \OC::$server->getDatabaseConnection()->getQueryBuilder(); + $qb->delete('mounts'); + $qb->execute(); + + $qb = \OC::$server->getDatabaseConnection()->getQueryBuilder(); + $qb->delete('filecache'); + $qb->execute(); + parent::tearDown(); }