[lib] create hook to run Device List Update Protocol
Needs ReviewPublic
Actions

Authored by kamil on Tue, Nov 19, 2:25 AM.

Details

Reviewers

bartek
tomek
varun

Summary

ENG-9905.

Currently, updating the device list and broadcasting device list update is easily coupled in the codebase, which makes it hard to maintain, as those two should always be done together. I just wanted to create one call that matches 5.3 from the whitepaper:

This is not fully correct yet, we need to change to order to first send the device list update to all the peers, but it's not yet possible before ENG-9769. Creating ENG-9948 to address that.

I am still keeping util functions, bad for readability but those are used in unit tests.

Logic should remain mostly unchanged, small changes are annotated with inline comments.

NOTE: It's not that easy to slice this into diffs, so it's possible that the middle of the stack protocol is not working, it needs to be reviewed as a whole.

Test Plan

Tested later in the stack but:

Log in secondary device
Log out secondary device
Remove device from Linked Devices list

And making sure that device list updates are disseminated

Diff Detail

Repository

rCOMM Comm

Branch

secondary-auth

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Tue, Nov 19, 2:25 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptTue, Nov 19, 2:25 AM

kamil edited the summary of this revision. (Show Details)Tue, Nov 19, 2:28 AM

kamil added 1 blocking reviewer(s): bartek.

minor update

Harbormaster completed remote builds in B32705: Diff 45871.Tue, Nov 19, 2:42 AM

kamil edited the summary of this revision. (Show Details)Tue, Nov 19, 2:45 AM

Harbormaster completed remote builds in B32706: Diff 45872.Tue, Nov 19, 2:49 AM

kamil added inline comments.Tue, Nov 19, 2:55 AM

lib/tunnelbroker/use-peer-to-peer-message-handler.js
115 ↗	(On Diff #45872)	We no longer use `userID` from the message but from `getAuthMetadata` - as we should log out only secondary device that belong to us (cc. @bartek)
native/profile/secondary-device-qr-code-scanner.react.js
140–142 ↗	(On Diff #45872)	Previously, we were downloading the device list history again because the secondary device log-in had multiple steps, and an update was sent after receiving success from the secondary, this is not needed anymore: In `useDeviceListUpdate` we use the device list signed by the primary right before. Also, this is going to be removed anyway, we need only a timestamp. See ENG-9769.

kamil added a child revision: D13964: [lib] temporary remove code performing backup restore.Tue, Nov 19, 2:55 AM

kamil attached a referenced file: F3307524: image.png. (Show Details)Tue, Nov 19, 2:55 AM

kamil published this revision for review.Tue, Nov 19, 4:12 AM

ashoat added inline comments.Tue, Nov 19, 5:44 AM

lib/shared/device-list-utils.js
355 ↗	(On Diff #45872)	For the other update types, we call `sendDeviceListUpdates`, which also calls `getAndUpdateDeviceListsForUsers`. The comment there says it's only for fetching platform details, but I'm wondering – do we need to do anything else to update `auxUserStore`?
355–357 ↗	(On Diff #45872)	For the other update types, we filter out the current device (primary device). Do we need to do that here too?
native/profile/secondary-device-qr-code-scanner.react.js
218–222 ↗	(On Diff #45872)	It looks like this call and the one before were not previously broadcasting the device list update, but now they are. Can you explain the context of this change?

kamil added inline comments.Tue, Nov 19, 6:44 AM

lib/shared/device-list-utils.js
355 ↗	(On Diff #45872)	It's not meant to say it's only for fetching platform details. It explains why we need to use `getAndUpdateDeviceListsForUsers` and make an Idenitiy call and not just update the `auxUserStore` directly by dispatching action - this code is running on primary so we have the newest device list (this is the source of truth). But we do this to fetch platform details.
355–357 ↗	(On Diff #45872)	Good questions. For removing peers, we don't filter and send device list updates to ourselves too, and later as a response to receiving device list update we call `getAndUpdateDeviceListsForUsers`. Looks like for adding devices/replacing devices we Send updates to all peers but not to ourselves and call `getAndUpdateDeviceListsForUsers` immediately. This was changed in D12430. I think we should do the same for removing to standardize the approach. Going to add this as a follow-up because this diff is already complicated.
native/profile/secondary-device-qr-code-scanner.react.js
218–222 ↗	(On Diff #45872)	They were, but not directly, after receiving a response from the Secondary Device - in the new protocol we don't wait with broadcasting for a Secondary Device. I also explained this in inline comment above: https://phab.comm.dev/D13963#inline-78572

ashoat added inline comments.Tue, Nov 19, 8:28 AM

lib/shared/device-list-utils.js
355 ↗	(On Diff #45872)	It sounds like for device add and replacing, `getAndUpdateDeviceListsForUsers` handles updating `auxUserStore`. For device removal, where do we handle updating `auxUserStore`?
355–357 ↗	(On Diff #45872)	Thanks, I agree it would be good to standardize.
native/profile/secondary-device-qr-code-scanner.react.js
218–222 ↗	(On Diff #45872)	Thanks for explaining!

ashoat added inline comments.Tue, Nov 19, 8:30 AM

lib/shared/device-list-utils.js
355 ↗	(On Diff #45872)	Oh, perhaps this was answered by your response to my other question: we send device list updates to ourselves, which then handle updating `auxUserStore`. And this is something you're looking into changing to standardize (with add / replace actions) in a follow-up diff. Let me know if that's right!