Skip to content

Commit e1ff63e

Browse files
nickvergessennickvergessen
authored
Merge pull request #812 from nextcloud/bugfix/439/rebased
pref(storage): Optimize getNode() by using existing mount point info
2 parents 77a4656 + 48358da commit e1ff63e

File tree

6 files changed

+62
-65
lines changed

6 files changed

+62
-65
lines changed

‎lib/CacheWrapper.php‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,24 @@
99
namespace OCA\FilesAccessControl;
1010

1111
use OC\Files\Cache\Wrapper\CacheWrapper as Wrapper;
12+
use OC\Files\Storage\Wrapper\Jail;
1213
use OCP\Constants;
1314
use OCP\Files\Cache\ICache;
1415
use OCP\Files\ForbiddenException;
16+
use OCP\Files\Mount\IMountPoint;
1517
use OCP\Files\Storage\IStorage;
1618

1719
class CacheWrapper extends Wrapper {
1820
protected readonly int $mask;
21+
protected readonly ?IStorage $storage;
1922

2023
public function __construct(
2124
ICache $cache,
22-
protected readonly IStorage $storage,
25+
protected readonly IMountPoint $mountPoint,
2326
protected readonly Operation $operation,
2427
) {
2528
parent::__construct($cache);
29+
$this->storage = $mountPoint->getStorage();
2630
$this->mask = Constants::PERMISSION_ALL
2731
& ~Constants::PERMISSION_READ
2832
& ~Constants::PERMISSION_CREATE
@@ -34,7 +38,14 @@ public function __construct(
3438
protected function formatCacheEntry($entry) {
3539
if (isset($entry['path']) && isset($entry['permissions'])) {
3640
try {
37-
$this->operation->checkFileAccess($this->storage, $entry['path'], $entry['mimetype'] === 'httpd/unix-directory', $entry);
41+
$storage = $this->storage;
42+
$path = $entry['path'];
43+
if ($storage?->instanceOfStorage(Jail::class)) {
44+
/** @var Jail $storage */
45+
$jailedPath = $storage->getJailedPath($path);
46+
$path = $jailedPath ?? $path;
47+
}
48+
$this->operation->checkFileAccess($path, $this->mountPoint, $entry['mimetype'] === 'httpd/unix-directory', $entry);
3849
} catch (ForbiddenException) {
3950
$entry['permissions'] &= $this->mask;
4051
}

‎lib/Operation.php‎

Lines changed: 23 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
use OCP\Files\Mount\IMountManager;
2121
use OCP\Files\Mount\IMountPoint;
2222
use OCP\Files\Node;
23-
use OCP\Files\NotFoundException;
2423
use OCP\Files\Storage\IStorage;
2524
use OCP\IL10N;
2625
use OCP\IURLGenerator;
@@ -29,7 +28,6 @@
2928
use OCP\WorkflowEngine\IRuleMatcher;
3029
use OCP\WorkflowEngine\ISpecificOperation;
3130
use Psr\Log\LoggerInterface;
32-
use ReflectionClass;
3331
use UnexpectedValueException;
3432

3533
class Operation implements IComplexOperation, ISpecificOperation {
@@ -50,20 +48,24 @@ public function __construct(
5048
* @param array|ICacheEntry|null $cacheEntry
5149
* @throws ForbiddenException
5250
*/
53-
public function checkFileAccess(IStorage $storage, string $path, bool $isDir = false, $cacheEntry = null): void {
54-
if (!$this->isBlockablePath($storage, $path) || $this->isCreatingSkeletonFiles() || $this->nestingLevel !== 0) {
51+
public function checkFileAccess(string $path, IMountPoint $mountPoint, bool $isDir, $cacheEntry = null): void {
52+
if (!$this->isBlockablePath($mountPoint, $path) || $this->isCreatingSkeletonFiles() || $this->nestingLevel !== 0) {
5553
// Allow creating skeletons and theming
5654
// https://github.com/nextcloud/files_accesscontrol/issues/5
5755
// https://github.com/nextcloud/files_accesscontrol/issues/12
5856
return;
5957
}
58+
$storage = $mountPoint->getStorage();
59+
if ($storage === null) {
60+
return;
61+
}
6062

6163
$this->nestingLevel++;
6264

6365
$filePath = $this->translatePath($storage, $path);
6466
$ruleMatcher = $this->manager->getRuleMatcher();
6567
$ruleMatcher->setFileInfo($storage, $filePath, $isDir);
66-
$node = $this->getNode($storage, $path, $cacheEntry);
68+
$node = $this->getNode($path, $mountPoint, $cacheEntry);
6769
if ($node !== null) {
6870
$ruleMatcher->setEntitySubject($this->fileEntity, $node);
6971
}
@@ -80,24 +82,8 @@ public function checkFileAccess(IStorage $storage, string $path, bool $isDir = f
8082
}
8183
}
8284

83-
protected function isBlockablePath(IStorage $storage, string $path): bool {
84-
if (property_exists($storage, 'mountPoint')) {
85-
$hasMountPoint = $storage instanceof StorageWrapper;
86-
if (!$hasMountPoint) {
87-
$ref = new ReflectionClass($storage);
88-
$prop = $ref->getProperty('mountPoint');
89-
$hasMountPoint = $prop->isPublic();
90-
}
91-
92-
if ($hasMountPoint) {
93-
/** @var StorageWrapper $storage */
94-
$fullPath = $storage->mountPoint . ltrim($path, '/');
95-
} else {
96-
$fullPath = $path;
97-
}
98-
} else {
99-
$fullPath = $path;
100-
}
85+
protected function isBlockablePath(IMountPoint $mountPoint, string $path): bool {
86+
$fullPath = $mountPoint->getMountPoint() . ltrim($path, '/');
10187

10288
if (substr_count($fullPath, '/') < 3) {
10389
return false;
@@ -297,40 +283,22 @@ public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatch
297283
// Noop
298284
}
299285

300-
/**
301-
* @param array|ICacheEntry|null $cacheEntry
302-
*/
303-
private function getNode(IStorage $storage, string $path, $cacheEntry = null): ?Node {
304-
if ($storage->instanceOfStorage(StorageWrapper::class)) {
305-
/** @var StorageWrapper $storage */
306-
$mountPoint = $storage->getMount();
307-
} else {
308-
// fairly sure this branch is never taken, but not 100%
309-
310-
/** @var IMountPoint|false $mountPoint */
311-
$mountPoint = current($this->mountManager->findByStorageId($storage->getId()));
312-
if (!$mountPoint) {
313-
return null;
314-
}
286+
private function getNode(string $path, IMountPoint $mountPoint, ICacheEntry|array|null $cacheEntry = null): ?Node {
287+
$fullPath = $mountPoint->getMountPoint() . $path;
288+
if (!$cacheEntry) {
289+
$cacheEntry = $mountPoint->getStorage()?->getCache()->get($path);
290+
}
291+
if (!$cacheEntry) {
292+
return null;
315293
}
316294

317-
$fullPath = $mountPoint->getMountPoint() . $path;
318-
if ($cacheEntry) {
319-
// todo: LazyNode?
320-
$info = new FileInfo($fullPath, $mountPoint->getStorage(), $path, $cacheEntry, $mountPoint);
321-
$isDir = $info->getType() === \OCP\Files\FileInfo::TYPE_FOLDER;
322-
$view = new View('');
323-
if ($isDir) {
324-
return new Folder($this->rootFolder, $view, $path, $info);
325-
} else {
326-
return new \OC\Files\Node\File($this->rootFolder, $view, $path, $info);
327-
}
328-
} else {
329-
try {
330-
return $this->rootFolder->get($fullPath);
331-
} catch (NotFoundException $e) {
332-
return null;
333-
}
295+
// todo: LazyNode?
296+
$info = new FileInfo($fullPath, $mountPoint->getStorage(), $path, $cacheEntry, $mountPoint);
297+
$isDir = $info->getType() === \OCP\Files\FileInfo::TYPE_FOLDER;
298+
$view = new View('');
299+
if ($isDir) {
300+
return new Folder($this->rootFolder, $view, $path, $info);
334301
}
302+
return new \OC\Files\Node\File($this->rootFolder, $view, $path, $info);
335303
}
336304
}

‎lib/StorageWrapper.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ class StorageWrapper extends Wrapper implements IWriteStreamStorage {
2121
protected readonly Operation $operation;
2222
public readonly string $mountPoint;
2323
protected readonly int $mask;
24-
private readonly IMountPoint $mount;
24+
protected readonly IMountPoint $mount;
2525

2626
/**
2727
* @param array $parameters
@@ -43,7 +43,7 @@ public function __construct($parameters) {
4343
* @throws ForbiddenException
4444
*/
4545
protected function checkFileAccess(string $path, ?bool $isDir = null): void {
46-
$this->operation->checkFileAccess($this, $path, is_bool($isDir) ? $isDir : $this->is_dir($path));
46+
$this->operation->checkFileAccess($path, $this->mount, is_bool($isDir) ? $isDir : $this->is_dir($path));
4747
}
4848

4949
/*
@@ -264,7 +264,7 @@ public function getCache($path = '', $storage = null): ICache {
264264
$storage = $this;
265265
}
266266
$cache = $this->storage->getCache($path, $storage);
267-
return new CacheWrapper($cache, $storage, $this->operation);
267+
return new CacheWrapper($cache, $this->mount, $this->operation);
268268
}
269269

270270
/**

‎psalm.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,6 @@
3030
<file name="tests/stubs/oc_hooks_emitter.php" />
3131
<file name="tests/stubs/oc_files_cache_wrapper_cachewrapper.php" />
3232
<file name="tests/stubs/oc_files_storage_wrapper_wrapper.php" />
33+
<file name="tests/stubs/oc_files_storage_wrapper_jail.php" />
3334
</stubs>
3435
</psalm>

‎tests/Unit/StorageWrapperTest.php‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,24 +20,30 @@ class StorageWrapperTest extends TestCase {
2020
protected IStorage&MockObject $storage;
2121
protected Operation&MockObject $operation;
2222

23+
/** @var IMountPoint|MockObject */
24+
protected $mountPoint;
25+
2326
protected function setUp(): void {
2427
parent::setUp();
2528

2629
$this->storage = $this->createMock(IStorage::class);
2730
$this->operation = $this->createMock(Operation::class);
31+
32+
$this->mountPoint = $this->createMock(IMountPoint::class);
33+
$this->mountPoint->method('getMountPoint')
34+
->willReturn('mountPoint');
35+
$this->mountPoint->method('getStorage')
36+
->willReturn($this->storage);
2837
}
2938

3039
protected function getInstance(array $methods = []): StorageWrapper&MockObject {
31-
$mount = $this->createMock(IMountPoint::class);
32-
$mount->method('getMountPoint')
33-
->willReturn('mountPoint');
3440
return $this->getMockBuilder(StorageWrapper::class)
3541
->setConstructorArgs([
3642
[
3743
'storage' => $this->storage,
3844
'mountPoint' => 'mountPoint',
45+
'mount' => $this->mountPoint,
3946
'operation' => $this->operation,
40-
'mount' => $mount,
4147
]
4248
])
4349
->onlyMethods($methods)
@@ -59,7 +65,7 @@ public function testCheckFileAccess(string $path, bool $isDir): void {
5965

6066
$this->operation->expects($this->once())
6167
->method('checkFileAccess')
62-
->with($storage, $path);
68+
->with($path, $this->mountPoint, $isDir);
6369

6470
self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
6571
}

‎tests/stubs/oc_files_storage_wrapper_jail.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-only
6+
*/
7+
8+
namespace OC\Files\Storage\Wrapper {
9+
class Jail extends Wrapper {
10+
}
11+
}

0 commit comments

Comments
 (0)