Page MenuHomePhabricator

[lib] Send primary device logout message to secondary devices
ClosedPublic

Authored by bartek on Jul 2 2024, 5:08 AM.
Tags
None
Referenced Files
F3383155: D12638.id42007.diff
Thu, Nov 28, 1:56 PM
Unknown Object (File)
Sun, Nov 24, 10:29 PM
Unknown Object (File)
Sun, Nov 24, 7:30 PM
Unknown Object (File)
Sat, Nov 16, 2:00 PM
Unknown Object (File)
Tue, Nov 12, 4:56 AM
Unknown Object (File)
Tue, Nov 12, 3:53 AM
Unknown Object (File)
Mon, Nov 11, 11:21 PM
Unknown Object (File)
Mon, Nov 11, 5:18 PM
Subscribers

Details

Summary

Primary device, before logging out itself, sends logout request to all secondary devices.

The logic is analogous to useSecondaryDeviceLogOut() hook, but this time recipients are secondary devices,
and sender is the primary device.

Depends on D12637

Test Plan

Logged in on 3 devices:

  • Physical iPhone as primary device
  • Native Simulator as secondary device 1
  • Web as secondary device 2

Pressed the "Primary device logout" dev button on the primary device.
Received console log on both Simulator and web about primary logout (introduced in D12636)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jul 2 2024, 5:52 AM
lib/actions/user-actions.js
303–329 ↗(On Diff #41881)

Looks like this try-catch is mirrored from D12224, where it was added following a request from @kamil to only call createOlmSessionWithPeer if the first attempt fails.

Some questions about the approach:

  1. If we're going to be doing this "try send Tunnelbroker message and if it fails, try initializing Olm session and then retry" approach in multiple places, should we consider factoring it out in some way?
  2. Does it really make sense to do this for any exception? Is there a specific type of error we expect if the session needs to be constructed?
lib/actions/user-actions.js
303–329 ↗(On Diff #41881)

There's some context in https://phab.comm.dev/D12132?id=40656#inline-71925
Looks like it's tracked in ENG-8527 but requires ENG-8333 first.

kamil added inline comments.
lib/actions/user-actions.js
276 ↗(On Diff #41881)

can't we just rely on local data from auxUserStore?

289–292 ↗(On Diff #41881)

Overall this approach is not fully safe and works only because when testing this, this was the only case when we sent encrypted data to another client.

Right now, for decrypting, we use decryptSequentialAndPersist which tracks order of messages, so the following case in future is possible:

  1. There is some information that the app tries to send to other peer (P1), e.g. some updates (M1).
  2. M1 is encrypted but not queued on Tunnelbroker.
  3. This code tries to send a logout message - M2.
  4. M2 is encrypted and sent to P1.
  5. P1 is unable to decrypt M2 because M1 is missing and decryptSequentialAndPersist throws an error because the ordering is not maintained.

To address that, we have PeerToPeerProvider which makes sure that messages are encrypted and sent in order from different callsties, this code should:

  1. create and persist outboundP2PMessages with logout messages (persisting is not needed because we going to delete DB anyway, so we can avoid this but this requires some changes).
  2. call processOutboundMessages() method from PeerToPeerProvider which encrypts and sends them in order.

However, yesterday we had a discussion with @tomek and @ashoat about different approaches, allowing mixing order of messages and making business logic resilient to this.

I think we can unblock this now, but could you create a task to document this somewhere and also make sure @tomek is aware when scoping and:

  1. If we stick to tracking order - this needs to be updated as described above before we ship this.
  2. If we change the approach and allow mixed order - this code is fine.
303–329 ↗(On Diff #41881)

1, 2 - I can handle this when working on a task linked by @bartek.

This revision is now accepted and ready to land.Jul 3 2024, 2:55 AM
lib/actions/user-actions.js
289–292 ↗(On Diff #41881)

Created ENG-8693 to track this and discuss further.

lib/actions/user-actions.js
276 ↗(On Diff #41881)

Looks like this was never responded to – will ping @bartek once he's back

lib/actions/user-actions.js
276 ↗(On Diff #41881)

It's on purpose. We theoretically could, but I consider asking Identity directly as a safer approach. At least until Identity is able to update device lists, there might be a [rare] race condition where device list was updated on Identity, but Redux state haven't been updated yet.
We do the same for other methods that directly update the device list.