Skip to content

Commit 092d80a

Browse files
icewind1991icewind1991
authored
Merge pull request #4755 from nextcloud/backport/4745/stable33
[stable33] filter ACL forbidden paths from search query
2 parents e3a2f8e + ab993e6 commit 092d80a

20 files changed

Lines changed: 1314 additions & 134 deletions

‎lib/ACL/ACLCacheWrapper.php‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,14 @@
99
namespace OCA\GroupFolders\ACL;
1010

1111
use OC\Files\Cache\Wrapper\CacheWrapper;
12+
use OC\Files\Search\SearchBinaryOperator;
13+
use OC\Files\Search\SearchComparison;
1214
use OCP\Constants;
1315
use OCP\Files\Cache\ICache;
1416
use OCP\Files\Cache\ICacheEntry;
17+
use OCP\Files\Search\ISearchBinaryOperator;
18+
use OCP\Files\Search\ISearchComparison;
19+
use OCP\Files\Search\ISearchOperator;
1520
use OCP\Files\Search\ISearchQuery;
1621

1722
class ACLCacheWrapper extends CacheWrapper {
@@ -66,23 +71,23 @@ public function getFolderContentsById($fileId): array {
6671

6772
#[\Override]
6873
public function search($pattern): array {
69-
$results = $this->getCache()->search($pattern);
74+
$results = parent::search($pattern);
7075
$this->preloadEntries($results);
7176

7277
return array_filter(array_map($this->formatCacheEntry(...), $results));
7378
}
7479

7580
#[\Override]
7681
public function searchByMime($mimetype): array {
77-
$results = $this->getCache()->searchByMime($mimetype);
82+
$results = parent::searchByMime($mimetype);
7883
$this->preloadEntries($results);
7984

8085
return array_filter(array_map($this->formatCacheEntry(...), $results));
8186
}
8287

8388
#[\Override]
8489
public function searchQuery(ISearchQuery $query): array {
85-
$results = $this->getCache()->searchQuery($query);
90+
$results = parent::searchQuery($query);
8691
$this->preloadEntries($results);
8792

8893
return array_filter(array_map($this->formatCacheEntry(...), $results));
@@ -97,4 +102,32 @@ private function preloadEntries(array $entries): array {
97102

98103
return $this->aclManager->getRelevantRulesForPath($this->getNumericStorageId(), $paths, false);
99104
}
105+
106+
/**
107+
* Construct a search operator that filters out any paths that the current user doesn't have read permissions for
108+
*/
109+
private function getSearchFilter(): ISearchOperator {
110+
$forbiddenPaths = $this->aclManager->getForbiddenPaths($this->getNumericStorageId(), '');
111+
112+
$filters = array_map(fn (string $path) => new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [
113+
new SearchComparison(ISearchComparison::COMPARE_LIKE_CASE_SENSITIVE, 'path', SearchComparison::escapeLikeParameter($path) . '/%')
114+
]), $forbiddenPaths);
115+
$filters[] = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [
116+
new SearchComparison(ISearchComparison::COMPARE_IN, 'path', $forbiddenPaths)
117+
]);
118+
return new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, $filters);
119+
}
120+
121+
#[\Override]
122+
public function getQueryFilterForStorage(): ISearchOperator {
123+
$storageFilter = parent::getQueryFilterForStorage();
124+
125+
return new SearchBinaryOperator(
126+
ISearchBinaryOperator::OPERATOR_AND,
127+
[
128+
$storageFilter,
129+
$this->getSearchFilter(),
130+
]
131+
);
132+
}
100133
}

‎lib/ACL/ACLManager.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,4 +293,23 @@ public function getBasePermission(int $folderId): int {
293293

294294
return Constants::PERMISSION_ALL;
295295
}
296+
297+
/**
298+
* Calculate all paths that the current user doesn't have access to.
299+
*
300+
* @return list<string>
301+
*/
302+
public function getForbiddenPaths(int $storageId, string $prefix): array {
303+
$rules = $this->ruleManager->getRulesForPrefix($this->user, $storageId, $prefix);
304+
$forbidden = [];
305+
306+
foreach ($rules as $path => $pathRules) {
307+
$mergedRule = Rule::mergeRules($pathRules);
308+
if ($mergedRule->applyPermissions(Constants::PERMISSION_READ) === 0) {
309+
$forbidden[] = $path;
310+
}
311+
}
312+
313+
return $forbidden;
314+
}
296315
}

