Skip to content

Commit 0b44a82

Browse files
juliusknorrjuliusknorr
authored
Merge pull request #28359 from nextcloud/backport/28220/stable22
[stable22] fix Folder->getById() when a single storage is mounted multiple times
2 parents 02d3de6 + 8a6e3e6 commit 0b44a82

File tree

2 files changed

+59
-83
lines changed

2 files changed

+59
-83
lines changed

‎lib/private/Files/Node/Folder.php‎

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
use OC\Files\Search\SearchOrder;
3838
use OC\Files\Search\SearchQuery;
3939
use OCP\Files\Cache\ICacheEntry;
40-
use OCP\Files\Config\ICachedMountInfo;
4140
use OCP\Files\FileInfo;
4241
use OCP\Files\Mount\IMountPoint;
4342
use OCP\Files\NotFoundException;
@@ -350,17 +349,28 @@ public function getById($id) {
350349
$user = null;
351350
}
352351
$mountsContainingFile = $mountCache->getMountsForFileId((int)$id, $user);
352+
353+
// when a user has access trough the same storage trough multiple paths
354+
// (such as an external storage that is both mounted for a user and shared to the user)
355+
// the mount cache will only hold a single entry for the storage
356+
// this can lead to issues as the different ways the user has access to a storage can have different permissions
357+
//
358+
// so instead of using the cached entries directly, we instead filter the current mounts by the rootid of the cache entry
359+
360+
$mountRootIds = array_map(function ($mount) {
361+
return $mount->getRootId();
362+
}, $mountsContainingFile);
363+
$mountRootPaths = array_map(function ($mount) {
364+
return $mount->getRootInternalPath();
365+
}, $mountsContainingFile);
366+
$mountRoots = array_combine($mountRootIds, $mountRootPaths);
367+
353368
$mounts = $this->root->getMountsIn($this->path);
354369
$mounts[] = $this->root->getMount($this->path);
355-
/** @var IMountPoint[] $folderMounts */
356-
$folderMounts = array_combine(array_map(function (IMountPoint $mountPoint) {
357-
return $mountPoint->getMountPoint();
358-
}, $mounts), $mounts);
359370

360-
/** @var ICachedMountInfo[] $mountsContainingFile */
361-
$mountsContainingFile = array_values(array_filter($mountsContainingFile, function (ICachedMountInfo $cachedMountInfo) use ($folderMounts) {
362-
return isset($folderMounts[$cachedMountInfo->getMountPoint()]);
363-
}));
371+
$mountsContainingFile = array_filter($mounts, function ($mount) use ($mountRoots) {
372+
return isset($mountRoots[$mount->getStorageRootId()]);
373+
});
364374

