[native] implement prekey rotation handler and call Identity
ClosedPublic
Actions

Authored by kamil on Jan 15 2024, 6:28 AM.

Details

Reviewers

Commits

Summary

Test Plan

Test if method is called
Test if uploading works (can be done by making prekey rotation is needed) and verify this by inspecting DDB content
Test if old cryptoModule is restored when RPC fails

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

kamil held this revision as a draft.

kamil retitled this revision from [native] implement prekey rotation to [native] implement prekey rotation handler and call Identity.

kamil published this revision for review.Jan 15 2024, 6:53 AM

kamil added inline comments.

native/components/prekeys-handler.react.js
23 ↗	(On Diff #35614)	we could get this from `IdentityServiceContextProvider` - not sure what is better

native/components/prekeys-handler.react.js
11 ↗	(On Diff #35614)	I don't understand the purpose of this delay
18 ↗	(On Diff #35614)	I think you can just `return undefined` here
44 ↗	(On Diff #35614)	Why we are only rotating prekeys after a login? Shouldn't we also rotate prekeys when they expire? Is the idea that the app will be killed / restarted frequently enough, and the rotation will typically happen then?

native/components/prekeys-handler.react.js
11 ↗	(On Diff #35614)	I intended to avoid blocking the initial app load when the app seems to be slowest - but if you think this doesn't improve anything I can remove it. There is an alternative I mentioned in ENG-5889 about using native API to schedule some tasks in the background (`WorkManager` on Android and I think `NSURLSession` on iOS) but this was a lot more complicated.
18 ↗	(On Diff #35614)	this causes consistent-return error
44 ↗	(On Diff #35614)	Why we are only rotating prekeys after a login? Shouldn't we also rotate prekeys when they expire? `loggedIn` is here only to avoid calling logic for logged-out users Is the idea that the app will be killed / restarted frequently enough, and the rotation will typically happen then? Yes, that was the intention. This causes the issue that if the user is not using the app for a very long time (longer than prekey expiring time) those will never be refreshed (see my comment above about an alternative solution that fixes this issue), but on the other hand, device will also not be able to upload one-time keys

Thanks for explaining!

native/components/prekeys-handler.react.js
18 ↗	(On Diff #35614)	Spoke about this with @kamil in our 1:1. Not sure why he was seeing that... we are able to do this in other parts of our codebase, such as here

native/components/prekeys-handler.react.js
23 ↗	(On Diff #35614)	i think we should use `getCommServicesAuthMetadataEmitter` like we do in `IdentityServiceContextProvider`, but curious for @marcin or @tomek 's thoughts

native/components/prekeys-handler.react.js
23 ↗	(On Diff #35614)	Emitter makes sense if we're storing the data for some time - it allows updating it. Here we're fetching it and using it immediately, and I don't see how the emitter might help.