Details

Reviewers

kamil
michal

Commits

rCOMM7341fa87cd21: Implement main <-> renderer IPC and use it to decrypt notifications

Summary

This differential implements main <-> rendered IPC and uses it combined with decryption function from previous differential to decrypt notifications. In
summary notification decryption workflow is as follows:

Desktop code in push-notifications.js receives notification and checks that it is encrypted (encryptedPayload) field.
Sends decrypted payload to renderer process (web app) using onEncryptedNotification IPC.
Renderer receives it in callback registered in push-notifs-handler.js in web code. In this callback it decrypts payload using decryptDesktopNotification

function.

Decrypted notification is sent to main process using showDecryptedNotification IPC.
Main process receives decrypted notification in ipcMain.on('show-decrypted-notification', (...) => {...}) where it shows notification.

Test Plan

Apply this patch to local keyserver (it basically enables session establishment for desktop device and encrypts notification):

https://gist.github.com/marcinwasowicz/2df9c9bd4491862173241744d54a016f

Send some notifications.
Ensure they are correctly displayed and olm session version for currently logged user in desktop app increments with each notification.

With this differential we can also test large influx of out of order notifications (https://linear.app/comm/issue/ENG-5385/handle-large-number-of-out-of-order-notifications-incoming-afer-long).

Apply this patch on top of first patch to your local keyserver (it artificially calls multiple sendPushNotifs promises on single message): https://gist.github.com/marcinwasowicz/673b62b70998588a7e6f4be31bddd2c8
Send message.
Ensure all notifications that arrive are correctly decrypted.

Diff Detail

Repository

rCOMM Comm

Branch

marcin/eng-5383

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Nov 23 2023, 9:01 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 23 2023, 9:01 AM

marcin added a parent revision: D9963: Extend showNotification method to cover the case of decryption error notitication.Nov 23 2023, 9:01 AM

marcin edited the summary of this revision. (Show Details)

marcin edited the test plan for this revision. (Show Details)Nov 23 2023, 9:07 AM

Harbormaster completed remote builds in B24382: Diff 33575.Nov 23 2023, 9:21 AM

marcin requested review of this revision.Nov 23 2023, 9:21 AM

Accepting, but please fix the issues in inline comments.

web/push-notif/push-notifs-handler.js
49–57	`electron?.onX` functions return a function that unregister the listener. This is the `() => void` in `electron-types.js` and `return () => ipcRenderer.removeListener('on-encrypted-notification', withEvent);` in `preload.js` We can return a cleanup function from `useEffect` to be called whenever we re-run it. Alternatively we can write React.useEffect(() => { const cleanupFunction = electron?.onEncryptedNotification(...); return cleanupFunction; }, [staffCanSee])
55	I don't think this `await` is needed here

This revision is now accepted and ready to land.Nov 24 2023, 6:13 AM

marcin added a child revision: D9973: Expose major version of desktop code to the keyserver.Nov 24 2023, 9:03 AM

marcin added inline comments.Nov 27 2023, 5:20 AM

web/push-notif/push-notifs-handler.js
49–57	Your first suggestion results in eslint error: React Hook useEffect received a function whose dependencies are unknown. Pass an inline function instead. That said I went ahead with the second option.