From 6d5de533a6f00e950c4b5ce202c5587e59ca9ba0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 13 Mar 2024 16:56:03 +0100 Subject: [PATCH 1/3] fix(federation): Send the display name when accepting the invite Signed-off-by: Joas Schilling --- lib/Federation/BackendNotifier.php | 2 ++ lib/Federation/CloudFederationProviderTalk.php | 5 +++++ lib/Federation/FederationManager.php | 2 +- .../integration/features/federation/chat.feature | 16 ++++++++-------- .../features/federation/invite.feature | 4 ++-- tests/php/Federation/FederationTest.php | 3 ++- 6 files changed, 20 insertions(+), 12 deletions(-) diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php index 15cfb0ac34e..cb2102afe96 100644 --- a/lib/Federation/BackendNotifier.php +++ b/lib/Federation/BackendNotifier.php @@ -179,6 +179,7 @@ public function sendShareAccepted( int $remoteAttendeeId, #[SensitiveParameter] string $accessToken, + string $displayName, ): bool { $remote = $this->prepareRemoteUrl($remoteServerUrl); @@ -191,6 +192,7 @@ public function sendShareAccepted( 'remoteServerUrl' => $this->getServerRemoteUrl(), 'sharedSecret' => $accessToken, 'message' => 'Recipient accepted the share', + 'displayName' => $displayName, ] ); diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 4615585afd7..8b3340388fe 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -213,6 +213,11 @@ public function notificationReceived($notificationType, $providerId, array $noti private function shareAccepted(int $id, array $notification): array { $attendee = $this->getLocalAttendeeAndValidate($id, $notification['sharedSecret']); + if (!empty($notification['displayName'])) { + $attendee->setDisplayName($notification['displayName']); + $this->attendeeMapper->update($attendee); + } + $this->session->set('talk-overwrite-actor-type', $attendee->getActorType()); $this->session->set('talk-overwrite-actor-id', $attendee->getActorId()); $this->session->set('talk-overwrite-actor-displayname', $attendee->getDisplayName()); diff --git a/lib/Federation/FederationManager.php b/lib/Federation/FederationManager.php index b5bd2ea25d9..88b56f4caee 100644 --- a/lib/Federation/FederationManager.php +++ b/lib/Federation/FederationManager.php @@ -134,7 +134,7 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant { // Add user to the room $room = $this->manager->getRoomById($invitation->getLocalRoomId()); if ( - !$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken()) + !$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken(), $user->getDisplayName()) ) { throw new CannotReachRemoteException(); } diff --git a/tests/integration/features/federation/chat.feature b/tests/integration/features/federation/chat.feature index 6636b0ea863..0849772cd0f 100644 --- a/tests/integration/features/federation/chat.feature +++ b/tests/integration/features/federation/chat.feature @@ -22,10 +22,10 @@ Feature: federation/chat | id | type | | room | 2 | And user "participant1" gets the following candidate mentions in room "room" for "" with 200 - | source | id | label | mentionId | - | calls | all | room | all | - | federated_users | participant2@{$REMOTE_URL} | participant2@localhost:8180 | federated_user/participant2@{$REMOTE_URL} | - | users | participant3 | participant3-displayname | participant3 | + | source | id | label | mentionId | + | calls | all | room | all | + | federated_users | participant2@{$REMOTE_URL} | participant2-displayname | federated_user/participant2@{$REMOTE_URL} | + | users | participant3 | participant3-displayname | participant3 | And user "participant2" gets the following candidate mentions in room "LOCAL::room" for "" with 200 | source | id | label | mentionId | | calls | all | room | all | @@ -56,10 +56,10 @@ Feature: federation/chat | id | type | | room | 2 | And user "participant1" gets the following candidate mentions in room "room" for "" with 200 - | source | id | label | mentionId | - | calls | all | room | all | - | federated_users | participant2@{$REMOTE_URL} | participant2@localhost:8180 | federated_user/participant2@{$REMOTE_URL} | - | federated_users | participant3@{$REMOTE_URL} | participant3@localhost:8180 | federated_user/participant3@{$REMOTE_URL} | + | source | id | label | mentionId | + | calls | all | room | all | + | federated_users | participant2@{$REMOTE_URL} | participant2-displayname | federated_user/participant2@{$REMOTE_URL} | + | federated_users | participant3@{$REMOTE_URL} | participant3-displayname | federated_user/participant3@{$REMOTE_URL} | And user "participant2" gets the following candidate mentions in room "LOCAL::room" for "" with 200 | source | id | label | mentionId | | calls | all | room | all | diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 9136077e5ad..684a29bc369 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -72,8 +72,8 @@ Feature: federation/invite | federated_users | participant2 | 3 | Then user "participant1" sees the following system messages in room "room" with 200 | room | actorType | actorId | systemMessage | message | messageParameters | - | room | federated_users | participant2@http://localhost:8180 | federated_user_added | {federated_user} accepted the invitation | {"actor":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | - | 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 | federated_users | participant2@http://localhost:8180 | federated_user_added | {federated_user} accepted the invitation | {"actor":{"type":"user","id":"participant2","name":"participant2-displayname","server":"http:\/\/localhost:8180"},"federated_user":{"type":"user","id":"participant2","name":"participant2-displayname","server":"http:\/\/localhost:8180"}} | + | 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-displayname","server":"http:\/\/localhost:8180"}} | | room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | # Remove a remote user after they joined When user "participant1" removes remote "participant2" from room "room" with 200 (v4) diff --git a/tests/php/Federation/FederationTest.php b/tests/php/Federation/FederationTest.php index 36b9143ecba..10e4c1c9387 100644 --- a/tests/php/Federation/FederationTest.php +++ b/tests/php/Federation/FederationTest.php @@ -404,6 +404,7 @@ public function testSendAcceptNotification() { 'sharedSecret' => $token, 'message' => 'Recipient accepted the share', 'remoteServerUrl' => 'http://example.tld', + 'displayName' => 'Foo Bar', ] ); @@ -428,7 +429,7 @@ public function testSendAcceptNotification() { ->with('/') ->willReturn('http://example.tld/index.php/'); - $success = $this->backendNotifier->sendShareAccepted($remote, $id, $token); + $success = $this->backendNotifier->sendShareAccepted($remote, $id, $token, 'Foo Bar'); $this->assertTrue($success); } From ec2ce3b97e0a8017b6209b70533331ae3478f05d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 13 Mar 2024 16:57:53 +0100 Subject: [PATCH 2/3] fix(federation): Fix unread marker handling in federated chats Signed-off-by: Joas Schilling --- appinfo/info.xml | 2 +- lib/Federation/BackendNotifier.php | 2 +- lib/Federation/CloudFederationProviderTalk.php | 4 +++- lib/Federation/FederationManager.php | 1 + .../Proxy/TalkV1/Controller/ChatController.php | 9 ++++++++- .../Proxy/TalkV1/Notifier/MessageSentListener.php | 3 ++- lib/Model/Attendee.php | 12 ++++++++++++ lib/Model/AttendeeMapper.php | 2 ++ lib/Model/SelectHelper.php | 2 ++ lib/Notification/FederationChatNotifier.php | 2 +- lib/Service/ParticipantService.php | 7 ++++--- lib/Service/RoomFormatter.php | 13 +++++++------ tests/php/Chat/ChatManagerTest.php | 7 +++++++ 13 files changed, 51 insertions(+), 15 deletions(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index d0d84c68973..16f164bbc22 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -16,7 +16,7 @@ And in the works for the [coming versions](https://github.com/nextcloud/spreed/m ]]> - 19.0.0-beta.1.1 + 19.0.0-beta.1.2 agpl Daniel Calviño Sánchez diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php index cb2102afe96..c45bedff61c 100644 --- a/lib/Federation/BackendNotifier.php +++ b/lib/Federation/BackendNotifier.php @@ -292,7 +292,7 @@ public function sendRoomModifiedUpdate( * Sent from Host server to Remote participant server * * @param array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string} $messageData - * @param array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool} $unreadInfo + * @param array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int} $unreadInfo */ public function sendMessageUpdate( string $remoteServer, diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 8b3340388fe..cb161b1193b 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -215,6 +215,7 @@ private function shareAccepted(int $id, array $notification): array { if (!empty($notification['displayName'])) { $attendee->setDisplayName($notification['displayName']); + $attendee->setState(Invitation::STATE_ACCEPTED); $this->attendeeMapper->update($attendee); } @@ -325,7 +326,7 @@ private function roomModified(int $remoteAttendeeId, array $notification): array /** * @param int $remoteAttendeeId - * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool}} $notification + * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int}} $notification * @return array * @throws ActionNotSupportedException * @throws AuthenticationFailedException @@ -416,6 +417,7 @@ private function messagePosted(int $remoteAttendeeId, array $notification): arra $notification['unreadInfo']['unreadMessages'], $notification['unreadInfo']['unreadMention'], $notification['unreadInfo']['unreadMentionDirect'], + $notification['unreadInfo']['lastReadMessage'], ); $this->federationChatNotifier->handleChatMessage($room, $participant, $message, $notification); diff --git a/lib/Federation/FederationManager.php b/lib/Federation/FederationManager.php index 88b56f4caee..d5753aad8f3 100644 --- a/lib/Federation/FederationManager.php +++ b/lib/Federation/FederationManager.php @@ -147,6 +147,7 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant { 'accessToken' => $invitation->getAccessToken(), 'remoteId' => $invitation->getRemoteAttendeeId(), 'invitedCloudId' => $invitation->getLocalCloudId(), + 'lastReadMessage' => $room->getLastMessageId(), ] ]; $attendees = $this->participantService->addUsers($room, $participant, $user); diff --git a/lib/Federation/Proxy/TalkV1/Controller/ChatController.php b/lib/Federation/Proxy/TalkV1/Controller/ChatController.php index 366c4bd552f..4ceb5a4a311 100644 --- a/lib/Federation/Proxy/TalkV1/Controller/ChatController.php +++ b/lib/Federation/Proxy/TalkV1/Controller/ChatController.php @@ -173,7 +173,12 @@ public function receiveMessages( ); if ($lookIntoFuture && $setReadMarker) { - $this->participantService->updateUnreadInfoForProxyParticipant($participant, 0, false, false); + $this->participantService->updateUnreadInfoForProxyParticipant($participant, + 0, + false, + false, + (int) ($proxy->getHeader('X-Chat-Last-Given') ?: $lastKnownMessageId), + ); } if ($proxy->getStatusCode() === Http::STATUS_NOT_MODIFIED) { @@ -384,6 +389,7 @@ public function setReadMarker(Room $room, Participant $participant, string $resp $data['unreadMessages'], $data['unreadMention'], $data['unreadMentionDirect'], + $data['lastReadMessage'], ); $headers = $lastCommonRead = []; @@ -424,6 +430,7 @@ public function markUnread(Room $room, Participant $participant, string $respons $data['unreadMessages'], $data['unreadMention'], $data['unreadMentionDirect'], + $data['lastReadMessage'], ); $headers = $lastCommonRead = []; diff --git a/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php b/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php index 7c12c9b32ed..6f1277c1b34 100644 --- a/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php +++ b/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php @@ -114,9 +114,10 @@ public function handle(Event $event): void { $lastMentionDirect = $attendee->getLastMentionDirect(); $unreadInfo = [ + 'lastReadMessage' => $lastReadMessage, 'unreadMessages' => $this->chatManager->getUnreadCount($event->getRoom(), $lastReadMessage), 'unreadMention' => $lastMention !== 0 && $lastReadMessage < $lastMention, - 'unreadMentionDirect' => $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect + 'unreadMentionDirect' => $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect, ]; $success = $this->backendNotifier->sendMessageUpdate( diff --git a/lib/Model/Attendee.php b/lib/Model/Attendee.php index dc2fd2dc644..823ac6fe7c7 100644 --- a/lib/Model/Attendee.php +++ b/lib/Model/Attendee.php @@ -66,6 +66,10 @@ * @method null|string getPhoneNumber() * @method void setCallId(?string $callId) * @method null|string getCallId() + * @method void setState(int $state) + * @method int getState() + * @method void setUnreadMessages(int $unreadMessages) + * @method int getUnreadMessages() */ class Attendee extends Entity { public const ACTOR_USERS = 'users'; @@ -168,6 +172,12 @@ class Attendee extends Entity { /** @var null|string */ protected $callId; + /** @var int */ + protected $state; + + /** @var int */ + protected $unreadMessages; + public function __construct() { $this->addType('roomId', 'int'); $this->addType('actorType', 'string'); @@ -189,6 +199,8 @@ public function __construct() { $this->addType('invitedCloudId', 'string'); $this->addType('phoneNumber', 'string'); $this->addType('callId', 'string'); + $this->addType('state', 'int'); + $this->addType('unreadMessages', 'int'); } public function getDisplayName(): string { diff --git a/lib/Model/AttendeeMapper.php b/lib/Model/AttendeeMapper.php index 268f5eab9f6..2cdab0b1901 100644 --- a/lib/Model/AttendeeMapper.php +++ b/lib/Model/AttendeeMapper.php @@ -299,6 +299,8 @@ public function createAttendeeFromRow(array $row): Attendee { 'invited_cloud_id' => (string) $row['invited_cloud_id'], 'phone_number' => $row['phone_number'], 'call_id' => $row['call_id'], + 'state' => (int) $row['state'], + 'unread_messages' => (int) $row['unread_messages'], ]); } } diff --git a/lib/Model/SelectHelper.php b/lib/Model/SelectHelper.php index a2513dda7e4..11304a0def5 100644 --- a/lib/Model/SelectHelper.php +++ b/lib/Model/SelectHelper.php @@ -87,6 +87,8 @@ public function selectAttendeesTable(IQueryBuilder $query, string $alias = 'a'): ->addSelect($alias . 'invited_cloud_id') ->addSelect($alias . 'phone_number') ->addSelect($alias . 'call_id') + ->addSelect($alias . 'state') + ->addSelect($alias . 'unread_messages') ->selectAlias($alias . 'id', 'a_id'); } diff --git a/lib/Notification/FederationChatNotifier.php b/lib/Notification/FederationChatNotifier.php index ef56078b181..08b6fc094b5 100644 --- a/lib/Notification/FederationChatNotifier.php +++ b/lib/Notification/FederationChatNotifier.php @@ -45,7 +45,7 @@ public function __construct( } /** - * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool}} $inboundNotification + * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int}} $inboundNotification */ public function handleChatMessage(Room $room, Participant $participant, ProxyCacheMessage $message, array $inboundNotification): void { /** @var array{silent?: bool, last_edited_time?: int, last_edited_by_type?: string, last_edited_by_id?: string, replyToActorType?: string, replyToActorId?: string} $metaData */ diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index 5060e80401e..d68ed521b80 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -249,9 +249,10 @@ public function updateLastReadMessage(Participant $participant, int $lastReadMes $this->attendeeMapper->update($attendee); } - public function updateUnreadInfoForProxyParticipant(Participant $participant, int $unreadMessageCount, bool $hasMention, bool $hadDirectMention): void { + public function updateUnreadInfoForProxyParticipant(Participant $participant, int $unreadMessageCount, bool $hasMention, bool $hadDirectMention, int $lastReadMessageId): void { $attendee = $participant->getAttendee(); - $attendee->setLastReadMessage($unreadMessageCount); + $attendee->setUnreadMessages($unreadMessageCount); + $attendee->setLastReadMessage($lastReadMessageId); $attendee->setLastMentionMessage($hasMention ? 1 : 0); $attendee->setLastMentionDirect($hadDirectMention ? 1 : 0); $this->attendeeMapper->update($attendee); @@ -505,7 +506,7 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null } $attendee->setParticipantType($participant['participantType'] ?? Participant::USER); $attendee->setPermissions(Attendee::PERMISSIONS_DEFAULT); - $attendee->setLastReadMessage($lastMessage); + $attendee->setLastReadMessage($participant['lastReadMessage'] ?? $lastMessage); $attendee->setReadPrivacy($readPrivacy); $attendees[] = $attendee; } diff --git a/lib/Service/RoomFormatter.php b/lib/Service/RoomFormatter.php index a14ce1957b4..1f6fee7b747 100644 --- a/lib/Service/RoomFormatter.php +++ b/lib/Service/RoomFormatter.php @@ -292,12 +292,11 @@ public function formatRoomV4( if ($attendee->getActorType() === Attendee::ACTOR_USERS) { $currentUser = $this->userManager->get($attendee->getActorId()); - if ($room->getRemoteServer() !== '') { - // For proxy conversations the information is the real counter, - // not the message ID requiring math afterward. - $roomData['unreadMessages'] = $attendee->getLastReadMessage(); - $roomData['unreadMention'] = (bool) $attendee->getLastMentionMessage(); - $roomData['unreadMentionDirect'] = (bool) $attendee->getLastMentionDirect(); + if ($room->isFederatedConversation()) { + $roomData['lastReadMessage'] = $attendee->getLastReadMessage(); + $roomData['unreadMention'] = (bool)$attendee->getLastMentionMessage(); + $roomData['unreadMentionDirect'] = (bool)$attendee->getLastMentionDirect(); + $roomData['unreadMessages'] = $attendee->getUnreadMessages(); } elseif ($currentUser instanceof IUser) { $lastReadMessage = $attendee->getLastReadMessage(); if ($lastReadMessage === -1) { @@ -338,6 +337,7 @@ public function formatRoomV4( $lastReadMessage = $attendee->getLastReadMessage(); $lastMention = $attendee->getLastMentionMessage(); $lastMentionDirect = $attendee->getLastMentionDirect(); + $roomData['lastReadMessage'] = $lastReadMessage; $roomData['unreadMessages'] = $this->chatManager->getUnreadCount($room, $lastReadMessage); $roomData['unreadMention'] = $lastMention !== 0 && $lastReadMessage < $lastMention; $roomData['unreadMentionDirect'] = $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect; @@ -388,6 +388,7 @@ public function formatRoomV4( $lastMessage, ); } elseif ($room->getRemoteServer() !== '') { + $roomData['lastCommonReadMessage'] = 0; try { $cachedMessage = $this->proxyCacheMessageMapper->findByRemote( $room->getRemoteServer(), diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index b8defc616b4..92246fd25a9 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -29,6 +29,7 @@ use OCA\Talk\Chat\Notifier; use OCA\Talk\Model\Attendee; use OCA\Talk\Model\AttendeeMapper; +use OCA\Talk\Model\Invitation; use OCA\Talk\Participant; use OCA\Talk\Room; use OCA\Talk\Service\AttachmentService; @@ -446,6 +447,8 @@ public function testDeleteMessage(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -505,6 +508,8 @@ public function testDeleteMessageFileShare(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -586,6 +591,8 @@ public function testDeleteMessageFileShareNotFound(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) From 30215a728be86a7ce25355f13f74f5c68425673d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 13 Mar 2024 17:18:55 +0100 Subject: [PATCH 3/3] fix(federation): Cache whether we have federated participants in a conversation Signed-off-by: Joas Schilling --- lib/Manager.php | 2 + .../Version19000Date20240313134926.php | 102 ++++++++++++++++++ lib/Model/SelectHelper.php | 1 + lib/Room.php | 23 ++++ lib/Service/ParticipantService.php | 11 ++ lib/Service/RoomService.php | 13 +++ tests/php/Service/RoomServiceTest.php | 1 + 7 files changed, 153 insertions(+) create mode 100644 lib/Migration/Version19000Date20240313134926.php diff --git a/lib/Manager.php b/lib/Manager.php index 2761fb8726d..b9a81c65e96 100644 --- a/lib/Manager.php +++ b/lib/Manager.php @@ -134,6 +134,7 @@ public function createRoomObjectFromData(array $data): Room { 'breakout_room_status' => 0, 'call_recording' => 0, 'recording_consent' => 0, + 'has_federation' => 0, ], $data)); } @@ -202,6 +203,7 @@ public function createRoomObject(array $row): Room { (int) $row['breakout_room_status'], (int) $row['call_recording'], (int) $row['recording_consent'], + (int) $row['has_federation'], ); } diff --git a/lib/Migration/Version19000Date20240313134926.php b/lib/Migration/Version19000Date20240313134926.php new file mode 100644 index 00000000000..f3713e293f7 --- /dev/null +++ b/lib/Migration/Version19000Date20240313134926.php @@ -0,0 +1,102 @@ + + * + * @author Joas Schilling + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Talk\Migration; + +use Closure; +use OCA\Talk\Model\Attendee; +use OCA\Talk\Model\Invitation; +use OCP\DB\ISchemaWrapper; +use OCP\DB\Types; +use OCP\IDBConnection; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +/** + * Cache the invite state in the attendees and room table to allow reducing efforts + */ +class Version19000Date20240313134926 extends SimpleMigrationStep { + public function __construct( + protected IDBConnection $connection, + ) { + } + + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + * @return null|ISchemaWrapper + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $table = $schema->getTable('talk_attendees'); + $table->addColumn('state', Types::SMALLINT, [ + 'default' => 0, + 'unsigned' => true, + ]); + $table->addColumn('unread_messages', Types::BIGINT, [ + 'default' => 0, + 'unsigned' => true, + ]); + + $table = $schema->getTable('talk_rooms'); + $table->addColumn('has_federation', Types::SMALLINT, [ + 'default' => 0, + 'unsigned' => true, + ]); + + return $schema; + } + + /** + * Set the invitation state to accepted for existing federated users + * Set the "has federation" for rooms with TalkV1 users + */ + public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options) { + $query = $this->connection->getQueryBuilder(); + $query->update('talk_attendees') + ->set('state', $query->createNamedParameter(Invitation::STATE_ACCEPTED)) + ->where($query->expr()->eq('actor_type', $query->createNamedParameter(Attendee::ACTOR_FEDERATED_USERS))); + $query->executeStatement(); + + $query = $this->connection->getQueryBuilder(); + $subQuery = $this->connection->getQueryBuilder(); + $subQuery->select('room_id') + ->from('talk_attendees') + ->where($subQuery->expr()->eq('actor_type', $query->createNamedParameter(Attendee::ACTOR_FEDERATED_USERS))) + ->groupBy('room_id'); + + $query = $this->connection->getQueryBuilder(); + $query->update('talk_rooms') + // Don't use const Room::HAS_FEDERATION_TALKv1 because the file might have been loaded with old content before the migration + // ->set('has_federation', $query->createNamedParameter(Room::HAS_FEDERATION_TALKv1)) + ->set('has_federation', $query->createNamedParameter(1)) + ->where($query->expr()->in('id', $query->createFunction($subQuery->getSQL()))); + $query->executeStatement(); + } +} diff --git a/lib/Model/SelectHelper.php b/lib/Model/SelectHelper.php index 11304a0def5..537992db42e 100644 --- a/lib/Model/SelectHelper.php +++ b/lib/Model/SelectHelper.php @@ -59,6 +59,7 @@ public function selectRoomsTable(IQueryBuilder $query, string $alias = 'r'): voi ->addSelect($alias . 'breakout_room_status') ->addSelect($alias . 'call_recording') ->addSelect($alias . 'recording_consent') + ->addSelect($alias . 'has_federation') ->selectAlias($alias . 'id', 'r_id'); } diff --git a/lib/Room.php b/lib/Room.php index 3fdaa081f09..e00625fda14 100644 --- a/lib/Room.php +++ b/lib/Room.php @@ -96,12 +96,16 @@ class Room { public const DESCRIPTION_MAXIMUM_LENGTH = 500; + public const HAS_FEDERATION_NONE = 0; + public const HAS_FEDERATION_TALKv1 = 1; + protected ?string $currentUser = null; protected ?Participant $participant = null; /** * @psalm-param Room::TYPE_* $type * @psalm-param RecordingService::CONSENT_REQUIRED_* $recordingConsent + * @psalm-param int-mask-of $hasFederation */ public function __construct( private Manager $manager, @@ -138,6 +142,7 @@ public function __construct( private int $breakoutRoomStatus, private int $callRecording, private int $recordingConsent, + private int $hasFederation, ) { } @@ -410,6 +415,9 @@ public function getRemoteToken(): string { return $this->remoteToken; } + /** + * @deprecated + */ public function isFederatedRemoteRoom(): bool { return $this->remoteServer !== ''; } @@ -558,4 +566,19 @@ public function getRecordingConsent(): int { public function setRecordingConsent(int $recordingConsent): void { $this->recordingConsent = $recordingConsent; } + + /** + * @psalm-return int-mask-of + */ + public function hasFederatedParticipants(): int { + return $this->hasFederation; + } + + /** + * @param int $hasFederation + * @psalm-param int-mask-of $hasFederation (bit map) + */ + public function setFederatedParticipants(int $hasFederation): void { + $this->hasFederation = $hasFederation; + } } diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index d68ed521b80..f8c0dae7cc6 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -514,6 +514,7 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null $event = new BeforeAttendeesAddedEvent($room, $attendees); $this->dispatcher->dispatchTyped($event); + $setFederationFlagAlready = $room->hasFederatedParticipants() & Room::HAS_FEDERATION_TALKv1; foreach ($attendees as $attendee) { try { $this->attendeeMapper->insert($attendee); @@ -524,6 +525,16 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null $this->attendeeMapper->delete($attendee); throw new CannotReachRemoteException(); } + + if (!$setFederationFlagAlready) { + $flag = $room->hasFederatedParticipants() | Room::HAS_FEDERATION_TALKv1; + + /** @var RoomService $roomService */ + $roomService = Server::get(RoomService::class); + $roomService->setHasFederation($room, $flag); + + $setFederationFlagAlready = true; + } } } catch (Exception $e) { if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { diff --git a/lib/Service/RoomService.php b/lib/Service/RoomService.php index d606282dd88..9dfae845d98 100644 --- a/lib/Service/RoomService.php +++ b/lib/Service/RoomService.php @@ -886,6 +886,19 @@ public function setLastMessageInfo(Room $room, int $messageId, \DateTime $dateTi $room->setLastActivity($dateTime); } + /** + * @psalm-param int-mask-of $hasFederation + */ + public function setHasFederation(Room $room, int $hasFederation): void { + $update = $this->db->getQueryBuilder(); + $update->update('talk_rooms') + ->set('has_federation', $update->createNamedParameter($hasFederation, IQueryBuilder::PARAM_INT)) + ->where($update->expr()->eq('id', $update->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))); + $update->executeStatement(); + + $room->setFederatedParticipants($hasFederation); + } + public function setLastActivity(Room $room, \DateTime $now): void { $update = $this->db->getQueryBuilder(); $update->update('talk_rooms') diff --git a/tests/php/Service/RoomServiceTest.php b/tests/php/Service/RoomServiceTest.php index 8c5119bf650..e944ff03cf3 100644 --- a/tests/php/Service/RoomServiceTest.php +++ b/tests/php/Service/RoomServiceTest.php @@ -400,6 +400,7 @@ public function testVerifyPassword(): void { BreakoutRoom::STATUS_STOPPED, Room::RECORDING_NONE, RecordingService::CONSENT_REQUIRED_NO, + Room::HAS_FEDERATION_NONE, ); $verificationResult = $service->verifyPassword($room, '1234');