Unable to send push notifications to Android after updating to 6.0.0

mman commented 2 months ago

New Issue Checklist

[x] I am not disclosing a vulnerability.
[x] I am not just asking a question.
[x] I have searched through existing issues.
[x] I can reproduce the issue with the latest versions of Parse Server and the Parse Server Push Adapter.

Issue Description

I have an existing code that is sending push notifications to both Android and iOS via the same call. The invocation in the cloud code looks like this:

 return Parse.Push.send({
    where: query,
    expiration_interval: 36000, // NOTE: in seconds this is 10 hours
    data: payload
})

where payload has been set to:

payload = {
    alert: {
      "body": "something",
      "title": "title",
    },
    "sound": "myfancysound.wav",
    "mutable-content": 1,
    "threadId": "something",
    "url": "https://something...",
    "interruptionLevel": "time-sensitive"
}

You can see that it uses default iOS alert dictionary with couple of standard iOS params (interruptionLevel, mutable-content, ...) as well as user defined params (for example url).

Updating to the latest parse-server-push-adapter@6 and adding firebaseServiceAccount to the Android configuration does not work out of the box:

Actual Outcome

Error 1: Error: android.data must only contain string values

Using the existing code and just reconfiguring android push adapter config to use firebaseServiceAccount does not work.

erbose: _PushStatus 1H56NNwx6E: sending push to installations with 1 batches
verbose: Sending push to 1
info parse-server-push-adapter FCM sending push to 1 devices
ERR! parse-server-push-adapter FCM error sending push: Error: android.data must only contain string values
verbose: _PushStatus 1H56NNwx6E: sent push! 0 success, 0 failures

This indicates that the push notification was correctly targeted and posted to FCM API, however FCM API returned an error android.data must only contain string values. This is most probably coming from the fact that push data contain an alert dictionary with body and title instead of using just flat alert and title.

Flattening the payload makes the error go away, but:

payload = {
    "alert": "something",
    "title": "title",
    "sound": "myfancysound.wav",
    "mutable-content": 1,
    "threadId": "something",
    "url": "https://something...",
    "interruptionLevel": "time-sensitive"
}

Parse log output is:

verbose: _PushStatus 28Tg8rzqsZ: sending push to installations with 1 batches
verbose: Sending push to 1
info parse-server-push-adapter FCM sending push to 1 devices
verb parse-server-push-adapter FCM tokens with successful pushes: ["f6d_Lyr6TH6OZhFLRMFDfs:APA91bH-ywNh186FRsx7XmzBKIjBZq4rFiKRTcCdiPXtiFmhF6oMKr-Lwwytise4TV0kAU_s4XVB6JpGShyga92oMUlr98mU5CcPYIrarTE2BJdHIaWBH6Rqff4exx50rzQGjwySZpfR"]
verbose: _PushStatus 28Tg8rzqsZ: sent push! 1 success, 0 failures

Error 2: Notification does not show up on Android device

The client side SDK receives a RemoteMessage with Bundle containing all the keys passed to sendPush but does not contain the data key, thus the code below returns null for all important params and push notification, despite reaching the device just gets silently dropped in handlePush.

https://github.com/parse-community/Parse-SDK-Android/blob/a08c5d1594b3a083671b493804eb1970c1ce6441/fcm/src/main/java/com/parse/fcm/ParseFirebaseMessagingService.java#L20-L42

Expected Outcome

I would expect existing code to either work or get some pointers how to adapt it to work using the new adapter.

Environment

Client

Parse Server Push Adapter version: 6.0.0

Server

Parse Server version: 7.0.0

parse-github-assistant[bot] commented 2 months ago

Thanks for opening this issue!

🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

mman commented 2 months ago

Initial investigation reveals that dictionary that is passed to firebase-admin sdk does not strictly respect the required data format.

This is what we get after preparing a dict to be sent to FCM:

{
  "data": {
    "android": {
      "priority": "high",
      "data": {
        "alert": "Something",
        "title": "Title",
        "category": "default",
        "threadId": "219cbd3423774dc1a407406e06bbc3a1",
        "cameraID": "219cbd3423774dc1a407406e06bbc3a1",
        "cameraName": "iPhone 15 Pro",
        "interruptionLevel": "time-sensitive"
      },
      "ttl": 35999
    },
    "tokens": [
      "f6d_Lyr6TH6OZhFLRMFDfs:APA91bH-ywNh186FRsx7XmzBKIjBZq4rFiKRTcCdiPXtiFmhF6oMKr-Lwwytise4TV0kAU_s4XVB6JpGShyga92oMUlr98mU5CcPYIrarTE2BJdHIaWBH6Rqff4exx50rzQGjwySZpfR"
    ]
  },
  "push_id": "Np3jwjmqHk",
  "time": "2024-04-22T18:31:58.403Z"
}

