Merge pull request #10678 from nextcloud/bugfix/5723/cron-job-removes…

…-invited-rooms

fix(federation): Don't remove federation rooms while waiting for accept/reject

lib/BackgroundJob/RemoveEmptyRooms.php

@@ -23,6 +23,7 @@
 
 namespace OCA\Talk\BackgroundJob;
 
+use OCA\Talk\Federation\FederationManager;
 use OCA\Talk\Manager;
 use OCA\Talk\Room;
 use OCA\Talk\Service\ParticipantService;
@@ -47,6 +48,7 @@ public function __construct(
 		protected Manager $manager,
 		protected RoomService $roomService,
 		protected ParticipantService $participantService,
+		protected FederationManager $federationManager,
 		protected LoggerInterface $logger,
 		protected IUserMountCache $userMountCache,
 	) {
@@ -89,6 +91,11 @@ private function deleteIfIsEmpty(Room $room): bool {
 			return false;
 		}
 
+		if ($room->getRemoteServer() && $room->getRemoteToken()
+			&& $this->federationManager->getNumberOfInvitations($room) !== 0) {
+			return false;
+		}
+
 		$this->doDeleteRoom($room);
 		return true;
 	}

lib/Controller/FederationController.php

@@ -113,17 +113,22 @@ public function getShares(): DataResponse {
 			throw new UnauthorizedException();
 		}
 		$invitations = $this->federationManager->getRemoteRoomShares($user);
