Forward notifications to applications using UnifiedPush library #115
Conversation
|
Hey there, thanks for your contribution. As this is a pretty big feature, that may create a long lasting API. We first have to thoroughly discuss how this feature should be implemented and what it should support. Could you introduce the changes you made in terms of security, functionality, etc. That it is easier for us to understand the change and its possible drawbacks? The discussion may be done inside #29 |
|
OK, well on gotify side :
There is only one change in the original code, at "onMessage", it cheks if the appId is registered and if it is it sends the notification to the registered app instead of popping it. On the client apps side: If you have a specifique question (and you probably will ^^), I'd be glad to answer |
|
Alright, thanks. I'll have a look at this later, this may take a little time to understand this as a whole (:. |
|
Yes no problem. You should also have a look on "gotification tester" mention before. Tell me if you want to talk via irc/riot/other for some details |
|
Yeah, I may come back to this offer later, first I want to grasp the current implementation (:. |
There was a problem hiding this comment.
Hey there, sorry that it took so long, but I'm currently pretty busy with my day job (:. Anyway, I added some questions in subcomments.
Currently, the "gotify-lib" and "gotifytester" repos are licenced with GPL-3, would it be possible to use MIT instead? (I don't want having different licences in Gotify). And, would you be ok, if both the repos would be migrated to the gotify organization?
app/src/main/java/com/github/gotify/service/MessagingDatabase.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/gotify/service/MessagingDatabase.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/gotify/service/WebSocketService.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/gotify/service/GotifyPushNotification.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/gotify/service/GotifyMessengerService.kt
Outdated
Show resolved
Hide resolved
| // The app is registered with the same uid : we re-register it | ||
| // the client may need to create a new app in the server | ||
| if (db.strictIsRegistered(clientPackageName, clientUid)) { | ||
| Log.i("$clientPackageName already registered : unregistering to register again") |
There was a problem hiding this comment.
The user of the gotify-android app should manually grant permissions to other applications to receive these messages. Otherwise, this looks like any app can just intercept gotify messages, without the user knowing it.
There was a problem hiding this comment.
Any app can register but not intercept messages. But it is a really good idea to grant permissions manually. We can do it with a notification maybe
There was a problem hiding this comment.
Ahh yeah, thought you could subscribe to existing apps. A notification like. XX likes to receive applications from gotify. Allow, Deny sounds great.
app/src/main/java/com/github/gotify/service/GotifyMessengerService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/gotify/service/GotifyMessengerService.kt
Outdated
Show resolved
Hide resolved
| // we only trust unregistered demands from the uid who registered the app | ||
| if (db.strictIsRegistered(clientPackageName, clientUid)) { | ||
| Log.i("Unregistering $clientPackageName uid: $clientUid") | ||
| deleteApp(clientPackageName) |
There was a problem hiding this comment.
I don't think we should ever delete an application, maybe there is some important stuff in it.
There was a problem hiding this comment.
I don't know, these apps aren't created manually but automatically. Maybe we should make it optionnal
There was a problem hiding this comment.
We can ask the user for this too, something like: "App XX unregistered, delete the application with all its messages?"
I'm OK for both. Maybe we should migrate before any further change? |
|
I've created https://github.com/gotify/android-connector and https://github.com/gotify/android-connector-template you could open pull requests to these repos. I'm not 100% sure about the names, but I think connector fits. The android-connector should be an android library which we later can publish a maven central / something else that it can be included as dependency without copying too much code. |
|
It will be a good idea to do a recursive git for the template in the same time. I was thinking of a name referring to gotify because it will be used in other apps and it can be the android lib name too (which is a good idea). |
|
What do you mean with
yeah, the lib itself can have gotify in its name, but on github this is already described by the organization f.ex gotify/android, I like to follow this pattern for the connector as well. |
|
Ok, I understand :) |
|
I think ConnectorService would be enough, with package it is then |
app/build.gradle
Outdated
| @@ -2,6 +2,8 @@ plugins { | |||
| id "com.diffplug.gradle.spotless" version "3.26.1" | |||
There was a problem hiding this comment.
How do you want to proceed with this PR, as there are still some todo comments inside the code?
There was a problem hiding this comment.
You are right about the TODO comments, they were there as a reminder for possible improvement.
To resume :
- A function to force to unregister an app: I don't think I have even needed it during my tests. There is still a workaround if it is really needed: wiping app data.
- As discussed, a notification to let the user to chose :
- If we accept an app registration
- If we delete the app on unregistration
- Automatic notification deletion (like issue Feature Request: Automatically Delete Old Messages #138)
- Implementing a special notification on URL change. As it happens very rarely, re-register an app is a good way to deal with it for the moment.
Should I delete the comments and we will write an issue as a feature request for these ? Implementing the notification for user acknowledge, and write a feature request for the others ? Something else ?
I'm committing the deletion of these lines for the moment
|
Hello there @p1gp1g When looking at Firebase messaging they seem to prefer using broadcasts to services for sending messages to the application but I am not sure why they chose it. Are there particular reasons you chose service binding over broadcasts for message delivery? To make sure that only gotify can deliver messages one could also have the gotify app define a custom permission and the receiving app requiring said permission on the service/receiver. |
That's exactly the idea :)
I know, I am not sure why they did that. Activities do very well the job, that's why I used this :) This still need a few things before being merged (notification control before allowing to register or delete an app, a page to see apps registered etc.). And help is welcome :) I am also working on a system to get push notifications without push provider. It would be nice if we could use one lib for both system (then we need to change 2 things on the connector here: the registered service should not be hardcoded and the message received by registered apps should not be gotify specific). If that lib is generic enough, any new way to get push notifications will be able to use it. PS: I've sent you a request on matrix |
p1gp1g
changed the title
… to theses apps
The app will only need to add these files to the project and enable a service. Then it is possible to register to gotify to receive notifications. Every time gotify receive a notification for a registered app, it will forward it (and pop the notification in the client app context)
Here is an example of a client app