From 081d8dcacd6306524d80cd30d6a575148e1dd594 Mon Sep 17 00:00:00 2001 From: Naoto Kobayashi Date: Wed, 1 Sep 2021 23:17:44 +0900 Subject: [PATCH 1/3] lib/ACL/ACLManager.php: Make permissions to inherit per user/group Curently advanced permissions inherit permissions merged all user/group's permissions every depth of path. This implementation gets incorrect permissions in some cases. e.g. foo/ : Allow all for 'manager' group foo/bar/ : Deny write for 'member' group Although the user who is member of 'manager' and 'member' should have write permission to foo/bar/, Writing to foo/bar is denied. Fix it by making permissions to inherit per user/group. Signed-off-by: Naoto Kobayashi --- lib/ACL/ACLManager.php | 43 +++++++++++++++++++++++++++--------- tests/ACL/ACLManagerTest.php | 26 +++++++++++++++++++--- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index f40fe48b7..9b21948f7 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -122,10 +122,21 @@ public function getACLPermissionsForPath(string $path): int { $path = ltrim($path, '/'); $rules = $this->getRules($this->getParents($path)); - return array_reduce($rules, function (int $permissions, array $rules) { - $mergedRule = Rule::mergeRules($rules); - return $mergedRule->applyPermissions($permissions); - }, Constants::PERMISSION_ALL); + $inheritedPermissionsByMapping = []; + array_walk_recursive($rules, function($rule) use(&$inheritedPermissionsByMapping) { + $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + if (!isset($inheritedPermissionsByMapping[$mappingKey])) { + $inheritedPermissionsByMapping[$mappingKey] = Constants::PERMISSION_ALL; + } + $inheritedPermissionsByMapping[$mappingKey] = $rule->applyPermissions($inheritedPermissionsByMapping[$mappingKey]); + }); + if (empty($inheritedPermissionsByMapping)) { + return Constants::PERMISSION_ALL; + } + + return array_reduce($inheritedPermissionsByMapping, function (int $mergedParmission, int $permissions) { + return $mergedParmission | $permissions; + }, 0); } /** @@ -138,15 +149,25 @@ 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) { - $mergedRule = Rule::mergeRules($rules); - - $invertedMask = ~$mergedRule->getMask(); + $inheritedPermissionsByMapping = []; + array_walk_recursive($rules, function($rule) use(&$inheritedPermissionsByMapping) { + $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + if (!isset($inheritedPermissionsByMapping[$mappingKey])) { + $inheritedPermissionsByMapping[$mappingKey] = Constants::PERMISSION_ALL; + } + $invertedMask = ~$rule->getMask(); // create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0 - $denyMask = $invertedMask | $mergedRule->getPermissions(); + $denyMask = $invertedMask | $rule->getPermissions(); // since we only care about the lower permissions, we ignore the allow values - return $permissions & $denyMask; - }, Constants::PERMISSION_ALL); + $inheritedPermissionsByMapping[$mappingKey] = $inheritedPermissionsByMapping[$mappingKey] & $denyMask; + }); + if (empty($inheritedPermissionsByMapping)) { + return Constants::PERMISSION_ALL; + } + + return array_reduce($inheritedPermissionsByMapping, function (int $mergedParmission, int $permissions) { + return $mergedParmission | $permissions; + }, 0); } } diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php index 18ce62d6f..9a30517da 100644 --- a/tests/ACL/ACLManagerTest.php +++ b/tests/ACL/ACLManagerTest.php @@ -24,7 +24,7 @@ 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\UserMapping; use OCP\Constants; use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountPoint; @@ -38,7 +38,7 @@ class ACLManagerTest extends TestCase { private $user; /** @var ACLManager */ private $aclManager; - /** @var IUserMapping */ + /** @var UserMapping */ private $dummyMapping; /** @var Rule[] */ private $rules = []; @@ -57,7 +57,7 @@ protected function setUp(): void { $this->aclManager = new ACLManager($this->ruleManager, $this->user, function () use ($rootFolder) { return $rootFolder; }); - $this->dummyMapping = $this->createMock(IUserMapping::class); + $this->dummyMapping = $this->createMock(UserMapping::class); $this->ruleManager->method('getRulesForFilesByPath') ->willReturnCallback(function (IUser $user, int $storageId, array $paths) { @@ -89,4 +89,24 @@ public function testGetACLPermissionsForPath() { $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $this->aclManager->getACLPermissionsForPath('foo/bar')); $this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo/bar/sub')); } + + public function testGetACLPermissionsForPathWithMultipleMappings() { + $anoterMapping = new UserMapping('group', 'test'); + $this->rules = [ + 'foo' => [ + new Rule($this->dummyMapping, 10, Constants::PERMISSION_ALL, Constants::PERMISSION_READ), // read only for dummyMapping + new Rule($anoterMapping, 10, 0, 0) // allow all for anotherMapping + ], + 'foo/bar' => [ + new Rule($this->dummyMapping, 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE), // add write for dummyMapping + new Rule($anoterMapping, 10, Constants::PERMISSION_UPDATE, 0) // deny write for anotherMapping + ], + 'foo/bar/sub' => [ + new Rule($this->dummyMapping, 10, Constants::PERMISSION_ALL, 0) // deny all for dummyMapping + ] + ]; + $this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo')); + $this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo/bar')); + $this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_UPDATE, $this->aclManager->getACLPermissionsForPath('foo/bar/sub')); + } } From d63ca9c3998988064507dd9f60b55d7ef9b92125 Mon Sep 17 00:00:00 2001 From: Naoto Kobayashi Date: Fri, 3 Sep 2021 21:32:57 +0900 Subject: [PATCH 2/3] README.md: Make clear that permissions inherit per user/group Signed-off-by: Naoto Kobayashi --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b95b75237..37a443732 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ Once configured, the folders will show up in the home folder for each user in th _Advanced Permissions_ allows entitled users to configure permissions inside groupfolders on a per file and folder basis. -Permissions are configured by setting one or more of "Read", "Write", "Create", "Delete" or "Share" permissions to "allow" or "deny". Any permission not explicitly set will inherit the permissions from the parent folder. If multiple configured advanced permissions for a single file or folder apply for a single user (such as when a user belongs to multiple groups), the "allow" permission will overwrite any "deny" permission. Denied permissions configured for the group folder itself cannot be overwritten to "allow" permissions by the advanced permission rules. +Permissions are configured by setting one or more of "Read", "Write", "Create", "Delete" or "Share" permissions to "allow" or "deny". Any permission not explicitly set for a user/group will inherit the user/group's permissions from the parent folder. If multiple configured advanced permissions for a single file or folder apply for a single user (such as when a user belongs to multiple groups), the "allow" permission will overwrite any "deny" permission. Denied permissions configured for the group folder itself cannot be overwritten to "allow" permissions by the advanced permission rules. ![advanced permissions](screenshots/acl.png) From 0509e304b0070c8e9cb5b23ffc400a9be46ff300 Mon Sep 17 00:00:00 2001 From: Naoto Kobayashi Date: Fri, 3 Sep 2021 21:39:25 +0900 Subject: [PATCH 3/3] lib/ACL/Rule.php: Remove unused method Signed-off-by: Naoto Kobayashi --- lib/ACL/Rule.php | 24 ------------------------ tests/ACL/RuleTest.php | 35 ----------------------------------- 2 files changed, 59 deletions(-) diff --git a/lib/ACL/Rule.php b/lib/ACL/Rule.php index 7a6c46428..4e11980ed 100644 --- a/lib/ACL/Rule.php +++ b/lib/ACL/Rule.php @@ -117,28 +117,4 @@ public static function xmlDeserialize(Reader $reader): Rule { (int)$elements[self::PERMISSIONS] ); } - - /** - * merge multiple rules that apply on the same file where allow overwrites deny - * - * @param array $rules - * @return Rule - */ - public static function mergeRules(array $rules): Rule { - // or'ing the masks to get a new mask that masks all set permissions - $mask = array_reduce($rules, function (int $mask, Rule $rule) { - return $mask | $rule->getMask(); - }, 0); - // or'ing the permissions combines them with allow overwriting deny - $permissions = array_reduce($rules, function (int $permissions, Rule $rule) { - return $permissions | $rule->getPermissions(); - }, 0); - - return new Rule( - new UserMapping('dummy', ''), - -1, - $mask, - $permissions - ); - } } diff --git a/tests/ACL/RuleTest.php b/tests/ACL/RuleTest.php index 00e3d3148..59f898351 100644 --- a/tests/ACL/RuleTest.php +++ b/tests/ACL/RuleTest.php @@ -43,39 +43,4 @@ public function testApplyPermissions($input, $mask, $permissions, $expected) { $rule = new Rule($this->createMock(IUserMapping::class), 0, $mask, $permissions); $this->assertEquals($expected, $rule->applyPermissions($input)); } - - public function mergeRulesProvider() { - return [ - [[ - [0b00001111, 0b00000011], - [0b00001111, 0b00000011], - ], 0b00001111, 0b00000011], - [[ - [0b00001111, 0b00000000], - [0b00001111, 0b00000011], - ], 0b00001111, 0b00000011], - [[ - [0b00000011, 0b00000011], - [0b00001100, 0b00000000], - ], 0b00001111, 0b00000011], - [[ - [0b00001100, 0b00000000], - [0b00000011, 0b00000011], - [0b00001111, 0b00000100], - ], 0b00001111, 0b00000111], - ]; - } - - /** - * @dataProvider mergeRulesProvider - */ - public function testMergeRules($inputs, $expectedMask, $expectedPermissions) { - $inputRules = array_map(function (array $input) { - return new Rule($this->createMock(IUserMapping::class), 0, $input[0], $input[1]); - }, $inputs); - - $result = Rule::mergeRules($inputRules); - $this->assertEquals($expectedMask, $result->getMask()); - $this->assertEquals($expectedPermissions, $result->getPermissions()); - } }