From 63cca1655db8e697f3a9a4e639e6d42039eb7cc8 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 24 Jul 2024 18:29:23 +0200 Subject: [PATCH 1/4] fix: handle trashbin acl cases where the original parent got deleted Signed-off-by: Robin Appelman --- lib/Trash/TrashBackend.php | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/Trash/TrashBackend.php b/lib/Trash/TrashBackend.php index 85e340d33..3dff237b6 100644 --- a/lib/Trash/TrashBackend.php +++ b/lib/Trash/TrashBackend.php @@ -317,9 +317,11 @@ private function getTrashForFolders(IUser $user, array $folders): array { }, $folders); $rows = $this->trashManager->listTrashForFolders($folderIds); $indexedRows = []; + $trashItemsByOriginalLocation = []; foreach ($rows as $row) { $key = $row['folder_id'] . '/' . $row['name'] . '/' . $row['deleted_time']; $indexedRows[$key] = $row; + $trashItemsByOriginalLocation[$row['original_location']] = $row; } $items = []; foreach ($folders as $folder) { @@ -334,12 +336,22 @@ private function getTrashForFolders(IUser $user, array $folders): array { $timestamp = (int)substr($pathParts['extension'], 1); $name = $pathParts['filename']; $key = $folderId . '/' . $name . '/' . $timestamp; + + $originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : ''; if (!$this->userHasAccessToPath($user, $item->getPath())) { continue; } + // if a parent of the original location has also been deleted, we also need to check it based on the now-deleted parent path + foreach ($this->getParentOriginalPaths($originalLocation, $trashItemsByOriginalLocation) as $parentOriginalPath) { + $parentTrashItem = $trashItemsByOriginalLocation[$parentOriginalPath]; + $relativePath = substr($originalLocation, strlen($parentOriginalPath)); + $parentTrashItemPath = "__groupfolders/trash/{$parentTrashItem['folder_id']}/{$parentTrashItem['name']}.d{$parentTrashItem['deleted_time']}"; + if (!$this->userHasAccessToPath($user, $parentTrashItemPath . $relativePath)) { + continue 2; + } + } $info = $item->getFileInfo(); $info['name'] = $name; - $originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : ''; $items[] = new GroupTrashItem( $this, $originalLocation, @@ -354,6 +366,20 @@ private function getTrashForFolders(IUser $user, array $folders): array { return $items; } + private function getParentOriginalPaths(string $path, array $trashItemsByOriginalPath): array { + $parentPaths = []; + while ($path !== '') { + $path = dirname($path); + + if ($path === '.' || $path === '/') { + break; + } elseif (isset($trashItemsByOriginalPath[$path])) { + $parentPaths[] = $path; + } + } + return $parentPaths; + } + public function getTrashNodeById(IUser $user, int $fileId): ?Node { try { /** @var Folder $trashFolder */ From 2f26b80dd147f07c57633534958a9277a754d5d9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 24 Jul 2024 18:40:02 +0200 Subject: [PATCH 2/4] fix: don't show acl groupfolders trash items if we don't know the original location Signed-off-by: Robin Appelman --- lib/Trash/TrashBackend.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/Trash/TrashBackend.php b/lib/Trash/TrashBackend.php index 3dff237b6..a9e358cb5 100644 --- a/lib/Trash/TrashBackend.php +++ b/lib/Trash/TrashBackend.php @@ -329,6 +329,7 @@ private function getTrashForFolders(IUser $user, array $folders): array { $mountPoint = $folder['mount_point']; $trashFolder = $this->getTrashFolder($folderId); $content = $trashFolder->getDirectoryListing(); + $userCanManageAcl = $this->folderManager->canManageACL($folderId, $user); $this->aclManagerFactory->getACLManager($user)->preloadRulesForFolder($trashFolder->getPath()); foreach ($content as $item) { /** @var \OC\Files\Node\Node $item */ @@ -338,6 +339,11 @@ private function getTrashForFolders(IUser $user, array $folders): array { $key = $folderId . '/' . $name . '/' . $timestamp; $originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : ''; + + // if we for any reason lost track of the original location, hide the item for non-managers as a fail-safe + if ($originalLocation === '' && !$userCanManageAcl) { + continue; + } if (!$this->userHasAccessToPath($user, $item->getPath())) { continue; } @@ -350,6 +356,7 @@ private function getTrashForFolders(IUser $user, array $folders): array { continue 2; } } + $info = $item->getFileInfo(); $info['name'] = $name; $items[] = new GroupTrashItem( From 6d3e607f414e1a4f93d783edaa60f92cd0908c03 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 24 Jul 2024 18:41:07 +0200 Subject: [PATCH 3/4] perf: don't perform trashbin access check if acls are not enabled for a folder Signed-off-by: Robin Appelman --- lib/Trash/TrashBackend.php | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/lib/Trash/TrashBackend.php b/lib/Trash/TrashBackend.php index a9e358cb5..565ab026b 100644 --- a/lib/Trash/TrashBackend.php +++ b/lib/Trash/TrashBackend.php @@ -31,6 +31,7 @@ use OCA\GroupFolders\Mount\MountProvider; use OCA\GroupFolders\Versions\VersionsBackend; use OCP\Constants; +use OCP\Files\Cache\ICacheEntry; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Node; @@ -309,6 +310,8 @@ private function getTrashFolder(int $folderId): Folder { } /** + * @param IUser $user + * @param array{folder_id: int, mount_point: string, permissions: int, quota: int, acl: bool, rootCacheEntry: ?ICacheEntry}[] $folders * @return list */ private function getTrashForFolders(IUser $user, array $folders): array { @@ -326,6 +329,7 @@ private function getTrashForFolders(IUser $user, array $folders): array { $items = []; foreach ($folders as $folder) { $folderId = $folder['folder_id']; + $folderHasAcl = $folder['acl']; $mountPoint = $folder['mount_point']; $trashFolder = $this->getTrashFolder($folderId); $content = $trashFolder->getDirectoryListing(); @@ -340,20 +344,22 @@ private function getTrashForFolders(IUser $user, array $folders): array { $originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : ''; - // if we for any reason lost track of the original location, hide the item for non-managers as a fail-safe - if ($originalLocation === '' && !$userCanManageAcl) { - continue; - } - if (!$this->userHasAccessToPath($user, $item->getPath())) { - continue; - } - // if a parent of the original location has also been deleted, we also need to check it based on the now-deleted parent path - foreach ($this->getParentOriginalPaths($originalLocation, $trashItemsByOriginalLocation) as $parentOriginalPath) { - $parentTrashItem = $trashItemsByOriginalLocation[$parentOriginalPath]; - $relativePath = substr($originalLocation, strlen($parentOriginalPath)); - $parentTrashItemPath = "__groupfolders/trash/{$parentTrashItem['folder_id']}/{$parentTrashItem['name']}.d{$parentTrashItem['deleted_time']}"; - if (!$this->userHasAccessToPath($user, $parentTrashItemPath . $relativePath)) { - continue 2; + if ($folderHasAcl) { + // if we for any reason lost track of the original location, hide the item for non-managers as a fail-safe + if ($originalLocation === '' && !$userCanManageAcl) { + continue; + } + if (!$this->userHasAccessToPath($user, $item->getPath())) { + continue; + } + // if a parent of the original location has also been deleted, we also need to check it based on the now-deleted parent path + foreach ($this->getParentOriginalPaths($originalLocation, $trashItemsByOriginalLocation) as $parentOriginalPath) { + $parentTrashItem = $trashItemsByOriginalLocation[$parentOriginalPath]; + $relativePath = substr($originalLocation, strlen($parentOriginalPath)); + $parentTrashItemPath = "__groupfolders/trash/{$parentTrashItem['folder_id']}/{$parentTrashItem['name']}.d{$parentTrashItem['deleted_time']}"; + if (!$this->userHasAccessToPath($user, $parentTrashItemPath . $relativePath)) { + continue 2; + } } } From f7638c0df0741f380c6f34dc26841ba35d5d970e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 24 Jul 2024 19:46:52 +0200 Subject: [PATCH 4/4] tests: add tests for trashbin acl handling Signed-off-by: Robin Appelman --- tests/Trash/TrashBackendTest.php | 198 +++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 tests/Trash/TrashBackendTest.php diff --git a/tests/Trash/TrashBackendTest.php b/tests/Trash/TrashBackendTest.php new file mode 100644 index 000000000..0bed10a63 --- /dev/null +++ b/tests/Trash/TrashBackendTest.php @@ -0,0 +1,198 @@ + + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\GroupFolders\Tests\Trash; + +use OC\Files\SetupManager; +use OC\Group\Database; +use OCA\GroupFolders\ACL\ACLManager; +use OCA\GroupFolders\ACL\ACLManagerFactory; +use OCA\GroupFolders\ACL\Rule; +use OCA\GroupFolders\ACL\RuleManager; +use OCA\GroupFolders\ACL\UserMapping\UserMapping; +use OCA\GroupFolders\Folder\FolderManager; +use OCA\GroupFolders\Mount\GroupFolderStorage; +use OCA\GroupFolders\Trash\TrashBackend; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\IUser; +use Test\TestCase; +use Test\Traits\UserTrait; + +/** + * @group DB + */ +class TrashBackendTest extends TestCase { + use UserTrait; + + private string $folderName; + private TrashBackend $trashBackend; + private FolderManager $folderManager; + private ACLManager $aclManager; + private RuleManager $ruleManager; + private int $folderId; + private Folder $managerUserFolder; + private Folder $normalUserFolder; + private IUser $managerUser; + private IUser $normalUser; + + public function setUp(): void { + parent::setUp(); + + $this->folderName = "gf"; + $this->managerUser = $this->createUser("manager", "test"); + $this->normalUser = $this->createUser("normal", "test"); + + /** @var Database $groupBackend */ + $groupBackend = \OC::$server->get(Database::class); + $groupBackend->createGroup("gf_manager"); + $groupBackend->createGroup("gf_normal"); + $groupBackend->addToGroup("manager", "gf_manager"); + $groupBackend->addToGroup("normal", "gf_normal"); + + $this->trashBackend = \OC::$server->get(TrashBackend::class); + $this->folderManager = \OC::$server->get(FolderManager::class); + /** @var ACLManagerFactory $aclManagerFactory */ + $aclManagerFactory = \OC::$server->get(ACLManagerFactory::class); + $this->aclManager = $aclManagerFactory->getACLManager($this->managerUser); + $this->ruleManager = \OC::$server->get(RuleManager::class); + + $this->folderId = $this->folderManager->createFolder($this->folderName); + $this->folderManager->addApplicableGroup($this->folderId, "gf_manager"); + $this->folderManager->addApplicableGroup($this->folderId, "gf_normal"); + $this->folderManager->setFolderACL($this->folderId, true); + $this->folderManager->setManageACL($this->folderId, "user", "manager", true); + + /** @var IRootFolder $rootFolder */ + $rootFolder = \OC::$server->get(IRootFolder::class); + + $this->managerUserFolder = $rootFolder->getUserFolder("manager"); + $this->normalUserFolder = $rootFolder->getUserFolder("normal"); + + $this->assertTrue($this->managerUserFolder->nodeExists($this->folderName)); + $this->assertTrue($this->normalUserFolder->nodeExists($this->folderName)); + + /** @var GroupFolderStorage $groupFolderStorage */ + $groupFolderStorage = $this->managerUserFolder->get($this->folderName)->getStorage(); + $this->assertTrue($groupFolderStorage->instanceOfStorage(GroupFolderStorage::class)); + $this->assertEquals($this->folderId, $groupFolderStorage->getFolderId()); + + } + + protected function tearDown(): void { + $this->trashBackend->cleanTrashFolder($this->folderId); + $this->folderManager->removeFolder($this->folderId); + + /** @var SetupManager $setupManager */ + $setupManager = \OC::$server->get(SetupManager::class); + $setupManager->tearDown(); + parent::tearDown(); + } + + + private function createNoReadRule(string $userId, int $fileId): Rule { + return new Rule( + new UserMapping("user", $userId), + $fileId, + 1, + 0, + ); + } + + public function testHideTrashItemAcl() { + $restricted = $this->managerUserFolder->newFile("{$this->folderName}/restricted.txt", "content"); + $this->ruleManager->saveRule($this->createNoReadRule("normal", $restricted->getId())); + + $this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/restricted.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/restricted.txt")); + + $this->trashBackend->moveToTrash($restricted->getStorage(), $restricted->getInternalPath()); + + $this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/restricted.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/restricted.txt")); + + // only the manager can see the deleted file + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->normalUser)); + } + + public function testHideItemInDeletedFolderAcl() { + $folder = $this->managerUserFolder->newFolder("{$this->folderName}/folder"); + $folder->newFile("file.txt", "content1"); + $restrictedChild = $folder->newFile("restricted.txt", "content2"); + $this->ruleManager->saveRule($this->createNoReadRule("normal", $restrictedChild->getId())); + + $this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/folder/restricted.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/folder/restricted.txt")); + + $this->trashBackend->moveToTrash($folder->getStorage(), $folder->getInternalPath()); + + $this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/folder")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/folder")); + + // everyone can see the parent folder + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->managerUser)); + $managerTrashFolder = current($this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->normalUser)); + $normalTrashFolder = current($this->trashBackend->listTrashRoot($this->normalUser)); + + // only the manager can see the restricted child, both can see the un-restricted child + $this->assertCount(2, $this->trashBackend->listTrashFolder($managerTrashFolder)); + $this->assertCount(1, $this->trashBackend->listTrashFolder($normalTrashFolder)); + } + + public function testHideDeletedTrashItemInDeletedFolderAcl() { + $folder = $this->managerUserFolder->newFolder("{$this->folderName}/restricted"); + $child = $folder->newFile("file.txt", "content1"); + $this->ruleManager->saveRule($this->createNoReadRule("normal", $folder->getId())); + + $this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + + $this->trashBackend->moveToTrash($child->getStorage(), $child->getInternalPath()); + + $this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + + // only the manager can see the deleted child + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->normalUser)); + + $this->trashBackend->moveToTrash($folder->getStorage(), $folder->getInternalPath()); + + // only the manager can see the deleted items + $this->assertCount(2, $this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->normalUser)); + } + + public function testHideDeletedTrashItemInDeletedParentFolderAcl() { + $parent = $this->managerUserFolder->newFolder("{$this->folderName}/parent"); + $folder = $parent->newFolder("restricted"); + $child = $folder->newFile("file.txt", "content1"); + $this->ruleManager->saveRule($this->createNoReadRule("normal", $folder->getId())); + + $this->assertTrue($this->managerUserFolder->nodeExists("{$this->folderName}/parent/restricted/file.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/parent/restricted/file.txt")); + + $this->trashBackend->moveToTrash($child->getStorage(), $child->getInternalPath()); + + $this->assertFalse($this->managerUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + $this->assertFalse($this->normalUserFolder->nodeExists("{$this->folderName}/restricted/file.txt")); + + // only the manager can see the deleted child + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(0, $this->trashBackend->listTrashRoot($this->normalUser)); + + $this->trashBackend->moveToTrash($parent->getStorage(), $parent->getInternalPath()); + + // only the manager can see the deleted child, both can see the deleted parent + $this->assertCount(2, $this->trashBackend->listTrashRoot($this->managerUser)); + $this->assertCount(1, $this->trashBackend->listTrashRoot($this->normalUser)); + } +}