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

Unifiedpush #3448

Merged
merged 6 commits into from
Jun 1, 2022
Merged

Unifiedpush #3448

merged 6 commits into from
Jun 1, 2022

Conversation

p1gp1g
Copy link
Contributor

@p1gp1g p1gp1g commented Jun 2, 2021

Pull Request Checklist

This new PR for UnifiedPush (old: #2993) is rebased, have a better git flow and the previously missing parts are done.

  • A phone without a Distributor:
    • The gplay flavor will use FCM if it is available, else it will use the background sync.
    • The fdroid flavor will use the background sync.
  • A phone using backgroundsync can use push by installing an unifiedpush distributor without relogging/wiping data.
  • It is possible to go again to backgroundsync if all distributor are uninstall (and it is not gplay flavor with a working FCM, since the fallback is FCM)
  • Settings and troubleshoot list are updated whether it is [ gplay flavor with valid FCM or fdroid flavor with unifiedpush distributor ] or [ gplay flavor without FCM or fdroid flavor without any distributor ].

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jun 3, 2021

The last commit enable to use sygnal for push with the embedded distrib. It needs this pushkin, which is mainly the gcmpushkin with a little modification on the data field.

Note: embedded distrib means FCM if nothing else is installed (or if the user want this), so probably most of users

Edit: Not true since 4a8fa4a

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jun 7, 2021

The last commit create a different pusher app id whether it uses the embedded distrib or not. It is now possible to have a pushkin for the actual main version (GCM), this PR version using the embedded distrib (FCM), and this PR version with unifiedpush.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jun 30, 2021

The last commit allows to use the current gateway as it is (sygnal with gcmpushkin) for the FCM embedded distrib.

Copy link
Member

@bmarty bmarty 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 the PR, it looks promising. One first question

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

@@ -15,6 +15,8 @@

<!-- Note: pusher_http_url should have path '/_matrix/push/v1/notify' -->
<string name="pusher_http_url" translatable="false">https://matrix.org/_matrix/push/v1/notify</string>
<!-- Note: pusher_http_url should have path '/_matrix/push/v1/notify' -->
<string name="default_push_gateway_http_url" translatable="false">https://matrix.gateway.unifiedpush.org/_matrix/push/v1/notify</string>
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment?

Copy link
Member

Choose a reason for hiding this comment

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

One remark, but an important one, what happen if this service is down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This push gateway is used to forward notifications to users' UnifiedPush endpoints and is not used by the FCM embedded distributor. So if it is down, users using another UnifiedPush distributor than the embedded one (which will be used by most users) won't receive push notifications. Just like users using FCM embedded distributor won't receive push notif if the matrix sygnal server is down.

This push gateway is the unifiedpush common-proxies on our server. It can be hosted by element/matrix team. It is also possible to write a sygnal pushkin for that: it should be quite easy to write (with a big warning on SSRF).

Choose a reason for hiding this comment

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

But if I want to use my server, without participation unifiedpush.org?

Choose a reason for hiding this comment

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

@manchelsi You can self host a push proxy like gotify or nfty alongside common-proxies (only required for gotify), install the apps, point them to your server, and tell element to use the app as a distributor.

vector/build.gradle Outdated Show resolved Hide resolved
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jul 7, 2021

It should be fine now :)

