UP-NextPush / server-app

UnifiedPush provider for Nextcloud - server application - Moved to https://codeberg.org/NextPush/uppush
GNU Affero General Public License v3.0
67 stars 8 forks source link

Handle all push routes #32

Closed nr23730 closed 1 year ago

nr23730 commented 1 year ago

Hi! 👋

First things first: I discovered UnifiedPush and NextPush about two weeks ago and I'm very enthusiastic about it. Thank you for all your effort so far!

Right now I was playing a bit with porting UnifiedPush to other applications and therefore would propose two changes:

  1. a5709dfe1665bc1194a87efd0ea9356ce5347260 so that every push request is forwarded to the controller, even if something is appended after the UUID. This also allows proper error messages.
  2. 97e3e59c246e884caab72896b3de5486a43a8114 to extract the UUID from the path and try continue with that. This is very helpful as the server side of application may append something to the endpoint provided by UnifiedPush. This would enable better compatibility for such cases. As far as I can tell this is not contradicting the specification.

However, I'm open for feedback and discussion on this. 😊

p1gp1g commented 1 year ago

Thanks for your contribution !

a5709df may be OK. We need to clarify in the spec if adding GET parameter are allowed. If we do, we must state that adding a GET parameter already used is forbidden (?) 97e3e59 : I don't think it is a good idea, the endpoint provided is definitely not the endpoint used by the application.

Ping @sparchatus @karmanyaahm

Do you have example of application this PR would help ?

nr23730 commented 1 year ago

The Nextcloud client is an example. By default it registers https://push-notifications.nextcloud.com/ as proxy server on your Nextcloud instance. However, the push is posted to https://push-notifications.nextcloud.com/notifications (which then goes through FCM).

So in a UnifiedPush version of that app, the server (without further modification) would post the notification to https://yournextcloud/index.php/apps/uppush/push/{token}/notifications. Right now this would result in nothing - the request would be routed nowhere, so there would also be no error message.

As the NextPush endpoint including the UUID still needs to be provided correctly it just makes sure that every push (even when it's send to subpaths like the one mentioned above) get's routed to the phone.

karmanyaahm commented 1 year ago

Unfortunately, something like this won't work for other distributors like ntfy, which have other useful routes on {topic}/stuff (not /notifications, but / opens a can of worms). So, this is against the UnifiedPush spec, push servers* should send notifications to exactly the right endpoints, not {endpoint}/stuff.

If we need a compatibility layer between Nextcloud and UnifiedPush, we must build a gateway, that works with all push servers. Hopefully, @Zocker1999NET's modification to NextCloud's push server will relieve the need for /notifications in the first place, avoiding a gateway.

karmanyaahm commented 1 year ago

Thanks for the idea and your interest though, @nr23730