From 0274a7f2f52dc108d5ae6815fb7dcc2af208dc3c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Fri, 8 Jul 2022 16:46:23 +0200 Subject: [PATCH 1/5] 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 e5afe84df9fb..61b9622697b7 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -276,49 +276,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/v2/3pid/unbind" % (id_server,) - url_bytes = b"/_matrix/identity/v2/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/v2/3pid/unbind" % (id_server,) + url_bytes = b"/_matrix/identity/v2/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 18d6d1058a3d..778608280fa5 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 18a719540978..1083391b41bc 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 @@ -931,3 +932,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") + ) From 97f991ed2ea36d4d46de5c114397fdc06bac8cdc Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 18 Jul 2022 15:35:25 +0200 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Brendan Abolivier --- changelog.d/13227.feature | 2 +- docs/modules/third_party_rules_callbacks.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog.d/13227.feature b/changelog.d/13227.feature index 9be99ac56618..377297cfcd89 100644 --- a/changelog.d/13227.feature +++ b/changelog.d/13227.feature @@ -1 +1 @@ -Add 3pid unbind callback to module API. +Add a module callback for unbinding a 3PID. diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 9394be10ad28..2ca23449e3d1 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -267,7 +267,7 @@ If multiple modules implement this callback, Synapse runs them all in order. ### `on_threepid_unbind` -_First introduced in Synapse v1.63.0_ +_First introduced in Synapse v1.64.0_ ```python async def on_threepid_unbind( From 9b4c0e79d8725cc4fec8b2c780d1bb5b51941a86 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Wed, 3 Aug 2022 16:24:44 +0200 Subject: [PATCH 3/5] Adress comments --- synapse/events/third_party_rules.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index 490acd657a4b..e438f712fd5d 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -554,19 +554,18 @@ async def on_threepid_unbind( was raised at some point. """ - global_changed = True + global_changed = False for callback in self._on_threepid_unbind_callbacks: try: (changed, stop) = await callback( user_id, medium, address, identity_server ) - global_changed &= changed + global_changed |= changed if stop: - return (global_changed, True) + 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) + return global_changed, False From 6073c0ecb1253299058d834d864a6fb9cb862278 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 12 Dec 2022 23:31:09 +0100 Subject: [PATCH 4/5] Adress comments --- docs/modules/third_party_rules_callbacks.md | 25 ++++++++++++++------- synapse/events/third_party_rules.py | 16 ++++++------- synapse/handlers/identity.py | 5 +---- synapse/module_api/__init__.py | 6 ++--- tests/rest/client/test_third_party_rules.py | 6 ++--- 5 files changed, 31 insertions(+), 27 deletions(-) diff --git a/docs/modules/third_party_rules_callbacks.md b/docs/modules/third_party_rules_callbacks.md index 2ca23449e3d1..76428f5e4371 100644 --- a/docs/modules/third_party_rules_callbacks.md +++ b/docs/modules/third_party_rules_callbacks.md @@ -265,23 +265,32 @@ server_. If multiple modules implement this callback, Synapse runs them all in order. -### `on_threepid_unbind` +### `unbind_threepid` -_First introduced in Synapse v1.64.0_ +_First introduced in Synapse v1.74.0_ ```python -async def on_threepid_unbind( +async def unbind_threepid( 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. +The module is given the Matrix ID of the user to which an association is to be removed, +as well as the medium (`email` or `msisdn`), address of the third-party identifier and +the identity server where the threepid was successfully registered. + +A module can hence do its own custom unbinding, if for example it did also registered a custom +binding logic with `on_threepid_bind`. + +It should return a tuple of 2 booleans: +- first one should be `True` on a success calling the identity server, otherwise `False` if +the identity server doesn't support unbinding (or no identity server found to contact). +- second one should be `True` if unbind needs to stop there. 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 diff --git a/synapse/events/third_party_rules.py b/synapse/events/third_party_rules.py index e438f712fd5d..80303116975c 100644 --- a/synapse/events/third_party_rules.py +++ b/synapse/events/third_party_rules.py @@ -45,9 +45,7 @@ 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]] -] +UNBIND_THREEPID_CALLBACK = Callable[[str, str, str, str], Awaitable[Tuple[bool, bool]]] def load_legacy_third_party_event_rules(hs: "HomeServer") -> None: @@ -177,7 +175,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] = [] + self._unbind_threepid_callbacks: List[UNBIND_THREEPID_CALLBACK] = [] def register_third_party_rules_callbacks( self, @@ -197,7 +195,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, + unbind_threepid: Optional[UNBIND_THREEPID_CALLBACK] = None, ) -> None: """Register callbacks from modules for each hook.""" if check_event_allowed is not None: @@ -235,8 +233,8 @@ 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) + if unbind_threepid is not None: + self._unbind_threepid_callbacks.append(unbind_threepid) async def check_event_allowed( self, event: EventBase, context: EventContext @@ -532,7 +530,7 @@ async def on_threepid_bind(self, user_id: str, medium: str, address: str) -> Non "Failed to run module API callback %s: %s", callback, e ) - async def on_threepid_unbind( + async def unbind_threepid( self, user_id: str, medium: str, address: str, identity_server: str ) -> Tuple[bool, bool]: """Called before a threepid association is removed. @@ -555,7 +553,7 @@ async def on_threepid_unbind( """ global_changed = False - for callback in self._on_threepid_unbind_callbacks: + for callback in self._unbind_threepid_callbacks: try: (changed, stop) = await callback( user_id, medium, address, identity_server diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index b98c7b3b40d8..3257b8d0db55 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -278,10 +278,7 @@ async def try_unbind_threepid_with_id_server( medium = threepid["medium"] address = threepid["address"] - ( - changed, - stop, - ) = await self.hs.get_third_party_event_rules().on_threepid_unbind( + (changed, stop,) = await self.hs.get_third_party_event_rules().unbind_threepid( mxid, medium, address, id_server ) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 6da9e1d8b2d2..dc34afcbff85 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -67,7 +67,7 @@ ON_NEW_EVENT_CALLBACK, ON_PROFILE_UPDATE_CALLBACK, ON_THREEPID_BIND_CALLBACK, - ON_THREEPID_UNBIND_CALLBACK, + UNBIND_THREEPID_CALLBACK, ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK, ) from synapse.handlers.account_data import ON_ACCOUNT_DATA_UPDATED_CALLBACK @@ -320,7 +320,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, + unbind_threepid: Optional[UNBIND_THREEPID_CALLBACK] = None, ) -> None: """Registers callbacks for third party event rules capabilities. @@ -337,7 +337,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, + unbind_threepid=unbind_threepid, ) 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 2e1b4753dcec..f6e9b4ea20b8 100644 --- a/tests/rest/client/test_third_party_rules.py +++ b/tests/rest/client/test_third_party_rules.py @@ -933,8 +933,8 @@ 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 + def test_unbind_threepid(self) -> None: + """Tests that the unbind_threepid module callback is called correctly before removing a 3PID mapping. """ @@ -972,7 +972,7 @@ def test_on_threepid_unbind(self) -> None: # 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) + third_party_rules._unbind_threepid_callbacks.append(threepid_unbind_mock) # Deactivate the account, this should remove the 3pid mapping # and call the module handler. From 4dc7b444bd167f217684cc4de68745e1f6a33ef5 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Tue, 13 Dec 2022 10:25:19 +0100 Subject: [PATCH 5/5] Fix import order --- 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 dc34afcbff85..53506f69d37c 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -67,8 +67,8 @@ ON_NEW_EVENT_CALLBACK, ON_PROFILE_UPDATE_CALLBACK, ON_THREEPID_BIND_CALLBACK, - UNBIND_THREEPID_CALLBACK, ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK, + UNBIND_THREEPID_CALLBACK, ) from synapse.handlers.account_data import ON_ACCOUNT_DATA_UPDATED_CALLBACK from synapse.handlers.account_validity import (