fix: implement 'merge rules per user' logic for folder delete permiss…

…ions check Signed-off-by: Robin Appelman <[email protected]>
nextcloud · Nov 25, 2024 · 6f65b03 · 6f65b03
1 parent 41d0477
commit 6f65b03
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 11 deletions.
diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php
@@ -215,16 +215,19 @@ 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): int {
-			$mergedRule = Rule::mergeRules($rules);
-
-			$invertedMask = ~$mergedRule->getMask();
-			// create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0
-			$denyMask = $invertedMask | $mergedRule->getPermissions();
-
-			// since we only care about the lower permissions, we ignore the allow values
-			return $permissions & $denyMask;
-		}, Constants::PERMISSION_ALL);
+		if ($this->inheritMergePerUser) {
+			$pathsWithRules = array_keys($rules);
+			$permissions = Constants::PERMISSION_ALL;
+			foreach ($pathsWithRules as $path) {
+				$permissions &= $this->getACLPermissionsForPath($path);
+			}
+			return $permissions;
+		} else {
+			return array_reduce($rules, function (int $permissions, array $rules): int {
+				$mergedRule = Rule::mergeRules($rules);
+				return $mergedRule->applyDenyPermissions($permissions);
+			}, Constants::PERMISSION_ALL);
+		}
 	}
 
 	public function preloadRulesForFolder(string $path): void {

diff --git a/lib/ACL/Rule.php b/lib/ACL/Rule.php
@@ -84,10 +84,26 @@ public function applyPermissions(int $permissions): int {
 		return $permissions | $allowMask;
 	}
 
+	/**
+	 * Apply the deny permissions this rule to an existing permission set, returning the resulting permissions
+	 *
+	 * Only the deny permissions included in the current mask will overwrite the existing permissions
+	 *
+	 * @param int $permissions
+	 * @return int
+	 */
+	public function applyDenyPermissions(int $permissions): int {
+		$invertedMask = ~$this->mask;
+		// create a bitmask that has all inherit and allow bits set to 1 and all deny bits to 0
+		$denyMask = $invertedMask | $this->permissions;
+
+		return $permissions & $denyMask;
+	}
+
 	/**
 	 * @return void
 	 */
-	public function xmlSerialize(Writer $writer) {
+	public function xmlSerialize(Writer $writer): void {
 		$data = [
 			self::ACL => [
 				self::MAPPING_TYPE => $this->getUserMapping()->getType(),

diff --git a/lib/ACL/RuleManager.php b/lib/ACL/RuleManager.php
@@ -206,6 +206,9 @@ private function rulesByPath(array $rows, array $result = []): array {
 				$result[$row['path']][] = $rule;
 			}
 		}
+
+		ksort($result);
+
 		return $result;
 	}
 

diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php
@@ -50,6 +50,13 @@ protected function setUp(): void {
 
 				return array_merge($rules, $actualRules);
 			});
+
+		$this->ruleManager->method('getRulesForPrefix')
+			->willReturnCallback(function (IUser $user, int $storageId, string $prefix) {
+				return array_filter($this->rules, function (string $path) use ($prefix) {
+					return $prefix === $path || str_starts_with($path, $prefix . '/');
+				}, ARRAY_FILTER_USE_KEY);
+			});
 	}
 
 	private function createMapping(string $id): IUserMapping {
@@ -160,4 +167,67 @@ public function testGetACLPermissionsForPathPerUserMerge(): void {
 		$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'));
 	}
+
+	public function testGetPermissionsForTree(): void {
+		$perUserAclManager = $this->getAclManager(true);
+
+		$this->rules = [
+			'foo' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
+			],
+			'foo/bar' => [
+				new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
+			],
+			'foo/bar/asd' => [
+				new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
+			],
+		];
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo'));
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $this->aclManager->getPermissionsForTree('foo/bar'));
+
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo'));
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar'));
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo/bar/asd'));
+
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo'));
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getPermissionsForTree('foo/bar'));
+
+		$this->rules = [
+			'foo2' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
+			],
+			'foo2/bar' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
+			],
+			'foo2/bar/asd' => [
+				new Rule($this->createMapping('2'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
+			],
+		];
+
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2'));
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo2/bar'));
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo2/bar/asd'));
+
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2'));
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo2/bar'));
+
+		$this->rules = [
+			'foo3' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_ALL, Constants::PERMISSION_ALL),
+			],
+			'foo3/bar' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, 0) // remove delete
+			],
+			'foo3/bar/asd' => [
+				new Rule($this->createMapping('1'), 10, Constants::PERMISSION_DELETE, Constants::PERMISSION_DELETE) // re-add delete
+			],
+		];
+
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3'));
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getACLPermissionsForPath('foo3/bar'));
+		$this->assertEquals(Constants::PERMISSION_ALL, $perUserAclManager->getACLPermissionsForPath('foo3/bar/asd'));
+
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3'));
+		$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_DELETE, $perUserAclManager->getPermissionsForTree('foo3/bar'));
+	}
 }
-Original file line number
+Diff line change
@@ Expand Up @@
     				$result[$row['path']][] = $rule;
     			}
     		}
+    		ksort($result);
     		return $result;
     	}
@@ Expand Down @@