From 11c800572b34fb4f66efbbf5dcac173fbb18ee8c Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 30 Oct 2026 16:15:31 +0100 Subject: [PATCH] fix(contexts): delete the applications of a deleted user Deleting a user removed their tables but left their applications behind, with pages, shares and navigation entries, owned by nobody. The user deleted listener now deletes the applications the user owns through ContextService::delete(), before their tables. Applications only shared with the user, or owned by a group, stay. ShareMapper::findAllSharesForNode() accepts a null sender: it is not used in the query, but deleting an application without a session user, as occ user:delete does, passed null and the TypeError aborted the user deletion. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde --- lib/Db/ShareMapper.php | 4 +- lib/Listener/UserDeletedListener.php | 34 ++++- .../unit/Listener/UserDeletedListenerTest.php | 124 ++++++++++++++++++ tests/unit/Service/ShareServiceTest.php | 33 +++++ 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 tests/unit/Listener/UserDeletedListenerTest.php diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index c8a42db6bc..9ef0f35f5c 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -119,12 +119,12 @@ public function findAllSharesFor(string $nodeType, array $receivers, string $use /** * @param string $nodeType * @param int $nodeId - * @param string $sender + * @param string|null $sender * @param array $excluded receiver types to exclude from results * @return Share[] * @throws Exception */ - public function findAllSharesForNode(string $nodeType, int $nodeId, string $sender = '', array $excluded = []): array { + public function findAllSharesForNode(string $nodeType, int $nodeId, ?string $sender = '', array $excluded = []): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->table) diff --git a/lib/Listener/UserDeletedListener.php b/lib/Listener/UserDeletedListener.php index 69b1944611..1ae0cefe4d 100644 --- a/lib/Listener/UserDeletedListener.php +++ b/lib/Listener/UserDeletedListener.php @@ -7,8 +7,13 @@ namespace OCA\Tables\Listener; +use OCA\Tables\AppInfo\Application; +use OCA\Tables\Db\Context; use OCA\Tables\Errors\InternalError; +use OCA\Tables\Errors\NotFoundError; +use OCA\Tables\Service\ContextService; use OCA\Tables\Service\TableService; +use OCP\DB\Exception; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\User\Events\BeforeUserDeletedEvent; @@ -20,6 +25,7 @@ class UserDeletedListener implements IEventListener { public function __construct( private readonly TableService $tableService, + private readonly ContextService $contextService, private readonly LoggerInterface $logger, ) { } @@ -29,9 +35,13 @@ public function handle(Event $event): void { return; } + $this->logger->info('event "user deleted" was triggered, will try to delete all data for the user: ' . $event->getUser()->getUID() . ' (' . $event->getUser()->getDisplayName() . ')'); + + // applications first, so deleting the tables does not detach them from applications that go anyway + $this->deleteApplications($event->getUser()->getUID()); + try { $tables = $this->tableService->findAll($event->getUser()->getUID(), true, true, false); - $this->logger->info('event "user deleted" was triggered, will try to delete all data for the user: ' . $event->getUser()->getUID() . ' (' . $event->getUser()->getDisplayName() . ')'); // delete tables $this->logger->debug('found ' . count($tables) . ' tables for the user'); @@ -42,4 +52,26 @@ public function handle(Event $event): void { } catch (InternalError) { } } + + private function deleteApplications(string $userId): void { + try { + $contexts = array_filter( + $this->contextService->findAll($userId), + static fn (Context $context): bool => $context->getOwnerType() === Application::OWNER_TYPE_USER && $context->getOwnerId() === $userId, + ); + } catch (InternalError|Exception $e) { + $this->logger->error('could not find the applications of the deleted user ' . $userId, ['exception' => $e]); + return; + } + + $this->logger->debug('found ' . count($contexts) . ' applications for the user'); + foreach ($contexts as $context) { + try { + $this->contextService->delete($context->getId(), $userId); + } catch (NotFoundError|Exception $e) { + $this->logger->error('could not delete application ' . $context->getId() . ' of the deleted user ' . $userId, ['exception' => $e]); + } + } + $this->logger->debug('applications for the deleted user removed'); + } } diff --git a/tests/unit/Listener/UserDeletedListenerTest.php b/tests/unit/Listener/UserDeletedListenerTest.php new file mode 100644 index 0000000000..b0d23e2f51 --- /dev/null +++ b/tests/unit/Listener/UserDeletedListenerTest.php @@ -0,0 +1,124 @@ +tableService = $this->createMock(TableService::class); + $this->contextService = $this->createMock(ContextService::class); + + $this->listener = new UserDeletedListener( + $this->tableService, + $this->contextService, + $this->createMock(LoggerInterface::class), + ); + } + + public function testDeletesOnlyTheApplicationsTheUserOwns(): void { + $this->contextService->method('findAll')->with('alice')->willReturn([ + $this->context(1, 'alice', Application::OWNER_TYPE_USER), + $this->context(2, 'bob', Application::OWNER_TYPE_USER), + $this->context(3, 'alice', 1), + $this->context(4, 'alice', Application::OWNER_TYPE_USER), + ]); + $this->tableService->method('findAll')->willReturn([]); + + $deleted = []; + $this->contextService->expects($this->exactly(2))->method('delete') + ->willReturnCallback(function (int $contextId, string $userId) use (&$deleted): Context { + $deleted[] = [$contextId, $userId]; + return new Context(); + }); + + $this->listener->handle($this->event('alice')); + + $this->assertSame([[1, 'alice'], [4, 'alice']], $deleted); + } + + public function testDeletesApplicationsBeforeTables(): void { + $calls = []; + $this->contextService->method('findAll')->willReturn([$this->context(1, 'alice', Application::OWNER_TYPE_USER)]); + $this->contextService->method('delete')->willReturnCallback(function () use (&$calls): Context { + $calls[] = 'application'; + return new Context(); + }); + $table = new Table(); + $table->setId(7); + $this->tableService->method('findAll')->with('alice', true, true, false)->willReturn([$table]); + $this->tableService->method('delete')->with(7, 'alice')->willReturnCallback(function () use (&$calls, $table): Table { + $calls[] = 'table'; + return $table; + }); + + $this->listener->handle($this->event('alice')); + + $this->assertSame(['application', 'table'], $calls); + } + + public function testAFailingApplicationDoesNotStopTheRest(): void { + $this->contextService->method('findAll')->willReturn([ + $this->context(1, 'alice', Application::OWNER_TYPE_USER), + $this->context(2, 'alice', Application::OWNER_TYPE_USER), + ]); + $this->contextService->expects($this->exactly(2))->method('delete') + ->willReturnCallback(function (int $contextId): Context { + if ($contextId === 1) { + throw new NotFoundError('gone'); + } + return new Context(); + }); + $table = new Table(); + $table->setId(7); + $this->tableService->method('findAll')->willReturn([$table]); + $this->tableService->expects($this->once())->method('delete')->with(7, 'alice'); + + $this->listener->handle($this->event('alice')); + } + + public function testIgnoresOtherEvents(): void { + $this->contextService->expects($this->never())->method('findAll'); + $this->tableService->expects($this->never())->method('findAll'); + + $this->listener->handle(new Event()); + } + + private function context(int $id, string $ownerId, int $ownerType): Context { + $context = new Context(); + $context->setId($id); + $context->setOwnerId($ownerId); + $context->setOwnerType($ownerType); + return $context; + } + + private function event(string $userId): BeforeUserDeletedEvent { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($userId); + $user->method('getDisplayName')->willReturn($userId); + return new BeforeUserDeletedEvent($user); + } +} diff --git a/tests/unit/Service/ShareServiceTest.php b/tests/unit/Service/ShareServiceTest.php index 1ab87968e5..8eba75378b 100644 --- a/tests/unit/Service/ShareServiceTest.php +++ b/tests/unit/Service/ShareServiceTest.php @@ -10,6 +10,7 @@ namespace OCA\Tables\Tests\Unit\Service; use OCA\Tables\AuditLog\Operation; +use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextNavigationMapper; use OCA\Tables\Db\Share; use OCA\Tables\Db\ShareMapper; @@ -351,4 +352,36 @@ public function testDeleteForShareReviewPropagatesDbException(): void { $this->expectException(Exception::class); $this->shareService->deleteForShareReview(5); } + + public function testDeleteAllForContextWorksWithoutASessionUser(): void { + $shareService = new ShareService( + $this->permissionsService, + $this->logger, + null, + $this->mapper, + $this->tableMapper, + $this->viewMapper, + $this->userHelper, + $this->groupHelper, + $this->circleHelper, + $this->contextNavigationMapper, + $this->dbc, + $this->secureRandom, + $this->userManager, + $this->hasher, + $this->shareManager, + $this->federationService, + $this->auditLogService, + ); + $context = new Context(); + $context->setId(9); + $share = new Share(); + $share->setId(4); + + $this->mapper->method('findAllSharesForNode')->with('context', 9)->willReturn([$share]); + $this->contextNavigationMapper->expects($this->once())->method('deleteByShareId')->with(4); + $this->mapper->expects($this->once())->method('deleteByNode')->with(9, 'context'); + + $shareService->deleteAllForContext($context); + } }