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

V-Gira · 2024-09-03T16:42:12Z

Description

See https://wearezeta.atlassian.net/browse/WPB-10737 for a more in depth description of the issue.

In a nutshell, it's possible for a federated user to create a 1:1 conversation even if it's not hosted on their own backend, in which case it can introduce a host of issues because they're lacking the correct removal key.

to address that issue, the response from the endpoint GET /conversations/one2one/{domain}/{userId} was changed to include that removal key, from

{ <conversation metadata> }

to

{
  "conversation": { <conversation metadata> },
  "public_keys": {
    "removal": {
      "<sig-algo>": "string",
      ...
    }
  }
}

Checklist

mentions the JIRA issue in the PR name (Ex. WPB-423)
PR has been self reviewed by the author;
Hard-to-understand areas of the code have been commented;
If it is a core feature, unit tests have been added;

V-Gira · 2024-09-11T14:11:28Z

packages/core/src/conversation/SubconversationService/SubconversationService.ts

-  /**
-   * 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);
-  }
-


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

packages/core/src/messagingProtocols/mls/MLSService/MLSService.ts

…ion in core

This reverts commit 1fb8cd3.

This reverts commit 9a57dc2.

This reverts commit ac6fa02.

V-Gira · 2024-09-13T12:43:20Z

packages/core/src/conversation/ConversationService/ConversationService.ts

    }

    // 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);


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

V-Gira · 2024-09-13T12:45:40Z

packages/core/src/messagingProtocols/mls/MLSService/MLSService.ts

@@ -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];


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)

packages/api-client/src/conversation/Conversation.ts

przemvs · 2024-09-13T13:03:27Z

packages/core/src/conversation/ConversationService/ConversationService.ts

+    if (isMLS1to1Conversation(response)) {
+      return response;
+    }
+    return {conversation: response};


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

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

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

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

Co-authored-by: Przemysław Jóźwik <[email protected]>

e-maad

Added few suggestions.

packages/api-client/src/conversation/Conversation.ts

e-maad · 2024-09-13T13:09:56Z

packages/api-client/src/conversation/Conversation.ts

+  if (typeof response === 'object' && response !== null) {
+    const conversation = response as MLS1to1Conversation;
+
+    if (!!conversation.conversation && !!conversation.public_keys) {
+      return true;
+    }
+
+    return false;
+  }
+
+  return false;


It is possible to normalize the logic?
Can we return early with false value?

Something like this?

export function isMLS1to1Conversation(response: unknown): response is MLS1to1Conversation { if (typeof response === 'object' && response !== null && 'conversation' in response && 'public_keys' in response) { return true; } return false; }

Yes, but I think in is an alternative for hasOwnProperty().
If you are sure that these property cannot contain null/undefined, then we are good to go.

// for example const response = {conversation: null, public_keys: undefined}; // 'conversation' in response will be true // 'public_keys' in response will be true

packages/core/src/conversation/ConversationService/ConversationService.ts

…nService.ts Co-authored-by: Immad Abdul Jabbar <[email protected]>

przemvs · 2024-09-16T12:14:36Z

packages/core/src/conversation/ConversationService/ConversationService.ts

@@ -580,19 +587,19 @@ export class ConversationService extends TypedEventEmitter<Events> {
      );

      await this.joinByExternalCommit(mlsConversation.qualified_id);
-      return this.getMLS1to1Conversation(otherUserId);
+      return (await this.getMLS1to1Conversation(otherUserId)).conversation;


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;

I went with this, WDYT?

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

sonarqubecloud · 2024-09-16T12:27:28Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

przemvs

Nice!

#6502) * feat: establish 1on1 mls conversation with new removal key [10744] * remove federated SFT 1:1 call temporary fix * type data structure for 1:1 MLS conversations * remove public_keys from NewConversation type * use nullish coalescing instead of ternary for removalKey logic * implement typeguard for the specific MLS1to1Conversation response * use union types for api-client getMLS1to1Conversation return value * use type guard and return MLS1to1Conversation for getMLS1to1Conversation in core * address tests * Update packages/api-client/src/conversation/Conversation.ts Co-authored-by: Przemysław Jóźwik <[email protected]> * rename variable in type guard * Update packages/core/src/conversation/ConversationService/ConversationService.ts Co-authored-by: Immad Abdul Jabbar <[email protected]> * address CR issue * simplify typeguard's logic * address CR --------- Co-authored-by: Przemysław Jóźwik <[email protected]> Co-authored-by: Immad Abdul Jabbar <[email protected]>

feat: establish 1on1 mls conversation with new removal key [10744]

1e57b90

otto-the-bot added the type: feature / request ✨ label Sep 3, 2024

V-Gira added 6 commits September 10, 2024 16:39

remove federated SFT 1:1 call temporary fix

39108e7

type data structure for 1:1 MLS conversations

4d19352

address tests

ac6fa02

remove public_keys from NewConversation type

9cfff54

fix tests

9a57dc2

fix tests

1fb8cd3

V-Gira changed the title ~~feat: establish 1on1 mls conversation with new removal key [10744]~~ feat!: establish 1on1 mls conversation with new removal key [10744] Sep 11, 2024

V-Gira changed the title ~~feat!: establish 1on1 mls conversation with new removal key [10744]~~ feat!: establish 1on1 mls conversation with new removal key [WPB-10744] Sep 11, 2024

echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Sep 11, 2024

V-Gira commented Sep 11, 2024

View reviewed changes

V-Gira marked this pull request as ready for review September 11, 2024 14:12

V-Gira added the do not merge label Sep 11, 2024

przemvs reviewed Sep 12, 2024

View reviewed changes

packages/core/src/messagingProtocols/mls/MLSService/MLSService.ts Outdated Show resolved Hide resolved

use nullish coalescing instead of ternary for removalKey logic

5ab1dec

otto-the-bot removed the type: feature / request ✨ label Sep 12, 2024

V-Gira added 7 commits September 13, 2024 14:18

implement typeguard for the specific MLS1to1Conversation response

effb545

use union types for api-client getMLS1to1Conversation return value

bec6335

use type guard and return MLS1to1Conversation for getMLS1to1Conversat…

9e2816e

…ion in core

Revert "fix tests"

c124b37

This reverts commit 1fb8cd3.

Revert "fix tests"

d7306bb

This reverts commit 9a57dc2.

Revert "address tests"

61e6578

This reverts commit ac6fa02.

address tests

23ee59b

V-Gira commented Sep 13, 2024

View reviewed changes

V-Gira changed the title ~~feat!: establish 1on1 mls conversation with new removal key [WPB-10744]~~ feat: establish 1on1 mls conversation with new removal key [WPB-10744] Sep 13, 2024

przemvs reviewed Sep 13, 2024

View reviewed changes

packages/api-client/src/conversation/Conversation.ts Outdated Show resolved Hide resolved

przemvs reviewed Sep 13, 2024

View reviewed changes

Update packages/api-client/src/conversation/Conversation.ts

b28eca5

Co-authored-by: Przemysław Jóźwik <[email protected]>

otto-the-bot added the type: feature / request ✨ label Sep 13, 2024

V-Gira added 2 commits September 13, 2024 15:06

rename variable in type guard

c012b0f

rename variable in type guard

6e89c7d

e-maad reviewed Sep 13, 2024

View reviewed changes

V-Gira and others added 3 commits September 13, 2024 15:20

Update packages/core/src/conversation/ConversationService/Conversatio…

0119cb2

…nService.ts Co-authored-by: Immad Abdul Jabbar <[email protected]>

address CR issue

ebad5ca

simplify typeguard's logic

a1d04b3

przemvs reviewed Sep 16, 2024

View reviewed changes

address CR

a476b1c

przemvs approved these changes Sep 16, 2024

View reviewed changes

V-Gira removed the do not merge label Sep 16, 2024

V-Gira merged commit 5ff3cf7 into main Sep 16, 2024
9 checks passed

V-Gira deleted the v/establish-mls-1on1 branch September 16, 2024 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

V-Gira commented Sep 3, 2024 •

edited

Loading

V-Gira Sep 11, 2024

V-Gira Sep 13, 2024

V-Gira Sep 13, 2024

przemvs Sep 13, 2024

przemvs Sep 13, 2024

V-Gira Sep 13, 2024

V-Gira Sep 13, 2024

e-maad left a comment

e-maad Sep 13, 2024

V-Gira Sep 13, 2024

e-maad Sep 13, 2024

przemvs Sep 16, 2024

V-Gira Sep 16, 2024

sonarqubecloud bot commented Sep 16, 2024

przemvs left a comment

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

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

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

Conversation

V-Gira commented Sep 3, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-maad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 16, 2024

Quality Gate passed

przemvs left a comment

Choose a reason for hiding this comment

V-Gira commented Sep 3, 2024 •

edited

Loading