Details

Reviewers

bartek
kamil
ashoat

Commits

rCOMMc7755c1d0935: [keyserver] finish secondary login via qr code

Summary

after successfully uploading keys and logging in to the identity service, we should mark the prekeys as published and save the access token and user ID returned by the identity service. then, we should close the websocket connection. when it is reopened by the onClose function, it should know that after authenticating with tunnelbroker, it needs to send a SECONDARY_DEVICE_REGISTRATION_SUCCESS message to the primary device.

Depends on D12212

Test Plan

successfully added my local keyserver as a secondary device by scanning the qr code from my physical device. verified that the device list was updated in ddb. also received an alert on primary confirming secondary login.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.May 24 2024, 9:02 AM

Herald added a subscriber: tomek. · View Herald TranscriptMay 24 2024, 9:02 AM

Harbormaster completed remote builds in B29243: Diff 40643.May 24 2024, 9:18 AM

varun requested review of this revision.May 24 2024, 9:18 AM

rebase

Harbormaster completed remote builds in B29246: Diff 40652.May 24 2024, 9:55 AM

Love that you got this working!! Apologies in advance for all the comments

keyserver/src/socket/tunnelbroker-socket.js
68 ↗	(On Diff #40652)	I think it's confusing that the constructor gets a param with this name, but it's not assigned here After reading the logic it makes sense, but I think things might be clearer if you picked a different name here, eg. `shouldNotifyPrimaryAfterReopening`
72–79 ↗	(On Diff #40652)	We have a lot of params here now. Can you refactor this to take a single object, so that the params are named at the callsite?
92–118 ↗	(On Diff #40652)	Given how much longer this got, it seems like it might be a good idea to factor it out into a function
93 ↗	(On Diff #40652)	We should definitely invert this condition to reduce indentation
96 ↗	(On Diff #40652)	Given that we fetch these together in `tunnelbroker.js`, and given that we check them together here and on line 166, should we just pass `identityInfo` in wholesale to `TunnelbrokerSocket` instead of passing these in individually?
97 ↗	(On Diff #40652)	Do we really need these logs given we already have some on line 165? If you definitely want to keep it, can you capitalize "Tunnelbroker"?
116 ↗	(On Diff #40652)	I would just `socket.send` directly in each branch and skip the `initMessageString` variable
173 ↗	(On Diff #40652)	Nit
384–387 ↗	(On Diff #40652)	Should we wrap this with a `try-catch`?
388 ↗	(On Diff #40652)	I don't think this `Promise.resolve` is necessary given you declared this as an `async` function
keyserver/src/socket/tunnelbroker.js
45 ↗	(On Diff #40652)	The recursive structure here is neat, but is the indirection really worth it? We have similar logic in the client class that maintains a WebSocket connection with the keyserver (`lib/socket/socket.react.js`), and in that case we don't reinstantiate the class when the `WebSocket` gets disconnected. I wonder if the logic would be more clear if we merge `tunnelbroker.js` and `tunnelbroker-socket.js`

This revision now requires changes to proceed.May 25 2024, 11:52 AM

bartek mentioned this in D12327: [lib][native] Send backup keys in QR auth only if requested.Jun 6 2024, 2:01 AM

bartek added inline comments.Jun 6 2024, 2:03 AM

keyserver/src/socket/tunnelbroker-socket.js
178–180 ↗	(On Diff #40652)	In D12327 I've added a way to skip fetching backup keys

bartek mentioned this in rCOMMc25b3d672f2a: [lib][native] Send backup keys in QR auth only if requested.Jun 17 2024, 2:54 AM

bartek mentioned this in rCOMMe9e330a65dd1: [lib][native] Send backup keys in QR auth only if requested.Jun 17 2024, 7:32 AM

address feedback

Harbormaster completed remote builds in B29896: Diff 41641.Jun 24 2024, 10:25 AM

bartek added inline comments.Jun 26 2024, 2:21 AM

keyserver/src/socket/tunnelbroker.js
131–132 ↗	(On Diff #41641)	I don't understand what's the difference between these two
234–236 ↗	(On Diff #41641)	Is `shouldNotifyPrimary` reset anywhere after actually notifying primary device?

varun added inline comments.Jun 26 2024, 11:38 AM

keyserver/src/socket/tunnelbroker.js
131–132 ↗	(On Diff #41641)	`shouldNotifyPrimaryAfterReopening` is set to true only after `uploadSecondaryDeviceKeysAndLogIn` succeeds. it means that when the anonymous connection is closed and the authenticated connection is opened, it should send a `SECONDARY_DEVICE_REGISTRATION_SUCCESS` message to the primary device. `shouldNotifyPrimary` is only set to true (line 149) if `shouldNotifyPrimaryAfterReopening` in the constructor params is true
234–236 ↗	(On Diff #41641)	i don't think it needs to be reset. we set `this.connected` to true on line 228, so if we receive another `CONNECTION_INITIALIZATION_RESPONSE` for some reason, we'll go to line 255

think i broke something in the latest revision

fix onOpen

varun added inline comments.Jun 27 2024, 11:59 AM

keyserver/src/socket/tunnelbroker.js
156 ↗	(On Diff #41763)	need to use a closure here because `ws$OpenListener` doesn't take any params: declare type ws$OpenListener = () => mixed;

Harbormaster completed remote builds in B29964: Diff 41763.Jun 27 2024, 12:14 PM

varun added a child revision: D12618: [keyserver] don't display the QR code if keyserver is already logged in.Jun 28 2024, 12:53 PM

Unfortunately, my comment about merging these two files was not closely read. As a result it appears that we've taken a step backward rather than simplifying this logic.

First, let me copy-paste the comment from earlier:

The recursive structure here is neat, but is the indirection really worth it? We have similar logic in the client class that maintains a WebSocket connection with the keyserver (lib/socket/socket.react.js), and in that case we don't reinstantiate the class when the WebSocket gets disconnected. I wonder if the logic would be more clear if we merge tunnelbroker.js and tunnelbroker-socket.js

My suggestion here was to remove the recursive structure, and to stop reinstantiating this class every time the WebSocket gets disconnected.

Instead, @varun appears to have naively just combined the two files into one. This does nothing to simplify the logic, and instead just makes for one huge confusing file...

Passing back to @varun with a question: are you able to make the changes originally requested? How long do you think it will take you?

This revision now requires changes to proceed.Jul 1 2024, 12:29 PM

Passing back to @varun with a question: are you able to make the changes originally requested? How long do you think it will take you?

Spoke to @varun about this in our 1:1 and we decided it didn't make sense to make these changes in this diff... it's already complicated enough as-is, and we should probably just land it soon to fix up secondary device login.

I still think it would be good to get rid of the recursive logic here. My assessment is that it would take maybe 1 day or so. It's probably not something we can prioritize this month (or probably even next) given our launch timeline, but @varun will create a follow-up task before landing.

This revision is now accepted and ready to land.Jul 3 2024, 9:43 AM

https://linear.app/comm/issue/ENG-8813/remove-recursive-logic-from-tunnelbroker-websocket-code-in-keyserver

Closed by commit rCOMMc7755c1d0935: [keyserver] finish secondary login via qr code (authored by varun). · Explain WhyJul 12 2024, 12:18 AM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMMc7755c1d0935: [keyserver] finish secondary login via qr code.

[keyserver] finish secondary login via qr code
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42250

keyserver/src/keyserver.js

keyserver/src/socket/tunnelbroker-socket.js

keyserver/src/socket/tunnelbroker.js

keyserver/src/utils/olm-utils.js

[keyserver] finish secondary login via qr codeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42250

keyserver/src/keyserver.js

keyserver/src/socket/tunnelbroker-socket.js

keyserver/src/socket/tunnelbroker.js

keyserver/src/utils/olm-utils.js

[keyserver] finish secondary login via qr code
ClosedPublic
Actions

Revision Contents
Changeset List