[stable32] filter ACL forbidden paths from search query

lib/ACL/ACLCacheWrapper.php

@@ -9,9 +9,14 @@
 namespace OCA\GroupFolders\ACL;
 
 use OC\Files\Cache\Wrapper\CacheWrapper;
+use OC\Files\Search\SearchBinaryOperator;
+use OC\Files\Search\SearchComparison;
 use OCP\Constants;
 use OCP\Files\Cache\ICache;
 use OCP\Files\Cache\ICacheEntry;
+use OCP\Files\Search\ISearchBinaryOperator;
+use OCP\Files\Search\ISearchComparison;
+use OCP\Files\Search\ISearchOperator;
 use OCP\Files\Search\ISearchQuery;
 
 class ACLCacheWrapper extends CacheWrapper {
@@ -62,21 +67,21 @@ public function getFolderContentsById($fileId): array {
 	}
 
 	public function search($pattern): array {
-		$results = $this->getCache()->search($pattern);
+		$results = parent::search($pattern);
 		$this->preloadEntries($results);
 
 		return array_filter(array_map($this->formatCacheEntry(...), $results));
 	}
 
 	public function searchByMime($mimetype): array {
-		$results = $this->getCache()->searchByMime($mimetype);
+		$results = parent::searchByMime($mimetype);
 		$this->preloadEntries($results);
 
 		return array_filter(array_map($this->formatCacheEntry(...), $results));
 	}
 
 	public function searchQuery(ISearchQuery $query): array {
-		$results = $this->getCache()->searchQuery($query);
+		$results = parent::searchQuery($query);
 		$this->preloadEntries($results);
 
 		return array_filter(array_map($this->formatCacheEntry(...), $results));
@@ -91,4 +96,35 @@ private function preloadEntries(array $entries): array {
 
 		return $this->aclManager->getRelevantRulesForPath($this->getNumericStorageId(), $paths, false);
 	}
+
+	/**
+	 * Construct a search operator that filters out any paths that the current user doesn't have read permissions for
+	 */
+	private function getSearchFilter(): ISearchOperator {
+		$forbiddenPaths = $this->aclManager->getForbiddenPaths($this->getNumericStorageId(), '');
+		if (count($forbiddenPaths) === 0) {
+			return new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, []);
+		}
+
+		$filters = array_map(fn (string $path) => new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [
+			new SearchComparison(ISearchComparison::COMPARE_LIKE_CASE_SENSITIVE, 'path', SearchComparison::escapeLikeParameter($path) . '/%')
+		]), $forbiddenPaths);
+		$filters[] = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [
+			new SearchComparison(ISearchComparison::COMPARE_IN, 'path', $forbiddenPaths)
+		]);
+		return new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, $filters);
+	}
+
+	#[\Override]
+	public function getQueryFilterForStorage(): ISearchOperator {
+		$storageFilter = parent::getQueryFilterForStorage();
+
+		return new SearchBinaryOperator(
+			ISearchBinaryOperator::OPERATOR_AND,
+			[
+				$storageFilter,
+				$this->getSearchFilter(),
+			]
+		);
+	}
 }

lib/ACL/ACLManager.php

@@ -276,4 +276,23 @@ private function filterApplicableRulesToUser(array $rules): array {
 			return false;
 		}));
 	}
+
+	/**
+	 * Calculate all paths that the current user doesn't have access to.
+	 *
+	 * @return list<string>
+	 */
+	public function getForbiddenPaths(int $storageId, string $prefix): array {
+		$rules = $this->ruleManager->getRulesForPrefix($this->user, $storageId, $prefix);
+		$forbidden = [];
+
+		foreach ($rules as $path => $pathRules) {
+			$mergedRule = Rule::mergeRules($pathRules);
+			if ($mergedRule->applyPermissions(Constants::PERMISSION_READ) === 0) {
+				$forbidden[] = $path;
+			}
+		}
+
+		return $forbidden;
+	}
 }

lib/ACL/RuleManager.php

@@ -306,16 +306,19 @@ public function getRulesForPrefix(IUser $user, int $storageId, string $prefix):
 		$query->select(['f.fileid', 'mapping_type', 'mapping_id', 'mask', 'a.permissions', 'f.path'])
 			->from('group_folders_acl', 'a')
 			->innerJoin('a', 'filecache', 'f', $query->expr()->eq('f.fileid', 'a.fileid'))
