From 656cd7fc5ed77cfb690c7af1c7d0d48b17fb6716 Mon Sep 17 00:00:00 2001 From: Stefan Lender Date: Tue, 22 Sep 2026 15:44:12 +0200 Subject: [PATCH] fix(TeamSpace): enable ACLs for team folders Enable ACLs before assigning team ownership and grant the owning circle ACL management rights. Keep the reserved .system directory accessible in team folders despite restrictive ACL rules, reject ACL changes for that path, and migrate existing team folders to ACL-enabled. Add coverage for team-folder creation and .system ACL handling. Co-authored-by: Copilot GPT-5.6 Terra Signed-off-by: Stefan Lender --- lib/ACL/ACLStorageWrapper.php | 16 +++++++++- lib/DAV/ACLPlugin.php | 15 ++++++++- .../Version2300000Date20260922000000.php | 32 +++++++++++++++++++ lib/Mount/FolderStorageManager.php | 2 ++ lib/TeamSpace/TeamSpaceService.php | 1 + tests/ACL/ACLScannerTest.php | 1 + tests/ACL/ACLStorageWrapperTest.php | 31 ++++++++++++++++++ tests/TeamSpace/TeamSpaceServiceTest.php | 1 + 8 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 lib/Migration/Version2300000Date20260922000000.php diff --git a/lib/ACL/ACLStorageWrapper.php b/lib/ACL/ACLStorageWrapper.php index 3b6fe1024..e856e85a3 100644 --- a/lib/ACL/ACLStorageWrapper.php +++ b/lib/ACL/ACLStorageWrapper.php @@ -22,9 +22,10 @@ class ACLStorageWrapper extends Wrapper implements IConstructableStorage { private readonly bool $inShare; private readonly int $folderId; private readonly int $storageId; + private readonly bool $isTeamSpace; /** - * @param array{storage: Storage, acl_manager: ACLManager, in_share: bool, folder_id: int, storage_id: int} $arguments + * @param array{storage: Storage, acl_manager: ACLManager, in_share: bool, folder_id: int, storage_id: int, is_team_space: bool} $arguments */ public function __construct(array $arguments) { parent::__construct($arguments); @@ -32,9 +33,14 @@ public function __construct(array $arguments) { $this->inShare = $arguments['in_share']; $this->folderId = $arguments['folder_id']; $this->storageId = $arguments['storage_id']; + $this->isTeamSpace = $arguments['is_team_space']; } private function getACLPermissionsForPath(string $path): int { + if ($this->isProtectedTeamSpacePath($path)) { + return Constants::PERMISSION_ALL; + } + $permissions = $this->aclManager->getACLPermissionsForPath($this->folderId, $this->storageId, $path); // if there is no read permissions, than deny everything @@ -47,6 +53,10 @@ private function getACLPermissionsForPath(string $path): int { return $canRead ? $permissions : 0; } + private function isProtectedTeamSpacePath(string $path): bool { + return $this->isTeamSpace && ($path === '.system' || str_starts_with($path, '.system/')); + } + private function checkPermissions(string $path, int $permissions): bool { return ($this->getACLPermissionsForPath($path) & $permissions) === $permissions; } @@ -169,6 +179,10 @@ public function unlink(string $path): bool { * This check is fairly expensive so we only do it for the actual delete and not metadata operations */ private function canDeleteTree(string $path): int { + if ($this->isProtectedTeamSpacePath($path)) { + return Constants::PERMISSION_DELETE; + } + return $this->aclManager->getPermissionsForTree($this->folderId, $this->storageId, $path) & Constants::PERMISSION_DELETE; } diff --git a/lib/DAV/ACLPlugin.php b/lib/DAV/ACLPlugin.php index f09064aae..da17dc792 100644 --- a/lib/DAV/ACLPlugin.php +++ b/lib/DAV/ACLPlugin.php @@ -93,6 +93,16 @@ private function getParents(string $path): array { return $paths; } + private function isProtectedTeamSpacePath(GroupMountPoint $mount, string $path): bool { + $folder = $this->folderManager->getFolder($mount->getFolderId()); + if ($folder === null || !$folder->isTeamSpace()) { + return false; + } + + $path = ltrim($path, '/'); + return $path === '.system' || str_starts_with($path, '.system/'); + } + public function propFind(PropFind $propFind, INode $node): void { if (!$node instanceof Node) { return; @@ -227,7 +237,7 @@ 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 { + $propPatch->handle(self::ACL_LIST, function (array $rawRules) use ($path, $mount): bool { if ($this->server === null) { return false; } @@ -238,6 +248,9 @@ public function propPatch(string $path, PropPatch $propPatch): void { } $fileInfo = $node->getFileInfo(); + if ($this->isProtectedTeamSpacePath($mount, $fileInfo->getInternalPath())) { + throw new BadRequest($this->l10n->t('Advanced permissions cannot be changed for the reserved .system directory.')); + } $fileInfoId = $fileInfo->getId(); if ($fileInfoId === null) { diff --git a/lib/Migration/Version2300000Date20260922000000.php b/lib/Migration/Version2300000Date20260922000000.php new file mode 100644 index 000000000..ad5a35226 --- /dev/null +++ b/lib/Migration/Version2300000Date20260922000000.php @@ -0,0 +1,32 @@ +connection->getQueryBuilder(); + $query->update('group_folders') + ->set('acl', $query->createNamedParameter(true, IQueryBuilder::PARAM_BOOL)) + ->where($query->expr()->isNotNull('team_circle_id')); + $query->executeStatement(); + } +} \ No newline at end of file diff --git a/lib/Mount/FolderStorageManager.php b/lib/Mount/FolderStorageManager.php index f2b084911..d9abd5a90 100644 --- a/lib/Mount/FolderStorageManager.php +++ b/lib/Mount/FolderStorageManager.php @@ -116,6 +116,7 @@ private function getBaseStorageForFolderSeparate( 'acl_manager' => $aclManager, 'in_share' => $inShare, 'folder_id' => $folderId, + 'is_team_space' => $folder->isTeamSpace(), // already loaded with the folder; avoids a storages lookup per folder 'storage_id' => $folder->storageId, ]); @@ -251,6 +252,7 @@ private function getBaseStorageForFolderRootJail( 'acl_manager' => $aclManager, 'in_share' => $inShare, 'folder_id' => $folderId, + 'is_team_space' => $folder->isTeamSpace(), // already loaded; the same id MountProvider uses for rule lookups (incl. legacy root-jail) 'storage_id' => $folder->storageId, ]); diff --git a/lib/TeamSpace/TeamSpaceService.php b/lib/TeamSpace/TeamSpaceService.php index ed8d8c9fb..55c572bc2 100644 --- a/lib/TeamSpace/TeamSpaceService.php +++ b/lib/TeamSpace/TeamSpaceService.php @@ -62,6 +62,7 @@ public function createTeamSpace(string $circleId, string $mountPoint, int $quota } $this->folderManager->addApplicableGroup($folderId, $circleId); + $this->folderManager->setFolderACL($folderId, true); $this->folderManager->setManageACL($folderId, 'circle', $circleId, true); $this->folderManager->setTeamCircleId($folderId, $circleId); diff --git a/tests/ACL/ACLScannerTest.php b/tests/ACL/ACLScannerTest.php index 8984d420c..e8766084e 100644 --- a/tests/ACL/ACLScannerTest.php +++ b/tests/ACL/ACLScannerTest.php @@ -61,6 +61,7 @@ public function testScanAclStorage(): void { 'in_share' => false, 'folder_id' => 0, 'storage_id' => $cache->getNumericStorageId(), + 'is_team_space' => false, ]); $scanner = $aclStorage->getScanner(); diff --git a/tests/ACL/ACLStorageWrapperTest.php b/tests/ACL/ACLStorageWrapperTest.php index 1a719ce8b..f6447c8b6 100644 --- a/tests/ACL/ACLStorageWrapperTest.php +++ b/tests/ACL/ACLStorageWrapperTest.php @@ -37,9 +37,40 @@ protected function setUp(): void { 'in_share' => false, 'folder_id' => 0, 'storage_id' => $this->source->getCache()->getNumericStorageId(), + 'is_team_space' => true, ]); } + public function testTeamSpaceSystemDirectoryBypassesACL(): void { + $this->source->mkdir('.system'); + $this->source->touch('.system/app-data.json'); + $this->source->mkdir('.system-copy'); + + $this->aclPermissions['.system'] = 0; + $this->aclPermissions['.system/app-data.json'] = 0; + $this->aclPermissions['.system-copy'] = 0; + + $this->assertTrue($this->storage->isReadable('.system')); + $this->assertTrue($this->storage->isReadable('.system/app-data.json')); + $this->assertFalse($this->storage->isReadable('.system-copy')); + } + + public function testRegularGroupFolderSystemDirectoryHonorsACL(): void { + $this->source->mkdir('.system'); + $this->aclPermissions['.system'] = 0; + + $storage = new ACLStorageWrapper([ + 'storage' => $this->source, + 'acl_manager' => $this->aclManager, + 'in_share' => false, + 'folder_id' => 0, + 'storage_id' => $this->source->getCache()->getNumericStorageId(), + 'is_team_space' => false, + ]); + + $this->assertFalse($storage->isReadable('.system')); + } + public function testNoReadImpliesNothing(): void { $this->source->mkdir('foo'); $this->aclPermissions['foo'] = Constants::PERMISSION_ALL - Constants::PERMISSION_READ; diff --git a/tests/TeamSpace/TeamSpaceServiceTest.php b/tests/TeamSpace/TeamSpaceServiceTest.php index cef66d756..fc854c598 100644 --- a/tests/TeamSpace/TeamSpaceServiceTest.php +++ b/tests/TeamSpace/TeamSpaceServiceTest.php @@ -58,6 +58,7 @@ static function (string $path) use (&$scannedPaths): void { ); $this->folderManager->expects($this->once())->method('setFolderQuota')->with(42, 1024); $this->folderManager->expects($this->once())->method('addApplicableGroup')->with(42, 'team-1'); + $this->folderManager->expects($this->once())->method('setFolderACL')->with(42, true); $this->folderManager->expects($this->once())->method('setManageACL')->with(42, 'circle', 'team-1', true); $this->folderManager->expects($this->once())->method('setTeamCircleId')->with(42, 'team-1');