-		return new DataResponse(array_map(function (Invitation $invitation) {
+
+		/** @var TalkFederationInvite[] $data */
+		$data = array_filter(array_map(function (Invitation $invitation) {
 			$data = $invitation->asArray();
 
 			try {
 				$room = $this->talkManager->getRoomById($invitation->getRoomId());
 				$data['remote_token'] = $room->getRemoteToken();
 				$data['remote_server'] = $room->getRemoteServer();
-			} catch (RoomNotFoundException $exception) {
+			} catch (RoomNotFoundException) {
+				return null;
 			}
 
 			return $data;
 		}, $invitations));
+
+		return new DataResponse($data);
 	}
 }

lib/Federation/FederationManager.php

@@ -36,8 +36,6 @@
 use OCA\Talk\Room;
 use OCA\Talk\Service\ParticipantService;
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Db\MultipleObjectsReturnedException;
-use OCP\DB\Exception as DBException;
 use OCP\IConfig;
 use OCP\IUser;
 
@@ -81,7 +79,6 @@ public function isEnabled(): bool {
 	 * @param string $remoteUrl
 	 * @param string $sharedSecret
 	 * @return int share id for this specific remote room share
-	 * @throws DBException
 	 */
 	public function addRemoteRoom(IUser $user, string $remoteId, int $roomType, string $roomName, string $roomToken, string $remoteUrl, string $sharedSecret): int {
 		try {
@@ -100,9 +97,7 @@ public function addRemoteRoom(IUser $user, string $remoteId, int $roomType, stri
 	}
 
 	/**
-	 * @throws DBException
 	 * @throws UnauthorizedException
-	 * @throws MultipleObjectsReturnedException
 	 * @throws DoesNotExistException
 	 * @throws CannotReachRemoteException
 	 */
@@ -136,9 +131,14 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): void {
 	}
 
 	/**
-	 * @throws DBException
+	 * @throws DoesNotExistException
+	 */
+	public function getRemoteShareById(int $shareId): Invitation {
+		return $this->invitationMapper->getInvitationById($shareId);
+	}
+
+	/**
 	 * @throws UnauthorizedException
-	 * @throws MultipleObjectsReturnedException
 	 * @throws DoesNotExistException
 	 */
 	public function rejectRemoteRoomShare(IUser $user, int $shareId): void {
@@ -157,15 +157,11 @@ public function rejectRemoteRoomShare(IUser $user, int $shareId): void {
 	/**
 	 * @param IUser $user
 	 * @return Invitation[]
-	 * @throws DBException
 	 */
 	public function getRemoteRoomShares(IUser $user): array {
 		return $this->invitationMapper->getInvitationsForUser($user);
 	}
 
-	/**
-	 * @throws DBException
-	 */
 	public function getNumberOfInvitations(Room $room): int {
 		return $this->invitationMapper->countInvitationsForRoom($room);
 	}

lib/Model/InvitationMapper.php

@@ -27,9 +27,7 @@
 
 use OCA\Talk\Room;
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\AppFramework\Db\QBMapper;
-use OCP\DB\Exception as DBException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
 use OCP\IUser;
@@ -50,8 +48,6 @@ public function __construct(IDBConnection $db) {
 	}
 
 	/**
-	 * @throws DBException
-	 * @throws MultipleObjectsReturnedException
 	 * @throws DoesNotExistException
 	 */
 	public function getInvitationById(int $id): Invitation {
@@ -67,7 +63,6 @@ public function getInvitationById(int $id): Invitation {
 	/**
 	 * @param Room $room
 	 * @return Invitation[]
-	 * @throws DBException
 	 */
 	public function getInvitationsForRoom(Room $room): array {
 		$qb = $this->db->getQueryBuilder();
@@ -82,7 +77,6 @@ public function getInvitationsForRoom(Room $room): array {
 	/**
 	 * @param IUser $user
 	 * @return Invitation[]
-	 * @throws DBException
 	 */
 	public function getInvitationsForUser(IUser $user): array {
 		$qb = $this->db->getQueryBuilder();
@@ -94,9 +88,6 @@ public function getInvitationsForUser(IUser $user): array {
 		return $this->findEntities($qb);
 	}
 
-	/**
-	 * @throws DBException
-	 */
 	public function countInvitationsForRoom(Room $room): int {
 		$qb = $this->db->getQueryBuilder();
 

lib/Notification/Notifier.php

@@ -31,6 +31,7 @@
 use OCA\Talk\Config;
 use OCA\Talk\Exceptions\ParticipantNotFoundException;
 use OCA\Talk\Exceptions\RoomNotFoundException;
+use OCA\Talk\Federation\FederationManager;
 use OCA\Talk\GuestManager;
 use OCA\Talk\Manager;
 use OCA\Talk\Model\Attendee;
@@ -45,7 +46,6 @@
 use OCP\Comments\ICommentsManager;
 use OCP\Comments\NotFoundException;
 use OCP\Files\IRootFolder;
-use OCP\HintException;
 use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IURLGenerator;
@@ -90,6 +90,7 @@ public function __construct(
 		protected Definitions $definitions,
 		protected AddressHandler $addressHandler,
 		protected BotServerMapper $botServerMapper,
+		protected FederationManager $federationManager,
 	) {
 		$this->commentManager = $commentManager;
 	}
@@ -395,12 +396,20 @@ protected function parseStoredRecording(
 		return $notification;
 	}
 
-	/**
-	 * @throws HintException
-	 */
 	protected function parseRemoteInvitationMessage(INotification $notification, IL10N $l): INotification {
 		$subjectParameters = $notification->getSubjectParameters();
 
+		try {
+			$invite = $this->federationManager->getRemoteShareById((int) $notification->getObjectId());
+			if ($invite->getUserId() !== $notification->getUser()) {
+				throw new AlreadyProcessedException();
+			}
+			$this->manager->getRoomById($invite->getRoomId());
+		} catch (RoomNotFoundException $e) {
+			// Room does not exist
+			throw new AlreadyProcessedException();
+		}
+
 		[$sharedById, $sharedByServer] = $this->addressHandler->splitUserRemote($subjectParameters['sharedByFederatedId']);
 
 		$message = $l->t('{user1} shared room {roomName} on {remoteServer} with you');

lib/ResponseDefinitions.php

@@ -98,8 +98,8 @@
  *     access_token: string,
  *     id: int,
  *     remote_id: string,
- *     remote_server: ?string,
- *     remote_token: ?string,
+ *     remote_server: string,
+ *     remote_token: string,
  *     room_id: int,
  *     user_id: string,
  * }

openapi.json

@@ -323,12 +323,10 @@
                         "type": "string"
                     },
                     "remote_server": {
-                        "type": "string",
-                        "nullable": true
+                        "type": "string"
                     },
                     "remote_token": {
-                        "type": "string",
-                        "nullable": true
+                        "type": "string"
                     },
                     "room_id": {
                         "type": "integer",

tests/integration/features/federation/invite.feature

@@ -40,6 +40,7 @@ Feature: federation/invite
       | room | actorType     | actorId      | systemMessage        | message                      | messageParameters |
       | room | users         | participant1 | federated_user_added | You invited {federated_user} | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} |
       | room | users         | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
+    And force run "OCA\Talk\BackgroundJob\RemoveEmptyRooms" background jobs
     And user "participant2" has the following invitations (v1)
       | remote_server | remote_token |
       | LOCAL         | room         |

tests/php/BackgroundJob/RemoveEmptyRoomsTest.php

@@ -26,6 +26,7 @@
 namespace OCA\Talk\Tests\BackgroundJob;
 
 use OCA\Talk\BackgroundJob\RemoveEmptyRooms;
+use OCA\Talk\Federation\FederationManager;
 use OCA\Talk\Manager;
 use OCA\Talk\Room;
 use OCA\Talk\Service\ParticipantService;
@@ -44,6 +45,8 @@ class RemoveEmptyRoomsTest extends TestCase {
 	protected $roomService;
 	/** @var ParticipantService|MockObject */
 	protected $participantService;
+	/** @var ParticipantService|MockObject */
+	protected $federationManager;
 	/** @var LoggerInterface|MockObject */
 	protected $loggerInterface;
 	/** @var IUserMountCache|MockObject */
@@ -56,6 +59,7 @@ public function setUp(): void {
 		$this->manager = $this->createMock(Manager::class);
 		$this->roomService = $this->createMock(RoomService::class);
 		$this->participantService = $this->createMock(ParticipantService::class);
+		$this->federationManager = $this->createMock(FederationManager::class);
 		$this->loggerInterface = $this->createMock(LoggerInterface::class);
 		$this->userMountCache = $this->createMock(IUserMountCache::class);
 	}
@@ -66,8 +70,9 @@ public function getBackgroundJob(): RemoveEmptyRooms {
 			$this->manager,
 			$this->roomService,
 			$this->participantService,
+			$this->federationManager,
 			$this->loggerInterface,
-			$this->userMountCache
+			$this->userMountCache,
 		);
 	}
 
@@ -77,12 +82,12 @@ public function testDoDeleteRoom(): void {
 		$room = $this->createMock(Room::class);
 		$room->method('getType')
 			->willReturn(Room::TYPE_GROUP);
-		$numDeletedRooms = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRooms = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals(0, $numDeletedRooms, 'Invalid default quantity of rooms');
 
-		$this->invokePrivate($backgroundJob, 'doDeleteRoom', [$room]);
+		self::invokePrivate($backgroundJob, 'doDeleteRoom', [$room]);
 
-		$numDeletedRooms = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRooms = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals(1, $numDeletedRooms, 'Invalid final quantity of rooms');
 	}
 
@@ -92,7 +97,7 @@ public function testDoDeleteRoom(): void {
 	public function testDeleteIfFileIsRemoved(string $objectType, array $fileList, int $numDeletedRoomsExpected): void {
 		$backgroundJob = $this->getBackgroundJob();
 
-		$numDeletedRoomsActual = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRoomsActual = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals(0, $numDeletedRoomsActual, 'Invalid default quantity of rooms');
 
 		$room = $this->createMock(Room::class);
@@ -101,13 +106,13 @@ public function testDeleteIfFileIsRemoved(string $objectType, array $fileList, i
 		$room->method('getObjectType')
 			->willReturn($objectType);
 
-		$userMountCache = $this->invokePrivate($backgroundJob, 'userMountCache');
+		$userMountCache = self::invokePrivate($backgroundJob, 'userMountCache');
 		$userMountCache->method('getMountsForFileId')
 			->willReturn($fileList);
 
-		$this->invokePrivate($backgroundJob, 'deleteIfFileIsRemoved', [$room]);
+		self::invokePrivate($backgroundJob, 'deleteIfFileIsRemoved', [$room]);
 
-		$numDeletedRoomsActual = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRoomsActual = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals($numDeletedRoomsExpected, $numDeletedRoomsActual, 'Invalid final quantity of rooms');
 	}
 
@@ -123,34 +128,44 @@ public static function dataDeleteIfFileIsRemoved(): array {
 	/**
 	 * @dataProvider dataDeleteIfIsEmpty
 	 */
-	public function testDeleteIfIsEmpty(string $objectType, int $actorsCount, int $numDeletedRoomsExpected): void {
+	public function testDeleteIfIsEmpty(string $objectType, int $actorsCount, int $inviteCount, int $numDeletedRoomsExpected): void {
 		$backgroundJob = $this->getBackgroundJob();
 
-		$numDeletedRoomsActual = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRoomsActual = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals(0, $numDeletedRoomsActual, 'Invalid default quantity of rooms');
 
 		$room = $this->createMock(Room::class);
 		$room->method('getType')
 			->willReturn(Room::TYPE_GROUP);
 		$room->method('getObjectType')
 			->willReturn($objectType);
+		$room->method('getRemoteServer')
+			->willReturn($inviteCount ? 'https://remote.example.tld' : '');
+		$room->method('getRemoteToken')
+			->willReturn($inviteCount ? 'remote' : '');
+
+		$this->federationManager->method('getNumberOfInvitations')
+			->with($room)
+			->willReturn($inviteCount);
 
-		$participantService = $this->invokePrivate($backgroundJob, 'participantService');
+		$participantService = self::invokePrivate($backgroundJob, 'participantService');
 		$participantService->method('getNumberOfActors')
 			->willReturn($actorsCount);
 
-		$this->invokePrivate($backgroundJob, 'deleteIfIsEmpty', [$room]);
+		self::invokePrivate($backgroundJob, 'deleteIfIsEmpty', [$room]);
 
-		$numDeletedRoomsActual = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRoomsActual = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals($numDeletedRoomsExpected, $numDeletedRoomsActual, 'Invalid final quantity of rooms');
 	}
 
 	public static function dataDeleteIfIsEmpty(): array {
 		return [
-			['', 1, 0],
-			['file', 1, 0],
-			['email', 1, 0],
-			['email', 0, 1]
+			'room with user' => ['', 1, 0, 0],
+			'room with fed invite' => ['', 0, 1, 0],
+			'room to delete' => ['', 0, 0, 1],
+			'file room with user' => ['file', 1, 0, 0],
+			'email room with user' => ['email', 1, 0, 0],
+			'email room without user' => ['email', 0, 0, 1]
 		];
 	}
 
@@ -165,7 +180,7 @@ public function testCallback(int $roomType, string $objectType, int $numDeletedR
 		$room->method('getObjectType')
 			->willReturn($objectType);
 		$backgroundJob->callback($room);
-		$numDeletedRoomsActual = $this->invokePrivate($backgroundJob, 'numDeletedRooms');
+		$numDeletedRoomsActual = self::invokePrivate($backgroundJob, 'numDeletedRooms');
 		$this->assertEquals($numDeletedRoomsExpected, $numDeletedRoomsActual, 'Invalid final quantity of rooms');
 	}