fix: FCM push notification payload incompatible with Parse Android SDK

[mman]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue

Issue Description

Closes: #237

Sending push notifications to Android phones does not work anymore because data are converted and sent out differently using FCM.js than they were using the GCM.js.

This means that Parse Android SDK is unable to parse the message correctly, and deliver it to user specified broadcast receiver, silently dropping any received messages.

Approach

Parse.Push.send was initially built around support for iOS, and later added support for Android, and Android SDK.

Parse.Push.send accepts a dictionary that we call requestData.

Historically sending push notifications to Apple meant passing in an alert: "Push Notification" key in requestData. Later when Apple added support for push notification titles and subtitles, push adapter offered two ways how to specify title and subtitle. Either in top level requestData, like this:

{
  "alert": "Push Notification",
  "title": "Title",
  "subtitle": "Subtitle"
}

or via nested alert dictionary inside of requestData like this:

{
  "alert": { body: "Push Notification", "title": "Title", "subtitle": "Subtitle" }
}

In both of these cases, such requestData would end up being converted to the payload accepted by the APNS always getting the second form with nested alert dictionary.

The Android side of things did not do any special formatting of user data, and would simply use whatever data user passed into requestData and just JSON encode them into string. For client side processing, Android SDK requires to specify pushId and time. The final Android payload then looked like this:

{
  "push_id": "anything you wish",
  "time": "2024-05-06T15:06:06Z"
  "data": "{ ... data passed to Parse.Push.send JSON encoded into string ... }"
}

This PR adjusts the code to provide support for consuming nested alert dictionary as before, and it modifies the Android path to properly encode data into JSON string and decorate the payload correctly with push_id and time to receive notifications on devices currently in circulation with existing versions of Parse Android SDK.

TODOs before merging

• Add tests
• Add entry to changelog
• Add changes to documentation (guides, repository pages, in-code descriptions)

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!

[mman]

@jimnor0xF I have intentionally left the tests to fail so that we can discuss what is right and what is wrong.

[jimnor0xF]

@mman
Should be OK. Tested myself, although not with the Parse Android SDK.

I guess rawPayload does not work with the Android SDK either, unless a user put those keys in themselves. Not sure if we want/need to support that out of the box.

[jimnor0xF]

@mtrezza
Can you chime in on the above comment?

Should rawPayload be supported out of the box for the Parse Android SDK? In that case we basically need to add those time/push_id keys if "android" is present in the payload as well.

Personally, I don't see the need, since I'd prefer it to not be dependent on some client side implementation.

[mtrezza]

@mman What do you think?

Not sure I fully understand what the issue is, could someone explain again, please? I'd just keep in mind that developers cannot easily change the (custom) push adapter implementation in all their rolled out apps, so whatever we can do here to make life easier for developers will likely be appreciated.

[mman]

@mtrezza @jimnor0xF I will try to explain what I have learned over the years:

