Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: establish 1on1 mls conversation with new removal key [WPB-10744] #6502

Merged
merged 22 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/api-client/src/conversation/Conversation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import {RECEIPT_MODE} from './data';

import {MLSPublicKeyRecord} from '../client';
import {QualifiedId} from '../user';

import {ConversationMembers, ConversationProtocol} from './';
Expand Down Expand Up @@ -107,3 +108,20 @@ export interface MLSConversation extends Conversation {
cipher_suite: number;
protocol: ConversationProtocol.MLS;
}

export interface MLS1to1Conversation {
e-maad marked this conversation as resolved.
Show resolved Hide resolved
conversation: MLSConversation;
public_keys?: {
removal: MLSPublicKeyRecord;
};
}

export function isMLS1to1Conversation(response: unknown): response is MLS1to1Conversation {
if (typeof response === 'object' && response !== null) {
const mls1to1Conversation = response as MLS1to1Conversation;

return !!mls1to1Conversation.conversation && !!mls1to1Conversation.public_keys;
}

return false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
JoinConversationByCodePayload,
Member,
MessageSendingStatus,
MLS1to1Conversation,
MLSConversation,
NewConversation,
QualifiedConversationIds,
Expand Down Expand Up @@ -420,13 +421,13 @@ export class ConversationAPI {
* Get a MLS 1:1-conversation with a given user.
* @param userId - qualified user id
*/
public async getMLS1to1Conversation({domain, id}: QualifiedId): Promise<MLSConversation> {
public async getMLS1to1Conversation({domain, id}: QualifiedId): Promise<MLS1to1Conversation | MLSConversation> {
const config: AxiosRequestConfig = {
method: 'get',
url: `${ConversationAPI.URL.CONVERSATIONS}/${ConversationAPI.URL.ONE_2_ONE}/${domain}/${id}`,
};

const response = await this.client.sendJSON<MLSConversation>(config);
const response = await this.client.sendJSON<MLS1to1Conversation | MLSConversation>(config);
return response.data;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ describe('ConversationService', () => {

expect(mlsService.wipeConversation).toHaveBeenCalledWith(mockGroupId);
expect(mlsService.register1to1Conversation).toHaveBeenCalledTimes(1);
expect(mlsService.register1to1Conversation).toHaveBeenCalledWith(mockGroupId, otherUserId, selfUser);
expect(mlsService.register1to1Conversation).toHaveBeenCalledWith(mockGroupId, otherUserId, selfUser, undefined);
expect(conversationService.joinByExternalCommit).not.toHaveBeenCalled();
expect(establishedConversation.epoch).toEqual(updatedEpoch);
});
Expand Down Expand Up @@ -477,7 +477,7 @@ describe('ConversationService', () => {

expect(mlsService.wipeConversation).toHaveBeenCalledWith(mockGroupId);
expect(mlsService.register1to1Conversation).toHaveBeenCalledTimes(2);
expect(mlsService.register1to1Conversation).toHaveBeenCalledWith(mockGroupId, otherUserId, selfUser);
expect(mlsService.register1to1Conversation).toHaveBeenCalledWith(mockGroupId, otherUserId, selfUser, undefined);
expect(conversationService.joinByExternalCommit).not.toHaveBeenCalled();
expect(establishedConversation.epoch).toEqual(updatedEpoch);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
MLSConversation,
SUBCONVERSATION_ID,
Subconversation,
isMLS1to1Conversation,
} from '@wireapp/api-client/lib/conversation';
import {CONVERSATION_TYPING, ConversationMemberUpdateData} from '@wireapp/api-client/lib/conversation/data';
import {
Expand Down Expand Up @@ -536,7 +537,12 @@
* @param userId - qualified user id
*/
async getMLS1to1Conversation(userId: QualifiedId) {
return this.apiClient.api.conversation.getMLS1to1Conversation(userId);
const response = await this.apiClient.api.conversation.getMLS1to1Conversation(userId);

if (isMLS1to1Conversation(response)) {
return response;
}
return {conversation: response};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we return response.conversation and response? 🤔
Code will be more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And nice to have passed type for fn what we returning there ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we return response.conversation and response? 🤔 Code will be more readable.

This function returns

export interface MLS1to1Conversation {
  conversation: MLSConversation;
  public_keys?: {
    removal: MLSPublicKeyRecord;
  };
}

the optional public_keys is needed in some cases so we can't only return MLSConversation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@przemvs see if you like @e-maad 's suggestion I applied

V-Gira marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -558,7 +564,8 @@
// Before trying to register a group, check if the group is already established o backend.
// If remote epoch is higher than 0, it means that the group was already established.
// It's possible that we've already received a welcome message.
const mlsConversation = await this.getMLS1to1Conversation(otherUserId);

const {conversation: mlsConversation, public_keys} = await this.getMLS1to1Conversation(otherUserId);

if (mlsConversation.epoch > 0) {
this.logger.debug(
Expand All @@ -580,19 +587,19 @@
);

await this.joinByExternalCommit(mlsConversation.qualified_id);
return this.getMLS1to1Conversation(otherUserId);
return (await this.getMLS1to1Conversation(otherUserId)).conversation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go with smth like this? Code will look more readable :)

Suggested change
return (await this.getMLS1to1Conversation(otherUserId)).conversation;
const updatedMLSConversation = await this.getMLS1to1Conversation(otherUserId);
return updatedMLSConversation.conversation;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this, WDYT?

      const {conversation: updatedMLSConversation} = await this.getMLS1to1Conversation(otherUserId);
      return updatedMLSConversation;

}

// If group is not established on backend,
// we wipe the it locally (in case it exsits in the local store) and try to register it.
await this.mlsService.wipeConversation(groupId);

try {
await this.mlsService.register1to1Conversation(groupId, otherUserId, selfUser);
await this.mlsService.register1to1Conversation(groupId, otherUserId, selfUser, public_keys?.removal);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change, we can register a conversation with a removal key that comes from a different endpoint


this.logger.info(`Conversation (id ${mlsConversation.qualified_id.id}) established successfully.`);

return this.getMLS1to1Conversation(otherUserId);
return (await this.getMLS1to1Conversation(otherUserId)).conversation;
} catch (error) {
if (!shouldRetry) {
this.logger.error(`Could not register MLS group with id ${groupId}: `, error);
Expand Down Expand Up @@ -667,7 +674,7 @@
throw new Error('Qualified conversation id is missing in the event');
}

queueConversationRejoin(conversationId.id, () =>

Check warning on line 677 in packages/core/src/conversation/ConversationService/ConversationService.ts

View workflow job for this annotation

GitHub Actions / lint

Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator
this.recoverMLSGroupFromEpochMismatch(conversationId, subconv),
);
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,46 +151,6 @@ export class SubconversationService extends TypedEventEmitter<Events> {
await this.clearSubconversationGroupId(conversationId, SUBCONVERSATION_ID.CONFERENCE);
}

/**
* Short term solution to an issues with 1:1 MLS call, see
* https://wearezeta.atlassian.net/wiki/spaces/PAD/pages/1314750477/2024-07-29+1+1+calls+over+SFT#Do-not-leave-the-subconversation
* Will delete a conference subconversation when hanging up on a 1:1 call instead of leaving.
*
* @param conversationId Id of the parent conversation which subconversation we want to leave
*/
public async leave1on1ConferenceSubconversation(conversationId: QualifiedId): Promise<void> {
const subconversationGroupId = await this.getSubconversationGroupId(conversationId, SUBCONVERSATION_ID.CONFERENCE);

if (!subconversationGroupId) {
return;
}

const doesGroupExistLocally = await this.mlsService.conversationExists(subconversationGroupId);
if (!doesGroupExistLocally) {
// If the subconversation was known by a client but is does not exist locally, we can remove it from the store.
return this.clearSubconversationGroupId(conversationId, SUBCONVERSATION_ID.CONFERENCE);
}

try {
const epochInfo = await this.getSubconversationEpochInfo(conversationId, subconversationGroupId);

if (!epochInfo) {
throw new Error('Failed to get epoch info for conference subconversation');
}
await this.apiClient.api.conversation.deleteSubconversation(conversationId, SUBCONVERSATION_ID.CONFERENCE, {
groupId: subconversationGroupId,
epoch: epochInfo.epoch,
});
} catch (error) {
this.logger.error(`Failed to delete conference subconversation:`, error);
}

await this.mlsService.wipeConversation(subconversationGroupId);

// once we've deleted the subconversation, we can remove it from the store
await this.clearSubconversationGroupId(conversationId, SUBCONVERSATION_ID.CONFERENCE);
}

Comment on lines -154 to -193
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block was a previous attempt to solve a related issue without requiring a change to the backend response, by calling the deleteSubconversation endpoint instead of deleteSubconversationSelf.

see #6434

This is no longer required

public async leaveStaleConferenceSubconversations(): Promise<void> {
const conversationIds = await this.getAllGroupIdsBySubconversationId(SUBCONVERSATION_ID.CONFERENCE);

Expand Down
14 changes: 10 additions & 4 deletions packages/core/src/messagingProtocols/mls/MLSService/MLSService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*
*/

import type {ClaimedKeyPackages, RegisteredClient} from '@wireapp/api-client/lib/client';
import type {ClaimedKeyPackages, MLSPublicKeyRecord, RegisteredClient} from '@wireapp/api-client/lib/client';
import {PostMlsMessageResponse, SUBCONVERSATION_ID} from '@wireapp/api-client/lib/conversation';
import {ConversationMLSMessageAddEvent, ConversationMLSWelcomeEvent} from '@wireapp/api-client/lib/event';
import {BackendError, StatusCode} from '@wireapp/api-client/lib/http';
Expand Down Expand Up @@ -471,7 +471,11 @@ export class MLSService extends TypedEventEmitter<Events> {
* @param groupId the id of the group to create inside of coreCrypto
* @param parentGroupId in case the conversation is a subconversation, the id of the parent conversation
*/
public async registerEmptyConversation(groupId: string, parentGroupId?: string): Promise<void> {
public async registerEmptyConversation(
groupId: string,
parentGroupId?: string,
removalKeyFor1to1Signature?: MLSPublicKeyRecord,
): Promise<void> {
const groupIdBytes = Decoder.fromBase64(groupId).asBytes;

let externalSenders: Uint8Array[] = [];
Expand All @@ -481,7 +485,8 @@ export class MLSService extends TypedEventEmitter<Events> {
} else {
const mlsKeys = (await this.apiClient.api.client.getPublicKeys()).removal;
const ciphersuiteSignature = getSignatureAlgorithmForCiphersuite(this.config.defaultCiphersuite);
const removalKeyForSignature = mlsKeys[ciphersuiteSignature];
const removalKeyForSignature =
removalKeyFor1to1Signature?.[ciphersuiteSignature] ?? mlsKeys[ciphersuiteSignature];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the removal key is provided by the conversation endpoint, we use that one, else, we use a removal key provided by getPublicKey (our own backend removal key)

if (!removalKeyForSignature) {
throw new Error(
`Cannot create conversation: No backend removal key found for the signature ${ciphersuiteSignature}`,
Expand Down Expand Up @@ -559,9 +564,10 @@ export class MLSService extends TypedEventEmitter<Events> {
groupId: string,
userId: QualifiedId,
selfUser: {user: QualifiedId; client: string},
removalKeyFor1to1Signature?: MLSPublicKeyRecord,
): Promise<PostMlsMessageResponse & {failures: AddUsersFailure[]}> {
try {
await this.registerEmptyConversation(groupId);
await this.registerEmptyConversation(groupId, undefined, removalKeyFor1to1Signature);

// We fist fetch key packages for the user we want to add
const {keyPackages: otherUserKeyPackages, failures: otherUserKeysClaimingFailures} =
Expand Down
Loading