Skip to content

Commit a5b1eb6

Browse files
nickvergessennickvergessen
committed
fix(federation): Prevent duplicated invites with different casing and heal the cloudId
Signed-off-by: Joas Schilling <coding@schilljs.com>
1 parent b1c6fc3 commit a5b1eb6

7 files changed

Lines changed: 103 additions & 5 deletions

File tree

‎lib/Federation/BackendNotifier.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ public function sendShareAccepted(
148148
#[SensitiveParameter]
149149
string $accessToken,
150150
string $displayName,
151+
string $cloudId,
151152
): bool {
152153
$remote = $this->prepareRemoteUrl($remoteServerUrl);
153154

@@ -161,6 +162,7 @@ public function sendShareAccepted(
161162
'sharedSecret' => $accessToken,
162163
'message' => 'Recipient accepted the share',
163164
'displayName' => $displayName,
165+
'cloudId' => $cloudId,
164166
]
165167
);
166168

‎lib/Federation/CloudFederationProviderTalk.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,11 @@ private function shareAccepted(int $id, array $notification): array {
219219
if (!empty($notification['displayName'])) {
220220
$attendee->setDisplayName($notification['displayName']);
221221
$attendee->setState(Invitation::STATE_ACCEPTED);
222+
223+
if (!empty($notification['cloudId'])) {
224+
$attendee->setActorId($notification['cloudId']);
225+
}
226+
222227
$this->attendeeMapper->update($attendee);
223228
}
224229

‎lib/Federation/FederationManager.php‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@
3939
use OCA\Talk\Room;
4040
use OCA\Talk\Service\ParticipantService;
4141
use OCP\AppFramework\Db\DoesNotExistException;
42+
use OCP\AppFramework\Http;
43+
use OCP\Federation\Exceptions\ProviderCouldNotAddShareException;
4244
use OCP\Federation\ICloudId;
45+
use OCP\Federation\ICloudIdManager;
4346
use OCP\IUser;
4447
use OCP\Notification\IManager;
4548
use SensitiveParameter;
@@ -68,6 +71,7 @@ public function __construct(
6871
private InvitationMapper $invitationMapper,
6972
private BackendNotifier $backendNotifier,
7073
private IManager $notificationManager,
74+
private ICloudIdManager $cloudIdManager,
7175
private RestrictionValidator $restrictionValidator,
7276
) {
7377
}
@@ -97,12 +101,23 @@ public function addRemoteRoom(
97101
string $inviterDisplayName,
98102
string $localCloudId,
99103
): Invitation {
104+
$couldHaveInviteWithOtherCasing = false;
100105
try {
101106
$room = $this->manager->getRoomByToken($remoteToken, null, $remoteServerUrl);
107+
$couldHaveInviteWithOtherCasing = true;
102108
} catch (RoomNotFoundException) {
103109
$room = $this->manager->createRemoteRoom($roomType, $roomName, $remoteToken, $remoteServerUrl);
104110
}
105111

112+
if ($couldHaveInviteWithOtherCasing) {
113+
try {
114+
$this->invitationMapper->getInvitationForUserByLocalRoom($room, $user->getUID(), true);
115+
throw new ProviderCouldNotAddShareException('User already invited', '', Http::STATUS_BAD_REQUEST);
116+
} catch (DoesNotExistException) {
117+
// Not invited with any casing already, so all good.
118+
}
119+
}
120+
106121
$invitation = new Invitation();
107122
$invitation->setUserId($user->getUID());
108123
$invitation->setState(Invitation::STATE_PENDING);
@@ -145,10 +160,13 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant {
145160
throw new \InvalidArgumentException('state');
146161
}
147162

163+
164+
$cloudId = $this->cloudIdManager->getCloudId($user->getUID(), null);
165+
148166
// Add user to the room
149167
$room = $this->manager->getRoomById($invitation->getLocalRoomId());
150168
if (
151-
!$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken(), $user->getDisplayName())
169+
!$this->backendNotifier->sendShareAccepted($invitation->getRemoteServerUrl(), $invitation->getRemoteAttendeeId(), $invitation->getAccessToken(), $user->getDisplayName(), $cloudId->getId())
152170
) {
153171
throw new CannotReachRemoteException();
154172
}
@@ -169,6 +187,7 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant {
169187
$attendee = array_pop($attendees);
170188

171189
$invitation->setState(Invitation::STATE_ACCEPTED);
190+
$invitation->setLocalCloudId($cloudId->getId());
172191
$this->invitationMapper->update($invitation);
173192

174193
$this->markNotificationProcessed($user->getUID(), $shareId);

‎lib/Model/InvitationMapper.php‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,14 +98,18 @@ public function getInvitationsForUser(IUser $user): array {
9898
/**
9999
* @throws DoesNotExistException
100100
*/
101-
public function getInvitationForUserByLocalRoom(Room $room, string $userId): Invitation {
101+
public function getInvitationForUserByLocalRoom(Room $room, string $userId, bool $caseInsensitive = false): Invitation {
102102
$query = $this->db->getQueryBuilder();
103103

104104
$query->select('*')
105105
->from($this->getTableName())
106-
->where($query->expr()->eq('user_id', $query->createNamedParameter($userId)))
107-
->andWhere($query->expr()->eq('local_room_id', $query->createNamedParameter($room->getId())));
106+
->where($query->expr()->eq('local_room_id', $query->createNamedParameter($room->getId())));
108107

108+
if ($caseInsensitive) {
109+
$query->andWhere($query->expr()->eq($query->func()->lower('user_id'), $query->createNamedParameter(strtolower($userId))));
110+
} else {
111+
$query->andWhere($query->expr()->eq('user_id', $query->createNamedParameter($userId)));
112+
}
109113
return $this->findEntity($query);
110114
}
111115

‎tests/integration/features/bootstrap/FeatureContext.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,9 @@ private function assertInvites($invites, TableNode $formData) {
586586
if (isset($expectedInvite['state'])) {
587587
$data['state'] = $invite['state'];
588588
}
589+
if (isset($expectedInvite['localCloudId'])) {
590+
$data['localCloudId'] = $invite['localCloudId'];
591+
}
589592

590593
return $data;
591594
}, $invites, $expectedInvites));

‎tests/integration/features/federation/invite.feature‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,70 @@ Feature: federation/invite
8989
| 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"}} |
9090
| room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
9191

92+
Scenario: Invite user with wrong casing
93+
Given the following "spreed" app config is set
94+
| federation_enabled | yes |
95+
Given user "participant1" creates room "room" (v4)
96+
| roomType | 3 |
97+
| roomName | room |
98+
And user "participant1" adds federated_user "PARTICIPANT2" to room "room" with 200 (v4)
99+
When user "participant1" sees the following attendees in room "room" with 200 (v4)
100+
| actorType | actorId | participantType |
101+
| users | participant1 | 1 |
102+
| federated_users | PARTICIPANT2 | 3 |
103+
Then user "participant1" sees the following system messages in room "room" with 200
104+
| room | actorType | actorId | systemMessage | message | messageParameters |
105+
| 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"}} |
106+
| room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
107+
And user "participant1" adds federated_user "participant2" to room "room" with 404 (v4)
108+
When user "participant1" sees the following attendees in room "room" with 200 (v4)
109+
| actorType | actorId | participantType |
110+
| users | participant1 | 1 |
111+
| federated_users | PARTICIPANT2 | 3 |
112+
Then user "participant1" sees the following system messages in room "room" with 200
113+
| room | actorType | actorId | systemMessage | message | messageParameters |
114+
| 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"}} |
115+
| room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
116+
And force run "OCA\Talk\BackgroundJob\RemoveEmptyRooms" background jobs
117+
And user "participant2" has the following invitations (v1)
118+
| remoteServerUrl | remoteToken | state | inviterCloudId | inviterDisplayName | localCloudId |
119+
| LOCAL | room | 0 | participant1@http://localhost:8080 | participant1-displayname | PARTICIPANT2@http://localhost:8180 |
120+
Then user "participant2" has the following notifications
121+
| app | object_type | object_id | subject | message |
122+
| 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 |
123+
And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1)
124+
| id | name | type | remoteServer | remoteToken |
125+
| room | room | 3 | LOCAL | room |
126+
And user "participant2" accepts invite to room "room" of server "LOCAL" with 400 (v1)
127+
| error | state |
128+
And user "participant2" declines invite to room "room" of server "LOCAL" with 400 (v1)
129+
| error | state |
130+
And user "participant2" has the following invitations (v1)
131+
| remoteServerUrl | remoteToken | state | inviterCloudId | inviterDisplayName |
132+
| LOCAL | room | 1 | participant1@http://localhost:8080 | participant1-displayname |
133+
When user "participant1" sees the following attendees in room "room" with 200 (v4)
134+
| actorType | actorId | participantType |
135+
| users | participant1 | 1 |
136+
| federated_users | participant2 | 3 |
137+
Then user "participant1" sees the following system messages in room "room" with 200
138+
| room | actorType | actorId | systemMessage | message | messageParameters |
139+
| 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"}} |
140+
| 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"}} |
141+
| room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
142+
# Remove a remote user after they joined
143+
When user "participant1" removes remote "participant2" from room "room" with 200 (v4)
144+
And user "participant2" has the following invitations (v1)
145+
Then user "participant2" is participant of the following rooms (v4)
146+
When user "participant1" sees the following attendees in room "room" with 200 (v4)
147+
| actorType | actorId | participantType |
148+
| users | participant1 | 1 |
149+
Then user "participant1" sees the following system messages in room "room" with 200
150+
| room | actorType | actorId | systemMessage | message | messageParameters |
151+
| 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"}} |
152+
| 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"}} |
153+
| 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"}} |
154+
| room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
155+
92156
Scenario: Declining an invite
93157
Given the following "spreed" app config is set
94158
| federation_enabled | yes |

‎tests/php/Federation/FederationTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,7 @@ public function testSendAcceptNotification() {
417417
'message' => 'Recipient accepted the share',
418418
'remoteServerUrl' => 'http://example.tld',
419419
'displayName' => 'Foo Bar',
420+
'cloudId' => 'cloudId@example.tld',
420421
]
421422
);
422423

@@ -441,7 +442,7 @@ public function testSendAcceptNotification() {
441442
->with('/')
442443
->willReturn('http://example.tld/index.php/');
443444

444-
$success = $this->backendNotifier->sendShareAccepted($remote, $id, $token, 'Foo Bar');
445+
$success = $this->backendNotifier->sendShareAccepted($remote, $id, $token, 'Foo Bar', 'cloudId@example.tld');
445446

446447
$this->assertTrue($success);
447448
}

0 commit comments

Comments
 (0)