Skip to content

[Notifier] [Firebase] Add 'HTTP v1' api endpoint #53336

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

Closed
wants to merge 11 commits into from

Conversation

cesurapp
Copy link
Contributor

@cesurapp cesurapp commented Jan 1, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #51147
License MIT

@symfony symfony deleted a comment from carsonbot Jan 1, 2024
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!
I'm wondering about two things:

  • would it be possible to keep the same scheme for the new API? Maybe the shape of the user/password can help? (e.g. no password means new API?)
  • more critically, I'm wondering about the need to deploy credentials.json to production, while the best practice is to use JWT tokens and to let another process refresh those tokens. Maybe we just need a way to also accept a JWT directly?

@OskarStark
Copy link
Contributor

would it be possible to keep the same scheme for the new API? Maybe the shape of the user/password can help? (e.g. no password means new API?)

Yes we should do it somehow, having a second transport looks wrong to me

@OskarStark OskarStark changed the title [FirebaseNotifier] Add 'HTTP v1' api endpoint [Notifier][Firebase] Add 'HTTP v1' api endpoint Jan 2, 2024
@carsonbot carsonbot changed the title [Notifier][Firebase] Add 'HTTP v1' api endpoint [Notifier] [Firebase] Add 'HTTP v1' api endpoint Jan 2, 2024
cesurapp and others added 2 commits January 6, 2024 00:01
…sport.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
…sport.php

Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@cesurapp
Copy link
Contributor Author

cesurapp commented Feb 16, 2024

Thanks for working on this! I'm wondering about two things:

  • would it be possible to keep the same scheme for the new API? Maybe the shape of the user/password can help? (e.g. no password means new API?)
  • more critically, I'm wondering about the need to deploy credentials.json to production, while the best practice is to use JWT tokens and to let another process refresh those tokens. Maybe we just need a way to also accept a JWT directly?

It seems like the only way to login to firebase is to use jwt. There is no explanation in the documentation other than JWT.

There are 4 parameters required and for compatibility we can do it like this:

firebase-jwt://firebase-adminsdk@stag.iam.gserviceaccount.com?project_id=<PROJECT_ID>&private_key_id=<PRIVATE_KEY_ID>&private_key=<PRIVATE_KEY>

Additionally, the token created with jwt can be used for 1 hour. We can ease the encryption burden by caching and using openssl.

@DaedalusDev
Copy link

Thanks for working on this! I'm wondering about two things:

  • would it be possible to keep the same scheme for the new API? Maybe the shape of the user/password can help? (e.g. no password means new API?)
  • more critically, I'm wondering about the need to deploy credentials.json to production, while the best practice is to use JWT tokens and to let another process refresh those tokens. Maybe we just need a way to also accept a JWT directly?

It seems like the only way to login to firebase is to use jwt. There is no explanation in the documentation other than JWT.

There are 4 parameters required and for compatibility we can do it like this:

firebase-jwt://firebase-adminsdk@stag.iam.gserviceaccount.com?project_id=<PROJECT_ID>&private_key_id=<PRIVATE_KEY_ID>&private_key=<PRIVATE_KEY>

Additionally, the token created with jwt can be used for 1 hour. We can ease the encryption burden by caching and using openssl.

I'm facing the same issue with an hand made integration.
I'm agree with @cesurapp, according to migration guide (https://firebase.google.com/docs/cloud-messaging/migrate-v1)

The HTTP v1 API uses short-lived access tokens according to the OAuth2 security model.

So Bearer with infinite TTL is no longer supported by the replacement API.

@aschempp
Copy link
Contributor

@cesurapp thanks for the work! I duplicated this into my Symfony (Contao on Sf 5.4) and it basically worked perfectly. However, I noticed there's quite a change in the Firebase API features.

Looking at https://firebase.google.com/docs/cloud-messaging/migrate-v1?hl=en#update-the-payload-of-send-requests, the to: /topics/news has been changed to topic: news. This PR however rewrites to: to token:, and it requires to: to be set. The current transport makes it impossible to send to topics.

Another thing is that the API now separates configuration for Android and Apple in the same request. The current options classes do not allow these additional configurations - and they incorrectly separate Apple and Android.

I see an issue with semantic versioning here. The options classes cannt be changed until Symfony 8, but that all makes no sense in 3 months once the API is shut down. So maybe FirebaseJwt should be a separate implementation with separate transport and options classes?

@OskarStark
Copy link
Contributor

So maybe FirebaseJwt should be a separate implementation with separate transport and options classes?

I think it makes sense as either one or the other will not work anymore, right?

@cesurapp
Copy link
Contributor Author

So maybe FirebaseJwt should be a separate implementation with separate transport and options classes?

I think it makes sense as either one or the other will not work anymore, right?

https://firebase.google.com/docs/cloud-messaging/migrate-v1?hl=en

Apps using the deprecated FCM legacy APIs for HTTP and XMPP should migrate to the HTTP v1 API at the earliest opportunity. Sending messages (including upstream messages) with those APIs was deprecated on June 20, 2023, and will be removed in June 2024.