-			->where($query->expr()->orX(
-				$query->expr()->like('f.path', $query->createNamedParameter($this->connection->escapeLikeParameter($prefix) . '/%')),
-				$query->expr()->eq('f.path_hash', $query->createNamedParameter(md5($prefix)))
-			))
-			->andWhere($query->expr()->eq('f.storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
+			->where($query->expr()->eq('f.storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->orX(...array_map(fn (IUserMapping $userMapping): ICompositeExpression => $query->expr()->andX(
 				$query->expr()->eq('mapping_type', $query->createNamedParameter($userMapping->getType())),
 				$query->expr()->eq('mapping_id', $query->createNamedParameter($userMapping->getId()))
 			), $userMappings)));
 
+		if ($prefix !== '') {
+			$query = $query->andWhere($query->expr()->orX(
+				$query->expr()->like('f.path', $query->createNamedParameter($this->connection->escapeLikeParameter($prefix) . '/%')),
+				$query->expr()->eq('f.path_hash', $query->createNamedParameter(md5($prefix)))
+			));
+		}
+
 		$rows = $query->executeQuery()->fetchAll();
 
 		return $this->rulesByPath($rows);

psalm.xml

@@ -41,6 +41,10 @@
 		<file name="tests/stubs/oc_files_objectstore_objectstorescanner.php" />
 		<file name="tests/stubs/oc_files_objectstore_objectstorestorage.php" />
 		<file name="tests/stubs/oc_files_objectstore_primaryobjectstoreconfig.php" />
+		<file name="tests/stubs/oc_files_search_searchbinaryoperator.php" />
+		<file name="tests/stubs/oc_files_search_searchcomparison.php" />
+		<file name="tests/stubs/oc_files_search_searchorder.php" />
+		<file name="tests/stubs/oc_files_search_searchquery.php" />
 		<file name="tests/stubs/oc_files_setupmanager.php" />
 		<file name="tests/stubs/oc_files_storage_common.php" />
 		<file name="tests/stubs/oc_files_storage_local.php" />

tests/ACL/ACLCacheWrapperTest.php

@@ -9,28 +9,77 @@
 namespace OCA\groupfolders\tests\ACL;
 
 use OC\Files\Cache\CacheEntry;
+use OC\Files\Search\SearchComparison;
+use OC\Files\Search\SearchOrder;
+use OC\Files\Search\SearchQuery;
+use OC\Files\Storage\Temporary;
 use OCA\GroupFolders\ACL\ACLCacheWrapper;
 use OCA\GroupFolders\ACL\ACLManager;
+use OCA\GroupFolders\ACL\Rule;
+use OCA\GroupFolders\ACL\RuleManager;
+use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
+use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
+use OCA\GroupFolders\Trash\TrashManager;
 use OCP\Constants;
 use OCP\Files\Cache\ICache;
