Merge pull request #2960 from nextcloud/prevent-revoke-self

fix(ACL): Add check to prevent users from revoking their own access

Author: provokateurin

lib/ACL/ACLManager.php

@@ -8,6 +8,7 @@
 
 namespace OCA\GroupFolders\ACL;
 
+use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
 use OCA\GroupFolders\Trash\TrashManager;
 use OCP\Cache\CappedMemoryCache;
 use OCP\Constants;
@@ -22,6 +23,7 @@ class ACLManager {
 	public function __construct(
 		private readonly RuleManager $ruleManager,
 		private readonly TrashManager $trashManager,
+		private readonly IUserMappingManager $userMappingManager,
 		private readonly LoggerInterface $logger,
 		private readonly IUser $user,
 		private readonly \Closure $rootFolderProvider,
@@ -85,7 +87,7 @@ private function getRelevantPaths(string $path): array {
 		$fromTrashbin = str_starts_with($path, '__groupfolders/trash/');
 		if ($fromTrashbin) {
 			/* Exploded path will look like ["__groupfolders", "trash", "1", "folderName.d2345678", "rest/of/the/path.txt"] */
-			[,,$groupFolderId,$rootTrashedItemName] = explode('/', $path, 5);
+			[, , $groupFolderId, $rootTrashedItemName] = explode('/', $path, 5);
 			$groupFolderId = (int)$groupFolderId;
 			/* Remove the date part */
 			$separatorPos = strrpos($rootTrashedItemName, '.d');
@@ -143,6 +145,20 @@ public function getACLPermissionsForPath(string $path): int {
 		return $this->calculatePermissionsForPath($rules);
 	}
 
+	/**
+	 * Check what the effective permissions would be for the current user for a path would be with a new set of rules
+	 *
+	 * @param list<Rule> $newRules
+	 */
+	public function testACLPermissionsForPath(string $path, array $newRules): int {
+		$path = ltrim($path, '/');
+		$rules = $this->getRules($this->getRelevantPaths($path));
+
+		$rules[$path] = $this->filterApplicableRulesToUser($newRules);
+
+		return $this->calculatePermissionsForPath($rules);
+	}
+
 	/**
 	 * @param array<string, Rule[]> $rules list of rules per path
 	 */
@@ -229,4 +245,25 @@ public function getPermissionsForTree(string $path): int {
 	public function preloadRulesForFolder(string $path): void {
 		$this->ruleManager->getRulesForFilesByParent($this->user, $this->getRootStorageId(), $path);
 	}
+
+	/**
+	 * Filter a list to only the rules applicable to the current user
+	 *
+	 * @param list<Rule> $rules
+	 * @return list<Rule>
+	 */
+	private function filterApplicableRulesToUser(array $rules): array {
+		$userMappings = $this->userMappingManager->getMappingsForUser($this->user);
+		return array_values(array_filter($rules, function (Rule $rule) use ($userMappings): bool {
+			foreach ($userMappings as $userMapping) {
+				if (
+					$userMapping->getType() == $rule->getUserMapping()->getType() &&
+					$userMapping->getId() == $rule->getUserMapping()->getId()
+				) {
+					return true;
+				}
+			}
+			return false;
+		}));
+	}
 }

lib/ACL/ACLManagerFactory.php

@@ -8,6 +8,7 @@
 
 namespace OCA\GroupFolders\ACL;
 
+use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
 use OCA\GroupFolders\Trash\TrashManager;
 use OCP\IAppConfig;
 use OCP\IUser;
@@ -19,6 +20,7 @@ public function __construct(
 		private readonly TrashManager $trashManager,
 		private readonly IAppConfig $config,
 		private readonly LoggerInterface $logger,
+		private readonly IUserMappingManager $userMappingManager,
 		private readonly \Closure $rootFolderProvider,
 	) {
 	}
@@ -27,6 +29,7 @@ public function getACLManager(IUser $user, ?int $rootStorageId = null): ACLManag
 		return new ACLManager(
 			$this->ruleManager,
 			$this->trashManager,
+			$this->userMappingManager,
 			$this->logger,
 			$user,
 			$this->rootFolderProvider,

lib/AppInfo/Application.php

@@ -217,6 +217,7 @@ public function register(IRegistrationContext $context): void {
 				$c->get(TrashManager::class),
 				$c->get(IAppConfig::class),
 				$c->get(LoggerInterface::class),
+				$c->get(IUserMappingManager::class),
 				$rootFolderProvider
 			);
 		});

lib/DAV/ACLPlugin.php

@@ -9,16 +9,19 @@
 namespace OCA\GroupFolders\DAV;
 
 use OCA\DAV\Connector\Sabre\Node;
+use OCA\GroupFolders\ACL\ACLManagerFactory;
 use OCA\GroupFolders\ACL\Rule;
 use OCA\GroupFolders\ACL\RuleManager;
 use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
 use OCA\GroupFolders\Folder\FolderManager;
 use OCA\GroupFolders\Mount\GroupMountPoint;
 use OCP\Constants;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IL10N;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Log\Audit\CriticalActionPerformedEvent;
+use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\INode;
 use Sabre\DAV\PropFind;
 use Sabre\DAV\PropPatch;
@@ -41,6 +44,8 @@ public function __construct(
 		private readonly IUserSession $userSession,
 		private readonly FolderManager $folderManager,
 		private readonly IEventDispatcher $eventDispatcher,
+		private readonly ACLManagerFactory $aclManagerFactory,
+		private readonly IL10N $l10n,
 	) {
 	}
 
@@ -56,7 +61,7 @@ private function isAdmin(string $path): bool {
 
 	public function initialize(Server $server): void {
 		$this->server = $server;
-		$this->user = $user = $this->userSession->getUser();
+		$this->user = $this->userSession->getUser();
 
 		$this->server->on('propFind', $this->propFind(...));
 		$this->server->on('propPatch', $this->propPatch(...));
@@ -192,15 +197,19 @@ public function propPatch(string $path, PropPatch $propPatch): void {
 				return false;
 			}
 
+			if ($this->user === null) {
+				return false;
+			}
+
 			$path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/');
 
 			// populate fileid in rules
-			$rules = array_map(fn (Rule $rule): Rule => new Rule(
+			$rules = array_values(array_map(fn (Rule $rule): Rule => new Rule(
 				$rule->getUserMapping(),
 				$fileInfo->getId(),
 				$rule->getMask(),
 				$rule->getPermissions()
-			), $rawRules);
+			), $rawRules));
 
 			$formattedRules = array_map(fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), $rules);
 			if (count($formattedRules)) {
@@ -217,6 +226,12 @@ public function propPatch(string $path, PropPatch $propPatch): void {
 				]));
 			}
 
+			$aclManager = $this->aclManagerFactory->getACLManager($this->user);
+			$newPermissions = $aclManager->testACLPermissionsForPath($fileInfo->getPath(), $rules);
+			if (!($newPermissions & Constants::PERMISSION_READ)) {
+				throw new BadRequest($this->l10n->t('You can not remove your own read permission.'));
+			}
+
 			$existingRules = array_reduce(
 				$this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]),
 				fn (array $rules, array $rulesForPath): array => array_merge($rules, $rulesForPath),

src/client.js

@@ -252,7 +252,8 @@ class AclDavService {
 				} else {
 					// Handle unexpected status codes
 					logger.error('Unexpected status:', { responseStatus: response.status, responseStatusText: response.statusText })
-					throw new Error(t('groupfolders', 'Unexpected status from server'))
+
+					throw new Error(response.xhr.responseXML?.querySelector('message')?.textContent ?? t('groupfolders', 'Unexpected status from server'))
 				}
 		  }).catch(error => {
 			// Handle network errors or exceptions

tests/ACL/ACLManagerTest.php

@@ -12,6 +12,7 @@
 use OCA\GroupFolders\ACL\Rule;
 use OCA\GroupFolders\ACL\RuleManager;
 use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
+use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
 use OCA\GroupFolders\Trash\TrashManager;
 use OCP\Constants;
 use OCP\Files\IRootFolder;
@@ -24,6 +25,7 @@
 class ACLManagerTest extends TestCase {
 	private RuleManager&MockObject $ruleManager;
 	private TrashManager&MockObject $trashManager;
+	private IUserMappingManager&MockObject $userMappingManager;
 	private LoggerInterface&MockObject $logger;
 	private IUser&MockObject $user;
 	private ACLManager $aclManager;
@@ -37,6 +39,7 @@ protected function setUp(): void {
 		$this->user = $this->createMock(IUser::class);
 		$this->ruleManager = $this->createMock(RuleManager::class);
 		$this->trashManager = $this->createMock(TrashManager::class);
+		$this->userMappingManager = $this->createMock(IUserMappingManager::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->aclManager = $this->getAclManager();
 		$this->dummyMapping = $this->createMapping('dummy');
@@ -75,7 +78,7 @@ private function getAclManager(bool $perUserMerge = false): ACLManager {
 		$rootFolder->method('getMountPoint')
 			->willReturn($rootMountPoint);
 
-		return new ACLManager($this->ruleManager, $this->trashManager, $this->logger, $this->user, fn (): IRootFolder&MockObject => $rootFolder, null, $perUserMerge);
+		return new ACLManager($this->ruleManager, $this->trashManager, $this->userMappingManager, $this->logger, $this->user, fn (): IRootFolder&MockObject => $rootFolder, null, $perUserMerge);
 	}
 
 	public function testGetACLPermissionsForPathNoRules(): void {