Page MenuHomePhabricator

Create react hook to initialize olm notiications session and get first encrypted message for the keyserver
ClosedPublic

Authored by marcin on Apr 27 2023, 6:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 16, 2:50 PM
Unknown Object (File)
Tue, Apr 16, 11:41 AM
Unknown Object (File)
Tue, Apr 16, 11:30 AM
Unknown Object (File)
Tue, Apr 16, 5:29 AM
Unknown Object (File)
Mon, Apr 15, 3:48 PM
Unknown Object (File)
Mon, Apr 15, 3:38 PM
Unknown Object (File)
Mon, Apr 15, 3:37 PM
Unknown Object (File)
Sat, Apr 13, 11:35 PM
Subscribers

Details

Summary

This differential implements react hook that:

  1. fetches porekey, signed identity keys blob and one time keys from the keyserver
  2. initializes olm session for notifs on the client.
  3. returns first initial encrypted message to be sent to the keyserver so that it can establish olm session as well
Test Plan

Call this hook after calling nativeLogInExrtraInfo selector (this ensures that crypto accounts were correclty initialized).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/utils/crypto-utils.js
26–46 ↗(On Diff #25821)

This is a somewhat unusual pattern. It's rare that we await dispatchActionPromise; instead, if there is something that depends on the result of the async function passed to dispatchActionPromise, we will wrap that in a new async function that we then pass to dispatchActionPromise.

Eg. we could do something like this:

return React.useCallback(
  (oneTimeKeysCount: number) => {
    const initializeNotifsPromise = (async () => {
      const { signedIdentityKeysBlob, prekey, oneTimeKeys } =
        await callGetOlmNotifsSessionInitializationData({ oneTimeKeysCount });
      const { notificationIdentityPublicKeys } = JSON.parse(
        signedIdentityKeysBlob.payload,
      );
      await commCoreModule.initializeNotificationsSession(
        JSON.stringify(notificationIdentityPublicKeys),
        prekey,
        oneTimeKeys,
      );
      return await commCoreModule.generateInitialNotificationsEncryptedMessage();
    })();
    dispatchActionPromise(
      getOlmNotifsSessionInitializationDataActionTypes,
      initializeNotifsPromise,
    );
  },
  [callGetOlmNotifsSessionInitializationData, dispatchActionPromise],
};

To be fair, it's probably okay to do this either way. The main difference is how the Redux actions look. In the "usual" approach I've outlined above, any exception will cause a _FAILED action to be dispatched, and a successful run will dispatch a _SUCCESS action with the payload being the encrypted message.

In this case, I wonder what you need Redux to look like. Does Redux need to get some info from all of this?

  1. If Redux only needs the result of olmNotifsSessionDataPromise, then I think your approach is solid.
  2. If Redux needs something from later (eg. the initial message), then I think we should use the approach I've outlined above. We would probably want to rename the action types to reflect the full process.
  3. If Redux needs something even later in the process, we could consider using my approach, but with additional code.
  4. If Redux doesn't need anything, we might reconsider using dispatchActionPromise.
40 ↗(On Diff #25821)

We will also need the prekey signature here

native/utils/crypto-utils.js
26–46 ↗(On Diff #25821)

For me it is always useful to use dispatchActionPromise because it e.g. allows checking request status without any additional logic. For how much we put into the promise, it all depends on how much we need in redux, but even if redux wants more, we can dispatch another action with it after awaiting the result. So my preference is to isolate requests from the rest of the logic, but it's not a strong one and we can violate it rather liberally.
On the other hand, if we want to display a loading state, we should consider for how long we would like to do it. In this case doing it for the whole duration of the logic makes more sense.
So overall, both options make sense, and I don't think that the difference is worth spending too much time.

It would've been nice to combine this with the next diff so it would be easier to see how useInitialNotificationsEncryptedMessage is used. Something to consider for next time (not worth making any changes now)

native/utils/crypto-utils.js
25 ↗(On Diff #26004)

Since we already await olmSessionDataPromise below, I don't think we need this await here. It likely has no negative effect on performance either way, but I think it's a good to remove it for consistency / simplicity

This revision is now accepted and ready to land.May 2 2023, 10:06 AM
  1. Remove unecessary await
  2. Adjust to new CommCoreModule API