Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make advanced permissions to inherit per user/group #1654

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Make advanced permissions to inherit per user/group #1654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
081d8dc
lib/ACL/ACLManager.php: Make permissions to inherit per user/group
YoitoFes Sep 1, 2021
d63ca9c
README.md: Make clear that permissions inherit per user/group
YoitoFes Sep 3, 2021
0509e30
lib/ACL/Rule.php: Remove unused method
YoitoFes Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
43 changes: 32 additions & 11 deletions lib/ACL/ACLManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand All @@ -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);
}
}
24 changes: 0 additions & 24 deletions lib/ACL/Rule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}
}
26 changes: 23 additions & 3 deletions tests/ACL/ACLManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,7 +38,7 @@ class ACLManagerTest extends TestCase {
private $user;
/** @var ACLManager */
private $aclManager;
/** @var IUserMapping */
/** @var UserMapping */
private $dummyMapping;
/** @var Rule[] */
private $rules = [];
Expand All @@ -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) {
Expand Down Expand Up @@ -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'));
}
}
35 changes: 0 additions & 35 deletions tests/ACL/RuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}