Skip to content

Commit 0ecb8ac

Browse files
icewind1991icewind1991
authored
Merge pull request #3435 from nextcloud/backport/3404/stable30
[stable30] fix: implement 'per user inherit' logic for folder delete permissions check
2 parents 41d0477 + 6f65b03 commit 0ecb8ac

File tree

4 files changed

+103
-11
lines changed

4 files changed

+103
-11
lines changed

‎lib/ACL/ACLManager.php

+13-10
Original file line numberDiff line numberDiff line change
@@ -215,16 +215,19 @@ public function getPermissionsForTree(string $path): int {
215215
$path = ltrim($path, '/');
216216
$rules = $this->ruleManager->getRulesForPrefix($this->user, $this->getRootStorageId(), $path);
217217

218-
return array_reduce($rules, function (int $permissions, array $rules): int {
219-
$mergedRule = Rule::mergeRules($rules);
220-
221-
$invertedMask = ~$mergedRule->getMask();
222-
// create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0
223-
$denyMask = $invertedMask | $mergedRule->getPermissions();
224-
225-
// since we only care about the lower permissions, we ignore the allow values
226-
return $permissions & $denyMask;
227-
}, Constants::PERMISSION_ALL);
218+
if ($this->inheritMergePerUser) {
219+
$pathsWithRules = array_keys($rules);
220+
$permissions = Constants::PERMISSION_ALL;
221+
foreach ($pathsWithRules as $path) {
222+
$permissions &= $this->getACLPermissionsForPath($path);
223+
}
224+
return $permissions;
225+
} else {
226+
return array_reduce($rules, function (int $permissions, array $rules): int {
227+
$mergedRule = Rule::mergeRules($rules);
228+
return $mergedRule->applyDenyPermissions($permissions);
229+
}, Constants::PERMISSION_ALL);
230+
}
228231
}
229232

230233
public function preloadRulesForFolder(string $path): void {

‎lib/ACL/Rule.php

+17-1
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,26 @@ public function applyPermissions(int $permissions): int {
8484
return $permissions | $allowMask;
8585
}
8686

87+
/**
88+
* Apply the deny permissions this rule to an existing permission set, returning the resulting permissions
89+
*
90+
* Only the deny permissions included in the current mask will overwrite the existing permissions
91+
*
92+
* @param int $permissions
93+
* @return int
94+
*/
95+
public function applyDenyPermissions(int $permissions): int {
96+
$invertedMask = ~$this->mask;
97+
// create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0
98+
$denyMask = $invertedMask | $this->permissions;
99+
100+
return $permissions & $denyMask;
101+
}
102+
87103
/**
88104
* @return void
89105
*/
90-
public function xmlSerialize(Writer $writer) {
106+
public function xmlSerialize(Writer $writer): void {
91107
$data = [
92108
self::ACL => [
93109
self::MAPPING_TYPE => $this->getUserMapping()->getType(),

‎lib/ACL/RuleManager.php

+3
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,9 @@ private function rulesByPath(array $rows, array $result = []): array {
206206
$result[$row['path']][] = $rule;
207207
}
208208
}
209+
210+
ksort($result);
211+
209212
return $result;
210213
}
211214

‎tests/ACL/ACLManagerTest.php

+70
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ protected function setUp(): void {
5050

5151
return array_merge($rules, $actualRules);
5252
});
53+
54+
$this->ruleManager->method('getRulesForPrefix')
55+
->willReturnCallback(function (IUser $user, int $storageId, string $prefix) {
56+
return array_filter($this->rules, function (string $path) use ($prefix) {
57+
return $prefix === $path || str_starts_with($path, $prefix . '/');
58+
}, ARRAY_FILTER_USE_KEY);
59+
});
5360
}
5461

5562
private function createMapping(string $id): IUserMapping {
@@ -160,4 +167,67 @@ public function testGetACLPermissionsForPathPerUserMerge(): void {
160167
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $aclManager->getACLPermissionsForPath('foo/blocked'));
161168
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $aclManager->getACLPermissionsForPath('foo/blocked2'));
162169
}
170+
171+
public function testGetPermissionsForTree(): void {
172+
$perUserAclManager = $this->getAclManager(true);
173+
174+
$this->rules = [
175+
'foo' => [
176+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
177+
],
178+
'foo/bar' => [
179+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
180+
],
181+
'foo/bar/asd' => [
182+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
183+
],
184+
];
185+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo'));
186+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo/bar'));
187+
188+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo'));
189+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar'));
190+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar/asd'));
191+
192+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo'));
193+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo/bar'));
194+
195+
$this->rules = [
196+
'foo2' => [
197+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
198+
],
199+
'foo2/bar' => [
200+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
201+
],
202+
'foo2/bar/asd' => [
203+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
204+
],
205+
];
206+
207+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2'));
208+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo2/bar'));
209+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2/bar/asd'));
210+
211+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2'));
212+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2/bar'));
213+
214+
$this->rules = [
215+
'foo3' => [
216+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
217+
],
218+
'foo3/bar' => [
219+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
220+
],
221+
'foo3/bar/asd' => [
222+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
223+
],
224+
];
225+
226+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3'));
227+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo3/bar'));
228+
$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3/bar/asd'));
229+
230+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3'));
231+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3/bar'));
232+
}
163233
}

0 commit comments

Comments
 (0)