From a90a0f5c8a38ff7f99a12dd436b75680d3fee747 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 21 Apr 2017 11:32:48 +0100 Subject: [PATCH 01/10] Propagate errors sensibly from proxied IS requests When we're proxying Matrix endpoints, parse out Matrix error responses and turn them into SynapseErrors so they can be propagated sensibly upstream. --- synapse/handlers/identity.py | 10 +++++----- synapse/http/client.py | 30 ++++++++++++++++++++++++++++++ synapse/server.py | 8 +++++++- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 6a53c5eb4..41f978d99 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -35,7 +35,7 @@ class IdentityHandler(BaseHandler): def __init__(self, hs): super(IdentityHandler, self).__init__(hs) - self.http_client = hs.get_simple_http_client() + self.proxy_client = hs.get_matrix_proxy_client() self.trusted_id_servers = set(hs.config.trusted_third_party_id_servers) self.trust_any_id_server_just_for_testing_do_not_use = ( @@ -83,7 +83,7 @@ class IdentityHandler(BaseHandler): data = {} try: - data = yield self.http_client.get_json( + data = yield self.proxy_client.get_json( "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/getValidated3pid" @@ -118,7 +118,7 @@ class IdentityHandler(BaseHandler): raise SynapseError(400, "No client_secret in creds") try: - data = yield self.http_client.post_urlencoded_get_json( + data = yield self.proxy_client.post_urlencoded_get_json( "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/bind" ), @@ -151,7 +151,7 @@ class IdentityHandler(BaseHandler): params.update(kwargs) try: - data = yield self.http_client.post_json_get_json( + data = yield self.proxy_client.post_json_get_json( "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/email/requestToken" @@ -185,7 +185,7 @@ class IdentityHandler(BaseHandler): params.update(kwargs) try: - data = yield self.http_client.post_json_get_json( + data = yield self.proxy_client.post_json_get_json( "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/msisdn/requestToken" diff --git a/synapse/http/client.py b/synapse/http/client.py index ca2f770f5..c8b76b219 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -145,6 +145,9 @@ class SimpleHttpClient(object): body = yield preserve_context_over_fn(readBody, response) + if response.code / 100 != 2: + raise CodeMessageException(response.code, body) + defer.returnValue(json.loads(body)) @defer.inlineCallbacks @@ -306,6 +309,33 @@ class SimpleHttpClient(object): defer.returnValue((length, headers, response.request.absoluteURI, response.code)) +class MatrixProxyClient(object): + """ + An HTTP client that proxies other Matrix endpoints, ie. if the remote endpoint + returns Matrix-style error response, this will raise the appropriate SynapseError + """ + def __init__(self, hs): + self.simpleHttpClient = SimpleHttpClient(hs) + + @defer.inlineCallbacks + def post_json_get_json(self, uri, post_json): + try: + result = yield self.simpleHttpClient.post_json_get_json(uri, post_json) + defer.returnValue(result) + except CodeMessageException as cme: + ex = None + try: + errbody = json.loads(cme.msg) + errcode = errbody['errcode'] + errtext = errbody['error'] + ex = SynapseError(cme.code, errtext, errcode) + except: + pass + if ex is not None: + raise ex + raise cme + + # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. # The two should be factored out. diff --git a/synapse/server.py b/synapse/server.py index 631015256..f73aef19c 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -48,7 +48,9 @@ from synapse.handlers.typing import TypingHandler from synapse.handlers.events import EventHandler, EventStreamHandler from synapse.handlers.initial_sync import InitialSyncHandler from synapse.handlers.receipts import ReceiptsHandler -from synapse.http.client import SimpleHttpClient, InsecureInterceptableContextFactory +from synapse.http.client import ( + SimpleHttpClient, InsecureInterceptableContextFactory, MatrixProxyClient +) from synapse.http.matrixfederationclient import MatrixFederationHttpClient from synapse.notifier import Notifier from synapse.push.pusherpool import PusherPool @@ -127,6 +129,7 @@ class HomeServer(object): 'filtering', 'http_client_context_factory', 'simple_http_client', + 'matrix_proxy_client', 'media_repository', 'federation_transport_client', 'federation_sender', @@ -188,6 +191,9 @@ class HomeServer(object): def build_simple_http_client(self): return SimpleHttpClient(self) + def build_matrix_proxy_client(self): + return MatrixProxyClient(self) + def build_v1auth(self): orf = Auth(self) # Matrix spec makes no reference to what HTTP status code is returned, From a1595cec787d4bfa4f4a2ede7ebafe54919c1f06 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 21 Apr 2017 11:51:17 +0100 Subject: [PATCH 02/10] Don't error for 3xx responses --- synapse/http/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index c8b76b219..483f37fe0 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -145,7 +145,7 @@ class SimpleHttpClient(object): body = yield preserve_context_over_fn(readBody, response) - if response.code / 100 != 2: + if response.code / 100 >= 4: raise CodeMessageException(response.code, body) defer.returnValue(json.loads(body)) From 70caf499142a3bc5e5a6e6228471983326d6e147 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 21 Apr 2017 16:09:03 +0100 Subject: [PATCH 03/10] Do the same for get_json --- synapse/http/client.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 483f37fe0..4458855ce 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -323,18 +323,31 @@ class MatrixProxyClient(object): result = yield self.simpleHttpClient.post_json_get_json(uri, post_json) defer.returnValue(result) except CodeMessageException as cme: - ex = None - try: - errbody = json.loads(cme.msg) - errcode = errbody['errcode'] - errtext = errbody['error'] - ex = SynapseError(cme.code, errtext, errcode) - except: - pass + ex = self._tryGetMatrixError(cme.msg) if ex is not None: raise ex raise cme + @defer.inlineCallbacks + def get_json(self, uri, args={}): + try: + result = yield self.simpleHttpClient.get_json(uri, args) + defer.returnValue(result) + except CodeMessageException as cme: + ex = self._tryGetMatrixError(cme.msg) + if ex is not None: + raise ex + raise cme + + def _tryGetMatrixError(self, body): + try: + errbody = json.loads(body) + errcode = errbody['errcode'] + errtext = errbody['error'] + return SynapseError(cme.code, errtext, errcode) + except: + return None + # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. # The two should be factored out. From a46982cee9308294da0fcd99e06a5f1b7a7c439a Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 21 Apr 2017 16:20:12 +0100 Subject: [PATCH 04/10] Need the HTTP status code --- synapse/http/client.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 4458855ce..df8c3e3c2 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -323,7 +323,7 @@ class MatrixProxyClient(object): result = yield self.simpleHttpClient.post_json_get_json(uri, post_json) defer.returnValue(result) except CodeMessageException as cme: - ex = self._tryGetMatrixError(cme.msg) + ex = self._tryGetMatrixError(cme) if ex is not None: raise ex raise cme @@ -334,17 +334,17 @@ class MatrixProxyClient(object): result = yield self.simpleHttpClient.get_json(uri, args) defer.returnValue(result) except CodeMessageException as cme: - ex = self._tryGetMatrixError(cme.msg) + ex = self._tryGetMatrixError(cme) if ex is not None: raise ex raise cme - def _tryGetMatrixError(self, body): + def _tryGetMatrixError(self, codeMessageException): try: - errbody = json.loads(body) + errbody = json.loads(codeMessageException.msg) errcode = errbody['errcode'] errtext = errbody['error'] - return SynapseError(cme.code, errtext, errcode) + return SynapseError(codeMessageException.code, errtext, errcode) except: return None From 1a9255c12eb73245bdbb626a1a0cad2fbe967caa Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 25 Apr 2017 19:30:55 +0100 Subject: [PATCH 05/10] Use CodeMessageException subclass instead Parse json errors from get_json client methods and throw special errors. --- synapse/api/errors.py | 11 +++++++ synapse/handlers/identity.py | 29 ++++++++++------ synapse/http/client.py | 64 +++++++++++------------------------- synapse/server.py | 8 +---- 4 files changed, 51 insertions(+), 61 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 6fbd5d687..d0dfa959d 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -66,6 +66,17 @@ class CodeMessageException(RuntimeError): return cs_error(self.msg) +class MatrixCodeMessageException(CodeMessageException): + """An error from a general matrix endpoint, eg. from a proxied Matrix API call. + + Attributes: + errcode (str): Matrix error code e.g 'M_FORBIDDEN' + """ + def __init__(self, code, msg, errcode=Codes.UNKNOWN): + super(MatrixCodeMessageException, self).__init__(code, msg) + self.errcode = errcode + + class SynapseError(CodeMessageException): """A base exception type for matrix errors which have an errcode and error message (as well as an HTTP status code). diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 41f978d99..8b407a307 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -18,7 +18,7 @@ from twisted.internet import defer from synapse.api.errors import ( - CodeMessageException + MatrixCodeMessageException, CodeMessageException ) from ._base import BaseHandler from synapse.util.async import run_on_reactor @@ -35,7 +35,7 @@ class IdentityHandler(BaseHandler): def __init__(self, hs): super(IdentityHandler, self).__init__(hs) - self.proxy_client = hs.get_matrix_proxy_client() + self.http_client = hs.get_simple_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 = ( @@ -83,13 +83,16 @@ class IdentityHandler(BaseHandler): data = {} try: - data = yield self.proxy_client.get_json( - "https://%s%s" % ( + data = yield self.http_client.get_json( + "http://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/getValidated3pid" ), {'sid': creds['sid'], 'client_secret': client_secret} ) + except MatrixCodeMessageException as e: + logger.info("getValidated3pid failed with Matrix error: %r", e) + raise SynapseError(e.code, e.msg, e.errcode) except CodeMessageException as e: data = json.loads(e.msg) @@ -118,8 +121,8 @@ class IdentityHandler(BaseHandler): raise SynapseError(400, "No client_secret in creds") try: - data = yield self.proxy_client.post_urlencoded_get_json( - "https://%s%s" % ( + data = yield self.http_client.post_urlencoded_get_json( + "http://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/bind" ), { @@ -151,14 +154,17 @@ class IdentityHandler(BaseHandler): params.update(kwargs) try: - data = yield self.proxy_client.post_json_get_json( - "https://%s%s" % ( + data = yield self.http_client.post_json_get_json( + "http://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/email/requestToken" ), params ) defer.returnValue(data) + except MatrixCodeMessageException as e: + logger.info("Proxied requestToken failed with Matrix error: %r", e) + raise SynapseError(e.code, e.msg, e.errcode) except CodeMessageException as e: logger.info("Proxied requestToken failed: %r", e) raise e @@ -185,14 +191,17 @@ class IdentityHandler(BaseHandler): params.update(kwargs) try: - data = yield self.proxy_client.post_json_get_json( - "https://%s%s" % ( + data = yield self.http_client.post_json_get_json( + "http://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/msisdn/requestToken" ), params ) defer.returnValue(data) + except MatrixCodeMessageException as e: + logger.info("Proxied requestToken failed with Matrix error: %r", e) + raise SynapseError(e.code, e.msg, e.errcode) except CodeMessageException as e: logger.info("Proxied requestToken failed: %r", e) raise e diff --git a/synapse/http/client.py b/synapse/http/client.py index df8c3e3c2..57a49b282 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -16,7 +16,7 @@ from OpenSSL import SSL from OpenSSL.SSL import VERIFY_NONE from synapse.api.errors import ( - CodeMessageException, SynapseError, Codes, + CodeMessageException, MatrixCodeMessageException, SynapseError, Codes, ) from synapse.util.logcontext import preserve_context_over_fn import synapse.metrics @@ -145,8 +145,10 @@ class SimpleHttpClient(object): body = yield preserve_context_over_fn(readBody, response) - if response.code / 100 >= 4: - raise CodeMessageException(response.code, body) + if 200 <= response.code < 300: + defer.returnValue(json.loads(body)) + else: + raise self._exceptionFromFailedRequest(response, body) defer.returnValue(json.loads(body)) @@ -168,7 +170,11 @@ class SimpleHttpClient(object): error message. """ body = yield self.get_raw(uri, args) - defer.returnValue(json.loads(body)) + + if 200 <= response.code < 300: + defer.returnValue(json.loads(body)) + else: + raise self._exceptionFromFailedRequest(response, body) @defer.inlineCallbacks def put_json(self, uri, json_body, args={}): @@ -249,6 +255,16 @@ class SimpleHttpClient(object): else: raise CodeMessageException(response.code, body) + def _exceptionFromFailedRequest(self, response, body): + try: + jsonBody = json.loads(body) + errcode = jsonBody['errcode'] + error = jsonBody['error'] + return MatrixCodeMessageException(response.code, error, errcode) + except e: + print e + return CodeMessageException(response.code, body) + # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. # The two should be factored out. @@ -309,46 +325,6 @@ class SimpleHttpClient(object): defer.returnValue((length, headers, response.request.absoluteURI, response.code)) -class MatrixProxyClient(object): - """ - An HTTP client that proxies other Matrix endpoints, ie. if the remote endpoint - returns Matrix-style error response, this will raise the appropriate SynapseError - """ - def __init__(self, hs): - self.simpleHttpClient = SimpleHttpClient(hs) - - @defer.inlineCallbacks - def post_json_get_json(self, uri, post_json): - try: - result = yield self.simpleHttpClient.post_json_get_json(uri, post_json) - defer.returnValue(result) - except CodeMessageException as cme: - ex = self._tryGetMatrixError(cme) - if ex is not None: - raise ex - raise cme - - @defer.inlineCallbacks - def get_json(self, uri, args={}): - try: - result = yield self.simpleHttpClient.get_json(uri, args) - defer.returnValue(result) - except CodeMessageException as cme: - ex = self._tryGetMatrixError(cme) - if ex is not None: - raise ex - raise cme - - def _tryGetMatrixError(self, codeMessageException): - try: - errbody = json.loads(codeMessageException.msg) - errcode = errbody['errcode'] - errtext = errbody['error'] - return SynapseError(codeMessageException.code, errtext, errcode) - except: - return None - - # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. # The two should be factored out. diff --git a/synapse/server.py b/synapse/server.py index 8325d77a7..12754c89a 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -49,9 +49,7 @@ from synapse.handlers.events import EventHandler, EventStreamHandler from synapse.handlers.initial_sync import InitialSyncHandler from synapse.handlers.receipts import ReceiptsHandler from synapse.handlers.read_marker import ReadMarkerHandler -from synapse.http.client import ( - SimpleHttpClient, InsecureInterceptableContextFactory, MatrixProxyClient -) +from synapse.http.client import SimpleHttpClient, InsecureInterceptableContextFactory from synapse.http.matrixfederationclient import MatrixFederationHttpClient from synapse.notifier import Notifier from synapse.push.pusherpool import PusherPool @@ -130,7 +128,6 @@ class HomeServer(object): 'filtering', 'http_client_context_factory', 'simple_http_client', - 'matrix_proxy_client', 'media_repository', 'federation_transport_client', 'federation_sender', @@ -193,9 +190,6 @@ class HomeServer(object): def build_simple_http_client(self): return SimpleHttpClient(self) - def build_matrix_proxy_client(self): - return MatrixProxyClient(self) - def build_v1auth(self): orf = Auth(self) # Matrix spec makes no reference to what HTTP status code is returned, From c3662760566a7cdf48442d399fbffb9163590dc5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 26 Apr 2017 10:07:01 +0100 Subject: [PATCH 06/10] Fix get_json --- synapse/http/client.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 57a49b282..6e0ddfa0f 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -169,12 +169,11 @@ class SimpleHttpClient(object): On a non-2xx HTTP response. The response body will be used as the error message. """ - body = yield self.get_raw(uri, args) - - if 200 <= response.code < 300: + try: + body = yield self.get_raw(uri, args) defer.returnValue(json.loads(body)) - else: - raise self._exceptionFromFailedRequest(response, body) + except CodeMessageException as e: + raise self._exceptionFromFailedRequest(e.code, e.msg) @defer.inlineCallbacks def put_json(self, uri, json_body, args={}): From 82ae0238f9ff15a5a5445cb681ab0cb507d865f3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 26 Apr 2017 11:43:16 +0100 Subject: [PATCH 07/10] Revert accidental commit --- synapse/handlers/identity.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 8b407a307..9efcdff1d 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -84,7 +84,7 @@ class IdentityHandler(BaseHandler): data = {} try: data = yield self.http_client.get_json( - "http://%s%s" % ( + "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/getValidated3pid" ), @@ -122,7 +122,7 @@ class IdentityHandler(BaseHandler): try: data = yield self.http_client.post_urlencoded_get_json( - "http://%s%s" % ( + "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/3pid/bind" ), { @@ -155,7 +155,7 @@ class IdentityHandler(BaseHandler): try: data = yield self.http_client.post_json_get_json( - "http://%s%s" % ( + "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/email/requestToken" ), @@ -192,7 +192,7 @@ class IdentityHandler(BaseHandler): try: data = yield self.http_client.post_json_get_json( - "http://%s%s" % ( + "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/msisdn/requestToken" ), From 5fd12dce013ad46f171dde10245d78a225b12387 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 26 Apr 2017 12:36:26 +0100 Subject: [PATCH 08/10] Remove debugging --- synapse/http/client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 6e0ddfa0f..379ddcb54 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -260,8 +260,7 @@ class SimpleHttpClient(object): errcode = jsonBody['errcode'] error = jsonBody['error'] return MatrixCodeMessageException(response.code, error, errcode) - except e: - print e + except: return CodeMessageException(response.code, body) # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. From c0380402bc0c736a88db35a48ad554cbc2770fa6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 3 May 2017 10:56:22 +0100 Subject: [PATCH 09/10] List caught expection types --- synapse/http/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index 379ddcb54..f8dff37d2 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -260,7 +260,7 @@ class SimpleHttpClient(object): errcode = jsonBody['errcode'] error = jsonBody['error'] return MatrixCodeMessageException(response.code, error, errcode) - except: + except (ValueError, KeyError) as e: return CodeMessageException(response.code, body) # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient. From 482a2ad12239cfcc4886e33a0b3ac5e16196e03a Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 3 May 2017 11:02:59 +0100 Subject: [PATCH 10/10] No need for the exception variable --- synapse/http/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/client.py b/synapse/http/client.py index f8dff37d2..9cf797043 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -260,7 +260,7 @@ class SimpleHttpClient(object): errcode = jsonBody['errcode'] error = jsonBody['error'] return MatrixCodeMessageException(response.code, error, errcode) - except (ValueError, KeyError) as e: + except (ValueError, KeyError): return CodeMessageException(response.code, body) # XXX: FIXME: This is horribly copy-pasted from matrixfederationclient.