Merge pull request #26615 from owncloud/share20-istargetallowed

Port isTargetAllowed to share 2.0
owncloud · Nov 29, 2016 · bebf120 · bebf120
2 parents 098b1bf + ec780cd
commit bebf120
Show file tree

Hide file tree

Showing 11 changed files with 241 additions and 55 deletions.
diff --git a/apps/files_sharing/lib/External/Mount.php b/apps/files_sharing/lib/External/Mount.php
@@ -67,4 +67,15 @@ public function moveMount($target) {
 	public function removeMount() {
 		return $this->manager->removeShare($this->mountPoint);
 	}
+
+	/**
+	 * Returns true
+	 * 
+	 * @param string $target unused
+	 * @return bool true
+	 */
+	public function isTargetAllowed($target) {
+		// note: home storage check already done in View
+		return true;
+	}
 }
diff --git a/apps/files_sharing/lib/SharedMount.php b/apps/files_sharing/lib/SharedMount.php
@@ -30,6 +30,7 @@
 use OC\Files\Mount\MountPoint;
 use OC\Files\Mount\MoveableMount;
 use OC\Files\View;
+use OCP\Share\IShare;
 
 /**
  * Shared mount points can be moved by the user
@@ -178,13 +179,60 @@ protected function stripUserFilesPath($path) {
 		return '/' . $relPath;
 	}
 
+	/**
+	 * Check whether it is allowed to move a mount point to a given target.
+	 * It is not allowed to move a mount point into a different mount point or
+	 * into an already shared folder
+	 *
+	 * @param string $target absolute target path
+	 * @return bool true if allowed, false otherwise
+	 */
+	public function isTargetAllowed($target) {
+		list($targetStorage, $targetInternalPath) = \OC\Files\Filesystem::resolvePath($target);
+		// note: cannot use the view because the target is already locked
+		$fileId = (int)$targetStorage->getCache()->getId($targetInternalPath);
+		if ($fileId === -1) {
+			// target might not exist, need to check parent instead
+			$fileId = (int)$targetStorage->getCache()->getId(dirname($targetInternalPath));
+		}
+
+		$targetNodes = \OC::$server->getRootFolder()->getById($fileId);
+		if (empty($targetNodes)) {
+			return false;
+		}
+
+		$shareManager = \OC::$server->getShareManager();
+		$targetNode = $targetNodes[0];
+		// FIXME: make it stop earlier in '/$userId/files'
+		while (!is_null($targetNode) && $targetNode->getPath() !== '/') { 
+			$shares = $shareManager->getSharesByPath($targetNode);
+
+			foreach ($shares as $share) {
+				if ($this->user === $share->getShareOwner()) {
+					\OCP\Util::writeLog('files',
+						'It is not allowed to move one mount point into a shared folder',
+						\OCP\Util::DEBUG);
+					return false;
+				}
+			}
+
+			$targetNode = $targetNode->getParent();
+		}
+
+		return true;
+	}
+
+
 	/**
 	 * Move the mount point to $target
 	 *
 	 * @param string $target the target mount point
 	 * @return bool
 	 */
 	public function moveMount($target) {
+		if (!$this->isTargetAllowed($target)) {
+			return false;
+		}
 
 		$relTargetPath = $this->stripUserFilesPath($target);
 		$share = $this->storage->getShare();

diff --git a/apps/files_sharing/tests/SharedMountTest.php b/apps/files_sharing/tests/SharedMountTest.php
@@ -27,6 +27,8 @@
 
 namespace OCA\Files_Sharing\Tests;
 
+use OCP\Files\NotPermittedException;
+
 /**
  * Class SharedMountTest
  *
@@ -388,6 +390,59 @@ function testPermissionUpgradeOnUserDeletedGroupShare() {
 		\OC_Group::removeFromGroup(self::TEST_FILES_SHARING_API_USER3, 'testGroup');
 	}
 
+	public function testIsTargetAllowed() {
+		$user1 = self::TEST_FILES_SHARING_API_USER1;
+		$user2 = self::TEST_FILES_SHARING_API_USER2;
+		$user3 = self::TEST_FILES_SHARING_API_USER3;
+
+		// user1 shares with user2
+		$userFolder = \OC::$server->getUserFolder($user1);
+		$sharedFolder = $userFolder->newFolder('user1-share');
+
+		$share = $this->share(
+			\OCP\Share::SHARE_TYPE_USER,
+			$sharedFolder,
+			$user1,
+			$user2,
+			\OCP\Constants::PERMISSION_ALL);
+
+		$this->loginAsUser($user2);
+
+		// user2 shares with user3
+		$userFolder2 = \OC::$server->getUserFolder($user2);
+
+		$sharedFolder2 = $userFolder2->newFolder('shareddir');
+		$userFolder2->newFolder('shareddir/sub');
+		$userFolder2->newFolder('shareddir/sub2');
+
+		$share2 = $this->share(
+			\OCP\Share::SHARE_TYPE_USER,
+			$sharedFolder2,
+			$user2,
+			$user3,
+			\OCP\Constants::PERMISSION_ALL);
+
+		$receivedFolder = $userFolder2->get('user1-share');
+
+		// cannot move into any of these dirs
+		foreach ([
+			'/' . $user2 . '/files/shareddir',
+			'/' . $user2 . '/files/shareddir/sub',
+			'/' . $user2 . '/files/shareddir/sub2',
+		] as $targetDir) {
+			$caught = null;
+			try {
+				$receivedFolder->move($targetDir);
+			} catch (NotPermittedException $e) {
+				$caught = $e;
+			}
+
+			$this->assertInstanceOf('\OCP\Files\NotPermittedException', $e);
+		}
+
+		$this->shareManager->deleteShare($share);
+		$this->shareManager->deleteShare($share2);
+	}
 }
 
 class DummyTestClassSharedMount extends \OCA\Files_Sharing\SharedMount {

diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php
@@ -217,8 +217,12 @@ protected function getShareFromId($shareID) {
 	 * @return \OCP\Share\IShare
 	 */
 	protected function share($type, $path, $initiator, $recipient, $permissions) {
-		$userFolder = $this->rootFolder->getUserFolder($initiator);
-		$node = $userFolder->get($path);
+		if (is_string($path)) {
+			$userFolder = $this->rootFolder->getUserFolder($initiator);
+			$node = $userFolder->get($path);
+		} else {
+			$node = $path;
+		}
 
 		$share = $this->shareManager->newShare();
 		$share->setShareType($type)

diff --git a/lib/private/Files/External/PersonalMount.php b/lib/private/Files/External/PersonalMount.php
@@ -85,4 +85,15 @@ public function removeMount() {
 		$this->storagesService->removeStorage($this->numericStorageId);
 		return true;
 	}
+
+	/**
+	 * Returns true
+	 * 
+	 * @param string $target unused
+	 * @return bool true
+	 */
+	public function isTargetAllowed($target) {
+		// note: home storage check already done in View
+		return true;
+	}
 }
diff --git a/lib/private/Files/Mount/MoveableMount.php b/lib/private/Files/Mount/MoveableMount.php
@@ -41,4 +41,15 @@ public function moveMount($target);
 	 * @return bool
 	 */
 	public function removeMount();
+
+	/**
+	 * Returns whether this mount point is allowed to be moved into the given absolute target
+	 *
+	 * @param string $target absolute target path
+	 *
+	 * @return bool true if allowed, false otherwise
+	 *
+	 * @since 9.2
+	 */
+	public function isTargetAllowed($target);
 }
diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php
@@ -767,7 +767,7 @@ public function rename($path1, $path2) {
 				$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE, true);
 
 				if ($internalPath1 === '' and $mount1 instanceof MoveableMount) {
-					if ($this->isTargetAllowed($absolutePath2)) {
+					if ($this->canMove($mount1, $absolutePath2)) {
 						/**
 						 * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1
 						 */
@@ -1726,10 +1726,11 @@ private function assertPathLength($path) {
 	 * It is not allowed to move a mount point into a different mount point or
 	 * into an already shared folder
 	 *
-	 * @param string $target path
+	 * @param MoveableMount $mount1 moveable mount
+	 * @param string $target absolute target path
 	 * @return boolean
 	 */
-	private function isTargetAllowed($target) {
+	private function canMove(MoveableMount $mount1, $target) {
 
 		list($targetStorage, $targetInternalPath) = \OC\Files\Filesystem::resolvePath($target);
 		if (!$targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage')) {
@@ -1739,30 +1740,7 @@ private function isTargetAllowed($target) {
 			return false;
 		}
 
-		// note: cannot use the view because the target is already locked
-		$fileId = (int)$targetStorage->getCache()->getId($targetInternalPath);
-		if ($fileId === -1) {
-			// target might not exist, need to check parent instead
-			$fileId = (int)$targetStorage->getCache()->getId(dirname($targetInternalPath));
-		}
-
-		// check if any of the parents were shared by the current owner (include collections)
-		$shares = \OCP\Share::getItemShared(
-			'folder',
-			$fileId,
-			\OCP\Share::FORMAT_NONE,
-			null,
-			true
-		);
-
-		if (count($shares) > 0) {
-			\OCP\Util::writeLog('files',
-				'It is not allowed to move one mount point into a shared folder',
-				\OCP\Util::DEBUG);
-			return false;
-		}
-
-		return true;
+		return $mount1->isTargetAllowed($target);
 	}
 
 	/**

diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
@@ -983,6 +983,21 @@ public function getShareById($id, $recipient = null) {
 	 * @return Share[]
 	 */
 	public function getSharesByPath(\OCP\Files\Node $path, $page=0, $perPage=50) {
+		$types = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP];
+		$providers = [];
+		$results = [];
+
+		foreach ($types as $type) {
+			$provider = $this->factory->getProviderForType($type);
+			// store this way to deduplicate entries by id
+			$providers[$provider->identifier()] = $provider;
+		}
+
+		foreach ($providers as $provider) {
+			$results = array_merge($results, $provider->getSharesByPath($path));
+		}
+
+		return $results;
 	}
 
 	/**

diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php
@@ -1519,17 +1519,22 @@ public function testHookPaths($root, $path, $shouldEmit) {
 	 * Create test movable mount points
 	 *
 	 * @param array $mountPoints array of mount point locations
+	 * @param bool $isTargetAllowed value to return for the isTargetAllowed call
 	 * @return array array of MountPoint objects
 	 */
-	private function createTestMovableMountPoints($mountPoints) {
+	private function createTestMovableMountPoints($mountPoints, $isTargetAllowed = true) {
 		$mounts = [];
 		foreach ($mountPoints as $mountPoint) {
 			$storage = new Temporary();
 
-			$mounts[] = $this->getMockBuilder('\Test\TestMoveableMountPoint')
-				->setMethods(['moveMount'])
+			$testMount = $this->getMockBuilder('\Test\TestMoveableMountPoint')
+				->setMethods(['moveMount', 'isTargetAllowed'])
 				->setConstructorArgs([$storage, $mountPoint])
 				->getMock();
+			$testMount->expects($this->any())
+				->method('isTargetAllowed')
+				->will($this->returnValue($isTargetAllowed));
+			$mounts[] = $testMount;
 		}
 
 		$mountProvider = $this->createMock('\OCP\Files\Config\IMountProvider');
@@ -1592,43 +1597,26 @@ public function testMoveMountPointIntoAnother() {
 	}
 
 	/**
-	 * Test that moving a mount point into a shared folder is forbidden
+	 * Test that moving a mount point that says it's not allowed will fail
 	 */
-	public function testMoveMountPointIntoSharedFolder() {
+	public function testMoveMountPointNotAllowed() {
 		$this->loginAsUser($this->user);
 
 		$userFolder = \OC::$server->getUserFolder($this->user);
 
 		list($mount1) = $this->createTestMovableMountPoints([
 			$this->user . '/files/mount1',
-		]);
+		], false);
 
 		$mount1->expects($this->never())
 			->method('moveMount');
 
 		$view = new View('/' . $this->user . '/files/');
-		$view->mkdir('shareddir');
-		$view->mkdir('shareddir/sub');
-		$view->mkdir('shareddir/sub2');
-
-		$userObject = \OC::$server->getUserManager()->createUser('test2', 'IHateNonMockableStaticClasses');
-
-		$sharedFolder = \OC::$server->getUserFolder($this->user)->get('shareddir');
-		$shareManager = \OC::$server->getShareManager();
-		$share = $shareManager->newShare();
-		$share->setSharedBy($this->user);
-		$share->setShareType(\OCP\Share::SHARE_TYPE_USER);
-		$share->setNode($sharedFolder);
-		$share->setSharedWith('test2');
-		$share->setPermissions(\OCP\Constants::PERMISSION_READ);
-		$share = $shareManager->createShare($share);
+		$view->mkdir('somedir');
 
-		$this->assertFalse($view->rename('mount1', 'shareddir'), 'Cannot overwrite shared folder');
-		$this->assertFalse($view->rename('mount1', 'shareddir/sub'), 'Cannot move mount point into shared folder');
-		$this->assertFalse($view->rename('mount1', 'shareddir/sub/sub2'), 'Cannot move mount point into shared subfolder');
+		$fileId = $view->getFileInfo('somedir')->getId();
 
-		$shareManager->deleteShare($share);
-		$userObject->delete();
+		$this->assertFalse($view->rename('mount1', 'shareddir'), 'Cannot overwrite shared folder');
 	}
 
 	public function basicOperationProviderForLocks() {