fix(ban): Check real bans on joining a conversation and ban guests by IPbugfix/noid/ban-guests-by-ip

Signed-off-by: Joas Schilling <coding@schilljs.com>

9 files changed, 193 insertions, 30 deletions

diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php
index bb184cba4..fe6316abb 100644
--- a/lib/Controller/RoomController.php
+++ b/lib/Controller/RoomController.php
@@ -37,6 +37,7 @@ use OCA\Talk\Model\Session;
 use OCA\Talk\Participant;
 use OCA\Talk\ResponseDefinitions;
 use OCA\Talk\Room;
+use OCA\Talk\Service\BanService;
 use OCA\Talk\Service\BreakoutRoomService;
 use OCA\Talk\Service\ChecksumVerificationService;
 use OCA\Talk\Service\NoteToSelfService;
@@ -108,6 +109,7 @@ class RoomController extends AEnvironmentAwareController {
 		protected Authenticator $federationAuthenticator,
 		protected Capabilities $capabilities,
 		protected FederationManager $federationManager,
+		protected BanService $banService,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -1512,13 +1514,10 @@ class RoomController extends AEnvironmentAwareController {
 			return $response;
 		}
 
-		if (strtolower($room->getName()) === 'ban user' && $this->userId === 'banned') {
-			return new DataResponse([
-				'error' => 'ban',
-			], Http::STATUS_FORBIDDEN);
-		}
-
-		if (strtolower($room->getName()) === 'ban guest' && !$this->userId) {
+		try {
+			$this->banService->throwIfActorIsBanned($room, $this->userId);
+		} catch (ForbiddenException $e) {
+			$this->logger->info('Participant ' . ($this->userId ?? 'guest') . ' is banned from room ' . $token . ' by ' . $e->getMessage());
 			return new DataResponse([
 				'error' => 'ban',
 			], Http::STATUS_FORBIDDEN);
@@ -1593,6 +1592,7 @@ class RoomController extends AEnvironmentAwareController {
 				$participant = $this->participantService->joinRoomAsFederatedUser($room, Attendee::ACTOR_FEDERATED_USERS, $this->federationAuthenticator->getCloudId());
 			} else {
 				$participant = $this->participantService->joinRoomAsNewGuest($this->roomService, $room, $password, $result['result'], $previousParticipant);
+				$this->session->setGuestActorIdForRoom($room->getToken(), $participant->getAttendee()->getActorId());
 			}
 			$this->throttler->resetDelay($this->request->getRemoteAddress(), 'talkRoomPassword', ['token' => $token, 'action' => 'talkRoomPassword']);
 			$this->throttler->resetDelay($this->request->getRemoteAddress(), 'talkRoomToken', ['token' => $token, 'action' => 'talkRoomToken']);
diff --git a/lib/Controller/SignalingController.php b/lib/Controller/SignalingController.php
index 3c8636489..8609847e6 100644
--- a/lib/Controller/SignalingController.php
+++ b/lib/Controller/SignalingController.php
@@ -12,6 +12,7 @@ use GuzzleHttp\Exception\ConnectException;
 use OCA\Talk\Config;
 use OCA\Talk\Events\AAttendeeRemovedEvent;
 use OCA\Talk\Events\BeforeSignalingResponseSentEvent;
+use OCA\Talk\Exceptions\ForbiddenException;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Exceptions\RoomNotFoundException;
 use OCA\Talk\Manager;
@@ -20,6 +21,7 @@ use OCA\Talk\Model\Session;
 use OCA\Talk\Participant;
 use OCA\Talk\ResponseDefinitions;
 use OCA\Talk\Room;
+use OCA\Talk\Service\BanService;
 use OCA\Talk\Service\CertificateService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\SessionService;
@@ -65,6 +67,7 @@ class SignalingController extends OCSController {
 		private IEventDispatcher $dispatcher,
 		private ITimeFactory $timeFactory,
 		private IClientService $clientService,
+		private BanService $banService,
 		private LoggerInterface $logger,
 		private ?string $userId,
 	) {
@@ -372,7 +375,8 @@ class SignalingController extends OCSController {
 			if ($participant->getSession() instanceof Session) {
 				$this->sessionService->updateLastPing($participant->getSession(), $pingTimestamp);
 			}
-		} catch (RoomNotFoundException $e) {
+		} catch (RoomNotFoundException) {
+			$this->banIpIfGuestGotBanned($token);
 			return new DataResponse([['type' => 'usersInRoom', 'data' => []]], Http::STATUS_NOT_FOUND);
 		}
 
@@ -413,7 +417,8 @@ class SignalingController extends OCSController {
 			// Add an update of the room participants at the end of the waiting
 			$room = $this->manager->getRoomForSession($this->userId, $sessionId);
 			$data[] = ['type' => 'usersInRoom', 'data' => $this->getUsersInRoom($room, $pingTimestamp)];
-		} catch (RoomNotFoundException $e) {
+		} catch (RoomNotFoundException) {
+			$this->banIpIfGuestGotBanned($token);
 			$data[] = ['type' => 'usersInRoom', 'data' => []];
 
 			// Was the session killed or the complete conversation?
@@ -479,6 +484,23 @@ class SignalingController extends OCSController {
 		return $usersInRoom;
 	}
 
+	protected function banIpIfGuestGotBanned(string $token): void {
+		if ($this->userId !== null) {
+			return;
+		}
+
+		try {
+			$room = $this->manager->getRoomByToken($token);
+		} catch (RoomNotFoundException) {
+			return;
+		}
+
+		try {
+			$this->banService->throwIfActorIsBanned($room, null);
+		} catch (ForbiddenException) {
+		}
+	}
+
 	/**
 	 * Check if the current request is coming from an allowed backend.
 	 *
diff --git a/lib/Middleware/InjectionMiddleware.php b/lib/Middleware/InjectionMiddleware.php
index 8e42788a0..0f4b29642 100644
--- a/lib/Middleware/InjectionMiddleware.php
+++ b/lib/Middleware/InjectionMiddleware.php
@@ -10,6 +10,7 @@ namespace OCA\Talk\Middleware;
 
 use OCA\Talk\Controller\AEnvironmentAwareController;
 use OCA\Talk\Exceptions\CannotReachRemoteException;
+use OCA\Talk\Exceptions\ForbiddenException;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Exceptions\PermissionsException;
 use OCA\Talk\Exceptions\RoomNotFoundException;
@@ -35,6 +36,7 @@ use OCA\Talk\Model\Attendee;
 use OCA\Talk\Model\InvitationMapper;
 use OCA\Talk\Participant;
 use OCA\Talk\Room;
+use OCA\Talk\Service\BanService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\TalkSession;
 use OCA\Talk\Webinary;
@@ -65,6 +67,7 @@ class InjectionMiddleware extends Middleware {
 		protected IURLGenerator $url,
 		protected InvitationMapper $invitationMapper,
 		protected Authenticator $federationAuthenticator,
+		protected BanService $banService,
 		protected LoggerInterface $logger,
 		protected ?string $userId,
 	) {
@@ -77,6 +80,7 @@ class InjectionMiddleware extends Middleware {
 	 * @throws ParticipantNotFoundException
 	 * @throws PermissionsException
 	 * @throws ReadOnlyException
+	 * @throws ForbiddenException
 	 * @throws RoomNotFoundException
 	 */
 	public function beforeController(Controller $controller, string $methodName): void {
@@ -151,11 +155,14 @@ class InjectionMiddleware extends Middleware {
 
 	/**
 	 * @param AEnvironmentAwareController $controller
+	 * @throws ForbiddenException
 	 */
 	protected function getRoom(AEnvironmentAwareController $controller): void {
 		$token = $this->request->getParam('token');
 		$room = $this->manager->getRoomByToken($token);
 		$controller->setRoom($room);
+
+		$this->banService->throwIfActorIsBanned($room, $this->userId);
 	}
 
 	/**
@@ -172,6 +179,8 @@ class InjectionMiddleware extends Middleware {
 		$participant = $this->participantService->getParticipant($room, $this->userId, $sessionId);
 		$controller->setParticipant($participant);
 
+		$this->banService->throwIfActorIsBanned($room, $this->userId);
+
 		if ($moderatorRequired && !$participant->hasModeratorPermissions(false)) {
 			throw new NotAModeratorException();
 		}
@@ -227,6 +236,8 @@ class InjectionMiddleware extends Middleware {
 			}
 		}
 
+		$this->banService->throwIfActorIsBanned($room, $this->userId);
+
 		if ($moderatorRequired && !$participant->hasModeratorPermissions()) {
 			throw new NotAModeratorException();
 		}
@@ -386,6 +397,7 @@ class InjectionMiddleware extends Middleware {
 
 		if ($exception instanceof NotAModeratorException ||
 			$exception instanceof ReadOnlyException ||
+			$exception instanceof ForbiddenException ||
 			$exception instanceof PermissionsException) {
 			if ($controller instanceof OCSController) {
 				throw new OCSException('', Http::STATUS_FORBIDDEN);
diff --git a/lib/Model/BanMapper.php b/lib/Model/BanMapper.php
index f17fb72a1..c6f053cff 100644
--- a/lib/Model/BanMapper.php
+++ b/lib/Model/BanMapper.php
@@ -28,7 +28,7 @@ class BanMapper extends QBMapper {
 	/**
 	 * @throws DoesNotExistException
 	 */
-	public function findForActorAndRoom(string $bannedActorType, string $bannedActorId, int $roomId): Ban {
+	public function findForBannedActorAndRoom(string $bannedActorType, string $bannedActorId, int $roomId): Ban {
 		$query = $this->db->getQueryBuilder();
 		$query->select('*')
 			->from($this->getTableName())
diff --git a/lib/Service/BanService.php b/lib/Service/BanService.php
index b2e8805f2..c9e4653d5 100644
--- a/lib/Service/BanService.php
+++ b/lib/Service/BanService.php
@@ -9,14 +9,19 @@ declare(strict_types=1);
 namespace OCA\Talk\Service;
 
 use DateTime;
+use OCA\Talk\Exceptions\ForbiddenException;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Manager;
 use OCA\Talk\Model\Attendee;
 use OCA\Talk\Model\Ban;
 use OCA\Talk\Model\BanMapper;
 use OCA\Talk\Room;
+use OCA\Talk\TalkSession;
 use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\DB\Exception;
+use OCP\IRequest;
 use OCP\IUserManager;
+use Psr\Log\LoggerInterface;
 
 class BanService {
 
@@ -25,6 +30,9 @@ class BanService {
 		protected Manager $manager,
 		protected ParticipantService $participantService,
 		protected IUserManager $userManager,
+		protected TalkSession $talkSession,
+		protected IRequest $request,
+		protected LoggerInterface $logger,
 	) {
 	}
 
@@ -81,13 +89,62 @@ class BanService {
 		return $this->banMapper->insert($ban);
 	}
 
+	public function copyBanForRemoteAddress(Ban $ban, string $remoteAddress): void {
+		$this->logger->info('Banned guest detected, banning IP address: ' . $remoteAddress . ' to prevent rejoining.');
+
+		$newBan = new Ban();
+		$newBan->setModeratorActorType($ban->getModeratorActorType());
+		$newBan->setModeratorActorId($ban->getModeratorActorId());
+		$newBan->setModeratorDisplayname($ban->getModeratorDisplayname());
+		$newBan->setRoomId($ban->getRoomId());
+		$newBan->setBannedTime($ban->getBannedTime());
+		$newBan->setInternalNote($ban->getInternalNote());
+
+		$newBan->setBannedActorType('ip');
+		$newBan->setBannedActorId($remoteAddress);
+
+		try {
+			$this->banMapper->insert($newBan);
+		} catch (Exception $e) {
+			if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+				return;
+			}
+			throw $e;
+		}
+	}
+
 	/**
-	 * Retrieve a ban for a specific actor and room.
-	 *
-	 * @throws DoesNotExistException
+	 * @throws ForbiddenException
 	 */
-	public function getBanForActorAndRoom(string $bannedActorType, string $bannedActorId, int $roomId): Ban {
-		return $this->banMapper->findForActorAndRoom($bannedActorType, $bannedActorId, $roomId);
+	public function throwIfActorIsBanned(Room $room, ?string $userId): void {
+		if ($userId !== null) {
+			$actorType = Attendee::ACTOR_USERS;
+			$actorId = $userId;
+		} else {
+			$actorType = Attendee::ACTOR_GUESTS;
+			$actorId = $this->talkSession->getGuestActorIdForRoom($room->getToken());
+		}
+
+		if ($actorId !== null) {
+			try {
+				$ban = $this->banMapper->findForBannedActorAndRoom($actorType, $actorId, $room->getId());
+				if ($actorType === Attendee::ACTOR_GUESTS) {
+					$this->copyBanForRemoteAddress($ban, $this->request->getRemoteAddress());
+				}
+				throw new ForbiddenException('actor');
+			} catch (DoesNotExistException) {
+			}
+		}
+
+		if ($actorType !== Attendee::ACTOR_GUESTS) {
+			return;
+		}
+
+		try {
+			$this->banMapper->findForBannedActorAndRoom($this->request->getRemoteAddress(), 'ip', $room->getId());
+			throw new ForbiddenException('ip');
+		} catch (DoesNotExistException) {
+		}
 	}
 
 	/**
diff --git a/lib/TalkSession.php b/lib/TalkSession.php
index 3e5516280..e85430d2c 100644
--- a/lib/TalkSession.php
+++ b/lib/TalkSession.php
@@ -37,6 +37,18 @@ class TalkSession {
 		$this->removeValue('spreed-session', $token);
 	}
 
+	public function getGuestActorIdForRoom(string $token): ?string {
+		return $this->getValue('spreed-guest-id', $token);
+	}
+
+	public function setGuestActorIdForRoom(string $token, string $actorId): void {
+		$this->setValue('spreed-guest-id', $token, $actorId);
+	}
+
+	public function removeGuestActorIdForRoom(string $token): void {
+		$this->removeValue('spreed-guest-id', $token);
+	}
+
 	public function getFileShareTokenForRoom(string $roomToken): ?string {
 		return $this->getValue('spreed-file-share-token', $roomToken);
 	}
diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php
index 99e65e13c..367714121 100644
--- a/tests/integration/features/bootstrap/FeatureContext.php
+++ b/tests/integration/features/bootstrap/FeatureContext.php
@@ -30,6 +30,8 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 	/** @var array<string, string> */
 	protected static array $sessionIdToUser;
 	/** @var array<string, string> */
+	protected static array $sessionNameToActorId;
+	/** @var array<string, string> */
 	protected static array $userToSessionId;
 	/** @var array<string, int> */
 	protected static array $userToAttendeeId;
@@ -167,6 +169,7 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 		self::$identifierToToken = [];
 		self::$identifierToId = [];
 		self::$tokenToIdentifier = [];
+		self::$sessionNameToActorId = [];
 		self::$sessionIdToUser = [
 			'cli' => 'cli',
 			'system' => 'system',
@@ -1218,14 +1221,6 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 	public function userJoinsRoomWithNamedSession(string $user, string $identifier, int $statusCode, string $apiVersion, string $sessionName, ?TableNode $formData = null): void {
 		$this->setCurrentUser($user, $identifier);
 
-		$userBanId = self::$userToBanId[self::$identifierToToken[$identifier]]['users'][$user] ?? null;
-		$guestBanId = self::$userToBanId[self::$identifierToToken[$identifier]]['guests'][$user] ?? null;
-
-		if ($userBanId !== null || $guestBanId !== null) {
-			//User is banned
-			return;
-		}
-
 		$this->sendRequest(
 			'POST', '/apps/spreed/api/' . $apiVersion . '/room/' . self::$identifierToToken[$identifier] . '/participants/active',
 			$formData
@@ -1246,6 +1241,7 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 			self::$userToSessionId[$user] = $response['sessionId'];
 			if ($sessionName) {
 				self::$userToSessionId[$user . '#' . $sessionName] = $response['sessionId'];
+				self::$sessionNameToActorId[$sessionName] = $response['actorId'];
 			}
 			if (!isset(self::$userToAttendeeId[$identifier][$response['actorType']])) {
 				self::$userToAttendeeId[$identifier][$response['actorType']] = [];
@@ -1468,7 +1464,7 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 	}
 
 	/**
-	 * @When /^user "([^"]*)" bans (user|group|email|remote) "([^"]*)" from room "([^"]*)" with (\d+) \((v1)\)$/
+	 * @When /^user "([^"]*)" bans (user|group|email|remote|guest) "([^"]*)" from room "([^"]*)" with (\d+) \((v1)\)$/
 	 *
 	 * @param string $user
 	 * @param string $actorType
@@ -1478,7 +1474,9 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 	 * @param string $apiVersion
 	 */
 	public function userBansUserFromRoom(string $user, string $actorType, string $actorId, string $identifier, int $statusCode, string $apiVersion = 'v1', TableNode $internalNote): void {
-		if ($actorId === 'stranger') {
+		if ($actorType === 'guest') {
+			$actorId = self::$sessionNameToActorId[$actorId];
+		} elseif ($actorId === 'stranger') {
 			$actorId = '123456789';
 		} else {
 			if ($actorType === 'remote') {
@@ -1529,12 +1527,36 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 			return;
 		}
 
-		$bans = $this->getDataFromResponse($this->response);
-		foreach ($bans as &$key) {
-			unset($key['id'], $key['roomId'], $key['bannedTime']);
+		if ($tableNode instanceof TableNode) {
+			$expected = array_map(static function (array $ban): array {
+				if (preg_match('/^SESSION\(([a-z0-9]+)\)$/', $ban['bannedActorId'], $match)) {
+					$ban['bannedActorId'] = self::$sessionNameToActorId[$match[1]];
+				}
+				if (preg_match('/^SESSION\(([a-z0-9]+)\)$/', $ban['bannedDisplayName'], $match)) {
+					$ban['bannedDisplayName'] = self::$sessionNameToActorId[$match[1]];
+				}
+				return $ban;
+			}, $tableNode->getColumnsHash());
+		} else {
+			$expected = [];
 		}
 
-		Assert::assertEquals($tableNode->getColumnsHash(), $bans);
+		$bans = array_map(static function (array $ban, array $expectedBan): array {
+			if ($expectedBan['bannedActorId'] === 'LOCAL_IP') {
+				if ($ban['bannedActorId'] === '127.0.0.1' || $ban['bannedActorId'] === '::1') {
+					$ban['bannedActorId'] = 'LOCAL_IP';
+				}
+			}
+			if ($expectedBan['bannedDisplayName'] === 'LOCAL_IP') {
+				if ($ban['bannedDisplayName'] === '127.0.0.1' || $ban['bannedDisplayName'] === '::1') {
+					$ban['bannedDisplayName'] = 'LOCAL_IP';
+				}
+			}
+			unset($ban['id'], $ban['roomId'], $ban['bannedTime']);
+			return $ban;
+		}, $this->getDataFromResponse($this->response), $expected);
+
+		Assert::assertEquals($expected, $bans);
 	}
 
 	/**
diff --git a/tests/integration/features/conversation-1/ban.feature b/tests/integration/features/conversation-1/ban.feature
index 33595a221..8e9a24b26 100644
--- a/tests/integration/features/conversation-1/ban.feature
+++ b/tests/integration/features/conversation-1/ban.feature
@@ -90,3 +90,37 @@ Feature: conversation/ban
         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     |
+
+  Scenario: Banned user can not join conversation
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 3 |
+      | roomName | room |
+    When user "participant2" joins room "room" with 200 (v4)
+    Then user "participant2" leaves room "room" with 200 (v4)
+    When user "participant1" bans user "participant2" from room "room" with 200 (v1)
+      | internalNote | BannedP2 |
+    Then user "participant2" joins room "room" with 403 (v4)
+
+  Scenario: Banned user can not send reactions or messages
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 3 |
+      | roomName | room |
+    And user "participant2" joins room "room" with 200 (v4)
+    And user "participant2" sends message "Message 1" to room "room" with 201
+    When user "participant1" bans user "participant2" from room "room" with 200 (v1)
+      | internalNote | BannedP2 |
+    And user "participant2" sends message "Message 2" to room "room" with 403
+    And user "participant2" react with "👍" on message "Message 1" to room "room" with 403
+
+  Scenario: Banning a guest bans their IP as well
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 3 |
+      | roomName | room |
+    And user "guest" joins room "room" with 200 (v4) session name "guest1"
+    And user "participant1" bans guest "guest1" from room "room" with 200 (v1)
+      | internalNote | Banned guest |
+    And user "guest" joins room "room" with 403 (v4) session name "guest2"
+    And user "participant1" sees the following bans in room "room" with 200 (v1)
+      | 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 |
diff --git a/tests/php/Controller/SignalingControllerTest.php b/tests/php/Controller/SignalingControllerTest.php
index 22f12046a..058b3e579 100644
--- a/tests/php/Controller/SignalingControllerTest.php
+++ b/tests/php/Controller/SignalingControllerTest.php
@@ -19,6 +19,7 @@ use OCA\Talk\Model\AttendeeMapper;
 use OCA\Talk\Model\SessionMapper;
 use OCA\Talk\Participant;
 use OCA\Talk\Room;
+use OCA\Talk\Service\BanService;
 use OCA\Talk\Service\CertificateService;
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\RoomService;
@@ -72,6 +73,7 @@ class SignalingControllerTest extends TestCase {
 	protected ITimeFactory&MockObje