From 6f65b03497d8ef2bd19a7c02e1a3f9b6c410b6b2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 8 Nov 2024 16:56:21 +0100 Subject: [PATCH] fix: implement 'merge rules per user' logic for folder delete permissions check Signed-off-by: Robin Appelman --- lib/ACL/ACLManager.php | 23 ++++++------ lib/ACL/Rule.php | 18 +++++++++- lib/ACL/RuleManager.php | 3 ++ tests/ACL/ACLManagerTest.php | 70 ++++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 11 deletions(-) diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index 0cb56ab66..0d0c5aa89 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -215,16 +215,19 @@ public function getPermissionsForTree(string $path): int { $path = ltrim($path, '/'); $rules = $this->ruleManager->getRulesForPrefix($this->user, $this->getRootStorageId(), $path); - return array_reduce($rules, function (int $permissions, array $rules): int { - $mergedRule = Rule::mergeRules($rules); - - $invertedMask = ~$mergedRule->getMask(); - // create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0 - $denyMask = $invertedMask | $mergedRule->getPermissions(); - - // since we only care about the lower permissions, we ignore the allow values - return $permissions & $denyMask; - }, Constants::PERMISSION_ALL); + if ($this->inheritMergePerUser) { + $pathsWithRules = array_keys($rules); + $permissions = Constants::PERMISSION_ALL; + foreach ($pathsWithRules as $path) { + $permissions &= $this->getACLPermissionsForPath($path); + } + return $permissions; + } else { + return array_reduce($rules, function (int $permissions, array $rules): int { + $mergedRule = Rule::mergeRules($rules); + return $mergedRule->applyDenyPermissions($permissions); + }, Constants::PERMISSION_ALL); + } } public function preloadRulesForFolder(string $path): void { diff --git a/lib/ACL/Rule.php b/lib/ACL/Rule.php index 068fd409e..53bd27184 100644 --- a/lib/ACL/Rule.php +++ b/lib/ACL/Rule.php @@ -84,10 +84,26 @@ public function applyPermissions(int $permissions): int { return $permissions | $allowMask; } + /** + * Apply the deny permissions this rule to an existing permission set, returning the resulting permissions + * + * Only the deny permissions included in the current mask will overwrite the existing permissions + * + * @param int $permissions + * @return int + */ + public function applyDenyPermissions(int $permissions): int { + $invertedMask = ~$this->mask; + // create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0 + $denyMask = $invertedMask | $this->permissions; + + return $permissions & $denyMask; + } + /** * @return void */ - public function xmlSerialize(Writer $writer) { + public function xmlSerialize(Writer $writer): void { $data = [ self::ACL => [ self::MAPPING_TYPE => $this->getUserMapping()->getType(), diff --git a/lib/ACL/RuleManager.php b/lib/ACL/RuleManager.php index f8e53dba4..e31aeda41 100644 --- a/lib/ACL/RuleManager.php +++ b/lib/ACL/RuleManager.php @@ -206,6 +206,9 @@ private function rulesByPath(array $rows, array $result = []): array { $result[$row['path']][] = $rule; } } + + ksort($result); + return $result; } diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php index 21d3c1a81..a7c591bc8 100644 --- a/tests/ACL/ACLManagerTest.php +++ b/tests/ACL/ACLManagerTest.php @@ -50,6 +50,13 @@ protected function setUp(): void { return array_merge($rules, $actualRules); }); + + $this->ruleManager->method('getRulesForPrefix') + ->willReturnCallback(function (IUser $user, int $storageId, string $prefix) { + return array_filter($this->rules, function (string $path) use ($prefix) { + return $prefix === $path || str_starts_with($path, $prefix . '/'); + }, ARRAY_FILTER_USE_KEY); + }); } private function createMapping(string $id): IUserMapping { @@ -160,4 +167,67 @@ public function testGetACLPermissionsForPathPerUserMerge(): void { $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $aclManager->getACLPermissionsForPath('foo/blocked')); $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $aclManager->getACLPermissionsForPath('foo/blocked2')); } + + public function testGetPermissionsForTree(): void { + $perUserAclManager = $this->getAclManager(true); + + $this->rules = [ + 'foo' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL), + ], + 'foo/bar' => [ + new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, 0) // remove delete + ], + 'foo/bar/asd' => [ + new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete + ], + ]; + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo/bar')); + + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo')); + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar')); + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar/asd')); + + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo')); + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo/bar')); + + $this->rules = [ + 'foo2' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL), + ], + 'foo2/bar' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete + ], + 'foo2/bar/asd' => [ + new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete + ], + ]; + + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo2/bar')); + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2/bar/asd')); + + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2/bar')); + + $this->rules = [ + 'foo3' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL), + ], + 'foo3/bar' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete + ], + 'foo3/bar/asd' => [ + new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete + ], + ]; + + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo3/bar')); + $this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3/bar/asd')); + + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3/bar')); + } }