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(), |
2 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