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

Authored by bartek on Tue, Jul 2, 5:08 AM.

Details

Reviewers

kamil

Commits

rCOMM2e14a6ae2beb: [lib] Send primary device logout message to secondary devices

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 created this revision.Tue, Jul 2, 5:08 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Jul 2, 5:08 AM

bartek added a child revision: D12639: [lib] Log out secondary devices upon receiving primary logout message.Tue, Jul 2, 5:10 AM

Harbormaster completed remote builds in B30050: Diff 41881.Tue, Jul 2, 5:23 AM

bartek published this revision for review.Tue, Jul 2, 5:52 AM

ashoat added inline comments.Tue, Jul 2, 11:43 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: 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? 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?

bartek added inline comments.Wed, Jul 3, 12:11 AM

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 accepted this revision.Wed, Jul 3, 2:55 AM

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: There is some information that the app tries to send to other peer (P1), e.g. some updates (M1). M1 is encrypted but not queued on Tunnelbroker. This code tries to send a logout message - M2. M2 is encrypted and sent to P1. 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: 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). 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: If we stick to tracking order - this needs to be updated as described above before we ship this. 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.