1. Apple APNS service defines an (let's call it) API that specifies what JSON payload you can push to apple servers, and what will happen on device. This has been evolving over time quite randomly. For example initially we only had alert: String, then alert migrated to dictionary alert: {title: String, body: String}, mutable-content: 1, content-available: 1, etc... It's also quite strict as in mutable-content must contain number 1, nothing else, otherwise it fails in random ways.

2. Google Cloud Messaging and later Firebase Messaging defines their own API for JSON payload to target Android devices. Android seems to be me much less strict and basically what you give it, will get sent to device where you reassemble as you wish... GCM did support arbitrary JSON it seems, whereas FCM insists on flat JSON.

3. Since FCM also support iOS, it further complicates that API in that it supports general payload, as well as platform specific payload for Android, and platform specific for iOS. All this mangled together in large JSON. What is general payload for? I have no idea, but I assume FCM can send more data to more destinations - like web for example.

4. Historically (I think) Parse Server Push Adapter tried to invent its own payload API that you pass to Parse.Push.send and that gets later converted by the adapter to appropriate platform specific API that gets sent via push adapter to the device itself.

Now point 3. is what @jimnor0xF calls rawPayload in that you directly specify what should be sent to FCM. Apple specific subset of rawPayload should be what 1. expects and that should work fine.

But because of 4. we have our own iOS and Android payload API that gets passed to Parse.Push.send and then converted to whatever is required by 1., 2., 3.

More over, Parse Server + Parse Push Adapter agreed with Parse Android SDK in the past that each Android push will have push_id and time always injected so that Parse Android SDK can use it despite it being largely a historic artefact that is probably no longer needed...

For now, I see one remaining problem:

Parse Server Push Adapter, should probably still support expected Parse.Push.Send API and convert that to whatever should be sent out. Since this format was never formalised it is pretty difficult to cover it with tests.

One issue I identified is that in the past we supported alert: { title: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." }, and that no longer works however it can be converted to "alert: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." (reason being FCM requires "flat payload")

Second issue is that when you send to Android we misplace the push_id and time (subject of this PR).

More issues perhaps may pop up as more people migrate to push adapter 6.x.

I will update this PR and will test it more, and try to at least open issues against thing that no longer work - but that have a reasonable workaround - so that we can mention them in README.

[mtrezza]

Thanks for the explanation. What is the suggested solution, with the least impact for developers? Mind that we have only 4 weeks left until Google will decommission the legacy API.

Let's assume the viewpoint of a developer for an app that currently sends pushes to Apple (directly to APNS via node-apn) and Android (directly to FCM legacy API via node-gcm) devices. For APNS there should be no change required, neither client nor server side. For Android there should be:

• a) No change required in Parse.Push.send, since we control this API and can convert the provided payload if necessary.
• b) No change required on the client app side, since the rollout of any change would take too much time.
• c) A change required in the Parse Server push adapter config to switch to the new FCM API and provide auth keys, etc.

So we'd need to transform the payload that is sent to the Parse Android SDK push module so that it works properly. For apps that subclassed the ParsePushBroadcastReceiver and implemented custom payload parsing, that means that the payload that arrives at the device must not be affected by the FCM API change.

Does that make sense and is that technically possibly?

Or does the new FCM API send a payload to the device that we cannot fully influence and therefore the Parse Android SDK cannot interpret the push correctly? That would be the worst case scenario. For example @mman wrote:

One issue I identified is that in the past we supported alert: { title: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." }, and that no longer works however it can be converted to "alert: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." (reason being FCM requires "flat payload")

If we change this structure, will the Parse Android SDK still interpret that correctly? What about apps that extend the ParsePushBroadcastReceiver and parse the push payload manually, expecting that the payload is not flat and that alert has the nested keys title and body?

[mman]

@mtrezza You summarized it nicely. My expectation is to simply add firebaseServiceAccount to Android push config and everything must continue to work.

I think it is technically possible and I will try to work on it next Monday. The issues I identified are:

• Support nested alert dictionary
• Encode Android payload into a string as before
• Properly add push_id and time

once these three are addressed it should work. I assume everybody affected will start migrating over the next few weeks so cutting 6.0.1 before the end of next week should work.

[mtrezza]

It would be amazing if we could get this done by next week. I believe this is an all-hands on deck issue, but few people are actually aware of this challenge. Once we have a release next week, we'll post this on our social media to invite more developers to test this out. Thanks @jimnor0xF and @mman for your efforts so far!

[mman]

@mman @jimnor0xF So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the firebaseServiceAccount configuration option for Android.

[mtrezza]

So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the firebaseServiceAccount configuration option for Android.

@mman Great - I understand you tested this out with Android and Apple push notifications on real devices?

[mman]

So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the firebaseServiceAccount configuration option for Android.

@mman Great - I understand you tested this out with Android and Apple push notifications on real devices?

I tested on real Android devices, where push for Android goes via FCM.js. For iOS push I do not use Firebase at all, but I have not touched and iOS related code.

[mtrezza]

@jimnor0xF Could you review as well before we merge?

[charles-ramos]

Hello guys, I raised this PR #246 with the compatibility for nested objects. Could you please leave your comments?

[mtrezza]

So I'll wait with merging until we have figured out what #246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?

[mman]

So I'll wait with merging until we have figured out what #246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?

I'm honestly not sure, since I started using push on Apple first and then added Android devices, I was only able to uncover the missing nested alert: {body:, title:} issue. What @charles-ramos has added is additional support for directly sending Android notification object. I am not 100% sure, but I think that Parse Android SDK never handled receiving Android notification directly, and always extracted user data from only three parameters: pushId, time, and data.

See here: https://github.com/parse-community/Parse-SDK-Android/blob/a08c5d1594b3a083671b493804eb1970c1ce6441/parse/src/main/java/com/parse/PushRouter.java#L105

So I need @charles-ramos to answer whether he intents to support new functionality, or whether migration to latest FCM broke something for him...

[charles-ramos]

So I'll wait with merging until we have figured out what #246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?

I'm honestly not sure, since I started using push on Apple first and then added Android devices, I was only able to uncover the missing nested alert: {body:, title:} issue. What @charles-ramos has added is additional support for directly sending Android notification object. I am not 100% sure, but I think that Parse Android SDK never handled receiving Android notification directly, and always extracted user data from only three parameters: pushId, time, and data.

See here: https://github.com/parse-community/Parse-SDK-Android/blob/a08c5d1594b3a083671b493804eb1970c1ce6441/parse/src/main/java/com/parse/PushRouter.java#L105

So I need @charles-ramos to answer whether he intents to support new functionality, or whether migration to latest FCM broke something for him...

The idea is to keep everything compatible with the current code (GCM) that uses "data" for sending push notifications for Android. Right now, in case I want to send push notifications to everyone (android and iOS), I must use both "data" and "notification". With the PR I raised, it will be possible to keep using "data" for both.

[charles-ramos]

During my tests using the changes mentioned in this branch, I got some results here:

First of all, I tested this code for sending push notifications:

Parse.Push.send(
    {
      where: {},
      data: {
        alert: "Alert Push Notification",
        badge: 123,
        title: 'Title data'
      }
       notification: {
         title: 'Title Notification',
         body: 'Test body'
       },
    },
    { useMasterKey: true },
  );

I got the following error:

  RangeError: Invalid time value
    at Date.toISOString (<anonymous>)

Due to this line:

parse-server-push-adapter/src/FCM.js

Line 265 in 8b93e22

time: new Date(timeStamp).toISOString(),

After adding a console.log() to see what was my androidPayload, I got this:

{
 android: {
  priority: 'high',
  notification: { title: 'Title Notification', body: 'Test body' }
}
}

Then, I removed and tested it, but I did not receive the notification for Android (only for iOS, with the content from the data).

If I tried sending a push notification without notification, my androidPayload was like this:

{ android: { priority: 'high' } }

Which seems to be expected.

Testing via Dashboard, I did not receive any notifications for android, as the dashboard only sends push notifications through alert: https://github.com/parse-community/parse-dashboard/blob/5.3.0/src/dashboard/Push/PushDetails.react.js#L42

Is there anything that I'm not concerned about?

As a reference, I used the code described in the documentation: https://docs.parseplatform.org/js/guide/#sending-pushes

And used the push options listed here: https://docs.parseplatform.org/js/guide/#sending-options

[breadluvr]

I'd like to chime in and share details about our current structure for sending push notifications. Here’s how we format our payload with a nested data object:

data: {
  alert: "foo",
  badge: 1,
  data: {
    notification_id: 1,
    path: "/bar"
  },
  sound: "*"
}

After updating to parse-server@7.1.0-alpha.6 and using the firebaseServiceAccount for authentication, we encountered the following error in our server logs:

ERR! parse-server-push-adapter FCM error sending push: Error: android.data must only contain string values

This error only shows when we send pushes manually through the server console. However, when our system sends pushes, they seem to fail silently without any error messages appearing in the logs.

[jimnor0xF]

I'd like to chime in and share details about our current structure for sending push notifications. Here’s how we format our payload with a nested data object:

data: {
  alert: "foo",
  badge: 1,
  data: {
    notification_id: 1,
    path: "/bar"
  },
  sound: "*"
}

After updating to parse-server@7.1.0-alpha.6 and using the firebaseServiceAccount for authentication, we encountered the following error in our server logs:

ERR! parse-server-push-adapter FCM error sending push: Error: android.data must only contain string values

This error only shows when we send pushes manually through the server console. However, when our system sends pushes, they seem to fail silently without any error messages appearing in the logs.

@mman
This problem should be fixed with this PR, with the following line, right?

https://github.com/parse-community/parse-server-push-adapter/pull/238/files#diff-033bf89e47e1c50c0218ba14fbd27977eefd8d2ae8bdcdc126ab73f7e2b09e63R266

@astroblemeal
Could you test with the code in this PR?

[jimnor0xF]

@charles-ramos

Strange, looks like the JSON body in your example is missing a comma though. Not sure if that has anything to do with it. Did that payload work before with the legacy FCM (GCM.js)? Can you try with this payload?

  const q = new Parse.Query(Parse.Installation);
  q.containedIn('user', userIds);

  await Parse.Push.send(
    {
      where: q,
      data: {
        alert: 'Test alert',
        badge: 666,
        title: 'Test title'
      },
      notification: {
        title: 'Test title',
        body: 'Test body',
      },
    },
    { useMasterKey: true },
  );

Could you share some info about what you are running on your Android device? Are you using the Parse Android SDK to handle pushes?

On the topic of using the notification key. Correct me if I'm wrong, this was never a "well-known" or documented key, even back when we used legacy FCM (GCM.js). You could only know it existed by reading the code.

I have to use the notification key since I do not use the Parse Android SDK for handling pushes. You only use the notification key if you use Firebase Cloud Messaging SDK on the client side, or perhaps if you have a customised version of the push handlers to handle that key (?). If you use the Parse Android SDK, using the data key only for Android should work.

And like your PR mentioned #246, you have to specify notification and data keys to get it to work for both Apple and Android.

But this was the case in the past as well. At least in my experience with a customised setup, and according to the code in GCM.js which does not merge the data and notification keys.

So I do not think we are dependent on #246 to fix some backwards incompatibility issue. Please correct me if I'm wrong though.

For people that intend to use the Firebase Cloud Messaging SDK on the client side to handle pushes or that are using a customised setup, you are better off using the rawPayload option I added here for better control.

[mtrezza]

So I'll go ahead and merge this PR and then @charles-ramos can rebase his PR on the latest commit. This may make it easier to test everything out. Please feel free to continue the discussion here, despite the PR being merged.

[parseplatformorg]

🎉 This change has been released in version 6.1.1

[mtrezza]

After merging this PR there is a conflict in #246 (comment); if @charles-ramos could resolve this, then we can get a clearer picture of what that PR adds after this PR.

[breadluvr]

@jimnor0xF It works keeping the same payload structure with the nested data object, and setup as before using parse/push-adapter@"6.1.1"! 🥳

Any idea when these changes will be released in parse-sever ? :)

