From e3e3fbc23aab45c50ca3c605568de10c8e04a518 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Wed, 17 Aug 2016 12:46:49 +0100 Subject: [PATCH 01/18] Initial empty implementation that just registers an API endpoint handler --- synapse/rest/__init__.py | 2 ++ synapse/rest/client/v2_alpha/thirdparty.py | 38 ++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 synapse/rest/client/v2_alpha/thirdparty.py diff --git a/synapse/rest/__init__.py b/synapse/rest/__init__.py index 14227f1cd..2e0e6babe 100644 --- a/synapse/rest/__init__.py +++ b/synapse/rest/__init__.py @@ -47,6 +47,7 @@ from synapse.rest.client.v2_alpha import ( report_event, openid, devices, + thirdparty, ) from synapse.http.server import JsonResource @@ -92,3 +93,4 @@ class ClientRestResource(JsonResource): report_event.register_servlets(hs, client_resource) openid.register_servlets(hs, client_resource) devices.register_servlets(hs, client_resource) + thirdparty.register_servlets(hs, client_resource) diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py new file mode 100644 index 000000000..9be88b2ba --- /dev/null +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -0,0 +1,38 @@ +# -*- coding: utf-8 -*- +# Copyright 2015, 2016 OpenMarket 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. + + +import logging + +from synapse.http.servlet import RestServlet +from ._base import client_v2_patterns + +logger = logging.getLogger(__name__) + + +class ThirdPartyUserServlet(RestServlet): + PATTERNS = client_v2_patterns("/3pu(/(?P[^/]+))?$", + releases=()) + + def __init__(self, hs): + super(ThirdPartyUserServlet, self).__init__() + pass + + def on_GET(self, request, protocol): + return (200, {"TODO":"TODO"}) + + +def register_servlets(hs, http_server): + ThirdPartyUserServlet(hs).register(http_server) From fa87c981e1efabe85a88144479b0dd7131b7da12 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Wed, 17 Aug 2016 13:15:06 +0100 Subject: [PATCH 02/18] =?UTF-8?q?Thread=203PU=20lookup=20through=20as=20fa?= =?UTF-8?q?r=20as=20the=20AS=20API=20object;=20which=20currently=20no?= =?UTF-8?q?=C3=B6ps=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- synapse/appservice/api.py | 3 +++ synapse/handlers/appservice.py | 21 +++++++++++++++++++++ synapse/rest/client/v2_alpha/thirdparty.py | 11 +++++++++-- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 6da6a1b62..6e5f7dc40 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -71,6 +71,9 @@ class ApplicationServiceApi(SimpleHttpClient): logger.warning("query_alias to %s threw exception %s", uri, ex) defer.returnValue(False) + def query_3pu(self, service, protocol, fields): + return False + @defer.inlineCallbacks def push_bulk(self, service, events, txn_id=None): events = self._serialize(events) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 051ccdb38..69fd76661 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -120,6 +120,21 @@ class ApplicationServicesHandler(object): ) defer.returnValue(result) + @defer.inlineCallbacks + def query_3pu(self, protocol, fields): + services = yield self._get_services_for_3pn(protocol) + + # TODO(paul): scattergather + results = [] + for service in services: + result = yield self.appservice_api.query_3pu( + service, protocol, fields + ) + if result: + results.append(result) + + defer.returnValue(results) + @defer.inlineCallbacks def _get_services_for_event(self, event, restrict_to="", alias_list=None): """Retrieve a list of application services interested in this event. @@ -163,6 +178,12 @@ class ApplicationServicesHandler(object): ] defer.returnValue(interested_list) + @defer.inlineCallbacks + def _get_services_for_3pn(self, protocol): + # TODO(paul): Filter by protocol + services = yield self.store.get_app_services() + defer.returnValue(services) + @defer.inlineCallbacks def _is_unknown_user(self, user_id): if not self.is_mine_id(user_id): diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index 9be88b2ba..0180b73e9 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -16,6 +16,8 @@ import logging +from twisted.internet import defer + from synapse.http.servlet import RestServlet from ._base import client_v2_patterns @@ -28,10 +30,15 @@ class ThirdPartyUserServlet(RestServlet): def __init__(self, hs): super(ThirdPartyUserServlet, self).__init__() - pass + self.appservice_handler = hs.get_application_service_handler() + + @defer.inlineCallbacks def on_GET(self, request, protocol): - return (200, {"TODO":"TODO"}) + fields = {} # TODO + results = yield self.appservice_handler.query_3pu(protocol, fields) + + defer.returnValue((200, results)) def register_servlets(hs, http_server): From 3ec10dffd6105e8fc78cb60f08c4636abf9d76e6 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 00:39:09 +0100 Subject: [PATCH 03/18] Actually make 3PU lookup calls out to ASes --- synapse/appservice/api.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 6e5f7dc40..bfc186659 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -71,8 +71,17 @@ class ApplicationServiceApi(SimpleHttpClient): logger.warning("query_alias to %s threw exception %s", uri, ex) defer.returnValue(False) + @defer.inlineCallbacks def query_3pu(self, service, protocol, fields): - return False + uri = service.url + ("/3pu/%s" % urllib.quote(protocol)) + response = None + try: + response = yield self.get_json(uri, fields) + defer.returnValue(response) + except: + # TODO: would be noisy to log lookup failures, but we want to log + # other things. Hrm. + defer.returnValue([]) @defer.inlineCallbacks def push_bulk(self, service, events, txn_id=None): From b3511adb920f81f8847e4cf3112018df08466ad6 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 13:45:18 +0100 Subject: [PATCH 04/18] Don't catch the return-value-as-exception that @defer.inlineCallbacks will use --- synapse/appservice/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index bfc186659..39b4bff55 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -78,7 +78,7 @@ class ApplicationServiceApi(SimpleHttpClient): try: response = yield self.get_json(uri, fields) defer.returnValue(response) - except: + except Exception: # TODO: would be noisy to log lookup failures, but we want to log # other things. Hrm. defer.returnValue([]) From f0c73a1e7a723585d5ca983d6743a64cab92d1f5 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 13:53:54 +0100 Subject: [PATCH 05/18] Extend individual list results into the main return list, don't append --- synapse/handlers/appservice.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 69fd76661..f124590e4 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -131,7 +131,7 @@ class ApplicationServicesHandler(object): service, protocol, fields ) if result: - results.append(result) + results.extend(result) defer.returnValue(results) From 38565827418b71e12b3ff37e1482ff71ffe170d9 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 14:06:02 +0100 Subject: [PATCH 06/18] Ensure that 3PU lookup request fields actually get passed in --- synapse/rest/client/v2_alpha/thirdparty.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index 0180b73e9..4b2a93f1b 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -35,7 +35,11 @@ class ThirdPartyUserServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): - fields = {} # TODO + fields = request.args + del fields["access_token"] + + # TODO(paul): Some type checking on the request args might be nice + # They should probably all be strings results = yield self.appservice_handler.query_3pu(protocol, fields) defer.returnValue((200, results)) From 718ffcf8bbf83975a211a1b840de696c0eabec01 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 14:18:37 +0100 Subject: [PATCH 07/18] Since empty lookups now return 200/empty list not 404, we can safely log failures as exceptions --- synapse/appservice/api.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 39b4bff55..e05570cc8 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -78,9 +78,8 @@ class ApplicationServiceApi(SimpleHttpClient): try: response = yield self.get_json(uri, fields) defer.returnValue(response) - except Exception: - # TODO: would be noisy to log lookup failures, but we want to log - # other things. Hrm. + except Exception as ex: + logger.warning("query_3pu to %s threw exception %s", uri, ex) defer.returnValue([]) @defer.inlineCallbacks From 434bbf2cb5b31f5a8430a06f53549248f7306cfd Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 14:56:02 +0100 Subject: [PATCH 08/18] Filter 3PU lookups by only ASes that declare knowledge of that protocol --- synapse/appservice/__init__.py | 9 ++++++++- synapse/config/appservice.py | 10 ++++++++++ synapse/handlers/appservice.py | 6 ++++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py index b1b91d0a5..bde9b51b2 100644 --- a/synapse/appservice/__init__.py +++ b/synapse/appservice/__init__.py @@ -81,13 +81,17 @@ class ApplicationService(object): NS_LIST = [NS_USERS, NS_ALIASES, NS_ROOMS] def __init__(self, token, url=None, namespaces=None, hs_token=None, - sender=None, id=None): + sender=None, id=None, protocols=None): self.token = token self.url = url self.hs_token = hs_token self.sender = sender self.namespaces = self._check_namespaces(namespaces) self.id = id + if protocols: + self.protocols = set(protocols) + else: + self.protocols = set() def _check_namespaces(self, namespaces): # Sanity check that it is of the form: @@ -219,6 +223,9 @@ class ApplicationService(object): or user_id == self.sender ) + def is_interested_in_protocol(self, protocol): + return protocol in self.protocols + def is_exclusive_alias(self, alias): return self._is_exclusive(ApplicationService.NS_ALIASES, alias) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index eade80390..3184d2084 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -122,6 +122,15 @@ def _load_appservice(hostname, as_info, config_filename): raise ValueError( "Missing/bad type 'exclusive' key in %s", regex_obj ) + # protocols check + protocols = as_info.get("protocols") + if protocols: + # Because strings are lists in python + if isinstance(protocols, str) or not isinstance(protocols, list): + raise KeyError("Optional 'protocols' must be a list if present.") + for p in protocols: + if not isinstance(p, str): + raise KeyError("Bad value for 'protocols' item") return ApplicationService( token=as_info["as_token"], url=as_info["url"], @@ -129,4 +138,5 @@ def _load_appservice(hostname, as_info, config_filename): hs_token=as_info["hs_token"], sender=user_id, id=as_info["id"], + protocols=protocols, ) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 52e897d8d..e0a6c9f19 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -191,9 +191,11 @@ class ApplicationServicesHandler(object): @defer.inlineCallbacks def _get_services_for_3pn(self, protocol): - # TODO(paul): Filter by protocol services = yield self.store.get_app_services() - defer.returnValue(services) + interested_list = [ + s for s in services if s.is_interested_in_protocol(protocol) + ] + defer.returnValue(interested_list) @defer.inlineCallbacks def _is_unknown_user(self, user_id): From 80f4740c8f638e7d07b72d87fcb608435f3f9c15 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 15:40:41 +0100 Subject: [PATCH 09/18] Scattergather the call out to ASes; validate received results --- synapse/handlers/appservice.py | 41 ++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index e0a6c9f19..cd55f6b7f 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -34,6 +34,26 @@ def log_failure(failure): ) ) +def _is_valid_3pu_result(r): + if not isinstance(r, dict): + return False + + for k in ("userid", "protocol"): + if k not in r: + return False + if not isinstance(r[k], str): + return False + + if "fields" not in r: + return False + fields = r["fields"] + if not isinstance(fields, dict): + return False + for k in fields.keys(): + if not isinstance(fields[k], str): + return False + + return True class ApplicationServicesHandler(object): @@ -150,16 +170,23 @@ class ApplicationServicesHandler(object): def query_3pu(self, protocol, fields): services = yield self._get_services_for_3pn(protocol) - # TODO(paul): scattergather - results = [] + deferreds = [] for service in services: - result = yield self.appservice_api.query_3pu( + deferreds.append(self.appservice_api.query_3pu( service, protocol, fields - ) - if result: - results.extend(result) + )) - defer.returnValue(results) + results = yield defer.DeferredList(deferreds, consumeErrors=True) + + ret = [] + for (success, result) in results: + if not success: + continue + if not isinstance(result, list): + continue + ret.extend(r for r in result if _is_valid_3pu_result(r)) + + defer.returnValue(ret) @defer.inlineCallbacks def _get_services_for_event(self, event): From d7b42afc74662afef983bc42ff6e50b2deb91e0e Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 15:49:55 +0100 Subject: [PATCH 10/18] Log a warning if an AS yields an invalid 3PU lookup result --- synapse/handlers/appservice.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index cd55f6b7f..5ed694e71 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -184,7 +184,12 @@ class ApplicationServicesHandler(object): continue if not isinstance(result, list): continue - ret.extend(r for r in result if _is_valid_3pu_result(r)) + for r in result: + if _is_valid_3pu_result(r): + ret.append(r) + else: + logger.warn("Application service returned an " + + "invalid result %r", r) defer.returnValue(ret) From f3afd6ef1a44ef8b87a3f7257a5e42e69c75523e Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 15:53:01 +0100 Subject: [PATCH 11/18] Remove TODO note about request fields being strings - they're always strings --- synapse/rest/client/v2_alpha/thirdparty.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index 4b2a93f1b..bce104c54 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -38,8 +38,6 @@ class ThirdPartyUserServlet(RestServlet): fields = request.args del fields["access_token"] - # TODO(paul): Some type checking on the request args might be nice - # They should probably all be strings results = yield self.appservice_handler.query_3pu(protocol, fields) defer.returnValue((200, results)) From 06964c4a0adabf7d983cbd0d2c6d83eba6fcaf79 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 16:09:50 +0100 Subject: [PATCH 12/18] Copypasta the 3PU support code to also do 3PL --- synapse/appservice/api.py | 11 ++++++++ synapse/handlers/appservice.py | 33 ++++++++++++++++++++-- synapse/rest/client/v2_alpha/thirdparty.py | 20 +++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index e05570cc8..4ccb5c43c 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -82,6 +82,17 @@ class ApplicationServiceApi(SimpleHttpClient): logger.warning("query_3pu to %s threw exception %s", uri, ex) defer.returnValue([]) + @defer.inlineCallbacks + def query_3pl(self, service, protocol, fields): + uri = service.url + ("/3pl/%s" % urllib.quote(protocol)) + response = None + try: + response = yield self.get_json(uri, fields) + defer.returnValue(response) + except Exception as ex: + logger.warning("query_3pl to %s threw exception %s", uri, ex) + defer.returnValue([]) + @defer.inlineCallbacks def push_bulk(self, service, events, txn_id=None): events = self._serialize(events) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 5ed694e71..72c36615d 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -34,11 +34,11 @@ def log_failure(failure): ) ) -def _is_valid_3pu_result(r): +def _is_valid_3pentity_result(r, field): if not isinstance(r, dict): return False - for k in ("userid", "protocol"): + for k in (field, "protocol"): if k not in r: return False if not isinstance(r[k], str): @@ -185,7 +185,34 @@ class ApplicationServicesHandler(object): if not isinstance(result, list): continue for r in result: - if _is_valid_3pu_result(r): + if _is_valid_3pentity_result(r, field="userid"): + ret.append(r) + else: + logger.warn("Application service returned an " + + "invalid result %r", r) + + defer.returnValue(ret) + + @defer.inlineCallbacks + def query_3pl(self, protocol, fields): + services = yield self._get_services_for_3pn(protocol) + + deferreds = [] + for service in services: + deferreds.append(self.appservice_api.query_3pl( + service, protocol, fields + )) + + results = yield defer.DeferredList(deferreds, consumeErrors=True) + + ret = [] + for (success, result) in results: + if not success: + continue + if not isinstance(result, list): + continue + for r in result: + if _is_valid_3pentity_result(r, field="alias"): ret.append(r) else: logger.warn("Application service returned an " + diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index bce104c54..eec08425e 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -43,5 +43,25 @@ class ThirdPartyUserServlet(RestServlet): defer.returnValue((200, results)) +class ThirdPartyLocationServlet(RestServlet): + PATTERNS = client_v2_patterns("/3pl(/(?P[^/]+))?$", + releases=()) + + def __init__(self, hs): + super(ThirdPartyLocationServlet, self).__init__() + + self.appservice_handler = hs.get_application_service_handler() + + @defer.inlineCallbacks + def on_GET(self, request, protocol): + fields = request.args + del fields["access_token"] + + results = yield self.appservice_handler.query_3pl(protocol, fields) + + defer.returnValue((200, results)) + + def register_servlets(hs, http_server): ThirdPartyUserServlet(hs).register(http_server) + ThirdPartyLocationServlet(hs).register(http_server) From 105ff162d4ae3776674cb1cbec6581e1511871d2 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 16:19:23 +0100 Subject: [PATCH 13/18] Authenticate 3PE lookup requests --- synapse/rest/client/v2_alpha/thirdparty.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index eec08425e..d229e4b81 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -31,10 +31,13 @@ class ThirdPartyUserServlet(RestServlet): def __init__(self, hs): super(ThirdPartyUserServlet, self).__init__() + self.auth = hs.get_auth() self.appservice_handler = hs.get_application_service_handler() @defer.inlineCallbacks def on_GET(self, request, protocol): + yield self.auth.get_user_by_req(request) + fields = request.args del fields["access_token"] @@ -50,10 +53,13 @@ class ThirdPartyLocationServlet(RestServlet): def __init__(self, hs): super(ThirdPartyLocationServlet, self).__init__() + self.auth = hs.get_auth() self.appservice_handler = hs.get_application_service_handler() @defer.inlineCallbacks def on_GET(self, request, protocol): + yield self.auth.get_user_by_req(request) + fields = request.args del fields["access_token"] From fcf1dec809e35826b50ed6841730dc0bfeff724a Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 16:26:19 +0100 Subject: [PATCH 14/18] Appease pep8 --- synapse/handlers/appservice.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 72c36615d..a2715e5cf 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -34,6 +34,7 @@ def log_failure(failure): ) ) + def _is_valid_3pentity_result(r, field): if not isinstance(r, dict): return False @@ -55,6 +56,7 @@ def _is_valid_3pentity_result(r, field): return True + class ApplicationServicesHandler(object): def __init__(self, hs): From 2a91799fccd0791083131e8b23ac0b900e42b7f4 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 16:58:25 +0100 Subject: [PATCH 15/18] Minor syntax neatenings --- synapse/appservice/api.py | 4 ++-- synapse/handlers/appservice.py | 22 ++++++++-------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 4ccb5c43c..d4cad1b1e 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -73,7 +73,7 @@ class ApplicationServiceApi(SimpleHttpClient): @defer.inlineCallbacks def query_3pu(self, service, protocol, fields): - uri = service.url + ("/3pu/%s" % urllib.quote(protocol)) + uri = "%s/3pu/%s" % (service.url, urllib.quote(protocol)) response = None try: response = yield self.get_json(uri, fields) @@ -84,7 +84,7 @@ class ApplicationServiceApi(SimpleHttpClient): @defer.inlineCallbacks def query_3pl(self, service, protocol, fields): - uri = service.url + ("/3pl/%s" % urllib.quote(protocol)) + uri = "%s/3pl/%s" % (service.url, urllib.quote(protocol)) response = None try: response = yield self.get_json(uri, fields) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index a2715e5cf..03452f6bb 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -172,13 +172,10 @@ class ApplicationServicesHandler(object): def query_3pu(self, protocol, fields): services = yield self._get_services_for_3pn(protocol) - deferreds = [] - for service in services: - deferreds.append(self.appservice_api.query_3pu( - service, protocol, fields - )) - - results = yield defer.DeferredList(deferreds, consumeErrors=True) + results = yield defer.DeferredList([ + self.appservice_api.query_3pu(service, protocol, fields) + for service in services + ], consumeErrors=True) ret = [] for (success, result) in results: @@ -199,13 +196,10 @@ class ApplicationServicesHandler(object): def query_3pl(self, protocol, fields): services = yield self._get_services_for_3pn(protocol) - deferreds = [] - for service in services: - deferreds.append(self.appservice_api.query_3pl( - service, protocol, fields - )) - - results = yield defer.DeferredList(deferreds, consumeErrors=True) + results = yield defer.DeferredList([ + self.appservice_api.query_3pl(service, protocol, fields) + for service in services + ], consumeErrors=True) ret = [] for (success, result) in results: From b515f844ee07c7d6aa1d7e56faa8b65d282e9341 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 17:19:55 +0100 Subject: [PATCH 16/18] Avoid so much copypasta between 3PU and 3PL query by unifying around a ThirdPartyEntityKind enumeration --- synapse/appservice/api.py | 25 +++++++-------- synapse/handlers/appservice.py | 37 ++++++---------------- synapse/rest/client/v2_alpha/thirdparty.py | 9 ++++-- synapse/types.py | 7 ++++ 4 files changed, 35 insertions(+), 43 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index d4cad1b1e..dd5e762e0 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -17,6 +17,7 @@ from twisted.internet import defer from synapse.api.errors import CodeMessageException from synapse.http.client import SimpleHttpClient from synapse.events.utils import serialize_event +from synapse.types import ThirdPartyEntityKind import logging import urllib @@ -72,25 +73,21 @@ class ApplicationServiceApi(SimpleHttpClient): defer.returnValue(False) @defer.inlineCallbacks - def query_3pu(self, service, protocol, fields): - uri = "%s/3pu/%s" % (service.url, urllib.quote(protocol)) - response = None - try: - response = yield self.get_json(uri, fields) - defer.returnValue(response) - except Exception as ex: - logger.warning("query_3pu to %s threw exception %s", uri, ex) - defer.returnValue([]) + def query_3pe(self, service, kind, protocol, fields): + if kind == ThirdPartyEntityKind.USER: + uri = "%s/3pu/%s" % (service.url, urllib.quote(protocol)) + elif kind == ThirdPartyEntityKind.LOCATION: + uri = "%s/3pl/%s" % (service.url, urllib.quote(protocol)) + else: + raise ValueError( + "Unrecognised 'kind' argument %r to query_3pe()", kind + ) - @defer.inlineCallbacks - def query_3pl(self, service, protocol, fields): - uri = "%s/3pl/%s" % (service.url, urllib.quote(protocol)) - response = None try: response = yield self.get_json(uri, fields) defer.returnValue(response) except Exception as ex: - logger.warning("query_3pl to %s threw exception %s", uri, ex) + logger.warning("query_3pe to %s threw exception %s", uri, ex) defer.returnValue([]) @defer.inlineCallbacks diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 03452f6bb..52c127d2c 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -18,6 +18,7 @@ from twisted.internet import defer from synapse.api.constants import EventTypes from synapse.util.metrics import Measure from synapse.util.logcontext import preserve_fn +from synapse.types import ThirdPartyEntityKind import logging @@ -169,14 +170,20 @@ class ApplicationServicesHandler(object): defer.returnValue(result) @defer.inlineCallbacks - def query_3pu(self, protocol, fields): + def query_3pe(self, kind, protocol, fields): services = yield self._get_services_for_3pn(protocol) results = yield defer.DeferredList([ - self.appservice_api.query_3pu(service, protocol, fields) + self.appservice_api.query_3pe(service, kind, protocol, fields) for service in services ], consumeErrors=True) + required_field = ( + "userid" if kind == ThirdPartyEntityKind.USER else + "alias" if kind == ThirdPartyEntityKind.LOCATION else + None + ) + ret = [] for (success, result) in results: if not success: @@ -184,31 +191,7 @@ class ApplicationServicesHandler(object): if not isinstance(result, list): continue for r in result: - if _is_valid_3pentity_result(r, field="userid"): - ret.append(r) - else: - logger.warn("Application service returned an " + - "invalid result %r", r) - - defer.returnValue(ret) - - @defer.inlineCallbacks - def query_3pl(self, protocol, fields): - services = yield self._get_services_for_3pn(protocol) - - results = yield defer.DeferredList([ - self.appservice_api.query_3pl(service, protocol, fields) - for service in services - ], consumeErrors=True) - - ret = [] - for (success, result) in results: - if not success: - continue - if not isinstance(result, list): - continue - for r in result: - if _is_valid_3pentity_result(r, field="alias"): + if _is_valid_3pentity_result(r, field=required_field): ret.append(r) else: logger.warn("Application service returned an " + diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index d229e4b81..9abca3a8a 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -19,6 +19,7 @@ import logging from twisted.internet import defer from synapse.http.servlet import RestServlet +from synapse.types import ThirdPartyEntityKind from ._base import client_v2_patterns logger = logging.getLogger(__name__) @@ -41,7 +42,9 @@ class ThirdPartyUserServlet(RestServlet): fields = request.args del fields["access_token"] - results = yield self.appservice_handler.query_3pu(protocol, fields) + results = yield self.appservice_handler.query_3pe( + ThirdPartyEntityKind.USER, protocol, fields + ) defer.returnValue((200, results)) @@ -63,7 +66,9 @@ class ThirdPartyLocationServlet(RestServlet): fields = request.args del fields["access_token"] - results = yield self.appservice_handler.query_3pl(protocol, fields) + results = yield self.appservice_handler.query_3pe( + ThirdPartyEntityKind.LOCATION, protocol, fields + ) defer.returnValue((200, results)) diff --git a/synapse/types.py b/synapse/types.py index 5349b0c45..fd17ecbbe 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -269,3 +269,10 @@ class RoomStreamToken(namedtuple("_StreamToken", "topological stream")): return "t%d-%d" % (self.topological, self.stream) else: return "s%d" % (self.stream,) + + +# Some arbitrary constants used for internal API enumerations. Don't rely on +# exact values; always pass or compare symbolically +class ThirdPartyEntityKind(object): + USER = 'user' + LOCATION = 'location' From 697872cf087d983d77e7c2174ad71361f703fb49 Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 17:24:39 +0100 Subject: [PATCH 17/18] More warnings about invalid results from AS 3PE query --- synapse/handlers/appservice.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 52c127d2c..6f162a3c0 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -187,15 +187,20 @@ class ApplicationServicesHandler(object): ret = [] for (success, result) in results: if not success: + logger.warn("Application service failed %r", result) continue if not isinstance(result, list): + logger.warn( + "Application service returned an invalid response %r", result + ) continue for r in result: if _is_valid_3pentity_result(r, field=required_field): ret.append(r) else: - logger.warn("Application service returned an " + - "invalid result %r", r) + logger.warn( + "Application service returned an invalid result %r", r + ) defer.returnValue(ret) From 65201631a407b71087bb52da8b591e0975c463ec Mon Sep 17 00:00:00 2001 From: "Paul \"LeoNerd\" Evans" Date: Thu, 18 Aug 2016 17:33:56 +0100 Subject: [PATCH 18/18] Move validation logic for AS 3PE query response into ApplicationServiceApi class, to keep the handler logic neater --- synapse/appservice/api.py | 43 ++++++++++++++++++++++++++++++- synapse/handlers/appservice.py | 46 ++-------------------------------- 2 files changed, 44 insertions(+), 45 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index dd5e762e0..066127b66 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -25,6 +25,28 @@ import urllib logger = logging.getLogger(__name__) +def _is_valid_3pe_result(r, field): + if not isinstance(r, dict): + return False + + for k in (field, "protocol"): + if k not in r: + return False + if not isinstance(r[k], str): + return False + + if "fields" not in r: + return False + fields = r["fields"] + if not isinstance(fields, dict): + return False + for k in fields.keys(): + if not isinstance(fields[k], str): + return False + + return True + + class ApplicationServiceApi(SimpleHttpClient): """This class manages HS -> AS communications, including querying and pushing. @@ -76,8 +98,10 @@ class ApplicationServiceApi(SimpleHttpClient): def query_3pe(self, service, kind, protocol, fields): if kind == ThirdPartyEntityKind.USER: uri = "%s/3pu/%s" % (service.url, urllib.quote(protocol)) + required_field = "userid" elif kind == ThirdPartyEntityKind.LOCATION: uri = "%s/3pl/%s" % (service.url, urllib.quote(protocol)) + required_field = "alias" else: raise ValueError( "Unrecognised 'kind' argument %r to query_3pe()", kind @@ -85,7 +109,24 @@ class ApplicationServiceApi(SimpleHttpClient): try: response = yield self.get_json(uri, fields) - defer.returnValue(response) + if not isinstance(response, list): + logger.warning( + "query_3pe to %s returned an invalid response %r", + uri, response + ) + defer.returnValue([]) + + ret = [] + for r in response: + if _is_valid_3pe_result(r, field=required_field): + ret.append(r) + else: + logger.warning( + "query_3pe to %s returned an invalid result %r", + uri, r + ) + + defer.returnValue(ret) except Exception as ex: logger.warning("query_3pe to %s threw exception %s", uri, ex) defer.returnValue([]) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 6f162a3c0..18dca462a 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -18,7 +18,6 @@ from twisted.internet import defer from synapse.api.constants import EventTypes from synapse.util.metrics import Measure from synapse.util.logcontext import preserve_fn -from synapse.types import ThirdPartyEntityKind import logging @@ -36,28 +35,6 @@ def log_failure(failure): ) -def _is_valid_3pentity_result(r, field): - if not isinstance(r, dict): - return False - - for k in (field, "protocol"): - if k not in r: - return False - if not isinstance(r[k], str): - return False - - if "fields" not in r: - return False - fields = r["fields"] - if not isinstance(fields, dict): - return False - for k in fields.keys(): - if not isinstance(fields[k], str): - return False - - return True - - class ApplicationServicesHandler(object): def __init__(self, hs): @@ -178,29 +155,10 @@ class ApplicationServicesHandler(object): for service in services ], consumeErrors=True) - required_field = ( - "userid" if kind == ThirdPartyEntityKind.USER else - "alias" if kind == ThirdPartyEntityKind.LOCATION else - None - ) - ret = [] for (success, result) in results: - if not success: - logger.warn("Application service failed %r", result) - continue - if not isinstance(result, list): - logger.warn( - "Application service returned an invalid response %r", result - ) - continue - for r in result: - if _is_valid_3pentity_result(r, field=required_field): - ret.append(r) - else: - logger.warn( - "Application service returned an invalid result %r", r - ) + if success: + ret.extend(result) defer.returnValue(ret)