Merge pull request #11808 from nextcloud/bugfix/noid/federated-notifications-with-guests

fix(notifications): Fix Undefined variable $comment at Notifier.php#693

4 files changed, 150 insertions, 63 deletions

diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php
index a17cb402e..43552b349 100644
--- a/lib/Federation/CloudFederationProviderTalk.php
+++ b/lib/Federation/CloudFederationProviderTalk.php
@@ -345,6 +345,19 @@ class CloudFederationProviderTalk implements ICloudFederationProvider {
 			throw new ShareNotFound(FederationManager::OCM_RESOURCE_NOT_FOUND);
 		}
 
+		// We transform the parameters when storing in the PCM, so we only have
+		// to do it once for each message.
+		// Note: `messageParameters` (array during parsing) vs `messageParameter` (string during sending)
+		$notification['messageData']['messageParameters'] = json_decode($notification['messageData']['messageParameter'], true, flags: JSON_THROW_ON_ERROR);
+		unset($notification['messageData']['messageParameter']);
+		$converted = $this->userConverter->convertMessage($room, $notification['messageData']);
+		$converted['messageParameter'] = json_encode($converted['messageParameters'], JSON_THROW_ON_ERROR);
+		unset($converted['messageParameters']);
+
+		/** @var array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string} $converted */
+		$notification['messageData'] = $converted;
+
+
 		$message = new ProxyCacheMessage();
 		$message->setLocalToken($room->getToken());
 		$message->setRemoteServerUrl($notification['remoteServerUrl']);
@@ -358,16 +371,6 @@ class CloudFederationProviderTalk implements ICloudFederationProvider {
 		if ($notification['messageData']['expirationDatetime']) {
 			$message->setExpirationDatetime(new \DateTime($notification['messageData']['expirationDatetime']));
 		}
-
-		// We transform the parameters when storing in the PCM, so we only have
-		// to do it once for each message.
-		$convertedParameters = $this->userConverter->convertMessageParameters($room, [
-			'message' => $notification['messageData']['message'],
-			'messageParameters' => json_decode($notification['messageData']['messageParameter'], true, flags: JSON_THROW_ON_ERROR),
-		]);
-		$notification['messageData']['message'] = $convertedParameters['message'];
-		$notification['messageData']['messageParameter'] = json_encode($convertedParameters['messageParameters'], JSON_THROW_ON_ERROR);
-
 		$message->setMessage($notification['messageData']['message']);
 		$message->setMessageParameters($notification['messageData']['messageParameter']);
 		$message->setCreationDatetime(new \DateTime($notification['messageData']['creationDatetime']));
diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php
index 939c7b8da..7896ac6cf 100644
--- a/lib/Notification/Notifier.php
+++ b/lib/Notification/Notifier.php
@@ -47,6 +47,7 @@ use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Comments\ICommentsManager;
 use OCP\Comments\NotFoundException;
+use OCP\Federation\ICloudIdManager;
 use OCP\Files\IRootFolder;
 use OCP\IGroupManager;
 use OCP\IL10N;
@@ -93,6 +94,7 @@ class Notifier implements INotifier {
 		protected AddressHandler $addressHandler,
 		protected BotServerMapper $botServerMapper,
 		protected FederationManager $federationManager,
+		protected ICloudIdManager $cloudIdManager,
 	) {
 		$this->commentManager = $commentManager;
 	}
@@ -476,6 +478,46 @@ class Notifier implements INotifier {
 			throw new \InvalidArgumentException('Unknown object type');
 		}
 
+		$messageParameters = $notification->getMessageParameters();
+		if (!isset($messageParameters['commentId']) && !isset($messageParameters['proxyId'])) {
+			throw new AlreadyProcessedException();
+		}
+
+		if (isset($messageParameters['commentId'])) {
+			if (!$this->notificationManager->isPreparingPushNotification()
+				&& $notification->getObjectType() === 'chat'
+				/**
+				 * Notification only contains the message id of the target comment
+				 * not the one of the reaction, so we can't determine if it was read.
+				 * @see Listener::markReactionNotificationsRead()
+				 */
+				&& $notification->getSubject() !== 'reaction'
+				&& ((int) $messageParameters['commentId']) <= $participant->getAttendee()->getLastReadMessage()) {
+				// Mark notifications of messages that are read as processed
+				throw new AlreadyProcessedException();
+			}
+
+			try {
+				$comment = $this->commentManager->get($messageParameters['commentId']);
+			} catch (NotFoundException $e) {
+				throw new AlreadyProcessedException();
+			}
+
+			$message = $this->messageParser->createMessage($room, $participant, $comment, $l);
+			$this->messageParser->parseMessage($message);
+
+			if (!$message->getVisibility()) {
+				throw new AlreadyProcessedException();
+			}
+		} else {
+			try {
+				$proxy = $this->proxyCacheMessageMapper->findById($messageParameters['proxyId']);
+				$message = $this->messageParser->createMessageFromProxyCache($room, $participant, $proxy, $l);
+			} catch (DoesNotExistException) {
+				throw new AlreadyProcessedException();
+			}
+		}
+
 		$subjectParameters = $notification->getSubjectParameters();
 
 		$richSubjectUser = null;
@@ -491,6 +533,22 @@ class Notifier implements INotifier {
 					'name' => $userDisplayName,
 				];
 			}
