From 2c7866d6643f4fd3bbffa9905ede6c36983ef29c Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Wed, 23 May 2018 14:38:56 +0100
Subject: [PATCH 01/12] Hit the 3pid unbind endpoint on deactivation

---
 synapse/handlers/deactivate_account.py | 22 +++++++++++++++-
 synapse/handlers/identity.py           | 35 ++++++++++++++++++++++++++
 synapse/http/matrixfederationclient.py |  9 +++++--
 synapse/storage/registration.py        |  9 -------
 4 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py
index d58ea6c650..0277f80b75 100644
--- a/synapse/handlers/deactivate_account.py
+++ b/synapse/handlers/deactivate_account.py
@@ -17,6 +17,7 @@ from twisted.internet import defer, reactor
 from ._base import BaseHandler
 from synapse.types import UserID, create_requester
 from synapse.util.logcontext import run_in_background
+from synapse.api.errors import SynapseError
 
 import logging
 
@@ -30,6 +31,7 @@ class DeactivateAccountHandler(BaseHandler):
         self._auth_handler = hs.get_auth_handler()
         self._device_handler = hs.get_device_handler()
         self._room_member_handler = hs.get_room_member_handler()
+        self._identity_handler = hs.get_handlers().identity_handler
 
         # Flag that indicates whether the process to part users from rooms is running
         self._user_parter_running = False
@@ -51,6 +53,25 @@ class DeactivateAccountHandler(BaseHandler):
         # FIXME: Theoretically there is a race here wherein user resets
         # password using threepid.
 
+        # delete threepids first. We remove these from the IS so if this fails,
+        # leave the user still active so they can try again.
+        # Ideally we would prevent password resets and then do this in the
+        # background thread.
+        threepids = yield self.store.user_get_threepids(user_id)
+        for threepid in threepids:
+            try:
+                yield self._identity_handler.unbind_threepid(user_id,
+                    {
+                        'medium': threepid['medium'],
+                        'address': threepid['address'],
+                    },
+                )
+            except:
+                # Do we want this to be a fatal error or should we carry on?
+                logger.exception("Failed to remove threepid from ID server")
+                raise SynapseError(400, "Failed to remove threepid from ID server")
+            yield self.store.user_delete_threepid(user_id, threepid['medium'], threepid['address'])
+
         # first delete any devices belonging to the user, which will also
         # delete corresponding access tokens.
         yield self._device_handler.delete_all_devices_for_user(user_id)
@@ -58,7 +79,6 @@ class DeactivateAccountHandler(BaseHandler):
         # a device.
         yield self._auth_handler.delete_access_tokens_for_user(user_id)
 
-        yield self.store.user_delete_threepids(user_id)
         yield self.store.user_set_password_hash(user_id, None)
 
         # Add the user to a table of users pending deactivation (ie.
diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 91a0898860..67a89a1d72 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 # Copyright 2015, 2016 OpenMarket Ltd
 # Copyright 2017 Vector Creations Ltd
+# Copyright 2018 New Vector Ltd
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -38,6 +39,7 @@ class IdentityHandler(BaseHandler):
         super(IdentityHandler, self).__init__(hs)
 
         self.http_client = hs.get_simple_http_client()
+        self.federation_http_client = hs.get_http_client()
 
         self.trusted_id_servers = set(hs.config.trusted_third_party_id_servers)
         self.trust_any_id_server_just_for_testing_do_not_use = (
@@ -138,6 +140,39 @@ class IdentityHandler(BaseHandler):
             data = json.loads(e.msg)
         defer.returnValue(data)
 
+    @defer.inlineCallbacks
+    def unbind_threepid(self, mxid, threepid):
+        yield run_on_reactor()
+        logger.debug("unbinding threepid %r from %s", threepid, mxid)
+        if not self.trusted_id_servers:
+            logger.warn("Can't unbind threepid: no trusted ID servers set in config")
+            defer.returnValue(False)
+        id_server = next(iter(self.trusted_id_servers))
+
+        url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,)
+        content = {
+            "mxid": mxid,
+            "threepid": threepid,
+        }
+        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.
+        self.federation_http_client.sign_request(
+            destination=None,
+            method='POST',
+            url_bytes='/_matrix/identity/api/v1/3pid/unbind'.encode('ascii'),
+            headers_dict=headers,
+            content=content,
+            destination_is=id_server,
+        )
+        yield self.http_client.post_json_get_json(
+            url,
+            content,
+            headers,
+        )
+        defer.returnValue(True)
+
     @defer.inlineCallbacks
     def requestEmailToken(self, id_server, email, client_secret, send_attempt, **kwargs):
         yield run_on_reactor()
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 4b2b85464d..21eaf77dc4 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -262,14 +262,19 @@ class MatrixFederationHttpClient(object):
             defer.returnValue(response)
 
     def sign_request(self, destination, method, url_bytes, headers_dict,
-                     content=None):
+                     content=None, destination_is=None):
         request = {
             "method": method,
             "uri": url_bytes,
             "origin": self.server_name,
-            "destination": destination,
         }
 
