Fix removing users when an old session id expires in the signaling server.

It could happen that an old session id expired while a user already re-joined
a room with a new session id. Once the old session id expired, the user got
removed from the room.

Signed-off-by: Joachim Bauch <bauch@struktur.de>

lib/Controller/SignalingController.php

@@ -422,7 +422,7 @@ class SignalingController extends OCSController {
 			$room->ping($userId, $sessionId, $this->timeFactory->getTime());
 		} else if ($action === 'leave') {
 			if (!empty($userId)) {
-				$room->leaveRoom($userId);
+				$room->leaveRoom($userId, $sessionId);
 			} else if ($participant instanceof Participant) {
 				$room->removeParticipantBySession($participant, Room::PARTICIPANT_LEFT);
 			}

lib/Room.php

@@ -632,8 +632,9 @@ class Room {
 
 	/**
 	 * @param string $userId
+	 * @param string|null $sessionId
 	 */
-	public function leaveRoom(string $userId): void {
+	public function leaveRoom(string $userId, ?string $sessionId = null): void {
 		try {
 			$participant = $this->getParticipant($userId);
 		} catch (ParticipantNotFoundException $e) {
@@ -642,6 +643,7 @@ class Room {
 
 		$this->dispatcher->dispatch(self::class . '::preUserDisconnectRoom', new GenericEvent($this, [
 			'userId' => $userId,
+			'sessionId' => $sessionId,
 			'participant' => $participant,
 		]));
 
@@ -653,6 +655,9 @@ class Room {
 			->where($query->expr()->eq('user_id', $query->createNamedParameter($userId)))
 			->andWhere($query->expr()->eq('room_id', $query->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->neq('participant_type', $query->createNamedParameter(Participant::USER_SELF_JOINED, IQueryBuilder::PARAM_INT)));
+		if (!empty($sessionId)) {
+			$query->andWhere($query->expr()->eq('session_id', $query->createNamedParameter($sessionId)));
+		}
 		$query->execute();
 
 		// And kill session when leaving a self joined room
@@ -661,10 +666,14 @@ class Room {
 			->where($query->expr()->eq('user_id', $query->createNamedParameter($userId)))
 			->andWhere($query->expr()->eq('room_id', $query->createNamedParameter($this->getId(), IQueryBuilder::PARAM_INT)))
 			->andWhere($query->expr()->eq('participant_type', $query->createNamedParameter(Participant::USER_SELF_JOINED, IQueryBuilder::PARAM_INT)));
+		if (!empty($sessionId)) {
+			$query->andWhere($query->expr()->eq('session_id', $query->createNamedParameter($sessionId)));
+		}
 		$selfJoined = (bool) $query->execute();
 
 		$this->dispatcher->dispatch(self::class . '::postUserDisconnectRoom', new GenericEvent($this, [
 			'userId' => $userId,
+			'sessionId' => $sessionId,
 			'participant' => $participant,
 			'selfJoin' => $selfJoined,
 		]));

tests/php/Controller/SignalingControllerTest.php

@@ -22,6 +22,7 @@
 
 namespace OCA\Spreed\Tests\php\Controller;
 
+use OCA\Spreed\Chat\CommentsManager;
 use OCA\Spreed\Config;
 use OCA\Spreed\Controller\SignalingController;
 use OCA\Spreed\Exceptions\ParticipantNotFoundException;
@@ -35,6 +36,8 @@ use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IDBConnection;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Security\IHasher;
+use OCP\Security\ISecureRandom;
 use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\EventDispatcher\GenericEvent;
@@ -74,6 +77,8 @@ class SignalingControllerTest extends \Test\TestCase {
 	protected $timeFactory;
 	/** @var string */
 	private $userId;
+	/** @var ISecureRandom */
+	private $secureRandom;
 	/** @var EventDispatcherInterface */
 	private $dispatcher;
 
@@ -84,7 +89,7 @@ class SignalingControllerTest extends \Test\TestCase {
 		parent::setUp();
 
 		$this->userId = 'testUser';
-		$secureRandom = \OC::$server->getSecureRandom();
+		$this->secureRandom = \OC::$server->getSecureRandom();
 		$timeFactory = $this->createMock(ITimeFactory::class);
 		$config = \OC::$server->getConfig();
 		$config->setAppValue('spreed', 'signaling_servers', json_encode([
@@ -92,7 +97,7 @@ class SignalingControllerTest extends \Test\TestCase {
 		]));
 		$config->setAppValue('spreed', 'signaling_ticket_secret', 'the-app-ticket-secret');
 		$config->setUserValue($this->userId, 'spreed', 'signaling_ticket_secret', 'the-user-ticket-secret');
-		$this->config = new Config($config, $secureRandom, $timeFactory);
+		$this->config = new Config($config, $this->secureRandom, $timeFactory);
 		$this->session = $this->createMock(TalkSession::class);
 		$this->dbConnection = \OC::$server->getDatabaseConnection();
 		$this->manager = $this->createMock(Manager::class);
@@ -680,4 +685,82 @@ class SignalingControllerTest extends \Test\TestCase {
 		], $result->getData());
 	}
 
+
+	public function testLeaveRoomWithOldSession() {
+		// Make sure that leaving a user with an old session id doesn't remove
+		// the current user from the room if he re-joined in the meantime.
+		$dbConnection = \OC::$server->getDatabaseConnection();
+		$dispatcher = \OC::$server->getEventDispatcher();
+		$this->manager = new Manager(
+			$dbConnection,
+			\OC::$server->getConfig(),
+			$this->secureRandom,
+			$this->createMock(CommentsManager::class),
+			$dispatcher,
+			$this->timeFactory,
+			$this->createMock(IHasher::class)
+		);
+		$this->recreateSignalingController();
+
+		$testUser = $this->createMock(IUser::class);
+		$testUser->expects($this->any())
+			->method('getDisplayName')
+			->willReturn('Test User');
+		$testUser->expects($this->any())
+			->method('getUID')
+			->willReturn($this->userId);
+
+		$room = $this->manager->createPublicRoom();
+
+		// The user joined the room.
+		$oldSessionId = $room->joinRoom($testUser, '');
+		$result = $this->performBackendRequest([
+			'type' => 'room',
+			'room' => [
+				'roomid' => $room->getToken(),
+				'userid' => $this->userId,
+				'sessionid' => $oldSessionId,
+				'action' => 'join',
+			],
+		]);
+		$participant = $room->getParticipant($this->userId);
+		$this->assertEquals($oldSessionId, $participant->getSessionId());
+
+		// The user is reloading the browser which will join him with another
+		// session id.
+		$newSessionId = $room->joinRoom($testUser, '');
+		$room->addUsers([
+			'userId' => $this->userId,
+			'sessionId' => $this->userId,
+		]);
+		$result = $this->performBackendRequest([
+			'type' => 'room',
+			'room' => [
+				'roomid' => $room->getToken(),
+				'userid' => $this->userId,
+				'sessionid' => $newSessionId,
+				'action' => 'join',
+			],
+		]);
+
+		// Now the new session id is stored in the database.
+		$participant = $room->getParticipant($this->userId);
+		$this->assertEquals($newSessionId, $participant->getSessionId());
+
+		// Leaving the old session id...
+		$result = $this->performBackendRequest([
+			'type' => 'room',
+			'room' => [
+				'roomid' => $room->getToken(),
+				'userid' => $this->userId,
+				'sessionid' => $oldSessionId,
+				'action' => 'leave',
+			],
+		]);
+
+		// ...will keep the new session id in the database.
+		$participant = $room->getParticipant($this->userId);
+		$this->assertEquals($newSessionId, $participant->getSessionId());
+	}
+
 }