Page MenuHomePhabricator

[web] Create identity client on shared worker
ClosedPublic

Authored by michal on Mar 7 2024, 8:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 23, 1:50 AM
Unknown Object (File)
Tue, Apr 23, 1:49 AM
Unknown Object (File)
Tue, Apr 23, 1:47 AM
Unknown Object (File)
Tue, Apr 2, 4:54 PM
Unknown Object (File)
Mon, Apr 1, 3:41 AM
Unknown Object (File)
Mar 29 2024, 9:21 AM
Unknown Object (File)
Mar 26 2024, 2:10 PM
Unknown Object (File)
Mar 25 2024, 3:03 PM

Details

Summary

Creating IdentityServiceClientWrapper inside of the shared worker.

Depends on D11273

Test Plan
  • Make sure opaque WASM is loaded on both dev and docker (prod)
  • Create identity service client and try to generate a nonce, check if it connected and returned a result

Creation code:

await sharedWorker.schedule({
  type: workerRequestMessageTypes.CREATE_IDENTITY_SERVICE_CLIENT,
  opaqueWasmPath: opaqueWasmPath(),
  platformDetails: getConfig().platformDetails,
  authLayer,
});

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/shared-worker/utils/constants.js
61–65 ↗(On Diff #37932)

Our handling of opaqueURL is a bit different to the other WASM urls. On dev we set it to http://localhost:8080/opaque-ke.wasm. So in this case this code path will trigger and we will use opaqueURL directly.

On prod we set it to compiled/${manifest['comm_opaque2_wasm_bg.wasm']} which will use the second return and base origin.

web/shared-worker/worker/worker-crypto.js
124–210 ↗(On Diff #37932)

These are copied from web/account/account-hooks.js but because crypto store no longer needs to be behind a hook, they can just be normal functions.

They will eventually replace the existing hooks and they will be able to be removed.

Changes:

  • removed react hooks
  • removed async code (it's not needed now)
  • dispatch is replaced with persistCryptoStore
michal requested review of this revision.Mar 7 2024, 8:30 AM
web/shared-worker/utils/constants.js
61–65 ↗(On Diff #37932)

Our handling of opaqueURL is a bit different to the other WASM urls.

Why is this? How do we handle other WASM urls? Can you explain the concrete differences?

Response to the opaque URL comment:

Below, I wrote some investigations that I did about the differences between opaque and other WASM files. But I guess if we want to treat opaque the exact same way as we treat the other WASM files it wouldn't be a problem:

  • Add a new CopyPlugin to webworkers webpack config
  • Add another global string variable injected into html e.g. opaqueWebworkerURL

The reason the differences exist in this revision of this diff is that I wanted to use the same opaqueURL for both tab-JS-context and webworker-JS-context. But the corresponding JS files for these context are hosted in different places and so have different default origins for network calls.


In website-responders.js this is how we specify the URLs:

olmFilename: '',
commQueryExecutorFilename: '',
opaqueURL: 'http://localhost:8080/opaque-ke.wasm',
backupClientFilename: '',

and for prod:

olmFilename: manifest['olm.wasm'],
commQueryExecutorFilename: webworkersManifest['comm_query_executor.wasm'],
opaqueURL: `compiled/${manifest['comm_opaque2_wasm_bg.wasm']}`,
backupClientFilename: webworkersManifest['backup-client-wasm_bg.wasm'],

I started investigating it and I discovered some weird things with the opaque WASM file:

  • Alongside the web/dist/opaque-ke.[contenthash].wasm which is created with yarn prod, another file is created -> 955995f625ff00ecc335.wasm. I calculated hashes of both of these files and they match so the second file is also the opaque WASM code.
    • This also happens for yarn dev but instead of files being created they are hosted on the dev server (localhost:8080)
    • After adding my code which starts to reference the opaque on the shared worker (compiled to dist/webworkers/database.build.js) this file is also created from the shared worker webpack config (dist/webworkers/955995f625ff00ecc335.wasm)
    • The actual [contenthash] matches the beginning of the weird file name (dist/opaque-ke.955995f625ff.wasm)
  • If instead of running initOpaqueKe(opaqueURL) (initialization function imported from opaque) I just run initOpaqueKe() the network call goes to http://localhost:8080/955995f625ff00ecc335.wasm (in dev mode). The WASM is downloaded successfully

So it looks like webpack is automatically copying the wasm file for us but I'm not sure why.

That still doesn't really explain why we use 'http://localhost:8080/opaque-ke.wasm'. Possibly just a mistake and it's not necessary?

Additionally in prod version, opaqueURL has compiled/ prefix while e.g. olm doesn't. This might be a difference in how the initialization code is generated by wasm-pack and emscripten?

Thanks @michal for this detailed research. Do we know who first introduced the opaque WASM? Was it me or @varun maybe?

I think the best next step here is to create a new Linear issue with your observations as well as proposed changes to make it consistent, and then to assign to whoever introduced the opaque WASM first. If it's a former team member, then you can assign to me.

kamil added a subscriber: marcin.

The reason the differences exist in this revision of this diff is that I wanted to use the same opaqueURL for both tab-JS-context and webworker-JS-context. But the corresponding JS files for these context are hosted in different places and so have different default origins for network calls.

I think after migrating the Identity client to worker we will no longer need opaque on tab-JS-context, so it probably makes the most sense to now introduce a new CopyPlugin and match handling opaque wasm as other wasm files in our codebase, and after flipping the switch we can stop hosting this file for tab-JS-context?

web/shared-worker/worker/identity-client.js
8–12 ↗(On Diff #37932)

can be merged

13–14 ↗(On Diff #37932)
web/shared-worker/worker/shared-worker.js
29 ↗(On Diff #37932)

can be merged

232 ↗(On Diff #37932)

I think we can be more specific and throw different Errors for whether it's DB or Idenitiy request

web/shared-worker/worker/worker-crypto.js
124–210 ↗(On Diff #37932)

It would be nice if @marcin could review this part

Updated the diff to match the changes in previous diffs.


Went with the approach suggested by @kamil for the WASM file -> new CopyPlugin in webpack config and added a new a webworkersOpaqueFilename global variable. @ashoat in this case I don't think we need to create a separate a task to make it consistent. If you agree I will just create a task to stop hosting the opaque WASM file for tab-JS-context (that we do in an inconsistent way).

web/shared-worker/worker/shared-worker.js
232 ↗(On Diff #37932)

Decided to just add the message type to the error message (but it's done in a previous diff as I introduced a similar request category for olm api there).

web/shared-worker/worker/worker-crypto.js
174–221 ↗(On Diff #38056)

I was able to simplify it even further after changes in D11272 -> we no longer need to unpickle and pickle the crypto store when doing any operations because we keep the olm runtime objects in memory.

Could you create a task, to after fully migrating Identitiy client to the worker to deprecate opaque on the main thread?

web/shared-worker/worker/identity-client.js
10–12 ↗(On Diff #38056)
web/shared-worker/worker/worker-crypto.js
197 ↗(On Diff #38056)

I guess now it's some code duplication - this is going to be removed in future diff?

This revision is now accepted and ready to land.Mar 15 2024, 10:24 AM

Fix imports, task created here: ENG-7242

web/shared-worker/worker/worker-crypto.js
197 ↗(On Diff #38056)

Yes, I will put up a diff shortly that removed the corresponding web hooks (in the diff where we "flip the switch" to the shared-worker-based identity client).