search

deltachat / deltachat-core-rust

Delta Chat Rust Core library, used by Android/iOS/desktop apps, bindings and bots 📧
https://delta.chat/en/contribute
Other
637 stars 82 forks source link

Avoid adding members twice via securejoin #5356

Open link2xt opened 4 months ago

link2xt commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.