Page MenuHomePhabricator

Prepare to conditionally process rescinds in NSE.
ClosedPublic

Authored by marcin on Mar 1 2023, 3:39 AM.
Tags
None
Referenced Files
F3407193: D6917.id23404.diff
Wed, Dec 4, 4:34 AM
F3407192: D6917.id.diff
Wed, Dec 4, 4:34 AM
F3407191: D6917.id23329.diff
Wed, Dec 4, 4:33 AM
F3407190: D6917.id23674.diff
Wed, Dec 4, 4:33 AM
F3407188: D6917.id23760.diff
Wed, Dec 4, 4:32 AM
F3407185: D6917.id23754.diff
Wed, Dec 4, 4:32 AM
F3407104: D6917.diff
Wed, Dec 4, 4:09 AM
Unknown Object (File)
Tue, Nov 19, 10:00 AM
Subscribers

Details

Summary

This differential is intended to provide reasonable test plans for further differentials in this stack. On its own it is a no-op, but if we remove codeVersion > 1000 condition then both visible notifications and rescinds start to be processed by NSE. This way we have a simple
method to test further differentials where we actually do some notifications processing in NSE and we are not introducing any breaking changes in the middle of the stack. Final differential in the stack will remove this condition and replace it with particular release build number.

What this differential does is to structure rescind as if it was a regular notification. This is required to launch NotificationService to process it. Using notifications filtering capability we make sure this artificial content is not displayed to the user (matter of next differential).

Test Plan

Remove codeVersion > 1000 condition. Visible notifications will start to be persisted in temporary flat file in NSE. Send some notifications, and watch in the debugger persisted notifications payloads in moveMessagesToDatabase method of AppDelegate. Send a rescind. It
won't work but you will see a strange notification with PLACEHOLDER as body.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Mar 1 2023, 3:55 AM
marcin edited the summary of this revision. (Show Details)
keyserver/src/push/rescind.js
172 ↗(On Diff #23329)

Is this necessary? What happens if we omit it? Is this what you were referring to in the diff description?

What this differential does is to structure rescind as if it was a regular notification. This is required to launch NotificationService to process it.

keyserver/src/push/rescind.js
172 ↗(On Diff #23329)

I was testing it without artificial body property and it worked. However I once came across this article by apple: https://developer.apple.com/documentation/usernotifications/modifying_content_in_newly_delivered_notifications?language=objc, they describe a case of decrypting encrypted notification in NSE. The payload for notification contains body property with artificial content in aps dictionary even though it is not used. Moreover this doc states:

The payload must include an alert dictionary with title, subtitle, or body information.

So I am afraid that sending notifications with alert field without anything that could have been displayed as an alert to the user might be considered as a violation for Apple.

keyserver/src/push/rescind.js
172 ↗(On Diff #23329)

Your reasoning makes sense, but I think you are setting alert to a string, rather than a dictionary here. See the code here

keyserver/src/push/rescind.js
172 ↗(On Diff #23329)

It is specified here, that alert might be string or dictionary: https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CreatingtheNotificationPayload.html. I was thinking more general: Apple wants some content that can potentially be displayed to the user.

keyserver/src/push/rescind.js
172 ↗(On Diff #23329)

But the doc you linked says "The payload must include an alert dictionary with title, subtitle, or body information."

It doesn't appear that a string qualifies as a dictionary?

172 ↗(On Diff #23329)

I think it'd be fine to remove this entirely if it works without it. But if we're including it just to match the docs, we should make sure we're actually matching the docs.

marcin edited the summary of this revision. (Show Details)

Remove artificial body property

atul added inline comments.
keyserver/src/push/send.js
637–641 ↗(On Diff #23404)

It would be helpful to add a comment here explaining what's going on. The pattern of using really high codeVersion checks is probably familiar to most developers on the team, but the % 2 even/odd condition probably isn't.

This revision is now accepted and ready to land.Mar 5 2023, 12:05 PM

Add comment explaining even release pattern