From a3354cbedbe5d47c069cb3f62d08f1b9757e82f7 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 31 Mar 2023 14:35:55 +0200
Subject: [PATCH 01/17] Add a module API to send an HTTP push notification

---
 changelog.d/15387.feature      |   1 +
 synapse/module_api/__init__.py |  39 ++++++++++
 synapse/push/httppusher.py     | 134 ++++++++++++++++++---------------
 3 files changed, 113 insertions(+), 61 deletions(-)
 create mode 100644 changelog.d/15387.feature

diff --git a/changelog.d/15387.feature b/changelog.d/15387.feature
new file mode 100644
index 000000000000..b36e33152049
--- /dev/null
+++ b/changelog.d/15387.feature
@@ -0,0 +1 @@
+Add a module API to send an HTTP push notification.
diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 595c23e78d7e..71d7feb39d1f 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -105,6 +105,7 @@
     ON_LEGACY_SEND_MAIL_CALLBACK,
     ON_USER_REGISTRATION_CALLBACK,
 )
+from synapse.push.httppusher import HttpPusher
 from synapse.rest.client.login import LoginResponse
 from synapse.storage import DataStore
 from synapse.storage.background_updates import (
@@ -248,6 +249,7 @@ def __init__(self, hs: "HomeServer", auth_handler: AuthHandler) -> None:
         self._registration_handler = hs.get_registration_handler()
         self._send_email_handler = hs.get_send_email_handler()
         self._push_rules_handler = hs.get_push_rules_handler()
+        self._pusherpool = hs.get_pusherpool()
         self._device_handler = hs.get_device_handler()
         self.custom_template_dir = hs.config.server.custom_template_directory
         self._callbacks = hs.get_module_api_callbacks()
@@ -1226,6 +1228,43 @@ async def sleep(self, seconds: float) -> None:
 
         await self._clock.sleep(seconds)
 
+    async def send_http_push_notification(
+        self,
+        user_id: str,
+        device_id: Optional[str],
+        content: JsonDict,
+        tweaks: Optional[JsonMapping] = None,
+    ) -> bool:
+        """Send an HTTP push notification that is forwarded to the registered push gateway
+        for the specified user/device.
+
+        Added in Synapse v1.82.0.
+
+        Args:
+            user_id: The user ID to send the push notification to.
+            device_id: The device ID of the device where to send the push notification. If `None`,
+            the notification will be sent to all registered HTTP pushers of the user.
+            content: A dict of values that will be put in the `notification` field of the push
+            (cf Push Gatway spec). `devices` field will be overrided if included.
+            tweaks: A dict of `tweaks` that will be inserted in the `devices` section, cf spec.
+
+        Returns:
+            True if at least one push was succesfully sent, False if no matching pusher has been
+            found or an error occured when sending the push.
+        """
+        sent = False
+        if user_id in self._pusherpool.pushers:
+            for p in self._pusherpool.pushers[user_id].values():
+                if isinstance(p, HttpPusher) and (
+                    not device_id or p.device_id == device_id
+                ):
+                    res = await p.dispatch_push(content, tweaks)
+                    if (
+                        isinstance(res, (list, tuple)) and len(res) == 0
+                    ) or res is not False:
+                        sent = True
+        return sent
+
     async def send_mail(
         self,
         recipient: str,
diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index b048b03a7477..ed1683d52102 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -14,7 +14,7 @@
 # limitations under the License.
 import logging
 import urllib.parse
-from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Union
+from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union
 
 from prometheus_client import Counter
 
@@ -27,6 +27,7 @@
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.push import Pusher, PusherConfig, PusherConfigException
 from synapse.storage.databases.main.event_push_actions import HttpPushAction
+from synapse.types import JsonDict, JsonMapping
 
 from . import push_tools
 
@@ -56,7 +57,7 @@
 )
 
 
-def tweaks_for_actions(actions: List[Union[str, Dict]]) -> Dict[str, Any]:
+def tweaks_for_actions(actions: List[Union[str, Dict]]) -> JsonMapping:
     """
     Converts a list of actions into a `tweaks` dict (which can then be passed to
         the push gateway).
@@ -101,6 +102,7 @@ def __init__(self, hs: "HomeServer", pusher_config: PusherConfig):
         self._storage_controllers = self.hs.get_storage_controllers()
         self.app_display_name = pusher_config.app_display_name
         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
@@ -324,7 +326,7 @@ async def _process_one(self, push_action: HttpPushAction) -> bool:
         event = await self.store.get_event(push_action.event_id, allow_none=True)
         if event is None:
             return True  # It's been redacted
-        rejected = await self.dispatch_push(event, tweaks, badge)
+        rejected = await self.dispatch_push_event(event, tweaks, badge)
         if rejected is False:
             return False
 
@@ -342,9 +344,12 @@ async def _process_one(self, push_action: HttpPushAction) -> bool:
                     await self._pusherpool.remove_pusher(self.app_id, pk, self.user_id)
         return True
 
-    async def _build_notification_dict(
-        self, event: EventBase, tweaks: Dict[str, bool], badge: int
-    ) -> Dict[str, Any]:
+    async def _build_event_notification(
+        self,
+        event: EventBase,
+        tweaks: JsonMapping,
+        badge: int,
+    ) -> Tuple[JsonDict, JsonMapping]:
         priority = "low"
         if (
             event.type == EventTypes.Encrypted
@@ -358,80 +363,70 @@ async def _build_notification_dict(
         # This was checked in the __init__, but mypy doesn't seem to know that.
         assert self.data is not None
         if self.data.get("format") == "event_id_only":
-            d: Dict[str, Any] = {
-                "notification": {
-                    "event_id": event.event_id,
-                    "room_id": event.room_id,
-                    "counts": {"unread": badge},
-                    "prio": priority,
-                    "devices": [
-                        {
-                            "app_id": self.app_id,
-                            "pushkey": self.pushkey,
-                            "pushkey_ts": int(self.pushkey_ts / 1000),
-                            "data": self.data_minus_url,
-                        }
-                    ],
-                }
+            content: JsonDict = {
+                "event_id": event.event_id,
+                "room_id": event.room_id,
+                "counts": {"unread": badge},
+                "prio": priority,
             }
-            return d
+            return content, {}
 
         ctx = await push_tools.get_context_for_event(
             self._storage_controllers, event, self.user_id
         )
 
-        d = {
-            "notification": {
-                "id": event.event_id,  # deprecated: remove soon
-                "event_id": event.event_id,
-                "room_id": event.room_id,
-                "type": event.type,
-                "sender": event.user_id,
-                "prio": priority,
-                "counts": {
-                    "unread": badge,
-                    # 'missed_calls': 2
-                },
-                "devices": [
-                    {
-                        "app_id": self.app_id,
-                        "pushkey": self.pushkey,
-                        "pushkey_ts": int(self.pushkey_ts / 1000),
-                        "data": self.data_minus_url,
-                        "tweaks": tweaks,
-                    }
-                ],
-            }
+        content = {
+            "id": event.event_id,  # deprecated: remove soon
+            "event_id": event.event_id,
+            "room_id": event.room_id,
+            "type": event.type,
+            "sender": event.user_id,
+            "prio": priority,
+            "counts": {
+                "unread": badge,
+                # 'missed_calls': 2
+            },
         }
         if event.type == "m.room.member" and event.is_state():
-            d["notification"]["membership"] = event.content["membership"]
-            d["notification"]["user_is_target"] = event.state_key == self.user_id
+            content["membership"] = event.content["membership"]
+            content["user_is_target"] = event.state_key == self.user_id
         if self.hs.config.push.push_include_content and event.content:
-            d["notification"]["content"] = event.content
+            content["content"] = event.content
 
         # We no longer send aliases separately, instead, we send the human
         # readable name of the room, which may be an alias.
         if "sender_display_name" in ctx and len(ctx["sender_display_name"]) > 0:
-            d["notification"]["sender_display_name"] = ctx["sender_display_name"]
+            content["sender_display_name"] = ctx["sender_display_name"]
         if "name" in ctx and len(ctx["name"]) > 0:
-            d["notification"]["room_name"] = ctx["name"]
+            content["room_name"] = ctx["name"]
+
+        return (content, tweaks)
+
+    def _build_notification_dict(
+        self, content: JsonDict, tweaks: Optional[JsonMapping]
+    ) -> JsonDict:
+        device = {
+            "app_id": self.app_id,
+            "pushkey": self.pushkey,
+            "pushkey_ts": int(self.pushkey_ts / 1000),
+            "data": self.data_minus_url,
+        }
+        if tweaks:
+            device["tweaks"] = tweaks
 
-        return d
+        content["devices"] = [device]
+
+        return {"notification": content}
 
     async def dispatch_push(
-        self, event: EventBase, tweaks: Dict[str, bool], badge: int
+        self, content: JsonDict, tweaks: Optional[JsonMapping] = None
     ) -> Union[bool, Iterable[str]]:
-        notification_dict = await self._build_notification_dict(event, tweaks, badge)
-        if not notification_dict:
-            return []
+        notif_dict = self._build_notification_dict(content, tweaks)
         try:
-            resp = await self.http_client.post_json_get_json(
-                self.url, notification_dict
-            )
+            resp = await self.http_client.post_json_get_json(self.url, notif_dict)
         except Exception as e:
             logger.warning(
-                "Failed to push event %s to %s: %s %s",
-                event.event_id,
+                "Failed to push data to %s: %s %s",
                 self.name,
                 type(e),
                 e,
@@ -440,10 +435,27 @@ async def dispatch_push(
         rejected = []
         if "rejected" in resp:
             rejected = resp["rejected"]
-        if not rejected:
-            self.badge_count_last_call = badge
         return rejected
 
+    async def dispatch_push_event(
+        self,
+        event: EventBase,
+        tweaks: JsonMapping,
+        badge: int,
+    ) -> Union[bool, Iterable[str]]:
+        content, tweaks = await self._build_event_notification(event, tweaks, badge)
+        if not content:
+            return []
+
+        res = await self.dispatch_push(content, tweaks)
+
+        if res is False:
+            return False
+        if not res:
+            self.badge_count_last_call = badge
+
+        return res
+
     async def _send_badge(self, badge: int) -> None:
         """
         Args:

From 71490681a93f53d7dca5a016dbefdad786f52ba8 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Wed, 5 Apr 2023 17:54:10 +0200
Subject: [PATCH 02/17] Adress comments

---
 synapse/module_api/__init__.py | 15 +++++---
 synapse/push/httppusher.py     | 64 +++++++++++++++++++++++++++++++---
 2 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 71d7feb39d1f..16c8fade8dd2 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1234,7 +1234,7 @@ async def send_http_push_notification(
         device_id: Optional[str],
         content: JsonDict,
         tweaks: Optional[JsonMapping] = None,
-    ) -> bool:
+    ) -> Dict[str, bool]:
         """Send an HTTP push notification that is forwarded to the registered push gateway
         for the specified user/device.
 
@@ -1249,21 +1249,26 @@ async def send_http_push_notification(
             tweaks: A dict of `tweaks` that will be inserted in the `devices` section, cf spec.
 
         Returns:
-            True if at least one push was succesfully sent, False if no matching pusher has been
-            found or an error occured when sending the push.
+            a dict reprensenting the status of the push per device ID
         """
