This differential refactors notifications session creation and usage on web and desktop so that notifications can be received from multiple keyservers.
Details
Details
Test migration of sessions:
- Open and log in to web/desktop app before applying this diff.
- Apply this diff and open web app again.
- Check the content of indexed db and ensure that session and encryption key are stored under key that conforms to new format.
- 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:
- Open web app and ensure that you are logged out.
- Log in.
- Ensure that web notifs work correctly.
Test race condition:
- Open logged out web app in three different windows.
- Quickly log in to each.
- Check that there are 6 entries in Indexed DB - 3 notifs sessions and 3 notifs sessions encryption keys.
- Send a web notif.
- 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
- both associated with largest cookie id value.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Diff will change significantly due to: https://linear.app/comm/issue/ENG-7194/we-introduced-a-regression-in-macos-notifications#comment-8d85367e
Comment Actions
- Approach changes - we are moving with this porject as it is and will fix desktop regression in separate project.
- Udpate createNotificationsSession hook - let mutliple promises run together provided they are for different keyservers.
| 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? |
| 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. |
Comment Actions
- Rename legacy keyserver id constant.
- Fix potential issues related to desktop-web code version discrepancy.
| 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? |