Page MenuHomePhabricator
Differential D7813

Implement function to encrypt relevant parts of iOS notification
ClosedPublic
Actions

Authored by marcin on May 15 2023, 8:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 2:59 AM
Unknown Object (File)
Sat, Mar 29, 8:57 AM
Unknown Object (File)
Sat, Mar 29, 8:28 AM
Unknown Object (File)
Wed, Mar 26, 9:46 AM
Unknown Object (File)
Fri, Mar 21, 10:54 AM
Unknown Object (File)
Wed, Mar 19, 9:51 PM
Unknown Object (File)
Tue, Mar 18, 6:31 AM
Unknown Object (File)
Tue, Mar 18, 6:31 AM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
bartek
ashoat
atul
Commits
rCOMM079d30b229ce: Implement function to encrypt relevant parts of iOS notification
Summary

This differential implements a function that encrypts parts of iOS notification payload that are important to user's privacy

Test Plan
  1. Pull cookie of currently logged in user from MariaDB.
  2. Call this function in the keyserver code where iOS notification is constructed with hardcoded cookie id.
  3. Send notification to the physical iOS device. Log content of send notification and ensure relevant fields are encrypted.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-3026
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.May 15 2023, 8:18 AM
Herald added a subscriber: ashoat. · View Herald TranscriptMay 15 2023, 8:18 AM
marcin added a parent revision: D7797: Implement olm session updater that has encrypting functionality.May 15 2023, 8:27 AM
marcin added a child revision: D7815: Send encrypted notifications to iOS devices.
Harbormaster completed remote builds in B19343: Diff 26469.May 15 2023, 8:38 AM
marcin requested review of this revision.May 15 2023, 8:38 AM
ashoat added a comment.May 15 2023, 9:57 AM

I can't tell where these functions will be used, so it's a little harder to review

keyserver/src/push/crypto.js
1

Newline here

38

Can we save space and set this to 1?

42

We can remove the async keyword here

46–48

We can use a shorthand

52

Does encryptIOSNotification need to be exported?

ashoat requested changes to this revision.May 15 2023, 9:57 AM

Back to you

This revision now requires changes to proceed.May 15 2023, 9:57 AM
marcin updated this revision to Diff 26554.May 16 2023, 8:39 AM
  1. Validate every notification field before encryption attempt
  2. Encrypt more fields in notification payload
Harbormaster completed remote builds in B19411: Diff 26554.May 16 2023, 9:00 AM
atul resigned from this revision.May 16 2023, 9:39 AM
ashoat accepted this revision.May 16 2023, 9:56 AM

One request inline; please address before landing, or feel free to re-request review if you disagree or if I'm missing something

keyserver/src/push/crypto.js
41 ↗(On Diff #26554)

Can we construct a new Notification rather than mutating the existing one?

This revision is now accepted and ready to land.May 16 2023, 9:56 AM
marcin updated this revision to Diff 26611.May 17 2023, 5:19 AM

Create new notification instead of mutating the existing one

Harbormaster completed remote builds in B19450: Diff 26611.May 17 2023, 5:38 AM
ashoat added a comment.May 17 2023, 7:26 AM

I should have been more specific

keyserver/src/push/crypto.js
12 ↗(On Diff #26611)

Rather than cloning the existing notification, can you create a new empty one, and fill in its fields as necessary?

marcin updated this revision to Diff 26651.May 18 2023, 9:41 AM

Return apn.Notifications object in case of encryption failure. Previous apprhac used '...' syntax, and it failed further since the returned object didn't have 'length()' method.

ashoat added inline comments.May 18 2023, 9:50 AM
keyserver/src/push/crypto.js
12 ↗(On Diff #26651)

Let's construct a new apn.Notification instead of cloning the one we're passed in

Harbormaster completed remote builds in B19486: Diff 26651.May 18 2023, 9:59 AM
marcin mentioned this in D7814: Implement method in NSE to decrypt notification.May 19 2023, 7:46 AM
marcin updated this revision to Diff 26784.May 22 2023, 6:57 AM

Create new notification instead of copying

Harbormaster completed remote builds in B19599: Diff 26784.May 22 2023, 7:14 AM
ashoat added inline comments.May 22 2023, 10:41 AM
keyserver/src/push/crypto.js
33–34 ↗(On Diff #26784)

Nit: can we swap the order here?

39–40 ↗(On Diff #26784)

Nit: can we swap the order here?

75 ↗(On Diff #26784)

Is this line necessary now that we're no longer cloning the notification? I think undefined is the default value

marcin updated this revision to Diff 26893.May 23 2023, 7:44 AM

Blob encrypt in one pass

Harbormaster completed remote builds in B19685: Diff 26893.May 23 2023, 8:04 AM
ashoat accepted this revision.May 23 2023, 11:31 AM
marcin updated this revision to Diff 27182.May 29 2023, 11:41 AM

Rebase before landing

marcin updated this revision to Diff 27195.May 29 2023, 11:58 AM

Rebase before landing

Harbormaster completed remote builds in B19851: Diff 27182.May 29 2023, 12:30 PM
Harbormaster completed remote builds in B19864: Diff 27195.May 29 2023, 1:12 PM
Closed by commit rCOMM079d30b229ce: Implement function to encrypt relevant parts of iOS notification (authored by marcin). · Explain WhyMay 29 2023, 1:41 PM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM079d30b229ce: Implement function to encrypt relevant parts of iOS notification.

Revision Contents
Changeset List

PathSize
keyserver/
src/
push/
52 lines

Diff 26469

View Options

keyserver/src/push/crypto.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines