Merge pull request #21489 from nextcloud/fix/share/use_correct_mount

Use the correct mountpoint to calculate

apps/files_sharing/tests/EtagPropagationTest.php

@@ -128,6 +128,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);
 

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

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

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);
 

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];

lib/private/Files/Mount/MountPoint.php

@@ -268,7 +268,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;

lib/private/Share20/Manager.php

@@ -298,13 +298,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(), IShare::TYPE_USER, $userMountPoint, -1, 0);
 			$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_GROUP, $userMountPoint, -1, 0));

tests/lib/Share20/ManagerTest.php

@@ -613,6 +613,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);
@@ -634,6 +635,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);
@@ -655,6 +657,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')
@@ -675,6 +678,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')
@@ -709,6 +713,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);

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')) {

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) {