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

Avoid adding members twice via securejoin #5356

Open
link2xt opened this issue Mar 18, 2024 · 2 comments
Open

Avoid adding members twice via securejoin #5356

link2xt opened this issue Mar 18, 2024 · 2 comments

Comments

@link2xt
Copy link
Collaborator

link2xt commented Mar 18, 2024

Since we have started synchronizing QR codes to other devices, this situation frequently happens:

  1. Alice1 creates a QR code for the group.
  2. QR code gets synchronized to device Alice2
  3. Alice2 goes offline.
  4. Bob scans QR code
  5. Alice1 completes securejoin protocol and adds Bob to the group.
  6. Some time later Alice2 goes online completes securejoin protocol and adds Bob to the group second time.

This results in a second "Member added" message in a group. Even worse scenario is if Alice2 goes online way later when Bob already left the group or was removed from it.

One solution discussed is to store securejoin messages in a separate table similar to how we handle webxdc updates and only "flush" it into outgoing SMTP queue at the end of message fetch, so Alice2 can notice that Alice1 has already added Bob and drop pending securejoin messages.

There is however a simpler solution that does not require separate state for pending securejoin messages. We can generate Message-ID for vg-member-added by hashing In-Reply-To, i.e. the Message-ID of vg-request-with-auth message and AUTH secret. This way both Alice1 and Alice2 will generate the same Message-ID for vg-member-added and the second vg-member-added will be ignored by recipients.

There are drawbacks to the Message-ID solution. First problem is additional traffic generated by Alice2. Also second message may still be processed if some device has not received first "Member added" message e.g. because they joined the group later than Bob they will still process second "Member added" message which might come way later. This can however be solved by not sending the message from Alice2 at all if it finds out that it generated the same Message-ID that it already has in the imap table from the "prefetch" step. Prefetch is guaranteed to be complete by the time Alice2 gets to the step of fetching securejoin messages.

@iequidoo
Copy link
Collaborator

I think the Message-ID solution can be even better because it resolves races when both Alices generate vg-member-added in parallel but one of these messages is delayed because of network problems or one of the Alice's devices being offline.

@hpk42
Copy link
Contributor

hpk42 commented Mar 20, 2024

There are drawbacks to the Message-ID solution. First problem is additional traffic generated by Alice2.

There is a single member-added message that might get sent extra NUM_DEVICES-1 times -- and membership changes are relatively rare (compared to regular chat messages) so not a big enough deal to tilt the balance to adding extra state and more involved code and tests to core. We could consider btw to remove securejoin request messages from IMAP folder after we handled them so when the second device comes online it will never see them, only the eventual BCCed member-added message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants