Delete pushers after calling on_logged_out module hook on device delete (#15410)

pull/15435/head
Mathieu Velten 2023-04-14 14:12:37 +02:00 committed by GitHub
parent de4390cd40
commit dabbb94faf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 58 additions and 3 deletions

1
changelog.d/15410.bugfix Normal file
View File

@ -0,0 +1 @@
Fix and document untold assumption that `on_logged_out` module hooks will be called before pushers deletion.

View File

@ -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`

View File

@ -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:

View File

@ -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"""