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) 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/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/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')); + } } 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()); - } }