+		} elseif ($subjectParameters['userType'] === Attendee::ACTOR_FEDERATED_USERS) {
+			try {
+				$cloudId = $this->cloudIdManager->resolveCloudId($message->getActorId());
+				$richSubjectUser = [
+					'type' => 'user',
+					'id' => $cloudId->getUser(),
+					'name' => $message->getActorDisplayName(),
+					'server' => $cloudId->getRemote(),
+				];
+			} catch (\InvalidArgumentException) {
+				$richSubjectUser = [
+					'type' => 'highlight',
+					'id' => $message->getActorId(),
+					'name' => $message->getActorId(),
+				];
+			}
 		} elseif ($subjectParameters['userType'] === Attendee::ACTOR_BOTS) {
 			$botId = $subjectParameters['userId'];
 			try {
@@ -519,46 +577,6 @@ class Notifier implements INotifier {
 			'icon-url' => $this->avatarService->getAvatarUrl($room),
 		];
 
-		$messageParameters = $notification->getMessageParameters();
-		if (!isset($messageParameters['commentId']) && !isset($messageParameters['proxyId'])) {
-			throw new AlreadyProcessedException();
-		}
-
-		if (isset($messageParameters['commentId'])) {
-			if (!$this->notificationManager->isPreparingPushNotification()
-				&& $notification->getObjectType() === 'chat'
-				/**
-				 * Notification only contains the message id of the target comment
-				 * not the one of the reaction, so we can't determine if it was read.
-				 * @see Listener::markReactionNotificationsRead()
-				 */
-				&& $notification->getSubject() !== 'reaction'
-				&& ((int) $messageParameters['commentId']) <= $participant->getAttendee()->getLastReadMessage()) {
-				// Mark notifications of messages that are read as processed
-				throw new AlreadyProcessedException();
-			}
-
-			try {
-				$comment = $this->commentManager->get($messageParameters['commentId']);
-			} catch (NotFoundException $e) {
-				throw new AlreadyProcessedException();
-			}
-
-			$message = $this->messageParser->createMessage($room, $participant, $comment, $l);
-			$this->messageParser->parseMessage($message);
-
-			if (!$message->getVisibility()) {
-				throw new AlreadyProcessedException();
-			}
-		} else {
-			try {
-				$proxy = $this->proxyCacheMessageMapper->findById($messageParameters['proxyId']);
-				$message = $this->messageParser->createMessageFromProxyCache($room, $participant, $proxy, $l);
-			} catch (DoesNotExistException) {
-				throw new AlreadyProcessedException();
-			}
-		}
-
 		// Set the link to the specific message
 		$notification->setLink($this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]) . '#message_' . $message->getMessageId());
 
