-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC4174: add support for WebPush pusher kind #17987
base: develop
Are you sure you want to change the base?
Changes from all commits
a3f58ad
920a3f1
5515c37
c029e9a
20f4a31
e2fe3b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
MSC4174: add support for WebPush pusher kind. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,97 @@ | ||||||
|
||||||
# WebPush | ||||||
|
||||||
## Setup & configuration | ||||||
|
||||||
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: | ||||||
|
||||||
```js | ||||||
serviceWorkerRegistration.pushManager.subscribe({ | ||||||
userVisibleOnly: true, | ||||||
applicationServerKey: "...", | ||||||
}); | ||||||
``` | ||||||
|
||||||
You also need to set an e-mail address in `vapid_contact_email` in the config file, | ||||||
where the push server operator can reach you in case they need to notify you | ||||||
about your usage of their API. | ||||||
|
||||||
Since for webpush, the push server endpoint is variable and comes from the browser | ||||||
through the push data, you may not want to have your synapse instance connect to any | ||||||
random addressable server. | ||||||
You can use the global options `ip_range_blacklist` and `ip_range_allowlist` to manage that. | ||||||
|
||||||
A default time-to-live of 15 minutes is set for webpush, but you can adjust this by setting | ||||||
the `ttl: <number of seconds>` configuration option for the pusher. | ||||||
If notifications can't be delivered by the push server aftet this time, they are dropped. | ||||||
|
||||||
## Push key and expected push data | ||||||
|
||||||
In your web application, [the push manager subscribe method](https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe) | ||||||
will return | ||||||
[a subscription](https://developer.mozilla.org/en-US/docs/Web/API/PushSubscription) | ||||||
with an `endpoint` and `keys` property, the latter containing a `p256dh` and `auth` | ||||||
property. The `p256dh` key is used as the push key, and the push data must contain | ||||||
`endpoint` and `auth`. You can also set `default_payload` in the push data; | ||||||
any properties set in it will be present in the push messages you receive, | ||||||
so it can be used to pass identifiers specific to your client | ||||||
(like which account the notification is for). | ||||||
|
||||||
### events_only | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
As of the time of writing, all webpush-supporting browsers require you to set | ||||||
`userVisibleOnly: true` when calling (`pushManager.subscribe`) | ||||||
[https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe], to | ||||||
(prevent abusing webpush to track users)[https://goo.gl/yqv4Q4] without their | ||||||
knowledge. With this (mandatory) flag, the browser will show a "site has been | ||||||
updated in the background" notification if no notifications are visible after | ||||||
your service worker processes a `push` event. This can easily happen when synapse | ||||||
sends a push message to clear the unread count, which is not specific | ||||||
to an event. With `events_only: true` in the pusher data, synapse won't forward | ||||||
any push message without a event id. This prevents your service worker being | ||||||
forced to show a notification to push messages that clear the unread count. | ||||||
|
||||||
### only_last_per_room | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
You can opt in to only receive the last notification per room by setting | ||||||
`only_last_per_room: true` in the push data. Note that if the first notification | ||||||
can be delivered before the second one is sent, you will still get both; | ||||||
it only has an effect when notifications are queued up on the gateway. | ||||||
|
||||||
### Multiple pushers on one origin | ||||||
|
||||||
Also note that because you can only have one push subscription per service worker, | ||||||
and hence per origin, you might create pushers for different accounts with the same | ||||||
p256dh push key. To prevent the server from removing other pushers with the same | ||||||
push key for your other users, you should set `append` to `true` when uploading | ||||||
your pusher. | ||||||
|
||||||
## Notification format | ||||||
|
||||||
The notification as received by your web application will contain the following keys | ||||||
(assuming non-null values were sent by the homeserver). These are the | ||||||
same as specified in [the push gateway spec](https://matrix.org/docs/spec/push_gateway/r0.1.0#post-matrix-push-v1-notify), | ||||||
but the sub-keys of `counts` (`unread` and `missed_calls`) are flattened into | ||||||
the notification object. | ||||||
|
||||||
``` | ||||||
room_id | ||||||
room_name | ||||||
room_alias | ||||||
membership | ||||||
event_id | ||||||
sender | ||||||
sender_display_name | ||||||
user_is_target | ||||||
type | ||||||
content | ||||||
unread | ||||||
missed_calls | ||||||
``` |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -38,6 +38,14 @@ | |||||||||||||
except ImportError: | ||||||||||||||
HAS_AUTHLIB = False | ||||||||||||||
|
||||||||||||||
# Determine whether pywebpush is installed. | ||||||||||||||
try: | ||||||||||||||
import pywebpush # noqa: F401 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment why we
Suggested change
For example: synapse/synapse/metrics/__init__.py Lines 53 to 54 in 9c5d08f
It looks like this is copying the pattern just above which should also have a note. |
||||||||||||||
|
||||||||||||||
HAS_PYWEBPUSH = True | ||||||||||||||
except ImportError: | ||||||||||||||
HAS_PYWEBPUSH = False | ||||||||||||||
|
||||||||||||||
if TYPE_CHECKING: | ||||||||||||||
# Only import this if we're type checking, as it might not be installed at runtime. | ||||||||||||||
from authlib.jose.rfc7517 import JsonWebKey | ||||||||||||||
|
@@ -256,6 +264,28 @@ class MSC3866Config: | |||||||||||||
require_approval_for_new_accounts: bool = False | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@attr.s(auto_attribs=True, frozen=True, slots=True) | ||||||||||||||
class MSC4174Config: | ||||||||||||||
"""Configuration for MSC4174""" | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comment doc for the attributes |
||||||||||||||
|
||||||||||||||
enabled: bool = attr.ib(default=False, validator=attr.validators.instance_of(bool)) | ||||||||||||||
|
||||||||||||||
@enabled.validator | ||||||||||||||
def _check_enabled(self, attribute: attr.Attribute, value: bool) -> None: | ||||||||||||||
# Only allow enabling MSC4174 if pywebpush is installed | ||||||||||||||
if value and not HAS_PYWEBPUSH: | ||||||||||||||
raise ConfigError( | ||||||||||||||
"MSC4174 is enabled but pywebpush is not installed. " | ||||||||||||||
"Please install pywebpush to use MSC4174.", | ||||||||||||||
("experimental", "msc4174", "enabled"), | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
vapid_contact_email: str = "" | ||||||||||||||
vapid_private_key: str = "" | ||||||||||||||
vapid_app_server_key: str = "" | ||||||||||||||
ttl: int = 12 * 60 * 60 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume?
Suggested change
|
||||||||||||||
|
||||||||||||||
|
||||||||||||||
class ExperimentalConfig(Config): | ||||||||||||||
"""Config section for enabling experimental features""" | ||||||||||||||
|
||||||||||||||
|
@@ -447,3 +477,23 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: | |||||||||||||
|
||||||||||||||
# MSC4076: Add `disable_badge_count`` to pusher configuration | ||||||||||||||
self.msc4076_enabled: bool = experimental.get("msc4076_enabled", False) | ||||||||||||||
|
||||||||||||||
# MSC4174: webpush push kind | ||||||||||||||
raw_msc4174_config = experimental.get("msc4174", {}) | ||||||||||||||
self.msc4174 = MSC4174Config(**raw_msc4174_config) | ||||||||||||||
if self.msc4174.enabled: | ||||||||||||||
if not self.msc4174.vapid_contact_email: | ||||||||||||||
raise ConfigError( | ||||||||||||||
"'vapid_contact_email' must be provided when enabling WebPush support", | ||||||||||||||
("experimental", "msc4174", "vapid_contact_email"), | ||||||||||||||
) | ||||||||||||||
if not self.msc4174.vapid_private_key: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a file path, we should use the standard idiom: |
||||||||||||||
raise ConfigError( | ||||||||||||||
"'vapid_private_key' must be provided when enabling WebPush support", | ||||||||||||||
("experimental", "msc4174", "vapid_private_key"), | ||||||||||||||
) | ||||||||||||||
if not self.msc4174.vapid_app_server_key: | ||||||||||||||
raise ConfigError( | ||||||||||||||
"'vapid_app_server_key' must be provided when enabling WebPush support", | ||||||||||||||
("experimental", "msc4174", "vapid_app_server_key"), | ||||||||||||||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,7 +111,6 @@ def __init__(self, hs: "HomeServer", pusher_config: PusherConfig): | |
self.device_display_name = pusher_config.device_display_name | ||
self.device_id = pusher_config.device_id | ||
self.pushkey_ts = pusher_config.ts | ||
self.data = pusher_config.data | ||
self.backoff_delay = HttpPusher.INITIAL_BACKOFF_SEC | ||
self.failing_since = pusher_config.failing_since | ||
self.timed_call: Optional[IDelayedCall] = None | ||
|
@@ -123,9 +122,9 @@ def __init__(self, hs: "HomeServer", pusher_config: PusherConfig): | |
|
||
self.push_jitter_delay_ms = hs.config.push.push_jitter_delay_ms | ||
|
||
self.data = pusher_config.data | ||
if self.data is None: | ||
if pusher_config.data is None: | ||
raise PusherConfigException("'data' key can not be null for HTTP pusher") | ||
self.data = pusher_config.data | ||
|
||
# Check if badge counts should be disabled for this push gateway | ||
self.disable_badge_count = self.hs.config.experimental.msc4076_enabled and bool( | ||
|
@@ -138,26 +137,29 @@ def __init__(self, hs: "HomeServer", pusher_config: PusherConfig): | |
pusher_config.pushkey, | ||
) | ||
|
||
# Validate that there's a URL and it is of the proper form. | ||
if "url" not in self.data: | ||
raise PusherConfigException("'url' required in data for HTTP pusher") | ||
|
||
url = self.data["url"] | ||
if not isinstance(url, str): | ||
raise PusherConfigException("'url' must be a string") | ||
url_parts = urllib.parse.urlparse(url) | ||
# Note that the specification also says the scheme must be HTTPS, but | ||
# it isn't up to the homeserver to verify that. | ||
if url_parts.path != "/_matrix/push/v1/notify": | ||
raise PusherConfigException( | ||
"'url' must have a path of '/_matrix/push/v1/notify'" | ||
) | ||
self.url = "" | ||
if pusher_config.kind == "http": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(and replace all other pusher kind usage and comparisons in the codebase) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like we should have a sanity check here that we're not trying to use this class with the wrong pusher config. This also makes it more obvious what pusher kinds this class is used with. assert pusher_config.kind in (PusherKind.HTTP, PusherKind.WEB_PUSH)
# ...
if pusher_config.kind == PusherKind.HTTP:
# ...
elif pusher_config.kind == PusherKind.WEB_PUSH:
pass
else:
# FIXME: We should use `assert_never` here but for some reason
# the exhaustive matching doesn't recognize the `Never` here.
# assert_never(pusher_config.kind)
raise AssertionError(
f"Unexpected pusher kind {pusher_config.kind} for HttpPusher"
) |
||
# Validate that there's a URL and it is of the proper form. | ||
if "url" not in self.data: | ||
raise PusherConfigException("'url' required in data for HTTP pusher") | ||
|
||
url = self.data["url"] | ||
if not isinstance(url, str): | ||
raise PusherConfigException("'url' must be a string") | ||
url_parts = urllib.parse.urlparse(url) | ||
# Note that the specification also says the scheme must be HTTPS, but | ||
# it isn't up to the homeserver to verify that. | ||
if url_parts.path != "/_matrix/push/v1/notify": | ||
raise PusherConfigException( | ||
"'url' must have a path of '/_matrix/push/v1/notify'" | ||
) | ||
self.url = url | ||
|
||
self.data_minus_url = {} | ||
self.data_minus_url.update(self.data) | ||
del self.data_minus_url["url"] | ||
|
||
self.url = url | ||
self.http_client = hs.get_proxied_blocklisted_http_client() | ||
self.data_minus_url = {} | ||
self.data_minus_url.update(self.data) | ||
del self.data_minus_url["url"] | ||
self.badge_count_last_call: Optional[int] = None | ||
|
||
def on_started(self, should_check_for_notifs: bool) -> None: | ||
|
@@ -188,7 +190,10 @@ async def _update_badge(self) -> None: | |
) | ||
if self.badge_count_last_call is None or self.badge_count_last_call != badge: | ||
self.badge_count_last_call = badge | ||
await self._send_badge(badge) | ||
if await self.send_badge(badge): | ||
http_badges_processed_counter.inc() | ||
else: | ||
http_badges_failed_counter.inc() | ||
|
||
def on_timer(self) -> None: | ||
self._start_processing() | ||
|
@@ -510,7 +515,7 @@ async def dispatch_push_event( | |
|
||
return res | ||
|
||
async def _send_badge(self, badge: int) -> None: | ||
async def send_badge(self, badge: int) -> bool: | ||
""" | ||
Args: | ||
badge: number of unread messages | ||
Comment on lines
519
to
521
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should update comment doc with what the returned bool represents (same for the other |
||
|
@@ -534,9 +539,9 @@ async def _send_badge(self, badge: int) -> None: | |
} | ||
try: | ||
await self.http_client.post_json_get_json(self.url, d) | ||
http_badges_processed_counter.inc() | ||
return True | ||
except Exception as e: | ||
logger.warning( | ||
"Failed to send badge count to %s: %s %s", self.name, type(e), e | ||
) | ||
http_badges_failed_counter.inc() | ||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import logging | ||
from typing import TYPE_CHECKING, Callable, Dict, Optional | ||
|
||
import synapse.config.experimental | ||
from synapse.push import Pusher, PusherConfig | ||
from synapse.push.emailpusher import EmailPusher | ||
from synapse.push.httppusher import HttpPusher | ||
|
@@ -42,6 +43,14 @@ def __init__(self, hs: "HomeServer"): | |
"http": HttpPusher | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit on code not from this PR:
|
||
|
||
if ( | ||
synapse.config.experimental.HAS_PYWEBPUSH | ||
and self.config.experimental.msc4174.enabled | ||
): | ||
from synapse.push.webpushpusher import WebPushPusher | ||
|
||
self.pusher_types["webpush"] = WebPushPusher | ||
|
||
logger.info("email enable notifs: %r", hs.config.email.email_enable_notifs) | ||
if hs.config.email.email_enable_notifs: | ||
self.mailers: Dict[str, Mailer] = {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It matters if the client isn't aware of this change