From dabbb94fafd335966bbdb5bd2187678872731a0d Mon Sep 17 00:00:00 2001
From: Mathieu Velten <mathieuv@matrix.org>
Date: Fri, 14 Apr 2023 14:12:37 +0200
Subject: [PATCH] Delete pushers after calling on_logged_out module hook on
 device delete (#15410)

---
 changelog.d/15410.bugfix                      |  1 +
 .../password_auth_provider_callbacks.md       |  3 ++
 synapse/handlers/device.py                    |  6 ++-
 tests/module_api/test_api.py                  | 51 ++++++++++++++++++-
 4 files changed, 58 insertions(+), 3 deletions(-)
 create mode 100644 changelog.d/15410.bugfix

diff --git a/changelog.d/15410.bugfix b/changelog.d/15410.bugfix
new file mode 100644
index 0000000000..eb540e33c5
--- /dev/null
+++ b/changelog.d/15410.bugfix
@@ -0,0 +1 @@
+Fix and document untold assumption that `on_logged_out` module hooks will be called before pushers deletion.
diff --git a/docs/modules/password_auth_provider_callbacks.md b/docs/modules/password_auth_provider_callbacks.md
index f6349d5404..8275f7ebdc 100644
--- a/docs/modules/password_auth_provider_callbacks.md
+++ b/docs/modules/password_auth_provider_callbacks.md
@@ -103,6 +103,9 @@ Called during a logout request for a user. It is passed the qualified user ID, t
 deactivated device (if any: access tokens are occasionally created without an associated
 device ID), and the (now deactivated) access token.
 
+Deleting the related pushers is done after calling `on_logged_out`, so you can rely on them
+to still be present.
+
 If multiple modules implement this callback, Synapse runs them all in order.
 
 ### `get_username_for_registration`
diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py
index d2063d4435..ae1d9337ad 100644
--- a/synapse/handlers/device.py
+++ b/synapse/handlers/device.py
@@ -513,8 +513,6 @@ class DeviceHandler(DeviceWorkerHandler):
             else:
                 raise
 
-        await self.hs.get_pusherpool().remove_pushers_by_devices(user_id, device_ids)
-
         # Delete data specific to each device. Not optimised as it is not
         # considered as part of a critical path.
         for device_id in device_ids:
@@ -533,6 +531,10 @@ class DeviceHandler(DeviceWorkerHandler):
                     f"org.matrix.msc3890.local_notification_settings.{device_id}",
                 )
 
+        # Pushers are deleted after `delete_access_tokens_for_user` is called so that
+        # modules using `on_logged_out` hook can use them if needed.
+        await self.hs.get_pusherpool().remove_pushers_by_devices(user_id, device_ids)
+
         await self.notify_device_update(user_id, device_ids)
 
     async def update_device(self, user_id: str, device_id: str, content: dict) -> None:
diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py
index 3a1929691e..758b4bc38b 100644
--- a/tests/module_api/test_api.py
+++ b/tests/module_api/test_api.py
@@ -11,7 +11,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from typing import Any, Dict
+from typing import Any, Dict, Optional
 from unittest.mock import Mock
 
 from twisted.internet import defer
@@ -21,6 +21,7 @@ from synapse.api.constants import EduTypes, EventTypes
 from synapse.api.errors import NotFoundError
 from synapse.events import EventBase
 from synapse.federation.units import Transaction
+from synapse.handlers.device import DeviceHandler
 from synapse.handlers.presence import UserPresenceState
 from synapse.handlers.push_rules import InvalidRuleException
 from synapse.module_api import ModuleApi
@@ -773,6 +774,54 @@ class ModuleApiTestCase(BaseModuleApiTestCase):
         # Check room alias.
         self.assertIsNone(room_alias)
 
+    def test_on_logged_out(self) -> None:
+        """Test that on_logged_out module hook is properly called when logging out
+        a device, and that related pushers are still available at this time.
+        """
+        device_id = "AAAAAAA"
+        user_id = self.register_user("test_on_logged_out", "secret")
+        self.login("test_on_logged_out", "secret", device_id)
+
+        self.get_success(
+            self.hs.get_pusherpool().add_or_update_pusher(
+                user_id=user_id,
+                device_id=device_id,
+                kind="http",
+                app_id="m.http",
+                app_display_name="HTTP Push Notifications",
+                device_display_name="pushy push",
+                pushkey="a@example.com",
+                lang=None,
+                data={"url": "http://example.com/_matrix/push/v1/notify"},
+            )
+        )
+
+        # Setup a callback counting the number of pushers.
+        number_of_pushers_in_callback: Optional[int] = None
+
+        async def _on_logged_out_mock(
+            user_id: str, device_id: Optional[str], access_token: str
+        ) -> None:
+            nonlocal number_of_pushers_in_callback
+            number_of_pushers_in_callback = len(
+                self.hs.get_pusherpool().pushers[user_id].values()
+            )
+
+        self.module_api.register_password_auth_provider_callbacks(
+            on_logged_out=_on_logged_out_mock
+        )
+
+        # Delete the device.
+        device_handler = self.hs.get_device_handler()
+        assert isinstance(device_handler, DeviceHandler)
+        self.get_success(device_handler.delete_devices(user_id, [device_id]))
+
+        # Check that the callback was called and the pushers still existed.
+        self.assertEqual(number_of_pushers_in_callback, 1)
+
+        # Ensure the pushers were deleted after the callback.
+        self.assertEqual(len(self.hs.get_pusherpool().pushers[user_id].values()), 0)
+
 
 class ModuleApiWorkerTestCase(BaseModuleApiTestCase, BaseMultiWorkerStreamTestCase):
     """For testing ModuleApi functionality in a multi-worker setup"""