Implement core architecture to handle push notifications (APNs, UnifiedPush)

[hjiangsu]

Pull Request Description

Note: This replaces the previous draft PR created in #1095. I've updated the description to better match the updates.

This is a draft PR which adds the ability to handle push notifications via Apple's push notification service (APNs) and UnifiedPush.

To do this, we have to rely on two separate packages:

• push: handles platform specific implementations for iOS (APNs) and Android (FCM - Firebase Cloud Messaging). This package will only be used to implement iOS APNs. We might add the ability to use FCM in the future if we get enough requests for it.
• unifiedpush: handles the UnifiedPush specification on Android. This is limited to Android devices as mentioned here: https://unifiedpush.org/users/faq/#will-unifiedpush-ever-work-on-ios

Additionally, implementing push notifications requires a separate server in order to perform notification polling and sending out push notifications for devices. This is still a WIP.

There is still a lot of work required for this to function properly. I'll add a to-do list here:

• Set up endpoint on the server to store device token and JWT token. The device token is required in order for APNs to work since it needs to know which device to send push notifications to. The JWT token is required to poll for new notifications. Ideally, Lemmy can implement a better method authentication (with scopes) so that it does not require the JWT token.
• Setup encryption for device token and JWT. We want to make sure that both the device token and JWT are encrypted through transit to prevent malicious actors from accessing the information. I'll need to perform more research on this and understand the implications of it.
• Setup server to perform periodic polling for new notifications, and send out push notifications. This step should be relatively straight-forward.

While I'm developing the server code, I'll keep it closed-source. I'll most likely make it open-source once I'm done with all the local testing (to ensure that no keys are accidentally committed to the repository)

Issue Being Fixed

Issue Number: #219

Screenshots / Recordings

N/A. Will update when I have more logic completed.

Checklist

• Did you update CHANGELOG.md?
• Did you use localized strings where applicable?
• Did you add semanticLabels where applicable for accessibility?

[micahmo]

Quick question! There were a couple tasks that I did not do as part of my first implementation of local notifications (such as the ability to check for every logged-in account, and the ability to display mentions and messages).

I believe most of these things can be implemented separately from the push notifications feature, but I just wanted to check and see whether you are working on any of these things and/or whether they should wait until this feature is done.

[hjiangsu]

but I just wanted to check and see whether you are working on any of these things and/or whether they should wait until this feature is done.

Sorry for getting back to you so late on this! The changes in this PR moves a lot of logic around, and adjusts a lot of existing logic to be able to handle push notification functionality. I think it might be better to hold off on those changes until this is completed (or at least merged in partially)

For instance, a lot of changes were made to the main file to push the notification stream down and I believe I moved some of the functions related to background notifications to a different file to clean up the main file.

[hjiangsu]

So a bit of an update here - I think this is mostly done at this point (at least for the first iteration of changes). This is a general summary of what has changed:

• Added push and unifiedpush packages. The push package is derived from a local clone that allows us to build using the later gradle versions. You can see the diff here: ben-xD/push@main...hjiangsu:push:main
• Deprecated enableInboxNotifications setting, and added inboxNotificationType and pushNotificationServer. Also added a migration for the deprecated setting
• Refactored the notification setting in general settings page to handle the new options. This includes notification type for local, unified push, and APNs.
• Refactored the local notifications logic. Moved all this logic into its own file lib/notification/utils/local_notifications.dart.
  • Also adjusted to logic a bit to be able to handle notifications for multiple accounts.
• Added logic for unified push and APNs into their respective files: lib/notification/utils/unified_push.dart and lib/notification/utils/apns.dart
• Added logic for sending and removing auth tokens to a specified server for use with notifications.

@micahmo, I've also added you to a new repo which contains the code for the notification server. It's closed off for now since I'd like to get it more fleshed out but you can take a look at that as well if you'd like!

As for all of this, I'd like to first get the code merged in if everything looks good. I'm thinking it would be good to merge the code in first to reduce merge conflicts as this is quite a large change, and just disabling the unified push/apns feature outright for now while I continue to refine the notification server logic. Thoughts on this approach?

[hjiangsu]

