Review fixes

Signed-off-by: Gary Kim <[email protected]>

lib/BackgroundJob/RetryJob.php

@@ -51,9 +51,6 @@ class RetryJob extends Job {
 	/** @var int max number of attempts to send the request */
 	private $maxTry = 20;
 
-	/** @var int how much time should be between two tries (10 minutes) */
-	private $interval = 600;
-
 
 	public function __construct(Notifications $notifications,
 								ITimeFactory $timeFactory) {
@@ -94,6 +91,11 @@ protected function run($argument): void {
 	 */
 	protected function shouldRun(array $argument): bool {
 		$lastRun = (int)$argument['lastRun'];
-		return (($this->time->getTime() - $lastRun) > $this->interval);
+		$try = (int)$argument['try'];
+		return (($this->time->getTime() - $lastRun) > $this->nextRunBreak($try));
+	}
+
+	protected function nextRunBreak(int $try): int {
+		return min(($try + 1) * 300, 3600);
 	}
 }

lib/Federation/FederationManager.php

@@ -50,6 +50,7 @@
  */
 class FederationManager {
 	public const TALK_ROOM_RESOURCE = 'talk-room';
+	public const TALK_PROTOCOL_NAME = 'nctalk';
 	public const TOKEN_LENGTH = 15;
 
 	/** @var IConfig */

lib/Federation/Notifications.php

@@ -29,10 +29,8 @@
 use OCA\Talk\AppInfo\Application;
 use OCA\Talk\BackgroundJob\RetryJob;
 use OCA\Talk\Exceptions\RoomHasNoModeratorException;
-use OCA\Talk\Model\Attendee;
-use OCA\Talk\Model\AttendeeMapper;
-use OCA\Talk\Participant;
 use OCA\Talk\Room;
+use OCA\Talk\Service\ParticipantService;
 use OCP\BackgroundJob\IJobList;
 use OCP\Federation\ICloudFederationFactory;
 use OCP\Federation\ICloudFederationNotification;
@@ -60,8 +58,8 @@ class Notifications {
 	/** @var IUserManager */
 	private $userManager;
 
-	/** @var AttendeeMapper */
-	private $attendeeMapper;
+	/** @var ParticipantService */
+	private $participantService;
 
 	public function __construct(
 		ICloudFederationFactory $cloudFederationFactory,
@@ -70,15 +68,15 @@ public function __construct(
 		ICloudFederationProviderManager $federationProviderManager,
 		IJobList $jobList,
 		IUserManager $userManager,
-		AttendeeMapper $attendeeMapper
+		ParticipantService $participantService
 	) {
 		$this->cloudFederationFactory = $cloudFederationFactory;
 		$this->addressHandler = $addressHandler;
 		$this->logger = $logger;
 		$this->federationProviderManager = $federationProviderManager;
 		$this->jobList = $jobList;
 		$this->userManager = $userManager;
-		$this->attendeeMapper = $attendeeMapper;
+		$this->participantService = $participantService;
 	}
 
 	/**
@@ -104,21 +102,11 @@ public function sendRemoteShare(string $providerId, string $token, string $share
 
 		/** @var IUser|null $roomOwner */
 		$roomOwner = null;
-		try {
-			$roomOwners = $this->attendeeMapper->getActorsByParticipantTypes($room->getId(), [Participant::OWNER]);
-			if (!empty($roomOwners) && $roomOwners[0]->getActorType() === Attendee::ACTOR_USERS) {
-				$roomOwner = $this->userManager->get($roomOwners[0]->getActorId());
-			}
-		} catch (\Exception $e) {
-			// Get a local moderator instead
-			try {
-				$roomOwners = $this->attendeeMapper->getActorsByParticipantTypes($room->getId(), [Participant::MODERATOR]);
-				if (!empty($roomOwners) && $roomOwners[0]->getActorType() === Attendee::ACTOR_USERS) {
-					$roomOwner = $this->userManager->get($roomOwners[0]->getActorId());
-				}
-			} catch (\Exception $e) {
-				throw new RoomHasNoModeratorException();
-			}
+		$roomOwnerAttendee = $this->participantService->getHighestPermissionAttendee($room);
+		if ($roomOwnerAttendee) {
+			$roomOwner = $this->userManager->get($roomOwnerAttendee->getActorId());
+		} else {
+			throw new RoomHasNoModeratorException();
 		}
 
 		$remote = $this->prepareRemoteUrl($remote);
@@ -141,7 +129,7 @@ public function sendRemoteShare(string $providerId, string $token, string $share
 		$protocol = $share->getProtocol();
 		$protocol['roomName'] = $roomName;
 		$protocol['roomType'] = $roomType;
-		$protocol['name'] = 'nctalk';
+		$protocol['name'] = FederationManager::TALK_PROTOCOL_NAME;
 		$share->setProtocol($protocol);
 
 		$response = $this->federationProviderManager->sendShare($share);

lib/Service/ParticipantService.php

@@ -53,6 +53,7 @@
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Comments\IComment;
+use OCP\DB\Exception as DBException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IConfig;
@@ -306,6 +307,7 @@ public function joinRoomAsNewGuest(Room $room, string $password, bool $passedPas
 	 * @param Room $room
 	 * @param array $participants
 	 * @param IUser|null $addedBy User that is attempting to add these users (must be set for federated users to be added)
+	 * @throws \Exception thrown if $addedBy is not set when adding a federated user
 	 */
 	public function addUsers(Room $room, array $participants, ?IUser $addedBy = null): void {
 		if (empty($participants)) {
@@ -326,7 +328,7 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null
 				$readPrivacy = $this->talkConfig->getUserReadPrivacy($participant['actorId']);
 			} elseif ($participant['actorType'] === Attendee::ACTOR_FEDERATED_USERS) {
 				if ($addedBy === null) {
-					continue;
+					throw new \Exception('$addedBy must be set to add a federated user');
 				}
 				$participant['accessToken'] = $this->secureRandom->generate(
 					FederationManager::TOKEN_LENGTH,
@@ -355,7 +357,7 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null
 			$attendees[] = $attendee;
 
 			if ($attendee->getActorType() === Attendee::ACTOR_FEDERATED_USERS) {
-				$this->sendRemoteShare($room, $addedBy, $participant['actorId'], $participant['accessToken'], $entity->getId());
+				$this->notifications->sendRemoteShare((string) $entity->getId(), $participant['accessToken'], $participant['actorId'], $addedBy->getDisplayName(), $addedBy->getCloudId(), 'user', $room);
 			}
 		}
 
@@ -365,8 +367,34 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null
 		$this->dispatcher->dispatch(Room::EVENT_AFTER_USERS_ADD, $event);
 	}
 
-	private function sendRemoteShare(Room $room, IUser $addedBy, string $addingUserId, string $token, int $attendeeId) {
-		$this->notifications->sendRemoteShare((string) $attendeeId, $token, $addingUserId, $addedBy->getDisplayName(), $addedBy->getCloudId(), 'user', $room);
+	public function getHighestPermissionAttendee(Room $room): ?Attendee {
+		$attendee = null;
+		try {
+			$roomOwners = $this->attendeeMapper->getActorsByParticipantTypes($room->getId(), [Participant::OWNER]);
+
+			if (!empty($roomOwners)) {
+				foreach ($roomOwners as $owner) {
+					if ($owner->getActorType() === Attendee::ACTOR_USERS) {
+						$attendee = $roomOwners[0];
+						break;
+					}
+				}
+
+			}
+			if (!$attendee) {
+				$roomModerators = $this->attendeeMapper->getActorsByParticipantTypes($room->getId(), [Participant::MODERATOR]);
+				if (!empty($roomOwners)) {
+					foreach ($roomModerators as $moderator) {
+						if ($moderator->getActorType() === Attendee::ACTOR_USERS) {
+							$attendee = $moderator;
+							break;
+						}
+					}
+				}
+			}
+		} catch (DBException $e) {
+		}
+		return $attendee;
 	}
 
 	/**

tests/php/Federation/FederationTest.php

@@ -28,7 +28,6 @@
 use OCA\Talk\Federation\FederationManager;
 use OCA\Talk\Federation\Notifications;
 use OCA\Talk\Manager;
-use OCA\Talk\MatterbridgeManager;
 use OCA\Talk\Model\Attendee;
 use OCA\Talk\Model\AttendeeMapper;
 use OCA\Talk\Participant;
@@ -75,6 +74,9 @@ class FederationTest extends TestCase {
 	/** @var AttendeeMapper */
 	protected $attendeeMapper;
 
+	/** @var ParticipantService */
+	protected $participantService;
+
 	public function setUp(): void {
 		parent::setUp();
 
@@ -83,6 +85,7 @@ public function setUp(): void {
 		$this->addressHandler = $this->createMock(AddressHandler::class);
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->attendeeMapper = $this->createMock(AttendeeMapper::class);
+		$this->participantService = $this->createMock(ParticipantService::class);
 
 		$this->notifications = new Notifications(
 			$this->cloudFederationFactory,
@@ -91,8 +94,7 @@ public function setUp(): void {
 			$this->cloudFederationProviderManager,
 			$this->createMock(IJobList::class),
 			$this->userManager,
-			$this->attendeeMapper,
-			$this->createMock(MatterbridgeManager::class),
+			$this->participantService,
 		);
 
 		$this->federationManager = $this->createMock(FederationManager::class);