Details

Reviewers

atul
michal
kamil

Commits

rCOMMcae7f78d9ede: Extract shareable notifications session creation logic to lib

Summary

Most of the logic implemented in useInitialNotificationsEncryptedMessage hook in native folder can be reused for olm session initialisation on web. That said the shareable part it should be moved to lib and only the platform specific part should be kept in native. Additionally this differential introduces necessary changes in native components (mostly boilerplate code).

Test Plan

Using native app ensure that brand new olm session for notifs (select * from olm_sessions; in mariadb console) is created when:

Logging in.
Registering.
Registering with siwe.

Additionally repeat the second part of test plan for D9532 but for native app.

This will ensure that existing functionality relying on olm data on native isn't broken by this differential.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Oct 19 2023, 3:36 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 19 2023, 3:36 AM

marcin added a parent revision: D9533: Enforce getSignedIdentityKeysBlob in socket selectors.Oct 19 2023, 3:38 AM

marcin added a child revision: D9535: Initialize olm sessions for notifications on web and keyserver via socket requests.Oct 19 2023, 3:40 AM

Rebase to update commit message

Harbormaster completed remote builds in B23353: Diff 32182.Oct 19 2023, 3:53 AM

Harbormaster completed remote builds in B23357: Diff 32186.Oct 19 2023, 4:02 AM

marcin requested review of this revision.Oct 19 2023, 4:02 AM

marcin edited the summary of this revision. (Show Details)Oct 19 2023, 4:16 AM

marcin edited the test plan for this revision. (Show Details)

Realize this is mostly a refactoring diff, but wonder if we should add some error handling or at least logging to useInitialNotificationsEncryptedMessage? Maybe as a follow-up or something

lib/shared/crypto-utils.js
29	Should we do any error handling in this callback in case any of the steps fail?

This revision is now accepted and ready to land.Oct 23 2023, 10:04 AM

marcin added inline comments.Oct 25 2023, 4:23 AM

lib/shared/crypto-utils.js
29	On `native` this function is used during log-in. That said if it fails then we let the entire log-in fail. This is something we agreed on during work on encrypted notifications. We could `try ... catch` and let notifications session be created via socket request. However it would probably have similar effect as to simply remove it from log-in and put the responsibility to create notfications session on the socket request which would be retried in case any of those steps fail. So the basic question si whether we want to let the user log-in even if they can't establish notifications session instantly. Additionally this is lib code so it can be argued whether the actual caller of this function will know better how to handle its failure. This is something I could discuss with @ashoat during 1:1.