Feel free to do a code review on this! I've tried to separate the logic as best as possible so that it's hopefully easier to review 😅

[hjiangsu]

Most of this logic was moved to lib/notification/utils/local_notifications.dart, including the background fetch task.

[hjiangsu]

Most of this logic was also moved to lib/notification/utils/local_notifications.dart and lib/notification/shared/android_notification.dart

[micahmo]

This is very exciting!! I'm totally ok with merging this in to continue testing iteratively.

I did look over the code, and from what I can see, it looks good. Very nicely organized. I did have a couple of questions that weren't obvious (and you can probably answer faster than I can find digging).

• Do we still have logic to handle the app starting due to tapping on a notification?
• It looks like you added multi-account support. On Android (where you can group notifications), are they grouped by account? Will Thunder correctly switch accounts and show the inbox for the right user for the tapped notifications?
• I pulled the code to test, but the "Enable Inbox Notifications" setting is disabled, so I'm not sure how to test. I guess we should at least support existing local notifications while we proceed?

BTW, just a note that I initially got this error when trying to build.

Execution failed for task ':unifiedpush_android:compileDebugJavaWithJavac'.
> Could not find tools.jar. Please check that C:\Program Files\Java\jre-1.8 contains a valid JDK installation.

I'm not sure what the "right" solution is, but I copied Program Files\Android\jdk\jdk-8.0.302.8-hotspot\jdk8u302-b08\lib\tools.jar to Program Files\Java\jre-1.8\lib and that worked.

[hjiangsu]

Do we still have logic to handle the app starting due to tapping on a notification?

Yup, I believe this located here. Its only enabled for local and unified push since iOS handles the tap:

thunder/lib/notification/notifications.dart

Lines 44 to 66 in 34893f1

	if (notificationType == NotificationType.local || notificationType == NotificationType.unifiedPush) {
	FlutterLocalNotificationsPlugin flutterLocalNotificationsPlugin = FlutterLocalNotificationsPlugin();
	// Initialize the Android-specific settings, using the splash asset as the notification icon.
	const AndroidInitializationSettings initializationSettingsAndroid = AndroidInitializationSettings('splash');
	const InitializationSettings initializationSettings = InitializationSettings(android: initializationSettingsAndroid);
	await flutterLocalNotificationsPlugin.initialize(
	initializationSettings,
	onDidReceiveNotificationResponse: (notificationResponse) => controller.add(notificationResponse),
	);
	// See if Thunder is launching because a notification was tapped. If so, we want to jump right to the appropriate page.
	final NotificationAppLaunchDetails? notificationAppLaunchDetails = await FlutterLocalNotificationsPlugin().getNotificationAppLaunchDetails();
	if (notificationAppLaunchDetails?.didNotificationLaunchApp == true && notificationAppLaunchDetails?.notificationResponse != null) {
	controller.add(notificationAppLaunchDetails!.notificationResponse!);
	bool startupDueToGroupNotification = notificationAppLaunchDetails.notificationResponse!.payload == repliesGroupKey;
	// Do a notifications check on startup, if the user isn't clicking on a group notification
	if (!startupDueToGroupNotification && notificationType == NotificationType.local) pollRepliesAndShowNotifications();
	}
	}

It looks like you added multi-account support. On Android (where you can group notifications), are they grouped by account?

It should be grouped by account! Previously, I think it was one single group for replies, but I tried to separate that into accounts. I think you can probably test it better than I can (I tried it on the emulator and it seemed correct).

Will Thunder correctly switch accounts and show the inbox for the right user for the tapped notifications?

Hmm, I have not tried this yet - we would have to do a test to see if this works properly.

I pulled the code to test, but the "Enable Inbox Notifications" setting is disabled, so I'm not sure how to test.

Ahh, I set it so that you need to have at least one logged in account to enable the setting. Can you try that and let me know?

BTW, just a note that I initially got this error when trying to build.

Hmm, I encountered this initially as well when I was initially setting it up. I'm also not sure what the best solution here is. I think the unified push package has a dependency on a specific toolchain version in their build.gradle. I'm not too familiar with this so I'm not sure the best way to go about it.

