From 169c4d1278e06a8d9c6481fd5048f3b95e66a561 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 18 Mar 2024 18:02:47 +0100 Subject: [PATCH] feat: add option to resolve inherited option per-user instead of per-path Signed-off-by: Robin Appelman --- lib/ACL/ACLManager.php | 41 +++++++++++++----- lib/ACL/ACLManagerFactory.php | 11 ++++- lib/ACL/Rule.php | 28 ++++++++++++ lib/AppInfo/Application.php | 1 + tests/ACL/ACLManagerTest.php | 80 ++++++++++++++++++++++++++++------- 5 files changed, 135 insertions(+), 26 deletions(-) diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index 44dcd03f8..bb471c979 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -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; @@ -168,12 +169,32 @@ 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); + 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); + } } /** diff --git a/lib/ACL/ACLManagerFactory.php b/lib/ACL/ACLManagerFactory.php index 805d78a3d..11ea7ddfb 100644 --- a/lib/ACL/ACLManagerFactory.php +++ b/lib/ACL/ACLManagerFactory.php @@ -24,6 +24,7 @@ namespace OCA\GroupFolders\ACL; use OCA\GroupFolders\Trash\TrashManager; +use OCP\IConfig; use OCP\IUser; class ACLManagerFactory { @@ -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', + ); } } diff --git a/lib/ACL/Rule.php b/lib/ACL/Rule.php index a0245d0b2..2d4c8c3a6 100644 --- a/lib/ACL/Rule.php +++ b/lib/ACL/Rule.php @@ -25,6 +25,7 @@ use OCA\GroupFolders\ACL\UserMapping\IUserMapping; use OCA\GroupFolders\ACL\UserMapping\UserMapping; +use OCP\Constants; use Sabre\Xml\Reader; use Sabre\Xml\Writer; use Sabre\Xml\XmlDeserializable; @@ -155,4 +156,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 + ); + } } diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index a06bde607..05ce1d0c5 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -223,6 +223,7 @@ public function register(IRegistrationContext $context): void { return new ACLManagerFactory( $c->get(RuleManager::class), $c->get(TrashManager::class), + $c->get(IConfig::class), $rootFolderProvider ); }); diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php index eacb23a8e..1b0ea88dc 100644 --- a/tests/ACL/ACLManagerTest.php +++ b/tests/ACL/ACLManagerTest.php @@ -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) { @@ -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')); @@ -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() { @@ -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')); + } }