Merge pull request #2870 from nextcloud/per-user-inherit

feat: add option to resolve inherited option per-user instead of per-path
nextcloud · Mar 22, 2024 · 3479e40 · 3479e40
2 parents d3a1f37 + c4b9374
commit 3479e40
Show file tree

Hide file tree

Showing 5 changed files with 151 additions and 26 deletions.
diff --git 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,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);
+		}
 	}
 
 	/**

diff --git 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
@@ -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
+		);
+	}
 }
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
@@ -224,6 +224,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
@@ -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'));
+	}
 }