Skip to content

Commit 7927317

Browse files
icewind1991icewind1991
authored
Merge pull request #3404 from nextcloud/per-mapping-merge-delete-folder
fix: implement 'per user inherit' logic for folder delete permissions check
2 parents dffd722 + 5818777 commit 7927317

File tree

4 files changed

+104
-10
lines changed

4 files changed

+104
-10
lines changed

‎lib/ACL/ACLManager.php

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

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

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

‎lib/ACL/Rule.php

+19
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,25 @@ public function applyPermissions(int $permissions): int {
8282
return $permissions | $allowMask;
8383
}
8484

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

‎lib/ACL/RuleManager.php

+2
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,8 @@ private function rulesByPath(array $rows, array $result = []): array {
193193
}
194194
}
195195

196+
ksort($result);
197+
196198
return $result;
197199
}
198200

‎tests/ACL/ACLManagerTest.php

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

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

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

0 commit comments

Comments
 (0)