diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 05d3c867e..3c928e4c2 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,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(), + ] + ); + } } 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); 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/ACL/ACLCacheWrapperTest.php b/tests/ACL/ACLCacheWrapperTest.php index 40d54d4cd..c3efd4128 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,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); + } } 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 @@ - - - - - - - - + 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 @@ +