From bd7e6b7e34f922e5c2ed721588b3f8bbc2200c8e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sat, 31 Oct 2026 18:40:38 +0100 Subject: [PATCH 1/2] fix(deletion): Clean up bans after deleting a user Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Joas Schilling --- lib/Listener/UserDeletedListener.php | 6 ++++ lib/Model/AttachmentMapper.php | 11 +++++++ lib/Model/BanMapper.php | 18 +++++++++++ lib/Service/AttachmentService.php | 4 +++ lib/Service/BanService.php | 7 +++++ lib/Service/RoomService.php | 6 ++++ .../conversation-5/delete-user.feature | 20 ++++++++++++ tests/php/Service/RoomServiceTest.php | 31 +++++++++++++++++++ 8 files changed, 103 insertions(+) diff --git a/lib/Listener/UserDeletedListener.php b/lib/Listener/UserDeletedListener.php index 08033cdc267..529f2780644 100644 --- a/lib/Listener/UserDeletedListener.php +++ b/lib/Listener/UserDeletedListener.php @@ -12,6 +12,8 @@ use OCA\Talk\Manager; use OCA\Talk\Matrix\Service\AccountService; use OCA\Talk\Model\Attendee; +use OCA\Talk\Service\AttachmentService; +use OCA\Talk\Service\BanService; use OCA\Talk\Service\ConsentService; use OCA\Talk\Service\ConversationTagService; use OCA\Talk\Service\PhoneService; @@ -37,6 +39,8 @@ public function __construct( private readonly ReminderService $reminderService, private readonly FederationManager $federationManager, private readonly AccountService $matrixAccountService, + private readonly BanService $banService, + private readonly AttachmentService $attachmentService, ) { } @@ -58,5 +62,7 @@ public function handle(Event $event): void { $this->reminderService->deleteAllRemindersForUser($user->getUID()); $this->federationManager->deleteInvitationsForUser($user->getUID()); $this->matrixAccountService->unlinkUser($user->getUID()); + $this->banService->neutralizeDeletedUser(Attendee::ACTOR_USERS, $user->getUID()); + $this->attachmentService->neutralizeDeletedUser(Attendee::ACTOR_USERS, $user->getUID()); } } diff --git a/lib/Model/AttachmentMapper.php b/lib/Model/AttachmentMapper.php index 1ef2b6ce133..d1bb1c9ec2d 100644 --- a/lib/Model/AttachmentMapper.php +++ b/lib/Model/AttachmentMapper.php @@ -10,6 +10,7 @@ use OCP\AppFramework\Db\QBMapper; use OCP\AppFramework\Db\TTransactional; +use OCP\Comments\ICommentsManager; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -75,4 +76,14 @@ public function deleteByRoomId(int $roomId): void { $query->executeStatement(); }, $this->db); } + + public function neutralizeDeletedUser(string $actorType, string $actorId): void { + $update = $this->db->getQueryBuilder(); + $update->update($this->getTableName()) + ->set('actor_type', $update->createNamedParameter(ICommentsManager::DELETED_USER)) + ->set('actor_id', $update->createNamedParameter(ICommentsManager::DELETED_USER)) + ->where($update->expr()->eq('actor_type', $update->createNamedParameter($actorType))) + ->andWhere($update->expr()->eq('actor_id', $update->createNamedParameter($actorId))); + $update->executeStatement(); + } } diff --git a/lib/Model/BanMapper.php b/lib/Model/BanMapper.php index 46295244973..df7fd550351 100644 --- a/lib/Model/BanMapper.php +++ b/lib/Model/BanMapper.php @@ -10,6 +10,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\QBMapper; +use OCP\Comments\ICommentsManager; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -95,4 +96,21 @@ public function updateDisplayNameForActor(string $actorType, string $actorId, st ->andWhere($update->expr()->eq('banned_actor_id', $update->createNamedParameter($actorId))); $update->executeStatement(); } + + public function neutralizeDeletedUser(string $actorType, string $actorId): void { + $delete = $this->db->getQueryBuilder(); + $delete->delete($this->getTableName()) + ->where($delete->expr()->eq('banned_actor_type', $delete->createNamedParameter($actorType))) + ->andWhere($delete->expr()->eq('banned_actor_id', $delete->createNamedParameter($actorId))); + $delete->executeStatement(); + + $update = $this->db->getQueryBuilder(); + $update->update($this->getTableName()) + ->set('moderator_displayname', $update->createNamedParameter('')) + ->set('moderator_actor_type', $update->createNamedParameter(ICommentsManager::DELETED_USER)) + ->set('moderator_actor_id', $update->createNamedParameter(ICommentsManager::DELETED_USER)) + ->where($update->expr()->eq('moderator_actor_type', $update->createNamedParameter($actorType))) + ->andWhere($update->expr()->eq('moderator_actor_id', $update->createNamedParameter($actorId))); + $update->executeStatement(); + } } diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index de7f90667b2..0dc5db94e48 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -92,4 +92,8 @@ public function deleteAttachmentByMessageId(int $messageId): void { public function deleteAttachmentsForRoom(Room $room): void { $this->attachmentMapper->deleteByRoomId($room->getId()); } + + public function neutralizeDeletedUser(string $actorType, string $actorId): void { + $this->attachmentMapper->neutralizeDeletedUser($actorType, $actorId); + } } diff --git a/lib/Service/BanService.php b/lib/Service/BanService.php index 9ad6c879e97..fb67ecf3b64 100644 --- a/lib/Service/BanService.php +++ b/lib/Service/BanService.php @@ -294,4 +294,11 @@ public function findAndDeleteBanByIdForRoom(int $banId, int $roomId): void { public function updateDisplayNameForActor(string $actorType, string $actorId, string $displayName): void { $this->banMapper->updateDisplayNameForActor($actorType, $actorId, $displayName); } + + /** + * Delete bans of the actor and anonymize bans the actor issued as moderator + */ + public function neutralizeDeletedUser(string $actorType, string $actorId): void { + $this->banMapper->neutralizeDeletedUser($actorType, $actorId); + } } diff --git a/lib/Service/RoomService.php b/lib/Service/RoomService.php index eba418f0e1c..3ba10562434 100644 --- a/lib/Service/RoomService.php +++ b/lib/Service/RoomService.php @@ -1576,6 +1576,12 @@ public function deleteRoom(Room $room): void { ->where($delete->expr()->eq('room_id', $delete->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))); $delete->executeStatement(); + // Delete bans + $delete = $this->db->getQueryBuilder(); + $delete->delete('talk_bans') + ->where($delete->expr()->eq('room_id', $delete->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))); + $delete->executeStatement(); + // Clear share access cache foreach ($userIds as $userId) { $user = new LazyUser($userId, $this->userManager); diff --git a/tests/integration/features/conversation-5/delete-user.feature b/tests/integration/features/conversation-5/delete-user.feature index f0285770e75..993031258ec 100644 --- a/tests/integration/features/conversation-5/delete-user.feature +++ b/tests/integration/features/conversation-5/delete-user.feature @@ -74,3 +74,23 @@ Feature: conversation-5/delete-user | roomName | public room | When user "participant2" is deleted Then user "participant1" joins room "public room" with 404 (v4) + + Scenario: delete user who is banned or banned others + Given user "participant3" exists + Given user "participant4" exists + Given user "participant1" creates room "room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" adds user "participant3" to room "room" with 200 (v4) + And user "participant1" promotes "participant3" in room "room" with 200 (v4) + And user "participant2" joins room "room" with 200 (v4) + And user "participant4" joins room "room" with 200 (v4) + And user "participant3" bans user "participant2" from room "room" with 200 (v1) + | internalNote | BannedP2 | + And user "participant3" bans user "participant4" from room "room" with 200 (v1) + | internalNote | BannedP4 | + When user "participant2" is deleted + And user "participant3" is deleted + Then user "participant1" sees the following bans in room "room" with 200 (v1) + | moderatorActorType | moderatorActorId | moderatorDisplayName | bannedActorType | bannedActorId | bannedDisplayName | internalNote | + | deleted_users | deleted_users | | users | participant4 | participant4-displayname | BannedP4 | diff --git a/tests/php/Service/RoomServiceTest.php b/tests/php/Service/RoomServiceTest.php index 282f376a6fd..f16d82adc80 100644 --- a/tests/php/Service/RoomServiceTest.php +++ b/tests/php/Service/RoomServiceTest.php @@ -19,6 +19,8 @@ use OCA\Talk\Exceptions\RoomProperty\TypeException; use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; +use OCA\Talk\Model\Ban; +use OCA\Talk\Model\BanMapper; use OCA\Talk\Model\BreakoutRoom; use OCA\Talk\Participant; use OCA\Talk\Room; @@ -618,4 +620,33 @@ public function testDeleteRoomLogsCriticalAction(): void { 'objectId' => '42', ], $auditEvents[0]->getParameters()); } + + public function testDeleteRoomDeletesBans(): void { + $banMapper = new BanMapper(Server::get(IDBConnection::class)); + $roomId = random_int(1_000_000, 9_000_000); + $otherRoomId = $roomId + 1; + foreach ([$roomId, $otherRoomId] as $banRoomId) { + $ban = new Ban(); + $ban->setRoomId($banRoomId); + $ban->setModeratorActorType(Attendee::ACTOR_USERS); + $ban->setModeratorActorId('moderator'); + $ban->setBannedActorType(Attendee::ACTOR_USERS); + $ban->setBannedActorId('banned'); + $ban->setBannedTime(new \DateTime()); + $banMapper->insert($ban); + } + + $room = $this->createMock(Room::class); + $room->method('getId')->willReturn($roomId); + $room->method('getBreakoutRoomMode')->willReturn(BreakoutRoom::MODE_NOT_CONFIGURED); + $room->method('isFederatedConversation')->willReturn(false); + $this->participantService->method('getParticipantUserIds')->willReturn([]); + + $this->service->deleteRoom($room); + + $this->assertSame([], $banMapper->findByRoomId($roomId)); + $otherBans = $banMapper->findByRoomId($otherRoomId); + $this->assertCount(1, $otherBans); + $banMapper->delete($otherBans[0]); + } } From 2320e7fb809460042454ed38380d7c8e77dc9e7e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sat, 31 Oct 2026 18:47:42 +0100 Subject: [PATCH 2/2] fix(room): Clean up avatar and bots when deleting a converesation Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Joas Schilling --- lib/Service/AvatarService.php | 14 ++++++++++- lib/Service/RoomService.php | 10 ++++++++ tests/php/Service/AvatarServiceTest.php | 31 +++++++++++++++++++++++++ tests/php/Service/RoomServiceTest.php | 27 +++++++++++++++++++-- 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/lib/Service/AvatarService.php b/lib/Service/AvatarService.php index 123d43b6092..22e7b750f99 100644 --- a/lib/Service/AvatarService.php +++ b/lib/Service/AvatarService.php @@ -301,12 +301,24 @@ private function getAvatarPath(Room $room, bool $darkTheme = false): string { } public function deleteAvatar(Room $room): void { + if ($this->deleteAvatarFolder($room)) { + $this->roomService->setAvatar($room, ''); + } + } + + /** + * Delete the stored avatar files without updating the room + * + * @return bool Whether an avatar folder existed + */ + public function deleteAvatarFolder(Room $room): bool { try { $folder = $this->appData->getFolder('room-avatar'); $avatarFolder = $folder->getFolder($room->getToken()); $avatarFolder->delete(); - $this->roomService->setAvatar($room, ''); + return true; } catch (NotFoundException) { + return false; } } diff --git a/lib/Service/RoomService.php b/lib/Service/RoomService.php index 3ba10562434..771d45f86c7 100644 --- a/lib/Service/RoomService.php +++ b/lib/Service/RoomService.php @@ -1582,6 +1582,16 @@ public function deleteRoom(Room $room): void { ->where($delete->expr()->eq('room_id', $delete->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))); $delete->executeStatement(); + // Delete bot installations + $delete = $this->db->getQueryBuilder(); + $delete->delete('talk_bots_conversation') + ->where($delete->expr()->eq('token', $delete->createNamedParameter($room->getToken()))); + $delete->executeStatement(); + + // AvatarService depends on RoomService, so can not use DI + $avatarService = Server::get(AvatarService::class); + $avatarService->deleteAvatarFolder($room); + // Clear share access cache foreach ($userIds as $userId) { $user = new LazyUser($userId, $this->userManager); diff --git a/tests/php/Service/AvatarServiceTest.php b/tests/php/Service/AvatarServiceTest.php index c0ce366d8bc..f5c3195b7f4 100644 --- a/tests/php/Service/AvatarServiceTest.php +++ b/tests/php/Service/AvatarServiceTest.php @@ -15,6 +15,8 @@ use OCP\Config\IUserConfig; use OCP\Files\IAppData; use OCP\Files\IFilenameValidator; +use OCP\Files\NotFoundException; +use OCP\Files\SimpleFS\ISimpleFolder; use OCP\IAvatarManager; use OCP\IL10N; use OCP\IURLGenerator; @@ -101,4 +103,33 @@ public function testGetAvatarVersion(string $avatar, string $expected): void { $this->assertEquals($expected, $actual); } } + + public function testDeleteAvatar(): void { + $room = $this->createMock(Room::class); + $room->method('getToken')->willReturn('abcdef'); + + $avatarFolder = $this->createMock(ISimpleFolder::class); + $avatarFolder->expects($this->once())->method('delete'); + $folder = $this->createMock(ISimpleFolder::class); + $folder->method('getFolder')->with('abcdef')->willReturn($avatarFolder); + $this->appData->method('getFolder')->with('room-avatar')->willReturn($folder); + + $this->roomService->expects($this->once())->method('setAvatar')->with($room, ''); + + $this->service->deleteAvatar($room); + } + + public function testDeleteAvatarWithoutFolder(): void { + $room = $this->createMock(Room::class); + $room->method('getToken')->willReturn('abcdef'); + + $folder = $this->createMock(ISimpleFolder::class); + $folder->method('getFolder')->willThrowException(new NotFoundException()); + $this->appData->method('getFolder')->willReturn($folder); + + $this->roomService->expects($this->never())->method('setAvatar'); + + $this->service->deleteAvatar($room); + $this->assertFalse($this->service->deleteAvatarFolder($room)); + } } diff --git a/tests/php/Service/RoomServiceTest.php b/tests/php/Service/RoomServiceTest.php index f16d82adc80..93c2a84723b 100644 --- a/tests/php/Service/RoomServiceTest.php +++ b/tests/php/Service/RoomServiceTest.php @@ -21,10 +21,13 @@ use OCA\Talk\Model\Attendee; use OCA\Talk\Model\Ban; use OCA\Talk\Model\BanMapper; +use OCA\Talk\Model\BotConversation; +use OCA\Talk\Model\BotConversationMapper; use OCA\Talk\Model\BreakoutRoom; use OCA\Talk\Participant; use OCA\Talk\Room; use OCA\Talk\RoomAttributes; +use OCA\Talk\Service\AvatarService; use OCA\Talk\Service\EmojiService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\RecordingService; @@ -621,8 +624,9 @@ public function testDeleteRoomLogsCriticalAction(): void { ], $auditEvents[0]->getParameters()); } - public function testDeleteRoomDeletesBans(): void { + public function testDeleteRoomDeletesRoomData(): void { $banMapper = new BanMapper(Server::get(IDBConnection::class)); + $botConversationMapper = new BotConversationMapper(Server::get(IDBConnection::class)); $roomId = random_int(1_000_000, 9_000_000); $otherRoomId = $roomId + 1; foreach ([$roomId, $otherRoomId] as $banRoomId) { @@ -634,19 +638,38 @@ public function testDeleteRoomDeletesBans(): void { $ban->setBannedActorId('banned'); $ban->setBannedTime(new \DateTime()); $banMapper->insert($ban); + + $bot = new BotConversation(); + $bot->setBotId(1); + $bot->setToken('t' . $banRoomId); + $botConversationMapper->insert($bot); } $room = $this->createMock(Room::class); $room->method('getId')->willReturn($roomId); + $room->method('getToken')->willReturn('t' . $roomId); $room->method('getBreakoutRoomMode')->willReturn(BreakoutRoom::MODE_NOT_CONFIGURED); $room->method('isFederatedConversation')->willReturn(false); $this->participantService->method('getParticipantUserIds')->willReturn([]); - $this->service->deleteRoom($room); + $avatarService = $this->createMock(AvatarService::class); + $avatarService->expects($this->once())->method('deleteAvatarFolder')->with($room); + $this->overwriteService(AvatarService::class, $avatarService); + + try { + $this->service->deleteRoom($room); + } finally { + $this->restoreService(AvatarService::class); + } $this->assertSame([], $banMapper->findByRoomId($roomId)); + $this->assertSame([], $botConversationMapper->findForToken('t' . $roomId)); + $otherBans = $banMapper->findByRoomId($otherRoomId); $this->assertCount(1, $otherBans); $banMapper->delete($otherBans[0]); + $otherBots = $botConversationMapper->findForToken('t' . $otherRoomId); + $this->assertCount(1, $otherBots); + $botConversationMapper->delete($otherBots[0]); } }