Page MenuHomePhabricator

Send iOS push notifications as visible pushes instead of silent
Closed, ResolvedPublic

Description

According to Apple documentation and confirmed in our testing, sending silent push notifications are heavily throttled by iOS and thus highly unreliable. We would like to have the push notifications service send to iOS as a visible push notification instead of silent.

On iOS we have the ability to intercept these notifications before they are displayed, fetch the notifications API and modify the push text before it is displayed to the user, via a Notifications Service Extension. There is no concern that the user would see "checkEchoV1". Even if the notifications API call failed or timed out, we can modify the text to display default text.

Event Timeline

Currently the alert msg when debug is enabled is something like:

`Message sent at ${transactionStart}`

Should we change that to something like checkEchoV1 so its easier for you to match the messages or its fine as it is?
In order to send a visible notification we just need to enable the debug topic for a specific app ID on the push notifications service configuration (already done on beta)

Some context for clarity:

  • When we first built the push notification service it was supposed to be sending silent notifications where the clients would handle after querying the MW notifications API
  • It looks like this doesn't work well on iOS
  • We already have visible notifications using the alert parameter for debug purposes when an App ID is added in the list of the debug_topics
  • This was built for demo purposes using a demo app

An easy way forward would be what I described on the previous comment, to reuse the demo functionality for iOS apps which pretty much does exactly what we want:

  • Makes the push notification mutable so client can change it (parameter mutableContent=True)
  • Adds a placeholder visible message with a timestamp (parameter alert=<msg>)

@MSantos thoughts?
The only think that I find space for improvement is the fact that it was used for demo purposes so we need to somehow make it more clear that its a feature that we use for that specific purpose in production.

@Tsevener Do you need something specific to match the notifications or the payload with { data: { type: "CheckEchoV1" } } is enough ?

@Jgiannelos we were planning on working with only "CheckEchoV1", so that payload should be fine. I figured you can't send anything more identifying for privacy purposes, but I'd be happy to be corrected if I'm wrong 🙂

An array of identifiers (wiki + ID) that represent which notifications from the API are a part of the push would be great, or timestamps that we can match up with the response would be cool too. But like I said we've already worked it out in our heads how to work around a "CheckEchoV1" payload. We also haven't done extensive development & testing yet so maybe we revisit with something more identifying if we can confirm that CheckEchoV1 triggers too many false alarms.

@Jgiannelos it also turns out we can suppress a visible notification on our end with some special entitlements. It'll require some rejiggering with our build server but that's our plan B if notifications feel too wonky after testing.

We can ask security/privacy team about adding more identifiers but the general idea was to share as little information as possible to 3rd party services. I don't think its unreasonable though to add the wiki that triggered the event as part of the payload. I am not sure which ID you are referring to.

@Jgiannelos I was thinking something we could match up with the id or timestamp fields in the notifications object returned from the API

{
                    "wiki": "enwiki",
                    "id": 220783287,
                    "type": "edit-user-talk",
                    "category": "edit-user-talk",
                    "section": "alert",
                    "timestamp": {
                        "utciso8601": "2021-06-18T21:33:29Z",
                        "utcunix": 1624052009,
                        "unix": "1624052009",
                        "utcmw": "20210618213329",
                        "mw": "20210618213329",
                        "date": "18 June"
                    },
                    "title": {
                        "full": "User talk:TSevener (WMF)",
                        "namespace": "User_talk",
                        "namespace-key": 3,
                        "text": "TSevener (WMF)"
                    },
                    "agent": {
                        "id": 35425575,
                        "name": "Tsevener"
                    },
                    ...

We can ask security/privacy team about adding more identifiers but the general idea was to share as little information as possible to 3rd party services.

Yeah that makes sense - let's just stick with only your "CheckEchoV1" payload above, since I don't think the wiki (without additional timestamps or ids) will be much better. We have options on our side to work around it if it becomes an issue. Just curious, is it possible a single push notification can represent new notifications from multiple wikis?

@Jgiannelos sorry if my last question caused any blocking, we are definitely fine with the payload being { data: { type: "CheckEchoV1" } }. Is there anything else you need from us on this? We're ready to test again against the beta cluster / sandbox flow whenever this and https://phabricator.wikimedia.org/T285417 is set up. It would be excellent if we could get these rolling within the next week or two, because then we'll need to work out the QA production testing flows soon after. They will be entirely blocked on that due to them needing to test in TestFlight.

I don't think that there is anything else we need from you for that. Lets see how testing goes on beta.

@Jgiannelos Update - confirmed I am able to see push notifications now via beta cluster / sandbox in our experimental app! Thanks! Just a quick question - I noticed it takes the format "Message sent at nnnnnnn".

IMG_0552.PNG (1×750 px, 3 MB)

Can we change that to checkEchoV1 for this flow? Just for the sake of matching what production will likely be.

I don't think we can change this only for the beta deployment if thats what you mean. But we definitely can change the debug message. Or make it configurable.

@Jgiannelos oh I just meant, can we change the sandbox/beta deployment message it to whatever the production push payload is planning on being, I would rather them match so we don't rely on anything that is only in sandbox. Will a production push be "Message sent at nnnnn" or "checkEchoV1"?

@Tsevener it looks like we are confusing what payload means for the APNS notification. If you look in the code [1], you are going to see that we set notification.payload with structured data that contains the information checkEchoV1.

Because of the silent notification unreliability what we enabled for you to test the notification properly is to give an alert by setting notification.alert which was developed by us as a debug mode.

My question is, do you have access to the data sent in notification.payload or only the content of notification.alert?

[1] https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/services/push-notifications/+/refs/heads/master/src/outgoing/apns/apns.ts#129

Change 713844 had a related patch set uploaded (by Jgiannelos; author: Jgiannelos):

[mediawiki/services/push-notifications@master] Stop support for debug mode for APNS

https://gerrit.wikimedia.org/r/713844

I think that overall the debug mode has caused confusion. I sent a patch to deprecate support. Here is my thinking around it:

  • Debug mode was introduced for demo purposes before push notifications services was integrated to the apps.
  • This was a workaround for the fact that production was supposed to use silent notifications but we needed a way to verify that notifications are sent for development purposes.
  • It showed a visible debug alert with a timestamp for specific topics only (based on configuration)
  • iOS app is not going to use silent notifications in production because they don't work as planned but instead override the alert message of the APNs payload

Now that we have actual support for push notifications in the iOS app and the notifications are not silent I don't see the need for a custom debug message or a demo app.

@MSantos sorry for the delay on this - yes, it's possible we can still see the data sent in notification.payload, I still need to breakpoint and inspect. So it may be fine, the "Message at" just threw me off since I was expecting "checkEchoV1". That being said I do think it makes sense to have the prod notification and sandbox notification match - I think @Jgiannelos is right that a separate "debug mode" functionality is no longer needed. So to be clear - both the prod flow and sandbox/beta cluster flow need to send visible push notifications, and both should look identical from our end, whether it's "checkEchoV1" or something else.

Change 713844 merged by jenkins-bot:

[mediawiki/services/push-notifications@master] Deprecate debug mode for APNS

https://gerrit.wikimedia.org/r/713844

@Jgiannelos @MSantos heads up, it looks like something is broken in the sandbox flow now, perhaps the last change did something? Triggering a push on beta labs no longer seems to push a notification to the org.wikimedia.wikipedia.tfalpha Experimental app on device.

Jgiannelos claimed this task.