Page MenuHomePhabricator

Refactor notifications sessions storage to support multiple keyservers on web and desktop
ClosedPublic

Authored by marcin on Mar 11 2024, 9:32 AM.
Tags
None
Referenced Files
F3515423: D11304.diff
Sun, Dec 22, 8:51 AM
Unknown Object (File)
Thu, Dec 19, 7:37 PM
Unknown Object (File)
Thu, Dec 19, 2:24 PM
Unknown Object (File)
Wed, Dec 18, 4:33 PM
Unknown Object (File)
Wed, Dec 18, 4:28 PM
Unknown Object (File)
Wed, Dec 18, 3:54 PM
Unknown Object (File)
Wed, Dec 18, 3:50 PM
Unknown Object (File)
Sun, Dec 15, 8:57 PM
Subscribers

Details

Summary

This differential refactors notifications session creation and usage on web and desktop so that notifications can be received from multiple keyservers.

Test Plan

Test migration of sessions:

  1. Open and log in to web/desktop app before applying this diff.
  2. Apply this diff and open web app again.
  3. Check the content of indexed db and ensure that session and encryption key are stored under key that conforms to new format.
  4. Depending on whether you are on old or new keyserver set-up you will or will not be able to decrypt notifs.

Test new session:

  1. Open web app and ensure that you are logged out.
  2. Log in.
  3. Ensure that web notifs work correctly.

Test race condition:

  1. Open logged out web app in three different windows.
  2. Quickly log in to each.
  3. Check that there are 6 entries in Indexed DB - 3 notifs sessions and 3 notifs sessions encryption keys.
  4. Send a web notif.
  5. Ensure that notif is correctly decrypted and displayed and after receiving notif there is only one notifs session in IndexedDB and one encryption key in IndexedDB
  6. both associated with largest cookie id value.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

  1. Approach changes - we are moving with this porject as it is and will fix desktop regression in separate project.
  2. Udpate createNotificationsSession hook - let mutliple promises run together provided they are for different keyservers.
michal requested changes to this revision.Mar 14 2024, 6:25 AM
michal added inline comments.
web/push-notif/notif-crypto-utils.js
58 ↗(On Diff #38054)

Can we also change it so that the name contains that this is only for migration (like on native)?

304–310 ↗(On Diff #38054)

Won't the startsWith break now that the keys are prefixed with KEYSERVER:<id>?

417–418 ↗(On Diff #38054)

Nit: it might be better to have an object (keys + values) here instead of depending on insertion index.

422 ↗(On Diff #38054)

Are NOTIFICATIONS_OLM_DATA_CONTENT and NOTIFICATIONS_OLM_DATA_ENCRYPTION_KEY used anywhere else? Should we update the names (+ add the comments) that they are only used inside of a legacy migration?

This revision now requires changes to proceed.Mar 14 2024, 6:25 AM
web/push-notif/notif-crypto-utils.js
304–310 ↗(On Diff #38054)

Now that the keys are prefixed with KEYSERVER:<id> the startsWith method receives olmDataContentKeyForKeyserverPrefix or olmEncryptionKeyDBLabelForKeyserverPrefix which are strings starting with KEYSERVER:<id>, because getOlmEncryptionKeyDBLabelForCookie and getOlmDataContentKeyForCookie are implemented this way.

422 ↗(On Diff #38054)

NOTIFICATIONS_OLM_DATA_CONTENT and NOTIFICATIONS_OLM_DATA_ENCRYPTION_KEY are used in getOlmDataContentKeyForCookie and getOlmEncryptionKeyDBLabelForCookie. They are used to construct indexdb keys in new approach.

  1. Rename legacy keyserver id constant.
  2. Fix potential issues related to desktop-web code version discrepancy.

Fix eslint by adding hook dependency

michal added inline comments.
web/account/account-hooks.js
300 ↗(On Diff #38100)

cc @ashoat: @marcin is using here (and in one other file) a NEXT_CODE_VERSION on web. This code version represents the next desktop version. It's used here because there can be a situation where a newer version of the webapp is running on older version of the desktop.

So this will have to be replaced with 12 (current desktop version is 11).

After the next web/ keyserver release we will also need to make a new desktop release.

web/push-notif/notif-crypto-utils.js
402 ↗(On Diff #38100)

Thanks for updating this comment! Can we also add a note that the legacy format is used when the webapp is run on an older version of desktop?

This revision is now accepted and ready to land.Mar 18 2024, 4:51 AM
web/account/account-hooks.js
300 ↗(On Diff #38100)

Thanks for calling this out!

@marcin, would be good to send a reminder about this to the Releases channel on Comm once you land this

  1. Update a comment about legacy IndexedDB key format.
  2. Rebase before landing