Details

Reviewers

kamil
tomek

Commits

rCOMM36cd50031add: [web] Use crypto store and olm from shared worker

Summary

ENG-6656 : Update the web code to use new cryptoStore

Replaces the rest of usages of the crypto store in redux in favour of shared-worker-based crypto store.

Note: this diff won't be landed with the future diffs that migrate crypto store data in redux to shared worekr

Depends on D11347

Test Plan

Tested on web, as on native any changes still call the same underlying methods.

Logged in to a keyserver in a single-keyserver mode, made sure that the new cookie row contained signed_identity_keys
Removed signed_identity_keys manually from keyserver db. Reloaded the webapp (which made a new websocket connection). Made sure that after reconnecting signed_identity_keys was again filled in, with the same value
Made sure that the identity client used a non-null device ID
Made sure that there weren't any error on QR login screen and that the QR login showed up
Made sure that connection to tunnelbroker worked and that I was able to send messages

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Mar 18 2024, 8:54 AM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 18 2024, 8:54 AM

Harbormaster completed remote builds in B27565: Diff 38144.Mar 18 2024, 9:09 AM

michal requested review of this revision.Mar 18 2024, 9:09 AM

michal added a child revision: D11351: [web] Migrate crypto store to shared worker.Mar 19 2024, 5:56 AM

kamil accepted this revision.Mar 19 2024, 8:35 AM

kamil added inline comments.

web/account/account-hooks.js
1 ↗	(On Diff #38144)	good to see this, now we can use code from `lib` everywhere
web/account/log-in-form.react.js
25 ↗	(On Diff #38144)	do we really need this?
web/account/qr-code-login.react.js
18–21 ↗	(On Diff #38144)	Can't find it right now but I think in one of diffs I saw `getContentSigningKey` method doing exactly the same
web/utils/tunnelbroker-utils.js
10 ↗	(On Diff #38144)	I think we can now use the same hook which is used for native? (Just move it lib) Separately, I think there is some more code that now can be de-duplicated or unified, so we can make our logic more platform-agnostic, but perhaps there is no time to prioritize it, we can just do it whenever someone touches it.

This revision is now accepted and ready to land.Mar 19 2024, 8:35 AM

Fixes, made useTunnelbrokerInitMessage cross-platform.

web/account/log-in-form.react.js
25 ↗	(On Diff #38144)	This was needed because previously we were sometimes using `cryptoStore` directly from redux and not through `getOrCreateCryptoStore` and it needed to be initialized. Now it's not needed so I'm going to remove this.

Harbormaster completed remote builds in B27595: Diff 38186.Mar 19 2024, 9:45 AM

ashoat added inline comments.Mar 19 2024, 5:57 PM

lib/tunnelbroker/tunnelbroker-context.js
406	Is there a risk that this will cache an old version of the `deviceID`, and fail to update it when the `deviceID` changes? Perhaps this is not a risk because if the `deviceID` fails, we can expect that it was because the device was logged out
web/grpc/identity-service-context-provider.react.js
24–29	Same question here. Is this safe because it refreshes when the `accessToken` changes? Confusing to see `accessToken` in the dep list given that it's not used

You are correct this doesn't actually work as I had hoped.

I tested it after logging out and while the tunnelbroker and identity client disconnected (because of missing userID/ access token) the deviceID was still cached. After logging in with the identity the next time the access token changed and the deviceID was fetched and updated, so while in the end I think it would work it's still buggy behaviour.

The reason I thought it would work is that after logout the CLEAR_SENSITIVE_DATA is dispatched and with D11299 it should finish before the deviceID is returned from the shared worker. But the CLEAR_SENSITIVE_DATA isn't run at the end of the reducer but in SQLiteDataHandler and the identity/tunnelbroker hooks run first.

For identity client we could go with a similar approach as on native -> fetch the deviceID on each method call.
Not sure what would be the best option for tunnelbroker, possibly check if the auth metadata is current when calling sendMessage and if not, close the socket so that it can be re-created? (cc @kamil)

michal mentioned this in D11372: [web/native] Create tunnelbroker init message on connection.Mar 22 2024, 8:55 AM

michal mentioned this in D11373: [web] Use shared worker crypto store in identity client provider.Mar 22 2024, 8:59 AM

Moved the identity client provider and tunnelbroker changes to D11373 and D11372 because they were more complicated after fixes.

The rest of this diff should be fine so removing "planned changes".

This revision is now accepted and ready to land.Mar 22 2024, 9:02 AM

michal added a parent revision: D11373: [web] Use shared worker crypto store in identity client provider.Mar 22 2024, 9:02 AM

Harbormaster completed remote builds in B27658: Diff 38279.Mar 22 2024, 9:16 AM

michal added a parent revision: D11386: [web/native] Add signMessage to olmAPI.Mar 26 2024, 7:03 AM

Rebase on changes to the web qr login screen.

Use the olm api signing method introduced in a previous diff.

Harbormaster completed remote builds in B27681: Diff 38329.Mar 26 2024, 7:28 AM

michal mentioned this in D11386: [web/native] Add signMessage to olmAPI.Mar 27 2024, 2:09 AM