Fix import order

Co-authored-by: Brendan Abolivier <babolivier@matrix.org>

changelog.d/13227.feature

@@ -0,0 +1 @@
+Add a module callback for unbinding a 3PID.

docs/modules/third_party_rules_callbacks.md

@@ -265,6 +265,33 @@ server_.
 
 If multiple modules implement this callback, Synapse runs them all in order.
 
+### `unbind_threepid`
+
+_First introduced in Synapse v1.74.0_
+
+```python
+async def unbind_threepid(
+    user_id: str, medium: str, address: str, identity_server: str
+) -> Tuple[bool, bool]:
+```
+
+Called before a threepid association is removed.
+
+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
 
 The example below is a module that implements the third-party rules callback

synapse/events/third_party_rules.py

@@ -45,6 +45,7 @@ CHECK_CAN_DEACTIVATE_USER_CALLBACK = Callable[[str, bool], Awaitable[bool]]
 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]
+UNBIND_THREEPID_CALLBACK = Callable[[str, str, str, str], Awaitable[Tuple[bool, bool]]]
 
 
 def load_legacy_third_party_event_rules(hs: "HomeServer") -> None:
@@ -174,6 +175,7 @@ class ThirdPartyEventRules:
             ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
         ] = []
         self._on_threepid_bind_callbacks: List[ON_THREEPID_BIND_CALLBACK] = []
+        self._unbind_threepid_callbacks: List[UNBIND_THREEPID_CALLBACK] = []
 
     def register_third_party_rules_callbacks(
         self,
@@ -193,6 +195,7 @@ class ThirdPartyEventRules:
             ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
         ] = None,
         on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None,
+        unbind_threepid: Optional[UNBIND_THREEPID_CALLBACK] = None,
     ) -> None:
         """Register callbacks from modules for each hook."""
         if check_event_allowed is not None:
@@ -230,6 +233,9 @@ class ThirdPartyEventRules:
         if on_threepid_bind is not None:
             self._on_threepid_bind_callbacks.append(on_threepid_bind)
 
+        if unbind_threepid is not None:
+            self._unbind_threepid_callbacks.append(unbind_threepid)
+
     async def check_event_allowed(
         self, event: EventBase, context: EventContext
     ) -> Tuple[bool, Optional[dict]]:
@@ -523,3 +529,41 @@ class ThirdPartyEventRules:
                 logger.exception(
                     "Failed to run module API callback %s: %s", callback, e
                 )
+
+    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.
+
+        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 = False
+        for callback in self._unbind_threepid_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
+                )
+
+        return global_changed, False

synapse/handlers/identity.py

@@ -275,49 +275,64 @@ class IdentityHandler:
             server doesn't support unbinding
         """
 
-        if not valid_id_server_location(id_server):
+        medium = threepid["medium"]
-            raise SynapseError(
+        address = threepid["address"]
-                400,
-                "id_server must be a valid hostname with optional port and path components",
-            )
 
-        url = "https://%s/_matrix/identity/v2/3pid/unbind" % (id_server,)
+        (changed, stop,) = await self.hs.get_third_party_event_rules().unbind_threepid(
-        url_bytes = b"/_matrix/identity/v2/3pid/unbind"
+            mxid, medium, address, id_server
-
-        content = {
-            "mxid": mxid,
-            "threepid": {"medium": threepid["medium"], "address": threepid["address"]},
-        }
-
-        # 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}
 
-        try:
+        # If a module wants to take over unbind it will return stop = True,
-            # Use the blacklisting http client as this call is only to identity servers
+        # in this case we should just purge the table from the 3pid record
-            # provided by a client
+        if not stop:
-            await self.blacklisting_http_client.post_json_get_json(
+            if not valid_id_server_location(id_server):
-                url, content, headers
+                raise SynapseError(
+                    400,
+                    "id_server must be a valid hostname with optional port and path components",
+                )
+
+            url = "https://%s/_matrix/identity/v2/3pid/unbind" % (id_server,)
+            url_bytes = b"/_matrix/identity/v2/3pid/unbind"
+
+            content = {
+                "mxid": mxid,
+                "threepid": {
+                    "medium": threepid["medium"],
+                    "address": threepid["address"],
+                },
+            }
+
+            # 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
+            headers = {b"Authorization": auth_headers}
-        except HttpResponseException as e:
+
-            changed = False
+            try:
-            if e.code in (400, 404, 501):
+                # Use the blacklisting http client as this call is only to identity servers
-                # The remote server probably doesn't support unbinding (yet)
+                # provided by a client
-                logger.warning("Received %d response while unbinding threepid", e.code)
+                await self.blacklisting_http_client.post_json_get_json(
-            else:
+                    url, content, headers
-                logger.error("Failed to unbind threepid on identity server: %s", e)
+                )
-                raise SynapseError(500, "Failed to contact identity server")
+                changed &= True
-        except RequestTimedOutError:
+            except HttpResponseException as e:
-            raise SynapseError(500, "Timed out contacting identity server")
+                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,

synapse/module_api/__init__.py

@@ -68,6 +68,7 @@ from synapse.events.third_party_rules import (
     ON_PROFILE_UPDATE_CALLBACK,
     ON_THREEPID_BIND_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 (
@@ -319,6 +320,7 @@ class ModuleApi:
             ON_USER_DEACTIVATION_STATUS_CHANGED_CALLBACK
         ] = None,
         on_threepid_bind: Optional[ON_THREEPID_BIND_CALLBACK] = None,
+        unbind_threepid: Optional[UNBIND_THREEPID_CALLBACK] = None,
     ) -> None:
         """Registers callbacks for third party event rules capabilities.
 
@@ -335,6 +337,7 @@ class ModuleApi:
             on_profile_update=on_profile_update,
             on_user_deactivation_status_changed=on_user_deactivation_status_changed,
             on_threepid_bind=on_threepid_bind,
+            unbind_threepid=unbind_threepid,
         )
 
     def register_presence_router_callbacks(

tests/rest/client/test_third_party_rules.py

@@ -15,6 +15,7 @@ import threading
 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 @@ class ThirdPartyRulesTestCase(unittest.FederatingHomeserverTestCase):
 
         # Check that the mock was called with the right parameters
         self.assertEqual(args, (user_id, "email", "foo@example.com"))
+
+    def test_unbind_threepid(self) -> None:
+        """Tests that the unbind_threepid 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._unbind_threepid_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")
+        )