‎lib/ACL/RuleManager.php‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -296,16 +296,19 @@ public function getRulesForPrefix(IUser $user, int $storageId, string $prefix):
296296
$query->select(['f.fileid', 'mapping_type', 'mapping_id', 'mask', 'a.permissions', 'f.path'])
297297
->from('group_folders_acl', 'a')
298298
->innerJoin('a', 'filecache', 'f', $query->expr()->eq('f.fileid', 'a.fileid'))
299-
->where($query->expr()->orX(
300-
$query->expr()->like('f.path', $query->createNamedParameter($this->connection->escapeLikeParameter($prefix) . '/%')),
301-
$query->expr()->eq('f.path_hash', $query->createNamedParameter(md5($prefix)))
302-
))
303-
->andWhere($query->expr()->eq('f.storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
299+
->where($query->expr()->eq('f.storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
304300
->andWhere($query->expr()->orX(...array_map(fn (IUserMapping $userMapping): ICompositeExpression => $query->expr()->andX(
305301
$query->expr()->eq('mapping_type', $query->createNamedParameter($userMapping->getType())),
306302
$query->expr()->eq('mapping_id', $query->createNamedParameter($userMapping->getId()))
307303
), $userMappings)));
308304

305+
if ($prefix !== '') {
306+
$query = $query->andWhere($query->expr()->orX(
307+
$query->expr()->like('f.path', $query->createNamedParameter($this->connection->escapeLikeParameter($prefix) . '/%')),
308+
$query->expr()->eq('f.path_hash', $query->createNamedParameter(md5($prefix)))
309+
));
310+
}
311+
309312
$rows = $query->executeQuery()->fetchAll();
310313

311314
return $this->rulesByPath($rows);

‎psalm.xml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,28 @@
2828
<file name="tests/stubs/oc_appframework_utility_simplecontainer.php" />
2929
<file name="tests/stubs/oc_core_command_base.php" />
3030
<file name="tests/stubs/oc_db_querybuilder_querybuilder.php" />
31+
<file name="tests/stubs/oc_db_querybuilder_typedquerybuilder.php" />
3132
<file name="tests/stubs/oc_db_querybuilder_sharded_sharddefinition.php" />
33+
<file name="tests/stubs/oc_encryption_exceptions_decryptionfailedexception.php" />
3234
<file name="tests/stubs/oc_files_cache_cache.php" />
3335
<file name="tests/stubs/oc_files_cache_cacheentry.php" />
3436
<file name="tests/stubs/oc_files_cache_scanner.php" />
3537
<file name="tests/stubs/oc_files_cache_wrapper_cachejail.php" />
3638
<file name="tests/stubs/oc_files_cache_wrapper_cachewrapper.php" />
3739
<file name="tests/stubs/oc_files_filesystem.php" />
40+
<file name="tests/stubs/oc_files_fileinfo.php" />
3841
<file name="tests/stubs/oc_files_mount_mountpoint.php" />
3942
<file name="tests/stubs/oc_files_node_lazyfolder.php" />
4043
<file name="tests/stubs/oc_files_node_node.php" />
4144
<file name="tests/stubs/oc_files_objectstore_objectstorescanner.php" />
4245
<file name="tests/stubs/oc_files_objectstore_objectstorestorage.php" />
4346
<file name="tests/stubs/oc_files_objectstore_primaryobjectstoreconfig.php" />
47+
<file name="tests/stubs/oc_files_search_searchbinaryoperator.php" />
48+
<file name="tests/stubs/oc_files_search_searchcomparison.php" />
49+
<file name="tests/stubs/oc_files_search_searchorder.php" />
50+
<file name="tests/stubs/oc_files_search_searchquery.php" />
51+
<file name="tests/stubs/oc_files_storage_wrapper_encryption.php" />
52+
<file name="tests/stubs/oc_settings_authorizedgroup.php" />
4453
<file name="tests/stubs/oc_files_setupmanager.php" />
4554
<file name="tests/stubs/oc_files_storage_common.php" />
4655
<file name="tests/stubs/oc_files_storage_local.php" />

‎tests/ACL/ACLCacheWrapperTest.php‎

Lines changed: 75 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,70 @@
99
namespace OCA\groupfolders\tests\ACL;
1010

1111
use OC\Files\Cache\CacheEntry;
12+
use OC\Files\Search\SearchComparison;
13+
use OC\Files\Search\SearchOrder;
14+
use OC\Files\Search\SearchQuery;
15+
use OC\Files\Storage\Temporary;
1216
use OCA\GroupFolders\ACL\ACLCacheWrapper;
1317
use OCA\GroupFolders\ACL\ACLManager;
18+
use OCA\GroupFolders\ACL\Rule;
19+
use OCA\GroupFolders\ACL\RuleManager;
20+
use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
21+
use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
1422
use OCP\Constants;
1523
use OCP\Files\Cache\ICache;
24+
use OCP\Files\Cache\ICacheEntry;
25+
use OCP\Files\Search\ISearchComparison;
26+
use OCP\Files\Search\ISearchOrder;
27+
use OCP\IUser;
1628
use PHPUnit\Framework\MockObject\MockObject;
1729
use Test\TestCase;
1830

1931
/**
2032
* @group DB
2133
*/
2234
class ACLCacheWrapperTest extends TestCase {
23-
private ACLManager&MockObject $aclManager;
35+
private ACLManager $aclManager;
2436
private ICache&MockObject $source;
2537
private ACLCacheWrapper $cache;
2638
private array $aclPermissions = [];
2739

40+
private function makeRule(int $permission): Rule {
41+
return new Rule($this->createMock(IUserMapping::class), 0, Constants::PERMISSION_ALL, $permission);
42+
}
43+
44+
/**
45+
* @param string[] $paths
46+
* @return array<string, Rule[]>
47+
*/
48+
private function getRulesForPaths(IUser $user, int $storageId, array $paths): array {
49+
return array_combine($paths, array_map(
50+
fn (string $path) => isset($this->aclPermissions[$path]) ? [$this->makeRule($this->aclPermissions[$path])] : [],
51+
$paths
52+
));
53+
}
54+
2855
#[\Override]
2956
protected function setUp(): void {
3057
parent::setUp();
3158

32-
$this->aclManager = $this->createMock(ACLManager::class);
33-
$this->aclManager->method('getACLPermissionsForPath')
34-
->willReturnCallback(fn (int $folderId, int $storageId, string $path) => $this->aclPermissions[$path] ?? Constants::PERMISSION_ALL);
59+
$user = $this->createMock(IUser::class);
60+
$ruleManager = $this->createMock(RuleManager::class);
61+
$ruleManager->method('getRulesForFilesByPath')
62+
->willReturnCallback($this->getRulesForPaths(...));
63+
$ruleManager->method('getRulesForPrefix')
64+
->willReturnCallback(fn (IUser $user, int $storageId, string $prefix) => array_map(
65+
fn (int $permission) => [$this->makeRule($permission)],
66+
array_filter(
67+
$this->aclPermissions,
68+
fn (string $path) => str_starts_with($path, $prefix),
69+
ARRAY_FILTER_USE_KEY,
70+
))
71+
);
72+
$mappingManager = $this->createMock(IUserMappingManager::class);
73+
74+
$this->aclManager = new ACLManager($ruleManager, $mappingManager, $user);
75+
3576
$this->source = $this->createMock(ICache::class);
3677
$this->source->method('getNumericStorageId')
3778
->willReturn(1);
@@ -75,4 +116,34 @@ public function testHideNonRead(): void {
75116

76117
$this->assertEquals($expected, $result);
77118
}
119+
120+
public function testSearchNonRead(): void {
121+
$sourceStorage = new Temporary([]);
122+
$sourceStorage->mkdir('foo');
123+
$sourceStorage->touch('foo/test1.txt', 100);
124+
$sourceStorage->touch('foo/test2.txt', 101);
125+
$sourceStorage->touch('foo/test3.txt', 102);
126+
$sourceStorage->mkdir('bar');
127+
$sourceStorage->touch('bar/test1.txt', 200);
128+
$sourceStorage->touch('bar/test2.txt', 201);
129+
$sourceStorage->touch('bar/test3.txt', 202);
130+
$sourceStorage->getScanner()->scan('');
131+
$this->aclPermissions['bar'] = 0;
132+
133+
$cache = new ACLCacheWrapper($sourceStorage->getCache(), $this->aclManager, 0, false);
134+
135+
$query = new SearchQuery(
136+
new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%test%'),
137+
2,
138+
0,
139+
[new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'mtime')]
140+
);
141+
$result = $cache->searchQuery($query);
142+
$paths = array_map(fn (ICacheEntry $entry) => $entry->getPath(), $result);
143+
144+
$this->assertEquals([
145+
'foo/test3.txt',
146+
'foo/test2.txt'
147+
], $paths);
148+
}
78149
}

‎tests/psalm-baseline.xml‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
<?xml version="1.0" encoding="UTF-8"?>
22
<files psalm-version="6.14.3@d0b040a91f280f071c1abcb1b77ce3822058725a">
33
<file src="lib/ACL/ACLCacheWrapper.php">
4-
<DeprecatedMethod>
5-
<code><![CDATA[search]]></code>
6-
<code><![CDATA[searchByMime]]></code>
7-
</DeprecatedMethod>
84
<PropertyNotSetInConstructor>
95
<code><![CDATA[ACLCacheWrapper]]></code>
106
<code><![CDATA[ACLCacheWrapper]]></code>

0 commit comments

Comments
 (0)