Seperate the action of removing a local 3pid assoc. vs unbinding from…

… an IS

It's a total footgun that we conflate these together into the same method, plus
there are times when you only want to remove a binding on an IS but not locally
and vice versa. This commit removes the unbinding functionality from
delete_threepid and changes callers to call the identity_handler method for
performing an unbind instead.

We also rename 'delete_and_unbind_threepid' to 'delete_local_threepid'.

synapse/handlers/auth.py

@@ -1595,34 +1595,24 @@ async def add_threepid(
         # has successfully been created.
         await self._third_party_rules.on_threepid_bind(user_id, medium, address)
 
-    async def delete_and_unbind_threepid(
-        self, user_id: str, medium: str, address: str, id_server: Optional[str] = None
-    ) -> bool:
-        """Attempts to unbind the 3pid on the identity servers and deletes it
-        from the local database.
+    async def delete_local_threepid(
+        self, user_id: str, medium: str, address: str
+    ) -> None:
+        """Deletes an association between a third-party ID and a user ID from the local
+        database. This method does not unbind the association from any identity servers.
+
+        If `medium` is 'email' and a pusher is associated with this third-party ID, the
+        pusher will also be deleted.
 
         Args:
             user_id: ID of user to remove the 3pid from.
             medium: The medium of the 3pid being removed: "email" or "msisdn".
             address: The 3pid address to remove.
-            id_server: Use the given identity server when unbinding
-                any threepids. If None then will attempt to unbind using the
-                identity server specified when binding (if known).
-
-        Returns:
-            Returns True if successfully unbound the 3pid on
-            the identity server, False if identity server doesn't support the
-            unbind API.
         """
-
         # 'Canonicalise' email addresses as per above
         if medium == "email":
             address = canonicalise_email(address)
 
-        result = await self.hs.get_identity_handler().try_unbind_threepid(
-            user_id, medium, address, id_server
-        )
-
         # Inform Synapse modules that a 3PID association is about to be deleted.
         await self._third_party_rules.on_remove_user_third_party_identifier(
             user_id, medium, address
@@ -1642,7 +1632,6 @@ async def delete_and_unbind_threepid(
             await self.store.delete_pusher_by_app_id_pushkey_user_id(
                 app_id="m.email", pushkey=address, user_id=user_id
             )
-        return result
 
     async def hash(self, password: str) -> str:
         """Computes a secure hash of password.

synapse/handlers/deactivate_account.py

@@ -105,7 +105,7 @@ async def deactivate_account(
         bound_threepids = await self.store.user_get_bound_threepids(user_id)
         for threepid in bound_threepids:
             try:
-                result = await self._auth_handler.delete_and_unbind_threepid(
+                result = await self._identity_handler.try_unbind_threepid(
                     user_id, threepid["medium"], threepid["address"], id_server
                 )
             except Exception:
@@ -118,7 +118,7 @@ async def deactivate_account(
         # Remove any local threepid associations for this account.
         local_threepids = await self.store.user_get_threepids(user_id)
         for threepid in local_threepids:
-            await self._auth_handler.delete_and_unbind_threepid(
+            await self._auth_handler.delete_local_threepid(
                 user_id, threepid["medium"], threepid["address"], id_server
             )
 

synapse/rest/admin/users.py

@@ -304,13 +304,20 @@ async def on_PUT(
                 # remove old threepids
                 for medium, address in del_threepids:
                     try:
-                        await self.auth_handler.delete_and_unbind_threepid(
-                            user_id, medium, address, None
+                        # Attempt to remove any known bindings of this third-party ID
+                        # and user ID from identity servers.
+                        await self.hs.get_identity_handler().try_unbind_threepid(
+                            user_id, medium, address, id_server=None
                         )
                     except Exception:
                         logger.exception("Failed to remove threepids")
                         raise SynapseError(500, "Failed to remove threepids")
 
+                    # Delete the local association of this user ID and third-party ID.
+                    await self.auth_handler.delete_local_threepid(
+                        user_id, medium, address, None
+                    )
+
                 # add new threepids
                 current_time = self.hs.get_clock().time_msec()
                 for medium, address in add_threepids:

synapse/rest/client/account.py

@@ -768,7 +768,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
         user_id = requester.user.to_string()
 
         try:
-            ret = await self.auth_handler.delete_and_unbind_threepid(
+            # Attempt to remove any known bindings of this third-party ID
+            # and user ID from identity servers.
+            ret = await self.hs.get_identity_handler().try_unbind_threepid(
                 user_id, body.medium, body.address, body.id_server
             )
         except Exception:
@@ -783,6 +785,11 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
         else:
             id_server_unbind_result = "no-support"
 
+        # Delete the local association of this user ID and third-party ID.
+        await self.auth_handler.delete_local_threepid(
+            user_id, body.medium, body.address, body.id_server
+        )
+
         return 200, {"id_server_unbind_result": id_server_unbind_result}
 
 

tests/push/test_email.py

@@ -367,7 +367,7 @@ def test_no_email_sent_after_removed(self) -> None:
 
         # disassociate the user's email address
         self.get_success(
-            self.auth_handler.delete_and_unbind_threepid(
+            self.auth_handler.delete_local_threepid(
                 user_id=self.user_id, medium="email", address="a@example.com"
             )
         )