From 0565afa2f9fad26d87bc17ab89544e9a1b81b5b2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Sun, 17 Sep 2023 16:07:24 +0200 Subject: [PATCH 1/3] pass around preload rules in acl wrapper instead of relying on the cache Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 23 +++++++++++++++-------- lib/ACL/ACLManager.php | 25 +++++++++++++++++++++---- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 69da859ae..1f15b3b81 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/lib/ACL/ACLCacheWrapper.php @@ -33,8 +33,12 @@ class ACLCacheWrapper extends CacheWrapper { private $aclManager; private $inShare; - private function getACLPermissionsForPath(string $path) { - $permissions = $this->aclManager->getACLPermissionsForPath($path); + private function getACLPermissionsForPath(string $path, array $rules = []) { + if ($rules) { + $permissions = $this->aclManager->getPermissionsForPathFromRules($path, $rules); + } else { + $permissions = $this->aclManager->getACLPermissionsForPath($path); + } // if there is no read permissions, than deny everything if ($this->inShare) { @@ -52,10 +56,10 @@ public function __construct(ICache $cache, ACLManager $aclManager, bool $inShare $this->inShare = $inShare; } - protected function formatCacheEntry($entry) { + protected function formatCacheEntry($entry, array $rules = []) { if (isset($entry['permissions'])) { $entry['scan_permissions'] = $entry['permissions']; - $entry['permissions'] &= $this->getACLPermissionsForPath($entry['path']); + $entry['permissions'] &= $this->getACLPermissionsForPath($entry['path'], $rules); if (!$entry['permissions']) { return false; } @@ -65,8 +69,10 @@ protected function formatCacheEntry($entry) { public function getFolderContentsById($fileId) { $results = $this->getCache()->getFolderContentsById($fileId); - $this->preloadEntries($results); - $entries = array_map([$this, 'formatCacheEntry'], $results); + $rules = $this->preloadEntries($results); + $entries = array_map(function($entry) use ($rules) { + return $this->formatCacheEntry($entry, $rules); + }, $results); return array_filter(array_filter($entries)); } @@ -90,11 +96,12 @@ public function searchQuery(ISearchQuery $query) { /** * @param ICacheEntry[] $entries + * @return Rule[][] */ - private function preloadEntries(array $entries) { + private function preloadEntries(array $entries): array { $paths = array_map(function (ICacheEntry $entry) { return $entry->getPath(); }, $entries); - $this->aclManager->preloadPaths($paths); + return $this->aclManager->preloadPaths($paths, false); } } diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index 5588e3598..beb37fd6c 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -59,7 +59,7 @@ private function getRootStorageId(): int { * @param array $paths * @return (Rule[])[] */ - private function getRules(array $paths): array { + private function getRules(array $paths, bool $cache = true): array { // beware: adding new rules to the cache besides the cap // might discard former cached entries, so we can't assume they'll stay // cached, so we read everything out initially to be able to return it @@ -74,7 +74,9 @@ private function getRules(array $paths): array { if (!empty($nonCachedPaths)) { $newRules = $this->ruleManager->getRulesForFilesByPath($this->user, $this->getRootStorageId(), $nonCachedPaths); foreach ($newRules as $path => $rulesForPath) { - $this->ruleCache->set($path, $rulesForPath); + if ($cache) { + $this->ruleCache->set($path, $rulesForPath); + } $rules[$path] = $rulesForPath; } } @@ -101,18 +103,33 @@ private function getParents(string $path): array { return $paths; } - public function preloadPaths(array $paths): void { + /** + * @return Rule[][] + */ + public function preloadPaths(array $paths, bool $cache = true): array { $allPaths = []; foreach ($paths as $path) { $allPaths = array_unique(array_merge($allPaths, $this->getParents($path))); } - $this->getRules($allPaths); + return $this->getRules($allPaths, $cache); } public function getACLPermissionsForPath(string $path): int { $path = ltrim($path, '/'); $rules = $this->getRules($this->getParents($path)); + return $this->calculatePermissionsForPath($path, $rules); + } + + public function getPermissionsForPathFromRules(string $path, array $rules): int { + $path = ltrim($path, '/'); + $parents = $this->getParents($path); + // filter to only the rules we care about + $rules = $nonCachedPaths = array_intersect_key($rules, array_flip($parents)); + return $this->calculatePermissionsForPath($path, $rules); + } + + private function calculatePermissionsForPath(string $path, array $rules): int { return array_reduce($rules, function (int $permissions, array $rules): int { $mergedRule = Rule::mergeRules($rules); return $mergedRule->applyPermissions($permissions); From 0d3274e8abcf29d9afe83f83bc2c48aadaf764dd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Sun, 17 Sep 2023 16:56:13 +0200 Subject: [PATCH 2/3] pass around preload rules for root acl instead of relying on the cache Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 6 +++--- lib/Listeners/CircleDestroyedEventListener.php | 1 - lib/Migration/Version1401000Date20230426112001.php | 1 - lib/Migration/Version16000Date20230821085801.php | 1 - lib/Mount/MountProvider.php | 14 ++++++++------ lib/Settings/Admin.php | 1 - lib/Versions/GroupVersion.php | 1 - 7 files changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 1f15b3b81..1c75c4bf7 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/lib/ACL/ACLCacheWrapper.php @@ -30,8 +30,8 @@ use OCP\Files\Search\ISearchQuery; class ACLCacheWrapper extends CacheWrapper { - private $aclManager; - private $inShare; + private ACLManager $aclManager; + private bool $inShare; private function getACLPermissionsForPath(string $path, array $rules = []) { if ($rules) { @@ -70,7 +70,7 @@ protected function formatCacheEntry($entry, array $rules = []) { public function getFolderContentsById($fileId) { $results = $this->getCache()->getFolderContentsById($fileId); $rules = $this->preloadEntries($results); - $entries = array_map(function($entry) use ($rules) { + $entries = array_map(function ($entry) use ($rules) { return $this->formatCacheEntry($entry, $rules); }, $results); return array_filter(array_filter($entries)); diff --git a/lib/Listeners/CircleDestroyedEventListener.php b/lib/Listeners/CircleDestroyedEventListener.php index fcd603965..9b1757392 100644 --- a/lib/Listeners/CircleDestroyedEventListener.php +++ b/lib/Listeners/CircleDestroyedEventListener.php @@ -32,7 +32,6 @@ use OCP\EventDispatcher\IEventListener; class CircleDestroyedEventListener implements IEventListener { - public function __construct( private FolderManager $folderManager, ) { diff --git a/lib/Migration/Version1401000Date20230426112001.php b/lib/Migration/Version1401000Date20230426112001.php index 63ec4e69f..2966193c5 100644 --- a/lib/Migration/Version1401000Date20230426112001.php +++ b/lib/Migration/Version1401000Date20230426112001.php @@ -33,7 +33,6 @@ use OCP\Migration\SimpleMigrationStep; class Version1401000Date20230426112001 extends SimpleMigrationStep { - public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); diff --git a/lib/Migration/Version16000Date20230821085801.php b/lib/Migration/Version16000Date20230821085801.php index e34346bff..557f23dd4 100644 --- a/lib/Migration/Version16000Date20230821085801.php +++ b/lib/Migration/Version16000Date20230821085801.php @@ -36,7 +36,6 @@ * Auto-generated migration step: Please modify to your needs! */ class Version16000Date20230821085801 extends SimpleMigrationStep { - /** * @param IOutput $output * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` diff --git a/lib/Mount/MountProvider.php b/lib/Mount/MountProvider.php index 8fc9e22fa..0e2d71093 100644 --- a/lib/Mount/MountProvider.php +++ b/lib/Mount/MountProvider.php @@ -138,9 +138,9 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { return $this->getJailPath($folder['folder_id']); }, $foldersWithAcl); $aclManager = $this->aclManagerFactory->getACLManager($user, $this->getRootStorageId()); - $aclManager->preloadPaths($aclRootPaths); + $rootRules = $aclManager->preloadPaths($aclRootPaths); - return array_values(array_filter(array_map(function ($folder) use ($user, $loader, $conflicts, $aclManager) { + return array_values(array_filter(array_map(function ($folder) use ($user, $loader, $conflicts, $aclManager, $rootRules) { // check for existing files in the user home and rename them if needed $originalFolderName = $folder['mount_point']; if (in_array($originalFolderName, $conflicts)) { @@ -168,7 +168,8 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { $loader, $folder['acl'], $user, - $aclManager + $aclManager, + $rootRules ); }, $folders))); } @@ -200,7 +201,8 @@ public function getMount( IStorageFactory $loader = null, bool $acl = false, IUser $user = null, - ?ACLManager $aclManager = null + ?ACLManager $aclManager = null, + array $rootRules = [] ): ?IMountPoint { if (!$cacheEntry) { // trigger folder creation @@ -219,12 +221,12 @@ public function getMount( if ($acl && $user) { $inShare = $this->getCurrentUID() === null || $this->getCurrentUID() !== $user->getUID(); $aclManager ??= $this->aclManagerFactory->getACLManager($user, $this->getRootStorageId()); + $aclRootPermissions = $aclManager->getPermissionsForPathFromRules($rootPath, $rootRules); $storage = new ACLStorageWrapper([ 'storage' => $storage, 'acl_manager' => $aclManager, - 'in_share' => $inShare + 'in_share' => $inShare, ]); - $aclRootPermissions = $aclManager->getACLPermissionsForPath($rootPath); $cacheEntry['permissions'] &= $aclRootPermissions; } diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 64c51516c..4052d5b42 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -30,7 +30,6 @@ use OCP\AppFramework\Services\IInitialState; class Admin implements IDelegatedSettings { - public function __construct( private IInitialState $initialState, private ApplicationService $applicationService, diff --git a/lib/Versions/GroupVersion.php b/lib/Versions/GroupVersion.php index 20fda8d72..4715c6181 100644 --- a/lib/Versions/GroupVersion.php +++ b/lib/Versions/GroupVersion.php @@ -30,7 +30,6 @@ use OCP\IUser; class GroupVersion extends Version { - public function __construct( int $timestamp, int $revisionId, From 7cc2c4420a89478af9ee273938446caaf673ede5 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 19 Sep 2023 10:19:40 +0200 Subject: [PATCH 3/3] clearify the acl logic a bit Signed-off-by: Robin Appelman --- lib/ACL/ACLCacheWrapper.php | 2 +- lib/ACL/ACLManager.php | 47 +++++++++++++++++++++++++++---------- lib/ACL/Rule.php | 8 +++++++ lib/Mount/MountProvider.php | 2 +- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/lib/ACL/ACLCacheWrapper.php b/lib/ACL/ACLCacheWrapper.php index 1c75c4bf7..c8d830c99 100644 --- a/lib/ACL/ACLCacheWrapper.php +++ b/lib/ACL/ACLCacheWrapper.php @@ -102,6 +102,6 @@ private function preloadEntries(array $entries): array { $paths = array_map(function (ICacheEntry $entry) { return $entry->getPath(); }, $entries); - return $this->aclManager->preloadPaths($paths, false); + return $this->aclManager->getRelevantRulesForPath($paths, false); } } diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index beb37fd6c..7c5115d03 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -56,8 +56,11 @@ private function getRootStorageId(): int { } /** - * @param array $paths - * @return (Rule[])[] + * Get the list of rules applicable for a set of paths + * + * @param string[] $paths + * @param bool $cache whether to cache the retrieved rules + * @return array sorted parent first */ private function getRules(array $paths, bool $cache = true): array { // beware: adding new rules to the cache besides the cap @@ -87,10 +90,14 @@ private function getRules(array $paths, bool $cache = true): array { } /** + * Get a list of all path that might contain relevant rules when calculating the permissions for a path + * + * This contains the $path itself and any parent folder + * * @param string $path * @return string[] */ - private function getParents(string $path): array { + private function getRelevantPaths(string $path): array { $paths = [$path]; while ($path !== '') { $path = dirname($path); @@ -104,32 +111,46 @@ private function getParents(string $path): array { } /** - * @return Rule[][] + * Get the list of rules applicable for a set of paths, including rules for any parent + * + * @param string[] $paths + * @param bool $cache whether to cache the retrieved rules + * @return array sorted parent first */ - public function preloadPaths(array $paths, bool $cache = true): array { + public function getRelevantRulesForPath(array $paths, bool $cache = true): array { $allPaths = []; foreach ($paths as $path) { - $allPaths = array_unique(array_merge($allPaths, $this->getParents($path))); + $allPaths = array_unique(array_merge($allPaths, $this->getRelevantPaths($path))); } return $this->getRules($allPaths, $cache); } public function getACLPermissionsForPath(string $path): int { $path = ltrim($path, '/'); - $rules = $this->getRules($this->getParents($path)); + $rules = $this->getRules($this->getRelevantPaths($path)); - return $this->calculatePermissionsForPath($path, $rules); + return $this->calculatePermissionsForPath($rules); } + /** + * @param string $path + * @param array $rules list of rules per path + * @return int + */ public function getPermissionsForPathFromRules(string $path, array $rules): int { $path = ltrim($path, '/'); - $parents = $this->getParents($path); - // filter to only the rules we care about - $rules = $nonCachedPaths = array_intersect_key($rules, array_flip($parents)); - return $this->calculatePermissionsForPath($path, $rules); + $relevantPaths = $this->getRelevantPaths($path); + $rules = array_intersect_key($rules, array_flip($relevantPaths)); + return $this->calculatePermissionsForPath($rules); } - private function calculatePermissionsForPath(string $path, array $rules): int { + /** + * @param array $rules list of rules per path, sorted parent first + * @return int + */ + private function calculatePermissionsForPath(array $rules): int { + // first combine all rules with the same path, then apply them on top of the current permissions + // since $rules is sorted parent first rules for subfolders overwrite the rules from the parent return array_reduce($rules, function (int $permissions, array $rules): int { $mergedRule = Rule::mergeRules($rules); return $mergedRule->applyPermissions($permissions); diff --git a/lib/ACL/Rule.php b/lib/ACL/Rule.php index c630a6dac..a538f838b 100644 --- a/lib/ACL/Rule.php +++ b/lib/ACL/Rule.php @@ -70,6 +70,14 @@ public function getPermissions(): int { return $this->permissions; } + /** + * Apply this rule to an existing permission set, returning the resulting permissions + * + * All permissions included in the current mask will overwrite the existing permissions + * + * @param int $permissions + * @return int + */ public function applyPermissions(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 diff --git a/lib/Mount/MountProvider.php b/lib/Mount/MountProvider.php index 0e2d71093..b6eb10f15 100644 --- a/lib/Mount/MountProvider.php +++ b/lib/Mount/MountProvider.php @@ -138,7 +138,7 @@ public function getMountsForUser(IUser $user, IStorageFactory $loader) { return $this->getJailPath($folder['folder_id']); }, $foldersWithAcl); $aclManager = $this->aclManagerFactory->getACLManager($user, $this->getRootStorageId()); - $rootRules = $aclManager->preloadPaths($aclRootPaths); + $rootRules = $aclManager->getRelevantRulesForPath($aclRootPaths); return array_values(array_filter(array_map(function ($folder) use ($user, $loader, $conflicts, $aclManager, $rootRules) { // check for existing files in the user home and rename them if needed