Page MenuHomePhabricator

[keyserver] finish secondary login via qr code
Needs RevisionPublic

Authored by varun on Fri, May 24, 9:02 AM.
Tags
None
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
Subscribers

Details

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

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

This revision now requires changes to proceed.Sat, May 25, 11:52 AM
keyserver/src/socket/tunnelbroker-socket.js
178–180

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