Paths

Table of Contentst

Differential D11303

Boilerplate for web and desktop to look for keyserverID during notifications session creation and notification decryption
ClosedPublic
Actions

Authored by marcin on Mar 11 2024, 9:25 AM.

Tags

None

Referenced Files

	F3489904: D11303.diff
	Wed, Dec 18, 2:09 PM

	Unknown Object (File)
	Sun, Dec 15, 4:04 AM

	Unknown Object (File)
	Wed, Dec 4, 5:26 PM

	Unknown Object (File)
	Mon, Dec 2, 10:50 AM

	Unknown Object (File)
	Thu, Nov 28, 3:50 AM

	Unknown Object (File)
	Sun, Nov 24, 1:23 PM

	Unknown Object (File)
	Nov 16 2024, 2:59 AM

	Unknown Object (File)
	Nov 16 2024, 2:46 AM

Subscribers

Details

Reviewers

Commits

rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications…

Summary

This differential updates web and desktop code so that:

The keyserverID is an argument to methods that create session.
The keyserverID is an argument of methods that decrypt notifications on desktop.

Actually using this argument will be introduced in next diff.

Test Plan

Flow
Test that notifs aren't broken on web and desktop

Diff Detail

Repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Mar 11 2024, 9:25 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptMar 11 2024, 9:25 AM

marcin added a parent revision: D11302: Send keyserverID in web and desktop notifications.Mar 11 2024, 9:33 AM

marcin added a child revision: D11304: Refactor notifications sessions storage to support multiple keyservers on web and desktop.

Rebase to update commit message

marcin edited the summary of this revision. (Show Details)Mar 11 2024, 9:37 AM

Harbormaster completed remote builds in B27450: Diff 37999.Mar 11 2024, 9:40 AM

Harbormaster completed remote builds in B27453: Diff 38002.Mar 11 2024, 9:51 AM

marcin requested review of this revision.Mar 11 2024, 9:51 AM

Diff will change significantly due to: https://linear.app/comm/issue/ENG-7194/we-introduced-a-regression-in-macos-notifications#comment-8d85367e

michal added inline comments.Mar 12 2024, 8:31 AM

desktop/flow-typed/npm/electron_v22.0.0.js
339–343	Sorry, I should have caught this when adding (single-keyserver) encrypted notifications to desktop -> types in `flow-typed` are types for specific libraries and shouldn't be mixed with our types (in this case the `keyserverID` and `encryptedPayload` fields). Could we (potentially in a different diff) remove them and only keep the `+[string]: mixed` and add a check in JS code that checks if these fields exists.

Approach changed - we are moving with this proejct as it is and will fix desktop regression as a separate project.
Fix types in electron_v22.0.0.js

Harbormaster completed remote builds in B27495: Diff 38052.Mar 14 2024, 4:29 AM

In general the code looks good but there is one major problem: if someone doesn't update their desktop app they can still run a newer version of the web app.

This diff breaks the API between webapp and electron code by adding changing the arguments to the onEncryptedNotification (adding the keyserverID as the last argument would probably be fine). But the bigger problem is handling of this edge case with encrypting notifications...

desktop/src/push-notifications.js
146 ↗	(On Diff #38052)	At this point the `encryptedPayload` must be a string. So I don't think the `showNewNotification(userInfo, handleClick);` will ever run. You should probably check for `if (!userInfo.encryptedPayload)` first.
156–157 ↗	(On Diff #38052)	Don't we need a `typeof keyserverID !== 'string' \|\| typeof encryptedPayload !== 'string'` check here too?

This revision now requires changes to proceed.Mar 14 2024, 6:30 AM

marcin added inline comments.Mar 15 2024, 2:26 AM

desktop/src/push-notifications.js
156–157 ↗	(On Diff #38052)	True - my oversight.

Address JS-nits
Fixx potential issues realted to desktop-web code version discrepancy

Harbormaster completed remote builds in B27528: Diff 38096.Mar 15 2024, 6:37 AM

michal accepted this revision.Mar 18 2024, 4:15 AM

This revision is now accepted and ready to land.Mar 18 2024, 4:15 AM

Rebase before landing

Harbormaster completed remote builds in B27624: Diff 38222.Mar 20 2024, 4:32 AM

Closed by commit rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications… (authored by marcin). · Explain WhyMar 20 2024, 5:37 AM

This revision was automatically updated to reflect the committed changes.

marcin added a commit: rCOMM87eab80d4fa7: Boilerplate for web and desktop to look for keyserverID during notifications….

Revision Contents
Changeset List

Path

Size

desktop/

flow-typed/

npm/

electron_v22.0.0.js

6 lines

src/

6 lines

push-notifications.js

15 lines

lib/

types/

electron-types.js

1 line

web/

account/

account-hooks.js

2 lines

push-notif/

notif-crypto-utils.js

7 lines

push-notifs-handler.js

9 lines

Diff 38002

desktop/flow-typed/npm/electron_v22.0.0.js

Loading...

desktop/src/main.js

Loading...

desktop/src/push-notifications.js

Loading...

lib/types/electron-types.js

Loading...

web/account/account-hooks.js

Loading...

web/push-notif/notif-crypto-utils.js

Loading...

web/push-notif/push-notifs-handler.js

Loading...