This differential impements methods in NSE to decrypt encrypted notification.
Details
- Repeat steps from parent differential test plan
- Ensure this time the encrypted notification is the one that is sent, not the original.
- Ensure decrypted notification is displayed on the device.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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. |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
196 ↗ | (On Diff #26555) | Should probably be keysToOmit? |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
196 ↗ | (On Diff #26555) | Agree |
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.
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) |
|
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? |
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) |
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.
Theoretically we could handle it there, but we would need if(payloadKey -- "badge") since:
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: 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. |
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:
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? |
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. |
native/ios/NotificationService/NotificationService.mm | ||
---|---|---|
219–221 ↗ | (On Diff #26612) | That is how the aps dictionary looks like. Therefore we can safely remove this if statement from the loop. |
(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)
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. |