[keyserver] finish secondary login via qr code
Needs RevisionPublic
Actions

Authored by varun on May 24 2024, 9:02 AM.

Details

Reviewers

bartek
kamil
ashoat

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

No Lint Coverage

Unit

No Test Coverage

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	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	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	Given how much longer this got, it seems like it might be a good idea to factor it out into a function
93	We should definitely invert this condition to reduce indentation
96	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	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	I would just `socket.send` directly in each branch and skip the `initMessageString` variable
173	Nit
384–387	Should we wrap this with a `try-catch`?
388	I don't think this `Promise.resolve` is necessary given you declared this as an `async` function
keyserver/src/socket/tunnelbroker.js
45	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`