diff options
author | Joas Schilling <coding@schilljs.com> | 2024-07-12 15:30:54 +0200 |
---|---|---|
committer | Joas Schilling <coding@schilljs.com> | 2024-07-17 14:45:31 +0200 |
commit | 3dd33ba74378c4e2e4ef6e363d5547ad0808f165 (patch) | |
tree | 8a230b405b8154d99132e507df31602fb292e7ae | |
parent | 12cf1bb53c706b1acac629e55309880a7237ac8e (diff) |
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>
-rw-r--r-- | lib/Controller/RoomController.php | 14 | ||||
-rw-r--r-- | lib/Controller/SignalingController.php | 26 | ||||
-rw-r--r-- | lib/Middleware/InjectionMiddleware.php | 12 | ||||
-rw-r--r-- | lib/Model/BanMapper.php | 2 | ||||
-rw-r--r-- | lib/Service/BanService.php | 67 | ||||
-rw-r--r-- | lib/TalkSession.php | 12 | ||||
-rw-r--r-- | tests/integration/features/bootstrap/FeatureContext.php | 50 | ||||
-rw-r--r-- | tests/integration/features/conversation-1/ban.feature | 34 | ||||
-rw-r--r-- | tests/php/Controller/SignalingControllerTest.php | 6 |
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 |