MSC4174: add support for WebPush pusher kind#17987
MSC4174: add support for WebPush pusher kind#17987MatMaul wants to merge 26 commits intoelement-hq:developfrom
Conversation
a561f07 to
09ba9c3
Compare
MadLittleMods
left a comment
There was a problem hiding this comment.
Here is a review of the code itself. I haven't inspected if this is actually a valid and good enough implementation of the WebPush spec itself.
docs/webpush.md
Outdated
| In the synapse virtualenv, generate the server key pair by running | ||
| `vapid --gen --applicationServerKey`. This will generate a `private_key.pem` | ||
| (which you'll refer to in the config file with `vapid_private_key`) | ||
| and `public_key.pem` file, and also a string labeled `Application Server Key`. | ||
|
|
||
| You'll copy the Application Server Key to `vapid_app_server_key` so that | ||
| web applications can fetch it through `/capabilities` and use it to subscribe | ||
| to the push manager: |
There was a problem hiding this comment.
This seems like a hassle. Any way to improve this?
Is it possible to make it generate if the file does not exist like we do for signing_key_path?
synapse/docs/usage/configuration/config_documentation.md
Lines 3128 to 3138 in 90a6bd0
Does it matter if this key changes from time to time?
There was a problem hiding this comment.
It matters if the client isn't aware of this change
There was a problem hiding this comment.
We would have to edit the config file since we need to also specify vapid_app_server_key.
However if we make vapid_app_server_key also (possibly) take a file as vapid_private_key does, it looks a lot more sane to do the generation on the fly.
There was a problem hiding this comment.
Turns out vapid_app_server_key is just the public key in a specific format, we only need to store the private key. I've rework everything related to config and added generation on the fly.
|
|
||
| async def send_webpush(self, content: JsonDict) -> Union[bool, List[str]]: | ||
| # web push only supports normal and low priority, so assume normal if absent | ||
| low_priority = content.get("prio") == "low" |
There was a problem hiding this comment.
"low" should be a constant PushPriority.LOW
| @@ -0,0 +1,2 @@ | |||
| MSC4174: add support for WebPush pusher kind. | |||
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
| MAX_CIPHERTEXT_LENGTH = 2000 | ||
|
|
||
|
|
||
| class WebPushPusher(HttpPusher): |
There was a problem hiding this comment.
A lot of the logic is reused, send_badge and dispatch_push are actually quite small compared to the rest of the logic in HttpPusher.
Also while email notif (it's not really push honestly :) ) content is quite different, having push content be mostly similar between them outside of medium specific properties makes sense I believe.
docs/webpush.md
Outdated
| In the synapse virtualenv, generate the server key pair by running | ||
| `vapid --gen --applicationServerKey`. This will generate a `private_key.pem` | ||
| (which you'll refer to in the config file with `vapid_private_key`) | ||
| and `public_key.pem` file, and also a string labeled `Application Server Key`. | ||
|
|
||
| You'll copy the Application Server Key to `vapid_app_server_key` so that | ||
| web applications can fetch it through `/capabilities` and use it to subscribe | ||
| to the push manager: |
There was a problem hiding this comment.
We would have to edit the config file since we need to also specify vapid_app_server_key.
However if we make vapid_app_server_key also (possibly) take a file as vapid_private_key does, it looks a lot more sane to do the generation on the fly.
| @@ -0,0 +1,2 @@ | |||
| MSC4174: add support for WebPush pusher kind. | |||
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
79e6b05 to
ecd0ed0
Compare
matrix-org/matrix-spec-proposals#4174
It has been tested using the hydrogen implementation.
Most of the code and doc is inspired from the sygnal implementation.
Pull Request Checklist
(run the linters)