Merge pull request #21774 from nextcloud/backport/21489/stable17

[stable17] Use the correct mountpoint to calculate

10 files changed, 268 insertions, 4 deletions

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();
 	}
 
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
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;
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);
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) {