Page MenuHomePhabricator

[lib] Broadcast device list update during primary logout
ClosedPublic

Authored by bartek on Fri, Jun 28, 2:18 AM.
Tags
None
Referenced Files
F2189113: D12605.id41782.diff
Thu, Jul 4, 10:10 AM
F2179684: D12605.id41781.diff
Wed, Jul 3, 1:40 PM
Unknown Object (File)
Tue, Jul 2, 8:43 AM
Unknown Object (File)
Tue, Jul 2, 7:48 AM
Unknown Object (File)
Mon, Jul 1, 11:09 PM
Unknown Object (File)
Fri, Jun 28, 6:04 AM
Unknown Object (File)
Fri, Jun 28, 6:04 AM
Unknown Object (File)
Fri, Jun 28, 6:04 AM
Subscribers

Details

Summary

Calling LogOutPrimaryDevice RPC performs device list update on Identity. After this is done (but before local Redux and DB is cleared) we need to broadcast new device list to peers.

Depends on D12604

Test Plan

Tested it on natvie and web - the update is broadcasted despite being already logged out

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.

Did you forget to submit this one?

lib/actions/user-actions.js
265–266

This seems like a risky assumption. How do you know it's safe?

bartek published this revision for review.Tue, Jul 2, 5:57 AM
bartek edited the test plan for this revision. (Show Details)
lib/actions/user-actions.js
265–266

Yes, this is risky. It works because we call await logOut() directly: At this point, we called Identity and Authoritative Keyserver logout endpoints, but locally are still logged in, including having active Tunnelbroker connection.
The whole primaryDeviceLogOut() (this hook) is then called in dispatchActionPromise which clears the CSAT and triggers Tunnelbroker context to reset the connection.

Discussed this with @kamil and we think there's no good way of doing this. The worst-case scenario is that peers won't receive the update and will keep having (now logged-out) secondary devices on their peer lists.
The other alternative is broadcasting the update before calling the RPC. However, peers treat Identity Service as a source of truth _(we agreed to do so)_, so if they process the "device list updated" messages too early, Identity will still have the old device list. It is updated on Identity during the RPC call.

If we wouldn't treat Identity as a source of truth, we could attach the new device list to the update message. But in current state, peers will ignore it anyway and still ask Identity.
If we're really concerned about this, one way is to add a lastDeviceListTimestamp field to the broadcasted "device list updated" P2P message, so peers, when asking Identity, could compare the timestamps and possibly wait-and-retry (with some timeout) fetching it until timestamps match.

kamil added inline comments.
lib/actions/user-actions.js
265–266

Agree with that.

Additionally, another solution is that we can have some mechanism that remembers the last communication with any user device and after some time interval asks the Identity for device list, to heal from the worst-case scenario finally.

This revision is now accepted and ready to land.Wed, Jul 3, 3:33 AM
lib/actions/user-actions.js
265–266

To be clear, is there really any risk here? It sounds like the Redux action won't be dispatched until await broadcastDeviceListUpdates concludes, which means that the Tunnelbroker connection won't be closed until after? Or is there a concern that Tunnelbroker will close the connection once the CSAT is invalidated by identity (during await logOut)?

lib/actions/user-actions.js
265–266

It sounds like the Redux action won't be dispatched until await broadcastDeviceListUpdates concludes, which means that the Tunnelbroker connection won't be closed until after?

Yes, exactly!

Or is there a concern that Tunnelbroker will close the connection once the CSAT is invalidated by identity (during await logOut)?

No, this is not the case here. The only case when Tunnelbroker will drop the connection is Primary device Backup Restore (TB drops connection with the old primary device to let the new primary device connect).

To be clear, is there really any risk here?

Looks like no risks are coming from our business logic. The only risk appears to be if the Tunnelbroker connection is closed due to network issues. But this one is already described in ENG-8380.