From f174fb91e0cf1774bff14bc2526d3c62ffdb806a 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 04093278694b7..f51bf87ee4345 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -298,13 +298,20 @@ protected function generalCreateChecks(\OCP\Share\IShare $share) { $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 e66ac51aeea93..f2086e388d379 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -613,6 +613,7 @@ public function dataGeneralChecks() { $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); @@ -634,6 +635,7 @@ public function dataGeneralChecks() { $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); @@ -655,6 +657,7 @@ public function dataGeneralChecks() { $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') @@ -675,6 +678,7 @@ public function dataGeneralChecks() { $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') @@ -709,6 +713,11 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) { $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 2ba6879937b13c78b128733f406d9f7c3be7a0bf 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 52fbe73dc9c80..8fc06fbddebba 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 b95ba97d27238fea36deae05dc67a6d825eaf1b9 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 b28be47875a3a..88c5b468543ff 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -441,7 +441,7 @@ protected function isFileLocked($view, $path, $type, $onMountPoint = false) { } } - 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 0437157e84ff7..379d33ea71c17 100644 --- a/tests/lib/Traits/MountProviderTrait.php +++ b/tests/lib/Traits/MountProviderTrait.php @@ -33,6 +33,12 @@ protected function registerMount($userId, $storage, $mountPoint, $arguments = nu $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 157f619812df98649aa5094f11cfe819fc989989 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 d3c4e1afc62a2..9d534815cdc08 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -437,6 +437,10 @@ public static function initMountPoints($user = '') { // 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 744a57981e2f6..68c69cc8cdc2f 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 2cb25f07044b5..f9cda6fbce8e1 100644 --- a/lib/private/Files/Mount/MountPoint.php +++ b/lib/private/Files/Mount/MountPoint.php @@ -268,7 +268,7 @@ public function getOptions() { * @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 379cfdda3c77b8909cc444a48ecd6635b8679143 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 081b720f820dd..a6e9e843529ee 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -127,6 +127,8 @@ protected function setUpShares() { $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 a201974de452a..34924110b30fa 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -125,6 +125,14 @@ protected function tearDown(): void { $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(); }