From de91c32826e2487fb5b52dd55f9615a62e646b59 Mon Sep 17 00:00:00 2001 From: Fabio Fantoni Date: Mon, 13 Jul 2026 13:46:18 +0200 Subject: [PATCH] fix(files_sharing): don't abort share:list when orphaned shares exist `occ share:list` resolved the source node of every share without handling NotFoundException, so a single orphaned share (e.g. the source file was deleted from disk externally, or is pending removal by DeleteOrphanedSharesJob) aborted the whole command with "Node for share not found" and produced no output at all. Catch the exception in both places that resolve the share node: orphaned shares are now listed with a null source-path, and the --parent filter skips them instead of crashing. Add unit tests for the command (previously untested), covering listing with an orphaned share present and the --parent filter with and without --recursive. Closes #61949 Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Fabio Fantoni --- apps/files_sharing/lib/Command/ListShares.php | 37 +++-- .../tests/Command/ListSharesTest.php | 152 ++++++++++++++++++ 2 files changed, 176 insertions(+), 13 deletions(-) create mode 100644 apps/files_sharing/tests/Command/ListSharesTest.php diff --git a/apps/files_sharing/lib/Command/ListShares.php b/apps/files_sharing/lib/Command/ListShares.php index 4a13aa92fa536..ea7361a57357a 100644 --- a/apps/files_sharing/lib/Command/ListShares.php +++ b/apps/files_sharing/lib/Command/ListShares.php @@ -72,11 +72,17 @@ public function execute(InputInterface $input, OutputInterface $output): int { }); $shares = iterator_to_array($shares); $data = array_map(function (IShare $share) { + try { + $sourcePath = $share->getNode()->getPath(); + } catch (NotFoundException) { + // orphaned share, source file no longer exists + $sourcePath = null; + } return [ 'id' => $share->getId(), 'file' => $share->getNodeId(), 'target-path' => $share->getTarget(), - 'source-path' => $share->getNode()->getPath(), + 'source-path' => $sourcePath, 'owner' => $share->getShareOwner(), 'recipient' => $share->getSharedWith(), 'by' => $share->getSharedBy(), @@ -140,19 +146,24 @@ private function shouldShowShare(InputInterface $input, IShare $share): bool { throw new \Exception("Parent {$parent->getPath()} is not a folder"); } $recursive = $input->getOption('recursive'); - if (!$recursive) { - $shareCacheEntry = $share->getNodeCacheEntry(); - if (!$shareCacheEntry) { - $shareCacheEntry = $share->getNode(); - } - if ($shareCacheEntry->getParentId() !== $parent->getId()) { - return false; - } - } else { - $shareNode = $share->getNode(); - if ($parent->getRelativePath($shareNode->getPath()) === null) { - return false; + try { + if (!$recursive) { + $shareCacheEntry = $share->getNodeCacheEntry(); + if (!$shareCacheEntry) { + $shareCacheEntry = $share->getNode(); + } + if ($shareCacheEntry->getParentId() !== $parent->getId()) { + return false; + } + } else { + $shareNode = $share->getNode(); + if ($parent->getRelativePath($shareNode->getPath()) === null) { + return false; + } } + } catch (NotFoundException) { + // orphaned share, can't be inside the parent folder + return false; } } if ($input->getOption('type') && $share->getShareType() !== $this->getShareType($input->getOption('type'))) { diff --git a/apps/files_sharing/tests/Command/ListSharesTest.php b/apps/files_sharing/tests/Command/ListSharesTest.php new file mode 100644 index 0000000000000..91cf310a51286 --- /dev/null +++ b/apps/files_sharing/tests/Command/ListSharesTest.php @@ -0,0 +1,152 @@ +shareManager = $this->createMock(IManager::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->command = new ListShares($this->shareManager, $this->rootFolder); + } + + private function createShare(int $id, ?Node $node, ?ICacheEntry $cacheEntry = null): IShare { + $share = $this->createMock(IShare::class); + $share->method('getId')->willReturn((string)$id); + $share->method('getNodeId')->willReturn($id * 100); + $share->method('getTarget')->willReturn("/target-$id"); + $share->method('getShareOwner')->willReturn('owner'); + $share->method('getSharedWith')->willReturn('recipient'); + $share->method('getSharedBy')->willReturn('owner'); + $share->method('getShareType')->willReturn(IShare::TYPE_USER); + $share->method('getNodeCacheEntry')->willReturn($cacheEntry); + if ($node === null) { + $share->method('getNode')->willThrowException(new NotFoundException('Node for share not found')); + } else { + $share->method('getNode')->willReturn($node); + } + return $share; + } + + private function createNode(string $path, int $parentId = 0): Node { + $node = $this->createMock(Node::class); + $node->method('getPath')->willReturn($path); + $node->method('getParentId')->willReturn($parentId); + return $node; + } + + private function createInput(array $options): InputInterface { + $options += [ + 'owner' => null, + 'recipient' => null, + 'by' => null, + 'file' => null, + 'parent' => null, + 'recursive' => false, + 'type' => null, + 'status' => null, + 'output' => 'json', + ]; + $input = $this->createMock(InputInterface::class); + $input->method('getOption') + ->willReturnCallback(fn (string $name) => $options[$name] ?? null); + return $input; + } + + public function testListIncludesOrphanShare(): void { + $shares = [ + $this->createShare(1, $this->createNode('/owner/files/document.txt')), + $this->createShare(2, null), + ]; + $this->shareManager->method('getAllShares') + ->willReturn(new \ArrayIterator($shares)); + + $output = new BufferedOutput(); + $exitCode = $this->command->execute($this->createInput([]), $output); + + $this->assertSame(0, $exitCode); + $rows = json_decode($output->fetch(), true); + $this->assertCount(2, $rows); + $this->assertSame('/owner/files/document.txt', $rows[0]['source-path']); + $this->assertNull($rows[1]['source-path']); + } + + public function testParentFilterSkipsOrphanShare(): void { + $parent = $this->createMock(Folder::class); + $parent->method('getId')->willReturn(42); + $this->rootFolder->method('get') + ->with('/owner/files/folder') + ->willReturn($parent); + + $cacheEntry = $this->createMock(ICacheEntry::class); + $cacheEntry->method('getParentId')->willReturn(42); + $shares = [ + $this->createShare(1, $this->createNode('/owner/files/folder/document.txt'), $cacheEntry), + $this->createShare(2, null), + ]; + $this->shareManager->method('getAllShares') + ->willReturn(new \ArrayIterator($shares)); + + $output = new BufferedOutput(); + $exitCode = $this->command->execute($this->createInput(['parent' => '/owner/files/folder']), $output); + + $this->assertSame(0, $exitCode); + $rows = json_decode($output->fetch(), true); + $this->assertCount(1, $rows); + $this->assertSame('1', $rows[0]['id']); + } + + public function testRecursiveParentFilterSkipsOrphanShare(): void { + $parent = $this->createMock(Folder::class); + $parent->method('getId')->willReturn(42); + $parent->method('getRelativePath') + ->willReturnCallback(fn (string $path) => str_starts_with($path, '/owner/files/folder/') + ? substr($path, strlen('/owner/files/folder')) + : null); + $this->rootFolder->method('get') + ->with('/owner/files/folder') + ->willReturn($parent); + + $shares = [ + $this->createShare(1, $this->createNode('/owner/files/folder/sub/document.txt')), + $this->createShare(2, null), + ]; + $this->shareManager->method('getAllShares') + ->willReturn(new \ArrayIterator($shares)); + + $output = new BufferedOutput(); + $exitCode = $this->command->execute( + $this->createInput(['parent' => '/owner/files/folder', 'recursive' => true]), + $output, + ); + + $this->assertSame(0, $exitCode); + $rows = json_decode($output->fetch(), true); + $this->assertCount(1, $rows); + $this->assertSame('1', $rows[0]['id']); + } +}