parse-community / parse-server-push-adapter

A push notification adapter for Parse Server
https://parseplatform.org
MIT License
85 stars 100 forks source link

Switch from `node-gcm` to `firebase-admin` dependency #219

Closed mtrezza closed 1 month ago

mtrezza commented 1 year ago

New Feature / Enhancement Checklist

Current Limitation

The push adapter is depending on https://github.com/parse-community/node-gcm, which is using a now deprecated API for Google's FCM. The API will be decommissioned in June 2024:

On June 20, 2024, we’re reducing the number of Firebase Cloud Messaging (FCM) legacy register APIs and legacy send APIs that provide similar functionality. This step will allow us to provide you with a more consistent experience and align with Google security standards to improve security, reliability and performance. Because of these API decommissions, some already-deprecated SDKs and features will stop working after June 20, 2024. Please consult the tables below to find which Firebase Cloud Messaging (FCM) APIs and corresponding services/SDKs/features will be discontinued and replaced with new alternatives.

It's unclear at this point whether the community of the original node-gcm repository will adapt the node-gcm to the new API, especially since Firebase provides a native SDK as alternative with firebase-admin-node.

Feature / Enhancement Description

Evaluate whether to adapt node-gcm or switch to the firebase-admin-node. If the community of the original node-gcm repository won't update it, then the initial assumption is that switching to the native SDK will bring more benefits over the long run, as it will be maintained by Google and receive continuous updates. The effort required to switch may be similar to the effort required to adapt node-gcm.

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this issue!

mtrezza commented 1 year ago

cc @parse-community/server This is a critical issue that will require everyone to upgrade the push adapter for continued FCM support.

jimnor0xF commented 10 months ago

Added support for this for myself since using APNS and GCM separately together with Flutter and the firebase_messaging package did not play very well.

Created a quick draft for this here #222. Not the best at JS, so probably needs to be refined.

It uses sendEachForMulticast() which is a bit concerning in its current state: https://eladnava.com/send-multicast-notifications-using-node-js-http-2-and-the-fcm-http-v1-api/

mtrezza commented 10 months ago

@jimnor0xF That's amazing, thanks for taking a lead on this.

henrik commented 4 months ago

To confirm:

Is the current state of parse-server-push-adapter (and thus of parse-server) that it does not support the new API, so if nothing changes, parse-server won't work with Firebase after June 2024 (4 months from now)?

jimnor0xF commented 4 months ago

To confirm:

Is the current state of parse-server-push-adapter (and thus of parse-server) that it does not support the new API, so if nothing changes, parse-server won't work with Firebase after June 2024 (4 months from now)?

In the current state only APNS based push notifications (Apple only) will work after June 2024. Haven't looked at this issue in a while, been busy with other things.

But I guess what remains here to get this merged in is adding some tests and a code review. I've been running this in production for quite some time with no problems.

mtrezza commented 4 months ago

Basically for the review of the existing PR we just need to make sure it's not a breaking change for people who are for example using the current API that will be removed in June. And at least a few tests. But it's good to know that this is running fine in production for a while already.

bdubale commented 4 months ago

@jimnor0xF Thank you for implementing this fix for everyone. This would have been a disaster for legacy systems. @mtrezza When will this be merged?

mtrezza commented 4 months ago

We'd need someone to test the PR out with their current set-up for node-gcm, so we know whether the push adapter still works without forcing to switch to the new API.

henrik commented 4 months ago

I’ve made a note to try this out in our staging environment in a few weeks, during our next cooldown.

If anyone is willing to write up specific instructions on how to test it (the package.json incantation for Parse to use this version of the push adapter; what auth to get from FCM; how to configure it in Parse), that would lower the barrier a lot and I’m sure more people would be happy to test it.

jimnor0xF commented 4 months ago

We'd need someone to test the PR out with their current set-up for node-gcm, so we know whether the push adapter still works without forcing to switch to the new API.

@mtrezza From what I can tell the v1 API does not support the same authorization scheme that was previously used with the legacy API. So I don't think it's possible to have a completely seamless migration without configuration changes, since the user has to switch out the API key for a Firebase service account.

https://firebase.google.com/docs/cloud-messaging/migrate-v1#update-authorization-of-send-requests

However, it should support the same push payload format that was used with node-gcm.

mtrezza commented 4 months ago

