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); + } }