diff options
author | Joas Schilling <coding@schilljs.com> | 2024-03-15 09:46:13 +0100 |
---|---|---|
committer | Joas Schilling <coding@schilljs.com> | 2024-03-15 10:00:14 +0100 |
commit | 30d5e7c3ab9fa2bace6d50f570042d546a626a7a (patch) | |
tree | 131d15b58fa91a30578e28b78099c79f9bff2513 | |
parent | 710f2c26ea6a9b977b87f49b530dbc15ef34b1ec (diff) |
fix(notifications): Fix federated users being authors of notifications
Signed-off-by: Joas Schilling <coding@schilljs.com>
-rw-r--r-- | lib/Federation/CloudFederationProviderTalk.php | 23 | ||||
-rw-r--r-- | lib/Notification/Notifier.php | 98 | ||||
-rw-r--r-- | tests/integration/features/federation/chat.feature | 34 | ||||
-rw-r--r-- | tests/php/Notification/NotifierTest.php | 4 |
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, ); } |