-        sent = False
+        status = {}
         if user_id in self._pusherpool.pushers:
             for p in self._pusherpool.pushers[user_id].values():
                 if isinstance(p, HttpPusher) and (
                     not device_id or p.device_id == device_id
                 ):
+                    sent = False
                     res = await p.dispatch_push(content, tweaks)
                     if (
                         isinstance(res, (list, tuple)) and len(res) == 0
                     ) or res is not False:
                         sent = True
-        return sent
+                    # This is mainly to accomodate mypy
+                    # device_id should never be empty after the `set_device_id_for_pushers`
+                    # background job has been properly run.
+                    if p.device_id:
+                        status[p.device_id] = sent
+        return status
 
     async def send_mail(
         self,
diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index ed1683d52102..f8c45b2e9648 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -344,12 +344,28 @@ async def _process_one(self, push_action: HttpPushAction) -> bool:
                     await self._pusherpool.remove_pusher(self.app_id, pk, self.user_id)
         return True
 
-    async def _build_event_notification(
+    async def _build_event_notification_content(
         self,
         event: EventBase,
         tweaks: JsonMapping,
         badge: int,
     ) -> Tuple[JsonDict, JsonMapping]:
+        """Build the content of a push notification from an event, as specified by the
+        Push Gateway spec. This excludes the top level `notification` element and
+        is not taking care of the `devices` section either, those are handled by
+        `_build_notification_dict`.
+
+        Args:
+            event: the event
+            tweaks: tweaks to apply
+            badge: unread count to send with the push notification
+
+        Returns: a tuple `(content, tweaks)`, where:
+           * `content` is the content to put in `notification` before sending it to the
+           push gateway.
+           * `tweaks` is the tweaks to put in the `devices` section, they could be
+           different from the ones inputed in this function.
+        """
         priority = "low"
         if (
             event.type == EventTypes.Encrypted
@@ -405,6 +421,18 @@ async def _build_event_notification(
     def _build_notification_dict(
         self, content: JsonDict, tweaks: Optional[JsonMapping]
     ) -> JsonDict:
+        """Build a full notification from the content of it, as specified by the
+        Push Gateway spec. It wraps the content in a top level `notification`
+        element and put relevant device info in the `devices` section.
+
+        Args:
+            content: the content
+            tweaks: tweaks to add into the `devices` section
+
+        Returns:
+            a full notification that can be send to the registered push gateway.
+        """
+        content = content.copy()
         device = {
             "app_id": self.app_id,
             "pushkey": self.pushkey,
@@ -421,6 +449,20 @@ def _build_notification_dict(
     async def dispatch_push(
         self, content: JsonDict, tweaks: Optional[JsonMapping] = None
     ) -> Union[bool, Iterable[str]]:
+        """Send a notification to the registered push gateway, with `content` being
+        the content of the `notification` top property specified in the spec.
+        If specified `devices` property will be overrided with device-specific
+        information for this pusher.
+
+        Args:
+            content: the content
+            tweaks: tweaks to add into the `devices` section
+
+        Returns:
+            False if an error occured when calling the push gateway, or an array of
+            rejected push keys otherwise. If this array is empty, the push fully
+            succeeded.
+        """
         notif_dict = self._build_notification_dict(content, tweaks)
         try:
             resp = await self.http_client.post_json_get_json(self.url, notif_dict)
@@ -443,9 +485,23 @@ async def dispatch_push_event(
         tweaks: JsonMapping,
         badge: int,
     ) -> Union[bool, Iterable[str]]:
-        content, tweaks = await self._build_event_notification(event, tweaks, badge)
-        if not content:
-            return []
+        """Send a notification to the registered push gateway by building it
+        from an event.
+
+        Args:
+            event: the event
+            tweaks: tweaks to add into the `devices` section, and to decide the
+            priority to use
+            badge: unread count to send with the push notification
+
+        Returns:
+            False if an error occured when calling the push gateway, or an array of
+            rejected push keys otherwise. If this array is empty, the push fully
+            succeeded.
+        """
+        content, tweaks = await self._build_event_notification_content(
+            event, tweaks, badge
+        )
 
         res = await self.dispatch_push(content, tweaks)
 

From c553bff8b999a646d5c9284ae3f0a0865267456e Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Wed, 5 Apr 2023 17:55:50 +0200
Subject: [PATCH 03/17] Update synapse/module_api/__init__.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/module_api/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 16c8fade8dd2..bf9999e070a5 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1245,7 +1245,7 @@ async def send_http_push_notification(
             device_id: The device ID of the device where to send the push notification. If `None`,
             the notification will be sent to all registered HTTP pushers of the user.
             content: A dict of values that will be put in the `notification` field of the push
-            (cf Push Gatway spec). `devices` field will be overrided if included.
+            (cf Push Gateway spec). `devices` field will be overrided if included.
             tweaks: A dict of `tweaks` that will be inserted in the `devices` section, cf spec.
 
         Returns:

From 5cbb9371ef3f1b7d8ae4dacba01be3d9d9c26719 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Thu, 6 Apr 2023 10:20:22 +0200
Subject: [PATCH 04/17] Add support for default_payload

---
 synapse/push/httppusher.py | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index f8c45b2e9648..72f687550493 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -419,7 +419,10 @@ async def _build_event_notification_content(
         return (content, tweaks)
 
     def _build_notification_dict(
-        self, content: JsonDict, tweaks: Optional[JsonMapping]
+        self,
+        content: JsonDict,
+        tweaks: Optional[JsonMapping],
+        default_payload: Optional[JsonMapping] = None,
     ) -> JsonDict:
         """Build a full notification from the content of it, as specified by the
         Push Gateway spec. It wraps the content in a top level `notification`
@@ -428,16 +431,26 @@ def _build_notification_dict(
         Args:
             content: the content
             tweaks: tweaks to add into the `devices` section
+            default_payload: default payload to add in `devices[0].data.default_payload`.
+            This will be merged (and override if some matching values already exist there)
+            with existing default_payload.
 
         Returns:
             a full notification that can be send to the registered push gateway.
         """
         content = content.copy()
+
+        data = self.data_minus_url.copy()
+        if default_payload:
+            if "default_payload" not in data:
+                data["default_payload"] = {}
+            data["default_payload"].update(default_payload)
+
         device = {
             "app_id": self.app_id,
             "pushkey": self.pushkey,
             "pushkey_ts": int(self.pushkey_ts / 1000),
-            "data": self.data_minus_url,
+            "data": data,
         }
         if tweaks:
             device["tweaks"] = tweaks
@@ -447,7 +460,10 @@ def _build_notification_dict(
         return {"notification": content}
 
     async def dispatch_push(
-        self, content: JsonDict, tweaks: Optional[JsonMapping] = None
+        self,
+        content: JsonDict,
+        tweaks: Optional[JsonMapping] = None,
+        default_payload: Optional[JsonMapping] = None,
     ) -> Union[bool, Iterable[str]]:
         """Send a notification to the registered push gateway, with `content` being
         the content of the `notification` top property specified in the spec.
@@ -463,7 +479,7 @@ async def dispatch_push(
             rejected push keys otherwise. If this array is empty, the push fully
             succeeded.
         """
-        notif_dict = self._build_notification_dict(content, tweaks)
+        notif_dict = self._build_notification_dict(content, tweaks, default_payload)
         try:
             resp = await self.http_client.post_json_get_json(self.url, notif_dict)
         except Exception as e:

From 9f49c20d09ed71cef9d1756a00e53b890c06fc94 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Thu, 6 Apr 2023 11:17:24 +0200
Subject: [PATCH 05/17] Missing param

---
 synapse/module_api/__init__.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index bf9999e070a5..1ed8c07451d8 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1234,6 +1234,7 @@ async def send_http_push_notification(
         device_id: Optional[str],
         content: JsonDict,
         tweaks: Optional[JsonMapping] = None,
+        default_payload: Optional[JsonMapping] = None,
     ) -> Dict[str, bool]:
         """Send an HTTP push notification that is forwarded to the registered push gateway
         for the specified user/device.
@@ -1247,6 +1248,9 @@ async def send_http_push_notification(
             content: A dict of values that will be put in the `notification` field of the push
             (cf Push Gateway spec). `devices` field will be overrided if included.
             tweaks: A dict of `tweaks` that will be inserted in the `devices` section, cf spec.
+            default_payload: default payload to add in `devices[0].data.default_payload`.
+            This will be merged (and override if some matching values already exist there)
+            with existing default_payload.
 
         Returns:
             a dict reprensenting the status of the push per device ID
@@ -1258,7 +1262,7 @@ async def send_http_push_notification(
                     not device_id or p.device_id == device_id
                 ):
                     sent = False
-                    res = await p.dispatch_push(content, tweaks)
+                    res = await p.dispatch_push(content, tweaks, default_payload)
                     if (
                         isinstance(res, (list, tuple)) and len(res) == 0
                     ) or res is not False:

From ceb4a9b4af6c3d0df15734e0d26f5ca57cf44eba Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 7 Apr 2023 13:32:33 +0200
Subject: [PATCH 06/17] Docstring tweaks

---
 synapse/module_api/__init__.py | 2 +-
 synapse/push/httppusher.py     | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 1ed8c07451d8..e04cc4fa1287 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1250,7 +1250,7 @@ async def send_http_push_notification(
             tweaks: A dict of `tweaks` that will be inserted in the `devices` section, cf spec.
             default_payload: default payload to add in `devices[0].data.default_payload`.
             This will be merged (and override if some matching values already exist there)
-            with existing default_payload.
+            with existing `default_payload`.
 
         Returns:
             a dict reprensenting the status of the push per device ID
diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 72f687550493..5823e1a35af9 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -433,7 +433,7 @@ def _build_notification_dict(
             tweaks: tweaks to add into the `devices` section
             default_payload: default payload to add in `devices[0].data.default_payload`.
             This will be merged (and override if some matching values already exist there)
-            with existing default_payload.
+            with existing `default_payload`.
 
         Returns:
             a full notification that can be send to the registered push gateway.
@@ -473,6 +473,9 @@ async def dispatch_push(
         Args:
             content: the content
             tweaks: tweaks to add into the `devices` section
+            default_payload: default payload to add in `devices[0].data.default_payload`.
+            This will be merged (and override if some matching values already exist there)
+            with existing `default_payload`.
 
         Returns:
             False if an error occured when calling the push gateway, or an array of

From dd2f395444f5cf07d7ece36eabbaeb00260f8cdc Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Tue, 11 Apr 2023 16:42:33 +0200
Subject: [PATCH 07/17] Apply suggestions from code review

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/push/httppusher.py | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 5823e1a35af9..7a219734d39b 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -350,21 +350,23 @@ async def _build_event_notification_content(
         tweaks: JsonMapping,
         badge: int,
     ) -> Tuple[JsonDict, JsonMapping]:
-        """Build the content of a push notification from an event, as specified by the
-        Push Gateway spec. This excludes the top level `notification` element and
-        is not taking care of the `devices` section either, those are handled by
-        `_build_notification_dict`.
+        """
+        Build the content of a push notification from an event, as specified by the
+        Push Gateway spec.
+
+        _build_notification_dict uses this to build the notification field and handles
+        adding the devices field.
 
         Args:
             event: the event
             tweaks: tweaks to apply
             badge: unread count to send with the push notification
 
-        Returns: a tuple `(content, tweaks)`, where:
-           * `content` is the content to put in `notification` before sending it to the
-           push gateway.
-           * `tweaks` is the tweaks to put in the `devices` section, they could be
-           different from the ones inputed in this function.
+        Returns:
+            A tuple of:
+                The content to put in `notification` before sending it to the push gateway.
+
+                The tweaks to put in the `devices` section, they may be different from the input.
         """
         priority = "low"
         if (
@@ -432,8 +434,8 @@ def _build_notification_dict(
             content: the content
             tweaks: tweaks to add into the `devices` section
             default_payload: default payload to add in `devices[0].data.default_payload`.
-            This will be merged (and override if some matching values already exist there)
-            with existing `default_payload`.
+                This will be merged (and override if some matching values already exist there)
+                with existing `default_payload`.
 
         Returns:
             a full notification that can be send to the registered push gateway.
@@ -467,15 +469,15 @@ async def dispatch_push(
     ) -> Union[bool, Iterable[str]]:
         """Send a notification to the registered push gateway, with `content` being
         the content of the `notification` top property specified in the spec.
-        If specified `devices` property will be overrided with device-specific
+        If specified, the `devices` property will be overridden with device-specific
         information for this pusher.
 
         Args:
             content: the content
             tweaks: tweaks to add into the `devices` section
             default_payload: default payload to add in `devices[0].data.default_payload`.
-            This will be merged (and override if some matching values already exist there)
-            with existing `default_payload`.
+                This will be merged (and override if some matching values already exist there)
+                with existing `default_payload`.
 
         Returns:
             False if an error occured when calling the push gateway, or an array of

From 6a61b0bd0b057cecb91cdd38bb28efa78c57d38a Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Tue, 11 Apr 2023 16:50:16 +0200
Subject: [PATCH 08/17] Update synapse/push/httppusher.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/push/httppusher.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 7a219734d39b..97a1a871a123 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -444,9 +444,7 @@ def _build_notification_dict(
 
         data = self.data_minus_url.copy()
         if default_payload:
-            if "default_payload" not in data:
-                data["default_payload"] = {}
-            data["default_payload"].update(default_payload)
+            data.setdefault("default_payload", {}).update(default_payload)
 
         device = {
             "app_id": self.app_id,

From d395157bf9b1a90c5bda015cbd94b21c924bc4e0 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Tue, 11 Apr 2023 17:09:50 +0200
Subject: [PATCH 09/17] Address comments

---
 synapse/push/httppusher.py | 185 ++++++++++++++-----------------------
 1 file changed, 68 insertions(+), 117 deletions(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 97a1a871a123..41a96804b0d3 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -14,7 +14,7 @@
 # limitations under the License.
 import logging
 import urllib.parse
-from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Dict, Iterable, List, Mapping, Optional, Tuple, Union
 
 from prometheus_client import Counter
 
@@ -27,7 +27,7 @@
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.push import Pusher, PusherConfig, PusherConfigException
 from synapse.storage.databases.main.event_push_actions import HttpPushAction
-from synapse.types import JsonDict, JsonMapping
+from synapse.types import JsonDict, JsonMapping, SimpleJsonValue
 
 from . import push_tools
 
@@ -57,7 +57,9 @@
 )
 
 
-def tweaks_for_actions(actions: List[Union[str, Dict]]) -> JsonMapping:
+def tweaks_for_actions(
+    actions: List[Union[str, Dict]]
+) -> Mapping[str, SimpleJsonValue]:
     """
     Converts a list of actions into a `tweaks` dict (which can then be passed to
         the push gateway).
@@ -344,91 +346,16 @@ async def _process_one(self, push_action: HttpPushAction) -> bool:
                     await self._pusherpool.remove_pusher(self.app_id, pk, self.user_id)
         return True
 
-    async def _build_event_notification_content(
-        self,
-        event: EventBase,
-        tweaks: JsonMapping,
-        badge: int,
-    ) -> Tuple[JsonDict, JsonMapping]:
-        """
-        Build the content of a push notification from an event, as specified by the
-        Push Gateway spec.
-
-        _build_notification_dict uses this to build the notification field and handles
-        adding the devices field.
-
-        Args:
-            event: the event
-            tweaks: tweaks to apply
-            badge: unread count to send with the push notification
-
-        Returns:
-            A tuple of:
-                The content to put in `notification` before sending it to the push gateway.
-
-                The tweaks to put in the `devices` section, they may be different from the input.
-        """
-        priority = "low"
-        if (
-            event.type == EventTypes.Encrypted
-            or tweaks.get("highlight")
-            or tweaks.get("sound")
-        ):
-            # HACK send our push as high priority only if it generates a sound, highlight
-            #  or may do so (i.e. is encrypted so has unknown effects).
-            priority = "high"
-
-        # This was checked in the __init__, but mypy doesn't seem to know that.
-        assert self.data is not None
-        if self.data.get("format") == "event_id_only":
-            content: JsonDict = {
-                "event_id": event.event_id,
-                "room_id": event.room_id,
-                "counts": {"unread": badge},
-                "prio": priority,
-            }
-            return content, {}
-
-        ctx = await push_tools.get_context_for_event(
-            self._storage_controllers, event, self.user_id
-        )
-
-        content = {
-            "id": event.event_id,  # deprecated: remove soon
-            "event_id": event.event_id,
-            "room_id": event.room_id,
-            "type": event.type,
-            "sender": event.user_id,
-            "prio": priority,
-            "counts": {
-                "unread": badge,
-                # 'missed_calls': 2
-            },
-        }
-        if event.type == "m.room.member" and event.is_state():
-            content["membership"] = event.content["membership"]
-            content["user_is_target"] = event.state_key == self.user_id
-        if self.hs.config.push.push_include_content and event.content:
-            content["content"] = event.content
-
-        # We no longer send aliases separately, instead, we send the human
-        # readable name of the room, which may be an alias.
-        if "sender_display_name" in ctx and len(ctx["sender_display_name"]) > 0:
-            content["sender_display_name"] = ctx["sender_display_name"]
-        if "name" in ctx and len(ctx["name"]) > 0:
-            content["room_name"] = ctx["name"]
-
-        return (content, tweaks)
-
-    def _build_notification_dict(
+    async def dispatch_push(
         self,
         content: JsonDict,
-        tweaks: Optional[JsonMapping],
+        tweaks: Optional[Mapping[str, SimpleJsonValue]] = None,
         default_payload: Optional[JsonMapping] = None,
-    ) -> JsonDict:
-        """Build a full notification from the content of it, as specified by the
-        Push Gateway spec. It wraps the content in a top level `notification`
-        element and put relevant device info in the `devices` section.
+    ) -> Union[bool, Iterable[str]]:
+        """Send a notification to the registered push gateway, with `content` being
+        the content of the `notification` top property specified in the spec.
+        If specified, the `devices` property will be overridden with device-specific
+        information for this pusher.
 
         Args:
             content: the content
@@ -438,7 +365,9 @@ def _build_notification_dict(
                 with existing `default_payload`.
 
         Returns:
-            a full notification that can be send to the registered push gateway.
+            False if an error occured when calling the push gateway, or an array of
+            rejected push keys otherwise. If this array is empty, the push fully
+            succeeded.
         """
         content = content.copy()
 
@@ -457,34 +386,10 @@ def _build_notification_dict(
 
         content["devices"] = [device]
 
-        return {"notification": content}
-
-    async def dispatch_push(
-        self,
-        content: JsonDict,
-        tweaks: Optional[JsonMapping] = None,
-        default_payload: Optional[JsonMapping] = None,
-    ) -> Union[bool, Iterable[str]]:
-        """Send a notification to the registered push gateway, with `content` being
-        the content of the `notification` top property specified in the spec.
-        If specified, the `devices` property will be overridden with device-specific
-        information for this pusher.
-
-        Args:
-            content: the content
-            tweaks: tweaks to add into the `devices` section
-            default_payload: default payload to add in `devices[0].data.default_payload`.
-                This will be merged (and override if some matching values already exist there)
-                with existing `default_payload`.
-
-        Returns:
-            False if an error occured when calling the push gateway, or an array of
-            rejected push keys otherwise. If this array is empty, the push fully
-            succeeded.
-        """
-        notif_dict = self._build_notification_dict(content, tweaks, default_payload)
         try:
-            resp = await self.http_client.post_json_get_json(self.url, notif_dict)
+            resp = await self.http_client.post_json_get_json(
+                self.url, {"notification": content}
+            )
         except Exception as e:
             logger.warning(
                 "Failed to push data to %s: %s %s",
@@ -501,7 +406,7 @@ async def dispatch_push(
     async def dispatch_push_event(
         self,
         event: EventBase,
-        tweaks: JsonMapping,
+        tweaks: Mapping[str, SimpleJsonValue],
         badge: int,
     ) -> Union[bool, Iterable[str]]:
         """Send a notification to the registered push gateway by building it
@@ -518,9 +423,55 @@ async def dispatch_push_event(
             rejected push keys otherwise. If this array is empty, the push fully
             succeeded.
         """
-        content, tweaks = await self._build_event_notification_content(
-            event, tweaks, badge
-        )
+        priority = "low"
+        if (
+            event.type == EventTypes.Encrypted
+            or tweaks.get("highlight")
+            or tweaks.get("sound")
+        ):
+            # HACK send our push as high priority only if it generates a sound, highlight
+            #  or may do so (i.e. is encrypted so has unknown effects).
+            priority = "high"
+
+        # This was checked in the __init__, but mypy doesn't seem to know that.
+        assert self.data is not None
+        if self.data.get("format") == "event_id_only":
+            content: JsonDict = {
+                "event_id": event.event_id,
+                "room_id": event.room_id,
+                "counts": {"unread": badge},
+                "prio": priority,
+            }
+            tweaks = {}
+        else:
+            ctx = await push_tools.get_context_for_event(
+                self._storage_controllers, event, self.user_id
+            )
+
+            content = {
+                "id": event.event_id,  # deprecated: remove soon
+                "event_id": event.event_id,
+                "room_id": event.room_id,
+                "type": event.type,
+                "sender": event.user_id,
+                "prio": priority,
+                "counts": {
+                    "unread": badge,
+                    # 'missed_calls': 2
+                },
+            }
+            if event.type == "m.room.member" and event.is_state():
+                content["membership"] = event.content["membership"]
+                content["user_is_target"] = event.state_key == self.user_id
+            if self.hs.config.push.push_include_content and event.content:
+                content["content"] = event.content
+
+            # We no longer send aliases separately, instead, we send the human
+            # readable name of the room, which may be an alias.
+            if "sender_display_name" in ctx and len(ctx["sender_display_name"]) > 0:
+                content["sender_display_name"] = ctx["sender_display_name"]
+            if "name" in ctx and len(ctx["name"]) > 0:
+                content["room_name"] = ctx["name"]
 
         res = await self.dispatch_push(content, tweaks)
 

From 84d5aaaaf6b7508d67a5dff0c756f4f12398bcab Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Tue, 11 Apr 2023 17:12:53 +0200
Subject: [PATCH 10/17] Unused import

---
 synapse/push/httppusher.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 41a96804b0d3..47a28855dfeb 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -14,7 +14,7 @@
 # limitations under the License.
 import logging
 import urllib.parse
-from typing import TYPE_CHECKING, Dict, Iterable, List, Mapping, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Dict, Iterable, List, Mapping, Optional, Union
 
 from prometheus_client import Counter
 

From 28ac8a027fd3922b460f0f4e3ea10e5432bd5950 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 09:59:30 +0200
Subject: [PATCH 11/17] Update synapse/push/httppusher.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/push/httppusher.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 47a28855dfeb..5089b51488fd 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -354,7 +354,7 @@ async def dispatch_push(
     ) -> Union[bool, Iterable[str]]:
         """Send a notification to the registered push gateway, with `content` being
         the content of the `notification` top property specified in the spec.
-        If specified, the `devices` property will be overridden with device-specific
+        Note that the `devices` property will be added with device-specific
         information for this pusher.
 
         Args:

From 7e9a3f3049ec4741db0b2b225b87acd83f364092 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 10:02:23 +0200
Subject: [PATCH 12/17] Apply suggestions from code review

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/push/httppusher.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 5089b51488fd..81d87b8d4859 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -351,7 +351,7 @@ async def dispatch_push(
         content: JsonDict,
         tweaks: Optional[Mapping[str, SimpleJsonValue]] = None,
         default_payload: Optional[JsonMapping] = None,
-    ) -> Union[bool, Iterable[str]]:
+    ) -> Union[bool, List[str]]:
         """Send a notification to the registered push gateway, with `content` being
         the content of the `notification` top property specified in the spec.
         Note that the `devices` property will be added with device-specific
@@ -414,8 +414,8 @@ async def dispatch_push_event(
 
         Args:
             event: the event
-            tweaks: tweaks to add into the `devices` section, and to decide the
-            priority to use
+            tweaks: tweaks to add into the `devices` section, used to decide the
+                push priority
             badge: unread count to send with the push notification
 
         Returns:
@@ -442,6 +442,7 @@ async def dispatch_push_event(
                 "counts": {"unread": badge},
                 "prio": priority,
             }
+            # event_id_only doesn't include the tweaks, so override them.
             tweaks = {}
         else:
             ctx = await push_tools.get_context_for_event(

From 3a374f336d35f0a16aac92b1b4bec7372d3bbf31 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 17:31:33 +0200
Subject: [PATCH 13/17] address comments

---
 synapse/module_api/__init__.py |  7 +++----
 synapse/push/httppusher.py     | 18 +++++++-----------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index e04cc4fa1287..50b05ac3b702 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1263,10 +1263,9 @@ async def send_http_push_notification(
                 ):
                     sent = False
                     res = await p.dispatch_push(content, tweaks, default_payload)
-                    if (
-                        isinstance(res, (list, tuple)) and len(res) == 0
-                    ) or res is not False:
-                        sent = True
+                    # Check if the push was successful and no pushers were rejected.
+                    sent = res is not False and not res
+
                     # This is mainly to accomodate mypy
                     # device_id should never be empty after the `set_device_id_for_pushers`
                     # background job has been properly run.
diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 81d87b8d4859..a24df5d01683 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -57,9 +57,7 @@
 )
 
 
-def tweaks_for_actions(
-    actions: List[Union[str, Dict]]
-) -> Mapping[str, SimpleJsonValue]:
+def tweaks_for_actions(actions: List[Union[str, Dict]]) -> JsonMapping:
     """
     Converts a list of actions into a `tweaks` dict (which can then be passed to
         the push gateway).
@@ -349,7 +347,7 @@ async def _process_one(self, push_action: HttpPushAction) -> bool:
     async def dispatch_push(
         self,
         content: JsonDict,
-        tweaks: Optional[Mapping[str, SimpleJsonValue]] = None,
+        tweaks: Optional[JsonMapping] = None,
         default_payload: Optional[JsonMapping] = None,
     ) -> Union[bool, List[str]]:
         """Send a notification to the registered push gateway, with `content` being
@@ -406,9 +404,9 @@ async def dispatch_push(
     async def dispatch_push_event(
         self,
         event: EventBase,
-        tweaks: Mapping[str, SimpleJsonValue],
+        tweaks: JsonMapping,
         badge: int,
-    ) -> Union[bool, Iterable[str]]:
+    ) -> Union[bool, List[str]]:
         """Send a notification to the registered push gateway by building it
         from an event.
 
@@ -476,11 +474,9 @@ async def dispatch_push_event(
 
         res = await self.dispatch_push(content, tweaks)
 
-        if res is False:
-            return False
-        if not res:
-            self.badge_count_last_call = badge
-
+        # If the push is successful and none are rejected, update the badge count.
+        if res is not False and not res:
+            self.bad_count_last_call = badge
         return res
 
     async def _send_badge(self, badge: int) -> None:

From e8d5340958f3aa5902f2c389f3d4d922cb932e88 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 17:33:42 +0200
Subject: [PATCH 14/17] unused type

---
 synapse/push/httppusher.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index a24df5d01683..4139e1296ba1 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -27,7 +27,7 @@
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.push import Pusher, PusherConfig, PusherConfigException
 from synapse.storage.databases.main.event_push_actions import HttpPushAction
-from synapse.types import JsonDict, JsonMapping, SimpleJsonValue
+from synapse.types import JsonDict, JsonMapping
 
 from . import push_tools
 

From 161c38c0adcecbb08c2077bc2a127b3b41dc3807 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 17:37:33 +0200
Subject: [PATCH 15/17] unused imports

---
 synapse/push/httppusher.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index 4139e1296ba1..edd0a85d1295 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -14,7 +14,7 @@
 # limitations under the License.
 import logging
 import urllib.parse
-from typing import TYPE_CHECKING, Dict, Iterable, List, Mapping, Optional, Union
+from typing import TYPE_CHECKING, Dict, List, Optional, Union
 
 from prometheus_client import Counter
 

From 6d0b1ecbfae89c6d82fce4c17f351b20c0e49ad1 Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 21 Apr 2023 18:09:41 +0200
Subject: [PATCH 16/17] typo

---
 synapse/push/httppusher.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py
index edd0a85d1295..4f8fa445d951 100644
--- a/synapse/push/httppusher.py
+++ b/synapse/push/httppusher.py
@@ -476,7 +476,8 @@ async def dispatch_push_event(
 
         # If the push is successful and none are rejected, update the badge count.
         if res is not False and not res:
-            self.bad_count_last_call = badge
+            self.badge_count_last_call = badge
+
         return res
 
     async def _send_badge(self, badge: int) -> None:

From 76c321ea79eeafa767bd6a0170fffa831e98ce9b Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Tue, 25 Apr 2023 13:06:52 -0400
Subject: [PATCH 17/17] Remove unused variable.

---
 synapse/module_api/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 2fbaf415ab11..90eff030b573 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -1260,7 +1260,6 @@ async def send_http_push_notification(
                 if isinstance(p, HttpPusher) and (
                     not device_id or p.device_id == device_id
                 ):
-                    sent = False
                     res = await p.dispatch_push(content, tweaks, default_payload)
                     # Check if the push was successful and no pushers were rejected.
                     sent = res is not False and not res