Skip to content

Commit 3479e40

Browse files
sorbaughsorbaugh
authored
Merge pull request #2870 from nextcloud/per-user-inherit
feat: add option to resolve inherited option per-user instead of per-path
2 parents d3a1f37 + c4b9374 commit 3479e40

File tree

5 files changed

+151
-26
lines changed

5 files changed

+151
-26
lines changed

‎lib/ACL/ACLManager.php

+48-10
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,12 @@ class ACLManager {
3535
private $rootFolderProvider;
3636

3737
public function __construct(
38-
private RuleManager $ruleManager,
38+
private RuleManager $ruleManager,
3939
private TrashManager $trashManager,
40-
private IUser $user,
41-
callable $rootFolderProvider,
42-
private ?int $rootStorageId = null,
40+
private IUser $user,
41+
callable $rootFolderProvider,
42+
private ?int $rootStorageId = null,
43+
private bool $inheritMergePerUser = false,
4344
) {
4445
$this->ruleCache = new CappedMemoryCache();
4546
$this->rootFolderProvider = $rootFolderProvider;
@@ -168,12 +169,49 @@ public function getPermissionsForPathFromRules(string $path, array $rules): int
168169
* @return int
169170
*/
170171
private function calculatePermissionsForPath(array $rules): int {
171-
// first combine all rules with the same path, then apply them on top of the current permissions
172-
// since $rules is sorted parent first rules for subfolders overwrite the rules from the parent
173-
return array_reduce($rules, function (int $permissions, array $rules): int {
174-
$mergedRule = Rule::mergeRules($rules);
175-
return $mergedRule->applyPermissions($permissions);
176-
}, Constants::PERMISSION_ALL);
172+
// given the following rules
173+
//
174+
// | Folder Rule | Read | Update | Share | Delete |
175+
// |-------------|------|--------|-------|--------|
176+
// | a: g1 | 1 | 1 | 1 | 1 |
177+
// | a: g2 | - | - | - | - |
178+
// | a/b: g1 | - | - | - | 0 |
179+
// | a/b: g2 | 0 | - | - | - |
180+
// |-------------|------|--------|-------|--------|
181+
//
182+
// and a user that is a member of g1 and g2
183+
//
184+
// Without `inheritMergePerUser` the user will not have access to `a/b`
185+
// as the merged rules for `a/b` ("-read,-delete") will overwrite the merged for `a` ("+read,+write+share+delete")
186+
//
187+
// With b`inheritMergePerUser` the user will have access to `a/b`
188+
// as the applied rules for `g1` ("+read,+write+share") merges with the applied rules for `g2` ("-read")
189+
if ($this->inheritMergePerUser) {
190+
// first combine all rules for the same user-mapping by path order
191+
// then merge the results with allow overwrites deny
192+
$rulesPerMapping = [];
193+
foreach ($rules as $rulesForPath) {
194+
foreach ($rulesForPath as $rule) {
195+
$mapping = $rule->getUserMapping();
196+
$key = $mapping->getType() . '/' . $mapping->getId();
197+
if (!isset($rulesPerMapping[$key])) {
198+
$rulesPerMapping[$key] = Rule::defaultRule();
199+
}
200+
201+
$rulesPerMapping[$key]->applyRule($rule);
202+
}
203+
}
204+
205+
$mergedRule = Rule::mergeRules($rulesPerMapping);
206+
return $mergedRule->applyPermissions(Constants::PERMISSION_ALL);
207+
} else {
208+
// first combine all rules with the same path, then apply them on top of the current permissions
209+
// since $rules is sorted parent first rules for subfolders overwrite the rules from the parent
210+
return array_reduce($rules, function (int $permissions, array $rules): int {
211+
$mergedRule = Rule::mergeRules($rules);
212+
return $mergedRule->applyPermissions($permissions);
213+
}, Constants::PERMISSION_ALL);
214+
}
177215
}
178216

179217
/**

‎lib/ACL/ACLManagerFactory.php

+10-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
namespace OCA\GroupFolders\ACL;
2525

2626
use OCA\GroupFolders\Trash\TrashManager;
27+
use OCP\IConfig;
2728
use OCP\IUser;
2829

2930
class ACLManagerFactory {
@@ -32,12 +33,20 @@ class ACLManagerFactory {
3233
public function __construct(
3334
private RuleManager $ruleManager,
3435
private TrashManager $trashManager,
36+
private IConfig $config,
3537
callable $rootFolderProvider,
3638
) {
3739
$this->rootFolderProvider = $rootFolderProvider;
3840
}
3941

4042
public function getACLManager(IUser $user, ?int $rootStorageId = null): ACLManager {
41-
return new ACLManager($this->ruleManager, $this->trashManager, $user, $this->rootFolderProvider, $rootStorageId);
43+
return new ACLManager(
44+
$this->ruleManager,
45+
$this->trashManager,
46+
$user,
47+
$this->rootFolderProvider,
48+
$rootStorageId,
49+
$this->config->getAppValue('groupfolders', 'acl-inherit-per-user', 'false') === 'true',
50+
);
4251
}
4352
}

‎lib/ACL/Rule.php

+27
Original file line numberDiff line numberDiff line change
@@ -155,4 +155,31 @@ public static function mergeRules(array $rules): Rule {
155155
$permissions
156156
);
157157
}
158+
159+
/**
160+
* apply a new rule on top of the existing
161+
*
162+
* All non-inherit fields of the new rule will overwrite the current permissions
163+
*
164+
* @param array $rules
165+
* @return void
166+
*/
167+
public function applyRule(Rule $rule): void {
168+
$this->permissions = $rule->applyPermissions($this->permissions);
169+
$this->mask |= $rule->getMask();
170+
}
171+
172+
/**
173+
* Create a default, no-op rule
174+
*
175+
* @return Rule
176+
*/
177+
public static function defaultRule(): Rule {
178+
return new Rule(
179+
new UserMapping('dummy', ''),
180+
-1,
181+
0,
182+
0
183+
);
184+
}
158185
}

‎lib/AppInfo/Application.php

+1
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@ public function register(IRegistrationContext $context): void {
224224
return new ACLManagerFactory(
225225
$c->get(RuleManager::class),
226226
$c->get(TrashManager::class),
227+
$c->get(IConfig::class),
227228
$rootFolderProvider
228229
);
229230
});

