diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index ecd2397bdfd..5f7e0c97606 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -1070,7 +1070,7 @@ protected function formatParticipantList(array $participants, bool $includeStatu * @return DataResponse, array{}>|DataResponse, array{}>|DataResponse * * 200: Participant successfully added - * 400: Adding participant is not possible + * 400: Adding participant is not possible, e.g. when the user is banned (check error attribute of response for detail key) * 404: User, group or other target to invite was not found * 501: SIP dial-out is not configured */ @@ -1117,6 +1117,11 @@ public function addParticipantToRoom(string $newParticipant, string $source = 'u return new DataResponse([], Http::STATUS_NOT_FOUND); } + //Check if the user is banned + if ($this->banService->isActorBanned($this->room, Attendee::ACTOR_USERS, $newUser->getUID())) { + return new DataResponse(['error' => 'ban'], Http::STATUS_BAD_REQUEST); + } + $participantsToAdd[] = [ 'actorType' => Attendee::ACTOR_USERS, 'actorId' => $newUser->getUID(), diff --git a/lib/Listener/AMembershipListener.php b/lib/Listener/AMembershipListener.php index f0d6ae6dce9..30286d510da 100644 --- a/lib/Listener/AMembershipListener.php +++ b/lib/Listener/AMembershipListener.php @@ -16,6 +16,7 @@ use OCA\Talk\Model\Attendee; use OCA\Talk\Participant; use OCA\Talk\Room; +use OCA\Talk\Service\BanService; use OCA\Talk\Service\ParticipantService; use OCP\App\IAppManager; use OCP\EventDispatcher\Event; @@ -35,6 +36,8 @@ public function __construct( protected IAppManager $appManager, protected IGroupManager $groupManager, protected ParticipantService $participantService, + protected BanService $banService, + protected LoggerInterface $logger, ) { } diff --git a/lib/Listener/CircleMembershipListener.php b/lib/Listener/CircleMembershipListener.php index 42439fb9ec7..ec9fae3433a 100644 --- a/lib/Listener/CircleMembershipListener.php +++ b/lib/Listener/CircleMembershipListener.php @@ -15,6 +15,7 @@ use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; use OCA\Talk\Participant; +use OCA\Talk\Service\BanService; use OCA\Talk\Service\ParticipantService; use OCP\App\IAppManager; use OCP\EventDispatcher\Event; @@ -22,6 +23,7 @@ use OCP\ISession; use OCP\IUser; use OCP\IUserManager; +use Psr\Log\LoggerInterface; class CircleMembershipListener extends AMembershipListener { @@ -30,6 +32,8 @@ public function __construct( IAppManager $appManager, IGroupManager $groupManager, ParticipantService $participantService, + BanService $banService, + LoggerInterface $logger, private IUserManager $userManager, private ISession $session, ) { @@ -37,7 +41,9 @@ public function __construct( $manager, $appManager, $groupManager, - $participantService + $participantService, + $banService, + $logger, ); } @@ -101,6 +107,10 @@ protected function addingCircleMemberEvent(AddingCircleMemberEvent $event): void } protected function addNewMemberToRooms(array $rooms, Member $member): void { + if (empty($rooms)) { + return; + } + if ($member->getUserType() !== Member::TYPE_USER || $member->getUserId() === '') { // Not a user? return; @@ -111,7 +121,13 @@ protected function addNewMemberToRooms(array $rooms, Member $member): void { return; } + $bannedRoomIds = $this->banService->getBannedRoomsForUserId($user->getUID()); foreach ($rooms as $room) { + if (isset($bannedRoomIds[$room->getId()])) { + $this->logger->warning('User ' . $user->getUID() . ' is banned from conversation ' . $room->getToken() . ' and was skipped while adding them to circle ' . $member->getCircle()->getDisplayName()); + continue; + } + try { $participant = $room->getParticipant($member->getUserId()); if ($participant->getAttendee()->getParticipantType() === Participant::USER_SELF_JOINED) { diff --git a/lib/Listener/GroupMembershipListener.php b/lib/Listener/GroupMembershipListener.php index 20adb8c0d3c..5b9b8393c8a 100644 --- a/lib/Listener/GroupMembershipListener.php +++ b/lib/Listener/GroupMembershipListener.php @@ -30,7 +30,17 @@ public function handle(Event $event): void { protected function addNewMemberToRooms(IGroup $group, IUser $user): void { $rooms = $this->manager->getRoomsForActor(Attendee::ACTOR_GROUPS, $group->getGID()); + if (empty($rooms)) { + return; + } + + $bannedRoomIds = $this->banService->getBannedRoomsForUserId($user->getUID()); foreach ($rooms as $room) { + if (isset($bannedRoomIds[$room->getId()])) { + $this->logger->warning('User ' . $user->getUID() . ' is banned from conversation ' . $room->getToken() . ' and was skipped while adding them to group ' . $group->getDisplayName()); + continue; + } + try { $participant = $this->participantService->getParticipant($room, $user->getUID()); if ($participant->getAttendee()->getParticipantType() === Participant::USER_SELF_JOINED) { diff --git a/lib/Model/BanMapper.php b/lib/Model/BanMapper.php index 5a539926dcc..52f52aff9f5 100644 --- a/lib/Model/BanMapper.php +++ b/lib/Model/BanMapper.php @@ -53,6 +53,17 @@ public function findByRoomId(int $roomId, ?string $bannedActorType = null): arra return $this->findEntities($query); } + public function findByUserId(string $userId): array { + $query = $this->db->getQueryBuilder(); + $query->select('*') + ->from($this->getTableName()) + ->where($query->expr()->eq('banned_actor_type', $query->createNamedParameter(Attendee::ACTOR_USERS, IQueryBuilder::PARAM_STR))) + ->andWhere($query->expr()->eq('banned_actor_id', $query->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->orderBy('id', 'ASC'); + + return $this->findEntities($query); + } + /** * @throws DoesNotExistException */ diff --git a/lib/Service/BanService.php b/lib/Service/BanService.php index 4802d36d656..09655f48022 100644 --- a/lib/Service/BanService.php +++ b/lib/Service/BanService.php @@ -188,6 +188,20 @@ public function throwIfActorIsBanned(Room $room, ?string $userId): void { } } + /** + * Check if the actor is banned without logging + * + * @return bool True if the actor is banned, false otherwise + */ + public function isActorBanned(Room $room, string $actorType, string $actorId): bool { + try { + $this->banMapper->findForBannedActorAndRoom($actorType, $actorId, $room->getId()); + return true; + } catch (DoesNotExistException) { + return false; + } + } + /** * Retrieve all bans for a specific room. * @@ -197,6 +211,26 @@ public function getBansForRoom(int $roomId): array { return $this->banMapper->findByRoomId($roomId); } + /** + * Retrieve all banned userIDs for a specific room. + * + * @return array Key is the user ID + */ + public function getBannedUserIdsForRoom(int $roomId): array { + $bans = $this->banMapper->findByRoomId($roomId, Attendee::ACTOR_USERS); + return array_flip(array_map(static fn (Ban $ban) => $ban->getBannedActorId(), $bans)); + } + + /** + * Retrieve all room IDs a user is banned from + * + * @return array Key is the room ID + */ + public function getBannedRoomsForUserId(string $userId): array { + $bans = $this->banMapper->findByUserId($userId); + return array_flip(array_map(static fn (Ban $ban) => $ban->getRoomId(), $bans)); + } + /** * Retrieve a ban by its ID and delete it. */ diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index 13249741675..3e368b8fd1c 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -73,6 +73,7 @@ use OCP\Server; use OCP\UserStatus\IManager as IUserStatusManager; use OCP\UserStatus\IUserStatus; +use Psr\Log\LoggerInterface; class ParticipantService { @@ -98,6 +99,7 @@ public function __construct( private ITimeFactory $timeFactory, private ICacheFactory $cacheFactory, private IUserStatusManager $userStatusManager, + private LoggerInterface $logger, ) { } @@ -456,7 +458,7 @@ public function joinRoomAsNewGuest(RoomService $roomService, Room $room, string * @throws CannotReachRemoteException thrown when sending the federation request didn't work * @throws \Exception thrown if $addedBy is not set when adding a federated user */ - public function addUsers(Room $room, array $participants, ?IUser $addedBy = null): array { + public function addUsers(Room $room, array $participants, ?IUser $addedBy = null, bool $bansAlreadyChecked = false): array { if (empty($participants)) { return []; } @@ -466,10 +468,20 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null $lastMessage = (int) $room->getLastMessage()->getId(); } + $bannedUserIds = []; + if (!$bansAlreadyChecked) { + $banService = Server::get(BanService::class); + $bannedUserIds = $banService->getBannedUserIdsForRoom($room->getId()); + } $attendees = []; foreach ($participants as $participant) { $readPrivacy = Participant::PRIVACY_PUBLIC; if ($participant['actorType'] === Attendee::ACTOR_USERS) { + if (isset($bannedUserIds[$participant['actorId']])) { + $this->logger->warning('User ' . $participant['actorId'] . ' is banned from conversation ' . $room->getToken() . ' and was skipped while adding users'); + continue; + } + $readPrivacy = $this->talkConfig->getUserReadPrivacy($participant['actorId']); } elseif ($participant['actorType'] === Attendee::ACTOR_FEDERATED_USERS) { if ($addedBy === null) { @@ -603,6 +615,8 @@ public function addGroup(Room $room, IGroup $group, array $existingParticipants $existingParticipants = $this->getParticipantsForRoom($room); } + $banService = Server::get(BanService::class); + $bannedUserIds = $banService->getBannedUserIdsForRoom($room->getId()); $participantsByUserId = []; foreach ($existingParticipants as $participant) { if ($participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) { @@ -622,6 +636,11 @@ public function addGroup(Room $room, IGroup $group, array $existingParticipants continue; } + if (isset($bannedUserIds[$user->getUID()])) { + $this->logger->warning('User ' . $user->getUID() . ' is banned from conversation ' . $room->getToken() . ' and was skipped while adding group ' . $group->getDisplayName()); + continue; + } + $newParticipants[] = [ 'actorType' => Attendee::ACTOR_USERS, 'actorId' => $user->getUID(), @@ -646,7 +665,7 @@ public function addGroup(Room $room, IGroup $group, array $existingParticipants $this->dispatcher->dispatchTyped($attendeeEvent); } - $this->addUsers($room, $newParticipants); + $this->addUsers($room, $newParticipants, bansAlreadyChecked: true); } /** @@ -688,6 +707,8 @@ public function addCircle(Room $room, Circle $circle, array $existingParticipant $existingParticipants = $this->getParticipantsForRoom($room); } + $banService = Server::get(BanService::class); + $bannedUserIds = $banService->getBannedUserIdsForRoom($room->getId()); $participantsByUserId = []; foreach ($existingParticipants as $participant) { if ($participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) { @@ -723,6 +744,11 @@ public function addCircle(Room $room, Circle $circle, array $existingParticipant continue; } + if (isset($bannedUserIds[$user->getUID()])) { + $this->logger->warning('User ' . $user->getUID() . ' is banned from conversation ' . $room->getToken() . ' and was skipped while adding circle ' . $circle->getDisplayName()); + continue; + } + $newParticipants[] = [ 'actorType' => Attendee::ACTOR_USERS, 'actorId' => $user->getUID(), @@ -747,7 +773,7 @@ public function addCircle(Room $room, Circle $circle, array $existingParticipant $this->dispatcher->dispatchTyped($attendeeEvent); } - $this->addUsers($room, $newParticipants); + $this->addUsers($room, $newParticipants, bansAlreadyChecked: true); } /** diff --git a/openapi-full.json b/openapi-full.json index bfd878b1007..3fcdb8046b5 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -12557,7 +12557,7 @@ } }, "400": { - "description": "Adding participant is not possible", + "description": "Adding participant is not possible, e.g. when the user is banned (check error attribute of response for detail key)", "content": { "application/json": { "schema": { diff --git a/openapi.json b/openapi.json index 47e89cb2bbe..a2ec3fccc4e 100644 --- a/openapi.json +++ b/openapi.json @@ -12677,7 +12677,7 @@ } }, "400": { - "description": "Adding participant is not possible", + "description": "Adding participant is not possible, e.g. when the user is banned (check error attribute of response for detail key)", "content": { "application/json": { "schema": { diff --git a/src/types/openapi/openapi-full.ts b/src/types/openapi/openapi-full.ts index 2b83c4f7269..66a1a12895f 100644 --- a/src/types/openapi/openapi-full.ts +++ b/src/types/openapi/openapi-full.ts @@ -6759,7 +6759,7 @@ export interface operations { }; }; }; - /** @description Adding participant is not possible */ + /** @description Adding participant is not possible, e.g. when the user is banned (check error attribute of response for detail key) */ 400: { headers: { [name: string]: unknown; diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e19e43a62ef..d6a32a2db6c 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -6337,7 +6337,7 @@ export interface operations { }; }; }; - /** @description Adding participant is not possible */ + /** @description Adding participant is not possible, e.g. when the user is banned (check error attribute of response for detail key) */ 400: { headers: { [name: string]: unknown; diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index f4ccf67dab3..85e512e66d3 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -900,6 +900,7 @@ protected function assertAttendeeList(string $identifier, ?TableNode $formData, } return $attendee; }, $formData->getHash(), $result); + $expected = array_filter($expected); $result = array_map(function ($attendee) { if (isset($attendee['permissions'])) { @@ -918,7 +919,7 @@ protected function assertAttendeeList(string $identifier, ?TableNode $formData, 'expected' => $expected, 'actual' => $attendees, 'result' => $result, - ])); + ], true)); } else { Assert::assertNull($formData); } diff --git a/tests/integration/features/conversation-1/ban.feature b/tests/integration/features/conversation-1/ban.feature index 8e9a24b2651..3746516f2b1 100644 --- a/tests/integration/features/conversation-1/ban.feature +++ b/tests/integration/features/conversation-1/ban.feature @@ -3,8 +3,7 @@ Feature: conversation/ban Given user "participant1" exists Given user "participant2" exists Given user "participant3" exists - And guest accounts can be created - And user "user-guest@example.com" is a guest account user + Given group "group1" exists Scenario: Moderator banning and unbanning multiple users Given user "participant1" creates room "room" (v4) @@ -124,3 +123,59 @@ Feature: conversation/ban | moderatorActorType | moderatorActorId | moderatorDisplayName | bannedActorType | bannedActorId | bannedDisplayName | internalNote | | users | participant1 | participant1-displayname | guests | SESSION(guest1) | SESSION(guest1) | Banned guest | | users | participant1 | participant1-displayname | ip | LOCAL_IP | LOCAL_IP | Banned guest | + + Scenario: Banned user cannot be added to the room + Given user "participant1" creates room "room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" bans user "participant2" from room "room" with 200 (v1) + | internalNote | BannedP2 | + And user "participant1" sees the following bans in room "room" with 200 (v1) + | moderatorActorType | moderatorActorId | moderatorDisplayName | bannedActorType | bannedActorId | bannedDisplayName | internalNote | + | users | participant1 | participant1-displayname | users | participant2 | participant2-displayname | BannedP2 | + And user "participant1" adds user "participant2" to room "room" with 400 (v4) + Then user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | + | users | participant1 | + And user "participant1" unbans user "participant2" from room "room" with 200 (v1) + And user "participant1" adds user "participant2" to room "room" with 200 (v4) + Then user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | + | users | participant1 | + | users | participant2 | + + Scenario: Banned user is not added when adding a group they are a member of + Given user "participant1" creates room "room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant2" is member of group "group1" + And user "participant1" bans user "participant2" from room "room" with 200 (v1) + | internalNote | BannedP2 | + And user "participant1" sees the following bans in room "room" with 200 (v1) + | moderatorActorType | moderatorActorId | moderatorDisplayName | bannedActorType | bannedActorId | bannedDisplayName | internalNote | + | users | participant1 | participant1-displayname | users | participant2 | participant2-displayname | BannedP2 | + And user "participant1" adds group "group1" to room "room" with 200 (v4) + Then user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | + | users | participant1 | + | groups | group1 | + + Scenario: Banned user is not added when adding them to a group that is member in a room they are banned in + Given user "participant1" creates room "room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" bans user "participant2" from room "room" with 200 (v1) + | internalNote | BannedP2 | + And user "participant1" sees the following bans in room "room" with 200 (v1) + | moderatorActorType | moderatorActorId | moderatorDisplayName | bannedActorType | bannedActorId | bannedDisplayName | internalNote | + | users | participant1 | participant1-displayname | users | participant2 | participant2-displayname | BannedP2 | + And user "participant1" adds group "group1" to room "room" with 200 (v4) + Then user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | + | users | participant1 | + | groups | group1 | + And user "participant2" is member of group "group1" + Then user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | + | users | participant1 | + | groups | group1 | diff --git a/tests/php/Service/ParticipantServiceTest.php b/tests/php/Service/ParticipantServiceTest.php index 9a68aae11f1..a024502ed0b 100644 --- a/tests/php/Service/ParticipantServiceTest.php +++ b/tests/php/Service/ParticipantServiceTest.php @@ -31,6 +31,7 @@ use OCP\Security\ISecureRandom; use OCP\UserStatus\IManager; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; /** @@ -53,6 +54,7 @@ class ParticipantServiceTest extends TestCase { protected ICacheFactory&MockObject $cacheFactory; protected IManager&MockObject $userStatusManager; private ?ParticipantService $service = null; + protected LoggerInterface&MockObject $logger; public function setUp(): void { @@ -73,6 +75,7 @@ public function setUp(): void { $this->time = $this->createMock(ITimeFactory::class); $this->cacheFactory = $this->createMock(ICacheFactory::class); $this->userStatusManager = $this->createMock(IManager::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->service = new ParticipantService( $this->serverConfig, $this->talkConfig, @@ -90,6 +93,7 @@ public function setUp(): void { $this->time, $this->cacheFactory, $this->userStatusManager, + $this->logger ); }