From 1666c0696a8891d39fd308e38e1b9b0042cabe34 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 10:16:13 +0100 Subject: [PATCH 1/9] Track IS used to bind 3PIDs This will then be used to know which IS to default to when unbinding the threepid. --- synapse/handlers/identity.py | 15 ++++ synapse/storage/registration.py | 77 +++++++++++++++++++ .../schema/delta/53/user_threepid_id.sql | 27 +++++++ 3 files changed, 119 insertions(+) create mode 100644 synapse/storage/schema/delta/53/user_threepid_id.sql diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 39184f0e2..cc480301a 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) @@ -188,6 +196,13 @@ class IdentityHandler(BaseHandler): content, headers, ) + + yield self.store.remove_user_bound_threepid( + user_id=mxid, + medium=threepid["medium"], + address=threepid["address"], + id_server=id_server, + ) except HttpResponseException as e: if e.code in (400, 404, 501,): # The remote server probably doesn't support unbinding (yet) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 9b6c28892..7c8c683de 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -328,6 +328,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 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 + """ + 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): 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 000000000..a68b797f9 --- /dev/null +++ b/synapse/storage/schema/delta/53/user_threepid_id.sql @@ -0,0 +1,27 @@ +/* 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 when +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 +); + From 9fbbc3d9e54dbe0a1ce71907b4844ba41cec741b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 10:18:40 +0100 Subject: [PATCH 2/9] For unbind poke IS used during binding of 3PID This changes the behaviour from using the server specified trusted identity server to using the IS that used during the binding of the 3PID, if known. This is the behaviour specified by MSC1915. --- synapse/handlers/identity.py | 44 ++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index cc480301a..765b16d1d 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -159,21 +159,47 @@ class IdentityHandler(BaseHandler): Deferred[bool]: True on success, otherwise False if the identity server doesn't support unbinding """ - 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") + 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 From 057715aaa2b143843692ebe5fd137fed6e5d671c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 10:21:12 +0100 Subject: [PATCH 3/9] Allowing specifying IS to use in unbind API. By default the homeserver will use the identity server used during the binding of the 3PID to unbind the 3PID. However, we need to allow clients to explicitly ask the homeserver to unbind via a particular identity server, for the case where the 3PID was bound out of band from the homeserver. Implements MSC915. --- synapse/handlers/auth.py | 7 ++++++- synapse/handlers/deactivate_account.py | 5 ++++- synapse/handlers/identity.py | 13 ++++++++----- synapse/rest/client/v2_alpha/account.py | 3 ++- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 4544de821..aa5d89a9a 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 97d3f31d9..101879f89 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 diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 765b16d1d..4c127ba12 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -159,11 +159,14 @@ class IdentityHandler(BaseHandler): Deferred[bool]: True on success, otherwise False if the identity server doesn't support unbinding """ - id_servers = yield self.store.get_id_servers_user_bound( - user_id=mxid, - medium=threepid["medium"], - address=threepid["address"], - ) + 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: diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 37b32dd37..50a434a50 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" @@ -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 From 3715c124b3376adaeac53e0dae38f48f6d084f02 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 13:23:18 +0100 Subject: [PATCH 4/9] Grandfather in existing user threepids We assume, as we did before, that users bound their threepid to one of the trusted identity servers. So we simply fill the new table with all threepids in `user_threepids` joined with the trusted identity servers. --- synapse/storage/registration.py | 35 +++++++++++++++++++ .../schema/delta/53/user_threepid_id.sql | 2 ++ 2 files changed, 37 insertions(+) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 7c8c683de..4d64107e1 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -433,6 +433,10 @@ class RegistrationStore(RegistrationWorkerStore, # 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. @@ -815,3 +819,34 @@ class RegistrationStore(RegistrationWorkerStore, 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 index a68b797f9..6c0b7ec0f 100644 --- a/synapse/storage/schema/delta/53/user_threepid_id.sql +++ b/synapse/storage/schema/delta/53/user_threepid_id.sql @@ -25,3 +25,5 @@ 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', '{}'); From 862d6e5ba5de58a6e1a9a5930d267c7cec4e3ab5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 15:21:11 +0100 Subject: [PATCH 5/9] Add unbind API to /r0 as it is now stabalised --- synapse/rest/client/v2_alpha/account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 50a434a50..ee069179f 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -364,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__() From 39fb971e853b6a9ff1ea65926ed8019baef38ac1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 1 Apr 2019 15:31:47 +0100 Subject: [PATCH 6/9] Newsfile --- changelog.d/4982.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4982.misc diff --git a/changelog.d/4982.misc b/changelog.d/4982.misc new file mode 100644 index 000000000..067c177d3 --- /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. From c75e2017f16bf18127f4f70f69100b8343a139c4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Apr 2019 11:15:19 +0100 Subject: [PATCH 7/9] Fixup docstrings --- synapse/handlers/identity.py | 6 ++++-- synapse/storage/registration.py | 6 +++--- synapse/storage/schema/delta/53/user_threepid_id.sql | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 4c127ba12..3eb112bf9 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -150,14 +150,16 @@ 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). """ if threepid.get("id_server"): id_servers = [threepid["id_server"]] diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 4d64107e1..8ada65572 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -358,9 +358,9 @@ class RegistrationWorkerStore(SQLBaseStore): ) def remove_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. + """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) diff --git a/synapse/storage/schema/delta/53/user_threepid_id.sql b/synapse/storage/schema/delta/53/user_threepid_id.sql index 6c0b7ec0f..80c2c573b 100644 --- a/synapse/storage/schema/delta/53/user_threepid_id.sql +++ b/synapse/storage/schema/delta/53/user_threepid_id.sql @@ -13,8 +13,8 @@ * limitations under the License. */ --- Tracks when -CREATE TABlE user_threepid_id_server ( +-- 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, From 24232514bfdd476b037052889d81a6651062446d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Apr 2019 11:16:57 +0100 Subject: [PATCH 8/9] Remove threepid binding if id server returns 400/404/501 --- synapse/handlers/identity.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 3eb112bf9..22469486d 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -227,23 +227,24 @@ class IdentityHandler(BaseHandler): content, headers, ) - - yield self.store.remove_user_bound_threepid( - user_id=mxid, - medium=threepid["medium"], - address=threepid["address"], - id_server=id_server, - ) + 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): From 4ef5d17b96c03355175917e61e743810ace3fc26 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 2 Apr 2019 11:17:51 +0100 Subject: [PATCH 9/9] Correctly handle id_server param --- synapse/handlers/deactivate_account.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 101879f89..6a91f7698 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -77,6 +77,7 @@ class DeactivateAccountHandler(BaseHandler): { 'medium': threepid['medium'], 'address': threepid['address'], + 'id_server': id_server, }, ) identity_server_supports_unbinding &= result