fix(notifications): Fix federated users being authors of notifications

Signed-off-by: Joas Schilling <coding@schilljs.com>

4 files changed, 109 insertions, 50 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 c3b404521..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());
 
diff --git a/tests/integration/features/federation/chat.feature b/tests/integration/features/federation/chat.feature
index 9405dbe26..d600b7e04 100644
--- a/tests/integration/features/federation/chat.feature
+++ b/tests/integration/features/federation/chat.feature
@@ -247,6 +247,40 @@ Feature: federation/chat
       | 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 e574a8aee..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,
 		);
 	}