From d587d5f847b9f8a476fdca4ed24f29af55e7c732 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 19 Jun 2020 16:25:49 +0200 Subject: 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 9d7234d45fa..6bf8203a080 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -291,13 +291,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 d200a0ae9b0..6db9beb3a0c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -602,6 +602,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); @@ -623,6 +624,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); @@ -644,6 +646,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') @@ -664,6 +667,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') @@ -698,6 +702,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); -- cgit v1.2.3 From c47a6411d59a20e80969ddd0dde1f6d5a58c8b78 Mon Sep 17 00:00:00 2001 From: Daniel Calviño Sánchez Date: Mon, 22 Jun 2020 19:40:11 +0200 Subject: 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 --- .../integration/features/sharing-v1-part2.feature | 228 +++++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature index 9fbb4cda947..0fc99b24594 100644 --- a/build/integration/features/sharing-v1-part2.feature +++ b/build/integration/features/sharing-v1-part2.feature @@ -231,6 +231,34 @@ 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 As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + 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 @@ -251,6 +279,86 @@ 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 As an "user0" + And creating a share with + | path | /textfile0.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + 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 As an "user0" + And creating a share with + | path | /textfile1.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + 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 As an "user0" + And creating a share with + | path | /textfile1.txt | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + 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 @@ -456,6 +564,37 @@ Feature: sharing | permissions | 1 | Then the OCS status code should be "100" + 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 As an "user0" + And creating a share with + | path | /TMP | + | shareType | 0 | + | shareWith | user1 | + | permissions | 17 | + And As an "user1" + 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 @@ -477,6 +616,95 @@ Feature: sharing | permissions | 31 | Then the OCS status code should be "404" + 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 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 | + | 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 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 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 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 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 -- cgit v1.2.3 From c341a4b3882eda5f394cd66a9b33a12f51d53aa2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 13:58:04 +0200 Subject: 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 96ee0f06f8c..57bddf461b0 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) { -- cgit v1.2.3 From 76e415a75c4e2e3ea821260f878bc41514de423b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 13:58:27 +0200 Subject: 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 60900128717..075c1964347 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -437,6 +437,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 9057f62995f..8496a296d88 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 36e10bfb45a..88c2a84c9b5 100644 --- a/lib/private/Files/Mount/MountPoint.php +++ b/lib/private/Files/Mount/MountPoint.php @@ -266,7 +266,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; -- cgit v1.2.3 From 50f49c2dc4d1e016241cab7e285f5f7de2f0ff1a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 8 Jul 2020 15:32:11 +0200 Subject: 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 4aba9e29113..c7458a55a32 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -120,6 +120,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 e49b696e68a..63a7aac5775 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -124,6 +124,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(); } -- cgit v1.2.3