Page MenuHomePhabricator

[web] Update `getSignedIdentityKeysBlobSelector` to `await initOlm();`
ClosedPublic

Authored by atul on Mar 15 2023, 1:10 PM.
Tags
None
Referenced Files
F1863423: D7081.diff
Sun, May 26, 6:00 AM
Unknown Object (File)
Sat, May 25, 10:59 AM
Unknown Object (File)
Sat, May 25, 10:58 AM
Unknown Object (File)
Sat, May 25, 10:58 AM
Unknown Object (File)
Fri, May 10, 7:47 AM
Unknown Object (File)
Sun, May 5, 9:22 AM
Unknown Object (File)
Thu, May 2, 11:24 PM
Unknown Object (File)
Thu, May 2, 4:15 AM
Subscribers

Details

Summary

Once I added cryptoStore to the redux-persist "whitelist," I noticed that refreshing the page caused an error. The error had to do with olm.Account() not being a constructor. After looking into it, I realized that the issue was that we weren't awaiting initOlm in getSignedIdentityKeysBlobSelector before trying to use olm functions.

Why wasn't this an issue before?

Previously, the only places on web where we were using olm were LoginForm, TraditionalLoginForm, and SIWELoginForm.

In all of those places we made sure that await initOlm had completed prior to using any olm functionality (either explicitly in LoginForm, or implicitly in the child TraditionalLoginForm and SIWELoginForm components) so this was never an issue.

I failed to ensure that we were handling await initOlm() when writing getSignedIdentityKeysBlobSelector. This caused a crash on refresh because the olm codepaths in getSignedIdentityKeysBlobSelector get "hit" immediately as part of Socket "rendering."

The reason I wasn't catching this previously is
A. I either had OLM initialized after authing in... so things worked.
B. primaryAccount/primaryIdentityKeys/notificationIdentityKeys were unset because they weren't persisted, and getSignedIdentityKeysBlobSelector early returned:

1628a9-1.png (134×1 px, 21 KB)

... so things worked.

Once I added cryptoStore to redux-persist, I ran into the situation where
A. OLM hadn't been initialized.
B. getSignedIdentityKeysBlobSelector was getting past the early return case... and the new olm.Account() call failed.


In this diff I add await initOlm() to getSignedIdentityKeysBlobSelector and make the necessary modifications in TraditionalLoginForm and SIWELoginForm to accomodate the changes.

Test Plan
  1. Made sure that I could log in via TraditionalLoginForm and set breakpoints on the keyserver side to ensure we got what we expect for signedIdentityKeysBlob
  2. Made sure that I could log in via SIWELoginForm and set breakpoints on the keyserver side to ensure we got what we expect for signedIdentityKeysBlob
  3. I patched in the commit where I add cryptoStore to the redux-persist "whitelist" and ensure that things are as expected when I refresh the page.
  4. I remove signedIdentityKeysBlob from the signed_identity_keys cookies table and test that it get repopulated via SIGNED_IDENTITY_KEYS_BLOB request by:

A. Killing keyserver and restarting it, checking that web sends back response with signedIdentityKeysBlob and DB gets updated
B. Refreshing weband checking that keyserver sent SIGNED_IDENTITY_KEYS_BLOB request and web responded with correct signedIdentityKeysBlob response and DB got updated

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Mar 15 2023, 1:10 PM
atul edited the test plan for this revision. (Show Details)
ashoat added inline comments.
web/account/siwe-login-form.react.js
84–100 ↗(On Diff #23769)

This could be factored out as a hook, eg. useSignedIdentityKeysBlob. Can you do that before landing?

This revision is now accepted and ready to land.Mar 15 2023, 1:51 PM

extract useSignedIdentityKeysBlob hook to cut repetition

This revision was landed with ongoing or failed builds.Mar 15 2023, 3:48 PM
This revision was automatically updated to reflect the committed changes.