‎tests/ACL/ACLManagerTest.php

+65-15
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,9 @@ protected function setUp(): void {
5353

5454
$this->user = $this->createMock(IUser::class);
5555
$this->ruleManager = $this->createMock(RuleManager::class);
56-
$rootMountPoint = $this->createMock(IMountPoint::class);
57-
$rootMountPoint->method('getNumericStorageId')
58-
->willReturn(1);
59-
$rootFolder = $this->createMock(IRootFolder::class);
60-
$rootFolder->method('getMountPoint')
61-
->willReturn($rootMountPoint);
6256
$this->trashManager = $this->createMock(TrashManager::class);
63-
$this->aclManager = new ACLManager($this->ruleManager, $this->trashManager, $this->user, function () use ($rootFolder) {
64-
return $rootFolder;
65-
});
66-
$this->dummyMapping = $this->createMock(IUserMapping::class);
57+
$this->aclManager = $this->getAclManager();
58+
$this->dummyMapping = $this->createMapping('dummy');
6759

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

72+
private function createMapping(string $id): IUserMapping {
73+
$mapping = $this->createMock(IUserMapping::class);
74+
$mapping->method('getType')->willReturn('dummy');
75+
$mapping->method('getId')->willReturn($id);
76+
$mapping->method('getDisplayName')->willReturn("display name for $id");
77+
return $mapping;
78+
}
79+
80+
private function getAclManager(bool $perUserMerge = false) {
81+
$rootMountPoint = $this->createMock(IMountPoint::class);
82+
$rootMountPoint->method('getNumericStorageId')
83+
->willReturn(1);
84+
$rootFolder = $this->createMock(IRootFolder::class);
85+
$rootFolder->method('getMountPoint')
86+
->willReturn($rootMountPoint);
87+
88+
return new ACLManager($this->ruleManager, $this->trashManager, $this->user, function () use ($rootFolder) {
89+
return $rootFolder;
90+
}, null, $perUserMerge);
91+
}
92+
8093
public function testGetACLPermissionsForPathNoRules() {
8194
$this->rules = [];
8295
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo'));
@@ -85,19 +98,27 @@ public function testGetACLPermissionsForPathNoRules() {
8598
public function testGetACLPermissionsForPath() {
8699
$this->rules = [
87100
'foo' => [
88-
new Rule($this->dummyMapping, 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
89-
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, 0) // deny share
101+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
102+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_SHARE, 0) // deny share
90103
],
91104
'foo/bar' => [
92-
new Rule($this->dummyMapping, 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
105+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
93106
],
94107
'foo/bar/sub' => [
95-
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
96-
]
108+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
109+
],
110+
'foo/blocked' => [
111+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_READ, 0) // remove read
112+
],
113+
'foo/blocked2' => [
114+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ, 0) // remove read
115+
],
97116
];
98117
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $this->aclManager->getACLPermissionsForPath('foo'));
99118
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $this->aclManager->getACLPermissionsForPath('foo/bar'));
100119
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo/bar/sub'));
120+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $this->aclManager->getACLPermissionsForPath('foo/blocked'));
121+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $this->aclManager->getACLPermissionsForPath('foo/blocked2'));
101122
}
102123

103124
public function testGetACLPermissionsForPathInTrashbin() {
@@ -127,4 +148,33 @@ public function testGetACLPermissionsForPathInTrashbin() {
127148
]);
128149
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('__groupfolders/trash/1/subfolder2.d1700752274/coucou.md'));
129150
}
151+
152+
153+
154+
public function testGetACLPermissionsForPathPerUserMerge() {
155+
$aclManager = $this->getAclManager(true);
156+
$this->rules = [
157+
'foo' => [
158+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
159+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_SHARE, 0) // deny share
160+
],
161+
'foo/bar' => [
162+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
163+
],
164+
'foo/bar/sub' => [
165+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
166+
],
167+
'foo/blocked' => [
168+
new Rule($this->createMapping('2'), 10, Constants::PERMISSION_READ, 0) // remove read
169+
],
170+
'foo/blocked2' => [
171+
new Rule($this->createMapping('1'), 10, Constants::PERMISSION_READ, 0) // remove read
172+
],
173+
];
174+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $aclManager->getACLPermissionsForPath('foo'));
175+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $aclManager->getACLPermissionsForPath('foo/bar'));
176+
$this->assertEquals(Constants::PERMISSION_ALL, $aclManager->getACLPermissionsForPath('foo/bar/sub'));
177+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE, $aclManager->getACLPermissionsForPath('foo/blocked'));
178+
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE - Constants::PERMISSION_UPDATE - Constants::PERMISSION_READ, $aclManager->getACLPermissionsForPath('foo/blocked2'));
179+
}
130180
}

0 commit comments

Comments
 (0)