+        if destination is not None:
+            request["destination"] = destination
+
+        if destination_is is not None:
+            request["destination_is"] = destination_is
+
         if content is not None:
             request["content"] = content
 
diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py
index a530e29f43..5d8a850ac9 100644
--- a/synapse/storage/registration.py
+++ b/synapse/storage/registration.py
@@ -452,15 +452,6 @@ class RegistrationStore(RegistrationWorkerStore,
             defer.returnValue(ret['user_id'])
         defer.returnValue(None)
 
-    def user_delete_threepids(self, user_id):
-        return self._simple_delete(
-            "user_threepids",
-            keyvalues={
-                "user_id": user_id,
-            },
-            desc="user_delete_threepids",
-        )
-
     def user_delete_threepid(self, user_id, medium, address):
         return self._simple_delete(
             "user_threepids",

From b3bff53178dc8dd9050b84bc953c55835e8410d1 Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Thu, 24 May 2018 11:08:05 +0100
Subject: [PATCH 02/12] Unbind 3pids when they're deleted too

---
 synapse/handlers/auth.py                |  8 ++++++++
 synapse/rest/client/v2_alpha/account.py | 13 ++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index a5365c4fe4..c3f20417c7 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -825,6 +825,14 @@ class AuthHandler(BaseHandler):
         if medium == 'email':
             address = address.lower()
 
+        identity_handler = self.hs.get_handlers().identity_handler
+        identity_handler.unbind_threepid(user_id,
+            {
+                'medium': medium,
+                'address': address,
+            },
+        )
+
         ret = yield self.store.user_delete_threepid(
             user_id, medium, address,
         )
diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py
index 30523995af..4310e78733 100644
--- a/synapse/rest/client/v2_alpha/account.py
+++ b/synapse/rest/client/v2_alpha/account.py
@@ -381,9 +381,16 @@ class ThreepidDeleteRestServlet(RestServlet):
         requester = yield self.auth.get_user_by_req(request)
         user_id = requester.user.to_string()
 
-        yield self.auth_handler.delete_threepid(
-            user_id, body['medium'], body['address']
-        )
+        try:
+            yield self.auth_handler.delete_threepid(
+                user_id, body['medium'], body['address']
+            )
+        except Exception as e:
+            # NB. This endpoint should succeed if there is nothing to
+            # delete, so it should only throw if something is wrong
+            # that we ought to care about.
+            logger.exception("Failed to remove threepid")
+            raise SynapseError(500, "Failed to remove threepid")
 
         defer.returnValue((200, {}))
 

From a21a41bad719cabfbe8ff1e7aea574ff0d0132ba Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Thu, 24 May 2018 11:19:59 +0100
Subject: [PATCH 03/12] comment

---
 synapse/handlers/identity.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 67a89a1d72..6bc3479755 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -147,6 +147,10 @@ class IdentityHandler(BaseHandler):
         if not self.trusted_id_servers:
             logger.warn("Can't unbind threepid: no trusted ID servers set in config")
             defer.returnValue(False)
+
+        # We don't track what ID server we added 3pids on (perhaps we ought to) but we assume
+        # that any of the servers in the trusted list are in the same ID server federation,
+        # so we can pick any one of them to send the deletion request to.
         id_server = next(iter(self.trusted_id_servers))
 
         url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,)

From 9700d15611ec93d1177d29181362fbd02df92629 Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Thu, 24 May 2018 11:23:15 +0100
Subject: [PATCH 04/12] pep8

---
 synapse/handlers/auth.py                | 3 ++-
 synapse/handlers/deactivate_account.py  | 9 ++++++---
 synapse/handlers/identity.py            | 7 ++++---
 synapse/rest/client/v2_alpha/account.py | 2 +-
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index c3f20417c7..512c31185d 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -826,7 +826,8 @@ class AuthHandler(BaseHandler):
             address = address.lower()
 
         identity_handler = self.hs.get_handlers().identity_handler
-        identity_handler.unbind_threepid(user_id,
+        identity_handler.unbind_threepid(
+            user_id,
             {
                 'medium': medium,
                 'address': address,
diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py
index 0277f80b75..f92f953a79 100644
--- a/synapse/handlers/deactivate_account.py
+++ b/synapse/handlers/deactivate_account.py
@@ -60,17 +60,20 @@ class DeactivateAccountHandler(BaseHandler):
         threepids = yield self.store.user_get_threepids(user_id)
         for threepid in threepids:
             try:
-                yield self._identity_handler.unbind_threepid(user_id,
+                yield self._identity_handler.unbind_threepid(
+                    user_id,
                     {
                         'medium': threepid['medium'],
                         'address': threepid['address'],
                     },
                 )
-            except:
+            except Exception:
                 # Do we want this to be a fatal error or should we carry on?
                 logger.exception("Failed to remove threepid from ID server")
                 raise SynapseError(400, "Failed to remove threepid from ID server")
-            yield self.store.user_delete_threepid(user_id, threepid['medium'], threepid['address'])
+            yield self.store.user_delete_threepid(
+                user_id, threepid['medium'], threepid['address'],
+            )
 
         # first delete any devices belonging to the user, which will also
         # delete corresponding access tokens.
diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 6bc3479755..92cd4019d8 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -148,9 +148,10 @@ class IdentityHandler(BaseHandler):
             logger.warn("Can't unbind threepid: no trusted ID servers set in config")
             defer.returnValue(False)
 
-        # We don't track what ID server we added 3pids on (perhaps we ought to) but we assume
-        # that any of the servers in the trusted list are in the same ID server federation,
-        # so we can pick any one of them to send the deletion request to.
+        # We don't track what ID server we added 3pids on (perhaps we ought to)
+        # but we assume that any of the servers in the trusted list are in the
+        # same ID server federation, so we can pick any one of them to send the
+        # deletion request to.
         id_server = next(iter(self.trusted_id_servers))
 
         url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,)
diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py
index 4310e78733..0291fba9e7 100644
--- a/synapse/rest/client/v2_alpha/account.py
+++ b/synapse/rest/client/v2_alpha/account.py
@@ -385,7 +385,7 @@ class ThreepidDeleteRestServlet(RestServlet):
             yield self.auth_handler.delete_threepid(
                 user_id, body['medium'], body['address']
             )
-        except Exception as e:
+        except Exception:
             # NB. This endpoint should succeed if there is nothing to
             # delete, so it should only throw if something is wrong
             # that we ought to care about.

From f731e42baf2ffd186a79cb941017389fda030b0b Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Mon, 4 Jun 2018 12:00:51 +0100
Subject: [PATCH 05/12] docstring

---
 synapse/handlers/identity.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 92cd4019d8..434eb17ef0 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -142,7 +142,15 @@ class IdentityHandler(BaseHandler):
 
     @defer.inlineCallbacks
     def unbind_threepid(self, mxid, threepid):
-        yield run_on_reactor()
+        """
+        Removes a binding from an identity server
+        Args:
+            mxid (str): Matrix user ID of binding to be removed
+            threepid (dict): Dict with medium & address of binding to be removed
+
+        Returns:
+            Deferred
+        """
         logger.debug("unbinding threepid %r from %s", threepid, mxid)
         if not self.trusted_id_servers:
             logger.warn("Can't unbind threepid: no trusted ID servers set in config")

From e44150a6de841dc56a108b7dadaad7ea2c597ae2 Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Mon, 4 Jun 2018 12:01:13 +0100
Subject: [PATCH 06/12] Missing yield

---
 synapse/handlers/auth.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 512c31185d..c058b7cabb 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -826,7 +826,7 @@ class AuthHandler(BaseHandler):
             address = address.lower()
 
         identity_handler = self.hs.get_handlers().identity_handler
-        identity_handler.unbind_threepid(
+        yield identity_handler.unbind_threepid(
             user_id,
             {
                 'medium': medium,

From 6a29e815fc58fec00b6f7001a20f29bc367a55fc Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Mon, 4 Jun 2018 12:01:23 +0100
Subject: [PATCH 07/12] Fix comment

---
 synapse/handlers/deactivate_account.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py
index ce6e2b14fe..8ec5ba2012 100644
--- a/synapse/handlers/deactivate_account.py
+++ b/synapse/handlers/deactivate_account.py
@@ -76,7 +76,7 @@ class DeactivateAccountHandler(BaseHandler):
                 user_id, threepid['medium'], threepid['address'],
             )
 
-        # first delete any devices belonging to the user, which will also
+        # delete any devices belonging to the user, which will also
         # delete corresponding access tokens.
         yield self._device_handler.delete_all_devices_for_user(user_id)
         # then delete any remaining access tokens which weren't associated with

From c5930d513a37f0f143afe49315cf56174e73ce6a Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Mon, 4 Jun 2018 12:05:58 +0100
Subject: [PATCH 08/12] Docstring

---
 synapse/http/matrixfederationclient.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index c7f919498c..1fa9fb3cb2 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -265,6 +265,20 @@ class MatrixFederationHttpClient(object):
 
     def sign_request(self, destination, method, url_bytes, headers_dict,
                      content=None, destination_is=None):
+        """
+        Signs a request by adding an Authorization header to headers_dict
+        Args:
+            destination (str): The desination home server of the request. May be null if the
+                destination is an identity server, in which case destination_is must be non-null.
+            method (str): The HTTP method of the request
+            url_bytes (str): ?
+            headers_dict (dict): Dictionary of request headers to append to
+            content (str): The body of the request
+            destination_is (str): As 'destination', but if the destination is an identity server
+
+        Returns:
+            Deferred
+        """
         request = {
             "method": method,
             "uri": url_bytes,

From d62162bbec27f8d14274ae56c8a6d0bcaa2941fe Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Tue, 5 Jun 2018 18:09:13 +0100
Subject: [PATCH 09/12] doc fixes

---
 synapse/handlers/identity.py           |  2 +-
 synapse/http/matrixfederationclient.py | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 434eb17ef0..529400955d 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -149,7 +149,7 @@ class IdentityHandler(BaseHandler):
             threepid (dict): Dict with medium & address of binding to be removed
 
         Returns:
-            Deferred
+            Deferred[bool]: True on success, otherwise False
         """
         logger.debug("unbinding threepid %r from %s", threepid, mxid)
         if not self.trusted_id_servers:
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 1fa9fb3cb2..6fc0c2296b 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -268,16 +268,16 @@ class MatrixFederationHttpClient(object):
         """
         Signs a request by adding an Authorization header to headers_dict
         Args:
-            destination (str): The desination home server of the request. May be null if the
+            destination (bytes): The desination home server of the request. May be None if the
                 destination is an identity server, in which case destination_is must be non-null.
-            method (str): The HTTP method of the request
-            url_bytes (str): ?
+            method (bytes): The HTTP method of the request
+            url_bytes (bytes): The URI path of the request
             headers_dict (dict): Dictionary of request headers to append to
-            content (str): The body of the request
-            destination_is (str): As 'destination', but if the destination is an identity server
+            content (bytes): The body of the request
+            destination_is (bytes): As 'destination', but if the destination is an identity server
 
         Returns:
-            Deferred
+            None
         """
         request = {
             "method": method,

From 607bd27c83c2fa9236ba88a7167fdb87e6e94f58 Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Tue, 5 Jun 2018 18:10:35 +0100
Subject: [PATCH 10/12] fix pep8

---
 synapse/http/matrixfederationclient.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 6fc0c2296b..98797c37df 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -268,13 +268,15 @@ class MatrixFederationHttpClient(object):
         """
         Signs a request by adding an Authorization header to headers_dict
         Args:
-            destination (bytes): The desination home server of the request. May be None if the
-                destination is an identity server, in which case destination_is must be non-null.
+            destination (bytes): The desination home server of the request. May be None
+                if the destination is an identity server, in which case destination_is
+                must be non-null.
             method (bytes): The HTTP method of the request
             url_bytes (bytes): The URI path of the request
             headers_dict (dict): Dictionary of request headers to append to
             content (bytes): The body of the request
-            destination_is (bytes): As 'destination', but if the destination is an identity server
+            destination_is (bytes): As 'destination', but if the destination is an
+                identity server
 
         Returns:
             None

From 3e4bc4488cf044e048935f8dd3bdf8b460aaa55f Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Wed, 6 Jun 2018 09:44:10 +0100
Subject: [PATCH 11/12] More doc fixes

---
 synapse/http/matrixfederationclient.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 98797c37df..bce7c631b4 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -268,9 +268,9 @@ class MatrixFederationHttpClient(object):
         """
         Signs a request by adding an Authorization header to headers_dict
         Args:
-            destination (bytes): The desination home server of the request. May be None
+            destination (bytes|None): The desination home server of the request. May be None
                 if the destination is an identity server, in which case destination_is
-                must be non-null.
+                must be non-None.
             method (bytes): The HTTP method of the request
             url_bytes (bytes): The URI path of the request
             headers_dict (dict): Dictionary of request headers to append to

From bf54c1cf6ce5097b817b262340e2e1bd4cb13be9 Mon Sep 17 00:00:00 2001
From: David Baker <dave@matrix.org>
Date: Wed, 6 Jun 2018 10:15:33 +0100
Subject: [PATCH 12/12] pep8

---
 synapse/http/matrixfederationclient.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index bce7c631b4..bf56e77c7a 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -268,9 +268,9 @@ class MatrixFederationHttpClient(object):
         """
         Signs a request by adding an Authorization header to headers_dict
         Args:
-            destination (bytes|None): The desination home server of the request. May be None
-                if the destination is an identity server, in which case destination_is
-                must be non-None.
+            destination (bytes|None): The desination home server of the request.
+                May be None if the destination is an identity server, in which case
+                destination_is must be non-None.
             method (bytes): The HTTP method of the request
             url_bytes (bytes): The URI path of the request
             headers_dict (dict): Dictionary of request headers to append to