The old API will not be available after about 1 month.

@aschempp
Copy link
Contributor

aschempp commented May 7, 2024

Exactly my point. But as far as I understand Symfony's BC promise, we cannot simply remove existing classes in a minor release, even if these classes loos their functionality. But maybe @nicolas-grekas can confirm this.

@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot
Copy link
Member

fabpot commented May 31, 2024

As 7.2 is going to be released after June 2024, I recommend throwing an exception when people try to use the old way (so that we can remove the code right away).

@aschempp
Copy link
Contributor

@cesurapp are you planning to finish this PR for Symfony 7.2? Anything I can assist?

Looking at https://firebase.google.com/docs/cloud-messaging/migrate-v1?hl=en#update-the-payload-of-send-requests, the to: /topics/news has been changed to topic: news. This PR however rewrites to: to token:, and it requires to: to be set. The current transport makes it impossible to send to topics.

I would suggest to parse to: here, and if it is /topics/... set it to the correct JSON property.

Another thing is that the API now separates configuration for Android and Apple in the same request. The current options classes do not allow these additional configurations - and they incorrectly separate Apple and Android.

My solution here would be to not make the FirebaseOptions class abstract anymore and add necessary setters there, similar to other Notifier components. We could then throw exceptions (or deprecations if a migration is possible) if people still use the old options classes.

@cesurapp
Copy link
Contributor Author

cesurapp commented Jul 9, 2024

Legacy code removed and dsn updated.

firebase://<CLIENT_EMAIL>?project_id=<PROJECT_ID>&private_key_id=<PRIVATE_KEY_ID>&private_key=<PRIVATE_KEY>

Copy link
Contributor

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Any plans on how to detail with the legacy Android and iOS notification classes?

public function __construct(string $to, array $options, array $data = [])
private bool $useTopic;