gplayImplementation('com.github.UnifiedPush:android-embedded_fcm_distributor:1.1.0') {
exclude group: 'com.google.firebase', module: 'firebase-core'
exclude group: 'com.google.firebase', module: 'firebase-analytics'
exclude group: 'com.google.firebase', module: 'firebase-measurement-connector'
Copy link
Member

Choose a reason for hiding this comment

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

It's weird that we have to exclude those modules again, since it's already done here: https://github.com/UnifiedPush/android-embedded_fcm_distributor/blob/main/lib/build.gradle#L44

Anyway, the CI is happy now, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's very weird. And when the lib is a module of the project it isn't necessary. That's probably because of jitpack then

@bmarty
Copy link
Member

bmarty commented Jul 12, 2021

Hello @p1gp1g ,

Thanks for the update of the PR.

Can you explain what will happen when the user will upgrade their GPlay application with a new version with this PR included? Does the app will have to register again the pusher? I'd like to ensure that this scenario is currently handled, and that the user will still receive push as normal.

Thanks

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jul 12, 2021

Can you explain what will happen when the user will upgrade their GPlay application with a new version with this PR included? Does the app will have to register again the pusher? I'd like to ensure that this scenario is currently handled, and that the user will still receive push as normal.

If the user doesn't have any unifiedpush distibutor (eg. gotify-up) it is transparent. I have tested the migration from main branch to this PR.

The reason is every time the application is started, it does the unifiedpush registration (to the saved distributor if it exists). So it receives a new endpoint and register the pusher again (https://github.com/vector-im/element-android/pull/3448/files#diff-89945d3e11caf4a6cddd30ff052082001992c86186660ee2e1c9093f04c87f30R164).
Maybe a check can be done to register only if the key (FCM Key or UnifiedPush endpoint) is different from previously before register (PushersManager.registerPusherVectorMessagingReceiver).

Edit: check done with dbfa5e1

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jul 12, 2021

We can mention issue #224 (in the roadmap too) :

https://github.com/vector-im/element-android/pull/3448/files#diff-44923f4e9b5a6e14764ee8b524a9d8afaeb48ac8f6cb8d708b0794965c557692R95

The gplay flavor works like the fdroid one if there is no play services

@jwsp1
Copy link

jwsp1 commented Jul 18, 2021

I guess the user has to install a unifiedpush app like gotify-up in order to make it work for element?
If yes, is it planned to show a hint in the notification settings? Or troubleshoot setting?

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Jul 20, 2021

I guess the user has to install a unifiedpush app like gotify-up in order to make it work for element?
If yes, is it planned to show a hint in the notification settings? Or troubleshoot setting?

Phones will always have a way to receive notifications, with and without unifiedpush distribtutor. cf #3448 (comment)

But it is possible to add a check on the troubleshoot list that say : the distributor used if any, no distributor is found if it uses the background sync. I don't know if it is useful since it is already viewable : the troubleshoot list displays the endpoint or background sync information

@jwsp1
Copy link

jwsp1 commented Jul 28, 2021

Ah I see, thanks for replying. Hope this gets merged soon - don't get any notifications from Element (F-Droid, Lineage with microG). Looking at PlayStore reviews, I'm not the only one (although oddly, they should have fcm).
Addressing notification issues should be prio 1 for an >instant< messenger.

@jwsp1
Copy link

jwsp1 commented Aug 15, 2021

In TWIM, you said that we should tell you if we didn't get a push notification last week... I didn't get one (Element Android, F-Droid).
I think this PR could be a solution.

Also: If you've received any push notifications in Element Android or Element iOS this week, they've come via our shiny new stateless release of Sygnal! If you haven't received expected push notifications on those platforms this week... please alert us immediately. 😉

Source: TWIM 2021-08-13

@genofire
Copy link

genofire commented Sep 7, 2021

Now there is an merge conflict 😢 - I really wish this PR is merged soon (On F-Droid this is not an instant messaging 😭 , nearly useless for Messaging )

Thank you for the great work till yet @p1gp1g

@jacktheripper19
Copy link

Now there is an merge conflict - I really wish this PR is merged soon (On F-Droid this is not an instant messaging , nearly useless for Messaging )

Thank you for the great work till yet @p1gp1g

Yes it's frustrating. This PR has been open for a long time. There was a time when there was no merge conflicts. If you're not patient, there is the beta version of SchildiChat which is a fork of Element. It had UnfiedPush implemented in it.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 8, 2021

The conflict is just another addition on the build.gradle, nothing relevant 😉

@genofire
Copy link

genofire commented Sep 8, 2021

Really SchildiChat#68 is still open and say it is an upstream issue? Or do you mean https://fluffychat.im ?

@SpiritCroc
Copy link
Contributor

Really SchildiChat#68 is still open and say it is an upstream issue? Or do you mean https://fuffychat.im ?

"Upstream" issues do not prevent SchildiChat from implementing it anyways. It's still open as it's not enabled in stable/production builds yet.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 16, 2021

Is there a reason why we don't get events in fast lane on push notification with a sync as a fallback ? (cf 94f9f7f) Should I merge this commit here ?

@bmarty
Copy link
Member

bmarty commented Oct 4, 2021

Is there a reason why we don't get events in fast lane on push notification with a sync as a fallback ? (cf p1gp1g@6cd73da) Should I merge this commit here ?

This commit is not OK to me, a sync is often necessary, to be able to decrypt encrypted messages, or to handle other messages, which was not triggering a Push.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Oct 7, 2021

Is there a reason why we don't get events in fast lane on push notification with a sync as a fallback ? (cf p1gp1g@6cd73da [/p1gp1g@94f9f7f]) Should I merge this commit here ?

This commit is not OK to me, a sync is often necessary, to be able to decrypt encrypted messages, or to handle other messages, which was not triggering a Push.

Indeed, I've observed that, as it is, the fallback to the sync does not always occured and that's the most important thing.

Moreover, it seems it doesn't go faster most of the time.

Fortunately, I didn't commit it here :)

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Apr 26, 2022

PR is rebased.

For your information, we have released a FOSS library to receive notification via google service [1][2]. This library does not use any proprietary lib, and so it follows FDroid rules. You might want to use it and having a single flavor. If you are interested, I can do it in another PR

Also, it would be great to have some information on the internal discussion state.

[1] https://unifiedpush.org/developers/embedded_fcm/
[2] https://github.com/UnifiedPush/android-foss_embedded_fcm_distributor/

@no-usernames-left
Copy link

no-usernames-left commented Apr 26, 2022

we have released a FOSS library to receive notification via google service

Please correct me if I'm wrong, but I think most of the point of UnifiedPush is because it's not Google.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Apr 26, 2022

we have released a FOSS library to receive notification via google service

Please correct me if I'm wrong, but I think most of the point of UnifiedPush is because it's not Google.

Indeed, but most users and developers will continue to use Google services. With these libraries (foss & non-foss embedded distributor) users can continue to receive notifications with google services if they want to and it is pretty easy to setup for developers. With the FOSS one, it is achieved without having any proprietary code in the app, so we know there is not any telemetry thing running in the app.

I'd like to avoid too much noise on this PR, if you want to, you're welcome to come in our room : https://matrix.to/#/#unifiedpush:matrix.org

@genofire
Copy link

genofire commented May 3, 2022

A half year without review, comment or so ...
Any news, if this will be merged soon @bmarty ?

@bmarty
Copy link
Member

bmarty commented Jun 1, 2022

Hello @p1gp1g, I will merge it on one on my branch to be able to fix the conflict, do some rework and add some documentation.

Sorry, I do not see the sign-off. Can you add a sign off to the PR, so that I can merge it in our repo please?

@bmarty bmarty changed the base branch from develop to bma/unifiedPush June 1, 2022 12:14
@bmarty bmarty deleted the branch element-hq:feature/bma/unifiedPush2 June 1, 2022 12:27
@bmarty bmarty closed this Jun 1, 2022
@bmarty bmarty reopened this Jun 1, 2022
@bmarty bmarty changed the base branch from bma/unifiedPush to feature/bma/unifiedPush June 1, 2022 12:36
@bmarty bmarty changed the base branch from feature/bma/unifiedPush to try2 June 1, 2022 12:43
@bmarty bmarty changed the base branch from try2 to feature/bma/unifiedPush2 June 1, 2022 12:45
@bmarty bmarty merged commit a8d5040 into element-hq:feature/bma/unifiedPush2 Jun 1, 2022
@bmarty
Copy link
Member

bmarty commented Jun 1, 2022

Merged in my branch, will do some rework on it and create a new PR.

@genofire
Copy link

genofire commented Jun 2, 2022

It would be really nice, when we get an issue or an Milestone, where we could trace, that is released.

@genofire
Copy link

genofire commented Jun 2, 2022

Maybe this issue #2743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.