[web] hook to get device key upload data
ClosedPublic
Actions

Authored by varun on Jan 24 2024, 10:02 PM.

Details

Reviewers

ashoat
kamil
tomek

Commits

rCOMM88478898fc74: [web] hook to get device key upload data

Summary

this diff introduces a hook that will be passed to IdentityServiceClientWrapper. this hook will let us get the data required for DeviceKeyUpload when the login methods are called.

Test Plan

tested in next diff

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Jan 24 2024, 10:02 PM

varun added a child revision: D10455: [web] client method for login password user.Jan 24 2024, 10:09 PM

Harbormaster completed remote builds in B26143: Diff 36119.Jan 24 2024, 10:20 PM

varun requested review of this revision.Jan 24 2024, 10:20 PM

tomek added inline comments.Jan 25 2024, 2:42 AM

lib/types/identity-service-types.js
86–93	This should be readonly
web/account/account-hooks.js
183–191	I think it would be more maintainable to put this comment above `useGetSignedIdentityKeysBlob`. Also, is there a downside to trying to initialize OLM twice?

varun added inline comments.Jan 25 2024, 7:26 AM

web/account/account-hooks.js
183–191	there might be a performance impact, but no other downside, i think

address feedback

Harbormaster completed remote builds in B26156: Diff 36133.Jan 25 2024, 8:02 AM

We need to think through prekey and one-time key lifecycle, and make sure the approach is consistent across platforms

web/account/account-hooks.js
190 ↗	(On Diff #36133)	Is there a reason we wait until the above line completes before starting this one?
217 ↗	(On Diff #36133)	Noticed a discrepancy here. In D10455, you call this function from `logInPasswordUser`. On `native`, `logInPasswordUser` doesn't seem to result in new one-time keys being generated (let me know if I'm missing something). We should think a bit more carefully about when we want to generate new one-time keys. There is a cap to the number of one-time keys that Olm persists (I think around 100?). Generating new one-time keys can result in old, unused ones being deleted, without them being cleared on the identity service. Perhaps what we want here is to check the number of available one-time keys. If there are fewer than N, we can generate new ones. Also note that we currently have a (partially-built?) mechanism in Tunnelbroker that allows one identity client to ask another for new one-time keys.

This revision now requires changes to proceed.Jan 25 2024, 2:47 PM

ashoat added inline comments.Jan 25 2024, 2:49 PM

web/account/account-hooks.js
217 ↗	(On Diff #36133)	Additionally – you mutate the Olm accounts here by generating new one-time keys, but I can't tell where you're persisting the updated Olm accounts

address feedback

varun added inline comments.Jan 29 2024, 8:18 PM

web/account/account-hooks.js
190 ↗	(On Diff #36133)	no
217 ↗	(On Diff #36133)	Noticed a discrepancy here. In D10455, you call this function from logInPasswordUser. On native, logInPasswordUser doesn't seem to result in new one-time keys being generated (let me know if I'm missing something). We should think a bit more carefully about when we want to generate new one-time keys. There is a cap to the number of one-time keys that Olm persists (I think around 100?). Generating new one-time keys can result in old, unused ones being deleted, without them being cleared on the identity service. Perhaps what we want here is to check the number of available one-time keys. If there are fewer than N, we can generate new ones. Now using the same approach that we do on keyserver, which is what you described here
217 ↗	(On Diff #36133)	Now persisting the updated olm accounts using `setCryptoStore`

varun added a parent revision: D10876: [lib] move olm utils from keyserver to lib.Jan 29 2024, 8:19 PM

Harbormaster completed remote builds in B26297: Diff 36344.Jan 29 2024, 8:33 PM

Looks good to me! Would appreciate if @kamil could take a look as well, to make sure we're matching the native logic, and the prekey rotation stuff he's working on

web/account/account-hooks.js
179–180 ↗	(On Diff #36344)	Nit: Should this comment be moved to where `getSignedIdentityKeysBlob()` is invoked?

varun added inline comments.Jan 30 2024, 4:23 PM

web/account/account-hooks.js
179–180 ↗	(On Diff #36344)	@tomek suggested moving it here in this comment. i'm fine with either

kamil accepted this revision.Jan 31 2024, 2:32 AM

kamil added a subscriber: michal.

kamil added inline comments.

web/account/account-hooks.js
231 ↗	(On Diff #36344)	I think at some point we should rename `primaryAccount` to `contentAccount` - maybe when @michal will work on moving crypto operations to worker?