365375
if (count($mountsContainingFile) === 0) {
366376
if ($user === $this->getAppDataDirectoryName()) {
@@ -369,18 +379,18 @@ public function getById($id) {
369379
return [];
370380
}
371381

372-
$nodes = array_map(function (ICachedMountInfo $cachedMountInfo) use ($folderMounts, $id) {
373-
$mount = $folderMounts[$cachedMountInfo->getMountPoint()];
382+
$nodes = array_map(function (IMountPoint $mount) use ($id, $mountRoots) {
383+
$rootInternalPath = $mountRoots[$mount->getStorageRootId()];
374384
$cacheEntry = $mount->getStorage()->getCache()->get((int)$id);
375385
if (!$cacheEntry) {
376386
return null;
377387
}
378388

379389
// cache jails will hide the "true" internal path
380-
$internalPath = ltrim($cachedMountInfo->getRootInternalPath() . '/' . $cacheEntry->getPath(), '/');
381-
$pathRelativeToMount = substr($internalPath, strlen($cachedMountInfo->getRootInternalPath()));
390+
$internalPath = ltrim($rootInternalPath . '/' . $cacheEntry->getPath(), '/');
391+
$pathRelativeToMount = substr($internalPath, strlen($rootInternalPath));
382392
$pathRelativeToMount = ltrim($pathRelativeToMount, '/');
383-
$absolutePath = rtrim($cachedMountInfo->getMountPoint() . $pathRelativeToMount, '/');
393+
$absolutePath = rtrim($mount->getMountPoint() . $pathRelativeToMount, '/');
384394
return $this->root->createNode($absolutePath, new \OC\Files\FileInfo(
385395
$absolutePath, $mount->getStorage(), $cacheEntry->getPath(), $cacheEntry, $mount,
386396
\OC::$server->getUserManager()->get($mount->getStorage()->getOwner($pathRelativeToMount))
@@ -389,9 +399,13 @@ public function getById($id) {
389399

390400
$nodes = array_filter($nodes);
391401

392-
return array_filter($nodes, function (Node $node) {
402+
$folders = array_filter($nodes, function (Node $node) {
393403
return $this->getRelativePath($node->getPath());
394404
});
405+
usort($folders, function ($a, $b) {
406+
return $b->getPath() <=> $a->getPath();
407+
});
408+
return $folders;
395409
}
396410

397411
protected function getAppDataDirectoryName(): string {

‎tests/lib/Files/Node/FolderTest.php‎

Lines changed: 30 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,7 @@ public function testGet() {
9999
->method('getUser')
100100
->willReturn($this->user);
101101

102-
$root->expects($this->once())
103-
->method('get')
102+
$root->method('get')
104103
->with('/bar/foo/asd');
105104

106105
$node = new Folder($root, $view, '/bar/foo');
@@ -122,8 +121,7 @@ public function testNodeExists() {
122121

123122
$child = new Folder($root, $view, '/bar/foo/asd');
124123

125-
$root->expects($this->once())
126-
->method('get')
124+
$root->method('get')
127125
->with('/bar/foo/asd')
128126
->willReturn($child);
129127

@@ -144,8 +142,7 @@ public function testNodeExistsNotExists() {
144142
->method('getUser')
145143
->willReturn($this->user);
146144

147-
$root->expects($this->once())
148-
->method('get')
145+
$root->method('get')
149146
->with('/bar/foo/asd')
150147
->will($this->throwException(new NotFoundException()));
151148

@@ -166,13 +163,11 @@ public function testNewFolder() {
166163
->method('getUser')
167164
->willReturn($this->user);
168165

169-
$view->expects($this->once())
170-
->method('getFileInfo')
166+
$view->method('getFileInfo')
171167
->with('/bar/foo')
172168
->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL]));
173169

174-
$view->expects($this->once())
175-
->method('mkdir')
170+
$view->method('mkdir')
176171
->with('/bar/foo/asd')
177172
->willReturn(true);
178173

@@ -194,12 +189,10 @@ public function testNewFolderNotPermitted() {
194189
$root = $this->getMockBuilder(Root::class)
195190
->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
196191
->getMock();
197-
$root->expects($this->any())
198-
->method('getUser')
192+
$root->method('getUser')
199193
->willReturn($this->user);
200194

201-
$view->expects($this->once())
202-
->method('getFileInfo')
195+
$view->method('getFileInfo')
203196
->with('/bar/foo')
204197
->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_READ]));
205198

@@ -220,13 +213,11 @@ public function testNewFile() {
220213
->method('getUser')
221214
->willReturn($this->user);
222215

223-
$view->expects($this->once())
224-
->method('getFileInfo')
216+
$view->method('getFileInfo')
225217
->with('/bar/foo')
226218
->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL]));
227219

228-
$view->expects($this->once())
229-
->method('touch')
220+
$view->method('touch')
230221
->with('/bar/foo/asd')
231222
->willReturn(true);
232223

@@ -248,12 +239,10 @@ public function testNewFileNotPermitted() {
248239
$root = $this->getMockBuilder(Root::class)
249240
->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
250241
->getMock();
251-
$root->expects($this->any())
252-
->method('getUser')
242+
$root->method('getUser')
253243
->willReturn($this->user);
254244

255-
$view->expects($this->once())
256-
->method('getFileInfo')
245+
$view->method('getFileInfo')
257246
->with('/bar/foo')
258247
->willReturn($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_READ]));
259248

@@ -270,12 +259,10 @@ public function testGetFreeSpace() {
270259
$root = $this->getMockBuilder(Root::class)
271260
->setConstructorArgs([$manager, $view, $this->user, $this->userMountCache, $this->logger, $this->userManager])
272261
->getMock();
273-
$root->expects($this->any())
274-
->method('getUser')
262+
$root->method('getUser')
275263
->willReturn($this->user);
276264

277-
$view->expects($this->once())
278-
->method('free_space')
265+
$view->method('free_space')
279266
->with('/bar/foo')
280267
->willReturn(100);
281268

@@ -501,8 +488,7 @@ public function testGetById() {
501488

502489
$fileInfo = new CacheEntry(['path' => 'foo/qwerty', 'mimetype' => 'text/plain'], null);
503490

504-
$storage->expects($this->once())
505-
->method('getCache')
491+
$storage->method('getCache')
506492
->willReturn($cache);
507493

508494
$this->userMountCache->expects($this->any())
@@ -517,18 +503,15 @@ public function testGetById() {
517503
''
518504
)]);
519505

520-
$cache->expects($this->once())
521-
->method('get')
506+
$cache->method('get')
522507
->with(1)
523508
->willReturn($fileInfo);
524509

525-
$root->expects($this->once())
526-
->method('getMountsIn')
510+
$root->method('getMountsIn')
527511
->with('/bar/foo')
528512
->willReturn([]);
529513

530-
$root->expects($this->once())
531-
->method('getMount')
514+
$root->method('getMount')
532515
->with('/bar/foo')
533516
->willReturn($mount);
534517

@@ -555,8 +538,7 @@ public function testGetByIdMountRoot() {
555538

556539
$fileInfo = new CacheEntry(['path' => '', 'mimetype' => 'text/plain'], null);
557540

558-
$storage->expects($this->once())
559-
->method('getCache')
541+
$storage->method('getCache')
560542
->willReturn($cache);
561543

562544
$this->userMountCache->expects($this->any())
@@ -571,13 +553,11 @@ public function testGetByIdMountRoot() {
571553
''
572554
)]);
573555

574-
$cache->expects($this->once())
575-
->method('get')
556+
$cache->method('get')
576557
->with(1)
577558
->willReturn($fileInfo);
578559

579-
$root->expects($this->once())
580-
->method('getMount')
560+
$root->method('getMount')
581561
->with('/bar')
582562
->willReturn($mount);
583563

@@ -604,8 +584,7 @@ public function testGetByIdOutsideFolder() {
604584

605585
$fileInfo = new CacheEntry(['path' => 'foobar', 'mimetype' => 'text/plain'], null);
606586

607-
$storage->expects($this->once())
608-
->method('getCache')
587+
$storage->method('getCache')
609588
->willReturn($cache);
610589

611590
$this->userMountCache->expects($this->any())
@@ -620,18 +599,15 @@ public function testGetByIdOutsideFolder() {
620599
''
621600
)]);
622601

623-
$cache->expects($this->once())
624-
->method('get')
602+
$cache->method('get')
625603
->with(1)
626604
->willReturn($fileInfo);
627605

628-
$root->expects($this->once())
629-
->method('getMountsIn')
606+
$root->method('getMountsIn')
630607
->with('/bar/foo')
631608
->willReturn([]);
632609

633-
$root->expects($this->once())
634-
->method('getMount')
610+
$root->method('getMount')
635611
->with('/bar/foo')
636612
->willReturn($mount);
637613

@@ -658,12 +634,10 @@ public function testGetByIdMultipleStorages() {
658634

659635
$fileInfo = new CacheEntry(['path' => 'foo/qwerty', 'mimetype' => 'text/plain'], null);
660636

661-
$storage->expects($this->exactly(2))
662-
->method('getCache')
637+
$storage->method('getCache')
663638
->willReturn($cache);
664639

665-
$this->userMountCache->expects($this->any())
666-
->method('getMountsForFileId')
640+
$this->userMountCache->method('getMountsForFileId')
667641
->with(1)
668642
->willReturn([
669643
new CachedMountInfo(
@@ -674,32 +648,20 @@ public function testGetByIdMultipleStorages() {
674648
1,
675649
''
676650
),
677-
new CachedMountInfo(
678-
$this->user,
679-
1,
680-
0,
681-
'/bar/foo/asd/',
682-
1,
683-
''
684-
),
685651
]);
686652

687-
$storage->expects($this->any())
688-
->method('getCache')
653+
$storage->method('getCache')
689654
->willReturn($cache);
690655

691-
$cache->expects($this->any())
692-
->method('get')
656+
$cache->method('get')
693657
->with(1)
694658
->willReturn($fileInfo);
695659

696-
$root->expects($this->any())
697-
->method('getMountsIn')
660+
$root->method('getMountsIn')
698661
->with('/bar/foo')
699662
->willReturn([$mount2]);
700663

701-
$root->expects($this->once())
702-
->method('getMount')
664+
$root->method('getMount')
703665
->with('/bar/foo')
704666
->willReturn($mount1);
705667

0 commit comments

Comments
 (0)