Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
mtrezza opened this issue Jun 24, 2023 · 21 comments · Fixed by #222
Closed
3 tasks done

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

mtrezza opened this issue Jun 24, 2023 · 21 comments · Fixed by #222
Labels
bounty:$250 Bounty applies for fixing this issue (Parse Bounty Program) type:feature

Comments

@mtrezza
Copy link
Member

mtrezza commented Jun 24, 2023

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
Copy link

parse-github-assistant bot commented Jun 24, 2023

Thanks for opening this issue!

  • 🎉 We are excited about your ideas for improvement!

@mtrezza mtrezza changed the title Switch from node-gcm to firebase-admin SDK Switch from node-gcm to firebase-admin dependency Jun 24, 2023
@mtrezza
Copy link
Member Author

mtrezza commented Jun 24, 2023

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

@mtrezza mtrezza pinned this issue Jun 24, 2023
@mtrezza mtrezza added the bounty:$250 Bounty applies for fixing this issue (Parse Bounty Program) label Jul 16, 2023
@jimnor0xF
Copy link
Contributor

jimnor0xF commented Aug 20, 2023

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
Copy link
Member Author

mtrezza commented Aug 21, 2023

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

@henrik
Copy link

henrik commented Feb 15, 2024

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
Copy link
Contributor

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
Copy link
Member Author

mtrezza commented Feb 15, 2024

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
Copy link

bdubale commented Feb 16, 2024

@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
Copy link
Member Author

mtrezza commented Feb 16, 2024

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
Copy link

henrik commented Feb 17, 2024

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
Copy link
Contributor

jimnor0xF commented Feb 17, 2024

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
Copy link
Member Author

mtrezza commented Feb 17, 2024

@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
Copy link
Contributor

jimnor0xF commented Feb 19, 2024

@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
Copy link
Member Author

mtrezza commented Feb 22, 2024

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
Copy link
Member Author

mtrezza commented Mar 2, 2024

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
Copy link
Contributor

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
Copy link
Member Author

mtrezza commented Mar 2, 2024

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
Copy link
Contributor

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
Copy link
Member Author

mtrezza commented Mar 3, 2024

Yep, sounds good

@mtrezza
Copy link
Member Author

mtrezza commented Apr 23, 2024

Keeping this issue open for now as we are still not quite there yet it seems, see #237.

@mtrezza
Copy link
Member Author

mtrezza commented May 25, 2024

#237 has been closed, so closing this as well.

@mtrezza mtrezza closed this as completed May 25, 2024
@mtrezza mtrezza unpinned this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty:$250 Bounty applies for fixing this issue (Parse Bounty Program) type:feature
Projects
None yet
4 participants