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

[stable27] feat: add option to resolve inherited option per-user instead of per-path #2874

Merged
merged 1 commit into from
Mar 25, 2024
Merged

[stable27] feat: add option to resolve inherited option per-user instead of per-path #2874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 48 additions & 10 deletions lib/ACL/ACLManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,12 @@ class ACLManager {
private $rootFolderProvider;

public function __construct(
private RuleManager $ruleManager,
private RuleManager $ruleManager,
private TrashManager $trashManager,
private IUser $user,
callable $rootFolderProvider,
private ?int $rootStorageId = null,
private IUser $user,
callable $rootFolderProvider,
private ?int $rootStorageId = null,
private bool $inheritMergePerUser = false,
) {
$this->ruleCache = new CappedMemoryCache();
$this->rootFolderProvider = $rootFolderProvider;
Expand Down Expand Up @@ -168,12 +169,49 @@ public function getPermissionsForPathFromRules(string $path, array $rules): int
* @return int
*/
private function calculatePermissionsForPath(array $rules): int {
// first combine all rules with the same path, then apply them on top of the current permissions
// since $rules is sorted parent first rules for subfolders overwrite the rules from the parent
return array_reduce($rules, function (int $permissions, array $rules): int {
$mergedRule = Rule::mergeRules($rules);
return $mergedRule->applyPermissions($permissions);
}, Constants::PERMISSION_ALL);
// given the following rules
//
// | Folder Rule | Read | Update | Share | Delete |
// |-------------|------|--------|-------|--------|
// | a: g1 | 1 | 1 | 1 | 1 |
// | a: g2 | - | - | - | - |
// | a/b: g1 | - | - | - | 0 |
// | a/b: g2 | 0 | - | - | - |
// |-------------|------|--------|-------|--------|
//
// and a user that is a member of g1 and g2
//
// Without `inheritMergePerUser` the user will not have access to `a/b`
// as the merged rules for `a/b` ("-read,-delete") will overwrite the merged for `a` ("+read,+write+share+delete")
//
// With b`inheritMergePerUser` the user will have access to `a/b`
// as the applied rules for `g1` ("+read,+write+share") merges with the applied rules for `g2` ("-read")
if ($this->inheritMergePerUser) {
// first combine all rules for the same user-mapping by path order
// then merge the results with allow overwrites deny
$rulesPerMapping = [];
foreach ($rules as $rulesForPath) {
foreach ($rulesForPath as $rule) {
$mapping = $rule->getUserMapping();
$key = $mapping->getType() . '/' . $mapping->getId();
if (!isset($rulesPerMapping[$key])) {
$rulesPerMapping[$key] = Rule::defaultRule();
}

$rulesPerMapping[$key]->applyRule($rule);
}
}

$mergedRule = Rule::mergeRules($rulesPerMapping);
return $mergedRule->applyPermissions(Constants::PERMISSION_ALL);
} else {
// first combine all rules with the same path, then apply them on top of the current permissions
// since $rules is sorted parent first rules for subfolders overwrite the rules from the parent
return array_reduce($rules, function (int $permissions, array $rules): int {
$mergedRule = Rule::mergeRules($rules);
return $mergedRule->applyPermissions($permissions);
}, Constants::PERMISSION_ALL);
}
}

/**
Expand Down
11 changes: 10 additions & 1 deletion lib/ACL/ACLManagerFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
namespace OCA\GroupFolders\ACL;

use OCA\GroupFolders\Trash\TrashManager;
use OCP\IConfig;
use OCP\IUser;

class ACLManagerFactory {
Expand All @@ -32,12 +33,20 @@ class ACLManagerFactory {
public function __construct(
private RuleManager $ruleManager,
private TrashManager $trashManager,
private IConfig $config,
callable $rootFolderProvider,
) {
$this->rootFolderProvider = $rootFolderProvider;
}

public function getACLManager(IUser $user, ?int $rootStorageId = null): ACLManager {
return new ACLManager($this->ruleManager, $this->trashManager, $user, $this->rootFolderProvider, $rootStorageId);
return new ACLManager(
$this->ruleManager,
$this->trashManager,
$user,
$this->rootFolderProvider,
$rootStorageId,
$this->config->getAppValue('groupfolders', 'acl-inherit-per-user', 'false') === 'true',
);
}
}
27 changes: 27 additions & 0 deletions lib/ACL/Rule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,31 @@ public static function mergeRules(array $rules): Rule {
$permissions
);
}

/**
* apply a new rule on top of the existing
*
* All non-inherit fields of the new rule will overwrite the current permissions
*
* @param array $rules
* @return void
*/
public function applyRule(Rule $rule): void {
$this->permissions = $rule->applyPermissions($this->permissions);
$this->mask |= $rule->getMask();
}

/**
* Create a default, no-op rule
*
* @return Rule
*/
public static function defaultRule(): Rule {
return new Rule(
new UserMapping('dummy', ''),
-1,
0,
0
);
}
}
1 change: 1 addition & 0 deletions lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ public function register(IRegistrationContext $context): void {
return new ACLManagerFactory(
$c->get(RuleManager::class),
$c->get(TrashManager::class),
$c->get(IConfig::class),
$rootFolderProvider
);
});
Expand Down
80 changes: 65 additions & 15 deletions tests/ACL/ACLManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,9 @@ protected function setUp(): void {

$this->user = $this->createMock(IUser::class);
$this->ruleManager = $this->createMock(RuleManager::class);
$rootMountPoint = $this->createMock(IMountPoint::class);
$rootMountPoint->method('getNumericStorageId')
->willReturn(1);
$rootFolder = $this->createMock(IRootFolder::class);
$rootFolder->method('getMountPoint')
->willReturn($rootMountPoint);
$this->trashManager = $this->createMock(TrashManager::class);
$this->aclManager = new ACLManager($this->ruleManager, $this->trashManager, $this->user, function () use ($rootFolder) {
return $rootFolder;
});
$this->dummyMapping = $this->createMock(IUserMapping::class);
$this->aclManager = $this->getAclManager();
$this->dummyMapping = $this->createMapping('dummy');

$this->ruleManager->method('getRulesForFilesByPath')
->willReturnCallback(function (IUser $user, int $storageId, array $paths) {
Expand All @@ -77,6 +69,27 @@ protected function setUp(): void {
});
}

private function createMapping(string $id): IUserMapping {
$mapping = $this->createMock(IUserMapping::class);
$mapping->method('getType')->willReturn('dummy');
$mapping->method('getId')->willReturn($id);
$mapping->method('getDisplayName')->willReturn("display name for $id");
return $mapping;
}

private function getAclManager(bool $perUserMerge = false) {
$rootMountPoint = $this->createMock(IMountPoint::class);
$rootMountPoint->method('getNumericStorageId')
->willReturn(1);
$rootFolder = $this->createMock(IRootFolder::class);
$rootFolder->method('getMountPoint')
->willReturn($rootMountPoint);

return new ACLManager($this->ruleManager, $this->trashManager, $this->user, function () use ($rootFolder) {
return $rootFolder;
}, null, $perUserMerge);
}

public function testGetACLPermissionsForPathNoRules() {
$this->rules = [];
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo'));
Expand All @@ -85,19 +98,27 @@ public function testGetACLPermissionsForPathNoRules() {
public function testGetACLPermissionsForPath() {
$this->rules = [
'foo' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, 0) // deny share
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_SHARE, 0) // deny share
],
'foo/bar' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
],
'foo/bar/sub' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
]
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
],
'foo/blocked' => [
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_READ, 0) // remove read
],
'foo/blocked2' => [
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ, 0) // remove read
],
];
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $this->aclManager->getACLPermissionsForPath('foo'));
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $this->aclManager->getACLPermissionsForPath('foo/bar'));
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo/bar/sub'));
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $this->aclManager->getACLPermissionsForPath('foo/blocked'));
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $this->aclManager->getACLPermissionsForPath('foo/blocked2'));
}

public function testGetACLPermissionsForPathInTrashbin() {
Expand Down Expand Up @@ -127,4 +148,33 @@ public function testGetACLPermissionsForPathInTrashbin() {
]);
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('__groupfolders/trash/1/subfolder2.d1700752274/coucou.md'));
}



public function testGetACLPermissionsForPathPerUserMerge() {
$aclManager = $this->getAclManager(true);
$this->rules = [
'foo' => [
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_SHARE, 0) // deny share
],
'foo/bar' => [
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
],
'foo/bar/sub' => [
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
],
'foo/blocked' => [
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_READ, 0) // remove read
],
'foo/blocked2' => [
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ, 0) // remove read
],
];
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $aclManager->getACLPermissionsForPath('foo'));
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $aclManager->getACLPermissionsForPath('foo/bar'));
$this->assertEquals(Constants::PERMISSION_ALL, $aclManager->getACLPermissionsForPath('foo/bar/sub'));
$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'));
}
}