Notice how push_id and time parameters are actually present at top level, where they make no sense at all, since FCM does not require/expect them at all (https://firebase.google.com/docs/reference/admin/node/firebase-admin.messaging.basemessage.md#basemessage_interface)

The offending code comes from here where the dict is constructed:

https://github.com/parse-community/parse-server-push-adapter/blob/300b9bb03d8e80e646bfa67f26da4a9e473f4dbd/src/FCM.js#L322-L326

The push_id and time keys should be pushed out to data if I understand things correctly.

mtrezza commented 2 months ago

@jimnor0xF would you mind taking a look at this? It may be easy for you to fix since you recently worked on some related changes 🙂

mman commented 2 months ago

Couple more interesting points. If we start from the end, which is the ParseFirebaseMessagingService handling incoming push delivery within the Android app, it actually expects to receive a message with three parameters:

{
  push_id: "randomString",
  time: "time in ISO format"
  data: "JSON encoded dictionary of anything useful..."
}

The code praising the incoming message has not changed for ages and is basically here:

https://github.com/parse-community/Parse-SDK-Android/blob/a08c5d1594b3a083671b493804eb1970c1ce6441/fcm/src/main/java/com/parse/fcm/ParseFirebaseMessagingService.java#L20-L43

See also my debugging screenshot when using legacy push delivery and fresh build of Android app:

Only when all of these three parameters of the incoming message are not null will the ParsePush handling do anything useful on the Android client side.

If I understand everything correctly, then legacy GCM.js would take whatever payload it gets, will only encode it to String, decorate it with push_id and time and send it out.

The FCM.js has two problems at the moment.

It stores push_id and time in wrong hierarchy location, so these (mandatory for Parse Android SKD) just get ignored.
It flattens the data dictionary so that it actually gets received decoded in the Parse Android SDK which breaks the receiving logic.

I am not sure yet what is the proper fix here.

Workaround may be to supply rawData with prepared android payload when sending a message, thus APN push sender will use existing working mechanism, and FCM sender will pick up raw data without any conversion.

mman commented 2 months ago

I have crafted an ugly PR which shows that data need to be JSON encoded to String and decorated with push_id and time in order to be successfully delivered to Parse Android SDK. I have tested it with my app and it seems to work without changing any Parse.Push.send code, however I have not tested it in any other way. I am using the FCM.js only for delivery of push notifications to Android.

jimnor0xF commented 2 months ago

@mman Nice find! Must have missed this since we do not use the Parse SDK on the client side for push notifications, only firebase_messaging. I'll have a look at your PR.

...on a quick side note, were you able to test this on ios as well? Does the Apple Parse SDK expect push_id etc to be present?

mman commented 2 months ago

@jimnor0xF Nope, I have not tested this with iOS. I send notifications to iOS directly via APNS.js, using Firebase only for Android.

mman commented 2 months ago

...on a quick side note, were you able to test this on ios as well? Does the Apple Parse SDK expect push_id etc to be present?

I do not think they are used/required by Apple or Parse iOS SDK. They seem to be only an implementation detail in that they are set on the Parse Server, then delivered to Parse Android SDK, and there used to queue/buffer incoming push notifications based on their push_id and time.

Code here: https://github.com/parse-community/Parse-SDK-Android/blob/a08c5d1594b3a083671b493804eb1970c1ce6441/parse/src/main/java/com/parse/PushHistory.java#L100

Looks like it may have been used to remove duplicates and keep only couple recent pushes queued?

mman commented 1 month ago

Just to confirm that I have pushed 6.2.0 to production and I can send push notifications to Android users without changes in the cloud code and without changes on the client side. Simply modifying the push adapter configuration by adding firebaseServiceAccount is enough. Thanks!

mtrezza commented 1 month ago

I've done some testing as well and can confirm @mman's result. I have only tested with Android, not with Apple. It may be possible that there are edge cases in which this doesn't work, but for the moment I'm unaware of any.

I've overhauled the README for better structure and to provide better guidance in light of the FCM legacy API deprecation. Everything seems to be ready, so if there are no contrary opinions we'll prepare to send out a tweet in the next days.

Are there any changes we should make in the Android SDK regarding push_id and push_time? If so, could someone open an issue there?

mman commented 1 month ago

No changes on the Android SDK side as far as I could see

mtrezza commented 1 month ago

From the comments above regarding push_id and push_time I assumed they were legacy params that should be removed, but we cannot remove them because the Parse Android SDK expects them. Or did i misread?

mman commented 1 month ago

We would need to look into history to figure out why they were added in the first place and what was their purpose. From my short look I could see that push_id meant to categorise incoming notifications into groups using time to sort them and than pass them to BroadcastReceiver where you anyway need to build a notification and display it and track of what is onscreen? So perhaps they can be removed and just push what is received into a broadcast receiver, but it will need more time to investigate. I will not have a time to look into it in the near future I am afraid...

mtrezza commented 1 month ago

Thanks for explaining, I guess we can leave as is for now. Opened https://github.com/parse-community/parse-server/issues/9126.

parse-community / parse-server-push-adapter