@jimnor0xF I meant that when using the current configuration of the deprecated API, the adapter should work without any changes necessary in the config or code. In other words, when upgrading the adapter to the new version with that PR, it should work using the deprecated API, and if someone wants to use the new API they just have to change the adapter config in the Parse Server options. The push payload should stay the same, regardless of which API is being used.

Is that currently the case?

jimnor0xF commented 4 months ago

@jimnor0xF I meant that when using the current configuration of the deprecated API, the adapter should work without any changes necessary in the config or code. In other words, when upgrading the adapter to the new version with that PR, it should work using the deprecated API, and if someone wants to use the new API they just have to change the adapter config in the Parse Server options. The push payload should stay the same, regardless of which API is being used.

Is that currently the case?

@mtrezza Yes, that is the case. If the "firebaseServiceAccount" key is not present under the push config it will use GCM (if android pushtypes) or APNS (if apple pushtypes). The code for those modules are untouched.

It should also support the old GCM push payload format. An additional key was also added to be able to use raw FCM payloads. I can add a test spec with some mocks to make sure this is the case.

mtrezza commented 4 months ago

I can add a test spec with some mocks to make sure this is the case.

That would be great, so I'll wait with merging.

And just to be sure, with the PR it's still possible to use APNS directly for Apple pushes and at the same time the new firebase-admin API only for Android pushes? In other words, the firebaseServiceAccount key would only be present in the android section of the adapter config, and so the Apple pushes would unchanged and continue to be sent to APNS directly.

mtrezza commented 4 months ago

I've merge the PR to make the feature available more quickly, but we are still lacking a small PR with the proper documentation for the feature, specifically the options firebaseServiceAccount and rawPayload and whether the Parse.Push.send payload stays the same or is fully backwards compatible. From looking at the code, it also seems that the FCM implementation is not backwards compatible with the push payload for APNS. @jimnor0xF would you want to add a quick chapter to the README?

jimnor0xF commented 4 months ago

I've merge the PR to make the feature available more quickly, but we are still lacking a small PR with the proper documentation for the feature, specifically the options firebaseServiceAccount and rawPayload and whether the Parse.Push.send payload stays the same or is fully backwards compatible. From looking at the code, it also seems that the FCM implementation is not backwards compatible with the push payload for APNS. @jimnor0xF would you want to add a quick chapter to the README?

@mtrezza Yes, it is not compatible with the APNS payloads, only GCM. I refrained from adding that support since I remember using the same payload for both android and apple was quite flaky to begin with in the old implementation. Specifically data payloads from what I remember. I think we should highlight moving to using rawPayload if the intention is to support both apple and android at the same time.

Regarding the section in README, sure, I can add that. I also have some tests I've written. Can we put that into the same PR perhaps?

mtrezza commented 4 months ago

Sure, same PR is fine. Would it be a lot of work to add the APNS implementation so that developers won't have to deal with rawPayload for Android and their usual payload for APNS? That could benefit a lot of developers, given that the API deprecation in June will affect the majority of Parse Sever deployments.

jimnor0xF commented 4 months ago

Sure, same PR is fine. Would it be a lot of work to add the APNS implementation so that developers won't have to deal with rawPayload for Android and their usual payload for APNS? That could benefit a lot of developers, given that the API deprecation in June will affect the majority of Parse Sever deployments.

@mtrezza Should be easy as long as we know which pushType it is when we instantiate so we know which payload to convert to (apple-type or android-type). Could perhaps pass the pushType as an argument to the constructor like below which can be used when generating the payload.

      switch (pushType) {
        case 'ios':
        case 'tvos':
        case 'osx':
          if (pushConfig[pushType].hasOwnProperty('firebaseServiceAccount')) {
            this.senderMap[pushType] = new FCM(pushConfig[pushType], pushType);
          } else {
            this.senderMap[pushType] = new APNS(pushConfig[pushType]);
          }
          break;
        case 'android':
        case 'fcm':
          if (pushConfig[pushType].hasOwnProperty('firebaseServiceAccount')) {
            this.senderMap[pushType] = new FCM(pushConfig[pushType], pushType);
          } else {
            this.senderMap[pushType] = new GCM(pushConfig[pushType]);
          }
          break;
      }
mtrezza commented 4 months ago

Yep, sounds good

mtrezza commented 2 months ago

Keeping this issue open for now as we are still not quite there yet it seems, see https://github.com/parse-community/parse-server-push-adapter/issues/237.

mtrezza commented 1 month ago

237 has been closed, so closing this as well.