Details

Reviewers

kamil
michal

Commits

rCOMM0f9ba5189730: Encrypt notifications for MacOS clients with code versions supporting…

Summary

This differential checks for MacOS client code versions and encrypts notification if both are sufficiently high. Additionaly some other places in the code
are updated to pass information on major desktop version to functions responsible for notification preparation.

Test Plan

Build desktop app.
Send a couple of notifications (visual and badge only). Ensure that they are displayed/ badge icon is updated. Ensure that no session in olm_sessions table in MariaDB increments its version.
Update NEXT_CODE_VERSION -> 0.
Repeat step 2. This time ensure that there is an entry in olm_sessions table that increments its version with each notification. Obviously ensure that notifications are working correctly (displayed in decrypted form and updating badge icon).
Finally, bring NEXT_CODE_VERSION back to its original value and repeat step 2. Ensure that notifications work correctly but the session that was incrementing its version in step 4 doesn't do so anymore.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Nov 27 2023, 6:56 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 27 2023, 6:56 AM

marcin added a parent revision: D10056: Rename IOS to APNs in function responsible for notifications encryption.Nov 27 2023, 6:56 AM

marcin edited the test plan for this revision. (Show Details)Nov 27 2023, 7:00 AM

Harbormaster completed remote builds in B24525: Diff 33740.Nov 27 2023, 10:22 AM

marcin requested review of this revision.Nov 27 2023, 10:22 AM

ashoat added inline comments.Nov 27 2023, 11:39 AM

keyserver/src/push/send.js
803–806 ↗	(On Diff #33740)	Should this be made read-only? Whenever we update a type, we should take that opportunity to try and improve it

Overall LGTM, just a few questions

keyserver/src/push/send.js
840–841 ↗	(On Diff #33740)	I think we can remove this hack now. This was added in D7903, when I was adding a web version and I didn't want to change the desktop notification code which operated under assumption that `codeVersion == -1` (the same as web before adding versioning). But now that we have a desktop version available, there shouldn't be an issue where someone uses the desktop `codeVersion` incorrectly. Separately shouldn't this made the `hasMinCodeVersion` check below fail?
924 ↗	(On Diff #33740)	Nit
1747–1753 ↗	(On Diff #33740)	What does this `map` do?

marcin added inline comments.Nov 28 2023, 5:22 AM

keyserver/src/push/send.js
840–841 ↗	(On Diff #33740)	Separately shouldn't this made the hasMinCodeVersion check below fail? The hack above that you request me to remove should make `hasMinCodeVersion` check fail. It didn't fail when I was executing the test plan since for testing I always set `NEXT_CODE_VERSION` to 0 during testing. But now that we remove this hack we should be fine.
1747–1753 ↗	(On Diff #33740)	This map is actually necessary since it strips some additional fields returned from `prepareEncryptedAPNsNotifications` that are not part of `TargetedAPNsNotification`. Latest flow will complain if we just assign `targetedNotifications` to `await prepareEncryptedAPNsNotifications`.