Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Propagate errors sensibly from proxied IS requests #2147

Merged
merged 12 commits into from
May 3, 2017
10 changes: 5 additions & 5 deletions synapse/handlers/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down Expand Up @@ -83,7 +83,7 @@ def threepid_from_creds(self, creds):

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"
Expand Down Expand Up @@ -118,7 +118,7 @@ def bind_threepid(self, creds, mxid):
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"
),
Expand Down Expand Up @@ -151,7 +151,7 @@ def requestEmailToken(self, id_server, email, client_secret, send_attempt, **kwa
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"
Expand Down Expand Up @@ -185,7 +185,7 @@ def requestMsisdnToken(
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"
Expand Down
30 changes: 30 additions & 0 deletions synapse/http/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ def post_json_get_json(self, uri, post_json):

body = yield preserve_context_over_fn(readBody, response)

if response.code / 100 != 2:
raise CodeMessageException(response.code, body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really want to raise an exception on 300s?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, yeah


defer.returnValue(json.loads(body))

@defer.inlineCallbacks
Expand Down Expand Up @@ -306,6 +309,33 @@ def get_file(self, url, output_stream, max_size=None):
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused - why can't we just pass the Matrix-style error response through to the client?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically we just want to indicate that an error has occurred, and since post_json_get_json just returns the body, previously we were masking the error status code and behaving as if the endpoint returned 2xx which confused things. The main thing is that we raise an exception on error responses, and if we turn it into a SynapseException then it just works without having to handle it specifically in all the different places.

"""
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.

Expand Down
8 changes: 7 additions & 1 deletion synapse/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@
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
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
Expand Down Expand Up @@ -128,6 +130,7 @@ def build_DEPENDENCY(self)
'filtering',
'http_client_context_factory',
'simple_http_client',
'matrix_proxy_client',
'media_repository',
'federation_transport_client',
'federation_sender',
Expand Down Expand Up @@ -190,6 +193,9 @@ def build_http_client_context_factory(self):
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,
Expand Down