Azure / azure-notificationhubs-dotnet

.NET SDK for Azure Notification Hubs
MIT License
70 stars 122 forks source link

AppleNotification lack support for new requered iOS 13 header apns-push-type #88

Closed Hatju closed 5 years ago

Hatju commented 5 years ago

https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns

According to Apples documentation above iOS 13 devices requires a "apns-push-type" header to be present. And leaving it out may cause delayed delivery or total loss of delivery.

So the AppleNotification class should add the below by default. AppleNotification.Headers["apns-push-type"] = "alert"

marstr commented 5 years ago

Howdy @Hatju,

Thanks for filing this issue, we're aware of this change and taking action to make sure that we at least support passing along this header should it be provided. We're also investigating the possibility of another mitigation, more information will follow as we learn more.

marstr commented 5 years ago

Update:

We've decided that instead of adding this header in the SDKs, we'll be adding it server side. The advantage of doing this, of course, is that customers won't need to change their code at all to ensure that this header is included as appropriate. Further more, we won't need to choose a default between alert or background, as we can inspect the contents of the notification to infer the correct value.

Before this issue is closed, we will add validation into the SDK so that if the header is included, that it is at least set appropriately.

marstr commented 5 years ago

Before this issue is closed, we will add validation into the SDK so that if the header is included, that it is at least set appropriately.

Looks like I spoke to soon on this part. We decided that we'll just pass through any values that are handed to us. That way if Apple changes their semantics, updating your NH SDK is one less thing you need to think about.

With that, I believe it's safe to close this issue! If there are future concerns around apns-push-type, please feel free to comment here or open a new issue.

AntRemo commented 5 years ago

@marstr Thanks for your replies.

Is there any documentation or example of how to patch our template to ensure the header is properly updated?

For example: Our template is like { "aps": { "content-available": 1, "DataMessage": "$(DataMessage)" } }

How would we properly update this to set the apns-push-type header?

Thanks 🙏

Hatju commented 5 years ago

I dont use templates, I create the json my self, so this is what I do. But I'm sure i spotted that you can add the headers even with templates in the documentation.

azureNotification = new AppleNotification(json) { Priority = 10, Expiry = DateTime.UtcNow.AddMonths(1) }; azureNotification.Headers["apns-push-type"] = "alert"; // Requered by iOS 13 and not yet supported by AppleNotification

await hubClient.SendNotificationAsync(azureNotification, tags);

The Priority and Expiry also creates headers but her I use the support in the lib to do it. The Headers["apns-push-type"] is the raw way to add a header.

Especially the Expiry is important. Without it apple will only try to deliver your notification one time. If the phone is not connected to APNS when it happens the notification is lost. Adding the Expiry will make APNS try to delivere it for the next month.

AntRemo commented 5 years ago

Thanks @Hatju for the detailed response.🙏

Hi @marstr I tried to apply @Hatju response above, however, I am using the InstallationTemplate class which does not have a Priority property.

How can I achieve the same result?

Many thanks 🙏

marstr commented 5 years ago

Howdy @AntRemo,

The inference that we do server-side waits to inspect the payload until after templates have been resolved. For that reason, you shouldn't need to take any action.

AntRemo commented 5 years ago

Thanks for your reply @marstr 🙏

I think I understand what you are saying, however, where would I assign the Priority = 10 using the InstallationTemplate class as it does not have a Priority property?

I believe I may need to make some changes to the portion of code responsible for registering the template to use the platform specific templates. It appears that AppleTemplateRegistrationDescription exposes a Priority property.

nerocristallo commented 4 years ago

Can you please clarify what "infer" means in this context?

Further more, we won't need to choose a default between alert or background, as we can inspect the contents of the notification to infer the correct value.

Which is the content check performed by the server? Does it check only the presence of the "content-available": 1 and the missing of the "alert" or there is also a check of the "alert" content? Is this change already deployed on server?

marstr commented 4 years ago

Our changes have already been deployed and should be live.

I'm a little hesitant to add too much detail here, in case we need/want to change our logic in the future. I would just advise to always have a valid APNS JSON body, with correctly populated content-available/alert properties.

AntRemo commented 4 years ago

Hi @marstr

I am using the InstallationTemplate class with the following APNS template

{ "aps": { "content-available": 1, "DataMessage": "$(DataMessage)" } }

Unfortunately, it is not working on an iOS 13.x device.

I have updated the logic to extract the device token using this approach https://stackoverflow.com/a/58028222/3260560 and it producing a device token which is in the same format as pre-iOS 13 tokens.

I also tried setting the InstallationTemplate.Headers property but receive the following error Template 'hello is invalid: 'Headers' only acceptable for WNS and MPNS...

Hoping you can help resolve my issue. Thanks!🙏

gertgjoka commented 4 years ago

I am having the same issue, I have the content-available in aps body and no alert set but no background push is hitting the device. Also as @AntRemo mentioned setting the headers "apns-push-type": "background", "apns-priority": "5" in the InstallationTemplate object will result in an exception being thrown from the Push sdk: Installation validation failed with following error(s): Template 'xx is invalid: 'Headers' only acceptable for WNS and MPNS. Any ideas on how to solve this? We have a production app heavily impacted by this. Thanks

marstr commented 4 years ago

Howdy @gertgjoka and @AntRemo, I've opened #96 to track investigation into this. Mind continuing conversation about this issue there?

sadgit commented 4 years ago

By adding the apns-push-type server side is causing notifications to fail.

Please ensure that developers targeting APNS are responsible for setting the headers and that these are faithfully respected all through the system as a mis-match will cause a message to fail without obvious reason.

NotificationHubs should not adorn the message in anyway as this changes the implementation of people's existing code bases and breaks VOIP notifications specifically.

We are using native notifications in-place of templates in order to achieve fine-grained control over platform specific quirks - and altering my notification in-flight has caused some side-effect that I cannot diagnose.

We need to be able to see what come out the other-side of a notification hub to ensure that it hasn't made some mistake in inferring my intention.