https://github.com/UnifiedPush/flutter-connector/blob/ca5ed6d078735235c261b8f7ba430a1a36d46203/unifiedpush_android/android/build.gradle#L34-L36

[micahmo]

Thanks for all the responses, everything sounds good!

Yes I think we will have to test the multi-account launching. Either we can use the temp account feature, or we can just completely switch the active account to whatever was prompted from the notification.

I set it so that you need to have at least one logged in account to enable the setting.

Oops! I was testing with a different flavor to not mess anything up and forgot to log in. Now it works and I am able to test a bit.

• Do you want to disable the "Push Notification Server" setting when using local notifications? That might ease some people's concerns who don't want their token sent anywhere that the setting has no effect in that case.
• I believe we've lost the ability to see whether the user revoked OS permissions for notifications. I'm not sure the best way to show this in the UI, since the setting subtitle has been replaced by the notification mode. Also when I tried to toggle them off, I got an error.

qemu-system-x86_64_BDao11D8Xx.mp4

That's all I got for now, but I'll play around!

[micahmo]

I did confirm that tapping a notification for an account that is not active does not show you the right inbox.

In addition, if I'm logged in with multiple accounts and get a notification from one, it seems there's always one for the other that's empty.

qemu-system-x86_64_GhPuiro7mT.mp4

EDIT: And it looks like the notifications generated via background fetch are empty.

[hjiangsu]

@micahmo I've updated the code to reflect some of the issues mentioned: 6f8f622

• The push notification server setting should now be hidden unless the user selects apns or unified push
• If the user revokes the permission setting, a snackbar will show up when they attempt to switch the notification type. Hopefully this is a good replacement for what it was previously!
• Fixed some of the edge cases you saw where the snackbar message was showing up - that should only be for unified push/apns
• Also, I've moved most of the notification setting logic to lib/notification/utils/notification_settings.dart for clarity!

[hjiangsu]

In addition, if I'm logged in with multiple accounts and get a notification from one, it seems there's always one for the other that's empty.

Weird, I thought I tested that scenario previously and it seemed to work properly. Let me take a closer look

[hjiangsu]

Just pushed another commit which should fix the issue with empty notification groups. Let me know if that works!

[hjiangsu]

I did confirm that tapping a notification for an account that is not active does not show you the right inbox.

For this, it seems like it might take a bit more work to get it running properly. Do you think it might be better for this to be added in a separate PR? If you'd like, you can have a go at it since you might be more familiar with the general flow!

[micahmo]

Thanks for all the quick updates! And yes I can definitely tackle the user issue. 😊

[hjiangsu]

Sounds good! Let me know if all the issues you mentioned earlier are fixed. If they are, then I'll go ahead and merge this in to get it out of the way 😅

[micahmo]

I probably won't be able to test for a little while, so if you want to go ahead and merge now, feel free!

[codenyte]

This looks very exciting! Is there currently a way to test this?

[micahmo]

@codenyte We are still working out the kinks, so it will probably be a little while until this comes to nightly. The only other option would be to build from source. There are some instructions in the README, although I'm not sure if they're completely up to date.

[codenyte]

I tried both building directly using Flutter and with the Docker container. Neither option worked for me, but this might also have to do with my weird setup or my incredible stupidity.

[micahmo]

Not at all, to be fair, the setup instructions are not documented to be completely foolproof, and there are a some assumptions that they make. If you want help building, feel free to reach out in the Matrix chat and send your errors logs.

[hjiangsu]

This looks very exciting! Is there currently a way to test this?

Thanks! As @micahmo mentioned, this is probably a little ways away still from being fully functional since there's still a lot of other stuff under-the-hood which would make this functional.

For UnifiedPush/Apple Push Notification service (APNs), you need a separate server that polls for new notifications. That server is still being worked on, and I'm trying to make it as simple to self-host as possible (through the use of docker). That would be the ideal scenario, rather than to have you trust me (or anyone else for that matter) with your JWT tokens.

[codenyte]

For UnifiedPush/Apple Push Notification service (APNs), you need a separate server that polls for new notifications

Right, I forgot that Lemmy doesn't support UP yet (I really hope this gets implemented)