Page MenuHomePhabricator

[web] olmAPI on shared worker
ClosedPublic

Authored by michal on Mar 15 2024, 10:19 AM.
Tags
None
Referenced Files
F3383709: D11338.diff
Thu, Nov 28, 5:23 PM
Unknown Object (File)
Mon, Nov 25, 4:42 AM
Unknown Object (File)
Mon, Nov 25, 2:21 AM
Unknown Object (File)
Sun, Nov 10, 10:21 PM
Unknown Object (File)
Thu, Nov 7, 2:27 AM
Unknown Object (File)
Oct 15 2024, 8:36 PM
Unknown Object (File)
Oct 15 2024, 8:36 PM
Unknown Object (File)
Oct 15 2024, 8:36 PM
Subscribers

Details

Summary

ENG-6768 : Migrate validateAndUploadPreKeys to shared worker
ENG-6658 : Migrate encrypt, decrypt to shared worker
Part of ENG-6657 : Migrate session creation code to shared worker

Using a similar approach as in D11275 (one message type for calling the methods) this diff moves the olmAPI to the shared worker. There are two logic changes which I have annotated in inline comments. Other than that the only changes are related to using real persisting functions and not the mocked ones in olm-api.js (which are now removed). In this case the olmAPI proxied functions are not hidden behind a flag as they were using mock functions anyway.

Depends on D11326

Test Plan

Called each of the methods and checked if they work. The encrypt/decrypt/contentInboundSessionCreator have been tested in context of peer to peer message communication.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/crypto/olm-api.js
41 ↗(On Diff #38103)

As the context type isn't available in shared worker this method now takes just AuthMetadata. The whole context isn't needed for web anyway now that olmAPI in shared worker can directly call the global identity client.

web/shared-worker/worker/worker-crypto.js
423–424 ↗(On Diff #38103)

This previously just contentAccount.mark_keys_as_published(); and was incorrect. We caught it with @kamil during testing.

kamil added inline comments.
web/shared-worker/worker/worker-crypto.js
396–401 ↗(On Diff #38103)

we should do the same for notif prekey

423–424 ↗(On Diff #38103)

thanks for fixing that

This revision is now accepted and ready to land.Mar 18 2024, 5:53 AM
web/shared-worker/worker/worker-crypto.js
214–215 ↗(On Diff #38103)
  1. Should we re-cast this to avoid polluting types with any?
  2. I prefer any to $FlowFixMe here. There is nothing to fix
404–406 ↗(On Diff #38103)

Why do we check this for the content account but not the notif account?

web/shared-worker/worker/worker-crypto.js
396–401 ↗(On Diff #38103)

to be more specific this is what I meant - but see my comment below

404–406 ↗(On Diff #38103)

Both accounts are created at the same time, each time keys are rotated synchronously so there is no need to because the result will be the same always (there is no way to eg. rotate content keys but not notifications, those are always in sync).
To make it more readable we can explicitly check both accounts or add a comment explaining this.

Fixed the type cast. Improved notification account handling and added a comment that we only need to check one of the accounts.