Page MenuHomePhabricator

Enable encrypted notification coalescing in NSE on iOS
ClosedPublic

Authored by marcin on Sep 12 2023, 4:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:55 AM
Unknown Object (File)
Mon, Jun 24, 2:53 AM
Unknown Object (File)
Mon, Jun 24, 2:33 AM
Subscribers

Details

Summary

This differential enables NSE to perform coalescing of encrypted notifications. Previously notification coalescing was done by setting apns-collapse-id header in notification. This header was then equal to the identifier property of UNNotificationRequest (object received by the NSE on each mutable notification): https://developer.apple.com/documentation/usernotifications/unnotificationrequest/1649634-identifier?language=objc. The OS automatically replaces previous notification with the latest notification with the same request identifier property. In the world of encrypted notification this header cannot be set anymore. It would be a security breach since apple would see two encrypted notifications sharing the same collapse id. Unfortunately it is not possible to modify identifier property of UNNotificationRequest in the NSE. This forces us to:

  1. put collapseID in the encrypted payload.
  2. after decryption check for this field.
  3. If found then we have to create local notification with the same contents as the original one and request identifier equal to collapse id.
  4. Display local notification and silence the original one by calling content handler with blank notification content ([[UNNotificationContent alloc] init])

This is implemented in this diff.

Test Plan

Impossible to test without some changes on the keyserver.

  1. Build iOS app
  2. Apply this patch: https://gist.github.com/marcinwasowicz/a318bb475e575df662429702905b4a01
  3. Send a couple of photos to the same user, or update some chat settings a couple of times (the same setting!).
  4. Ensure that coalescing takes place.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin edited the test plan for this revision. (Show Details)

As mentioned in the description coalescing mechanism changes slightly and so does user experience. I recommend that reviewers watch recordings attached below:
Current coalescing:

NSE coalescing:

Update coalescing condition.

If the UX change originating from this diff is not acceptable the alternative approach we can experiment with is to silence every collapsible notifications and schedule local notifications from the NSE. This way we will be able to set request.identifier property (since we are the ones creating the UNNotificationRequest instance) and let the OS do the coalescing.

Nice! The UI change looks good to me, but letting others review as well.

Nit/offtop: you can use some online video resizer (e.g. this - first link on google) to make video previews smaller because Phabricator isn't able to resize video previews.
Alternatively, you could use a table, It's better but still too large - I mean try this remarkup

| before | after |
| {F754694} | {F754700} |
This revision is now accepted and ready to land.Sep 13 2023, 3:26 AM

The UX change doesn't seem particularly bad to me, but I'd like to understand if the approach you're using here (deleting the old notif) is similar to the approaches that other encrypted apps take. Is there no way to set a "collapse ID" from the NSE?

The UX change doesn't seem particularly bad to me, but I'd like to understand if the approach you're using here (deleting the old notif) is similar to the approaches that other encrypted apps take. Is there no way to set a "collapse ID" from the NSE?

I examined NSE code of:

  • Signal
  • Telegram
  • Element

I am not sure of any of those apps actually does some source of notifications coalescing but I am sure that none of them sets identifier property of UNNotificationRequest. However what is most interesting is that Signal appears to actually always call contentHandler with emoty content (silence notification) and then construct local notifications based on data delivered in the UNNotificationContent instance. Therefore if this apps wanted to do coalescing then it can just display new local notification with the same identifier as previous one and obtain the same behaviour as setting apns-collapse-id on the server side. Another interesting this is that Signal seems not to be sending any data in notifications at all. Its NSE contacts their server to fetch actual messages when it receives notification. I am not sure what Telegram and Element do since their source codes are not that easy to understand as Signal',s but I am pretty positive that if they do coalescing they probably also use local notifications.

That said I will allocate 2 - 3 hours of my work time to investigate if is is straightforwardly possible to use local notifications for coalescing in our codebase and update diff it is possible (or not update if reviewers think it is not that necessary).

Use local notifications for collapsible notifications to achieve previous UX experience.

tomek added inline comments.
native/ios/NotificationService/NotificationService.mm
360 ↗(On Diff #31256)

Should we really wait forever?

This revision is now accepted and ready to land.Sep 19 2023, 8:42 AM
native/ios/NotificationService/NotificationService.mm
360 ↗(On Diff #31256)

This doesn't look nice at the first sight but we have been already doing so for rescinds (see line 325) since march and haven't had any issues with that. However for the sake of code quality we can replace it wth 30 seconds time interval which is the time NSE has to process notification.

  1. Don't wait forever
  2. Rebase before landing