Page MenuHomePhabricator

Implement method in NSE to decrypt notification
ClosedPublic

Authored by marcin on May 15 2023, 8:20 AM.
Tags
None
Referenced Files
F1650064: D7814.diff
Wed, Apr 24, 2:10 AM
Unknown Object (File)
Wed, Apr 17, 5:56 PM
Unknown Object (File)
Tue, Apr 16, 9:43 AM
Unknown Object (File)
Mon, Apr 15, 6:56 PM
Unknown Object (File)
Mon, Apr 15, 4:23 PM
Unknown Object (File)
Mon, Apr 15, 3:55 PM
Unknown Object (File)
Mon, Apr 15, 3:40 PM
Unknown Object (File)
Sun, Apr 14, 4:16 PM
Subscribers

Details

Summary

This differential impements methods in NSE to decrypt encrypted notification.

Test Plan
  1. Repeat steps from parent differential test plan
  2. Ensure this time the encrypted notification is the one that is sent, not the original.
  3. Ensure decrypted notification is displayed on the 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:35 AM
Harbormaster failed remote builds in B19344: Diff 26470!
native/ios/NotificationService/NotificationService.mm
70 ↗(On Diff #26470)

Can you remind me – what does it mean to call self.contentHandler with an empty UNNotificationContent like this?

190–199 ↗(On Diff #26470)

We should expect messageInfos to be missing if they are too large to fit into the APNs payload, so I suspect we need a conditional check here

native/ios/NotificationService/NotificationService.mm
70 ↗(On Diff #26470)

This is how notifications filtering works. It silences notification.
I decided not to display notification if we couldn't decrypt it. With https://linear.app/comm/issue/ENG-2670/c-equivalent-of-dev-flag#comment-45f70230 being done we could instead display error details as to why notification decryption failed.

  1. Refactor to adjust to changes in olm-session-updater
atul added a subscriber: atul.
atul added inline comments.
native/ios/NotificationService/NotificationService.mm
196 ↗(On Diff #26555)

Should probably be keysToOmit?

ashoat added inline comments.
native/ios/NotificationService/NotificationService.mm
196 ↗(On Diff #26555)

Agree

This revision is now accepted and ready to land.May 16 2023, 10:03 AM

Handle aps dictionary.

I wasn't a mistake not to overwrite content of aps dictionary. We don't look inside this property anywhere in the code (apart from the "alert" field, but this one was actually handled). Nevertheless I decided to overwrite this since it might be good in long term not to pass to JS anything that contains encrypted data.

ashoat requested changes to this revision.May 17 2023, 12:35 PM

Confused about the most recent revision

native/ios/NotificationService/NotificationService.mm
191 ↗(On Diff #26612)

What is this thread-id field and why do we need to set it?

201 ↗(On Diff #26612)
  1. Why are we setting this here? Was badge in aps initially? If yes, then couldn't this be handled by iterating through aps and decrypting the data? If no, then why are we adding it to aps after decryption?
  2. Wouldn't this be handled by the code on line 220? (Which I am also confused about... see comment below)
219–221 ↗(On Diff #26612)

I'm confused why we're setting the payloadKey in aps here, when the original key was in userInfo.

I can see that you're checking if the key is ALSO in aps before setting it, but I don't think we can assume that the value for the key in userInfo is the same as the value for the key in aps.

I also don't understand WHY we want to do this... can you explain?

This revision now requires changes to proceed.May 17 2023, 12:35 PM
marcin added inline comments.
native/ios/NotificationService/NotificationService.mm
191 ↗(On Diff #26612)

When you set the threadId property on apn.Notification object (we do this in our code) it is stored in aps property under thread-id key: https://github.com/parse-community/node-apn/blob/master/lib/notification/apsProperties.js#L124. Then iOS assigns this value to UNNotificationContent.threadIdentifier, but the entire aps dictionary is still present userInfo attribute of UNNotificationContent.

Decrypting UNNotificationContent.threadIdentifier is enough to display the right content to the user. Generally we don't any of this if(mutableAps ... statements (apart from the one with @"alert" which is crucial) to display the right content to the user and to store the right data in SQLite. However it just felt odd to propagate notification that still contains encrypted data to the application memory.

201 ↗(On Diff #26612)

Why are we setting this here? Was badge in aps initially?

It wasn't. parse-apn adds badge to aps by default: https://github.com/parse-community/node-apn/blob/master/lib/notification/apsProperties.js#L67, but it does not allow to add it as a string. However we must add it as a string since it is encrypted. Therefore in D7813 I am setting encrypted badge as a string in payload property (which is transformed to userInfo when delivered). I decided it would be wise to add it back to aps after it is decrypted and cast to numerical value again.

Wouldn't this be handled by the code on line 220? (Which I am also confused about... see comment below)

Theoretically we could handle it there, but we would need if(payloadKey -- "badge") since:

  1. We cast it to number.
  2. We assign it to UNNotificationContent.badge attribute.

We can either handle it as an if statement inside a loop or handle it earlier and omit it inside a loop.

219–221 ↗(On Diff #26612)

This is basically for body , title and prefix keys. We assign them directly to apn.Notification object as well as to its payload property:
https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L648-L670

Assigning directly to apn.Notification puts those keys in aps dictionary. Therefore those properties are present in both userInfo and aps. I consider it unwise to have decrypted data in userInfo and encrypted data in aps if those are the same data.

Note: we cannot iterate over both (userInfo and aps) independently since olm throws if we try to decrypt the same data twice.

ashoat requested changes to this revision.May 19 2023, 8:30 AM

Back to your queue for one question

native/ios/NotificationService/NotificationService.mm
191 ↗(On Diff #26612)

Thanks for explaining! Let's keep this as-is – your reasoning makes sense to me

201 ↗(On Diff #26612)

Thanks for explaining! Let's keep this as-is – your reasoning makes sense to me

219–221 ↗(On Diff #26612)

Thanks for your explanation. Just one question:

We assign them directly to apn.Notification object as well as to its payload property:
https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L648-L670

In that code snippet, I can see where we assign body, title, and prefix to notiifcation.payload. And I can see where we assign merged to notification.body. But I can't see where we assign body, title and prefix directly to the apn.Notification object, nor can I see where we assign merged to notification.payload. Perhaps I am missing something, but it seems to me that for each property, we only assign it to one place in apn.Notification.

Perhaps either @parse/node-apn or Apple are "duplicating" this for us, similar to how thread-id appears in two places?

This revision now requires changes to proceed.May 19 2023, 8:30 AM
marcin added inline comments.
native/ios/NotificationService/NotificationService.mm
219–221 ↗(On Diff #26612)

Here we assign 'rest' to notification.payload. https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L652-L663. 'rest' comes from notifTexts object that is of type ResolvedNotifText. This type is defined as:

export type ResolvedNotifTexts = {
  +merged: string,
  +body: string,
  +title: string,
  +prefix?: string,
};
native/ios/NotificationService/NotificationService.mm
219–221 ↗(On Diff #26612)

Ignore the previous comment. I didn't read the question well.
I went through the code of parse-apn once again and I habve to admit that there are no signs that data assigned to payload atumatically gets duplicated in aps. Probably the if statement at the end of the for loop should be deleted. I will run a session in XCode debugger to examine the content of aps dictionary here.

native/ios/NotificationService/NotificationService.mm
219–221 ↗(On Diff #26612)

Screenshot 2023-05-22 at 14.17.35.png (626×1 px, 218 KB)

That is how the aps dictionary looks like. Therefore we can safely remove this if statement from the loop.

Remove unecessary if (it was a dead code)

This revision is now accepted and ready to land.May 22 2023, 10:47 AM

(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)

Blob decryption in one pass

native/ios/NotificationService/NotificationService.mm
81 ↗(On Diff #26894)

In the "Blob decryption in one pass" update you stopped setting the succesfullyDecrypted field, so this check is meaningless. Best to get rid of it (also there's a typo there)

native/ios/NotificationService/NotificationService.mm
81 ↗(On Diff #26894)

Thanks for catching this! I think we should rather bring back setting succesfullyDecrypted field.