Details

Reviewers

michal
kamil
atul

Commits

rCOMM4ac711b8ffa4: Initialize olm sessions for notifications on web and keyserver via socket…

Summary

This differential initialises olm session for notifications on web via server request mechanism. The keyserver checks whether the web client has the olm session. If it doesn't it requests session intialisation. The web client uses the shared logic to get necessary keys from the keyserver and web specific logic to create the session and store it in IndexedDB.

Test Plan

Checkout to master.
Log-out.
Log-in.
Checkout to the branch with this diff stack.

Then:

Using mariadb console ensure that new olm session was created on the keyserver.
Using Developer Tools ensure that new entry was created in IndexedDB and that it contains pickled olm session for notifs.
Log-out.
Using Developer Tools ensure that entry with olm session for notifs was removed (it is not implemented in this differential but this line(https://github.com/CommE2E/comm/blob/master/web/database/worker/db-worker.js#L154) should clear entire IndexedDB on log-out).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

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

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

marcin added a parent revision: D9534: Extract shareable notifications session creation logic to lib.Oct 19 2023, 3:40 AM

Rebase to update commit message

Harbormaster completed remote builds in B23354: Diff 32183.Oct 19 2023, 3:54 AM

Harbormaster completed remote builds in B23358: Diff 32187.Oct 19 2023, 4:03 AM

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

Update invariant error message

marcin added inline comments.Oct 19 2023, 4:26 AM

web/account/account-hooks.js
168 ↗	(On Diff #32188)	If we try to initialize olm session for notifications but there are no olm accounts it means that we either: Didn't include `SignedIdentityKeysBlob` in log in data. or We try to answer server request to create olm session for notifs but we haven't answered yet the request to upload `SignedIdentityKeysBlob`. Both are programmer errors. Theoretically we could use `gerOrCreateCryptoStore` here but I am afraid it would aallow programmers to introduce errors described above.

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

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

Harbormaster completed remote builds in B23359: Diff 32188.Oct 19 2023, 4:44 AM

ashoat added inline comments.Oct 19 2023, 11:13 AM

keyserver/src/session/cookies.js
838 ↗	(On Diff #32188)	Note that this will need to be updated to `NEXT_CODE_VERSION` in order for me to update it before a deploy (I think you're aware of this though)
web/account/account-hooks.js
154 ↗	(On Diff #32188)	Same concern as I outlined here: https://phab.comm.dev/D9532#inline-60149

Ensure that there are no two notifications session creations promises running at the same time.

Harbormaster completed remote builds in B23491: Diff 32403.Oct 25 2023, 8:40 AM

Correctly "contextify" web notifications session creator

Rebase

Harbormaster completed remote builds in B23503: Diff 32419.Oct 26 2023, 4:04 AM

Harbormaster completed remote builds in B23508: Diff 32424.Oct 26 2023, 5:00 AM

Looks good, might be good to have someone with more knowledge about crypto system take another look

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

Bundle picling key and pickled notifications session together and persist encrypted in IndexedDB.
This is necessary since we can't access redux (needed for pickling key of notifications account) and persisting pickling key in IndexedDB without additional
encryption is not acceptable either (due to securoty issues).

marcin added a parent revision: D9619: Move database encryption utils to generic location.Oct 27 2023, 6:32 AM

marcin removed a parent revision: D9534: Extract shareable notifications session creation logic to lib.

marcin added a child revision: D9620: Encrypt notifications for web clients supporting decryption.Oct 27 2023, 6:37 AM

Harbormaster completed remote builds in B23535: Diff 32460.Oct 27 2023, 7:54 AM

Handle Safari bug when storing CryptoKey

Harbormaster failed remote builds in B23788: Diff 32765!Nov 6 2023, 6:17 AM

Harbormaster completed remote builds in B23788: Diff 32765.Nov 6 2023, 6:32 AM

ashoat added inline comments.Nov 6 2023, 6:57 AM

web/account/account-hooks.js
230 ↗	(On Diff #32765)	Might be good to explain why Safari is special-cased here with a comment

Add comment to explain special treatment of Safari browser

Harbormaster completed remote builds in B24030: Diff 33113.Nov 13 2023, 3:22 AM

Change the structure of created notifications olm data for "debounce" approach.

This seems like a significant change. @marcin, should you consider re-requesting review?

Substantial changes are actually done to D9661. Logics of this differential remains unchanged - we just change the structure of data kept in IndexedDB. But for better safety I can re-request review.

keyserver/src/session/cookies.js
838 ↗	(On Diff #32188)	Yes, I understand this. I always start with `FURUTRE_CODE_VERSION` to handle the possibility that we will not be able to release the feature immediately after landing. If I now that the feature can be released immediately after landing I replace it with `NEXT_CODE_VERSION`.

Harbormaster failed remote builds in B24217: Diff 33350!Nov 16 2023, 11:11 AM

Harbormaster completed remote builds in B24217: Diff 33350.Nov 17 2023, 1:21 AM

Make pendingSessionUpdate and updateCreationTimestamp fialds mandatory and initialize them with a copy of mainSession and Date.now() respectively.

Harbormaster completed remote builds in B24239: Diff 33377.Nov 17 2023, 3:45 AM

michal accepted this revision.Nov 22 2023, 6:05 AM

michal added inline comments.