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

Signed-off-by: Robin Appelman <[email protected]>
Signed-off-by: provokateurin <[email protected]>

lib/ACL/ACLManager.php

@@ -9,6 +9,7 @@
 namespace OCA\GroupFolders\ACL;
 
 use OC\Cache\CappedMemoryCache;
+use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
 use OCA\GroupFolders\Trash\TrashManager;
 use OCP\Constants;
 use OCP\Files\IRootFolder;
@@ -23,6 +24,7 @@ class ACLManager {
 	public function __construct(
 		private RuleManager  $ruleManager,
 		private TrashManager $trashManager,
+		private IUserMappingManager $userMappingManager,
 		private LoggerInterface $logger,
 		private IUser        $user,
 		callable             $rootFolderProvider,
@@ -91,7 +93,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 string $path
 	 * @param array<string, Rule[]> $rules list of rules per path
@@ -233,4 +249,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\IConfig;
 use OCP\IUser;
@@ -21,6 +22,7 @@ public function __construct(
 		private TrashManager $trashManager,
 		private IConfig $config,
 		private LoggerInterface $logger,
+		private IUserMappingManager $userMappingManager,
 		callable $rootFolderProvider,
 	) {
 		$this->rootFolderProvider = $rootFolderProvider;
@@ -30,6 +32,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

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

lib/DAV/ACLPlugin.php

@@ -9,15 +9,18 @@
 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\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;
@@ -40,6 +43,8 @@ public function __construct(
 		private IUserSession $userSession,
 		private FolderManager $folderManager,
 		private IEventDispatcher $eventDispatcher,
+		private ACLManagerFactory $aclManagerFactory,
+		private IL10N $l10n,
 	) {
 	}
 
@@ -54,7 +59,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']);
@@ -181,6 +186,10 @@ public function propPatch(string $path, PropPatch $propPatch): void {
 			if (!$mount instanceof GroupMountPoint) {
 				return false;
 			}
+			if ($this->user === null) {
+				return false;
+			}
+
 			$path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/');
 
 			// populate fileid in rules
@@ -210,6 +219,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]),
 				function (array $rules, array $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;
@@ -23,6 +24,7 @@
 class ACLManagerTest extends TestCase {
 	private RuleManager $ruleManager;
 	private TrashManager $trashManager;
+	private IUserMappingManager $userMappingManager;
 	private LoggerInterface $logger;
 	private IUser $user;
 	private ACLManager $aclManager;
@@ -36,6 +38,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, function () use ($rootFolder) {
+		return new ACLManager($this->ruleManager, $this->trashManager, $this->userMappingManager, $this->logger, $this->user, function () use ($rootFolder) {
 			return $rootFolder;
 		}, null, $perUserMerge);
 	}