public function __construct(string $tokenOrTopic, array $options, array $data = [], bool $useTopic = false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to parse $tokenOrTopic for a string like /topic/... and add a deprecation message (but handle the flag correctly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If set to "$useTopic = true" no deprecation message will be needed. The default topic is used in the previous version.

public function __construct(#[\SensitiveParameter] array $credentials, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
{
$this->credentials = $credentials;
$this->client = $client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->client is protected in the parent class AbstractTransport, so no need to add it here.

*/
final class FirebaseTransport extends AbstractTransport
{
protected const HOST = 'fcm.googleapis.com/fcm/send';
protected const HOST = 'fcm.googleapis.com/v1/projects/project_id/messages:send';
Copy link
Contributor

@aschempp aschempp Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at all the other Notifier transports, there seems to be almost none that use a full URL in the HOST constant – they mostly only have the host in there and build the necessary project URL in the doSend method. Not sure if that's really relevant, but it would prevent $host from always being set and the HOST constant from being an actually invalid URL.

@adrolter
Copy link
Contributor

adrolter commented Aug 27, 2024

Is this close to being merged? Google is actively sunsetting the legacy API and about half of my Firebase notifications are now failing to send as a result.

Unable to post the Firebase message: {"error":"Deprecated endpoint, see https:\/\/firebase.google.com\/docs\/cloud-messaging\/migrate-v1"}

Edit: Just saw this is set to land in 7.2? Is there a temporary solution in the interim to restore functionality?

@aschempp
Copy link
Contributor

Just saw this is set to land in 7.2? Is there a temporary solution in the interim to restore functionality?

The temporary solution for me was to add the services from this PR into the app itself.

throw new InvalidArgumentException(\sprintf('The "%s" transport required the "to" option to be set.', __CLASS__));
// Generate Options
$options = $message->getOptions()?->toArray() ?? [];
if (!$options['token'] && !$options['topic']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not if (!isset($options['token']) && !isset($options['topic'])) {?

It generates this warning:

Warning: Undefined array key “token”

@misaert
Copy link
Contributor

misaert commented Sep 9, 2024

For us, this PR doesn't work this way. We have to get the access token to call the Firebase API in a second time.

This is our worked branch: symfony/firebase-notifier@6.4...geonativefr:firebase-notifier:geonative

@cesurapp
Copy link
Contributor Author

cesurapp commented Sep 9, 2024

For us, this PR doesn't work this way. We have to get the access token to call the Firebase API in a second time.

This is our worked branch: symfony/firebase-notifier@6.4...geonativefr:firebase-notifier:geonative

You shouldn't use 3rd party components in Notifier packages. "google/apiclient": "^2.0@dev"

@misaert
Copy link
Contributor

misaert commented Sep 9, 2024

For us, this PR doesn't work this way. We have to get the access token to call the Firebase API in a second time.
This is our worked branch: symfony/firebase-notifier@6.4...geonativefr:firebase-notifier:geonative

You shouldn't use 3rd party components in Notifier packages. "google/apiclient": "^2.0@dev"

It is just for us to fix the push notifications.

The API client is useful to get the access token.

@orangevinz
Copy link

@cesurapp I'm running a 5.4 Symfony version and I'm trying to use your patch that way but it fails
Any clue ?

//...
"require": {
        "php": ">=8.1.12",
        // ...
        "symfony/firebase-notifier": "dev-feature_7.1",
        "symfony/notifier": "5.4.*",
    },
"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/cesurapp/symfony"
        }
    ]
//...

@cesurapp
Copy link
Contributor Author

@cesurapp I'm running a 5.4 Symfony version and I'm trying to use your patch that way but it fails Any clue ?

//...
"require": {
        "php": ">=8.1.12",
        // ...
        "symfony/firebase-notifier": "dev-feature_7.1",
        "symfony/notifier": "5.4.*",
    },
"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/cesurapp/symfony"
        }
    ]
//...

This package required symfony 6.4+ , I have not tested it on Symfony 5x version.

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@rsereir
Copy link

rsereir commented Dec 8, 2024

Hello all !

This MR is ready ? I am trying the current version but it looks not working correctly ( i didn't know how to have a username / password combination on my Firebase dashboard ). It looks be fixed by the new way to call firebase

@fabpot
Copy link
Member

fabpot commented Dec 9, 2024

Hello all !

This MR is ready ? I am trying the current version but it looks not working correctly ( i didn't know how to have a username / password combination on my Firebase dashboard ). It looks be fixed by the new way to call firebase

AFAICS, there are still some comments that are not addressed yet.

@SherinBloemendaal
Copy link
Contributor

Would be great to have this finished before 7.3. Let me know if I can help in anyway!

@OskarStark OskarStark changed the title [Notifier] [Firebase] Add 'HTTP v1' api endpoint [Notifier][Firebase] Add 'HTTP v1' api endpoint Apr 1, 2025
@OskarStark
Copy link
Contributor

@cesurapp are you interested in finishing this PR?

@carsonbot carsonbot changed the title [Notifier][Firebase] Add 'HTTP v1' api endpoint [Notifier] [Firebase] Add 'HTTP v1' api endpoint Apr 1, 2025
@cesurapp
Copy link
Contributor Author

cesurapp commented Apr 1, 2025

@cesurapp are you interested in finishing this PR?

Sorry I don't have time for that right now.

@OskarStark
Copy link
Contributor

Sorry I don't have time for that right now.

No worries, thanks for the quick feedback.

@SherinBloemendaal do you want to take over?

@aschempp
Copy link
Contributor

aschempp commented Apr 1, 2025

I'm also happy to contribute my fork that is based on this and running for months now in my Symfony 5 setup. But I didn't wanna take away @cesurapp's work.

@SherinBloemendaal
Copy link
Contributor

I do have time to work on it but if @aschempp already has a working version, we can use that of course :)

@aschempp
Copy link
Contributor

aschempp commented Apr 2, 2025

I looked at my stuff, unfortunately I copied files from this PR to my client's App and modified to make it work (so not actually a fork). Appart from probably not being really usable, I think we need to solve the problem of having an existing API (in Symfony) that no longer works in Firebase. This PR originally introduced a BC layer for the old configuration, but that BC layer makes little sence since the old API does not work anymore anyway.

I wonder if we should throw away all of the current stuff, which will result in an error for existing users. Not sure we can do that in a minor release, but it did cause errors anyway as far as I know, so it can't really be used by anyone?

@SherinBloemendaal
Copy link
Contributor

I understand that removing the code might seem logical. But even though Firebase's legacy API is returning 404s which always results in a TransportException being thrown, we still need to maintain backward compatibility at the Symfony level in my opinion and according to Symfony's BC policy.

What we could do:

  • 7.3: Introduce new Firebase V1 API support and deprecate the legacy API
  • 8.0: Drop support for the legacy API

@aschempp
Copy link
Contributor

aschempp commented Apr 3, 2025

sounds good to me. Just be aware that the separation into AndroidNotification and IOSNotification etc. do not make sense with the new API, so we need a BC way to keep those classes as well as some new options object to handle the new API. Feel free to take on this if you have some spare time 😊

@vojtechsmejkal
Copy link

Hi, I needed to use FCM in one of our projects, so I created a working version based on what @cesurapp prepared in this PR.

I was wondering if anyone is currently working on this feature. I don't want to take over someone else's PR, but if no one is working on it, I'm happy to take it on. 🙂

@fabpot
Copy link
Member

fabpot commented Apr 6, 2025

Hi, I needed to use FCM in one of our projects, so I created a working version based on what @cesurapp prepared in this PR.

I was wondering if anyone is currently working on this feature. I don't want to take over someone else's PR, but if no one is working on it, I'm happy to take it on. 🙂

Apparently, @cesurapp does not have time anymore (see #53336 (comment)). So, feel free to take over.

@vojtechsmejkal
Copy link

Thank you, I will take over and post an update at the end of this week.

@vojtechsmejkal
Copy link

A quick update as promised:

I have created a new PR #60205 for adding support for Firebase v1 API and I think it would be best to close this PR and continue the discussion in the new one.

@fabpot fabpot closed this Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firebase notifier does not work anymore since messaging protocol updated by google