From 36871453829632d08f3287a8ccf33ad9d80007f3 Mon Sep 17 00:00:00 2001 From: skalidindi53 Date: Wed, 24 Jul 2024 11:15:41 -0500 Subject: [PATCH 1/3] fix: Banned user cannot be added to the room Signed-off-by: skalidindi53 --- lib/Controller/RoomController.php | 7 ++++++- lib/Service/BanService.php | 14 ++++++++++++++ openapi-full.json | 2 +- openapi.json | 2 +- src/types/openapi/openapi-full.ts | 2 +- src/types/openapi/openapi.ts | 2 +- .../features/conversation-1/ban.feature | 16 ++++++++++++++++ 7 files changed, 40 insertions(+), 5 deletions(-) diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index ecd2397bdf..5f7e0c9760 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -1070,7 +1070,7 @@ class RoomController extends AEnvironmentAwareController { * @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 @@ class RoomController extends AEnvironmentAwareController { 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/Service/BanService.php b/lib/Service/BanService.php index 4802d36d65..cc6b33b289 100644 --- a/lib/Service/BanService.php +++ b/lib/Service/BanService.php @@ -188,6 +188,20 @@ class BanService { } } + /** + * 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. * diff --git a/openapi-full.json b/openapi-full.json index bfd878b100..3fcdb8046b 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 47e89cb2bb..a2ec3fccc4 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 2b83c4f726..66a1a12895 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 e19e43a62e..d6a32a2db6 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/conversation-1/ban.feature b/tests/integration/features/conversation-1/ban.feature index 8e9a24b265..1f96b68e19 100644 --- a/tests/integration/features/conversation-1/ban.feature +++ b/tests/integration/features/conversation-1/ban.feature @@ -124,3 +124,19 @@ 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 "participant2" joins room "room" with 200 (v4) + 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) + And user "participant1" sends message "Message 1" to room "room" with 201 + And user "participant2" sees the following messages in room "room" with 403 + And user "participant1" unbans user "participant2" from room "room" with 200 (v1) + And user "participant1" adds user "participant2" to room "room" with 200 (v4) From c66439d55948eb6c1a7f3da2a3ae8e325133cd3e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 2 Aug 2024 13:46:48 +0200 Subject: [PATCH 2/3] fix(ban): Skip banned users when adding groups or circles Signed-off-by: Joas Schilling --- lib/Listener/AMembershipListener.php | 3 ++ lib/Listener/CircleMembershipListener.php | 18 ++++++- lib/Listener/GroupMembershipListener.php | 10 ++++ lib/Model/BanMapper.php | 11 +++++ lib/Service/BanService.php | 20 ++++++++ lib/Service/ParticipantService.php | 33 +++++++++++-- .../features/bootstrap/FeatureContext.php | 3 +- .../features/conversation-1/ban.feature | 49 +++++++++++++++++-- 8 files changed, 137 insertions(+), 10 deletions(-) diff --git a/lib/Listener/AMembershipListener.php b/lib/Listener/AMembershipListener.php index f0d6ae6dce..30286d510d 100644 --- a/lib/Listener/AMembershipListener.php +++ b/lib/Listener/AMembershipListener.php @@ -16,6 +16,7 @@ use OCA\Talk\Manager; 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 @@ abstract class AMembershipListener implements IEventListener { 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 42439fb9ec..ec9fae3433 100644 --- a/lib/Listener/CircleMembershipListener.php +++ b/lib/Listener/CircleMembershipListener.php @@ -15,6 +15,7 @@ use OCA\Talk\Exceptions\ParticipantNotFoundException; 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\IGroupManager; use OCP\ISession; use OCP\IUser; use OCP\IUserManager; +use Psr\Log\LoggerInterface; class CircleMembershipListener extends AMembershipListener { @@ -30,6 +32,8 @@ class CircleMembershipListener extends AMembershipListener { IAppManager $appManager, IGroupManager $groupManager, ParticipantService $participantService, + BanService $banService, + LoggerInterface $logger, private IUserManager $userManager, private ISession $session, ) { @@ -37,7 +41,9 @@ class CircleMembershipListener extends AMembershipListener { $manager, $appManager, $groupManager, - $participantService + $participantService, + $banService, + $logger, ); } @@ -101,6 +107,10 @@ class CircleMembershipListener extends AMembershipListener { } 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 @@ class CircleMembershipListener extends AMembershipListener { 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 20adb8c0d3..5b9b8393c8 100644 --- a/lib/Listener/GroupMembershipListener.php +++ b/lib/Listener/GroupMembershipListener.php @@ -30,7 +30,17 @@ class GroupMembershipListener extends AMembershipListener { 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 5a539926dc..52f52aff9f 100644 --- a/lib/Model/BanMapper.php +++ b/lib/Model/BanMapper.php @@ -53,6 +53,17 @@ class BanMapper extends QBMapper { 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 cc6b33b289..09655f4802 100644 --- a/lib/Service/BanService.php +++ b/lib/Service/BanService.php @@ -211,6 +211,26 @@ class BanService { 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 1324974167..c9f0515e9a 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -47,6 +47,7 @@ use OCA\Talk\Federation\FederationManager; use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; use OCA\Talk\Model\AttendeeMapper; +use OCA\Talk\Model\Ban; use OCA\Talk\Model\BreakoutRoom; use OCA\Talk\Model\SelectHelper; use OCA\Talk\Model\Session; @@ -73,6 +74,7 @@ use OCP\Security\ISecureRandom; use OCP\Server; use OCP\UserStatus\IManager as IUserStatusManager; use OCP\UserStatus\IUserStatus; +use Psr\Log\LoggerInterface; class ParticipantService { @@ -98,6 +100,7 @@ class ParticipantService { private ITimeFactory $timeFactory, private ICacheFactory $cacheFactory, private IUserStatusManager $userStatusManager, + private LoggerInterface $logger, ) { } @@ -456,7 +459,7 @@ class ParticipantService { * @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 +469,20 @@ class ParticipantService { $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 +616,8 @@ class ParticipantService { $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 +637,11 @@ class ParticipantService { 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 +666,7 @@ class ParticipantService { $this->dispatcher->dispatchTyped($attendeeEvent); } - $this->addUsers($room, $newParticipants); + $this->addUsers($room, $newParticipants, bansAlreadyChecked: true); } /** @@ -688,6 +708,8 @@ class ParticipantService { $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 +745,11 @@ class ParticipantService { 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 +774,7 @@ class ParticipantService { $this->dispatcher->dispatchTyped($attendeeEvent); } - $this->addUsers($room, $newParticipants); + $this->addUsers($room, $newParticipants, bansAlreadyChecked: true); } /** diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index f4ccf67dab..85e512e66d 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -900,6 +900,7 @@ class FeatureContext implements Context, SnippetAcceptingContext { } return $attendee; }, $formData->getHash(), $result); + $expected = array_filter($expected); $result = array_map(function ($attendee) { if (isset($attendee['permissions'])) { @@ -918,7 +919,7 @@ class FeatureContext implements Context, SnippetAcceptingContext { '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 1f96b68e19..3746516f2b 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) @@ -129,14 +128,54 @@ Feature: conversation/ban Given user "participant1" creates room "room" (v4) | roomType | 3 | | roomName | room | - And user "participant2" joins room "room" with 200 (v4) 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) - And user "participant1" sends message "Message 1" to room "room" with 201 - And user "participant2" sees the following messages in room "room" with 403 + 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 | From 7ff8b5865cc4aec7001796d3c83dfa3a81a7b711 Mon Sep 17 00:00:00 2001 From: skalidindi53 Date: Fri, 2 Aug 2024 15:50:00 -0500 Subject: [PATCH 3/3] fix: Resolved failing checks Signed-off-by: skalidindi53 --- lib/Service/ParticipantService.php | 1 - tests/php/Service/ParticipantServiceTest.php | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index c9f0515e9a..3e368b8fd1 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -47,7 +47,6 @@ use OCA\Talk\Federation\FederationManager; use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; use OCA\Talk\Model\AttendeeMapper; -use OCA\Talk\Model\Ban; use OCA\Talk\Model\BreakoutRoom; use OCA\Talk\Model\SelectHelper; use OCA\Talk\Model\Session; diff --git a/tests/php/Service/ParticipantServiceTest.php b/tests/php/Service/ParticipantServiceTest.php index 9a68aae11f..a024502ed0 100644 --- a/tests/php/Service/ParticipantServiceTest.php +++ b/tests/php/Service/ParticipantServiceTest.php @@ -31,6 +31,7 @@ use OCP\IUserManager; 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 @@ class ParticipantServiceTest extends TestCase { $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 @@ class ParticipantServiceTest extends TestCase { $this->time, $this->cacheFactory, $this->userStatusManager, + $this->logger ); }