From 89ef48775c02cf24aa077f0a051ce8ad6b9fb37c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Fri, 8 Jul 2022 16:46:23 +0200 Subject: [PATCH] Add 3pid unbind callback to module API --- changelog.d/13227.feature | 1 + docs/modules/third_party_rules_callbacks.md | 18 ++++ synapse/events/third_party_rules.py | 47 +++++++++++ synapse/handlers/identity.py | 94 ++++++++++++--------- synapse/module_api/__init__.py | 3 + tests/rest/client/test_third_party_rules.py | 60 +++++++++++++ 6 files changed, 185 insertions(+), 38 deletions(-) create mode 100644 changelog.d/13227.feature diff --git a/changelog.d/13227.feature b/changelog.d/13227.feature new file mode 100644 index 000000000000..9be99ac56618 --- /dev/null +++ b/changelog.d/13227.feature @@ -0,0 +1 @@ +Add 3pid unbind callback to module API. diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index e1a5b6524fb4..9394be10ad28 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -265,6 +265,24 @@ server_. If multiple modules implement this callback, Synapse runs them all in order. +### `on_threepid_unbind` + +_First introduced in Synapse v1.63.0_ + +```python +async def on_threepid_unbind( + user_id: str, medium: str, address: str, identity_server: str +) -> Tuple[bool, bool]: +``` + +Called before a threepid association is removed. + +It should return a tuple of 2 booleans reporting if a changed happened for the first, and if unbind +needs to stop there for the second (True value). In this case no other module unbind will be +called, and the default unbind made to the IS that was used on bind will also be skipped. +In any case the mapping will be removed from the Synapse 3pid remote table, except if an Exception +was raised at some point. + ## Example The example below is a module that implements the third-party rules callback diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 72ab69689887..490acd657a4b 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -45,6 +45,9 @@ ON_PROFILE_UPDATE_CALLBACK = Callable[[str, ProfileInfo, bool, bool], Awaitable] ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK = Callable[[str, bool, bool], Awaitable] ON_THREEPID_BIND_CALLBACK = Callable[[str, str, str], Awaitable] +ON_THREEPID_UNBIND_CALLBACK = Callable[ + [str, str, str, str], Awaitable[Tuple[bool, bool]] +] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: @@ -174,6 +177,7 @@ def __init__(self, hs: "HomeServer"): ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK ] = [] self._on_threepid_bind_callbacks: List[ON_THREEPID_BIND_CALLBACK] = [] + self._on_threepid_unbind_callbacks: List[ON_THREEPID_UNBIND_CALLBACK] = [] def register_third_party_rules_callbacks( self, @@ -193,6 +197,7 @@ def register_third_party_rules_callbacks( ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK ] = None, on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None, + on_threepid_unbind: Optional[ON_THREEPID_UNBIND_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" if check_event_allowed is not None: @@ -230,6 +235,9 @@ def register_third_party_rules_callbacks( if on_threepid_bind is not None: self._on_threepid_bind_callbacks.append(on_threepid_bind) + if on_threepid_unbind is not None: + self._on_threepid_unbind_callbacks.append(on_threepid_unbind) + async def check_event_allowed( self, event: EventBase, context: EventContext ) -> Tuple[bool, Optional[dict]]: @@ -523,3 +531,42 @@ async def on_threepid_bind(self, user_id: str, medium: str, address: str) -> Non logger.exception( "Failed to run module API callback %s: %s", callback, e ) + + async def on_threepid_unbind( + self, user_id: str, medium: str, address: str, identity_server: str + ) -> Tuple[bool, bool]: + """Called before a threepid association is removed. + + Note that this callback is called before an association is deleted on the + local homeserver. + + Args: + user_id: the user being associated with the threepid. + medium: the threepid's medium. + address: the threepid's address. + identity_server: the identity server where the threepid was successfully registered. + + Returns: + A tuple of 2 booleans reporting if a changed happened for the first, and if unbind + needs to stop there for the second (True value). In this case no other module unbind will be + called, and the default unbind made to the IS that was used on bind will also be skipped. + In any case the mapping will be removed from the Synapse 3pid remote table, except if an Exception + was raised at some point. + """ + + global_changed = True + for callback in self._on_threepid_unbind_callbacks: + try: + (changed, stop) = await callback( + user_id, medium, address, identity_server + ) + global_changed &= changed + if stop: + return (global_changed, True) + except Exception as e: + logger.exception( + "Failed to run module API callback %s: %s", callback, e + ) + raise e + + return (global_changed, False) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 164d891e9005..ef0710c707e2 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -275,49 +275,67 @@ async def try_unbind_threepid_with_id_server( server doesn't support unbinding """ - if not valid_id_server_location(id_server): - raise SynapseError( - 400, - "id_server must be a valid hostname with optional port and path components", - ) + medium = threepid["medium"] + address = threepid["address"] + + ( + changed, + stop, + ) = await self.hs.get_third_party_event_rules().on_threepid_unbind( + mxid, medium, address, id_server + ) - url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,) - url_bytes = b"/_matrix/identity/api/v1/3pid/unbind" + # If a module wants to take over unbind it will return stop = True, + # in this case we should just purge the table from the 3pid record + if not stop: + if not valid_id_server_location(id_server): + raise SynapseError( + 400, + "id_server must be a valid hostname with optional port and path components", + ) - content = { - "mxid": mxid, - "threepid": {"medium": threepid["medium"], "address": threepid["address"]}, - } + url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,) + url_bytes = b"/_matrix/identity/api/v1/3pid/unbind" - # we abuse the federation http client to sign the request, but we have to send it - # using the normal http client since we don't want the SRV lookup and want normal - # 'browser-like' HTTPS. - auth_headers = self.federation_http_client.build_auth_headers( - destination=None, - method=b"POST", - url_bytes=url_bytes, - content=content, - destination_is=id_server.encode("ascii"), - ) - headers = {b"Authorization": auth_headers} + content = { + "mxid": mxid, + "threepid": { + "medium": threepid["medium"], + "address": threepid["address"], + }, + } - try: - # Use the blacklisting http client as this call is only to identity servers - # provided by a client - await self.blacklisting_http_client.post_json_get_json( - url, content, headers + # we abuse the federation http client to sign the request, but we have to send it + # using the normal http client since we don't want the SRV lookup and want normal + # 'browser-like' HTTPS. + auth_headers = self.federation_http_client.build_auth_headers( + destination=None, + method=b"POST", + url_bytes=url_bytes, + content=content, + destination_is=id_server.encode("ascii"), ) - changed = True - except HttpResponseException as e: - changed = False - if e.code in (400, 404, 501): - # The remote server probably doesn't support unbinding (yet) - logger.warning("Received %d response while unbinding threepid", e.code) - else: - logger.error("Failed to unbind threepid on identity server: %s", e) - raise SynapseError(500, "Failed to contact identity server") - except RequestTimedOutError: - raise SynapseError(500, "Timed out contacting identity server") + headers = {b"Authorization": auth_headers} + + try: + # Use the blacklisting http client as this call is only to identity servers + # provided by a client + await self.blacklisting_http_client.post_json_get_json( + url, content, headers + ) + changed &= True + except HttpResponseException as e: + changed &= False + if e.code in (400, 404, 501): + # The remote server probably doesn't support unbinding (yet) + logger.warning( + "Received %d response while unbinding threepid", e.code + ) + else: + logger.error("Failed to unbind threepid on identity server: %s", e) + raise SynapseError(500, "Failed to contact identity server") + except RequestTimedOutError: + raise SynapseError(500, "Timed out contacting identity server") await self.store.remove_user_bound_threepid( user_id=mxid, diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 6d8bf5408364..60a39dda7540 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -67,6 +67,7 @@ ON_NEW_EVENT_CALLBACK, ON_PROFILE_UPDATE_CALLBACK, ON_THREEPID_BIND_CALLBACK, + ON_THREEPID_UNBIND_CALLBACK, ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK, ) from synapse.handlers.account_data import ON_ACCOUNT_DATA_UPDATED_CALLBACK @@ -317,6 +318,7 @@ def register_third_party_rules_callbacks( ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK ] = None, on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None, + on_threepid_unbind: Optional[ON_THREEPID_UNBIND_CALLBACK] = None, ) -> None: """Registers callbacks for third party event rules capabilities. @@ -333,6 +335,7 @@ def register_third_party_rules_callbacks( on_profile_update=on_profile_update, on_user_deactivation_status_changed=on_user_deactivation_status_changed, on_threepid_bind=on_threepid_bind, + on_threepid_unbind=on_threepid_unbind, ) def register_presence_router_callbacks( diff --git a/tests/rest/client/test_third_party_rules.py b/tests/rest/client/test_third_party_rules.py index 5eb0f243f747..76b94a82194b 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -15,6 +15,7 @@ from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, Union from unittest.mock import Mock +from twisted.internet import defer from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventTypes, LoginType, Membership @@ -937,3 +938,62 @@ def test_on_threepid_bind(self) -> None: # Check that the mock was called with the right parameters self.assertEqual(args, (user_id, "email", "foo@example.com")) + + def test_on_threepid_unbind(self) -> None: + """Tests that the on_threepid_unbind module callback is called correctly before + removing a 3PID mapping. + """ + + # Register an admin user. + self.register_user("admin", "password", admin=True) + admin_tok = self.login("admin", "password") + + # Also register a normal user we can modify. + user_id = self.register_user("user", "password") + + # Add a 3PID to the user. + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % user_id, + { + "threepids": [ + { + "medium": "email", + "address": "foo@example.com", + }, + ], + }, + access_token=admin_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Add the mapping to the remote 3pid assoc table + defer.ensureDeferred( + self.hs.get_module_api().store_remote_3pid_association( + user_id, "email", "foo@example.com", "identityserver.org" + ) + ) + + # Register a mocked callback with stop = True since we don't want to actually + # call identityserver.org + threepid_unbind_mock = Mock(return_value=make_awaitable((True, True))) + third_party_rules = self.hs.get_third_party_event_rules() + third_party_rules._on_threepid_unbind_callbacks.append(threepid_unbind_mock) + + # Deactivate the account, this should remove the 3pid mapping + # and call the module handler. + channel = self.make_request( + "POST", + "/_synapse/admin/v1/deactivate/%s" % user_id, + access_token=admin_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Check that the mock was called once. + threepid_unbind_mock.assert_called_once() + args = threepid_unbind_mock.call_args[0] + + # Check that the mock was called with the right parameters + self.assertEqual( + args, (user_id, "email", "foo@example.com", "identityserver.org") + )