fix(operation): Improve getNode logic

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

Author: icewind1991

lib/CacheWrapper.php

@@ -9,6 +9,7 @@
 namespace OCA\FilesAccessControl;
 
 use OC\Files\Cache\Wrapper\CacheWrapper as Wrapper;
+use OC\Files\Storage\Wrapper\Jail;
 use OCP\Constants;
 use OCP\Files\Cache\ICache;
 use OCP\Files\ForbiddenException;
@@ -17,14 +18,15 @@
 
 class CacheWrapper extends Wrapper {
 	protected readonly int $mask;
+	protected readonly IStorage $storage;
 
 	public function __construct(
 		ICache $cache,
-		protected readonly IStorage $storage,
 		protected readonly IMountPoint $mountPoint,
 		protected readonly Operation $operation,
 	) {
 		parent::__construct($cache);
+		$this->storage = $mountPoint->getStorage();
 		$this->mask = Constants::PERMISSION_ALL
 			& ~Constants::PERMISSION_READ
 			& ~Constants::PERMISSION_CREATE
@@ -36,7 +38,13 @@ public function __construct(
 	protected function formatCacheEntry($entry) {
 		if (isset($entry['path']) && isset($entry['permissions'])) {
 			try {
-				$this->operation->checkFileAccess($this->storage, $entry['path'], $this->mountPoint, $entry['mimetype'] === 'httpd/unix-directory', $entry);
+				$storage = $this->storage;
+				$path = $entry['path'];
+				if ($storage->instanceOfStorage(Jail::class)) {
+					/** @var Jail $storage */
+					$path = $storage->getJailedPath($path);
+				}
+				$this->operation->checkFileAccess($path, $this->mountPoint, $entry['mimetype'] === 'httpd/unix-directory', $entry);
 			} catch (ForbiddenException) {
 				$entry['permissions'] &= $this->mask;
 			}

lib/Operation.php

@@ -50,13 +50,14 @@ public function __construct(
 	 * @param array|ICacheEntry|null $cacheEntry
 	 * @throws ForbiddenException
 	 */
-	public function checkFileAccess(IStorage $storage, string $path, IMountPoint $mountPoint, bool $isDir, $cacheEntry = null): void {
-		if (!$this->isBlockablePath($storage, $path) || $this->isCreatingSkeletonFiles() || $this->nestingLevel !== 0) {
+	public function checkFileAccess(string $path, IMountPoint $mountPoint, bool $isDir, $cacheEntry = null): void {
+		if (!$this->isBlockablePath($mountPoint, $path) || $this->isCreatingSkeletonFiles() || $this->nestingLevel !== 0) {
 			// Allow creating skeletons and theming
 			// https://github.com/nextcloud/files_accesscontrol/issues/5
 			// https://github.com/nextcloud/files_accesscontrol/issues/12
 			return;
 		}
+		$storage = $mountPoint->getStorage();
 
 		$this->nestingLevel++;
 
@@ -80,24 +81,8 @@ public function checkFileAccess(IStorage $storage, string $path, IMountPoint $mo
 		}
 	}
 
-	protected function isBlockablePath(IStorage $storage, string $path): bool {
-		if (property_exists($storage, 'mountPoint')) {
-			$hasMountPoint = $storage instanceof StorageWrapper;
-			if (!$hasMountPoint) {
-				$ref = new ReflectionClass($storage);
-				$prop = $ref->getProperty('mountPoint');
-				$hasMountPoint = $prop->isPublic();
-			}
-
-			if ($hasMountPoint) {
-				/** @var StorageWrapper $storage */
-				$fullPath = $storage->mountPoint . ltrim($path, '/');
-			} else {
-				$fullPath = $path;
-			}
-		} else {
-			$fullPath = $path;
-		}
+	protected function isBlockablePath(IMountPoint $mountPoint, string $path): bool {
+		$fullPath = $mountPoint->getMountPoint() . ltrim($path, '/');
 
 		if (substr_count($fullPath, '/') < 3) {
 			return false;
@@ -299,22 +284,20 @@ public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatch
 
 	private function getNode(string $path, IMountPoint $mountPoint, ICacheEntry|array|null $cacheEntry = null): ?Node {
 		$fullPath = $mountPoint->getMountPoint() . $path;
-		if ($cacheEntry) {
-			// todo: LazyNode?
-			$info = new FileInfo($fullPath, $mountPoint->getStorage(), $path, $cacheEntry, $mountPoint);
-			$isDir = $info->getType() === \OCP\Files\FileInfo::TYPE_FOLDER;
-			$view = new View('');
-			if ($isDir) {
-				return new Folder($this->rootFolder, $view, $path, $info);
-			} else {
-				return new \OC\Files\Node\File($this->rootFolder, $view, $path, $info);
-			}
-		} else {
-			try {
-				return $this->rootFolder->get($fullPath);
-			} catch (NotFoundException) {
-				return null;
-			}
+		if (!$cacheEntry) {
+			$cacheEntry = $mountPoint->getStorage()->getCache()->get($path);
+		}
+		if (!$cacheEntry) {
+			return null;
+		}
+
+		// todo: LazyNode?
+		$info = new FileInfo($fullPath, $mountPoint->getStorage(), $path, $cacheEntry, $mountPoint);
+		$isDir = $info->getType() === FileInfo::TYPE_FOLDER;
+		$view = new View('');
+		if ($isDir) {
+			return new Folder($this->rootFolder, $view, $path, $info);
 		}
+		return new \OC\Files\Node\File($this->rootFolder, $view, $path, $info);
 	}
 }

lib/StorageWrapper.php

@@ -43,7 +43,7 @@ public function __construct($parameters) {
 	 * @throws ForbiddenException
 	 */
 	protected function checkFileAccess(string $path, ?bool $isDir = null): void {
-		$this->operation->checkFileAccess($this, $path, $this->mount, is_bool($isDir) ? $isDir : $this->is_dir($path));
+		$this->operation->checkFileAccess($path, $this->mount, is_bool($isDir) ? $isDir : $this->is_dir($path));
 	}
 
 	/*
@@ -264,7 +264,7 @@ public function getCache($path = '', $storage = null): ICache {
 			$storage = $this;
 		}
 		$cache = $this->storage->getCache($path, $storage);
-		return new CacheWrapper($cache, $storage, $this->operation, $this->mount);
+		return new CacheWrapper($cache, $this->operation, $this->mount);
 	}
 
 	/**

tests/Unit/StorageWrapperTest.php

@@ -20,24 +20,30 @@ class StorageWrapperTest extends TestCase {
 	protected IStorage&MockObject $storage;
 	protected Operation&MockObject $operation;
 
+	/** @var IMountPoint|MockObject */
+	protected $mountPoint;
+
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->storage = $this->createMock(IStorage::class);
 		$this->operation = $this->createMock(Operation::class);
+
+		$this->mountPoint = $this->createMock(IMountPoint::class);
+		$this->mountPoint->method('getMountPoint')
+			->willReturn('mountPoint');
+		$this->mountPoint->method('getStorage')
+			->willReturn($this->storage);
 	}
 
 	protected function getInstance(array $methods = []): StorageWrapper&MockObject {
-		$mount = $this->createMock(IMountPoint::class);
-		$mount->method('getMountPoint')
-			->willReturn('mountPoint');
 		return $this->getMockBuilder(StorageWrapper::class)
 			->setConstructorArgs([
 				[
 					'storage' => $this->storage,
 					'mountPoint' => 'mountPoint',
+					'mount' => $this->mountPoint,
 					'operation' => $this->operation,
-					'mount' => $mount,
 				]
 			])
 			->onlyMethods($methods)
@@ -59,7 +65,7 @@ public function testCheckFileAccess(string $path, bool $isDir): void {
 
 		$this->operation->expects($this->once())
 			->method('checkFileAccess')
-			->with($storage, $path, $this->createMock(IMountPoint::class), false);
+			->with($path, $this->mountPoint, $isDir);
 
 		self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
 	}