diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 95b688fef5..c8dbc001f2 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -1602,25 +1602,46 @@ class View { /** * check if 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 + * It is not allowed to move a mount point into a different mount point or + * into an already shared folder * * @param string $target path * @return boolean */ private function isTargetAllowed($target) { - $result = false; - - list($targetStorage,) = \OC\Files\Filesystem::resolvePath($target); - if ($targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage')) { - $result = true; - } else { + list($targetStorage, $targetInternalPath) = \OC\Files\Filesystem::resolvePath($target); + if (!$targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage')) { \OCP\Util::writeLog('files', 'It is not allowed to move one mount point into another one', \OCP\Util::DEBUG); + return false; } - return $result; + // 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; } /** diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index ceeb9ba7a9..94f9e20915 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -104,6 +104,9 @@ class View extends \Test\TestCase { $this->userObject->delete(); $this->groupObject->delete(); + $mountProviderCollection = \OC::$server->getMountProviderCollection(); + \Test\TestCase::invokePrivate($mountProviderCollection, 'providers', [[]]); + parent::tearDown(); } @@ -1486,6 +1489,112 @@ class View extends \Test\TestCase { $this->assertEquals($shouldEmit, $result); } + /** + * Create test movable mount points + * + * @param array $mountPoints array of mount point locations + * @return array array of MountPoint objects + */ + private function createTestMovableMountPoints($mountPoints) { + $mounts = []; + foreach ($mountPoints as $mountPoint) { + $storage = $this->getMockBuilder('\OC\Files\Storage\Temporary') + ->setMethods([]) + ->getMock(); + + $mounts[] = $this->getMock( + '\Test\TestMoveableMountPoint', + ['moveMount'], + [$storage, $mountPoint] + ); + } + + $mountProvider = $this->getMock('\OCP\Files\Config\IMountProvider'); + $mountProvider->expects($this->any()) + ->method('getMountsForUser') + ->will($this->returnValue($mounts)); + + $mountProviderCollection = \OC::$server->getMountProviderCollection(); + $mountProviderCollection->registerProvider($mountProvider); + + return $mounts; + } + + /** + * Test mount point move + */ + public function testMountPointMove() { + $this->loginAsUser($this->user); + + list($mount1, $mount2) = $this->createTestMovableMountPoints([ + $this->user . '/files/mount1', + $this->user . '/files/mount2', + ]); + $mount1->expects($this->once()) + ->method('moveMount') + ->will($this->returnValue(true)); + + $mount2->expects($this->once()) + ->method('moveMount') + ->will($this->returnValue(true)); + + $view = new \OC\Files\View('/' . $this->user . '/files/'); + $view->mkdir('sub'); + + $this->assertTrue($view->rename('mount1', 'renamed_mount'), 'Can rename mount point'); + $this->assertTrue($view->rename('mount2', 'sub/moved_mount'), 'Can move a mount point into a subdirectory'); + } + /** + * Test that moving a mount point into another is forbidden + */ + public function testMoveMountPointIntoAnother() { + $this->loginAsUser($this->user); + + list($mount1, $mount2) = $this->createTestMovableMountPoints([ + $this->user . '/files/mount1', + $this->user . '/files/mount2', + ]); + + $mount1->expects($this->never()) + ->method('moveMount'); + + $mount2->expects($this->never()) + ->method('moveMount'); + + $view = new \OC\Files\View('/' . $this->user . '/files/'); + + $this->assertFalse($view->rename('mount1', 'mount2'), 'Cannot overwrite another mount point'); + $this->assertFalse($view->rename('mount1', 'mount2/sub'), 'Cannot move a mount point into another'); + } + /** + * Test that moving a mount point into a shared folder is forbidden + */ + public function testMoveMountPointIntoSharedFolder() { + $this->loginAsUser($this->user); + + list($mount1) = $this->createTestMovableMountPoints([ + $this->user . '/files/mount1', + ]); + + $mount1->expects($this->never()) + ->method('moveMount'); + + $view = new \OC\Files\View('/' . $this->user . '/files/'); + $view->mkdir('shareddir'); + $view->mkdir('shareddir/sub'); + $view->mkdir('shareddir/sub2'); + + $fileId = $view->getFileInfo('shareddir')->getId(); + $userObject = \OC::$server->getUserManager()->createUser('test2', 'IHateNonMockableStaticClasses'); + $this->assertTrue(\OCP\Share::shareItem('folder', $fileId, \OCP\Share::SHARE_TYPE_USER, 'test2', \OCP\Constants::PERMISSION_READ)); + + $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'); + + $this->assertTrue(\OCP\Share::unshare('folder', $fileId, \OCP\Share::SHARE_TYPE_USER, 'test2')); + $userObject->delete(); + } public function basicOperationProviderForLocks() { return [ @@ -2042,23 +2151,9 @@ class View extends \Test\TestCase { public function testLockMoveMountPoint() { $this->loginAsUser('test'); - $subStorage = $this->getMockBuilder('\OC\Files\Storage\Temporary') - ->setMethods([]) - ->getMock(); - - $mount = $this->getMock( - '\Test\TestMoveableMountPoint', - ['moveMount'], - [$subStorage, $this->user . '/files/substorage'] - ); - - $mountProvider = $this->getMock('\OCP\Files\Config\IMountProvider'); - $mountProvider->expects($this->once()) - ->method('getMountsForUser') - ->will($this->returnValue([$mount])); - - $mountProviderCollection = \OC::$server->getMountProviderCollection(); - $mountProviderCollection->registerProvider($mountProvider); + list($mount) = $this->createTestMovableMountPoints([ + $this->user . '/files/substorage', + ]); $view = new \OC\Files\View('/' . $this->user . '/files/'); $view->mkdir('subdir'); @@ -2109,8 +2204,6 @@ class View extends \Test\TestCase { $this->assertNull($this->getFileLockType($view, $sourcePath, false), 'Shared storage root not locked after operation'); $this->assertNull($this->getFileLockType($view, $sourcePath, true), 'Source path not locked after operation'); $this->assertNull($this->getFileLockType($view, $targetPath, true), 'Target path not locked after operation'); - - \Test\TestCase::invokePrivate($mountProviderCollection, 'providers', [[]]); } /**