From 9b8e691aabb3760c762cd8b3f09babe521c9ab64 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 14 May 2024 17:36:44 +0200 Subject: [PATCH] fix(ACL): Add check to prevent users from revoking their own access Signed-off-by: Robin Appelman Signed-off-by: provokateurin --- lib/ACL/ACLManager.php | 39 ++++++++++++++++++++++++++++++++++- lib/ACL/ACLManagerFactory.php | 3 +++ lib/AppInfo/Application.php | 1 + lib/DAV/ACLPlugin.php | 17 ++++++++++++++- src/client.js | 3 ++- tests/ACL/ACLManagerTest.php | 5 ++++- 6 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index 0d0c5aa89..602ae58b0 100644 --- a/lib/ACL/ACLManager.php +++ b/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 $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 $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 $rules + * @return list + */ + 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; + })); + } } diff --git a/lib/ACL/ACLManagerFactory.php b/lib/ACL/ACLManagerFactory.php index ccd93fd29..8be5300a0 100644 --- a/lib/ACL/ACLManagerFactory.php +++ b/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, diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index fd4cfcd82..2a4bc650c 100644 --- a/lib/AppInfo/Application.php +++ b/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 ); }); diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index 923060f2e..9a193f7f5 100644 --- a/lib/DAV/ACLPlugin.php +++ b/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) { diff --git a/src/client.js b/src/client.js index ca8757607..02e9ee61a 100644 --- a/src/client.js +++ b/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 diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php index a7c591bc8..a6d6cb179 100644 --- a/tests/ACL/ACLManagerTest.php +++ b/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); }