From bc89619b493eb1b94de0e149412de8ba7912d1a5 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 15 Dec 2025 19:45:27 -0500 Subject: [PATCH 01/16] docs: Add docblock for GroupFolderNode class Signed-off-by: Josh --- lib/DAV/GroupFolderNode.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/DAV/GroupFolderNode.php b/lib/DAV/GroupFolderNode.php index e63adc038..1d9950519 100644 --- a/lib/DAV/GroupFolderNode.php +++ b/lib/DAV/GroupFolderNode.php @@ -12,6 +12,13 @@ use OCA\DAV\Connector\Sabre\Directory; use OCP\Files\FileInfo; +/** + * WebDAV node representing a group folder directory. + * + * Extends the standard Directory node to track the associated group folder ID, + * allowing the system to identify and apply group folder-specific permissions + * and logic when accessed via WebDAV. + */ class GroupFolderNode extends Directory { public function __construct( View $view, From fe0ec1a7bb518fb665de5f1d44c29bdc7bf7cc72 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 15 Dec 2025 21:28:10 -0500 Subject: [PATCH 02/16] refactor(dav): clean up GroupFoldersHome - add type hints, unify errors, simplify code, and improve docs Improved readability and maintainability: added type hints, made error messages consistent, simplified code by removing unnecessary storage checks and redundant logic, clarified documentation, and aligned implementation with Nextcloud conventions. Signed-off-by: Josh --- lib/DAV/GroupFoldersHome.php | 38 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/DAV/GroupFoldersHome.php b/lib/DAV/GroupFoldersHome.php index 72c92e1f9..9dc5daceb 100644 --- a/lib/DAV/GroupFoldersHome.php +++ b/lib/DAV/GroupFoldersHome.php @@ -18,6 +18,13 @@ use Sabre\DAV\Exception\NotFound; use Sabre\DAV\ICollection; +/** + * WebDAV collection representing a user's group folders home directory. + * + * Serves as a container for all group folders accessible to a specific user, + * providing read-only access to the list of group folders they can access. + * Each child node is a GroupFolderNode representing an individual group folder. + */ class GroupFoldersHome implements ICollection { public function __construct( private array $principalInfo, @@ -28,7 +35,7 @@ public function __construct( } public function delete(): never { - throw new Forbidden(); + throw new Forbidden('Permission denied to delete this folder'); } public function getName(): string { @@ -36,24 +43,19 @@ public function getName(): string { return $name; } - public function setName($name): never { + public function setName(string $name): never { throw new Forbidden('Permission denied to rename this folder'); } - public function createFile($name, $data = null): never { - throw new Forbidden('Not allowed to create files in this folder'); + public function createFile(string $name, $data = null): never { + throw new Forbidden('Permission denied to create files in this folder'); } - public function createDirectory($name): never { + public function createDirectory(string $name): never { throw new Forbidden('Permission denied to create folders in this folder'); } private function getFolder(string $name): ?FolderDefinition { - $storageId = $this->rootFolder->getMountPoint()->getNumericStorageId(); - if ($storageId === null) { - return null; - } - $folders = $this->folderManager->getFoldersForUser($this->user); foreach ($folders as $folder) { if (basename($folder->mountPoint) === $name) { @@ -64,6 +66,11 @@ private function getFolder(string $name): ?FolderDefinition { return null; } + /** + * Creates a GroupFolderNode for the given folder definition. + * + * @throws RuntimeException If the filesystem view cannot be obtained + */ private function getDirectoryForFolder(FolderDefinition $folder): GroupFolderNode { $userHome = '/' . $this->user->getUID() . '/files'; $node = $this->rootFolder->get($userHome . '/' . $folder->mountPoint); @@ -76,9 +83,9 @@ private function getDirectoryForFolder(FolderDefinition $folder): GroupFolderNod return new GroupFolderNode($view, $node, $folder->id); } - public function getChild($name): GroupFolderNode { + public function getChild(string $name): GroupFolderNode { $folder = $this->getFolder($name); - if ($folder) { + if ($folder !== null) { return $this->getDirectoryForFolder($folder); } @@ -89,11 +96,6 @@ public function getChild($name): GroupFolderNode { * @return GroupFolderNode[] */ public function getChildren(): array { - $storageId = $this->rootFolder->getMountPoint()->getNumericStorageId(); - if ($storageId === null) { - return []; - } - $folders = $this->folderManager->getFoldersForUser($this->user); // Filter out non top-level folders @@ -102,7 +104,7 @@ public function getChildren(): array { return array_map($this->getDirectoryForFolder(...), $folders); } - public function childExists($name): bool { + public function childExists(string $name): bool { return $this->getFolder($name) !== null; } From 226d4762771b0c2e4c6c528418ab309f06708a10 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 15 Dec 2025 22:20:27 -0500 Subject: [PATCH 03/16] docs: docblock for getChildForPrincipal class Signed-off-by: Josh --- lib/DAV/RootCollection.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/DAV/RootCollection.php b/lib/DAV/RootCollection.php index 1861870a0..85a9f6b33 100644 --- a/lib/DAV/RootCollection.php +++ b/lib/DAV/RootCollection.php @@ -11,9 +11,15 @@ use OCA\GroupFolders\Folder\FolderManager; use OCP\Files\IRootFolder; use OCP\IUserSession; +use Sabre\DAV\Exception\Forbidden; use Sabre\DAVACL\AbstractPrincipalCollection; use Sabre\DAVACL\PrincipalBackend; +/** + * WebDAV root collection for the GroupFolders app. + * + * Provides access to user principal nodes representing each user's group folders home. + */ class RootCollection extends AbstractPrincipalCollection { public function __construct( private readonly IUserSession $userSession, @@ -25,17 +31,20 @@ public function __construct( } /** - * This method returns a node for a principal. + * Returns a GroupFoldersHome for the principal if the authenticated user matches. * * The passed array contains principal information, and is guaranteed to * at least contain a uri item. Other properties may or may not be * supplied by the authentication backend. + * + * @throws \Sabre\DAV\Exception\Forbidden If the principal does not match the currently logged-in user. */ public function getChildForPrincipal(array $principalInfo): GroupFoldersHome { [, $name] = \Sabre\Uri\split($principalInfo['uri']); $user = $this->userSession->getUser(); + if (is_null($user) || $name !== $user->getUID()) { - throw new \Sabre\DAV\Exception\Forbidden(); + throw new Forbidden('Access to this groupfolders principal is not allowed for this user.'); } return new GroupFoldersHome($principalInfo, $this->folderManager, $this->rootFolder, $user); From 9ac4b856d06e100940f0674275a234dc3674261b Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 16 Dec 2025 11:02:49 -0500 Subject: [PATCH 04/16] refactor(dav): improve readability of PropFindPlugin - Minor docblock imporvements - Extract mount path calculation to dedicated method Signed-off-by: Josh --- lib/DAV/PropFindPlugin.php | 44 +++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/DAV/PropFindPlugin.php b/lib/DAV/PropFindPlugin.php index 630a848ac..b055cf478 100644 --- a/lib/DAV/PropFindPlugin.php +++ b/lib/DAV/PropFindPlugin.php @@ -16,13 +16,21 @@ use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; +/** + * SabreDAV plugin that adds group folder metadata to PROPFIND responses. + * + * Adds the mount point and group folder ID as custom WebDAV properties for group folder nodes. + */ class PropFindPlugin extends ServerPlugin { private ?Folder $userFolder = null; public const MOUNT_POINT_PROPERTYNAME = '{http://nextcloud.org/ns}mount-point'; public const GROUP_FOLDER_ID_PROPERTYNAME = '{http://nextcloud.org/ns}group-folder-id'; - public function __construct(IRootFolder $rootFolder, IUserSession $userSession) { + public function __construct( + IRootFolder $rootFolder, + IUserSession $userSession + ) { $user = $userSession->getUser(); if ($user === null) { return; @@ -41,20 +49,30 @@ public function initialize(Server $server): void { } public function propFind(PropFind $propFind, INode $node): void { - if ($this->userFolder === null) { + if (!($node instanceof GroupFolderNode) || $this->userFolder === null) { return; } - if ($node instanceof GroupFolderNode) { - $propFind->handle( - self::MOUNT_POINT_PROPERTYNAME, - /** @psalm-suppress PossiblyNullReference Null already checked above */ - fn () => $this->userFolder->getRelativePath($node->getFileInfo()->getMountPoint()->getMountPoint()) - ); - $propFind->handle( - self::GROUP_FOLDER_ID_PROPERTYNAME, - fn (): int => $node->getFolderId() - ); - } + $propFind->handle( + self::MOUNT_POINT_PROPERTYNAME, + fn() => $this->getRelativeMountPointPath($node) + ); + $propFind->handle( + self::GROUP_FOLDER_ID_PROPERTYNAME, + fn (): int => $node->getFolderId() + ); + } + + /** + * Compute the path of the mount point relative to the root of the current user's folder. + * + * TODO: This may be a candidate for a utility function in GF or API addition in core. + */ + private function getRelativeMountPointPath(GroupFolderNode $node): ?string { + // TODO: Seems there could be some more defensive null/error handling here (perhaps throwing a 404/not found + logging) + $fileInfo = $node->getFileInfo(); + $mount = $fileInfo->getMountPoint(); + $mountPointPath = $mount->getMountPoint(); + return $this->userFolder->getRelativePath($mountPointPath); } } From 423bcc7bfccda212bcac6261126e3a7590591ed1 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 08:01:14 -0500 Subject: [PATCH 05/16] chore: Add TODOs for logging silent edge cases Added TODO comments to log silent edge cases Signed-off-by: Josh --- lib/DAV/PropFindPlugin.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/DAV/PropFindPlugin.php b/lib/DAV/PropFindPlugin.php index b055cf478..bb6fe1222 100644 --- a/lib/DAV/PropFindPlugin.php +++ b/lib/DAV/PropFindPlugin.php @@ -33,13 +33,17 @@ public function __construct( ) { $user = $userSession->getUser(); if ($user === null) { + // TODO: make this "silent" edge case visible in logs return; } $this->userFolder = $rootFolder->getUserFolder($user->getUID()); + if ($this->userFolder === null) { + // TODO: make this "silent" edge case visible in logs + return; + } } - public function getPluginName(): string { return 'groupFoldersDavPlugin'; } From 0ad5a776281a66cb8395946d5c3ba9e165f284b0 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 08:28:21 -0500 Subject: [PATCH 06/16] refactor(dav): Formatting and docs for ACLPlugin Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 388 ++++++++++++++++++++++++------------------ 1 file changed, 225 insertions(+), 163 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index 0fa0851bc..ee4183d71 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -29,6 +29,11 @@ use Sabre\DAV\ServerPlugin; use Sabre\Xml\Reader; +/** + * SabreDAV plugin for exposing and updating advanced ACL properties. + * + * Handles WebDAV PROPFIND and PROPPATCH events for Nextcloud group folders with granular access controls. + */ class ACLPlugin extends ServerPlugin { public const ACL_ENABLED = '{http://nextcloud.org/ns}acl-enabled'; public const ACL_CAN_MANAGE = '{http://nextcloud.org/ns}acl-can-manage'; @@ -52,42 +57,18 @@ public function __construct( ) { } - private function isAdmin(IUser $user, string $path): bool { - $folderId = $this->folderManager->getFolderByPath($path); - - if (!isset($this->canManageACL[$folderId])) { - $this->canManageACL[$folderId] = $this->folderManager->canManageACL($folderId, $user); - } - - return $this->canManageACL[$folderId]; - } - public function initialize(Server $server): void { $this->server = $server; - $this->user = $this->userSession->getUser(); + $this->user = $this->userSession->getUser(); // move to constructor? $this->server->on('propFind', $this->propFind(...)); $this->server->on('propPatch', $this->propPatch(...)); - $this->server->xml->elementMap[Rule::ACL] = Rule::class; - $this->server->xml->elementMap[self::ACL_LIST] = fn (Reader $reader): array => \Sabre\Xml\Deserializer\repeatingElements($reader, Rule::ACL); - } - - /** - * @return string[] - */ - private function getParents(string $path): array { - $paths = []; - while ($path !== '') { - $path = dirname($path); - if ($path === '.' || $path === '/') { - $path = ''; - } - - $paths[] = $path; - } - - return $paths; + $this->server->xml->elementMap[Rule::ACL] = + Rule::class; + $this->server->xml->elementMap[self::ACL_LIST] = + fn (Reader $reader): array => + \Sabre\Xml\Deserializer\repeatingElements($reader, Rule::ACL); } public function propFind(PropFind $propFind, INode $node): void { @@ -101,94 +82,120 @@ public function propFind(PropFind $propFind, INode $node): void { return; } - $propFind->handle(self::ACL_LIST, function () use ($fileInfo, $mount): ?array { - // Happens when sharing with a remote instance - if ($this->user === null) { - return []; - } + $propFind->handle( + self::ACL_LIST, + function () use ($fileInfo, $mount): ?array { + // Happens when sharing with a remote instance + if ($this->user === null) { + return []; + } - $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - if ($this->isAdmin($this->user, $fileInfo->getPath())) { - $rules = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]); - } else { - $rules = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), [$path]); - } + $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - return array_pop($rules); - }); + if ($this->isAdmin($this->user, $fileInfo->getPath())) { + $rules = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]); + } else { + $rules = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), [$path]); + } - $propFind->handle(self::INHERITED_ACL_LIST, function () use ($fileInfo, $mount): array { - // Happens when sharing with a remote instance - if ($this->user === null) { - return []; - } + return array_pop($rules); + }); - $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); - $parentPaths = array_map(fn (string $internalPath): string => trim($mount->getSourcePath() . '/' . $internalPath, '/'), $parentInternalPaths); - if ($this->isAdmin($this->user, $fileInfo->getPath())) { - $rulesByPath = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), $parentPaths); - } else { - $rulesByPath = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), $parentPaths); - } + $propFind->handle( + self::INHERITED_ACL_LIST, + function () use ($fileInfo, $mount): array { + // Happens when sharing with a remote instance + if ($this->user === null) { + return []; + } - $aclManager = $this->aclManagerFactory->getACLManager($this->user); + $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); + $parentPaths = array_map( + fn (string $internalPath): string => trim($mount->getSourcePath() . '/' . $internalPath, '/'), + $parentInternalPaths + ); + // Also include the mount root + $parentPaths[] = $mount->getSourcePath(); + + if ($this->isAdmin($this->user, $fileInfo->getPath())) { + $rulesByPath = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), $parentPaths); + } else { + $rulesByPath = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), $parentPaths); + } - ksort($rulesByPath); - $inheritedPermissionsByMapping = []; - $inheritedMaskByMapping = []; - $mappings = []; - foreach ($rulesByPath as $rules) { - foreach ($rules as $rule) { - $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); - if (!isset($mappings[$mappingKey])) { - $mappings[$mappingKey] = $rule->getUserMapping(); + $aclManager = $this->aclManagerFactory->getACLManager($this->user); + + ksort($rulesByPath); + $inheritedPermissionsByMapping = []; + $inheritedMaskByMapping = []; + $mappings = []; + foreach ($rulesByPath as $rules) { + foreach ($rules as $rule) { + $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + if (!isset($mappings[$mappingKey])) { + $mappings[$mappingKey] = $rule->getUserMapping(); + } + + if (!isset($inheritedPermissionsByMapping[$mappingKey])) { + $inheritedPermissionsByMapping[$mappingKey] = $aclManager->getBasePermission($mount->getFolderId()); + } + + if (!isset($inheritedMaskByMapping[$mappingKey])) { + $inheritedMaskByMapping[$mappingKey] = 0; + } + + $inheritedPermissionsByMapping[$mappingKey] = $rule->applyPermissions($inheritedPermissionsByMapping[$mappingKey]); + $inheritedMaskByMapping[$mappingKey] |= $rule->getMask(); } + } - if (!isset($inheritedPermissionsByMapping[$mappingKey])) { - $inheritedPermissionsByMapping[$mappingKey] = $aclManager->getBasePermission($mount->getFolderId()); - } + return array_map( + fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( + $mapping, + $fileInfo->getId(), + $mask, + $permissions + ), + $mappings, + $inheritedPermissionsByMapping, + $inheritedMaskByMapping + ); + } + ); - if (!isset($inheritedMaskByMapping[$mappingKey])) { - $inheritedMaskByMapping[$mappingKey] = 0; - } + $propFind->handle( + self::GROUP_FOLDER_ID, + fn (): int => $this->folderManager->getFolderByPath($fileInfo->getPath()) + ); - $inheritedPermissionsByMapping[$mappingKey] = $rule->applyPermissions($inheritedPermissionsByMapping[$mappingKey]); - $inheritedMaskByMapping[$mappingKey] |= $rule->getMask(); - } + $propFind->handle( + self::ACL_ENABLED, + function () use ($fileInfo): bool { + $folderId = $this->folderManager->getFolderByPath($fileInfo->getPath()); + return $this->folderManager->getFolderAclEnabled($folderId); } + ); - return array_map(fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( - $mapping, - $fileInfo->getId(), - $mask, - $permissions - ), $mappings, $inheritedPermissionsByMapping, $inheritedMaskByMapping); - }); - - $propFind->handle(self::GROUP_FOLDER_ID, fn (): int => $this->folderManager->getFolderByPath($fileInfo->getPath())); - - $propFind->handle(self::ACL_ENABLED, function () use ($fileInfo): bool { - $folderId = $this->folderManager->getFolderByPath($fileInfo->getPath()); - return $this->folderManager->getFolderAclEnabled($folderId); - }); - - $propFind->handle(self::ACL_CAN_MANAGE, function () use ($fileInfo): bool { - // Happens when sharing with a remote instance - if ($this->user === null) { - return false; + $propFind->handle( + self::ACL_CAN_MANAGE, + function () use ($fileInfo): bool { + // Happens when sharing with a remote instance + if ($this->user === null) { + return false; + } + return $this->isAdmin($this->user, $fileInfo->getPath()); } + ); - return $this->isAdmin($this->user, $fileInfo->getPath()); - }); - - $propFind->handle(self::ACL_BASE_PERMISSION_PROPERTYNAME, function () use ($mount): int { - // Happens when sharing with a remote instance - if ($this->user === null) { - return Constants::PERMISSION_ALL; + $propFind->handle( + self::ACL_BASE_PERMISSION_PROPERTYNAME, + function () use ($mount): int { + // Happens when sharing with a remote instance + if ($this->user === null) { + return Constants::PERMISSION_ALL; + } + return $this->aclManagerFactory->getACLManager($this->user)->getBasePermission($mount->getFolderId()); } - - return $this->aclManagerFactory->getACLManager($this->user)->getBasePermission($mount->getFolderId()); - } ); } @@ -203,87 +210,142 @@ public function propPatch(string $path, PropPatch $propPatch): void { } $node = $this->server->tree->getNodeForPath($path); + if (!$node instanceof Node) { return; } $fileInfo = $node->getFileInfo(); $mount = $fileInfo->getMountPoint(); - if (!$mount instanceof GroupMountPoint || !$this->isAdmin($this->user, $fileInfo->getPath())) { + + if (!$mount instanceof GroupMountPoint) { + return; + } + + if (!$this->isAdmin($this->user, $fileInfo->getPath())) { return; } // Mapping the old property to the new property. - $propPatch->handle(self::ACL_LIST, function (array $rawRules) use ($path): bool { - $node = $this->server->tree->getNodeForPath($path); - if (!$node instanceof Node) { - return false; - } + $propPatch->handle( + self::ACL_LIST, + function (array $rawRules) use ($path): bool { + $node = $this->server->tree->getNodeForPath($path); + if (!$node instanceof Node) { + return false; + } - $fileInfo = $node->getFileInfo(); - $mount = $fileInfo->getMountPoint(); - if (!$mount instanceof GroupMountPoint) { - return false; - } + $fileInfo = $node->getFileInfo(); + $mount = $fileInfo->getMountPoint(); + if (!$mount instanceof GroupMountPoint) { + return false; + } - if ($this->user === null) { - return false; - } + if ($this->user === null) { + return false; + } - $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - - // populate fileid in rules - $rules = array_values(array_map(fn (Rule $rule): Rule => new Rule( - $rule->getUserMapping(), - $fileInfo->getId(), - $rule->getMask(), - $rule->getPermissions() - ), $rawRules)); - - $formattedRules = array_map(fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), $rules); - if (count($formattedRules)) { - $formattedRules = implode(', ', $formattedRules); - $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', [ - $fileInfo->getInternalPath(), - $mount->getFolderId(), - $formattedRules, - ])); - } else { - $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was cleared', [ - $fileInfo->getInternalPath(), - $mount->getFolderId(), - ])); - } + $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + + // populate fileid in rules + $rules = array_values(array_map(fn (Rule $rule): Rule => new Rule( + $rule->getUserMapping(), + $fileInfo->getId(), + $rule->getMask(), + $rule->getPermissions() + ), $rawRules)); + + $formattedRules = array_map(fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), $rules); + if (count($formattedRules)) { + $formattedRules = implode(', ', $formattedRules); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', [ + $fileInfo->getInternalPath(), + $mount->getFolderId(), + $formattedRules, + ])); + } else { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was cleared', [ + $fileInfo->getInternalPath(), + $mount->getFolderId(), + ])); + } - $aclManager = $this->aclManagerFactory->getACLManager($this->user); - $newPermissions = $aclManager->testACLPermissionsForPath($mount->getFolderId(), $mount->getNumericStorageId(), $path, $rules); - if (!($newPermissions & Constants::PERMISSION_READ)) { - throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); - } + $aclManager = $this->aclManagerFactory->getACLManager($this->user); + $newPermissions = $aclManager->testACLPermissionsForPath($mount->getFolderId(), $mount->getNumericStorageId(), $path, $rules); + if (!($newPermissions & Constants::PERMISSION_READ)) { + throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); + } - $existingRules = array_reduce( - $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]), - array_merge(...), - [] - ); + $existingRules = array_reduce( + $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]), + array_merge(...), + [] + ); - $deletedRules = array_udiff($existingRules, $rules, fn (Rule $obj_a, Rule $obj_b): int => ( - $obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType() - && $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId() - ) ? 0 : -1); - foreach ($deletedRules as $deletedRule) { - $this->ruleManager->deleteRule($deletedRule); - } + $deletedRules = array_udiff($existingRules, $rules, fn (Rule $obj_a, Rule $obj_b): int => ( + $obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType() + && $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId() + ) ? 0 : -1); + foreach ($deletedRules as $deletedRule) { + $this->ruleManager->deleteRule($deletedRule); + } + + foreach ($rules as $rule) { + $this->ruleManager->saveRule($rule); + } + + $node->getNode()->getStorage()->getPropagator()->propagateChange($fileInfo->getInternalPath(), $fileInfo->getMtime()); - foreach ($rules as $rule) { - $this->ruleManager->saveRule($rule); + return true; } + ); + } + /** + * Checks if the given user has admin (ACL management) rights for the group folder at the given path. + * + * Caches the result per folder ID for efficiency. + * + * @param IUser $user The user to check. + * @param string $path The full path to a file or folder inside a group folder. + * @return bool True if the user can manage ACLs for the group folder at the given path, false otherwise. + * @throws \OCP\Files\NotFoundException If the path does not exist or is not part of a group folder. + */ + private function isAdmin(IUser $user, string $path): bool { + // TODO: catch/handle gracefully if folder disappeared between node fetch and this check (i.e. by another user / session) + $folderId = $this->folderManager->getFolderByPath($path); + + if (isset($this->canManageACL[$folderId])) { + return $this->canManageACL[$folderId]; + } + + $canManage = $this->folderManager->canManageACL($folderId, $user); + $this->canManageACL[$folderId] = $canManage; + return $canManage; + } - $node->getNode()->getStorage()->getPropagator()->propagateChange($fileInfo->getInternalPath(), $fileInfo->getMtime()); + /** + * Returns all parent directory paths for the given path, based solely on the path itself. + * + * The array is ordered from immediate parent upward, excluding the original path. + * + * Example: + * getParents('a/b/c.txt') returns ['a/b', 'a'] + * + * Note: Callers should add contextual parents (such as mount points or absolute roots) if needed. + * + * @param string $path Path to a file or directory. + * @return string[] Parent directory paths, from closest to furthest. + */ + private function getParents(string $path): array { + $parents = []; + $parent = dirname($path); + while ($parent !== '' && $parent !== '.' && $parent !== '/') { + $parents[] = $parent; + $parent = dirname($parent); + } - return true; - }); + return $parents; } } From e0c7a1548b8f37c743013cff529e8018ac402dd9 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 08:55:57 -0500 Subject: [PATCH 07/16] chore: set userFolder prop to readonly Signed-off-by: Josh --- lib/DAV/PropFindPlugin.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/DAV/PropFindPlugin.php b/lib/DAV/PropFindPlugin.php index bb6fe1222..fff1d981a 100644 --- a/lib/DAV/PropFindPlugin.php +++ b/lib/DAV/PropFindPlugin.php @@ -22,7 +22,7 @@ * Adds the mount point and group folder ID as custom WebDAV properties for group folder nodes. */ class PropFindPlugin extends ServerPlugin { - private ?Folder $userFolder = null; + private readonly ?Folder $userFolder = null; public const MOUNT_POINT_PROPERTYNAME = '{http://nextcloud.org/ns}mount-point'; public const GROUP_FOLDER_ID_PROPERTYNAME = '{http://nextcloud.org/ns}group-folder-id'; @@ -61,6 +61,7 @@ public function propFind(PropFind $propFind, INode $node): void { self::MOUNT_POINT_PROPERTYNAME, fn() => $this->getRelativeMountPointPath($node) ); + $propFind->handle( self::GROUP_FOLDER_ID_PROPERTYNAME, fn (): int => $node->getFolderId() From 32dd476696c8c5b30a50ace3eefe886348a788f1 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 09:12:22 -0500 Subject: [PATCH 08/16] refactor(dav): make user readonly move to const in ACLPlugin Refactor user property to be readonly and initialize in constructor. Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index ee4183d71..c55cbb029 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -43,7 +43,7 @@ class ACLPlugin extends ServerPlugin { public const ACL_BASE_PERMISSION_PROPERTYNAME = '{http://nextcloud.org/ns}acl-base-permission'; private ?Server $server = null; - private ?IUser $user = null; + private readonly ?IUser $user; /** @var array */ private array $canManageACL = []; @@ -55,11 +55,14 @@ public function __construct( private readonly ACLManagerFactory $aclManagerFactory, private readonly IL10N $l10n, ) { + $this->user = $this->userSession->getUser(); + if ($this->user === null) { + return; + } } public function initialize(Server $server): void { $this->server = $server; - $this->user = $this->userSession->getUser(); // move to constructor? $this->server->on('propFind', $this->propFind(...)); $this->server->on('propPatch', $this->propPatch(...)); From 93e92e4dcc8dc48b27eb7b95544bbcce1fedabfc Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 10:36:45 -0500 Subject: [PATCH 09/16] refactor(dav): ACLPlugin.php for improved readability Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 132 ++++++++++++++++++++++++++++-------------- 1 file changed, 87 insertions(+), 45 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index c55cbb029..90b13c00f 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -96,9 +96,16 @@ function () use ($fileInfo, $mount): ?array { $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); if ($this->isAdmin($this->user, $fileInfo->getPath())) { - $rules = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]); + $rules = $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + [$path] + ); } else { - $rules = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), [$path]); + $rules = $this->ruleManager->getRulesForFilesByPath( + $this->user, + $mount->getNumericStorageId(), + [$path] + ); } return array_pop($rules); @@ -121,9 +128,16 @@ function () use ($fileInfo, $mount): array { $parentPaths[] = $mount->getSourcePath(); if ($this->isAdmin($this->user, $fileInfo->getPath())) { - $rulesByPath = $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), $parentPaths); + $rulesByPath = $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + $parentPaths + ); } else { - $rulesByPath = $this->ruleManager->getRulesForFilesByPath($this->user, $mount->getNumericStorageId(), $parentPaths); + $rulesByPath = $this->ruleManager->getRulesForFilesByPath( + $this->user, + $mount->getNumericStorageId(), + $parentPaths + ); } $aclManager = $this->aclManagerFactory->getACLManager($this->user); @@ -132,9 +146,11 @@ function () use ($fileInfo, $mount): array { $inheritedPermissionsByMapping = []; $inheritedMaskByMapping = []; $mappings = []; + foreach ($rulesByPath as $rules) { foreach ($rules as $rule) { $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + if (!isset($mappings[$mappingKey])) { $mappings[$mappingKey] = $rule->getUserMapping(); } @@ -195,9 +211,11 @@ function () use ($fileInfo): bool { function () use ($mount): int { // Happens when sharing with a remote instance if ($this->user === null) { - return Constants::PERMISSION_ALL; + return Constants::PERMISSION_ALL; // ??? } - return $this->aclManagerFactory->getACLManager($this->user)->getBasePermission($mount->getFolderId()); + return $this->aclManagerFactory + ->getACLManager($this->user) + ->getBasePermission($mount->getFolderId()); } ); } @@ -232,64 +250,82 @@ public function propPatch(string $path, PropPatch $propPatch): void { // Mapping the old property to the new property. $propPatch->handle( self::ACL_LIST, - function (array $rawRules) use ($path): bool { - $node = $this->server->tree->getNodeForPath($path); - if (!$node instanceof Node) { - return false; - } - - $fileInfo = $node->getFileInfo(); - $mount = $fileInfo->getMountPoint(); - if (!$mount instanceof GroupMountPoint) { - return false; - } - - if ($this->user === null) { - return false; - } + function (array $rawRules) use ($node, $fileInfo, $mount, $path): bool { $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); // populate fileid in rules - $rules = array_values(array_map(fn (Rule $rule): Rule => new Rule( - $rule->getUserMapping(), - $fileInfo->getId(), - $rule->getMask(), - $rule->getPermissions() - ), $rawRules)); - - $formattedRules = array_map(fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), $rules); + $rules = array_values( + array_map( + fn (Rule $rule): Rule => new Rule( + $rule->getUserMapping(), + $fileInfo->getId(), + $rule->getMask(), + $rule->getPermissions() + ), + $rawRules + ) + ); + + $formattedRules = array_map( + fn (Rule $rule): string => + $rule->getUserMapping()->getType() + . ' ' + . $rule->getUserMapping()->getDisplayName() + . ': ' . $rule->formatPermissions(), + $rules + ); + if (count($formattedRules)) { $formattedRules = implode(', ', $formattedRules); - $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', [ - $fileInfo->getInternalPath(), - $mount->getFolderId(), - $formattedRules, - ])); + $this->eventDispatcher->dispatchTyped( + new CriticalActionPerformedEvent + ( + 'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', + [ $fileInfo->getInternalPath(), $mount->getFolderId(), $formattedRules ] + ) + ); } else { - $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The advanced permissions for "%s" in Team folder with ID %d was cleared', [ - $fileInfo->getInternalPath(), - $mount->getFolderId(), - ])); + $this->eventDispatcher->dispatchTyped( + new CriticalActionPerformedEvent + ( + 'The advanced permissions for "%s" in Team folder with ID %d was cleared', + [ $fileInfo->getInternalPath(), $mount->getFolderId() ] + ) + ); } $aclManager = $this->aclManagerFactory->getACLManager($this->user); - $newPermissions = $aclManager->testACLPermissionsForPath($mount->getFolderId(), $mount->getNumericStorageId(), $path, $rules); + $newPermissions = $aclManager->testACLPermissionsForPath( + $mount->getFolderId(), + $mount->getNumericStorageId(), + $path, + $rules + ); + if (!($newPermissions & Constants::PERMISSION_READ)) { throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); } $existingRules = array_reduce( - $this->ruleManager->getAllRulesForPaths($mount->getNumericStorageId(), [$path]), + $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + [$path] + ), array_merge(...), [] ); - $deletedRules = array_udiff($existingRules, $rules, fn (Rule $obj_a, Rule $obj_b): int => ( - $obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType() - && $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId() - ) ? 0 : -1); + $deletedRules = array_udiff( + $existingRules, + $rules, + fn (Rule $obj_a, Rule $obj_b): int => ( + $obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType() + && $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId() + ) ? 0 : -1 + ); + foreach ($deletedRules as $deletedRule) { $this->ruleManager->deleteRule($deletedRule); } @@ -298,7 +334,13 @@ function (array $rawRules) use ($path): bool { $this->ruleManager->saveRule($rule); } - $node->getNode()->getStorage()->getPropagator()->propagateChange($fileInfo->getInternalPath(), $fileInfo->getMtime()); + $node->getNode() + ->getStorage() + ->getPropagator() + ->propagateChange( + $fileInfo->getInternalPath(), + $fileInfo->getMtime() + ); return true; } From 4a44b9ec9d7fb745cacd3732fd5587a16cd0cdd8 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 17 Dec 2025 11:06:10 -0500 Subject: [PATCH 10/16] refactor(dav): ACLPlugin variable naming Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index 90b13c00f..7f39efe59 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -45,7 +45,7 @@ class ACLPlugin extends ServerPlugin { private ?Server $server = null; private readonly ?IUser $user; /** @var array */ - private array $canManageACL = []; + private array $canManageACLForFolder = []; public function __construct( private readonly RuleManager $ruleManager, @@ -93,18 +93,18 @@ function () use ($fileInfo, $mount): ?array { return []; } - $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + $mountRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); if ($this->isAdmin($this->user, $fileInfo->getPath())) { $rules = $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - [$path] + [$mountRelativePath] ); } else { $rules = $this->ruleManager->getRulesForFilesByPath( $this->user, $mount->getNumericStorageId(), - [$path] + [$mountRelativePath] ); } @@ -120,23 +120,23 @@ function () use ($fileInfo, $mount): array { } $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); - $parentPaths = array_map( + $parentMountRelativePaths = array_map( fn (string $internalPath): string => trim($mount->getSourcePath() . '/' . $internalPath, '/'), $parentInternalPaths ); // Also include the mount root - $parentPaths[] = $mount->getSourcePath(); + $parentMountRelativePaths[] = $mount->getSourcePath(); if ($this->isAdmin($this->user, $fileInfo->getPath())) { $rulesByPath = $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - $parentPaths + $parentMountRelativePaths ); } else { $rulesByPath = $this->ruleManager->getRulesForFilesByPath( $this->user, $mount->getNumericStorageId(), - $parentPaths + $parentMountRelativePaths ); } @@ -250,9 +250,9 @@ public function propPatch(string $path, PropPatch $propPatch): void { // Mapping the old property to the new property. $propPatch->handle( self::ACL_LIST, - function (array $rawRules) use ($node, $fileInfo, $mount, $path): bool { + function (array $rawRules) use ($node, $fileInfo, $mount): bool { - $path = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + $mountRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); // populate fileid in rules $rules = array_values( @@ -299,7 +299,7 @@ function (array $rawRules) use ($node, $fileInfo, $mount, $path): bool { $newPermissions = $aclManager->testACLPermissionsForPath( $mount->getFolderId(), $mount->getNumericStorageId(), - $path, + $mountRelativePath, $rules ); @@ -310,7 +310,7 @@ function (array $rawRules) use ($node, $fileInfo, $mount, $path): bool { $existingRules = array_reduce( $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - [$path] + [$mountRelativePath] ), array_merge(...), [] @@ -361,12 +361,12 @@ private function isAdmin(IUser $user, string $path): bool { // TODO: catch/handle gracefully if folder disappeared between node fetch and this check (i.e. by another user / session) $folderId = $this->folderManager->getFolderByPath($path); - if (isset($this->canManageACL[$folderId])) { - return $this->canManageACL[$folderId]; + if (isset($this->canManageACLForFolder[$folderId])) { + return $this->canManageACLForFolder[$folderId]; } $canManage = $this->folderManager->canManageACL($folderId, $user); - $this->canManageACL[$folderId] = $canManage; + $this->canManageACLForFolder[$folderId] = $canManage; return $canManage; } From d3ef9dbfa60300980b36187cd12027e442f77d2e Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 18 Dec 2025 18:24:17 -0500 Subject: [PATCH 11/16] refactor(dav): more ACLPlugin tidying for clarity (vars - WIP/fixup) Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 60 ++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index 7f39efe59..d46eb4f56 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -93,18 +93,18 @@ function () use ($fileInfo, $mount): ?array { return []; } - $mountRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); if ($this->isAdmin($this->user, $fileInfo->getPath())) { $rules = $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - [$mountRelativePath] + [$aclRelativePath] ); } else { $rules = $this->ruleManager->getRulesForFilesByPath( $this->user, $mount->getNumericStorageId(), - [$mountRelativePath] + [$aclRelativePath] ); } @@ -247,15 +247,15 @@ public function propPatch(string $path, PropPatch $propPatch): void { return; } - // Mapping the old property to the new property. + // Handler to process and save changes to a folder's ACL rules via WebDAV property update $propPatch->handle( self::ACL_LIST, - function (array $rawRules) use ($node, $fileInfo, $mount): bool { + function (array $submittedRules) use ($node, $fileInfo, $mount): bool { - $mountRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - // populate fileid in rules - $rules = array_values( + // Make sure each submitted rule is associated with the current file's ID + $preparedRules = array_values( array_map( fn (Rule $rule): Rule => new Rule( $rule->getUserMapping(), @@ -263,26 +263,28 @@ function (array $rawRules) use ($node, $fileInfo, $mount): bool { $rule->getMask(), $rule->getPermissions() ), - $rawRules + $submittedRules ) ); - $formattedRules = array_map( + // Generate a display-friendly description string for each rule + $rulesDescriptions = array_map( fn (Rule $rule): string => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), - $rules + $preparedRules ); - if (count($formattedRules)) { - $formattedRules = implode(', ', $formattedRules); + // Record changes to ACL rules in the audit log + if (count($rulesDescriptions)) { + $rulesDescriptions = implode(', ', $rulesDescriptions); $this->eventDispatcher->dispatchTyped( new CriticalActionPerformedEvent ( 'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', - [ $fileInfo->getInternalPath(), $mount->getFolderId(), $formattedRules ] + [ $fileInfo->getInternalPath(), $mount->getFolderId(), $rulesDescriptions ] ) ); } else { @@ -299,41 +301,45 @@ function (array $rawRules) use ($node, $fileInfo, $mount): bool { $newPermissions = $aclManager->testACLPermissionsForPath( $mount->getFolderId(), $mount->getNumericStorageId(), - $mountRelativePath, - $rules + $aclRelativePath, + $preparedRules ); if (!($newPermissions & Constants::PERMISSION_READ)) { throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); } + // Compute all existing ACL rules associated with the file path $existingRules = array_reduce( $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - [$mountRelativePath] + [$aclRelativePath] ), array_merge(...), [] ); - - $deletedRules = array_udiff( + // Compute the ACL rules which are not present in the new new rules set so they can be deleted + $rulesToDelete = array_udiff( $existingRules, - $rules, - fn (Rule $obj_a, Rule $obj_b): int => ( - $obj_a->getUserMapping()->getType() === $obj_b->getUserMapping()->getType() - && $obj_a->getUserMapping()->getId() === $obj_b->getUserMapping()->getId() - ) ? 0 : -1 + $preparedRules, + fn (Rule $existingRule, Rule $submittedRule): int => ( + ($existingRule->getUserMapping()->getType() <=> $submittedRule->getUserMapping()->getType()) + ?: ($existingRule->getUserMapping()->getId() <=> $submittedRule->getUserMapping()->getId()) + ) ); - foreach ($deletedRules as $deletedRule) { - $this->ruleManager->deleteRule($deletedRule); + // Delete no longer present rules + foreach ($rulesToDelete as $ruleToDelete) { + $this->ruleManager->deleteRule($ruleToDelete); } - foreach ($rules as $rule) { + // Save new rules + foreach ($preparedRules as $rule) { $this->ruleManager->saveRule($rule); } + // Propagate changes to file cache $node->getNode() ->getStorage() ->getPropagator() From b718ba828d0e515594166a0a218d01a584748bdd Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 19 Dec 2025 10:42:36 -0500 Subject: [PATCH 12/16] refactor(dav): Clarify ACL handling in ACLPlugin comments And update variable names for clarity and consistency throughout. Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 141 ++++++++++++++++++++++++++++++++---------- 1 file changed, 110 insertions(+), 31 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index d46eb4f56..a94dadba8 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -32,7 +32,19 @@ /** * SabreDAV plugin for exposing and updating advanced ACL properties. * - * Handles WebDAV PROPFIND and PROPPATCH events for Nextcloud group folders with granular access controls. + * Handles WebDAV PROPFIND and PROPPATCH events for Nextcloud Teams/Group Folders with granular access controls. + * + * These handlers: + * - Ensures only relevant information is returned/modifiable for the target node. + * - Support both admin and user-level requests. + * + * Admins have a full overview and control: + * - can see and manage all inherited permission entries. + * - can see and manage rules for other users/groups. + * + * Standard users see only their own effective inherited permissions: + * - only see inherited permissions that affect them specifically. + * - can't view or manage rules for other users/groups. */ class ACLPlugin extends ServerPlugin { public const ACL_ENABLED = '{http://nextcloud.org/ns}acl-enabled'; @@ -74,6 +86,11 @@ public function initialize(Server $server): void { \Sabre\Xml\Deserializer\repeatingElements($reader, Rule::ACL); } + /** + * Property request handlers. + * + * These handlers provide read-only access to ACL related information. + */ public function propFind(PropFind $propFind, INode $node): void { if (!$node instanceof Node) { return; @@ -85,9 +102,22 @@ public function propFind(PropFind $propFind, INode $node): void { return; } + /* + * Handler to return the direct ACL rules for a specific file or folder via a WebDAV property request. + * + * - Direct ACL rules are those assigned directly to a specific file or folder (i.e. regardless of inheritance) + * - Admins or managers set these rules on individual nodes (files or folders). + * - Rules grant/restrict permissions for specific entities (users/groups/teams) for only that exact node. + * + * Example: If you set a rule to allow "Group X” to write to the folder `/Documents/Reports`, + * that is a direct ACL rule for `/Documents/Reports`. + * + * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the + * child will remain inaccessible and invisible to the user. + */ $propFind->handle( self::ACL_LIST, - function () use ($fileInfo, $mount): ?array { + function () use ($fileInfo, $mount): ?array { // TODO: Move out of here // Happens when sharing with a remote instance if ($this->user === null) { return []; @@ -95,12 +125,15 @@ function () use ($fileInfo, $mount): ?array { $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + // Retrieve the direct rules if ($this->isAdmin($this->user, $fileInfo->getPath())) { + // Admin $rules = $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), [$aclRelativePath] ); } else { + // Standard user $rules = $this->ruleManager->getRulesForFilesByPath( $this->user, $mount->getNumericStorageId(), @@ -108,66 +141,102 @@ function () use ($fileInfo, $mount): ?array { ); } + // Return the rules for the requested path (only one path is queried, so take the single result) return array_pop($rules); }); + /* + * Handler to return the inherited (effective) ACL rules for a file or folder via a WebDAV property request. + * + * Inherited (effective) ACL rules: + * - are those that apply to a file or folder because they were set on one of its parent folders. + * - are not set directly on the node in question -- they "cascade down" from parent directories with + * specific ACLs. + * - influence the effective permissions on a node by combining the rules set on its parent directories. + * + * Example: If `/Documents` grants "Group Y" read access, then `/Documents/Reports/file.txt` inherits that + * permission even if no direct rule exists for `/Documents/Reports/file.txt`. + * + * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the + * child is inaccessible and invisible to the user. + */ $propFind->handle( self::INHERITED_ACL_LIST, - function () use ($fileInfo, $mount): array { + function () use ($fileInfo, $mount): array { // TODO: Move out of here // Happens when sharing with a remote instance if ($this->user === null) { return []; } $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); - $parentMountRelativePaths = array_map( - fn (string $internalPath): string => trim($mount->getSourcePath() . '/' . $internalPath, '/'), + $parentAclRelativePaths = array_map( + fn (string $internalPath): string => + trim($mount->getSourcePath() . '/' . $internalPath, '/'), $parentInternalPaths ); - // Also include the mount root - $parentMountRelativePaths[] = $mount->getSourcePath(); + // Include the mount root + $parentAclRelativePaths[] = $mount->getSourcePath(); + // Retrieve the inherited rules if ($this->isAdmin($this->user, $fileInfo->getPath())) { + // Admin $rulesByPath = $this->ruleManager->getAllRulesForPaths( $mount->getNumericStorageId(), - $parentMountRelativePaths + $parentAclRelativePaths ); } else { + // Standard user $rulesByPath = $this->ruleManager->getRulesForFilesByPath( $this->user, $mount->getNumericStorageId(), - $parentMountRelativePaths + $parentAclRelativePaths ); } + /** + * Aggregrate inherited permissions for each relevant user/group/team across all parent paths. + * + * For each mapping (identified by type + ID): + * - Initialize the mapping if it hasn't been seen yet. + * - Accumulate permissions by applying each parent rule in order + * (to correctly resolve permissions as they cascade from ancestor to descendant). + * - Bitwise-OR the masks to track all inherited permission bits. + */ + ksort($rulesByPath); // Ensure parent paths are applied from root down + $inheritedPermissionsByUserKey = []; // Effective permissions per mapping + $inheritedMaskByUserKey = []; // Combined permission masks per mapping + $userMappingsByKey = []; // Mapping reference for later rule creation $aclManager = $this->aclManagerFactory->getACLManager($this->user); - ksort($rulesByPath); - $inheritedPermissionsByMapping = []; - $inheritedMaskByMapping = []; - $mappings = []; - foreach ($rulesByPath as $rules) { foreach ($rules as $rule) { - $mappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + // Create a unique key for each user/group/team mapping + $userMappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); - if (!isset($mappings[$mappingKey])) { - $mappings[$mappingKey] = $rule->getUserMapping(); + // Store mapping object if first encounter + if (!isset($userMappingsByKey[$userMappingKey])) { + $userMappingsByKey[$userMappingKey] = $rule->getUserMapping(); } - if (!isset($inheritedPermissionsByMapping[$mappingKey])) { - $inheritedPermissionsByMapping[$mappingKey] = $aclManager->getBasePermission($mount->getFolderId()); + // Initialize inherited permissions if not set + if (!isset($inheritedPermissionsByUserKey[$userMappingKey])) { + $inheritedPermissionsByUserKey[$userMappingKey] = $aclManager->getBasePermission($mount->getFolderId()); } - if (!isset($inheritedMaskByMapping[$mappingKey])) { - $inheritedMaskByMapping[$mappingKey] = 0; + // Initialize mask if not set + if (!isset($inheritedMaskByUserKey[$userMappingKey])) { + $inheritedMaskByUserKey[$userMappingKey] = 0; } - $inheritedPermissionsByMapping[$mappingKey] = $rule->applyPermissions($inheritedPermissionsByMapping[$mappingKey]); - $inheritedMaskByMapping[$mappingKey] |= $rule->getMask(); + // Apply rule's permissions to current inherited permissions + $inheritedPermissionsByUserKey[$userMappingKey] = $rule->applyPermissions($inheritedPermissionsByUserKey[$userMappingKey]); + + // Accumulate mask bits + $inheritedMaskByUserKey[$userMappingKey] |= $rule->getMask(); } } + // Build and return Rule objects representing the effective inherited permissions for each mapping return array_map( fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( $mapping, @@ -175,18 +244,20 @@ function () use ($fileInfo, $mount): array { $mask, $permissions ), - $mappings, - $inheritedPermissionsByMapping, - $inheritedMaskByMapping + $userMappingsByKey, + $inheritedPermissionsByUserKey, + $inheritedMaskByUserKey ); } ); + // Handler to provide the group folder ID for the current file or folder as a WebDAV property $propFind->handle( self::GROUP_FOLDER_ID, fn (): int => $this->folderManager->getFolderByPath($fileInfo->getPath()) ); + // Handler to provide whether ACLs are enabled for the current group folder as a WebDAV property $propFind->handle( self::ACL_ENABLED, function () use ($fileInfo): bool { @@ -195,6 +266,7 @@ function () use ($fileInfo): bool { } ); + // Handler to determine if the current user can manage ACLs for this group folder and return as a WebDAV property $propFind->handle( self::ACL_CAN_MANAGE, function () use ($fileInfo): bool { @@ -206,6 +278,7 @@ function () use ($fileInfo): bool { } ); + // Handler to provide the effective base permissions for the current group folder as a WebDAV property $propFind->handle( self::ACL_BASE_PERMISSION_PROPERTYNAME, function () use ($mount): int { @@ -220,6 +293,11 @@ function () use ($mount): int { ); } + /** + * Property update handlers. + * + * These handlers enable modifying ACL related configuration. + */ public function propPatch(string $path, PropPatch $propPatch): void { if ($this->server === null) { return; @@ -247,10 +325,10 @@ public function propPatch(string $path, PropPatch $propPatch): void { return; } - // Handler to process and save changes to a folder's ACL rules via WebDAV property update + // Handler to process and save changes to a folder's ACL rules via a WebDAV property update $propPatch->handle( self::ACL_LIST, - function (array $submittedRules) use ($node, $fileInfo, $mount): bool { + function (array $submittedRules) use ($node, $fileInfo, $mount): bool { // TODO: Move out of here $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); @@ -277,7 +355,7 @@ function (array $submittedRules) use ($node, $fileInfo, $mount): bool { $preparedRules ); - // Record changes to ACL rules in the audit log + // Record changes in the audit log if (count($rulesDescriptions)) { $rulesDescriptions = implode(', ', $rulesDescriptions); $this->eventDispatcher->dispatchTyped( @@ -297,6 +375,7 @@ function (array $submittedRules) use ($node, $fileInfo, $mount): bool { ); } + // Simulate new ACL rules to ensure the user does not remove their own read access before saving changes $aclManager = $this->aclManagerFactory->getACLManager($this->user); $newPermissions = $aclManager->testACLPermissionsForPath( $mount->getFolderId(), @@ -304,7 +383,6 @@ function (array $submittedRules) use ($node, $fileInfo, $mount): bool { $aclRelativePath, $preparedRules ); - if (!($newPermissions & Constants::PERMISSION_READ)) { throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); } @@ -319,11 +397,12 @@ function (array $submittedRules) use ($node, $fileInfo, $mount): bool { [] ); - // Compute the ACL rules which are not present in the new new rules set so they can be deleted + // If a mapping is missing in the new set, it means its rule should be deleted, regardless of its old permissions. $rulesToDelete = array_udiff( $existingRules, $preparedRules, fn (Rule $existingRule, Rule $submittedRule): int => ( + // Only compare by mapping (type + ID) since all rules here are already contextual to the same path. ($existingRule->getUserMapping()->getType() <=> $submittedRule->getUserMapping()->getType()) ?: ($existingRule->getUserMapping()->getId() <=> $submittedRule->getUserMapping()->getId()) ) From cbfe5fd35fc592e5d59e699f9b4027840d9ba111 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 19 Dec 2025 11:35:20 -0500 Subject: [PATCH 13/16] refactor(dav): Move long callbacks to private methods for readability Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 423 ++++++++++++++++++++++-------------------- 1 file changed, 217 insertions(+), 206 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index a94dadba8..35070a6dd 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -17,6 +17,7 @@ use OCA\GroupFolders\Mount\GroupMountPoint; use OCP\Constants; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\FileInfo; use OCP\IL10N; use OCP\IUser; use OCP\IUserSession; @@ -117,33 +118,8 @@ public function propFind(PropFind $propFind, INode $node): void { */ $propFind->handle( self::ACL_LIST, - function () use ($fileInfo, $mount): ?array { // TODO: Move out of here - // Happens when sharing with a remote instance - if ($this->user === null) { - return []; - } - - $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - - // Retrieve the direct rules - if ($this->isAdmin($this->user, $fileInfo->getPath())) { - // Admin - $rules = $this->ruleManager->getAllRulesForPaths( - $mount->getNumericStorageId(), - [$aclRelativePath] - ); - } else { - // Standard user - $rules = $this->ruleManager->getRulesForFilesByPath( - $this->user, - $mount->getNumericStorageId(), - [$aclRelativePath] - ); - } - - // Return the rules for the requested path (only one path is queried, so take the single result) - return array_pop($rules); - }); + fn () => $this->getDirectAclRulesForPath($fileInfo, $mount) + ); /* * Handler to return the inherited (effective) ACL rules for a file or folder via a WebDAV property request. @@ -162,93 +138,7 @@ function () use ($fileInfo, $mount): ?array { // TODO: Move out of here */ $propFind->handle( self::INHERITED_ACL_LIST, - function () use ($fileInfo, $mount): array { // TODO: Move out of here - // Happens when sharing with a remote instance - if ($this->user === null) { - return []; - } - - $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); - $parentAclRelativePaths = array_map( - fn (string $internalPath): string => - trim($mount->getSourcePath() . '/' . $internalPath, '/'), - $parentInternalPaths - ); - // Include the mount root - $parentAclRelativePaths[] = $mount->getSourcePath(); - - // Retrieve the inherited rules - if ($this->isAdmin($this->user, $fileInfo->getPath())) { - // Admin - $rulesByPath = $this->ruleManager->getAllRulesForPaths( - $mount->getNumericStorageId(), - $parentAclRelativePaths - ); - } else { - // Standard user - $rulesByPath = $this->ruleManager->getRulesForFilesByPath( - $this->user, - $mount->getNumericStorageId(), - $parentAclRelativePaths - ); - } - - /** - * Aggregrate inherited permissions for each relevant user/group/team across all parent paths. - * - * For each mapping (identified by type + ID): - * - Initialize the mapping if it hasn't been seen yet. - * - Accumulate permissions by applying each parent rule in order - * (to correctly resolve permissions as they cascade from ancestor to descendant). - * - Bitwise-OR the masks to track all inherited permission bits. - */ - ksort($rulesByPath); // Ensure parent paths are applied from root down - $inheritedPermissionsByUserKey = []; // Effective permissions per mapping - $inheritedMaskByUserKey = []; // Combined permission masks per mapping - $userMappingsByKey = []; // Mapping reference for later rule creation - $aclManager = $this->aclManagerFactory->getACLManager($this->user); - - foreach ($rulesByPath as $rules) { - foreach ($rules as $rule) { - // Create a unique key for each user/group/team mapping - $userMappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); - - // Store mapping object if first encounter - if (!isset($userMappingsByKey[$userMappingKey])) { - $userMappingsByKey[$userMappingKey] = $rule->getUserMapping(); - } - - // Initialize inherited permissions if not set - if (!isset($inheritedPermissionsByUserKey[$userMappingKey])) { - $inheritedPermissionsByUserKey[$userMappingKey] = $aclManager->getBasePermission($mount->getFolderId()); - } - - // Initialize mask if not set - if (!isset($inheritedMaskByUserKey[$userMappingKey])) { - $inheritedMaskByUserKey[$userMappingKey] = 0; - } - - // Apply rule's permissions to current inherited permissions - $inheritedPermissionsByUserKey[$userMappingKey] = $rule->applyPermissions($inheritedPermissionsByUserKey[$userMappingKey]); - - // Accumulate mask bits - $inheritedMaskByUserKey[$userMappingKey] |= $rule->getMask(); - } - } - - // Build and return Rule objects representing the effective inherited permissions for each mapping - return array_map( - fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( - $mapping, - $fileInfo->getId(), - $mask, - $permissions - ), - $userMappingsByKey, - $inheritedPermissionsByUserKey, - $inheritedMaskByUserKey - ); - } + fn () => $this->getInheritedAclRulesForPath($fileInfo, $mount) ); // Handler to provide the group folder ID for the current file or folder as a WebDAV property @@ -321,6 +211,7 @@ public function propPatch(string $path, PropPatch $propPatch): void { return; } + // Only allow ACL modifications if the current user has admin rights for this group folder if (!$this->isAdmin($this->user, $fileInfo->getPath())) { return; } @@ -328,108 +219,228 @@ public function propPatch(string $path, PropPatch $propPatch): void { // Handler to process and save changes to a folder's ACL rules via a WebDAV property update $propPatch->handle( self::ACL_LIST, - function (array $submittedRules) use ($node, $fileInfo, $mount): bool { // TODO: Move out of here - - $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); - - // Make sure each submitted rule is associated with the current file's ID - $preparedRules = array_values( - array_map( - fn (Rule $rule): Rule => new Rule( - $rule->getUserMapping(), - $fileInfo->getId(), - $rule->getMask(), - $rule->getPermissions() - ), - $submittedRules - ) - ); - - // Generate a display-friendly description string for each rule - $rulesDescriptions = array_map( - fn (Rule $rule): string => - $rule->getUserMapping()->getType() - . ' ' - . $rule->getUserMapping()->getDisplayName() - . ': ' . $rule->formatPermissions(), - $preparedRules - ); - - // Record changes in the audit log - if (count($rulesDescriptions)) { - $rulesDescriptions = implode(', ', $rulesDescriptions); - $this->eventDispatcher->dispatchTyped( - new CriticalActionPerformedEvent - ( - 'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', - [ $fileInfo->getInternalPath(), $mount->getFolderId(), $rulesDescriptions ] - ) - ); - } else { - $this->eventDispatcher->dispatchTyped( - new CriticalActionPerformedEvent - ( - 'The advanced permissions for "%s" in Team folder with ID %d was cleared', - [ $fileInfo->getInternalPath(), $mount->getFolderId() ] - ) - ); - } + fn (array $submittedRules) => $this->updateAclRulesForPath($submittedRules, $node, $fileInfo, $mount) + ); + } + + private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): ?array { + // Happens when sharing with a remote instance + if ($this->user === null) { + return []; + } + + $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + + // Retrieve the direct rules + if ($this->isAdmin($this->user, $fileInfo->getPath())) { + // Admin + $rules = $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + [$aclRelativePath] + ); + } else { + // Standard user + $rules = $this->ruleManager->getRulesForFilesByPath( + $this->user, + $mount->getNumericStorageId(), + [$aclRelativePath] + ); + } + + // Return the rules for the requested path (only one path is queried, so take the single result) + return array_pop($rules); + } + + private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): array { + // Happens when sharing with a remote instance + if ($this->user === null) { + return []; + } - // Simulate new ACL rules to ensure the user does not remove their own read access before saving changes - $aclManager = $this->aclManagerFactory->getACLManager($this->user); - $newPermissions = $aclManager->testACLPermissionsForPath( - $mount->getFolderId(), - $mount->getNumericStorageId(), - $aclRelativePath, - $preparedRules - ); - if (!($newPermissions & Constants::PERMISSION_READ)) { - throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); + $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); + $parentAclRelativePaths = array_map( + fn (string $internalPath): string => + trim($mount->getSourcePath() . '/' . $internalPath, '/'), + $parentInternalPaths + ); + // Include the mount root + $parentAclRelativePaths[] = $mount->getSourcePath(); + + // Retrieve the inherited rules + if ($this->isAdmin($this->user, $fileInfo->getPath())) { + // Admin + $rulesByPath = $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + $parentAclRelativePaths + ); + } else { + // Standard user + $rulesByPath = $this->ruleManager->getRulesForFilesByPath( + $this->user, + $mount->getNumericStorageId(), + $parentAclRelativePaths + ); + } + + /* + * Aggregate inherited permissions for each relevant user/group/team across all parent paths. + * + * For each mapping (identified by type + ID): + * - Initialize the mapping if it hasn't been seen yet. + * - Accumulate permissions by applying each parent rule in order + * (to correctly resolve permissions as they cascade from ancestor to descendant). + * - Bitwise-OR the masks to track all inherited permission bits. + */ + ksort($rulesByPath); // Ensure parent paths are applied from root down + $inheritedPermissionsByUserKey = []; // Effective permissions per mapping + $inheritedMaskByUserKey = []; // Combined permission masks per mapping + $userMappingsByKey = []; // Mapping reference for later rule creation + $aclManager = $this->aclManagerFactory->getACLManager($this->user); + + foreach ($rulesByPath as $rules) { + foreach ($rules as $rule) { + // Create a unique key for each user/group/team mapping + $userMappingKey = $rule->getUserMapping()->getType() . '::' . $rule->getUserMapping()->getId(); + + // Store mapping object if first encounter + if (!isset($userMappingsByKey[$userMappingKey])) { + $userMappingsByKey[$userMappingKey] = $rule->getUserMapping(); } - // Compute all existing ACL rules associated with the file path - $existingRules = array_reduce( - $this->ruleManager->getAllRulesForPaths( - $mount->getNumericStorageId(), - [$aclRelativePath] - ), - array_merge(...), - [] - ); - - // If a mapping is missing in the new set, it means its rule should be deleted, regardless of its old permissions. - $rulesToDelete = array_udiff( - $existingRules, - $preparedRules, - fn (Rule $existingRule, Rule $submittedRule): int => ( - // Only compare by mapping (type + ID) since all rules here are already contextual to the same path. - ($existingRule->getUserMapping()->getType() <=> $submittedRule->getUserMapping()->getType()) - ?: ($existingRule->getUserMapping()->getId() <=> $submittedRule->getUserMapping()->getId()) - ) - ); - - // Delete no longer present rules - foreach ($rulesToDelete as $ruleToDelete) { - $this->ruleManager->deleteRule($ruleToDelete); + // Initialize inherited permissions if not set + if (!isset($inheritedPermissionsByUserKey[$userMappingKey])) { + $inheritedPermissionsByUserKey[$userMappingKey] = $aclManager->getBasePermission($mount->getFolderId()); } - // Save new rules - foreach ($preparedRules as $rule) { - $this->ruleManager->saveRule($rule); + // Initialize mask if not set + if (!isset($inheritedMaskByUserKey[$userMappingKey])) { + $inheritedMaskByUserKey[$userMappingKey] = 0; } - // Propagate changes to file cache - $node->getNode() - ->getStorage() - ->getPropagator() - ->propagateChange( - $fileInfo->getInternalPath(), - $fileInfo->getMtime() - ); + // Apply rule's permissions to current inherited permissions + $inheritedPermissionsByUserKey[$userMappingKey] = $rule->applyPermissions($inheritedPermissionsByUserKey[$userMappingKey]); - return true; + // Accumulate mask bits + $inheritedMaskByUserKey[$userMappingKey] |= $rule->getMask(); } + } + + // Build and return Rule objects representing the effective inherited permissions for each mapping + return array_map( + fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( + $mapping, + $fileInfo->getId(), + $mask, + $permissions + ), + $userMappingsByKey, + $inheritedPermissionsByUserKey, + $inheritedMaskByUserKey + ); + } + + /** + * @throws BadRequest + */ + private function updateAclRulesForPath(array $submittedRules, Node $node, FileInfo $fileInfo, GroupMountPoint $mount): bool { + $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + + // Make sure each submitted rule is associated with the current file's ID + $preparedRules = array_values( + array_map( + fn (Rule $rule): Rule => new Rule( + $rule->getUserMapping(), + $fileInfo->getId(), + $rule->getMask(), + $rule->getPermissions() + ), + $submittedRules + ) + ); + + // Generate a display-friendly description string for each rule + $rulesDescriptions = array_map( + fn (Rule $rule): string => + $rule->getUserMapping()->getType() + . ' ' + . $rule->getUserMapping()->getDisplayName() + . ': ' . $rule->formatPermissions(), + $preparedRules ); + + // Record changes in the audit log + if (count($rulesDescriptions)) { + $rulesDescriptionsStr = implode(', ', $rulesDescriptions); + $this->eventDispatcher->dispatchTyped( + new CriticalActionPerformedEvent + ( + 'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', + [ $fileInfo->getInternalPath(), $mount->getFolderId(), $rulesDescriptionsStr ] + ) + ); + } else { + $this->eventDispatcher->dispatchTyped( + new CriticalActionPerformedEvent + ( + 'The advanced permissions for "%s" in Team folder with ID %d was cleared', + [ $fileInfo->getInternalPath(), $mount->getFolderId() ] + ) + ); + } + + // Simulate new ACL rules to ensure the user does not remove their own read access before saving changes + $aclManager = $this->aclManagerFactory->getACLManager($this->user); + $newPermissions = $aclManager->testACLPermissionsForPath( + $mount->getFolderId(), + $mount->getNumericStorageId(), + $aclRelativePath, + $preparedRules + ); + if (!($newPermissions & Constants::PERMISSION_READ)) { + throw new BadRequest($this->l10n->t('You cannot remove your own read permission.')); + } + + // Compute all existing ACL rules associated with the file path + $existingRules = array_reduce( + $this->ruleManager->getAllRulesForPaths( + $mount->getNumericStorageId(), + [$aclRelativePath] + ), + array_merge(...), + [] + ); + + // If a mapping is missing in the new set, it means its rule should be deleted, regardless of its old permissions. + $rulesToDelete = array_udiff( + $existingRules, + $preparedRules, + fn (Rule $existingRule, Rule $submittedRule): int => ( + // Only compare by mapping (type + ID) since all rules here are already contextual to the same path. + ($existingRule->getUserMapping()->getType() <=> $submittedRule->getUserMapping()->getType()) + ?: ($existingRule->getUserMapping()->getId() <=> $submittedRule->getUserMapping()->getId()) + ) + ); + + // Delete no longer present rules + foreach ($rulesToDelete as $ruleToDelete) { + $this->ruleManager->deleteRule($ruleToDelete); + } + + // Save new rules + foreach ($preparedRules as $rule) { + $this->ruleManager->saveRule($rule); + } + + // Propagate changes to file cache + $node->getNode() + ->getStorage() + ->getPropagator() + ->propagateChange( + $fileInfo->getInternalPath(), + $fileInfo->getMtime() + ); + + return true; } /** From 6b76d84b4b1c7d7a8e27262fddbfdd4e735cb007 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 19 Dec 2025 11:59:39 -0500 Subject: [PATCH 14/16] refactor(DAV): var name + move pvts in GroupFoldersHome Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 87 ++++++++++++++++++++++-------------- lib/DAV/GroupFolderNode.php | 2 +- lib/DAV/GroupFoldersHome.php | 71 ++++++++++++++++------------- lib/DAV/PropFindPlugin.php | 35 ++++++++------- 4 files changed, 114 insertions(+), 81 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index 35070a6dd..d8c24ffde 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -32,7 +32,7 @@ /** * SabreDAV plugin for exposing and updating advanced ACL properties. - * + * * Handles WebDAV PROPFIND and PROPPATCH events for Nextcloud Teams/Group Folders with granular access controls. * * These handlers: @@ -56,7 +56,7 @@ class ACLPlugin extends ServerPlugin { public const ACL_BASE_PERMISSION_PROPERTYNAME = '{http://nextcloud.org/ns}acl-base-permission'; private ?Server $server = null; - private readonly ?IUser $user; + private ?IUser $user = null; /** @var array */ private array $canManageACLForFolder = []; @@ -68,29 +68,33 @@ public function __construct( private readonly ACLManagerFactory $aclManagerFactory, private readonly IL10N $l10n, ) { - $this->user = $this->userSession->getUser(); - if ($this->user === null) { - return; - } } public function initialize(Server $server): void { $this->server = $server; + // may be null for public links / federated shares; handler logic must account for this. + $this->user = $this->userSession->getUser(); + $this->server->on('propFind', $this->propFind(...)); $this->server->on('propPatch', $this->propPatch(...)); - $this->server->xml->elementMap[Rule::ACL] = - Rule::class; - $this->server->xml->elementMap[self::ACL_LIST] = - fn (Reader $reader): array => - \Sabre\Xml\Deserializer\repeatingElements($reader, Rule::ACL); + $this->server->xml->elementMap[Rule::ACL] + = Rule::class; + $this->server->xml->elementMap[self::ACL_LIST] + = fn (Reader $reader): array + => \Sabre\Xml\Deserializer\repeatingElements($reader, Rule::ACL); } /** * Property request handlers. * * These handlers provide read-only access to ACL related information. + * + * In the current implementation, individual property level handlers that depend on $this->user + * are expected to determine and return an appropriate safe value when $this->user is null + * (e.g., for public links or federated shares). + * */ public function propFind(PropFind $propFind, INode $node): void { if (!$node instanceof Node) { @@ -105,16 +109,18 @@ public function propFind(PropFind $propFind, INode $node): void { /* * Handler to return the direct ACL rules for a specific file or folder via a WebDAV property request. - * + * * - Direct ACL rules are those assigned directly to a specific file or folder (i.e. regardless of inheritance) * - Admins or managers set these rules on individual nodes (files or folders). * - Rules grant/restrict permissions for specific entities (users/groups/teams) for only that exact node. * - * Example: If you set a rule to allow "Group X” to write to the folder `/Documents/Reports`, + * Example: If you set a rule to allow "Group X” to write to the folder `/Documents/Reports`, * that is a direct ACL rule for `/Documents/Reports`. * - * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the + * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the * child will remain inaccessible and invisible to the user. + * + * Returns an empty array if non-user sessions. */ $propFind->handle( self::ACL_LIST, @@ -133,8 +139,10 @@ public function propFind(PropFind $propFind, INode $node): void { * Example: If `/Documents` grants "Group Y" read access, then `/Documents/Reports/file.txt` inherits that * permission even if no direct rule exists for `/Documents/Reports/file.txt`. * - * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the + * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the * child is inaccessible and invisible to the user. + * + * Returns an empty array if non-user sessions. */ $propFind->handle( self::INHERITED_ACL_LIST, @@ -160,7 +168,7 @@ function () use ($fileInfo): bool { $propFind->handle( self::ACL_CAN_MANAGE, function () use ($fileInfo): bool { - // Happens when sharing with a remote instance + // Fail softly for non-user sessions if ($this->user === null) { return false; } @@ -172,9 +180,9 @@ function () use ($fileInfo): bool { $propFind->handle( self::ACL_BASE_PERMISSION_PROPERTYNAME, function () use ($mount): int { - // Happens when sharing with a remote instance + // Fail softly for non-user sessions if ($this->user === null) { - return Constants::PERMISSION_ALL; // ??? + return Constants::PERMISSION_ALL; } return $this->aclManagerFactory ->getACLManager($this->user) @@ -193,7 +201,7 @@ public function propPatch(string $path, PropPatch $propPatch): void { return; } - // Happens when sharing with a remote instance + // Non-user sessions (public link or federated share); no update handling is supported. if ($this->user === null) { return; } @@ -224,7 +232,7 @@ public function propPatch(string $path, PropPatch $propPatch): void { } private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): ?array { - // Happens when sharing with a remote instance + // Fail softly for non-user sessions if ($this->user === null) { return []; } @@ -252,16 +260,16 @@ private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $m } private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): array { - // Happens when sharing with a remote instance + // Fail softly for non-user sessions if ($this->user === null) { return []; } $parentInternalPaths = $this->getParents($fileInfo->getInternalPath()); $parentAclRelativePaths = array_map( - fn (string $internalPath): string => - trim($mount->getSourcePath() . '/' . $internalPath, '/'), - $parentInternalPaths + fn (string $internalPath): string + => trim($mount->getSourcePath() . '/' . $internalPath, '/'), + $parentInternalPaths ); // Include the mount root $parentAclRelativePaths[] = $mount->getSourcePath(); @@ -325,11 +333,17 @@ private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint } } + $fileId = $fileInfo->getId(); + if ($fileId === null) { + // shouldn't ever happen (only part files can return null) + throw new \LogicException('File ID cannot be null'); + } + // Build and return Rule objects representing the effective inherited permissions for each mapping return array_map( fn (IUserMapping $mapping, int $permissions, int $mask): Rule => new Rule( $mapping, - $fileInfo->getId(), + $fileId, $mask, $permissions ), @@ -345,12 +359,18 @@ private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint private function updateAclRulesForPath(array $submittedRules, Node $node, FileInfo $fileInfo, GroupMountPoint $mount): bool { $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); + $fileId = $fileInfo->getId(); + if ($fileId === null) { + // shouldn't ever happen (only part files can return null) + throw new \LogicException('File ID cannot be null'); + } + // Make sure each submitted rule is associated with the current file's ID $preparedRules = array_values( array_map( fn (Rule $rule): Rule => new Rule( $rule->getUserMapping(), - $fileInfo->getId(), + $fileId, $rule->getMask(), $rule->getPermissions() ), @@ -360,8 +380,8 @@ private function updateAclRulesForPath(array $submittedRules, Node $node, FileIn // Generate a display-friendly description string for each rule $rulesDescriptions = array_map( - fn (Rule $rule): string => - $rule->getUserMapping()->getType() + fn (Rule $rule): string + => $rule->getUserMapping()->getType() . ' ' . $rule->getUserMapping()->getDisplayName() . ': ' . $rule->formatPermissions(), @@ -372,16 +392,14 @@ private function updateAclRulesForPath(array $submittedRules, Node $node, FileIn if (count($rulesDescriptions)) { $rulesDescriptionsStr = implode(', ', $rulesDescriptions); $this->eventDispatcher->dispatchTyped( - new CriticalActionPerformedEvent - ( + new CriticalActionPerformedEvent( 'The advanced permissions for "%s" in Team folder with ID %d was set to "%s"', [ $fileInfo->getInternalPath(), $mount->getFolderId(), $rulesDescriptionsStr ] ) ); } else { $this->eventDispatcher->dispatchTyped( - new CriticalActionPerformedEvent - ( + new CriticalActionPerformedEvent( 'The advanced permissions for "%s" in Team folder with ID %d was cleared', [ $fileInfo->getInternalPath(), $mount->getFolderId() ] ) @@ -389,6 +407,7 @@ private function updateAclRulesForPath(array $submittedRules, Node $node, FileIn } // Simulate new ACL rules to ensure the user does not remove their own read access before saving changes + /** @psalm-suppress PossiblyNullArgument already checked by caller */ $aclManager = $this->aclManagerFactory->getACLManager($this->user); $newPermissions = $aclManager->testACLPermissionsForPath( $mount->getFolderId(), @@ -445,9 +464,9 @@ private function updateAclRulesForPath(array $submittedRules, Node $node, FileIn /** * Checks if the given user has admin (ACL management) rights for the group folder at the given path. - * + * * Caches the result per folder ID for efficiency. - * + * * @param IUser $user The user to check. * @param string $path The full path to a file or folder inside a group folder. * @return bool True if the user can manage ACLs for the group folder at the given path, false otherwise. diff --git a/lib/DAV/GroupFolderNode.php b/lib/DAV/GroupFolderNode.php index 1d9950519..ca9b5c8e2 100644 --- a/lib/DAV/GroupFolderNode.php +++ b/lib/DAV/GroupFolderNode.php @@ -13,7 +13,7 @@ use OCP\Files\FileInfo; /** - * WebDAV node representing a group folder directory. + * WebDAV node representing a group folder directory. * * Extends the standard Directory node to track the associated group folder ID, * allowing the system to identify and apply group folder-specific permissions diff --git a/lib/DAV/GroupFoldersHome.php b/lib/DAV/GroupFoldersHome.php index 9dc5daceb..1b5369469 100644 --- a/lib/DAV/GroupFoldersHome.php +++ b/lib/DAV/GroupFoldersHome.php @@ -55,34 +55,6 @@ public function createDirectory(string $name): never { throw new Forbidden('Permission denied to create folders in this folder'); } - private function getFolder(string $name): ?FolderDefinition { - $folders = $this->folderManager->getFoldersForUser($this->user); - foreach ($folders as $folder) { - if (basename($folder->mountPoint) === $name) { - return $folder; - } - } - - return null; - } - - /** - * Creates a GroupFolderNode for the given folder definition. - * - * @throws RuntimeException If the filesystem view cannot be obtained - */ - private function getDirectoryForFolder(FolderDefinition $folder): GroupFolderNode { - $userHome = '/' . $this->user->getUID() . '/files'; - $node = $this->rootFolder->get($userHome . '/' . $folder->mountPoint); - - $view = Filesystem::getView(); - if ($view === null) { - throw new RuntimeException('Unable to create view.'); - } - - return new GroupFolderNode($view, $node, $folder->id); - } - public function getChild(string $name): GroupFolderNode { $folder = $this->getFolder($name); if ($folder !== null) { @@ -99,9 +71,12 @@ public function getChildren(): array { $folders = $this->folderManager->getFoldersForUser($this->user); // Filter out non top-level folders - $folders = array_filter($folders, fn (FolderDefinition $folder): bool => !str_contains($folder->mountPoint, '/')); + $topLevelFolders = array_filter( + $folders, + fn (FolderDefinition $folder): bool => !str_contains($folder->mountPoint, '/') + ); - return array_map($this->getDirectoryForFolder(...), $folders); + return array_map($this->getDirectoryForFolder(...), $topLevelFolders); } public function childExists(string $name): bool { @@ -111,4 +86,40 @@ public function childExists(string $name): bool { public function getLastModified(): int { return 0; } + + /** + * Finds a group folder definition among the user's folders by folder name. + * + * @param string $name The name (basename of mountPoint) to match. + * @return FolderDefinition|null The folder definition if found, or null if no matching folder exists. + */ + private function getFolder(string $name): ?FolderDefinition { + $folders = $this->folderManager->getFoldersForUser($this->user); + foreach ($folders as $folder) { + if (basename($folder->mountPoint) === $name) { + return $folder; + } + } + + return null; + } + + /** + * Returns a GroupFolderNode representing the given group folder in the user's home directory. + * + * @param FolderDefinition $folder The group folder to represent. + * @return GroupFolderNode The DAV node for the specified group folder. + * @throws RuntimeException If the filesystem view cannot be obtained. + */ + private function getDirectoryForFolder(FolderDefinition $folder): GroupFolderNode { + $userHome = '/' . $this->user->getUID() . '/files'; + $node = $this->rootFolder->get($userHome . '/' . $folder->mountPoint); + + $view = Filesystem::getView(); + if ($view === null) { + throw new RuntimeException('Unable to create view.'); + } + + return new GroupFolderNode($view, $node, $folder->id); + } } diff --git a/lib/DAV/PropFindPlugin.php b/lib/DAV/PropFindPlugin.php index fff1d981a..4128d2ce1 100644 --- a/lib/DAV/PropFindPlugin.php +++ b/lib/DAV/PropFindPlugin.php @@ -22,26 +22,15 @@ * Adds the mount point and group folder ID as custom WebDAV properties for group folder nodes. */ class PropFindPlugin extends ServerPlugin { - private readonly ?Folder $userFolder = null; + private ?Folder $userFolder = null; public const MOUNT_POINT_PROPERTYNAME = '{http://nextcloud.org/ns}mount-point'; public const GROUP_FOLDER_ID_PROPERTYNAME = '{http://nextcloud.org/ns}group-folder-id'; public function __construct( - IRootFolder $rootFolder, - IUserSession $userSession + private readonly IRootFolder $rootFolder, + private readonly IUserSession $userSession, ) { - $user = $userSession->getUser(); - if ($user === null) { - // TODO: make this "silent" edge case visible in logs - return; - } - - $this->userFolder = $rootFolder->getUserFolder($user->getUID()); - if ($this->userFolder === null) { - // TODO: make this "silent" edge case visible in logs - return; - } } public function getPluginName(): string { @@ -49,17 +38,28 @@ public function getPluginName(): string { } public function initialize(Server $server): void { + $user = $this->userSession->getUser(); + // Gracefully handle non-user sessions + if ($user !== null) { + $this->userFolder = $this->rootFolder->getUserFolder($user->getUID()); + } + $server->on('propFind', $this->propFind(...)); } public function propFind(PropFind $propFind, INode $node): void { - if (!($node instanceof GroupFolderNode) || $this->userFolder === null) { + // non-user sessions aren't supported for any of these handlers currently + if ($this->userFolder === null) { + return; + } + + if (!($node instanceof GroupFolderNode)) { return; } $propFind->handle( self::MOUNT_POINT_PROPERTYNAME, - fn() => $this->getRelativeMountPointPath($node) + fn () => $this->getRelativeMountPointPath($node) ); $propFind->handle( @@ -74,6 +74,9 @@ public function propFind(PropFind $propFind, INode $node): void { * TODO: This may be a candidate for a utility function in GF or API addition in core. */ private function getRelativeMountPointPath(GroupFolderNode $node): ?string { + if ($this->userFolder === null) { // make psalm happy + throw new \LogicException('userFolder cannot be null'); + } // TODO: Seems there could be some more defensive null/error handling here (perhaps throwing a 404/not found + logging) $fileInfo = $node->getFileInfo(); $mount = $fileInfo->getMountPoint(); From a0c1dab6cec3253551065c89da29ab772a03c42f Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 24 Dec 2025 12:08:17 -0500 Subject: [PATCH 15/16] chore: enhance comments in ACLPlugin moved details to private method docblocks Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 142 +++++++++++++++++++++--------------------- 1 file changed, 72 insertions(+), 70 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index d8c24ffde..ca88688ef 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -2,7 +2,7 @@ declare(strict_types=1); /** - * SPDX-FileCopyrightText: 2019 Nextcloud GmbH and Nextcloud contributors + * SPDX-FileCopyrightText: 2019-2025 Nextcloud GmbH and Nextcloud contributors * SPDX-License-Identifier: AGPL-3.0-or-later */ @@ -31,7 +31,7 @@ use Sabre\Xml\Reader; /** - * SabreDAV plugin for exposing and updating advanced ACL properties. + * SabreDAV plugin for exposing ACL (advanced permissions) properties. * * Handles WebDAV PROPFIND and PROPPATCH events for Nextcloud Teams/Group Folders with granular access controls. * @@ -57,7 +57,7 @@ class ACLPlugin extends ServerPlugin { private ?Server $server = null; private ?IUser $user = null; - /** @var array */ + /** @var array Folder ID => can manage ACLs */ private array $canManageACLForFolder = []; public function __construct( @@ -73,7 +73,7 @@ public function __construct( public function initialize(Server $server): void { $this->server = $server; - // may be null for public links / federated shares; handler logic must account for this. + // Note: a null user is permitted (i.e. for public links / federated shares); handler logic must account for this. $this->user = $this->userSession->getUser(); $this->server->on('propFind', $this->propFind(...)); @@ -87,14 +87,9 @@ public function initialize(Server $server): void { } /** - * Property request handlers. - * - * These handlers provide read-only access to ACL related information. - * - * In the current implementation, individual property level handlers that depend on $this->user - * are expected to determine and return an appropriate safe value when $this->user is null - * (e.g., for public links or federated shares). - * + * WebDAV PROPFIND event handler for ACL-related properties. + * Provides read-only access to ACL information for the current node. + * If the session is unauthenticated, safe defaults are returned. */ public function propFind(PropFind $propFind, INode $node): void { if (!$node instanceof Node) { @@ -107,55 +102,25 @@ public function propFind(PropFind $propFind, INode $node): void { return; } - /* - * Handler to return the direct ACL rules for a specific file or folder via a WebDAV property request. - * - * - Direct ACL rules are those assigned directly to a specific file or folder (i.e. regardless of inheritance) - * - Admins or managers set these rules on individual nodes (files or folders). - * - Rules grant/restrict permissions for specific entities (users/groups/teams) for only that exact node. - * - * Example: If you set a rule to allow "Group X” to write to the folder `/Documents/Reports`, - * that is a direct ACL rule for `/Documents/Reports`. - * - * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the - * child will remain inaccessible and invisible to the user. - * - * Returns an empty array if non-user sessions. - */ + // Handler to provide ACL rules directly assigned to the file or folder. $propFind->handle( self::ACL_LIST, fn () => $this->getDirectAclRulesForPath($fileInfo, $mount) ); - /* - * Handler to return the inherited (effective) ACL rules for a file or folder via a WebDAV property request. - * - * Inherited (effective) ACL rules: - * - are those that apply to a file or folder because they were set on one of its parent folders. - * - are not set directly on the node in question -- they "cascade down" from parent directories with - * specific ACLs. - * - influence the effective permissions on a node by combining the rules set on its parent directories. - * - * Example: If `/Documents` grants "Group Y" read access, then `/Documents/Reports/file.txt` inherits that - * permission even if no direct rule exists for `/Documents/Reports/file.txt`. - * - * Note: Even if permission is granted directly to a child, if a parent folder does not grant read/list, the - * child is inaccessible and invisible to the user. - * - * Returns an empty array if non-user sessions. - */ + // Handler to provide the ACL rules inherited from parent folders (not set directly). $propFind->handle( self::INHERITED_ACL_LIST, fn () => $this->getInheritedAclRulesForPath($fileInfo, $mount) ); - // Handler to provide the group folder ID for the current file or folder as a WebDAV property + // Handler to provide the group folder ID for the current file or folder. $propFind->handle( self::GROUP_FOLDER_ID, fn (): int => $this->folderManager->getFolderByPath($fileInfo->getPath()) ); - // Handler to provide whether ACLs are enabled for the current group folder as a WebDAV property + // Handler to provide whether ACLs are enabled for the current group folder. $propFind->handle( self::ACL_ENABLED, function () use ($fileInfo): bool { @@ -164,11 +129,11 @@ function () use ($fileInfo): bool { } ); - // Handler to determine if the current user can manage ACLs for this group folder and return as a WebDAV property + // Handler to determine and return if the current user can manage ACLs for this group folder. $propFind->handle( self::ACL_CAN_MANAGE, function () use ($fileInfo): bool { - // Fail softly for non-user sessions + // Gracefully handle non-user sessions if ($this->user === null) { return false; } @@ -176,11 +141,11 @@ function () use ($fileInfo): bool { } ); - // Handler to provide the effective base permissions for the current group folder as a WebDAV property + // Handler to provide the effective base permissions for the current group folder. $propFind->handle( self::ACL_BASE_PERMISSION_PROPERTYNAME, function () use ($mount): int { - // Fail softly for non-user sessions + // Gracefully handle non-user sessions if ($this->user === null) { return Constants::PERMISSION_ALL; } @@ -192,9 +157,8 @@ function () use ($mount): int { } /** - * Property update handlers. - * - * These handlers enable modifying ACL related configuration. + * WebDAV PROPPATCH event handler for ACL-related properties. + * Enables modification of ACL assignments if the user has admin rights on the current node. */ public function propPatch(string $path, PropPatch $propPatch): void { if ($this->server === null) { @@ -219,20 +183,36 @@ public function propPatch(string $path, PropPatch $propPatch): void { return; } - // Only allow ACL modifications if the current user has admin rights for this group folder + // Only allow if user has admin rights for this group folder if (!$this->isAdmin($this->user, $fileInfo->getPath())) { return; } - // Handler to process and save changes to a folder's ACL rules via a WebDAV property update + // Handler to update (replace) the direct ACL rules for the specified file/folder. $propPatch->handle( self::ACL_LIST, fn (array $submittedRules) => $this->updateAclRulesForPath($submittedRules, $node, $fileInfo, $mount) ); } + /** + * Retrieves ACL rules assigned directly (not inherited) to the given file or folder. + * + * - For admins/managers: returns all direct rules for the node. + * - For non-admins/users: returns only direct rules relevant to the user. + * - For public/federated sessions: returns an empty array. + * + * Example: Granting "Group X" write access to `/Documents/Reports` is a direct ACL rule for that folder. + * + * Note: Direct rules alone do not determine effective access: + * - Read/list access must be permitted by all parent folders, regardless of direct rules. + * - For other permissions, direct rules take priority; missing permissions may be filled via inheritance. + * + * Example: If "Group X" has no read access on `/Documents`, they still can't access `/Documents/Reports`. + * + * @return list|null Direct ACL rules for the file/folder (may be empty). + */ private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): ?array { - // Fail softly for non-user sessions if ($this->user === null) { return []; } @@ -259,6 +239,19 @@ private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $m return array_pop($rules); } + /** + * Retrieves ACL rules inherited from parent folders (not set directly) for the given file or folder. + * + * - For admins/managers: returns all inherited rules affecting the node. + * - For non-admins/users: returns only inherited rules relevant to the user. + * - For public/federated sessions: returns an empty array. + * + * Note: Inherited rules are merged from all ancestor folders; direct rules for this node are excluded. + * - Effective read/list access requires all parent folders to permit access. + * - Other permissions are determined by merging inherited and direct rules separately. + * + * @return list Inherited ACL rules affecting the file/folder (may be empty). + */ private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): array { // Fail softly for non-user sessions if ($this->user === null) { @@ -354,7 +347,22 @@ private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint } /** - * @throws BadRequest + * Update (replace) the entire set of direct ACL rules for the given file or folder. + * + * This method overwrites all existing direct ACL rules for the node with a new set. + * Only users with ACL management rights (admins/managers) can perform this operation. + * + * - All provided Rule objects are written as the new direct ACLs for this file/folder. + * - Inherited ACL rules from parent folders are not modified. + * - If the rules array is empty, all direct ACLs on this node are removed. + * - Logs critical actions and dispatches audit events for ACL changes. + * + * @param Rule[] $submittedRules Array of new direct ACL Rule objects to apply. + * @param $node object of file/folder being updated. + * @param FileInfo $fileInfo object of file or folder being updated. + * @param GroupMountPoint $mount context for storage and folder resolution. + * + * @throws BadRequest if the operation is invalid (e.g. user's own read access would be removed) */ private function updateAclRulesForPath(array $submittedRules, Node $node, FileInfo $fileInfo, GroupMountPoint $mount): bool { $aclRelativePath = trim($mount->getSourcePath() . '/' . $fileInfo->getInternalPath(), '/'); @@ -463,13 +471,12 @@ private function updateAclRulesForPath(array $submittedRules, Node $node, FileIn } /** - * Checks if the given user has admin (ACL management) rights for the group folder at the given path. - * - * Caches the result per folder ID for efficiency. + * Checks if a user has admin (ACL management) rights for the group folder at the provided path. + * Results are cached per folder for efficiency. * * @param IUser $user The user to check. * @param string $path The full path to a file or folder inside a group folder. - * @return bool True if the user can manage ACLs for the group folder at the given path, false otherwise. + * * @throws \OCP\Files\NotFoundException If the path does not exist or is not part of a group folder. */ private function isAdmin(IUser $user, string $path): bool { @@ -486,17 +493,12 @@ private function isAdmin(IUser $user, string $path): bool { } /** - * Returns all parent directory paths for the given path, based solely on the path itself. - * - * The array is ordered from immediate parent upward, excluding the original path. - * - * Example: - * getParents('a/b/c.txt') returns ['a/b', 'a'] - * - * Note: Callers should add contextual parents (such as mount points or absolute roots) if needed. + * Returns all parent directory paths of the given path, from nearest to root. + * Excludes the original path itself. + * E.g.: 'a/b/c.txt' → ['a/b', 'a'] * * @param string $path Path to a file or directory. - * @return string[] Parent directory paths, from closest to furthest. + * @return list Parent directory paths, from closest to furthest. */ private function getParents(string $path): array { $parents = []; From 7597263055aab9b1ca9b91743fbff86538ba5c14 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 24 Dec 2025 13:29:27 -0500 Subject: [PATCH 16/16] fix: Update PHPDoc return type and parameter descriptions Signed-off-by: Josh --- lib/DAV/ACLPlugin.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index ca88688ef..c877895a2 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -210,9 +210,9 @@ public function propPatch(string $path, PropPatch $propPatch): void { * * Example: If "Group X" has no read access on `/Documents`, they still can't access `/Documents/Reports`. * - * @return list|null Direct ACL rules for the file/folder (may be empty). + * @return list Direct ACL rules for the file/folder (may be empty). */ - private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): ?array { + private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $mount): array { if ($this->user === null) { return []; } @@ -236,7 +236,7 @@ private function getDirectAclRulesForPath(FileInfo $fileInfo, GroupMountPoint $m } // Return the rules for the requested path (only one path is queried, so take the single result) - return array_pop($rules); + return array_values(array_pop($rules) ?? []); } /** @@ -357,7 +357,7 @@ private function getInheritedAclRulesForPath(FileInfo $fileInfo, GroupMountPoint * - If the rules array is empty, all direct ACLs on this node are removed. * - Logs critical actions and dispatches audit events for ACL changes. * - * @param Rule[] $submittedRules Array of new direct ACL Rule objects to apply. + * @param Rule[] $submittedRules Array of new direct ACL Rule objects to apply. * @param $node object of file/folder being updated. * @param FileInfo $fileInfo object of file or folder being updated. * @param GroupMountPoint $mount context for storage and folder resolution.