Conversation
| )?, | ||
| Auth::clone_from_slice(&base64::engine::general_purpose::URL_SAFE_NO_PAD.decode(ua_auth)?), | ||
| ) | ||
| .with_vapid(vapid_key, "https://github.com/chatmail/notifiers/issues") |
There was a problem hiding this comment.
This is RFC8292 contact info ("sub" field), in case something goes wrong and a push service (e.g. Mozilla) wants to inform you about the bug
| Auth::clone_from_slice(&base64::engine::general_purpose::URL_SAFE_NO_PAD.decode(ua_auth)?), | ||
| ) | ||
| .with_vapid(vapid_key, "https://github.com/chatmail/notifiers/issues") | ||
| .build("ping")?; |
There was a problem hiding this comment.
We send a simple "ping" message. This message is encrypted with the client keys, but it isn't relevant as it doesn't contain any usable data
| .build("ping")?; | ||
|
|
||
| let res = client | ||
| .post(endpoint) |
There was a problem hiding this comment.
Here we do a POST. We do not control if the endpoint resolve to a private IP - which would be a problem for a server hosted in a private network, or with other services on the machine.
If this is a problem for you, I can easily update the function to first do the DNS resolution, then filter out private IPs, disable redirections, and do the request (e.g. what I did for mollysocket: https://github.com/mollyim/mollysocket/blob/main/src/utils/post_allowed.rs). Just let me know
| // Map web push responses to chatmail/relay notifier values | ||
| match status.as_u16() { | ||
| 201 => Ok(StatusCode::OK), | ||
| 404 | 403 | 401 => Ok(StatusCode::GONE), |
There was a problem hiding this comment.
This is to avoid modifying chatmail/relay
| schedule: Schedule, | ||
|
|
||
| fcm_client: reqwest::Client, | ||
| http_client: reqwest::Client, |
There was a problem hiding this comment.
It is now used by FCM, UBport and Web Push - I think it is better renaming it
| http_client: reqwest::Client, | ||
|
|
||
| production_client: Client, | ||
| apns_production_client: Client, |
There was a problem hiding this comment.
This used to be named when notifiers was only about apns. I think apns prefix makes things more clear to read today
|
Clippy lint failure is from main, I have fixed it in #54 so can be fixed with rebase but not necessary. |
|
Thanks, I've rebased the PR 👍 |
link2xt
left a comment
link2xt
left a comment
There was a problem hiding this comment.
Code-wise looks good, we can deploy it quite easily for testing once Android PR is ready.
|
I'm pushing your suggestions The PR for Android is here: deltachat/deltachat-android#4251 - we will need notifiers VAPID public key there :) |
| registry.register( | ||
| "webpush_notifications", | ||
| "Number of web push notifications", | ||
| ubports_notifications_total.clone(), |
|
Does Web Push need to have a centralised server-side component like Apple and Google push notifications? If not, could it be included in chatmail relays, either replacing this or using the relay by default? |
Theoretically code from here can be included in chatmail relays, but then chatmail relays will need to see unencrypted push token to use it. https://github.com/chatmail/core currently encrypts the token with the OpenPGP key of notifiers so relays cannot see if user is on Google or Apple or Ubuntu Touch or using WebPush. Disadvantage of this is that webpush users will be very visible to chatmail relays. deltachat/deltachat-android#4251 also does not create per-profile push token currently as far as I understand, so relays will be able to tell that user is the same across relays by comparing push token. Relays can get support for unencrypted tokens, recognize that they are unencrypted webpush and use them directly, then https://github.com/chatmail/core can be changed to upload webpush tokens unencrypted. But that's a tradeoff, you will stand out as a webpush user and tell chatmail relay which webpush distributor you use, which can even be your XMPP server so revealing your identity. We also need to get deltachat/deltachat-android#4251 merged first so there are webpush users and they can at least not be recognized just by being a webpush user. |
|
I think it can be handy in some situation if the chatmail relay can push directly to the push server. But it covers other needs and would require a fallback to the centralized notifier server anyway, if the relay doesn't support or enable the feature.
Exactly. That's definitely possible to do per-profile push registrations, but it requires a modification of the core lib - this is why I went with a single registration here |
3 participants
This PR adds web push support. It allows Android and Linux clients to use UnifiedPush, and potential web apps to use the browser push service. I'm opening a PR to DeltaChat android client right after this one.
I have also updated the README, it missed some doc about the current cli arguments.
If you want to try a local notifier without setting up FCM and APNS, I've a branch (https://github.com/p1gp1g/chatmail-notifiers/tree/dev) where they are behind a config flag (8015980). If you want I can open a PR with the code for the flags, and I can put webpush behind another one
I will add a few comments on the PR