Page MenuHomePhabricator

[web] hook to get device key upload data
ClosedPublic

Authored by varun on Jan 24 2024, 10:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 1:27 AM
Unknown Object (File)
Fri, Oct 18, 3:36 PM
Unknown Object (File)
Fri, Oct 18, 3:36 PM
Unknown Object (File)
Fri, Oct 18, 3:36 PM
Unknown Object (File)
Fri, Oct 18, 3:36 PM
Unknown Object (File)
Fri, Oct 18, 3:36 PM
Unknown Object (File)
Fri, Oct 18, 7:10 AM
Unknown Object (File)
Thu, Oct 17, 6:44 PM
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/types/identity-service-types.js
86–93 ↗(On Diff #36119)

This should be readonly

web/account/account-hooks.js
183–191 ↗(On Diff #36119)

I think it would be more maintainable to put this comment above useGetSignedIdentityKeysBlob. Also, is there a downside to trying to initialize OLM twice?

web/account/account-hooks.js
183–191 ↗(On Diff #36119)

there might be a performance impact, but no other downside, i think

ashoat requested changes to this revision.Jan 25 2024, 2:47 PM

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
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

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

ashoat added 1 blocking reviewer(s): kamil.

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?

web/account/account-hooks.js
179–180 ↗(On Diff #36344)

@tomek suggested moving it here in this comment. i'm fine with either

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?

This revision is now accepted and ready to land.Jan 31 2024, 2:32 AM
web/account/account-hooks.js
231 ↗(On Diff #36344)

Makes sense, as I will be moving it to the database so we might as well handle it then.

web/account/account-hooks.js
179–180 ↗(On Diff #36344)

Ah okay. I don't really care

web/account/account-hooks.js
231 ↗(On Diff #36344)

should we create a task?

web/account/account-hooks.js
231 ↗(On Diff #36344)
This revision was automatically updated to reflect the committed changes.