From 8145d2b8af9febdb4208361678deb9a8ace1c21d 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 | 21 ++++++++++++++++--- src/client.js | 3 ++- tests/ACL/ACLManagerTest.php | 5 ++++- 6 files changed, 66 insertions(+), 6 deletions(-) diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index ad1041f4d..9cec787a1 100644 --- a/lib/ACL/ACLManager.php +++ b/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 RuleManager $ruleManager, private TrashManager $trashManager, + private IUserMappingManager $userMappingManager, private LoggerInterface $logger, private IUser $user, private \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 $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 $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 $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 cd5915d3a..cf9f14462 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\IAppConfig; use OCP\IUser; @@ -19,6 +20,7 @@ public function __construct( private TrashManager $trashManager, private IAppConfig $config, private LoggerInterface $logger, + private IUserMappingManager $userMappingManager, private \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, diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 5ca54e458..f21e66595 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -218,6 +218,7 @@ public function register(IRegistrationContext $context): void { $c->get(TrashManager::class), $c->get(IAppConfig::class), $c->get(LoggerInterface::class), + $c->get(IUserMappingManager::class), $rootFolderProvider ); }); diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index a27a68e16..09afdc099 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -9,6 +9,7 @@ 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; @@ -16,9 +17,11 @@ 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 IUserSession $userSession, private FolderManager $folderManager, private IEventDispatcher $eventDispatcher, + private ACLManagerFactory $aclManagerFactory, + private 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), 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 136d4865b..06be3cf83 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; @@ -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 {