fix Folder->getById() when a single storage is mounted multiple times

Signed-off-by: Robin Appelman <robin@icewind.nl>

2 files changed, 59 insertions, 83 deletions

diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php
index 1f6edfd3bbc..400fd6bedcc 100644
--- a/lib/private/Files/Node/Folder.php
+++ b/lib/private/Files/Node/Folder.php
@@ -37,7 +37,6 @@ use OC\Files\Search\SearchComparison;
 use OC\Files\Search\SearchOrder;
 use OC\Files\Search\SearchQuery;
 use OCP\Files\Cache\ICacheEntry;
-use OCP\Files\Config\ICachedMountInfo;
 use OCP\Files\FileInfo;
 use OCP\Files\Mount\IMountPoint;
 use OCP\Files\NotFoundException;
@@ -350,17 +349,28 @@ class Folder extends Node implements \OCP\Files\Folder {
 			$user = null;
 		}
 		$mountsContainingFile = $mountCache->getMountsForFileId((int)$id, $user);
+
+		// when a user has access trough the same storage trough multiple paths
+		// (such as an external storage that is both mounted for a user and shared to the user)
+		// the mount cache will only hold a single entry for the storage
+		// this can lead to issues as the different ways the user has access to a storage can have different permissions
+		//
+		// so instead of using the cached entries directly, we instead filter the current mounts by the rootid of the cache entry
+
+		$mountRootIds = array_map(function ($mount) {
+			return $mount->getRootId();
+		}, $mountsContainingFile);
+		$mountRootPaths = array_map(function ($mount) {
+			return $mount->getRootInternalPath();
+		}, $mountsContainingFile);
+		$mountRoots = array_combine($mountRootIds, $mountRootPaths);
+
 		$mounts = $this->root->getMountsIn($this->path);
 		$mounts[] = $this->root->getMount($this->path);
-		/** @var IMountPoint[] $folderMounts */
-		$folderMounts = array_combine(array_map(function (IMountPoint $mountPoint) {
-			return $mountPoint->getMountPoint();
-		}, $mounts), $mounts);
 
