From 36fd2f8f6286e54e93e94ae8ef48e0f8cd029d1b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 4 Jun 2026 19:44:21 +0200 Subject: [PATCH 1/5] test: add test for searching with ACL filtered results Signed-off-by: Robin Appelman --- tests/ACL/ACLCacheWrapperTest.php | 87 +++++++++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 4 deletions(-) diff --git a/tests/ACL/ACLCacheWrapperTest.php b/tests/ACL/ACLCacheWrapperTest.php index 40d54d4cd..0b3bade0a 100644 --- a/tests/ACL/ACLCacheWrapperTest.php +++ b/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 + */ + 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,34 @@ 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(''); + $this->aclPermissions['bar'] = 0; + + $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([ + 'foo/test3.txt', + 'foo/test2.txt' + ], $paths); + } } From 0e90d1408c34e177c366c4caeff48223b3941dd9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 4 Jun 2026 21:23:27 +0200 Subject: [PATCH 2/5] fix: filter ACL forbidden paths from search query Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 39 ++++++++++++++++++++++++++++++++++--- lib/ACL/ACLManager.php | 19 ++++++++++++++++++ lib/ACL/RuleManager.php | 13 ++++++++----- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 05d3c867e..7da8e8583 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/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,32 @@ 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(), ''); + + $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(), + ] + ); + } } diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index acc1b671b..dbbfb3e76 100644 --- a/lib/ACL/ACLManager.php +++ b/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 + */ + 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; + } } diff --git a/lib/ACL/RuleManager.php b/lib/ACL/RuleManager.php index 90c7d7c6f..8b23e3312 100644 --- a/lib/ACL/RuleManager.php +++ b/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); From d4b94ccc25a2cddcce6c9aa25b2b259ab02e5452 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 19 Jun 2026 15:01:57 +0200 Subject: [PATCH 3/5] chore: update stubs Signed-off-by: Robin Appelman --- psalm.xml | 4 + .../oc_files_search_searchbinaryoperator.php | 65 +++++++++++++++++ .../oc_files_search_searchcomparison.php | 73 +++++++++++++++++++ tests/stubs/oc_files_search_searchorder.php | 52 +++++++++++++ tests/stubs/oc_files_search_searchquery.php | 63 ++++++++++++++++ 5 files changed, 257 insertions(+) create mode 100644 tests/stubs/oc_files_search_searchbinaryoperator.php create mode 100644 tests/stubs/oc_files_search_searchcomparison.php create mode 100644 tests/stubs/oc_files_search_searchorder.php create mode 100644 tests/stubs/oc_files_search_searchquery.php diff --git a/psalm.xml b/psalm.xml index 19b20bb46..a89402c09 100644 --- a/psalm.xml +++ b/psalm.xml @@ -41,6 +41,10 @@ + + + + diff --git a/tests/stubs/oc_files_search_searchbinaryoperator.php b/tests/stubs/oc_files_search_searchbinaryoperator.php new file mode 100644 index 000000000..b5013303f --- /dev/null +++ b/tests/stubs/oc_files_search_searchbinaryoperator.php @@ -0,0 +1,65 @@ + Date: Fri, 19 Jun 2026 14:48:38 +0200 Subject: [PATCH 4/5] fix: fix search results when no paths are forbidden Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 3 +++ tests/ACL/ACLCacheWrapperTest.php | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 7da8e8583..5942deb35 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/lib/ACL/ACLCacheWrapper.php @@ -102,6 +102,9 @@ private function preloadEntries(array $entries): array { */ private function getSearchFilter(): ISearchOperator { $forbiddenPaths = $this->aclManager->getForbiddenPaths($this->getNumericStorageId(), ''); + if (empty($forbiddenPaths)) { + 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) . '/%') diff --git a/tests/ACL/ACLCacheWrapperTest.php b/tests/ACL/ACLCacheWrapperTest.php index 0b3bade0a..c3efd4128 100644 --- a/tests/ACL/ACLCacheWrapperTest.php +++ b/tests/ACL/ACLCacheWrapperTest.php @@ -135,7 +135,6 @@ public function testSearchNonRead(): void { $sourceStorage->touch('bar/test2.txt', 201); $sourceStorage->touch('bar/test3.txt', 202); $sourceStorage->getScanner()->scan(''); - $this->aclPermissions['bar'] = 0; $cache = new ACLCacheWrapper($sourceStorage->getCache(), $this->aclManager, false); @@ -148,6 +147,16 @@ public function testSearchNonRead(): void { $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' From 4f33b9cb19d43cd0a9aba0453511bfeddcbe46fb Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 19 Jun 2026 15:15:32 +0200 Subject: [PATCH 5/5] chore: update baseline Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 2 +- tests/psalm-baseline.xml | 9 +-------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 5942deb35..3c928e4c2 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/lib/ACL/ACLCacheWrapper.php @@ -102,7 +102,7 @@ private function preloadEntries(array $entries): array { */ private function getSearchFilter(): ISearchOperator { $forbiddenPaths = $this->aclManager->getForbiddenPaths($this->getNumericStorageId(), ''); - if (empty($forbiddenPaths)) { + if (count($forbiddenPaths) === 0) { return new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, []); } diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 5d175c45b..69e54f6e6 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -1,9 +1,2 @@ - - - - - - - - +