From a5b1eb6692f3914ddecb5ecb4e8bdd94a5616a5c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 25 Mar 2024 14:07:19 +0100 Subject: [PATCH] fix(federation): Prevent duplicated invites with different casing and heal the cloudId Signed-off-by: Joas Schilling --- lib/Federation/BackendNotifier.php | 2 + .../CloudFederationProviderTalk.php | 5 ++ lib/Federation/FederationManager.php | 21 +++++- lib/Model/InvitationMapper.php | 10 ++- .../features/bootstrap/FeatureContext.php | 3 + .../features/federation/invite.feature | 64 +++++++++++++++++++ tests/php/Federation/FederationTest.php | 3 +- 7 files changed, 103 insertions(+), 5 deletions(-) diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php index a70d8e4be99..83f11e934ff 100644 --- a/lib/Federation/BackendNotifier.php +++ b/lib/Federation/BackendNotifier.php @@ -148,6 +148,7 @@ public function sendShareAccepted( #[SensitiveParameter] string $accessToken, string $displayName, + string $cloudId, ): bool { $remote = $this->prepareRemoteUrl($remoteServerUrl); @@ -161,6 +162,7 @@ public function sendShareAccepted( 'sharedSecret' => $accessToken, 'message' => 'Recipient accepted the share', 'displayName' => $displayName, + 'cloudId' => $cloudId, ] ); diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 6a0ae28a087..5c9e2c8240e 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -219,6 +219,11 @@ private function shareAccepted(int $id, array $notification): array { if (!empty($notification['displayName'])) { $attendee->setDisplayName($notification['displayName']); $attendee->setState(Invitation::STATE_ACCEPTED); + + if (!empty($notification['cloudId'])) { + $attendee->setActorId($notification['cloudId']); + } + $this->attendeeMapper->update($attendee); } diff --git a/lib/Federation/FederationManager.php b/lib/Federation/FederationManager.php index 4d1e7a15ab1..a45c6c8cd0b 100644 --- a/lib/Federation/FederationManager.php +++ b/lib/Federation/FederationManager.php @@ -39,7 +39,10 @@ use OCA\Talk\Room; use OCA\Talk\Service\ParticipantService; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Http; +use OCP\Federation\Exceptions\ProviderCouldNotAddShareException; use OCP\Federation\ICloudId; +use OCP\Federation\ICloudIdManager; use OCP\IUser; use OCP\Notification\IManager; use SensitiveParameter; @@ -68,6 +71,7 @@ public function __construct( private InvitationMapper $invitationMapper, private BackendNotifier $backendNotifier, private IManager $notificationManager, + private ICloudIdManager $cloudIdManager, private RestrictionValidator $restrictionValidator, ) { } @@ -97,12 +101,23 @@ public function addRemoteRoom( string $inviterDisplayName, string $localCloudId, ): Invitation { + $couldHaveInviteWithOtherCasing = false; try { $room = $this->manager->getRoomByToken($remoteToken, null, $remoteServerUrl); + $couldHaveInviteWithOtherCasing = true; } catch (RoomNotFoundException) { $room = $this->manager->createRemoteRoom($roomType, $roomName, $remoteToken, $remoteServerUrl); } + if ($couldHaveInviteWithOtherCasing) { + try { + $this->invitationMapper->getInvitationForUserByLocalRoom($room, $user->getUID(), true); + throw new ProviderCouldNotAddShareException('User already invited', '', Http::STATUS_BAD_REQUEST); + } catch (DoesNotExistException) { + // Not invited with any casing already, so all good. + } + } + $invitation = new Invitation(); $invitation->setUserId($user->getUID()); $invitation->setState(Invitation::STATE_PENDING); @@ -145,10 +160,13 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant { throw new \InvalidArgumentException('state'); } + + $cloudId = $this->cloudIdManager->getCloudId($user->getUID(), null); + // Add user to the room $room = $this->manager->getRoomById($invitation->getLocalRoomId()); if ( - !$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken(), $user->getDisplayName()) + !$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken(), $user->getDisplayName(), $cloudId->getId()) ) { throw new CannotReachRemoteException(); } @@ -169,6 +187,7 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant { $attendee = array_pop($attendees); $invitation->setState(Invitation::STATE_ACCEPTED); + $invitation->setLocalCloudId($cloudId->getId()); $this->invitationMapper->update($invitation); $this->markNotificationProcessed($user->getUID(), $shareId); diff --git a/lib/Model/InvitationMapper.php b/lib/Model/InvitationMapper.php index 5568f2fabe9..1c676aa4564 100644 --- a/lib/Model/InvitationMapper.php +++ b/lib/Model/InvitationMapper.php @@ -98,14 +98,18 @@ public function getInvitationsForUser(IUser $user): array { /** * @throws DoesNotExistException */ - public function getInvitationForUserByLocalRoom(Room $room, string $userId): Invitation { + public function getInvitationForUserByLocalRoom(Room $room, string $userId, bool $caseInsensitive = false): Invitation { $query = $this->db->getQueryBuilder(); $query->select('*') ->from($this->getTableName()) - ->where($query->expr()->eq('user_id', $query->createNamedParameter($userId))) - ->andWhere($query->expr()->eq('local_room_id', $query->createNamedParameter($room->getId()))); + ->where($query->expr()->eq('local_room_id', $query->createNamedParameter($room->getId()))); + if ($caseInsensitive) { + $query->andWhere($query->expr()->eq($query->func()->lower('user_id'), $query->createNamedParameter(strtolower($userId)))); + } else { + $query->andWhere($query->expr()->eq('user_id', $query->createNamedParameter($userId))); + } return $this->findEntity($query); } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 10931efc5ae..fb790f697ac 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -586,6 +586,9 @@ private function assertInvites($invites, TableNode $formData) { if (isset($expectedInvite['state'])) { $data['state'] = $invite['state']; } + if (isset($expectedInvite['localCloudId'])) { + $data['localCloudId'] = $invite['localCloudId']; + } return $data; }, $invites, $expectedInvites)); diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 684a29bc369..9388736433e 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -89,6 +89,70 @@ Feature: federation/invite | 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 | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | + Scenario: Invite user with wrong casing + 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) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + | federated_users | PARTICIPANT2 | 3 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | 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 | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | + And user "participant1" adds federated_user "participant2" to room "room" with 404 (v4) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + | federated_users | PARTICIPANT2 | 3 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | 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 | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | + And force run "OCA\Talk\BackgroundJob\RemoveEmptyRooms" background jobs + And user "participant2" has the following invitations (v1) + | remoteServerUrl | remoteToken | state | inviterCloudId | inviterDisplayName | localCloudId | + | LOCAL | room | 0 | participant1@http://localhost:8080 | participant1-displayname | PARTICIPANT2@http://localhost:8180 | + Then user "participant2" has the following notifications + | app | object_type | object_id | subject | message | + | spreed | remote_talk_share | INVITE_ID(LOCAL::room) | @participant1-displayname invited you to a federated conversation | @participant1-displayname invited you to join room on http://localhost:8080 | + And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1) + | id | name | type | remoteServer | remoteToken | + | room | room | 3 | LOCAL | room | + And user "participant2" accepts invite to room "room" of server "LOCAL" with 400 (v1) + | error | state | + And user "participant2" declines invite to room "room" of server "LOCAL" with 400 (v1) + | error | state | + And user "participant2" has the following invitations (v1) + | remoteServerUrl | remoteToken | state | inviterCloudId | inviterDisplayName | + | LOCAL | room | 1 | participant1@http://localhost:8080 | participant1-displayname | + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + | 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-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@localhost:8180","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) + And user "participant2" has the following invitations (v1) + Then user "participant2" is participant of the following rooms (v4) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | room | users | participant1 | federated_user_removed | You removed {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@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 | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | + Scenario: Declining an invite Given the following "spreed" app config is set | federation_enabled | yes | diff --git a/tests/php/Federation/FederationTest.php b/tests/php/Federation/FederationTest.php index 922586a0c5f..e0d83b6817c 100644 --- a/tests/php/Federation/FederationTest.php +++ b/tests/php/Federation/FederationTest.php @@ -417,6 +417,7 @@ public function testSendAcceptNotification() { 'message' => 'Recipient accepted the share', 'remoteServerUrl' => 'http://example.tld', 'displayName' => 'Foo Bar', + 'cloudId' => 'cloudId@example.tld', ] ); @@ -441,7 +442,7 @@ public function testSendAcceptNotification() { ->with('/') ->willReturn('http://example.tld/index.php/'); - $success = $this->backendNotifier->sendShareAccepted($remote, $id, $token, 'Foo Bar'); + $success = $this->backendNotifier->sendShareAccepted($remote, $id, $token, 'Foo Bar', 'cloudId@example.tld'); $this->assertTrue($success); }