Page MenuHomePhabricator

Send encrypted notifications to iOS devices
ClosedPublic

Authored by marcin on May 15 2023, 8:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 2:07 AM
Unknown Object (File)
Thu, Mar 7, 2:00 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Unknown Object (File)
Thu, Mar 7, 1:58 AM
Subscribers

Details

Summary

This differential implements iOS notification encryption on the keyserver.

  1. In message-creator cookie ids are pulled alongside with device tokens.
  2. In utils we enable sending different notification contents
  3. In send and rescind we adjust to new changes.
Test Plan

Send notifications to iOS device.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 15 2023, 8:42 AM
Harbormaster failed remote builds in B19345: Diff 26471!
keyserver/src/creators/message-creator.js
339 ↗(On Diff #26471)

We should remove this constraint, as this query is used for other things that are not related to notifs, and need to happen even if a user has no logged-in sessions (no cookies)

keyserver/src/push/send.js
587 ↗(On Diff #26471)

It would be good to update this to $ReadOnlyArray

603 ↗(On Diff #26471)

Take Set out here

660 ↗(On Diff #26471)

Don't mutate the payload just to avoid updating Flow types

843 ↗(On Diff #26471)

The reason for the !collapseKey check is that we want to consider collapsing notifications in the NSE out-of-scope for this month's goal

845 ↗(On Diff #26471)

Why not call this in prepareAPNsNotification? It would be easier to check messageTypes.TEXT there, and it would make more sense to keep preparation / sending separate

  1. Remove NOT NULL constraint from cookie id pull query
  2. DO the encryption inside prepareAPNsNotification method.
  3. Implement more accurate check for text notifs.
ashoat requested changes to this revision.May 16 2023, 10:09 AM
ashoat added inline comments.
keyserver/src/push/send.js
687 ↗(On Diff #26556)

We need to be comparing the encrypted size to the max payload limit. Otherwise we may still exceed the limit after encryption, which will cause the notif to be ignored

865 ↗(On Diff #26556)

We should generate an APNsDelivery for every delivered notif

This revision now requires changes to proceed.May 16 2023, 10:09 AM
keyserver/src/push/send.js
865 ↗(On Diff #26556)

Why do we need different APNsDelivery object for each notification? The APNsDelivery object looks like this:

type APNsDelivery = {
  source: $PropertyType<NotificationInfo, 'source'>,
  deviceType: 'ios' | 'macos',
  iosID: string,
  deviceTokens: $ReadOnlyArray<string>,
  codeVersion: number,
  errors?: $ReadOnlyArray<ResponseFailure>,
};

Regardless of whether notifications are encrypted or not, one APNsDelivery gives us complete information on which deviceToken received notifications and which didn't. Why having many APNsDelivery objects with only one deviceToken and at most one error is better for encrypted notifications than one APNsDelivery with many deviceTokens and possibly may errors?

Correctly calculate whether messageInfos should be included into notification payload

ashoat requested changes to this revision.May 17 2023, 10:17 AM
ashoat added inline comments.
keyserver/src/push/send.js
879 ↗(On Diff #26613)

Why do we need different APNsDelivery object for each notification? The APNsDelivery object looks like this:

type APNsDelivery = {
  source: $PropertyType<NotificationInfo, 'source'>,
  deviceType: 'ios' | 'macos',
  iosID: string,
  deviceTokens: $ReadOnlyArray<string>,
  codeVersion: number,
  errors?: $ReadOnlyArray<ResponseFailure>,
};

Regardless of whether notifications are encrypted or not, one APNsDelivery gives us complete information on which deviceToken received notifications and which didn't. Why having many APNsDelivery objects with only one deviceToken and at most one error is better for encrypted notifications than one APNsDelivery with many deviceTokens and possibly may errors?

Let me be more specific: by only taking the first notification here, and ignoring the rest, you are failing to record in the notifications DB the full list of notification IDs that have been delivered.

This has a number of negative effects, most notably that are you breaking rescinding, which relies on the iosID being present. (I didn't realize this in the initial feedback, but I certainly suspected that something sketchy was happening here, since you are throwing away all but the first notification.)

Perhaps the easiest solution would be to go back to the old behavior, where there is one delivery for each iosID. That was my initial suggestion. However, I am open to other solutions that do not break notif rescinding.

Separately, some feedback: this sort of thing frankly should not have to be discussed in code review, and certainly should not take two "Request Changes" for you to look into. When you make a change like this (making it so we only look at the first iosID), you should think like a reviewer, and treat this as a "red flag" that warrants significant investigation. It is a sketchy decision and is likely to lead to sketchy consequences, so you should be very sure that it is a correct direction before putting up the diff (and certainly before ignoring review feedback).

This revision now requires changes to proceed.May 17 2023, 10:17 AM
keyserver/src/push/send.js
698–720 ↗(On Diff #26613)

Instead of doing three passes here, we can simply combine them:

const notificationsToSend = notificationsWithMessageInfos.map((notifWithMessageInfos, idx) => {
  const copyWithMessageInfos = _cloneDeep(notif);
  if (copyWithMessageInfos.length() <= apnMaxNotificationPayloadByteSize) {
    return notifWithMessageInfos;
  }
  const notifWithoutMessageInfos = notifications[idx];
  const copyWithoutMessageInfos = _cloneDeep(notifWithoutMessageInfos);
  if (copyWithoutMessageInfos.length() <= apnMaxNotificationPayloadByteSize) {
    console.warn(
      `${platformDetails.platform} notification ${uniqueID} ` +
      `exceeds size limit, even with messageInfos omitted`,
    );
  }
  return notifWithoutMessageInfos;
});
879 ↗(On Diff #26613)

In retrospect, this whole analysis may be incorrect – I am not sure if iosID is perhaps the exact same for every single one of these notifications. Apologies for my aggressive note... I'll review this and get back.

keyserver/src/push/send.js
879 ↗(On Diff #26613)

Indeed, after rereading this I can see that my analysis was incorrect. The notification.id used here is actually the same for each of the notifications.

Again, apologies for my aggressive note. That said, I think this is a case of a brittle assumption that is not being enforced, similar to the case of the assumption regarding the length of the SQLCipher encryption key. When we have assumptions like this, we should make sure the assumptions are enforced.

Can you add some code that checks over the list of notifications and guarantees that they have the same notification.id?

Fix mistakes made during conflict resolution

keyserver/src/push/send.js
879 ↗(On Diff #26613)

Sure, I will do it.

  1. One pass instead of three
  2. Enforce that all encrypted copies of the same notification share id so that rescinding is transparent to e2e encryption

Accepting since comments are all addressed, but it looks like we'll have to revise this after we adjust all encryption to occur on a single blob, instead of being field-by-field. Might be a good idea to re-request review after that

This revision is now accepted and ready to land.May 22 2023, 10:50 AM
  1. Return array of pairs from getDevicesByPlatform instead of two arrays to ensure cookieIDs length matches deviceTokens length
  2. One pass instead of three during notifications payload with messageInfos size validation.
  3. Refactor updateBadgeCount to create delivery promise from iOS badge notification encryption and iOS notifdications transport
ashoat added inline comments.
keyserver/src/push/send.js
945–949 ↗(On Diff #26979)

Personally I would find this more readable

Change typing so that notification are packaged in advance with corresponding deviceTokens

ashoat added inline comments.
keyserver/src/push/send.js
939–943 ↗(On Diff #27161)

Personally I would find this more readable

Rebase before landing. Use real code version