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

Workaround (or fix maybe) for NEW_NOTIFICATION messages #12672

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

starypatyk
Copy link
Contributor

@starypatyk starypatyk commented Mar 12, 2024

This is an attempt of a workaround (or fix) for #1964.

TL;DR

Incoming notifications are initially handled by system-level FCM code. Then an intent is passed to the application, that is partially handled by Firebase SDK code and partially by the application code in the onMessageReceived method.

Sometimes (I have not been able to find when), the intent contains an additional flag that causes the Firebase SDK code to handle (display) the notification without invoking at all the onMessageReceived method defined in application code.

As the unencrypted body of the notification message contains just the NEW_NOTIFICATION text, this is what gets shown to the user.

Code in this PR "intercepts" the incoming intent before the SDK code and clears the notification flag, so the SDK code always invokes the onMessageReceived method. The application code can then handle the message as usual - i.e. decrypt it and display proper notification to the user.

More detailed description below

Firebase documentation is quite clear that notification messages are delivered to the application code only when the application is in foreground.

onMessageReceived is provided for most message types, with the following exceptions:

  • Notification messages delivered when your app is in the background. In this case, the notification is delivered to the device’s system tray. A user tap on a notification opens the app launcher by default.

  • Messages with both notification and data payload, when received in the background. In this case, the notification is delivered to the device’s system tray, and the data payload is delivered in the extras of the intent of your launcher Activity.

So it seems that notification messages should not work at all. But sometimes they do. 😉

I have not been able to find out, why in some cases incoming messages are treated as combined notification/data messages and in other cases as just data messages. This would probably need insight into inner working of the push-notifications.nextcloud.com server.

I did find out however, that even in cases when the message is treated as a combined notification/data message, the application code is involved in handling of the message.

After some investigation of the Firebase Android SDK, I have found that the decision to show notification or invoke the onMessageReceived method is made by the FirebaseMessagingService class, in the dispatchMessage method.

Then, I found that the NotificationParams.isNotification method just checks for values of two extras in the intent sent by the FCM system code.

So in this PR, I propose to override the handleIntent method and clear these two extras from the incoming intent before letting the Firebase SDK code process it. This means that the notifications will be always delivered to the onMessageReceived method - as expected.

I have been able to get the NEW_NOTIFICATION messages on the emulator - although I am not sure if I can provide a repeatable scenario.

With the additional debug code from this PR, but without clearing the extras in handleIntent, I get the wrong notification and the following sequence in the logs.

03-12 22:13:34.752 12640 12782 D NCFirebaseMessagingService: handleIntent - extras: gcm.n.e: null, gcm.notification.e: 1
03-12 22:13:34.753 12640 12782 W FirebaseMessaging: Unable to log event: analytics library is missing
03-12 22:13:34.756 12640 12782 W FirebaseMessaging: Missing Default Notification Channel metadata in AndroidManifest. Default value will be used.

When I clear the extras in handleIntent, I get the correct notification and the following logs.

03-12 22:15:18.645 12832 12942 D NCFirebaseMessagingService: handleIntent - extras: gcm.n.e: null, gcm.notification.e: 1
03-12 22:15:18.646 12832 12942 W FirebaseMessaging: Unable to log event: analytics library is missing
03-12 22:15:18.646 12832 12942 D NCFirebaseMessagingService: onMessageReceived
03-12 22:15:18.662 12832 12946 D NotificationJob: doWork started
03-12 22:15:18.670 12832 12946 D OwnCloudClient #0: REQUEST GET /ocs/v2.php/apps/notifications/api/v2/notifications/7284
03-12 22:15:18.671 12832 12946 D AdvancedSslSocketFactory: Creating SSL Socket with remote ***SERVER NAME REDACTED***:443, local null:0, params: org.apache.commons.httpclient.params.HttpConnectionParams@bf36ecc
03-12 22:15:18.671 12832 12946 D AdvancedSslSocketFactory:  ... with connection timeout 60000 and socket timeout 60000
03-12 22:15:18.672 12832 12946 I ServerNameIndicator: SSLSocket implementation: com.google.android.gms.org.conscrypt.Java8FileDescriptorSocket
03-12 22:15:18.672 12832 12946 I ServerNameIndicator: SNI done, hostname: ***SERVER NAME REDACTED***
03-12 22:15:18.754 12832 12946 D GetNotificationRemoteOperation: Successful response: {"ocs":{"meta":{"status":"ok","statuscode":200,"message":"OK"},"data":{"notification_id":7284,"app":"admin_notifications","user":"darek","datetime":"2024-03-12T21:15:18+00:00","object_type":"admin_notifications","object_id":"65f0c5e6","subject":"Testing push notifications","message":"","link":"","subjectRich":"","subjectRichParameters":[],"messageRich":"","messageRichParameters":[],"icon":"https:\/\/***SERVER NAME REDACTED***\/apps\/notifications\/img\/notifications-dark.svg","shouldNotify":true,"actions":[]}}}
03-12 22:15:18.758 12832 12946 D skia    : --- Failed to create image decoder with message 'unimplemented'
03-12 22:15:18.759 12832 12946 D skia    : --- Failed to create image decoder with message 'unimplemented'
03-12 22:15:18.763 12832 12871 I WM-WorkerWrapper: Worker result SUCCESS for Work [ id=be3ca944-2897-4657-812c-3878caf82aaa, tags={ com.nextcloud.client.jobs.NotificationWork, *, name:notification, timestamp:1710278118647, class:NotificationWork } ]

@starypatyk starypatyk added bug 3. to review hotspot: push notifications Push notification distribution (and, in theory, pull) labels Mar 12, 2024
@joshtrichards joshtrichards added the feature: activity and notification Server activity and notifications label Mar 22, 2024
@joshtrichards joshtrichards linked an issue Mar 22, 2024 that may be closed by this pull request
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
Copy link

Codacy

Lint

TypemasterPR
Warnings7171
Errors33

SpotBugs

CategoryBaseNew
Bad practice6868
Correctness6868
Dodgy code350350
Experimental22
Internationalization77
Malicious code vulnerability22
Multithreaded correctness66
Performance5656
Security1919
Total578578

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12672.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

Copy link

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: activity and notification Server activity and notifications feedback-requested hotspot: push notifications Push notification distribution (and, in theory, pull)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push: show real text instead of generic "new notification"
2 participants