[mman]

@mtrezza Looks like parse server itself is has hard pinned the dependency on push adapter 6.0.0, and despite my custom build depending on 6.2.0 npm was still pulling in 6.0.0. I needed to manually up the dependency to 6.2.0 in forked parse server to actually get latest version with the fixes. I'm not versed in npm that much to see what is behind it, but are we sure that simple npm up will pick up the latest push adapter?

[mtrezza]

It's actually much simpler. See Installation, Configure and Bundled with Parse Server. I rewrote the README yesterday to explain this. But we should make a Parse Server release with the new adapter as well before June.

[breadluvr]

It's actually much simpler. See Installation, Configure and Bundled with Parse Server. I rewrote the README yesterday to explain this. But we should make a Parse Server release with the new adapter as well before June.

hmm, not fully getting it at first glance. I got around it by also adding push-adapter@"6.1.1 to my project and resolving it in my package json while still using "parse-server": "^6.0.0".

  "resolutions": {
    "@parse/push-adapter": "6.1.1"
  }

[mtrezza]

1. Install push adapter:

   npm i -E  @parse/push-adapter@6.2.0

2. Import push adapter where you init Parse Server:

   import { ParsePushAdapter } from '@parse/push-adapter';
   
   // For CommonJS replace the import statemtent above with the following line:
   // const ParsePushAdapter = require('@parse/push-adapter').default;

3. Modify push options; note that the object under push is now nested under push.adapter:

   const parseServerOptions = {
     push: {
       adapter: new ParsePushAdapter({
         ios: {
           // Apple push options
         },
         android: {
           // Android push options
         },
         web: {
           // Web push options
         },
         expo: {
           // Expo push options
         }
       })
     }
     // Other Parse Server options
   };