Page MenuHomePhabricator

[keyserver] finish secondary login via qr code
Needs RevisionPublic

Authored by varun on Fri, May 24, 9:02 AM.
Referenced Files
F2034443: D12213.id40652.diff
Tue, Jun 18, 6:44 AM
Unknown Object (File)
Tue, Jun 11, 10:05 AM
Unknown Object (File)
Tue, Jun 11, 10:05 AM
Unknown Object (File)
Thu, Jun 6, 6:25 PM
Unknown Object (File)
Thu, Jun 6, 5:58 PM
Unknown Object (File)
Thu, Jun 6, 2:31 AM
Unknown Object (File)
Wed, Jun 5, 10:37 PM
Unknown Object (File)
Tue, Jun 4, 7:03 PM



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

rCOMM Comm
No Lint Coverage
No Test Coverage

Event Timeline

varun requested review of this revision.Fri, May 24, 9:18 AM
ashoat requested changes to this revision.Sat, May 25, 11:52 AM

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


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


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?


Given how much longer this got, it seems like it might be a good idea to factor it out into a function


We should definitely invert this condition to reduce indentation


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?


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"?


I would just socket.send directly in each branch and skip the initMessageString variable




Should we wrap this with a try-catch?


I don't think this Promise.resolve is necessary given you declared this as an async function


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.Sat, May 25, 11:52 AM

In D12327 I've added a way to skip fetching backup keys