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

fix(mls-client): skip MLSClient registration if it has registered #2983

Conversation

mchenani
Copy link
Contributor

@mchenani mchenani commented Sep 2, 2024


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Check if the MLS Client has registered, then skip the registration.

Issues

When the user logs out without clearing the data, we were trying to register the exist MLSClient again; that lead to an error!

Solutions

If the client has registered before and the user log in again to continue on a previously established session no need to register the client again and simply skip the api call.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

Have a user with MLS Enabled on their team
1- Log in and log out without clearing the data
2- When you try to log in again with the same use, you should be able to proceed successfully


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Copy link

sonarqubecloud bot commented Sep 2, 2024

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Test Results

3 064 tests   2 957 ✔️  3m 24s ⏱️
   524 suites     107 💤
   524 files           0

Results for commit 443d746.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

Datadog Report

All test runs 77d96f3 🔗

2 Total Test Services: 0 Failed, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Test Service View
kalium-ios 0 0 0 2806 123 1.33s Link
kalium-jvm 0 0 0 2957 107 6.05s Link

@@ -214,13 +215,16 @@ class ClientDataSource(
clientId: ClientId,
publicKey: ByteArray,
cipherSuite: CipherSuite
): Either<CoreFailure, Unit> =
): Either<CoreFailure, Unit> = if (clientRegistrationStorage.hasRegisteredMLSClient()) {
Copy link
Member

@MohamadJaara MohamadJaara Sep 2, 2024

Choose a reason for hiding this comment

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

does clientRegistrationStorage.hasRegisteredMLSClient take into account if the registered client still exists remotely, meaning if my app session expired and during that time the client was removed remotely, will this function still return true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's another edge case, if the use was logged out without clearing the data, and delete the client from another device and afterwards tries to login on this device we can't detect it.
What we can do:
Currently if the client is registered, we get this error from the server API:
InvalidRequestError(errorResponse=ErrorResponse(code=400, message=MLS public key for the given signature scheme already exists, label=mls-duplicate-public-key))
We can instead of checking the local flag always try to register the client against the server, if we get that error then we ignore it.

Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

For Proteus, we already check if the local client still exists remotely or not, I suggest MLS clients follow the same path as Proteus

@mchenani mchenani disabled auto-merge September 2, 2024 08:17
@mchenani
Copy link
Contributor Author

mchenani commented Sep 3, 2024

Closing this: decided to go with another solution #2987

@mchenani mchenani closed this Sep 3, 2024
@vitorhugods vitorhugods deleted the fix/skip-mls-client-registeration-if-has-registered branch January 29, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants