[native] update one time keys logic
ClosedPublic
Actions

Authored by varun on Feb 6 2024, 2:52 PM.

Details

Reviewers

Commits

rCOMM75d0111308c5: [native] update one time keys logic

Summary

A lot of changes here... will annotate all of them inline. Here are the big things:

consolidated getPrimaryOneTimeKeys and getNotificationsOneTimeKeys into a single API, getOneTimeKeys
make sure we only generate one-time keys if we don't have enough unpublished one-time keys
rename CryptoModule::getOneTimeKeys to CryptoModule::getOneTimeKeysForPublishing so that it's clear to the caller that the returned keys should be published immediately

Test Plan

Modified getOneTimeKeysForPublishing to generate one unpublished one-time key
called `commCoreModule.getOneTimeKeys(10) twice from JS
confirmed that the one unpublished key appeared in otks but not otks2 (all the keys in otks get marked as published so they are no longer accessible on the second call)
confirmed that otks2 was a brand new set of 10 keys
(not in patch) called commCoreModule.getOneTimeKeys(101) and got an error: {"message": "error generateKeys => invalid amount of one-time keys published. Expected 101, got 100"}. this is expected since the account can only hold 100 one-time keys.

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

varun edited the test plan for this revision. (Show Details)Feb 6 2024, 3:00 PM

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
72–74 ↗	(On Diff #36751)	this check didn't make sense to me and looked incorrect. we aren't reading from or writing to `this->keys.oneTimeKeys` in this method.
157 ↗	(On Diff #36751)	unfortunately there is no function to get the number of unpublished keys in the `olm` library, so we have to use `folly` to convert the buffer to JSON and check the size of `parsedUnpublishedKeys["curve25519"]`
160 ↗	(On Diff #36751)	only generate enough one-time keys to reach `oneTimeKeysAmount`
native/cpp/CommonCpp/CryptoTools/CryptoModule.h
46 ↗	(On Diff #36751)	50 was arbitrary based on discussion in D1843 and didn't match the default of 10 that we use on web and keyserver

varun requested review of this revision.Feb 6 2024, 3:10 PM

varun edited the test plan for this revision. (Show Details)Feb 6 2024, 3:25 PM

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
591 ↗	(On Diff #36751)	As I pointed in D10927 - please early reject and return here so that we don't have to wrap `try ... catch` with `else` and indentation is reduced.

kamil added inline comments.

native/handlers/peer-to-peer-message-handler.js
67–68 ↗	(On Diff #36751)	same comment as below
native/identity-service/identity-service-context-provider.react.js
220–221 ↗	(On Diff #36751)	we should update this here or in D10964 - depending on who lands diff first

Should this diff have a stack?

In D10977#317023, @ashoat wrote:

Should this diff have a stack?

no, it's a standalone diff

Please rename NotificationsCryptoModule::getNotificationsOneTimeKeys before landing

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
550–566 ↗	(On Diff #36751)	Can this be replaced with `jsi::valueFromDynamic`? folly::dynamic parsedOneTimeKeys = folly::parseJson(oneTimeKeysBlob); jsi::Value jsiOneTimeKeys = jsi::valueFromDynamic(rt, parsedOneTimeKeys); return jsiOneTimeKeys.asObject(rt); You'll need to import this header to use it. This should probably be handled as a separate diff.
591 ↗	(On Diff #36751)	Agree with this
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
239 ↗	(On Diff #36751)	Shouldn't this be renamed to include the `ForPublishing` suffix as well?

This revision is now accepted and ready to land.Feb 7 2024, 10:47 AM

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
160 ↗	(On Diff #36751)	This is inconsistent with `retrieveAccountKeysSet`. We should adjust either one or the other to be consistent
164–170 ↗	(On Diff #36751)	It's not clear to me why we need this check, and I suspect it will make it harder to deprecate `this->keys`, since `publishOneTimeKeys` will need to return both this number, as well as the actual one-time keys that need to be published to identity.

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
160 ↗	(On Diff #36751)	will adjust retrieveAccountKeysSet in https://linear.app/comm/issue/ENG-6688/update-retrieveaccountkeysset-to-only-generate-enough-one-time-keys-to
164–170 ↗	(On Diff #36751)	removing this check. the only case where `publishedOneTimeKeys` should be different from `oneTimeKeysAmount` is when `oneTimeKeysAmount` is greater than 100 (MAX_ONE_TIME_KEYS), but we will add better error handling for that case in https://linear.app/comm/issue/ENG-6689/add-a-check-in-commcoremodule-and-cryptomodule-to-make-sure-that
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
550–566 ↗	(On Diff #36751)	https://linear.app/comm/issue/ENG-6696/use-jsivaluefromdynamic-in-parseolmonetimekeys
591 ↗	(On Diff #36751)	makes sense
native/identity-service/identity-service-context-provider.react.js
220–221 ↗	(On Diff #36751)	landing this now so will probably have to update in D10964