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

runfix: Correctly check if user has mls device before performing mls operations #17344

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

atomrc
Copy link
Contributor

@atomrc atomrc commented Apr 30, 2024

Description

see https://github.com/wireapp/wire-web-packages/pull/6185/files

Checklist

  • 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;

@@ -443,7 +443,7 @@ export class App {
// We load all the users the self user is connected with
await userRepository.loadUsers(selfUser, connections, conversations, teamMembers);

if (supportsMLS()) {
if (this.core.hasMLSDevice) {
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 we have a MLS device it means that:

  • the env supportsMLS (previous check)
  • the team has MLS enabled
  • the user has a valid mls device

(it a more exhaustive check than the previous supportsMLS check)

Comment on lines 60 to 61
// We need to check if the core service is available and if the e2eIdentity is available
if (!this.core.service?.mls || !this.core.service?.e2eIdentity) {
if (!this.core.hasMLSDevice || !this.core.service?.e2eIdentity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? If so, it doesn't match the code comment anymore right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed the comment needs updating, good catch :)
The fix is needed though (we should never check if mls is enabled by checking that the service is there, anymore)

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha, I guess we're just not spying on that hasMLSDevice in the unit test then

@@ -472,7 +472,7 @@ export class App {

await conversationRepository.init1To1Conversations(connections, conversations);

if (supportsMLS()) {
if (this.core.hasMLSDevice) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is the fix to automation correct?
Seems like the change in MLSConversationVerificationStateHandler isn't necessarry? (that's the one breaking unit tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, I'm working on the fix right now :)

Copy link

sonarcloud bot commented Apr 30, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@atomrc atomrc enabled auto-merge (squash) April 30, 2024 11:56
@atomrc atomrc merged commit 5d36718 into release/q1-2024 Apr 30, 2024
11 checks passed
@atomrc atomrc deleted the fix/mls-enabled branch April 30, 2024 11:57
atomrc added a commit that referenced this pull request Apr 30, 2024
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.

None yet

3 participants