Mirrors
/
nextcloud-spreed
mirror of https://github.com/nextcloud/spreed
3
0
Fork 0
Code Releases Activity
Browse Source

fix(ban): Check real bans on joining a conversation and ban guests by IP 

Signed-off-by: Joas Schilling <coding@schilljs.com>
pull/12691/head
Joas Schilling 1 year ago
parent
090ff63aa6
commit
ba276d686a
No known key found for this signature in database GPG Key ID: 74434EFE0D2E2205
9 changed files with 193 additions and 30 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 14
      lib/Controller/RoomController.php
  2. 26
      lib/Controller/SignalingController.php
  3. 12
      lib/Middleware/InjectionMiddleware.php
  4. 2
      lib/Model/BanMapper.php
  5. 67
      lib/Service/BanService.php
  6. 12
      lib/TalkSession.php
  7. 50
      tests/integration/features/bootstrap/FeatureContext.php
  8. 34
      tests/integration/features/conversation-1/ban.feature
  9. 6
      tests/php/Controller/SignalingControllerTest.php

14
lib/Controller/RoomController.php
View File

@ -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']);

26
lib/Controller/SignalingController.php
View File

@ -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.
*

12
lib/Middleware/InjectionMiddleware.php
View File

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

2
lib/Model/BanMapper.php
View File

@ -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())

67
lib/Service/BanService.php
View File

@ -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,
) {
}
@ -87,13 +95,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) {
}
}
/**

12
lib/TalkSession.php
View File

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

50
tests/integration/features/bootstrap/FeatureContext.php
View File

@ -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);
}
/**

34
tests/integration/features/conversation-1/ban.feature
View File

@ -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 |

6
tests/php/Controller/SignalingControllerTest.php
View File

@ -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&MockObject $timeFactory;
protected IClientService&MockObject $clientService;
protected IThrottler&MockObject $throttler;
protected BanService&MockObject $banService;
protected LoggerInterface&MockObject $logger;
protected IDBConnection $dbConnection;
protected IConfig $serverConfig;
@ -111,6 +113,7 @@ class SignalingControllerTest extends TestCase {
$this->messages = $this->createMock(Messages::class);
$this->timeFactory = $this->createMock(ITimeFactory::class);
$this->clientService = $this->createMock(IClientService::class);
$this->banService = $this->createMock(BanService::class);
$this->logger = $this->createMock(LoggerInterface::class);
$this->recreateSignalingController();
}
@ -132,8 +135,9 @@ class SignalingControllerTest extends TestCase {
$this->dispatcher,
$this->timeFactory,
$this->clientService,
$this->banService,
$this->logger,
$this->userId
$this->userId,
);
}

Write Preview
Loading…
Cancel
Save