Merge pull request #12801 from nextcloud/skalidindi53/12793/Cannot-add-banned-user

fix: Banned user cannot be added to the room

lib/Controller/RoomController.php

@@ -1070,7 +1070,7 @@ class RoomController extends AEnvironmentAwareController {
 	 * @return DataResponse<Http::STATUS_OK, array{type: int}|array<empty>, array{}>|DataResponse<Http::STATUS_NOT_FOUND|Http::STATUS_NOT_IMPLEMENTED, array<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error?: string}, array{}>
 	 *
 	 * 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(),

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,
 	) {
 	}
 

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) {

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) {

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
 	 */

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.
 	 *
@@ -197,6 +211,26 @@ class BanService {
 		return $this->banMapper->findByRoomId($roomId);
 	}
 
+	/**
+	 * Retrieve all banned userIDs for a specific room.
+	 *
+	 * @return array<string, mixed> 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<int, mixed> 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.
 	 */

lib/Service/ParticipantService.php

@@ -73,6 +73,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 +99,7 @@ class ParticipantService {
 		private ITimeFactory $timeFactory,
 		private ICacheFactory $cacheFactory,
 		private IUserStatusManager $userStatusManager,
+		private LoggerInterface $logger,
 	) {
 	}
 
@@ -456,7 +458,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 +468,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 +615,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 +636,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 +665,7 @@ class ParticipantService {
 			$this->dispatcher->dispatchTyped($attendeeEvent);
 		}
 
-		$this->addUsers($room, $newParticipants);
+		$this->addUsers($room, $newParticipants, bansAlreadyChecked: true);
 	}
 
 	/**
@@ -688,6 +707,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 +744,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 +773,7 @@ class ParticipantService {
 			$this->dispatcher->dispatchTyped($attendeeEvent);
 		}
 
-		$this->addUsers($room, $newParticipants);
+		$this->addUsers($room, $newParticipants, bansAlreadyChecked: true);
 	}
 
 	/**

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": {

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": {

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;

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;

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

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                      |

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