@@ -603,7 +621,7 @@ class Notifier implements INotifier {
 			}
 			$richSubjectParameters['message'] = [
 				'type' => 'highlight',
-				'id' => $message->getComment()->getId(),
+				'id' => $message->getMessageId(),
 				'name' => $shortenMessage,
 			];
 			if ($notification->getSubject() === 'reminder') {
@@ -621,7 +639,7 @@ class Notifier implements INotifier {
 					$subject = $l->t('Reminder: Deleted user in {call}') . "\n{message}";
 				} else {
 					try {
-						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 						// TRANSLATORS Reminder for a message from a guest in conversation {call}
 						$subject = $l->t('Reminder: {guest} (guest) in {call}') . "\n{message}";
 					} catch (ParticipantNotFoundException $e) {
@@ -644,7 +662,7 @@ class Notifier implements INotifier {
 					$subject = $l->t('Deleted user reacted with {reaction} in {call}') . "\n{message}";
 				} else {
 					try {
-						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 						$subject = $l->t('{guest} (guest) reacted with {reaction} in {call}') . "\n{message}";
 					} catch (ParticipantNotFoundException $e) {
 						$subject = $l->t('Guest reacted with {reaction} in {call}') . "\n{message}";
@@ -659,7 +677,7 @@ class Notifier implements INotifier {
 					$subject = $l->t('Deleted user in {call}') . "\n{message}";
 				} else {
 					try {
-						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+						$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 						$subject = $l->t('{guest} (guest) in {call}') . "\n{message}";
 					} catch (ParticipantNotFoundException $e) {
 						$subject = $l->t('Guest in {call}') . "\n{message}";
@@ -675,7 +693,7 @@ class Notifier implements INotifier {
 				$subject = $l->t('A deleted user sent a message in conversation {call}');
 			} else {
 				try {
-					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 					$subject = $l->t('{guest} (guest) sent a message in conversation {call}');
 				} catch (ParticipantNotFoundException $e) {
 					$subject = $l->t('A guest sent a message in conversation {call}');
@@ -690,7 +708,7 @@ class Notifier implements INotifier {
 				$subject = $l->t('A deleted user replied to your message in conversation {call}');
 			} else {
 				try {
-					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 					$subject = $l->t('{guest} (guest) replied to your message in conversation {call}');
 				} catch (ParticipantNotFoundException $e) {
 					$subject = $l->t('A guest replied to your message in conversation {call}');
@@ -715,7 +733,7 @@ class Notifier implements INotifier {
 				$subject = $l->t('Reminder: A deleted user in conversation {call}');
 			} else {
 				try {
-					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 					$subject = $l->t('Reminder: {guest} (guest) in conversation {call}');
 				} catch (ParticipantNotFoundException) {
 					$subject = $l->t('Reminder: A guest in conversation {call}');
@@ -736,7 +754,7 @@ class Notifier implements INotifier {
 				$subject = $l->t('A deleted user reacted with {reaction} to your message in conversation {call}');
 			} else {
 				try {
-					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+					$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 					$subject = $l->t('{guest} (guest) reacted with {reaction} to your message in conversation {call}');
 				} catch (ParticipantNotFoundException $e) {
 					$subject = $l->t('A guest reacted with {reaction} to your message in conversation {call}');
@@ -776,7 +794,7 @@ class Notifier implements INotifier {
 			}
 		} else {
 			try {
-				$richSubjectParameters['guest'] = $this->getGuestParameter($room, $comment->getActorId());
+				$richSubjectParameters['guest'] = $this->getGuestParameter($room, $message->getActorId());
 				if ($notification->getSubject() === 'mention_group') {
 					$groupName = $this->groupManager->getDisplayName($subjectParameters['sourceId']) ?? $subjectParameters['sourceId'];
 					$richSubjectParameters['group'] = [
@@ -821,7 +839,7 @@ class Notifier implements INotifier {
 						[
 							'apiVersion' => 'v1',
 							'token' => $room->getToken(),
-							'messageId' => $comment->getId(),
+							'messageId' => $message->getMessageId(),
 						]
 					),
 					IAction::TYPE_DELETE
diff --git a/tests/integration/features/federation/chat.feature b/tests/integration/features/federation/chat.feature
index 0c12fb3f0..d600b7e04 100644
--- a/tests/integration/features/federation/chat.feature
+++ b/tests/integration/features/federation/chat.feature
@@ -222,6 +222,65 @@ Feature: federation/chat
       | spreed | chat        | room/Hi @"federated_user/participant2@{$REMOTE_URL}" bye | participant1-displayname mentioned you in conversation room | Hi @participant2-displayname bye |
       | spreed | chat        | room/Message 1-1         | participant1-displayname replied to your message in conversation room  | Message 1-1 |
 
+  Scenario: Mentioning a federated user as a guest also triggers a notification for them
+    Given the following "spreed" app config is set
+      | federation_enabled | yes |
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 3 |
+      | roomName | room |
+    And user "participant1" adds federated_user "participant2" to room "room" with 200 (v4)
+    And user "participant2" has the following invitations (v1)
+      | remoteServerUrl | remoteToken | state | inviterCloudId                     | inviterDisplayName       |
+      | LOCAL           | room        | 0     | participant1@http://localhost:8080 | participant1-displayname |
+    And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1)
+      | id   | name | type | remoteServer | remoteToken |
+      | room | room | 3    | LOCAL        | room        |
+    Then user "participant2" is participant of the following rooms (v4)
+      | id   | type |
+      | room | 3    |
+    # Join and leave to clear the invite notification
+    Given user "participant2" joins room "LOCAL::room" with 200 (v4)
+    Given user "participant2" leaves room "LOCAL::room" with 200 (v4)
+    And user "guest" joins room "room" with 200 (v4)
+    When user "guest" sends message 'Hi @"federated_user/participant2@{$REMOTE_URL}" bye' to room "room" with 201
+    Then user "participant2" has the following notifications
+      | app    | object_type | object_id                | subject                                                                | message     |
+      | spreed | chat        | room/Hi @"federated_user/participant2@{$REMOTE_URL}" bye | A guest mentioned you in conversation room | Hi @participant2-displayname bye |
+
+  Scenario: Mentioning a federated user as a federated user that is a local user to the mentioned one also triggers a notification for them
+    Given the following "spreed" app config is set
+      | federation_enabled | yes |
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 3 |
+      | roomName | room |
+    And user "participant1" adds federated_user "participant2" to room "room" with 200 (v4)
+    And user "participant2" has the following invitations (v1)
+      | remoteServerUrl | remoteToken | state | inviterCloudId                     | inviterDisplayName       |
+      | LOCAL           | room        | 0     | participant1@http://localhost:8080 | participant1-displayname |
+    And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1)
+      | id   | name | type | remoteServer | remoteToken |
+      | room | room | 3    | LOCAL        | room        |
+    Then user "participant2" is participant of the following rooms (v4)
+      | id   | type |
+      | room | 3    |
+    And user "participant1" adds federated_user "participant3" to room "room" with 200 (v4)
+    And user "participant3" has the following invitations (v1)
+      | remoteServerUrl | remoteToken | state | inviterCloudId                     | inviterDisplayName       |
+      | LOCAL           | room        | 0     | participant1@http://localhost:8080 | participant1-displayname |
+    And user "participant3" accepts invite to room "room" of server "LOCAL" with 200 (v1)
+      | id   | name | type | remoteServer | remoteToken |
+      | room | room | 3    | LOCAL        | room        |
+    Then user "participant3" is participant of the following rooms (v4)
+      | id   | type |
+      | room | 3    |
+    # Join and leave to clear the invite notification
+    Given user "participant2" joins room "LOCAL::room" with 200 (v4)
+    Given user "participant2" leaves room "LOCAL::room" with 200 (v4)
+    When user "participant3" sends message 'Hi @"federated_user/participant2@{$REMOTE_URL}" bye' to room "LOCAL::room" with 201
+    Then user "participant2" has the following notifications
+      | app    | object_type | object_id                | subject                                                                | message     |
+      | spreed | chat        | room/Hi @"federated_user/participant2@{$REMOTE_URL}" bye | participant3-displayname mentioned you in conversation room | Hi @participant2-displayname bye |
+
   Scenario: Reaction on federated chat messages
     Given the following "spreed" app config is set
       | federation_enabled | yes |
diff --git a/tests/php/Notification/NotifierTest.php b/tests/php/Notification/NotifierTest.php
index db22eff66..0f9de3c1e 100644
--- a/tests/php/Notification/NotifierTest.php
+++ b/tests/php/Notification/NotifierTest.php
@@ -41,6 +41,7 @@ use OCA\Talk\Service\AvatarService;
 use OCA\Talk\Service\ParticipantService;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Comments\IComment;
+use OCP\Federation\ICloudIdManager;
 use OCP\Files\IRootFolder;
 use OCP\IGroupManager;
 use OCP\IL10N;
@@ -98,6 +99,7 @@ class NotifierTest extends TestCase {
 	protected $botServerMapper;
 	/** @var FederationManager|MockObject */
 	protected $federationManager;
+	protected ICloudIdManager|MockObject $cloudIdManager;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -122,6 +124,7 @@ class NotifierTest extends TestCase {
 		$this->addressHandler = $this->createMock(AddressHandler::class);
 		$this->botServerMapper = $this->createMock(BotServerMapper::class);
 		$this->federationManager = $this->createMock(FederationManager::class);
+		$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
 
 		$this->notifier = new Notifier(
 			$this->lFactory,
@@ -144,6 +147,7 @@ class NotifierTest extends TestCase {
 			$this->addressHandler,
 			$this->botServerMapper,
 			$this->federationManager,
+			$this->cloudIdManager,
 		);
 	}
 
@@ -966,9 +970,6 @@ class NotifierTest extends TestCase {
 		$comment->expects($this->any())
 			->method('getActorId')
 			->willReturn('random-hash');
-		$comment->expects($this->any())
-			->method('getId')
-			->willReturn('123456789');
 		$this->commentsManager->expects($this->once())
 			->method('get')
 			->with('23')
@@ -1010,6 +1011,12 @@ class NotifierTest extends TestCase {
 			->willReturn(true);
 		$chatMessage->method('getComment')
 			->willReturn($comment);
+		$chatMessage->expects($this->any())
+			->method('getMessageId')
+			->willReturn(123456789);
+		$chatMessage->expects($this->any())
+			->method('getActorId')
+			->willReturn('random-hash');
 
 		$this->messageParser->expects($this->once())
 			->method('createMessage')