diff options
| -rw-r--r-- | changelog.d/15410.bugfix | 1 | ||||
| -rw-r--r-- | docs/modules/password_auth_provider_callbacks.md | 3 | ||||
| -rw-r--r-- | synapse/handlers/device.py | 6 | ||||
| -rw-r--r-- | tests/module_api/test_api.py | 51 |
4 files changed, 58 insertions, 3 deletions
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""" |