+use OCP\Files\Cache\ICacheEntry;
+use OCP\Files\Search\ISearchComparison;
+use OCP\Files\Search\ISearchOrder;
+use OCP\IUser;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /**
  * @group DB
  */
 class ACLCacheWrapperTest extends TestCase {
-	private ACLManager&MockObject $aclManager;
+	private ACLManager $aclManager;
 	private ICache&MockObject $source;
 	private ACLCacheWrapper $cache;
 	private array $aclPermissions = [];
 
+	private function makeRule(int $permission): Rule {
+		return new Rule($this->createMock(IUserMapping::class), 0, Constants::PERMISSION_ALL, $permission);
+	}
+
+	/**
+	 * @param string[] $paths
+	 * @return array<string, Rule[]>
+	 */
+	private function getRulesForPaths(IUser $user, int $storageId, array $paths): array {
+		return array_combine($paths, array_map(
+			fn (string $path) => isset($this->aclPermissions[$path]) ? [$this->makeRule($this->aclPermissions[$path])] : [],
+			$paths
+		));
+	}
+
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->aclManager = $this->createMock(ACLManager::class);
-		$this->aclManager->method('getACLPermissionsForPath')
-			->willReturnCallback(fn (int $_storageId, string $path) => $this->aclPermissions[$path] ?? Constants::PERMISSION_ALL);
+		$user = $this->createMock(IUser::class);
+		$ruleManager = $this->createMock(RuleManager::class);
+		$ruleManager->method('getRulesForFilesByPath')
+			->willReturnCallback($this->getRulesForPaths(...));
+		$ruleManager->method('getRulesForPrefix')
+			->willReturnCallback(fn (IUser $user, int $storageId, string $prefix) => array_map(
+				fn (int $permission) => [$this->makeRule($permission)],
+				array_filter(
+					$this->aclPermissions,
+					fn (string $path) => str_starts_with($path, $prefix),
+					ARRAY_FILTER_USE_KEY,
+				))
+			);
+		$mappingManager = $this->createMock(IUserMappingManager::class);
+
+		$this->aclManager = new ACLManager(
+			$ruleManager,
+			$this->createMock(TrashManager::class),
+			$mappingManager,
+			$this->createMock(LoggerInterface::class),
+			$user
+		);
+
 		$this->source = $this->createMock(ICache::class);
 		$this->source->method('getNumericStorageId')
 			->willReturn(1);
@@ -74,4 +123,43 @@ public function testHideNonRead(): void {
 
 		$this->assertEquals($expected, $result);
 	}
+
+	public function testSearchNonRead(): void {
+		$sourceStorage = new Temporary([]);
+		$sourceStorage->mkdir('foo');
+		$sourceStorage->touch('foo/test1.txt', 100);
+		$sourceStorage->touch('foo/test2.txt', 101);
+		$sourceStorage->touch('foo/test3.txt', 102);
+		$sourceStorage->mkdir('bar');
+		$sourceStorage->touch('bar/test1.txt', 200);
+		$sourceStorage->touch('bar/test2.txt', 201);
+		$sourceStorage->touch('bar/test3.txt', 202);
+		$sourceStorage->getScanner()->scan('');
+
+		$cache = new ACLCacheWrapper($sourceStorage->getCache(), $this->aclManager, false);
+
+		$query = new SearchQuery(
+			new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%test%'),
+			2,
+			0,
+			[new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'mtime')]
+		);
+		$result = $cache->searchQuery($query);
+		$paths = array_map(fn (ICacheEntry $entry) => $entry->getPath(), $result);
+
+		$this->assertEquals([
+			'bar/test3.txt',
+			'bar/test2.txt'
+		], $paths);
+
+		$this->aclPermissions['bar'] = 0;
+
+		$result = $cache->searchQuery($query);
+		$paths = array_map(fn (ICacheEntry $entry) => $entry->getPath(), $result);
+
+		$this->assertEquals([
+			'foo/test3.txt',
+			'foo/test2.txt'
+		], $paths);
+	}
 }

tests/psalm-baseline.xml

@@ -1,9 +1,2 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="6.5.0@38fc8444edf0cebc9205296ee6e30e906ade783b">
-  <file src="lib/ACL/ACLCacheWrapper.php">
-    <DeprecatedMethod>
-      <code><![CDATA[search]]></code>
-      <code><![CDATA[searchByMime]]></code>
-    </DeprecatedMethod>
-  </file>
-</files>
+<files psalm-version="6.5.0@38fc8444edf0cebc9205296ee6e30e906ade783b"/>

tests/stubs/oc_files_search_searchbinaryoperator.php

@@ -0,0 +1,65 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OC\Files\Search;
+
+use OCP\Files\Search\ISearchBinaryOperator;
+use OCP\Files\Search\ISearchOperator;
+
+class SearchBinaryOperator implements ISearchBinaryOperator {
+	/**
+	 * SearchBinaryOperator constructor.
+	 *
+	 * @param string $type
+	 * @param ISearchOperator[] $arguments
+	 */
+	public function __construct(
+		private $type,
+		private array $arguments,
+	) {
+	}
+
+	/**
+	 * @return string
+	 */
+	#[\Override]
+    public function getType()
+    {
+    }
+
+	/**
+	 * @return ISearchOperator[]
+	 */
+	#[\Override]
+    public function getArguments()
+    {
+    }
+
+	/**
+	 * @param ISearchOperator[] $arguments
+	 * @return void
+	 */
+	public function setArguments(array $arguments): void
+    {
+    }
+
+	#[\Override]
+    public function getQueryHint(string $name, $default)
+    {
+    }
+
+	#[\Override]
+    public function setQueryHint(string $name, $value): void
+    {
+    }
+
+	public function __toString(): string
+    {
+    }
+}