-		/** @var ICachedMountInfo[] $mountsContainingFile */
-		$mountsContainingFile = array_values(array_filter($mountsContainingFile, function (ICachedMountInfo $cachedMountInfo) use ($folderMounts) {
-			return isset($folderMounts[$cachedMountInfo->getMountPoint()]);
-		}));
+		$mountsContainingFile = array_filter($mounts, function ($mount) use ($mountRoots) {
+			return isset($mountRoots[$mount->getStorageRootId()]);
+		});
 
 		if (count($mountsContainingFile) === 0) {
 			if ($user === $this->getAppDataDirectoryName()) {
@@ -369,18 +379,18 @@ class Folder extends Node implements \OCP\Files\Folder {
 			return [];
 		}
 
-		$nodes = array_map(function (ICachedMountInfo $cachedMountInfo) use ($folderMounts, $id) {
-			$mount = $folderMounts[$cachedMountInfo->getMountPoint()];
+		$nodes = array_map(function (IMountPoint $mount) use ($id, $mountRoots) {
+			$rootInternalPath = $mountRoots[$mount->getStorageRootId()];
 			$cacheEntry = $mount->getStorage()->getCache()->get((int)$id);
 			if (!$cacheEntry) {
 				return null;
 			}
 
 			// cache jails will hide the "true" internal path
-			$internalPath = ltrim($cachedMountInfo->getRootInternalPath() . '/' . $cacheEntry->getPath(), '/');
-			$pathRelativeToMount = substr($internalPath, strlen($cachedMountInfo->getRootInternalPath()));
+			$internalPath = ltrim($rootInternalPath . '/' . $cacheEntry->getPath(), '/');
+			$pathRelativeToMount = substr($internalPath, strlen($rootInternalPath));
 			$pathRelativeToMount = ltrim($pathRelativeToMount, '/');
-			$absolutePath = rtrim($cachedMountInfo->getMountPoint() . $pathRelativeToMount, '/');
+			$absolutePath = rtrim($mount->getMountPoint() . $pathRelativeToMount, '/');
 			return $this->root->createNode($absolutePath, new \OC\Files\FileInfo(
 				$absolutePath, $mount->getStorage(), $cacheEntry->getPath(), $cacheEntry, $mount,
 				\OC::$server->getUserManager()->get($mount->getStorage()->getOwner($pathRelativeToMount))
@@ -389,9 +399,13 @@ class Folder extends Node implements \OCP\Files\Folder {
 
 		$nodes = array_filter($nodes);
 
-		return array_filter($nodes, function (Node $node) {
+		$folders = array_filter($nodes, function (Node $node) {
 			return $this->getRelativePath($node->getPath());
 		});
+		usort($folders, function ($a, $b) {
+			return $b->getPath() <=> $a->getPath();
+		});
+		return $folders;
 	}
 
 	protected function getAppDataDirectoryName(): string {
diff --git a/tests/lib/Files/Node/FolderTest.php b/tests/lib/Files/Node/FolderTest.php
index 72bf70a68b7..94bcac4aa00 100644
--- a/tests/lib/Files/Node/FolderTest.php
+++ b/tests/lib/Files/Node/FolderTest.php
@@ -99,8 +99,7 @@ class FolderTest extends NodeTest {
 			->method('getUser')
 			->willReturn($this->user);
 
-		$root->expects($this->once())
-			->method('get')
+		$root->method('get')
 			->with('/bar/foo/asd');
 
 		$node = new Folder($root, $view, '/bar/foo');
@@ -122,8 +121,7 @@ class FolderTest extends NodeTest {
 
 		$child = new Folder($root, $view, '/bar/foo/asd');
 
-		$root->expects($this->once())
-			->method('get')
+		$root->method('get')
 			->with('/bar/foo/asd')
 			->willReturn($child);
 
@@ -144,8 +142,7 @@ class FolderTest extends NodeTest {
 			->method('getUser')
 			->willReturn($this->user);
 
-		$root->expects($this->once())
-			->method('get')
+		$root->method('get')
 			->with('/bar/foo/asd')
 			->will($this->throwException(new NotFoundException()));
 
@@ -166,13 +163,11 @@ class FolderTest extends NodeTest {
 			->method('getUser')
 			->willReturn($this->user);
 
-		$view->expects($this->once())
-			->method('getFileInfo')
+		$view->method('getFileInfo')
 			->with('/bar/foo')
 			->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL]));
 
-		$view->expects($this->once())
-			->method('mkdir')
+		$view->method('mkdir')
 			->with('/bar/foo/asd')
 			->willReturn(true);
 
@@ -194,12 +189,10 @@ class FolderTest extends NodeTest {
 		$root = $this->getMockBuilder(Root::class)
 			->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
 			->getMock();
-		$root->expects($this->any())
-			->method('getUser')
+		$root->method('getUser')
 			->willReturn($this->user);
 
-		$view->expects($this->once())
-			->method('getFileInfo')
+		$view->method('getFileInfo')
 			->with('/bar/foo')
 			->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_READ]));
 
@@ -220,13 +213,11 @@ class FolderTest extends NodeTest {
 			->method('getUser')
 			->willReturn($this->user);
 
-		$view->expects($this->once())
-			->method('getFileInfo')
+		$view->method('getFileInfo')
 			->with('/bar/foo')
 			->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL]));
 
-		$view->expects($this->once())
-			->method('touch')
+		$view->method('touch')
 			->with('/bar/foo/asd')
 			->willReturn(true);
 
@@ -248,12 +239,10 @@ class FolderTest extends NodeTest {
 		$root = $this->getMockBuilder(Root::class)
 			->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
 			->getMock();
-		$root->expects($this->any())
-			->method('getUser')
+		$root->method('getUser')
 			->willReturn($this->user);
 
-		$view->expects($this->once())
-			->method('getFileInfo')
+		$view->method('getFileInfo')
 			->with('/bar/foo')
 			->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_READ]));
 
@@ -270,12 +259,10 @@ class FolderTest extends NodeTest {
 		$root = $this->getMockBuilder(Root::class)
 			->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
 			->getMock();
-		$root->expects($this->any())
-			->method('getUser')
+		$root->method('getUser')
 			->willReturn($this->user);
 
-		$view->expects($this->once())
-			->method('free_space')
+		$view->method('free_space')
 			->with('/bar/foo')
 			->willReturn(100);
 
@@ -501,8 +488,7 @@ class FolderTest extends NodeTest {
 
 		$fileInfo = new CacheEntry(['path' => 'foo/qwerty', 'mimetype' => 'text/plain'], null);
 
-		$storage->expects($this->once())
-			->method('getCache')
+		$storage->method('getCache')
 			->willReturn($cache);
 
 		$this->userMountCache->expects($this->any())
@@ -517,18 +503,15 @@ class FolderTest extends NodeTest {
 				''
 			)]);
 
-		$cache->expects($this->once())
-			->method('get')
+		$cache->method('get')
 			->with(1)
 			->willReturn($fileInfo);
 
-		$root->expects($this->once())
-			->method('getMountsIn')
+		$root->method('getMountsIn')
 			->with('/bar/foo')
 			->willReturn([]);
 
-		$root->expects($this->once())
-			->method('getMount')
+		$root->method('getMount')
 			->with('/bar/foo')
 			->willReturn($mount);
 
@@ -555,8 +538,7 @@ class FolderTest extends NodeTest {
 
 		$fileInfo = new CacheEntry(['path' => '', 'mimetype' => 'text/plain'], null);
 
-		$storage->expects($this->once())
-			->method('getCache')
+		$storage->method('getCache')
 			->willReturn($cache);
 
 		$this->userMountCache->expects($this->any())
@@ -571,13 +553,11 @@ class FolderTest extends NodeTest {
 				''
 			)]);
 
-		$cache->expects($this->once())
-			->method('get')
+		$cache->method('get')
 			->with(1)
 			->willReturn($fileInfo);
 
-		$root->expects($this->once())
-			->method('getMount')
+		$root->method('getMount')
 			->with('/bar')
 			->willReturn($mount);
 
@@ -604,8 +584,7 @@ class FolderTest extends NodeTest {
 
 		$fileInfo = new CacheEntry(['path' => 'foobar', 'mimetype' => 'text/plain'], null);
 
-		$storage->expects($this->once())
-			->method('getCache')
+		$storage->method('getCache')
 			->willReturn($cache);
 
 		$this->userMountCache->expects($this->any())
@@ -620,18 +599,15 @@ class FolderTest extends NodeTest {
 				''
 			)]);
 
-		$cache->expects($this->once())
-			->method('get')
+		$cache->method('get')
 			->with(1)
 			->willReturn($fileInfo);
 
-		$root->expects($this->once())
-			->method('getMountsIn')
+		$root->method('getMountsIn')
 			->with('/bar/foo')
 			->willReturn([]);
 
-		$root->expects($this->once())
-			->method('getMount')
+		$root->method('getMount')
 			->with('/bar/foo')
 			->willReturn($mount);
 
@@ -658,12 +634,10 @@ class FolderTest extends NodeTest {
 
 		$fileInfo = new CacheEntry(['path' => 'foo/qwerty', 'mimetype' => 'text/plain'], null);
 
-		$storage->expects($this->exactly(2))
-			->method('getCache')
+		$storage->method('getCache')
 			->willReturn($cache);
 
-		$this->userMountCache->expects($this->any())
-			->method('getMountsForFileId')
+		$this->userMountCache->method('getMountsForFileId')
 			->with(1)
 			->willReturn([
 				new CachedMountInfo(
@@ -674,32 +648,20 @@ class FolderTest extends NodeTest {
 					1,
 					''
 				),
-				new CachedMountInfo(
-					$this->user,
-					1,
-					0,
-					'/bar/foo/asd/',
-					1,
-					''
-				),
 			]);
 
-		$storage->expects($this->any())
-			->method('getCache')
+		$storage->method('getCache')
 			->willReturn($cache);
 
-		$cache->expects($this->any())
-			->method('get')
+		$cache->method('get')
 			->with(1)
 			->willReturn($fileInfo);
 
-		$root->expects($this->any())
-			->method('getMountsIn')
+		$root->method('getMountsIn')
 			->with('/bar/foo')
 			->willReturn([$mount2]);
 
-		$root->expects($this->once())
-			->method('getMount')
+		$root->method('getMount')
 			->with('/bar/foo')
 			->willReturn($mount1);