diff --git a/changelog.d/4982.misc b/changelog.d/4982.misc
new file mode 100644
index 0000000000..067c177d39
--- /dev/null
+++ b/changelog.d/4982.misc
@@ -0,0 +1 @@
+Track which identity server is used when binding a threepid and use that for unbinding, as per MSC1915.
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 4544de821d..aa5d89a9ac 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -912,7 +912,7 @@ class AuthHandler(BaseHandler):
)
@defer.inlineCallbacks
- def delete_threepid(self, user_id, medium, address):
+ def delete_threepid(self, user_id, medium, address, id_server=None):
"""Attempts to unbind the 3pid on the identity servers and deletes it
from the local database.
@@ -920,6 +920,10 @@ class AuthHandler(BaseHandler):
user_id (str)
medium (str)
address (str)
+ id_server (str|None): 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:
Deferred[bool]: Returns True if successfully unbound the 3pid on
@@ -937,6 +941,7 @@ class AuthHandler(BaseHandler):
{
'medium': medium,
'address': address,
+ 'id_server': id_server,
},
)
diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py
index 97d3f31d98..6a91f7698e 100644
--- a/synapse/handlers/deactivate_account.py
+++ b/synapse/handlers/deactivate_account.py
@@ -43,12 +43,15 @@ class DeactivateAccountHandler(BaseHandler):
hs.get_reactor().callWhenRunning(self._start_user_parting)
@defer.inlineCallbacks
- def deactivate_account(self, user_id, erase_data):
+ def deactivate_account(self, user_id, erase_data, id_server=None):
"""Deactivate a user's account
Args:
user_id (str): ID of user to be deactivated
erase_data (bool): whether to GDPR-erase the user's data
+ id_server (str|None): 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:
Deferred[bool]: True if identity server supports removing
@@ -74,6 +77,7 @@ class DeactivateAccountHandler(BaseHandler):
{
'medium': threepid['medium'],
'address': threepid['address'],
+ 'id_server': id_server,
},
)
identity_server_supports_unbinding &= result
diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py
index 39184f0e22..22469486d7 100644
--- a/synapse/handlers/identity.py
+++ b/synapse/handlers/identity.py
@@ -132,6 +132,14 @@ class IdentityHandler(BaseHandler):
}
)
logger.debug("bound threepid %r to %s", creds, mxid)
+
+ # Remember where we bound the threepid
+ yield self.store.add_user_bound_threepid(
+ user_id=mxid,
+ medium=data["medium"],
+ address=data["address"],
+ id_server=id_server,
+ )
except CodeMessageException as e:
data = json.loads(e.msg) # XXX WAT?
defer.returnValue(data)
@@ -142,30 +150,61 @@ class IdentityHandler(BaseHandler):
Args:
mxid (str): Matrix user ID of binding to be removed
- threepid (dict): Dict with medium & address of binding to be removed
+ threepid (dict): Dict with medium & address of binding to be
+ removed, and an optional id_server.
Raises:
SynapseError: If we failed to contact the identity server
Returns:
Deferred[bool]: True on success, otherwise False if the identity
- server doesn't support unbinding
+ server doesn't support unbinding (or no identity server found to
+ contact).
"""
- 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")
+ if threepid.get("id_server"):
+ id_servers = [threepid["id_server"]]
+ else:
+ id_servers = yield self.store.get_id_servers_user_bound(
+ user_id=mxid,
+ medium=threepid["medium"],
+ address=threepid["address"],
+ )
+
+ # We don't know where to unbind, so we don't have a choice but to return
+ if not id_servers:
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))
+ changed = True
+ for id_server in id_servers:
+ changed &= yield self.try_unbind_threepid_with_id_server(
+ mxid, threepid, id_server,
+ )
+
+ defer.returnValue(changed)
+
+ @defer.inlineCallbacks
+ def try_unbind_threepid_with_id_server(self, mxid, threepid, id_server):
+ """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
+ id_server (str): Identity server to unbind from
+
+ Raises:
+ SynapseError: If we failed to contact the identity server
+
+ Returns:
+ Deferred[bool]: True on success, otherwise False if the identity
+ server doesn't support unbinding
+ """
url = "https://%s/_matrix/identity/api/v1/3pid/unbind" % (id_server,)
content = {
"mxid": mxid,
- "threepid": threepid,
+ "threepid": {
+ "medium": threepid["medium"],
+ "address": threepid["address"],
+ },
}
# we abuse the federation http client to sign the request, but we have to send it
@@ -188,16 +227,24 @@ class IdentityHandler(BaseHandler):
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.warn("Received %d response while unbinding threepid", e.code)
- defer.returnValue(False)
else:
logger.error("Failed to unbind threepid on identity server: %s", e)
raise SynapseError(502, "Failed to contact identity server")
- defer.returnValue(True)
+ yield self.store.remove_user_bound_threepid(
+ user_id=mxid,
+ medium=threepid["medium"],
+ address=threepid["address"],
+ id_server=id_server,
+ )
+
+ defer.returnValue(changed)
@defer.inlineCallbacks
def requestEmailToken(self, id_server, email, client_secret, send_attempt, **kwargs):
diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py
index 37b32dd37b..ee069179f0 100644
--- a/synapse/rest/client/v2_alpha/account.py
+++ b/synapse/rest/client/v2_alpha/account.py
@@ -215,6 +215,7 @@ class DeactivateAccountRestServlet(RestServlet):
)
result = yield self._deactivate_account_handler.deactivate_account(
requester.user.to_string(), erase,
+ id_server=body.get("id_server"),
)
if result:
id_server_unbind_result = "success"
@@ -363,7 +364,7 @@ class ThreepidRestServlet(RestServlet):
class ThreepidDeleteRestServlet(RestServlet):
- PATTERNS = client_v2_patterns("/account/3pid/delete$", releases=())
+ PATTERNS = client_v2_patterns("/account/3pid/delete$")
def __init__(self, hs):
super(ThreepidDeleteRestServlet, self).__init__()
@@ -380,7 +381,7 @@ class ThreepidDeleteRestServlet(RestServlet):
try:
ret = yield self.auth_handler.delete_threepid(
- user_id, body['medium'], body['address']
+ user_id, body['medium'], body['address'], body.get("id_server"),
)
except Exception:
# NB. This endpoint should succeed if there is nothing to
diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py
index 0a66a2c17d..e30b86c346 100644
--- a/synapse/storage/registration.py
+++ b/synapse/storage/registration.py
@@ -325,6 +325,83 @@ class RegistrationWorkerStore(SQLBaseStore):
desc="user_delete_threepids",
)
+ def add_user_bound_threepid(self, user_id, medium, address, id_server):
+ """The server proxied a bind request to the given identity server on
+ behalf of the given user. We need to remember this in case the user
+ asks us to unbind the threepid.
+
+ Args:
+ user_id (str)
+ medium (str)
+ address (str)
+ id_server (str)
+
+ Returns:
+ Deferred
+ """
+ # We need to use an upsert, in case they user had already bound the
+ # threepid
+ return self._simple_upsert(
+ table="user_threepid_id_server",
+ keyvalues={
+ "user_id": user_id,
+ "medium": medium,
+ "address": address,
+ "id_server": id_server,
+ },
+ values={},
+ insertion_values={},
+ desc="add_user_bound_threepid",
+ )
+
+ def remove_user_bound_threepid(self, user_id, medium, address, id_server):
+ """The server proxied an unbind request to the given identity server on
+ behalf of the given user, so we remove the mapping of threepid to
+ identity server.
+
+ Args:
+ user_id (str)
+ medium (str)
+ address (str)
+ id_server (str)
+
+ Returns:
+ Deferred
+ """
+ return self._simple_delete(
+ table="user_threepid_id_server",
+ keyvalues={
+ "user_id": user_id,
+ "medium": medium,
+ "address": address,
+ "id_server": id_server,
+ },
+ desc="remove_user_bound_threepid",
+ )
+
+ def get_id_servers_user_bound(self, user_id, medium, address):
+ """Get the list of identity servers that the server proxied bind
+ requests to for given user and threepid
+
+ Args:
+ user_id (str)
+ medium (str)
+ address (str)
+
+ Returns:
+ Deferred[list[str]]: Resolves to a list of identity servers
+ """
+ return self._simple_select_onecol(
+ table="user_threepid_id_server",
+ keyvalues={
+ "user_id": user_id,
+ "medium": medium,
+ "address": address,
+ },
+ retcol="id_server",
+ desc="get_id_servers_user_bound",
+ )
+
class RegistrationStore(
RegistrationWorkerStore, background_updates.BackgroundUpdateStore
@@ -353,6 +430,10 @@ class RegistrationStore(
# clear the background update.
self.register_noop_background_update("refresh_tokens_device_index")
+ self.register_background_update_handler(
+ "user_threepids_grandfather", self._bg_user_threepids_grandfather,
+ )
+
@defer.inlineCallbacks
def add_access_token_to_user(self, user_id, token, device_id=None):
"""Adds an access token for the given user.
@@ -707,3 +788,34 @@ class RegistrationStore(
allow_none=True,
desc="get_users_pending_deactivation",
)
+
+ @defer.inlineCallbacks
+ def _bg_user_threepids_grandfather(self, progress, batch_size):
+ """We now track which identity servers a user binds their 3PID to, so
+ we need to handle the case of existing bindings where we didn't track
+ this.
+
+ We do this by grandfathering in existing user threepids assuming that
+ they used one of the server configured trusted identity servers.
+ """
+
+ id_servers = set(self.config.trusted_third_party_id_servers)
+
+ def _bg_user_threepids_grandfather_txn(txn):
+ sql = """
+ INSERT INTO user_threepid_id_server
+ (user_id, medium, address, id_server)
+ SELECT user_id, medium, address, ?
+ FROM user_threepids
+ """
+
+ txn.executemany(sql, [(id_server,) for id_server in id_servers])
+
+ if id_servers:
+ yield self.runInteraction(
+ "_bg_user_threepids_grandfather", _bg_user_threepids_grandfather_txn,
+ )
+
+ yield self._end_background_update("user_threepids_grandfather")
+
+ defer.returnValue(1)
diff --git a/synapse/storage/schema/delta/53/user_threepid_id.sql b/synapse/storage/schema/delta/53/user_threepid_id.sql
new file mode 100644
index 0000000000..80c2c573b6
--- /dev/null
+++ b/synapse/storage/schema/delta/53/user_threepid_id.sql
@@ -0,0 +1,29 @@
+/* Copyright 2019 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.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+-- Tracks which identity server a user bound their threepid via.
+CREATE TABLE user_threepid_id_server (
+ user_id TEXT NOT NULL,
+ medium TEXT NOT NULL,
+ address TEXT NOT NULL,
+ id_server TEXT NOT NULL
+);
+
+CREATE UNIQUE INDEX user_threepid_id_server_idx ON user_threepid_id_server(
+ user_id, medium, address, id_server
+);
+
+INSERT INTO background_updates (update_name, progress_json) VALUES
+ ('user_threepids_grandfather', '{}');
|
