From 525dd02bbe3700f4cd53db17d54183b03ac16c30 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 16:55:52 +0000 Subject: Remove trailing slashes from outbound federation requests --- synapse/federation/transport/client.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 8e2be218e2..492cd4e64b 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -51,7 +51,7 @@ class TransportLayerClient(object): logger.debug("get_room_state dest=%s, room=%s", destination, room_id) - path = _create_v1_path("/state/%s/", room_id) + path = _create_v1_path("/state/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, ) @@ -73,7 +73,7 @@ class TransportLayerClient(object): logger.debug("get_room_state_ids dest=%s, room=%s", destination, room_id) - path = _create_v1_path("/state_ids/%s/", room_id) + path = _create_v1_path("/state_ids/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, ) @@ -95,7 +95,7 @@ class TransportLayerClient(object): logger.debug("get_pdu dest=%s, event_id=%s", destination, event_id) - path = _create_v1_path("/event/%s/", event_id) + path = _create_v1_path("/event/%s", event_id) return self.client.get_json(destination, path=path, timeout=timeout) @log_function @@ -121,7 +121,7 @@ class TransportLayerClient(object): # TODO: raise? return - path = _create_v1_path("/backfill/%s/", room_id) + path = _create_v1_path("/backfill/%s", room_id) args = { "v": event_tuples, @@ -167,7 +167,7 @@ class TransportLayerClient(object): # generated by the json_data_callback. json_data = transaction.get_dict() - path = _create_v1_path("/send/%s/", transaction.transaction_id) + path = _create_v1_path("/send/%s", transaction.transaction_id) response = yield self.client.put_json( transaction.destination, @@ -959,7 +959,7 @@ def _create_v1_path(path, *args): Example: - _create_v1_path("/event/%s/", event_id) + _create_v1_path("/event/%s", event_id) Args: path (str): String template for the path @@ -980,7 +980,7 @@ def _create_v2_path(path, *args): Example: - _create_v2_path("/event/%s/", event_id) + _create_v2_path("/event/%s", event_id) Args: path (str): String template for the path -- cgit 1.5.1 From 64ff11019e11a712278fdb4fbc0a5f8307f83ddb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 18:22:47 +0000 Subject: Retry certain federation requests on 404 --- synapse/federation/transport/client.py | 10 ++++---- synapse/http/matrixfederationclient.py | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 492cd4e64b..2bd0e0040b 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -52,7 +52,7 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state/%s", room_id) - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args={"event_id": event_id}, ) @@ -74,7 +74,7 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state_ids/%s", room_id) - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args={"event_id": event_id}, ) @@ -96,7 +96,7 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json(destination, path=path, timeout=timeout) + return self.client.get_json_with_trailing_slashes_on_404(destination, path=path, timeout=timeout) @log_function def backfill(self, destination, room_id, event_tuples, limit): @@ -128,7 +128,7 @@ class TransportLayerClient(object): "limit": [str(limit)], } - return self.client.get_json( + return self.client.get_json_with_trailing_slashes_on_404( destination, path=path, args=args, @@ -169,7 +169,7 @@ class TransportLayerClient(object): path = _create_v1_path("/send/%s", transaction.transaction_id) - response = yield self.client.put_json( + response = yield self.client.put_json_with_trailing_slashes_on_404( transaction.destination, path=path, data=json_data, diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1682c9af13..8776639d6a 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -643,6 +643,51 @@ class MatrixFederationHttpClient(object): ) defer.returnValue(body) + @defer.inlineCallbacks + def get_json_with_trailing_slashes_on_404(self, args={}): + """Runs client.get_json under the hood, but if receiving a 404, tries + the request again with a trailing slash. This is a result of removing + trailing slashes from some federation endpoints and in an effort to + remain backwards compatible with older versions of Synapse, we try + again if a server requires a trailing slash. + + Args: + args (dict): A dictionary of arguments matching those provided by put_json. + Returns: + Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The + result will be the decoded JSON body. + """ + response = yield self.get_json(**args) + + # Retry with a trailing slash if we received a 404 + if response.code == 404: + args["path"] += "/" + response = yield self.get_json(**args) + + defer.returnValue(response) + + @defer.inlineCallbacks + def put_json_with_trailing_slashes_on_404(self, args={}): + """Runs client.put_json under the hood, but if receiving a 404, tries + the request again with a trailing slash. + + See get_json_with_trailing_slashes_on_404 for more details. + + Args: + args (dict): A dictionary of arguments matching those provided by put_json. + Returns: + Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The + result will be the decoded JSON body. + """ + response = yield self.put_json(**args) + + # Retry with a trailing slash if we received a 404 + if response.code == 404: + args["path"] += "/" + response = yield self.put_json(**args) + + defer.returnValue(response) + @defer.inlineCallbacks def delete_json(self, destination, path, long_retries=False, timeout=None, ignore_backoff=False, args={}): -- cgit 1.5.1 From a5dd335cd867f71fe3833fd8edc4cb284faa2415 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 8 Mar 2019 18:25:59 +0000 Subject: lint --- synapse/federation/transport/client.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 2bd0e0040b..68808e9203 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -96,7 +96,9 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json_with_trailing_slashes_on_404(destination, path=path, timeout=timeout) + return self.client.get_json_with_trailing_slashes_on_404( + destination, path=path, timeout=timeout, + ) @log_function def backfill(self, destination, room_id, event_tuples, limit): -- cgit 1.5.1 From 0ea8582f8bd83fc9f4cba80c0462f44583e059a3 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 12 Mar 2019 14:11:11 +0000 Subject: Cleaner way of implementing trailing slashes --- synapse/federation/transport/client.py | 15 +++-- synapse/http/matrixfederationclient.py | 115 +++++++++++++++------------------ tests/handlers/test_typing.py | 6 +- 3 files changed, 66 insertions(+), 70 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 68808e9203..37f37b3d41 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -52,8 +52,9 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state/%s", room_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args={"event_id": event_id}, + try_trailing_slash_on_404=True, ) @log_function @@ -74,8 +75,9 @@ class TransportLayerClient(object): destination, room_id) path = _create_v1_path("/state_ids/%s", room_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args={"event_id": event_id}, + try_trailing_slash_on_404=True, ) @log_function @@ -96,8 +98,9 @@ class TransportLayerClient(object): destination, event_id) path = _create_v1_path("/event/%s", event_id) - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, timeout=timeout, + try_trailing_slash_on_404=True, ) @log_function @@ -130,10 +133,11 @@ class TransportLayerClient(object): "limit": [str(limit)], } - return self.client.get_json_with_trailing_slashes_on_404( + return self.client.get_json( destination, path=path, args=args, + try_trailing_slash_on_404=True, ) @defer.inlineCallbacks @@ -171,13 +175,14 @@ class TransportLayerClient(object): path = _create_v1_path("/send/%s", transaction.transaction_id) - response = yield self.client.put_json_with_trailing_slashes_on_404( + response = yield self.client.put_json( transaction.destination, path=path, data=json_data, json_data_callback=json_data_callback, long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone + try_trailing_slash_on_404=True, ) defer.returnValue(response) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8776639d6a..fec7f5f882 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -196,7 +196,8 @@ class MatrixFederationHttpClient(object): timeout=None, long_retries=False, ignore_backoff=False, - backoff_on_404=False + backoff_on_404=False, + try_trailing_slash_on_404=False, ): """ Sends a request to the given server. @@ -212,6 +213,11 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): Back off if we get a 404 + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. This will be attempted before backing off if backing + off has been enabled. + Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. @@ -473,7 +479,8 @@ class MatrixFederationHttpClient(object): json_data_callback=None, long_retries=False, timeout=None, ignore_backoff=False, - backoff_on_404=False): + backoff_on_404=False, + try_trailing_slash_on_404=False): """ Sends the specifed json data using PUT Args: @@ -493,7 +500,11 @@ class MatrixFederationHttpClient(object): and try the request anyway. backoff_on_404 (bool): True if we should count a 404 response as a failure of the server (and should therefore back off future - requests) + requests). + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. This will be attempted before backing off if backing + off has been enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -509,7 +520,6 @@ class MatrixFederationHttpClient(object): RequestSendFailed: If there were problems connecting to the remote, due to e.g. DNS failures, connection timeouts etc. """ - request = MatrixFederationRequest( method="PUT", destination=destination, @@ -519,13 +529,26 @@ class MatrixFederationHttpClient(object): json=data, ) - response = yield self._send_request( - request, - long_retries=long_retries, - timeout=timeout, - ignore_backoff=ignore_backoff, - backoff_on_404=backoff_on_404, - ) + send_request_args = { + "request": request, + "long_retries": long_retries, + "timeout": timeout, + "ignore_backoff": ignore_backoff, + # Do not backoff on the initial request if we're trying with trailing slashes + # Otherwise we may end up waiting to contact a server that is actually up + "backoff_on_404": False if try_trailing_slash_on_404 else backoff_on_404, + } + + response = yield self._send_request(**send_request_args) + + # If enabled, retry with a trailing slash if we received a 404 + if try_trailing_slash_on_404 and response.code == 404: + args["path"] += "/" + + # Re-enable backoff if enabled + send_request_args["backoff_on_404"] = backoff_on_404 + + response = yield self.get_json(**send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -592,7 +615,8 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def get_json(self, destination, path, args=None, retry_on_dns_fail=True, - timeout=None, ignore_backoff=False): + timeout=None, ignore_backoff=False, + try_trailing_slash_on_404=False): """ GETs some json from the given host homeserver and path Args: @@ -606,6 +630,9 @@ class MatrixFederationHttpClient(object): be retried. ignore_backoff (bool): true to ignore the historical backoff data and try the request anyway. + try_trailing_slash_on_404 (bool): True if on a 404 response we + should try appending a trailing slash to the end of the + request. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. @@ -631,63 +658,25 @@ class MatrixFederationHttpClient(object): query=args, ) - response = yield self._send_request( - request, - retry_on_dns_fail=retry_on_dns_fail, - timeout=timeout, - ignore_backoff=ignore_backoff, - ) + send_request_args = { + "request": request, + "retry_on_dns_fail": retry_on_dns_fail, + "timeout": timeout, + "ignore_backoff": ignore_backoff, + } + + response = yield self._send_request(**send_request_args) + + # If enabled, retry with a trailing slash if we received a 404 + if try_trailing_slash_on_404 and response.code == 404: + args["path"] += "/" + response = yield self._send_request(**send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) defer.returnValue(body) - @defer.inlineCallbacks - def get_json_with_trailing_slashes_on_404(self, args={}): - """Runs client.get_json under the hood, but if receiving a 404, tries - the request again with a trailing slash. This is a result of removing - trailing slashes from some federation endpoints and in an effort to - remain backwards compatible with older versions of Synapse, we try - again if a server requires a trailing slash. - - Args: - args (dict): A dictionary of arguments matching those provided by put_json. - Returns: - Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The - result will be the decoded JSON body. - """ - response = yield self.get_json(**args) - - # Retry with a trailing slash if we received a 404 - if response.code == 404: - args["path"] += "/" - response = yield self.get_json(**args) - - defer.returnValue(response) - - @defer.inlineCallbacks - def put_json_with_trailing_slashes_on_404(self, args={}): - """Runs client.put_json under the hood, but if receiving a 404, tries - the request again with a trailing slash. - - See get_json_with_trailing_slashes_on_404 for more details. - - Args: - args (dict): A dictionary of arguments matching those provided by put_json. - Returns: - Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The - result will be the decoded JSON body. - """ - response = yield self.put_json(**args) - - # Retry with a trailing slash if we received a 404 - if response.code == 404: - args["path"] += "/" - response = yield self.put_json(**args) - - defer.returnValue(response) - @defer.inlineCallbacks def delete_json(self, destination, path, long_retries=False, timeout=None, ignore_backoff=False, args={}): diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 5a3670c939..e17abf2fb4 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -177,7 +177,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): timeout=20000, )) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 + put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", @@ -192,6 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, + trailing_slashes_on_404=True, ) def test_started_typing_remote_recv(self): @@ -254,7 +255,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): [call('typing_key', 1, rooms=[ROOM_ID])] ) - put_json = self.hs.get_http_client().put_json_with_trailing_slashes_on_404 + put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", path="/_matrix/federation/v1/send/1000000", @@ -269,6 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, + trailing_slashes_on_404=True, ) self.assertEquals(self.event_source.get_current_key(), 1) -- cgit 1.5.1 From cf301e37d873fcc00a9fb2d5efffd8ff7ad1b372 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 11:14:43 +0000 Subject: Add workaround note --- synapse/http/matrixfederationclient.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fec7f5f882..d22051a47d 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -197,7 +197,6 @@ class MatrixFederationHttpClient(object): long_retries=False, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False, ): """ Sends a request to the given server. @@ -213,11 +212,6 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): Back off if we get a 404 - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. This will be attempted before backing off if backing - off has been enabled. - Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. @@ -502,9 +496,9 @@ class MatrixFederationHttpClient(object): a failure of the server (and should therefore back off future requests). try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. This will be attempted before backing off if backing - off has been enabled. + should try appending a trailing slash to the end of the request. + Workaround for #3622 in Synapse <0.99.2. This will be attempted + before backing off if backing off has been enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -632,7 +626,7 @@ class MatrixFederationHttpClient(object): and try the request anyway. try_trailing_slash_on_404 (bool): True if on a 404 response we should try appending a trailing slash to the end of the - request. + request. Workaround for #3622 in Synapse <0.99.2. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. -- cgit 1.5.1 From 7e75d9644bfb5d687c3b789ad2279da58e09ed7b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 11:15:23 +0000 Subject: Fix paranthesis indent --- synapse/http/matrixfederationclient.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index d22051a47d..7efa7b7572 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -474,7 +474,8 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_404=False, + ): """ Sends the specifed json data using PUT Args: -- cgit 1.5.1 From 7d053cfe10c7be4327b774c6552d21594dcdd2be Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 12:10:33 +0000 Subject: Retry on 400:M_UNRECOGNIZED --- synapse/http/matrixfederationclient.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 7efa7b7572..fc8cf92067 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -474,8 +474,7 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False, - ): + try_trailing_slash_on_404=False): """ Sends the specifed json data using PUT Args: @@ -662,14 +661,19 @@ class MatrixFederationHttpClient(object): response = yield self._send_request(**send_request_args) + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) + # If enabled, retry with a trailing slash if we received a 404 - if try_trailing_slash_on_404 and response.code == 404: + # or if a 400 with "M_UNRECOGNIZED" which some endpoints return + if (try_trailing_slash_on_404 and + (response.code == 404 + or (response.code == 400 + and body.get("errcode") == "M_UNRECOGNIZED"))): args["path"] += "/" response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) defer.returnValue(body) @defer.inlineCallbacks -- cgit 1.5.1 From 09626bfd39615e25c735ae5d17ad650aca5fac84 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:26:06 +0000 Subject: Switch to wrapper function around _send_request --- synapse/federation/transport/client.py | 10 ++-- synapse/http/matrixfederationclient.py | 103 +++++++++++++++++++++++---------- tests/handlers/test_typing.py | 4 +- 3 files changed, 78 insertions(+), 39 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 37f37b3d41..e424c40fdf 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -54,7 +54,7 @@ class TransportLayerClient(object): path = _create_v1_path("/state/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -77,7 +77,7 @@ class TransportLayerClient(object): path = _create_v1_path("/state_ids/%s", room_id) return self.client.get_json( destination, path=path, args={"event_id": event_id}, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -100,7 +100,7 @@ class TransportLayerClient(object): path = _create_v1_path("/event/%s", event_id) return self.client.get_json( destination, path=path, timeout=timeout, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @log_function @@ -137,7 +137,7 @@ class TransportLayerClient(object): destination, path=path, args=args, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) @defer.inlineCallbacks @@ -182,7 +182,7 @@ class TransportLayerClient(object): json_data_callback=json_data_callback, long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) defer.returnValue(response) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fc8cf92067..9019c8791a 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -188,6 +188,55 @@ class MatrixFederationHttpClient(object): self._cooperator = Cooperator(scheduler=schedule) + @defer.inlineCallbacks + def _send_request_with_optional_trailing_slash( + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs, + ): + """Wrapper for _send_request which can optionally retry the request + upon receiving a combination of a 400 HTTP response code and a + 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 + due to #3622. + + Args: + request + try_trailing_slash_on_400 (bool): Whether on receiving a 400 + 'M_UNRECOGNIZED' from the server to retry the request with a + trailing slash appended to the request path. + backoff_on_404 (bool): Whether to backoff on 404 when making a + request with a trailing slash (only affects request if + try_trailing_slash_on_400 is True). + kwargs (Dict): A dictionary of arguments to pass to + `_send_request()`. + + Returns: + Deferred[twisted.web.client.Response]: resolves with the HTTP + response object on success. + """ + response = self._send_request(**kwargs) + + if not try_trailing_slash_on_400: + defer.returnValue(response) + + # Check if it's necessary to retry with a trailing slash + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) + + # Retry with a trailing slash if we received a 400 with + # 'M_UNRECOGNIZED' which some endpoints can return when omitting a + # trailing slash on Synapse <=v0.99.2. + if (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): + # Enable backoff if initially disabled + kwargs["backoff_on_404"] = backoff_on_404 + + kwargs["path"] += "/" + response = yield self._send_request(**kwargs) + + defer.returnValue(response) + @defer.inlineCallbacks def _send_request( self, @@ -474,7 +523,7 @@ class MatrixFederationHttpClient(object): long_retries=False, timeout=None, ignore_backoff=False, backoff_on_404=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_400=False): """ Sends the specifed json data using PUT Args: @@ -495,10 +544,11 @@ class MatrixFederationHttpClient(object): backoff_on_404 (bool): True if we should count a 404 response as a failure of the server (and should therefore back off future requests). - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the request. - Workaround for #3622 in Synapse <0.99.2. This will be attempted - before backing off if backing off has been enabled. + try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED + response we should try appending a trailing slash to the end + of the request. Workaround for #3622 in Synapse <=v0.99.2. This + will be attempted before backing off if backing off has been + enabled. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The @@ -528,21 +578,21 @@ class MatrixFederationHttpClient(object): "long_retries": long_retries, "timeout": timeout, "ignore_backoff": ignore_backoff, - # Do not backoff on the initial request if we're trying with trailing slashes - # Otherwise we may end up waiting to contact a server that is actually up - "backoff_on_404": False if try_trailing_slash_on_404 else backoff_on_404, + # Do not backoff on the initial request if we're trying again with + # trailing slashes Otherwise we may end up waiting to contact a + # server that is actually up + "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404, } - response = yield self._send_request(**send_request_args) + response = yield self._send_request_with_optional_trailing_slash( + request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, + ) - # If enabled, retry with a trailing slash if we received a 404 - if try_trailing_slash_on_404 and response.code == 404: + # If enabled, retry with a trailing slash if we received a 400 + if try_trailing_slash_on_400 and response.code == 400: args["path"] += "/" - # Re-enable backoff if enabled - send_request_args["backoff_on_404"] = backoff_on_404 - - response = yield self.get_json(**send_request_args) + response = yield self._send_request(**send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -610,7 +660,7 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def get_json(self, destination, path, args=None, retry_on_dns_fail=True, timeout=None, ignore_backoff=False, - try_trailing_slash_on_404=False): + try_trailing_slash_on_400=False): """ GETs some json from the given host homeserver and path Args: @@ -624,9 +674,9 @@ class MatrixFederationHttpClient(object): be retried. ignore_backoff (bool): true to ignore the historical backoff data and try the request anyway. - try_trailing_slash_on_404 (bool): True if on a 404 response we - should try appending a trailing slash to the end of the - request. Workaround for #3622 in Synapse <0.99.2. + try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED + response we should try appending a trailing slash to the end of + the request. Workaround for #3622 in Synapse <=v0.99.2. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. @@ -659,21 +709,10 @@ class MatrixFederationHttpClient(object): "ignore_backoff": ignore_backoff, } - response = yield self._send_request(**send_request_args) - - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, + response = yield self._send_request_with_optional_trailing_slash( + request, try_trailing_slash_on_400, False, **send_request_args, ) - # If enabled, retry with a trailing slash if we received a 404 - # or if a 400 with "M_UNRECOGNIZED" which some endpoints return - if (try_trailing_slash_on_404 and - (response.code == 404 - or (response.code == 400 - and body.get("errcode") == "M_UNRECOGNIZED"))): - args["path"] += "/" - response = yield self._send_request(**send_request_args) - defer.returnValue(body) @defer.inlineCallbacks diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 244a0bc80c..6460cbc708 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -192,7 +192,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) def test_started_typing_remote_recv(self): @@ -270,7 +270,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): json_data_callback=ANY, long_retries=True, backoff_on_404=True, - try_trailing_slash_on_404=True, + try_trailing_slash_on_400=True, ) self.assertEquals(self.event_source.get_current_key(), 1) -- cgit 1.5.1 From 5526b054aaf4ca4c0b459a19019d41a0cd3a1978 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:35:21 +0000 Subject: Fix syntax issues --- synapse/http/matrixfederationclient.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 9019c8791a..5b89a2e05c 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,6 +190,7 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( + self, request, try_trailing_slash_on_400=False, backoff_on_404=False, @@ -215,7 +216,7 @@ class MatrixFederationHttpClient(object): Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. """ - response = self._send_request(**kwargs) + response = yield self._send_request(**kwargs) if not try_trailing_slash_on_400: defer.returnValue(response) @@ -225,6 +226,9 @@ class MatrixFederationHttpClient(object): self.hs.get_reactor(), self.default_timeout, request, response, ) + logger.info(" *** BODY IS *** ") + logger.info(body) + # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a # trailing slash on Synapse <=v0.99.2. @@ -588,15 +592,10 @@ class MatrixFederationHttpClient(object): request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, ) - # If enabled, retry with a trailing slash if we received a 400 - if try_trailing_slash_on_400 and response.code == 400: - args["path"] += "/" - - response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) + defer.returnValue(body) @defer.inlineCallbacks @@ -713,6 +712,10 @@ class MatrixFederationHttpClient(object): request, try_trailing_slash_on_400, False, **send_request_args, ) + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) + defer.returnValue(body) @defer.inlineCallbacks -- cgit 1.5.1 From 660b77f3626583fcc49a76e5c2b3d1143677799b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:38:16 +0000 Subject: Add missing docstring detail --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 5b89a2e05c..fca6e242be 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -202,7 +202,7 @@ class MatrixFederationHttpClient(object): due to #3622. Args: - request + request (MatrixFederationRequest): details of request to be sent try_trailing_slash_on_400 (bool): Whether on receiving a 400 'M_UNRECOGNIZED' from the server to retry the request with a trailing slash appended to the request path. -- cgit 1.5.1 From 220607a6183a60a62cfa3fbcfcd30fadc0bdff4b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:43:40 +0000 Subject: Remove testing code --- synapse/http/matrixfederationclient.py | 3 --- 1 file changed, 3 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index fca6e242be..380bf294e5 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -226,9 +226,6 @@ class MatrixFederationHttpClient(object): self.hs.get_reactor(), self.default_timeout, request, response, ) - logger.info(" *** BODY IS *** ") - logger.info(body) - # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a # trailing slash on Synapse <=v0.99.2. -- cgit 1.5.1 From 9dd0e34679f711116f39a35466e7dc956b10ab42 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:45:17 +0000 Subject: Syntax test --- synapse/http/matrixfederationclient.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 380bf294e5..cdeb3792b2 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,12 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs, - ): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 -- cgit 1.5.1 From ee8ba397e89d61ec7beeb549391b9ffeaf7e484a Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 18:48:31 +0000 Subject: Are you happy now --- synapse/http/matrixfederationclient.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index cdeb3792b2..f554d5e21c 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -585,7 +585,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, send_request_args, ) body = yield _handle_json_response( @@ -705,7 +705,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, **send_request_args, + request, try_trailing_slash_on_400, False, send_request_args, ) body = yield _handle_json_response( -- cgit 1.5.1 From c2d848b80d8b83646b1a0413057efad910a3df12 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:04:43 +0000 Subject: Destructure again --- synapse/http/matrixfederationclient.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index f554d5e21c..380bf294e5 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,11 +190,12 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs, + ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 @@ -585,7 +586,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, ) body = yield _handle_json_response( @@ -705,7 +706,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, send_request_args, + request, try_trailing_slash_on_400, False, **send_request_args, ) body = yield _handle_json_response( -- cgit 1.5.1 From c991e7aec78d7cb299c4f9c16f3b80f03c887bfb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:08:08 +0000 Subject: Syntax checker is bork --- synapse/http/matrixfederationclient.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 380bf294e5..cdeb3792b2 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,12 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - **kwargs, - ): + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + **kwargs): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 -- cgit 1.5.1 From bec313818c03f89b501fb8b07efa577f47efee8e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:10:56 +0000 Subject: go home python, you're drunk --- synapse/http/matrixfederationclient.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index cdeb3792b2..de6bc4de00 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -585,8 +585,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args, - ) + request, try_trailing_slash_on_400, backoff_on_404, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -705,8 +704,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, **send_request_args, - ) + request, try_trailing_slash_on_400, False, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, -- cgit 1.5.1 From 66cdb840a6fdf24431cce50e776d65290c419c86 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:18:25 +0000 Subject: Or perhaps I was the one who was drunk --- synapse/http/matrixfederationclient.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index de6bc4de00..f596ccfdf4 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,6 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - backoff_on_404=False, **kwargs): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a @@ -205,9 +204,6 @@ class MatrixFederationHttpClient(object): try_trailing_slash_on_400 (bool): Whether on receiving a 400 'M_UNRECOGNIZED' from the server to retry the request with a trailing slash appended to the request path. - backoff_on_404 (bool): Whether to backoff on 404 when making a - request with a trailing slash (only affects request if - try_trailing_slash_on_400 is True). kwargs (Dict): A dictionary of arguments to pass to `_send_request()`. @@ -585,7 +581,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, **send_request_args) + request, try_trailing_slash_on_400, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -701,10 +697,11 @@ class MatrixFederationHttpClient(object): "retry_on_dns_fail": retry_on_dns_fail, "timeout": timeout, "ignore_backoff": ignore_backoff, + "backoff_on_404": False, } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, **send_request_args) + request, try_trailing_slash_on_400, **send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, -- cgit 1.5.1 From 7c0295f13c00ab1cc613584214af1bfc98041edd Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:27:10 +0000 Subject: no kwargs today --- synapse/http/matrixfederationclient.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index f596ccfdf4..47c7aedfff 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,9 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - **kwargs): + backoff_on_404=False, + send_request_args={}, + ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 @@ -204,14 +206,17 @@ class MatrixFederationHttpClient(object): try_trailing_slash_on_400 (bool): Whether on receiving a 400 'M_UNRECOGNIZED' from the server to retry the request with a trailing slash appended to the request path. - kwargs (Dict): A dictionary of arguments to pass to + 404_backoff (bool): Whether to backoff on 404 when making a + request with a trailing slash (only affects request if + try_trailing_slash_on_400 is True). + send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. Returns: Deferred[twisted.web.client.Response]: resolves with the HTTP response object on success. """ - response = yield self._send_request(**kwargs) + response = yield self._send_request(**send_request_args) if not try_trailing_slash_on_400: defer.returnValue(response) @@ -226,10 +231,10 @@ class MatrixFederationHttpClient(object): # trailing slash on Synapse <=v0.99.2. if (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): # Enable backoff if initially disabled - kwargs["backoff_on_404"] = backoff_on_404 + send_request_args["backoff_on_404"] = backoff_on_404 - kwargs["path"] += "/" - response = yield self._send_request(**kwargs) + send_request_args["path"] += "/" + response = yield self._send_request(**send_request_args) defer.returnValue(response) @@ -581,7 +586,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, **send_request_args) + request, try_trailing_slash_on_400, backoff_on_404, send_request_args) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, -- cgit 1.5.1 From 5ca857ad849784ce72660fce8d8a94bb4aa55d5f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:35:23 +0000 Subject: as above --- synapse/http/matrixfederationclient.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 47c7aedfff..502ff15125 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -586,7 +586,8 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args) + request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + ) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, @@ -706,7 +707,8 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, **send_request_args) + request, try_trailing_slash_on_400, send_request_args, + ) body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, -- cgit 1.5.1 From 26f8e2d099e59a20abf56895b7958f5af853db32 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 19:49:08 +0000 Subject: there comes a time when you should give up. but you dont --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 502ff15125..269cf21488 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -707,7 +707,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, send_request_args, + request, try_trailing_slash_on_400, backoff_on_404, send_request_args, ) body = yield _handle_json_response( -- cgit 1.5.1 From 8d16ffaf7a1ab9ae17e59b2d80b82aa65294bf95 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:03:10 +0000 Subject: i should have given up --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 269cf21488..8ad28d1e21 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -707,7 +707,7 @@ class MatrixFederationHttpClient(object): } response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + request, try_trailing_slash_on_400, False, send_request_args, ) body = yield _handle_json_response( -- cgit 1.5.1 From 45524f2f5e9dc7eca5c439e4a8bff938abc17ee8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:17:39 +0000 Subject: i should have given up x2 --- synapse/http/matrixfederationclient.py | 31 ++++++++++++--------------- tests/http/test_fedclient.py | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 18 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8ad28d1e21..74ea6bcf8e 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -190,11 +190,11 @@ class MatrixFederationHttpClient(object): @defer.inlineCallbacks def _send_request_with_optional_trailing_slash( - self, - request, - try_trailing_slash_on_400=False, - backoff_on_404=False, - send_request_args={}, + self, + request, + try_trailing_slash_on_400=False, + backoff_on_404=False, + send_request_args={}, ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a @@ -213,8 +213,7 @@ class MatrixFederationHttpClient(object): `_send_request()`. Returns: - Deferred[twisted.web.client.Response]: resolves with the HTTP - response object on success. + Deferred[Dict]: Parsed JSON response body. """ response = yield self._send_request(**send_request_args) @@ -236,7 +235,11 @@ class MatrixFederationHttpClient(object): send_request_args["path"] += "/" response = yield self._send_request(**send_request_args) - defer.returnValue(response) + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) + + defer.returnValue(body) @defer.inlineCallbacks def _send_request( @@ -585,14 +588,10 @@ class MatrixFederationHttpClient(object): "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404, } - response = yield self._send_request_with_optional_trailing_slash( + body = yield self._send_request_with_optional_trailing_slash( request, try_trailing_slash_on_400, backoff_on_404, send_request_args, ) - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) - defer.returnValue(body) @defer.inlineCallbacks @@ -706,14 +705,10 @@ class MatrixFederationHttpClient(object): "backoff_on_404": False, } - response = yield self._send_request_with_optional_trailing_slash( + body = yield self._send_request_with_optional_trailing_slash( request, try_trailing_slash_on_400, False, send_request_args, ) - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) - defer.returnValue(body) @defer.inlineCallbacks diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b03b37affe..0d0161d13d 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -268,6 +268,45 @@ class FederationClientTests(HomeserverTestCase): self.assertIsInstance(f.value, TimeoutError) + def test_client_requires_trailing_slashes(self): + """ + If a connection is made to a client but the client rejects it due to + requiring a trailing slash. We need to retry the request with a + trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + """ + d = self.cl.get_json( + "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, + ) + + self.pump() + + # there should have been a call to connectTCP + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (_host, _port, factory, _timeout, _bindAddress) = clients[0] + + # complete the connection and wire it up to a fake transport + client = factory.buildProtocol(None) + conn = StringTransport() + client.makeConnection(conn) + + # that should have made it send the request to the connection + self.assertRegex(conn.value(), b"^GET /foo/bar") + + # Send the HTTP response + client.dataReceived( + b"HTTP/1.1 400 Bad Request\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 59\r\n" + b"\r\n" + b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' + ) + + # We should get a successful response + r = self.successResultOf(d) + self.assertEqual(r.code, 400) + self.assertEqual(r, {}) + def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.5.1 From 9a2e22fd41845b6fa7f30c66cf89a368af269147 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 20:29:38 +0000 Subject: is this what purgatory feels like --- synapse/http/matrixfederationclient.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 74ea6bcf8e..59e758fefb 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -217,14 +217,14 @@ class MatrixFederationHttpClient(object): """ response = yield self._send_request(**send_request_args) - if not try_trailing_slash_on_400: - defer.returnValue(response) - # Check if it's necessary to retry with a trailing slash body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) + if not try_trailing_slash_on_400: + defer.returnValue(body) + # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a # trailing slash on Synapse <=v0.99.2. -- cgit 1.5.1 From b2df0e8e2cb72a2147e6d962a0e8c5ce35233ed4 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 21:08:10 +0000 Subject: receiving a 400 caused an exception. handle it --- synapse/http/matrixfederationclient.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 59e758fefb..6b41639741 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -212,27 +212,34 @@ class MatrixFederationHttpClient(object): send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. + Raises: + HttpResponseException: If we get an HTTP response code >= 300 + (except 429). + Returns: Deferred[Dict]: Parsed JSON response body. """ - response = yield self._send_request(**send_request_args) + try: + response = yield self._send_request(**send_request_args) + except HttpResponseException as e: + # Received a 400. Raise unless we're retrying + if not try_trailing_slash_on_400: + raise e # Check if it's necessary to retry with a trailing slash body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) - if not try_trailing_slash_on_400: - defer.returnValue(body) - # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a # trailing slash on Synapse <=v0.99.2. - if (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): + if not (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): # Enable backoff if initially disabled send_request_args["backoff_on_404"] = backoff_on_404 send_request_args["path"] += "/" + response = yield self._send_request(**send_request_args) body = yield _handle_json_response( -- cgit 1.5.1 From ecea5af491cd69933afe1dbea665d85c84d1d94e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 13 Mar 2019 21:21:03 +0000 Subject: Correct var name --- synapse/http/matrixfederationclient.py | 4 +-- tests/http/test_fedclient.py | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 6b41639741..b27c4c1c38 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -238,10 +238,10 @@ class MatrixFederationHttpClient(object): # Enable backoff if initially disabled send_request_args["backoff_on_404"] = backoff_on_404 - send_request_args["path"] += "/" + # Add trailing slash + send_request_args["request"].path += "/" response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b03b37affe..b45eee3a82 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -268,6 +268,60 @@ class FederationClientTests(HomeserverTestCase): self.assertIsInstance(f.value, TimeoutError) + def test_client_requires_trailing_slashes(self): + """ + If a connection is made to a client but the client rejects it due to + requiring a trailing slash. We need to retry the request with a + trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + """ + d = self.cl.get_json( + "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, + ) + + self.pump() + + # there should have been a call to connectTCP + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (_host, _port, factory, _timeout, _bindAddress) = clients[0] + + # complete the connection and wire it up to a fake transport + client = factory.buildProtocol(None) + conn = StringTransport() + client.makeConnection(conn) + + # that should have made it send the request to the connection + self.assertRegex(conn.value(), b"^GET /foo/bar") + + # Send the HTTP response + client.dataReceived( + b"HTTP/1.1 400 Bad Request\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 59\r\n" + b"\r\n" + b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' + ) + + # We should get a 400 response, then try again + self.pump() + + # We should get another request wiht a trailing slash + self.assertRegex(conn.value(), b"^GET /foo/bar/") + + # Send a happy response this time + client.dataReceived( + b"HTTP/1.1 200 OK\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 2\r\n" + b"\r\n" + b'{}' + ) + + # We should get a successful response + r = self.successResultOf(d) + self.assertEqual(r.code, 200) + self.assertEqual(r, {}) + def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.5.1 From 621e7f37f1a7f32ff5046060f17a1da825f9ff8b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 18 Mar 2019 17:45:54 +0000 Subject: Better exception handling --- synapse/http/matrixfederationclient.py | 33 +++++++++++++++++---------------- tests/http/test_fedclient.py | 5 +---- 2 files changed, 18 insertions(+), 20 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index b27c4c1c38..3c27686a89 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -221,30 +221,31 @@ class MatrixFederationHttpClient(object): """ try: response = yield self._send_request(**send_request_args) + + # Check if it's necessary to retry with a trailing slash + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) except HttpResponseException as e: - # Received a 400. Raise unless we're retrying if not try_trailing_slash_on_400: + # Received an error >= 300. Raise unless we're retrying raise e - - # Check if it's necessary to retry with a trailing slash - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) + except: + raise e # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a - # trailing slash on Synapse <=v0.99.2. - if not (response.code == 400 and body.get("errcode") == "M_UNRECOGNIZED"): - # Enable backoff if initially disabled - send_request_args["backoff_on_404"] = backoff_on_404 + # trailing slash on Synapse <= v0.99.2. + # Enable backoff if initially disabled + send_request_args["backoff_on_404"] = backoff_on_404 - # Add trailing slash - send_request_args["request"].path += "/" + # Add trailing slash + send_request_args["request"].path += "/" - response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) + response = yield self._send_request(**send_request_args) + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, + ) defer.returnValue(body) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b45eee3a82..84216db44f 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -272,7 +272,7 @@ class FederationClientTests(HomeserverTestCase): """ If a connection is made to a client but the client rejects it due to requiring a trailing slash. We need to retry the request with a - trailing slash. Workaround for Synapse <=v0.99.2, explained in #3622. + trailing slash. Workaround for Synapse <= v0.99.2, explained in #3622. """ d = self.cl.get_json( "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, @@ -302,9 +302,6 @@ class FederationClientTests(HomeserverTestCase): b'{"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}' ) - # We should get a 400 response, then try again - self.pump() - # We should get another request wiht a trailing slash self.assertRegex(conn.value(), b"^GET /foo/bar/") -- cgit 1.5.1 From a8ad39eec74ff973f6bfe8d84dfd8d2d3ca32913 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 18 Mar 2019 17:47:39 +0000 Subject: lint --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 3c27686a89..2be0244870 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -230,7 +230,7 @@ class MatrixFederationHttpClient(object): if not try_trailing_slash_on_400: # Received an error >= 300. Raise unless we're retrying raise e - except: + except Exception as e: raise e # Retry with a trailing slash if we received a 400 with -- cgit 1.5.1 From 551ea1155966c023d7d2eb41454598bf14cb4be2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 11:07:36 +0000 Subject: Just return if not doing any trailing slash shennanigans --- synapse/http/matrixfederationclient.py | 2 ++ 1 file changed, 2 insertions(+) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 2be0244870..1307508e5a 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -226,6 +226,8 @@ class MatrixFederationHttpClient(object): body = yield _handle_json_response( self.hs.get_reactor(), self.default_timeout, request, response, ) + + defer.returnValue(body) except HttpResponseException as e: if not try_trailing_slash_on_400: # Received an error >= 300. Raise unless we're retrying -- cgit 1.5.1 From c69df5d5d37652b98d670a25c8c4291002af9242 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 11:27:18 +0000 Subject: Fix comments. v0.99.2 -> v0.99.3 --- synapse/http/matrixfederationclient.py | 10 +++++----- tests/http/test_fedclient.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1307508e5a..8312e4fc3f 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -198,7 +198,7 @@ class MatrixFederationHttpClient(object): ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a - 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <=v0.99.2 + 'M_UNRECOGNIZED' errcode. This is a workaround for Synapse <= v0.99.3 due to #3622. Args: @@ -237,7 +237,7 @@ class MatrixFederationHttpClient(object): # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a - # trailing slash on Synapse <= v0.99.2. + # trailing slash on Synapse <= v0.99.3. # Enable backoff if initially disabled send_request_args["backoff_on_404"] = backoff_on_404 @@ -560,7 +560,7 @@ class MatrixFederationHttpClient(object): requests). try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED response we should try appending a trailing slash to the end - of the request. Workaround for #3622 in Synapse <=v0.99.2. This + of the request. Workaround for #3622 in Synapse <= v0.99.3. This will be attempted before backing off if backing off has been enabled. @@ -593,7 +593,7 @@ class MatrixFederationHttpClient(object): "timeout": timeout, "ignore_backoff": ignore_backoff, # Do not backoff on the initial request if we're trying again with - # trailing slashes Otherwise we may end up waiting to contact a + # trailing slashes. Otherwise we may end up waiting to contact a # server that is actually up "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404, } @@ -681,7 +681,7 @@ class MatrixFederationHttpClient(object): and try the request anyway. try_trailing_slash_on_400 (bool): True if on a 400 M_UNRECOGNIZED response we should try appending a trailing slash to the end of - the request. Workaround for #3622 in Synapse <=v0.99.2. + the request. Workaround for #3622 in Synapse <= v0.99.3. Returns: Deferred[dict|list]: Succeeds when we get a 2xx HTTP response. The result will be the decoded JSON body. diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index b1b3a025ef..de5da1694a 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -272,7 +272,7 @@ class FederationClientTests(HomeserverTestCase): """ If a connection is made to a client but the client rejects it due to requiring a trailing slash. We need to retry the request with a - trailing slash. Workaround for Synapse <= v0.99.2, explained in #3622. + trailing slash. Workaround for Synapse <= v0.99.3, explained in #3622. """ d = self.cl.get_json( "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, -- cgit 1.5.1 From cd36a1283b6036505b77f51a0cc860e8e5a0878d Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:00:39 +0000 Subject: New test, fix issues --- synapse/http/matrixfederationclient.py | 77 +++++++++++++--------------------- tests/http/test_fedclient.py | 45 ++++++++++++++++++++ 2 files changed, 73 insertions(+), 49 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8312e4fc3f..dd358536f1 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -194,7 +194,7 @@ class MatrixFederationHttpClient(object): request, try_trailing_slash_on_400=False, backoff_on_404=False, - send_request_args={}, + **send_request_args ): """Wrapper for _send_request which can optionally retry the request upon receiving a combination of a 400 HTTP response code and a @@ -206,9 +206,8 @@ class MatrixFederationHttpClient(object): try_trailing_slash_on_400 (bool): Whether on receiving a 400 'M_UNRECOGNIZED' from the server to retry the request with a trailing slash appended to the request path. - 404_backoff (bool): Whether to backoff on 404 when making a - request with a trailing slash (only affects request if - try_trailing_slash_on_400 is True). + backoff_on_404 (bool): Whether to backoff on 404 when making a + request with a trailing slash. send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. @@ -220,36 +219,24 @@ class MatrixFederationHttpClient(object): Deferred[Dict]: Parsed JSON response body. """ try: - response = yield self._send_request(**send_request_args) - - # Check if it's necessary to retry with a trailing slash - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) - - defer.returnValue(body) + response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) except HttpResponseException as e: + # Received an HTTP error > 300. Check if it meets the requirements + # to retry with a trailing slash if not try_trailing_slash_on_400: - # Received an error >= 300. Raise unless we're retrying - raise e - except Exception as e: - raise e + raise - # Retry with a trailing slash if we received a 400 with - # 'M_UNRECOGNIZED' which some endpoints can return when omitting a - # trailing slash on Synapse <= v0.99.3. - # Enable backoff if initially disabled - send_request_args["backoff_on_404"] = backoff_on_404 + if e.code != 400 or e.to_synapse_error().errcode != "M_UNRECOGNIZED": + raise - # Add trailing slash - send_request_args["request"].path += "/" + # Retry with a trailing slash if we received a 400 with + # 'M_UNRECOGNIZED' which some endpoints can return when omitting a + # trailing slash on Synapse <= v0.99.3. + request.path += "/" - response = yield self._send_request(**send_request_args) - body = yield _handle_json_response( - self.hs.get_reactor(), self.default_timeout, request, response, - ) + response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) - defer.returnValue(body) + defer.returnValue(response) @defer.inlineCallbacks def _send_request( @@ -587,19 +574,13 @@ class MatrixFederationHttpClient(object): json=data, ) - send_request_args = { - "request": request, - "long_retries": long_retries, - "timeout": timeout, - "ignore_backoff": ignore_backoff, - # Do not backoff on the initial request if we're trying again with - # trailing slashes. Otherwise we may end up waiting to contact a - # server that is actually up - "backoff_on_404": False if try_trailing_slash_on_400 else backoff_on_404, - } + response = yield self._send_request_with_optional_trailing_slash( + request, try_trailing_slash_on_400, backoff_on_404, + long_retries=long_retries, timeout=timeout, ignore_backoff=ignore_backoff, + ) - body = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, send_request_args, + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, ) defer.returnValue(body) @@ -707,16 +688,14 @@ class MatrixFederationHttpClient(object): query=args, ) - send_request_args = { - "request": request, - "retry_on_dns_fail": retry_on_dns_fail, - "timeout": timeout, - "ignore_backoff": ignore_backoff, - "backoff_on_404": False, - } + response = yield self._send_request_with_optional_trailing_slash( + request, try_trailing_slash_on_400, False, + retry_on_dns_fail=retry_on_dns_fail, timeout=timeout, + ignore_backoff=ignore_backoff, + ) - body = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, send_request_args, + body = yield _handle_json_response( + self.hs.get_reactor(), self.default_timeout, request, response, ) defer.returnValue(body) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index de5da1694a..fbe87d4d0b 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -322,6 +322,51 @@ class FederationClientTests(HomeserverTestCase): r = self.successResultOf(d) self.assertEqual(r, {}) + def test_client_does_not_retry_on_400_plus(self): + """ + Another test for trailing slashes but now test that we don't retry on + trailing slashes on a non-400/M_UNRECOGNIZED response. + + See test_client_requires_trailing_slashes() for context. + """ + d = self.cl.get_json( + "testserv:8008", "foo/bar", try_trailing_slash_on_400=True, + ) + + # Send the request + self.pump() + + # there should have been a call to connectTCP + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (_host, _port, factory, _timeout, _bindAddress) = clients[0] + + # complete the connection and wire it up to a fake transport + client = factory.buildProtocol(None) + conn = StringTransport() + client.makeConnection(conn) + + # that should have made it send the request to the connection + self.assertRegex(conn.value(), b"^GET /foo/bar") + + # Clear the original request data before sending a response + conn.clear() + + # Send the HTTP response + client.dataReceived( + b"HTTP/1.1 404 Not Found\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 2\r\n" + b"\r\n" + b"{}" + ) + + # We should not get another request + self.assertEqual(conn.value(), b"") + + # We should get a 404 failure response + r = self.failureResultOf(d) + def test_client_sends_body(self): self.cl.post_json( "testserv:8008", "foo/bar", timeout=10000, -- cgit 1.5.1 From bb52a2e65308f10285d5d6290786e889d203f3db Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:08:57 +0000 Subject: lint --- synapse/http/matrixfederationclient.py | 8 ++++++-- tests/http/test_fedclient.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index dd358536f1..2c1bcf66fd 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -219,7 +219,9 @@ class MatrixFederationHttpClient(object): Deferred[Dict]: Parsed JSON response body. """ try: - response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) + response = yield self._send_request( + request, backoff_on_404=backoff_on_404, **send_request_args, + ) except HttpResponseException as e: # Received an HTTP error > 300. Check if it meets the requirements # to retry with a trailing slash @@ -234,7 +236,9 @@ class MatrixFederationHttpClient(object): # trailing slash on Synapse <= v0.99.3. request.path += "/" - response = yield self._send_request(request, backoff_on_404=backoff_on_404, **send_request_args) + response = yield self._send_request( + request, backoff_on_404=backoff_on_404, **send_request_args, + ) defer.returnValue(response) diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index fbe87d4d0b..cd8e086f86 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -365,7 +365,7 @@ class FederationClientTests(HomeserverTestCase): self.assertEqual(conn.value(), b"") # We should get a 404 failure response - r = self.failureResultOf(d) + self.failureResultOf(d) def test_client_sends_body(self): self.cl.post_json( -- cgit 1.5.1 From 2150151abe919884671fe2d080e5145d9face5fa Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 20 Mar 2019 14:13:32 +0000 Subject: kwargs doesn't like commas on calling funcs either. TIL --- synapse/http/matrixfederationclient.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 2c1bcf66fd..c654c1cf12 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -220,7 +220,7 @@ class MatrixFederationHttpClient(object): """ try: response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args, + request, backoff_on_404=backoff_on_404, **send_request_args ) except HttpResponseException as e: # Received an HTTP error > 300. Check if it meets the requirements @@ -237,7 +237,7 @@ class MatrixFederationHttpClient(object): request.path += "/" response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args, + request, backoff_on_404=backoff_on_404, **send_request_args ) defer.returnValue(response) -- cgit 1.5.1 From 536a2665204ae6765ec131e985e9828c6c363539 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 11:20:13 +0000 Subject: Deny peeking into rooms that have been blocked --- synapse/handlers/events.py | 7 +++- synapse/handlers/initial_sync.py | 6 +++- tests/rest/client/v1/test_admin.py | 66 +++++++++++++++++++++++++++++++++++++- 3 files changed, 76 insertions(+), 3 deletions(-) (limited to 'synapse') diff --git a/synapse/handlers/events.py b/synapse/handlers/events.py index f772e62c28..d883e98381 100644 --- a/synapse/handlers/events.py +++ b/synapse/handlers/events.py @@ -19,7 +19,7 @@ import random from twisted.internet import defer from synapse.api.constants import EventTypes, Membership -from synapse.api.errors import AuthError +from synapse.api.errors import AuthError, SynapseError from synapse.events import EventBase from synapse.events.utils import serialize_event from synapse.types import UserID @@ -61,6 +61,11 @@ class EventStreamHandler(BaseHandler): If `only_keys` is not None, events from keys will be sent down. """ + if room_id: + blocked = yield self.store.is_room_blocked(room_id) + if blocked: + raise SynapseError(403, "This room has been blocked on this server") + # send any outstanding server notices to the user. yield self._server_notices_sender.on_user_syncing(auth_user_id) diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py index 563bb3cea3..7dfae78db0 100644 --- a/synapse/handlers/initial_sync.py +++ b/synapse/handlers/initial_sync.py @@ -18,7 +18,7 @@ import logging from twisted.internet import defer from synapse.api.constants import EventTypes, Membership -from synapse.api.errors import AuthError, Codes +from synapse.api.errors import AuthError, Codes, SynapseError from synapse.events.utils import serialize_event from synapse.events.validator import EventValidator from synapse.handlers.presence import format_user_presence_state @@ -262,6 +262,10 @@ class InitialSyncHandler(BaseHandler): A JSON serialisable dict with the snapshot of the room. """ + blocked = yield self.store.is_room_blocked(room_id) + if blocked: + raise SynapseError(403, "This room has been blocked on this server") + user_id = requester.user.to_string() membership, member_event_id = yield self._check_in_room_or_world_readable( diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index fb4ac6b95f..8ea19351fe 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -20,7 +20,7 @@ import json from mock import Mock from synapse.api.constants import UserTypes -from synapse.rest.client.v1 import admin, login, room +from synapse.rest.client.v1 import admin, login, room, events from tests import unittest @@ -359,7 +359,9 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): servlets = [ admin.register_servlets, login.register_servlets, + events.register_servlets, room.register_servlets, + room.register_deprecated_servlets, ] def prepare(self, reactor, clock, hs): @@ -422,3 +424,65 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): self.store.get_users_in_room(room_id), ) self.assertEqual([], users_in_room) + + @unittest.DEBUG + def test_shutdown_room_block_peek(self): + """Test that a world_readable room can no longer be peeked into after + it has been shut down. + """ + + self.event_creation_handler._block_events_without_consent_error = None + + room_id = self.helper.create_room_as(self.other_user, tok=self.other_user_token) + + # Enable world readable + url = "rooms/%s/state/m.room.history_visibility" % (room_id,) + request, channel = self.make_request( + "PUT", + url.encode('ascii'), + json.dumps({"history_visibility": "world_readable"}), + access_token=self.other_user_token, + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Test that the admin can still send shutdown + url = "admin/shutdown_room/" + room_id + request, channel = self.make_request( + "POST", + url.encode('ascii'), + json.dumps({"new_room_user_id": self.admin_user}), + access_token=self.admin_user_tok, + ) + self.render(request) + + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Assert we can no longer peek into the room + self._assert_peek(room_id, expect_code=403) + + def _assert_peek(self, room_id, expect_code): + """Assert that the admin user can (or cannot) peek into the room. + """ + + url = "rooms/%s/initialSync" % (room_id,) + request, channel = self.make_request( + "GET", + url.encode('ascii'), + access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual( + expect_code, int(channel.result["code"]), msg=channel.result["body"], + ) + + url = "events?timeout=0&room_id=" + room_id + request, channel = self.make_request( + "GET", + url.encode('ascii'), + access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual( + expect_code, int(channel.result["code"]), msg=channel.result["body"], + ) -- cgit 1.5.1 From b41c2eaadc1b9d6150ac0f7979e877e5ee9ab80e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 21 Mar 2019 14:32:47 +0000 Subject: Clean up backoff_on_404 and metehod calls --- synapse/http/matrixfederationclient.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'synapse') diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index c654c1cf12..8e855d13d6 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -193,7 +193,6 @@ class MatrixFederationHttpClient(object): self, request, try_trailing_slash_on_400=False, - backoff_on_404=False, **send_request_args ): """Wrapper for _send_request which can optionally retry the request @@ -206,8 +205,6 @@ class MatrixFederationHttpClient(object): try_trailing_slash_on_400 (bool): Whether on receiving a 400 'M_UNRECOGNIZED' from the server to retry the request with a trailing slash appended to the request path. - backoff_on_404 (bool): Whether to backoff on 404 when making a - request with a trailing slash. send_request_args (Dict): A dictionary of arguments to pass to `_send_request()`. @@ -220,7 +217,7 @@ class MatrixFederationHttpClient(object): """ try: response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args + request, **send_request_args ) except HttpResponseException as e: # Received an HTTP error > 300. Check if it meets the requirements @@ -237,7 +234,7 @@ class MatrixFederationHttpClient(object): request.path += "/" response = yield self._send_request( - request, backoff_on_404=backoff_on_404, **send_request_args + request, **send_request_args ) defer.returnValue(response) @@ -579,8 +576,12 @@ class MatrixFederationHttpClient(object): ) response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, backoff_on_404, - long_retries=long_retries, timeout=timeout, ignore_backoff=ignore_backoff, + request, + try_trailing_slash_on_400, + backoff_on_404=backoff_on_404, + ignore_backoff=ignore_backoff, + long_retries=long_retries, + timeout=timeout, ) body = yield _handle_json_response( @@ -693,9 +694,12 @@ class MatrixFederationHttpClient(object): ) response = yield self._send_request_with_optional_trailing_slash( - request, try_trailing_slash_on_400, False, - retry_on_dns_fail=retry_on_dns_fail, timeout=timeout, + request, + try_trailing_slash_on_400, + backoff_on_404=False, ignore_backoff=ignore_backoff, + retry_on_dns_fail=retry_on_dns_fail, + timeout=timeout, ) body = yield _handle_json_response( -- cgit 1.5.1 From 3677548a82be347bcc69cfd4bfa4570581ee755f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 22 Mar 2019 10:20:17 +0000 Subject: Use yaml safe_load --- scripts-dev/convert_server_keys.py | 2 +- synapse/config/_base.py | 6 +++--- synapse/config/appservice.py | 2 +- synapse/config/logger.py | 2 +- synctl | 4 ++-- tests/config/test_load.py | 2 +- tests/config/test_room_directory.py | 4 ++-- 7 files changed, 11 insertions(+), 11 deletions(-) (limited to 'synapse') diff --git a/scripts-dev/convert_server_keys.py b/scripts-dev/convert_server_keys.py index dde8596697..ac152b5c42 100644 --- a/scripts-dev/convert_server_keys.py +++ b/scripts-dev/convert_server_keys.py @@ -76,7 +76,7 @@ def rows_v2(server, json): def main(): - config = yaml.load(open(sys.argv[1])) + config = yaml.safe_load(open(sys.argv[1])) valid_until = int(time.time() / (3600 * 24)) * 1000 * 3600 * 24 server_name = config["server_name"] diff --git a/synapse/config/_base.py b/synapse/config/_base.py index a219a83550..f7d7f153bb 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -137,7 +137,7 @@ class Config(object): @staticmethod def read_config_file(file_path): with open(file_path) as file_stream: - return yaml.load(file_stream) + return yaml.safe_load(file_stream) def invoke_all(self, name, *args, **kargs): results = [] @@ -318,7 +318,7 @@ class Config(object): ) config_file.write(config_str) - config = yaml.load(config_str) + config = yaml.safe_load(config_str) obj.invoke_all("generate_files", config) print( @@ -390,7 +390,7 @@ class Config(object): server_name=server_name, generate_secrets=False, ) - config = yaml.load(config_string) + config = yaml.safe_load(config_string) config.pop("log_config") config.update(specified_config) diff --git a/synapse/config/appservice.py b/synapse/config/appservice.py index 9e64c76544..7e89d345d8 100644 --- a/synapse/config/appservice.py +++ b/synapse/config/appservice.py @@ -68,7 +68,7 @@ def load_appservices(hostname, config_files): try: with open(config_file, 'r') as f: appservice = _load_appservice( - hostname, yaml.load(f), config_file + hostname, yaml.safe_load(f), config_file ) if appservice.id in seen_ids: raise ConfigError( diff --git a/synapse/config/logger.py b/synapse/config/logger.py index 464c28c2d9..c1febbe9d3 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -195,7 +195,7 @@ def setup_logging(config, use_worker_options=False): else: def load_log_config(): with open(log_config, 'r') as f: - logging.config.dictConfig(yaml.load(f)) + logging.config.dictConfig(yaml.safe_load(f)) def sighup(*args): # it might be better to use a file watcher or something for this. diff --git a/synctl b/synctl index 816c898b36..07a68e6d85 100755 --- a/synctl +++ b/synctl @@ -164,7 +164,7 @@ def main(): sys.exit(1) with open(configfile) as stream: - config = yaml.load(stream) + config = yaml.safe_load(stream) pidfile = config["pid_file"] cache_factor = config.get("synctl_cache_factor") @@ -206,7 +206,7 @@ def main(): workers = [] for worker_configfile in worker_configfiles: with open(worker_configfile) as stream: - worker_config = yaml.load(stream) + worker_config = yaml.safe_load(stream) worker_app = worker_config["worker_app"] if worker_app == "synapse.app.homeserver": # We need to special case all of this to pick up options that may diff --git a/tests/config/test_load.py b/tests/config/test_load.py index d5f1777093..6bfc1970ad 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -43,7 +43,7 @@ class ConfigLoadingTestCase(unittest.TestCase): self.generate_config() with open(self.file, "r") as f: - raw = yaml.load(f) + raw = yaml.safe_load(f) self.assertIn("macaroon_secret_key", raw) config = HomeServerConfig.load_config("", ["-c", self.file]) diff --git a/tests/config/test_room_directory.py b/tests/config/test_room_directory.py index 3dc2631523..47fffcfeb2 100644 --- a/tests/config/test_room_directory.py +++ b/tests/config/test_room_directory.py @@ -22,7 +22,7 @@ from tests import unittest class RoomDirectoryConfigTestCase(unittest.TestCase): def test_alias_creation_acl(self): - config = yaml.load(""" + config = yaml.safe_load(""" alias_creation_rules: - user_id: "*bob*" alias: "*" @@ -74,7 +74,7 @@ class RoomDirectoryConfigTestCase(unittest.TestCase): )) def test_room_publish_acl(self): - config = yaml.load(""" + config = yaml.safe_load(""" alias_creation_rules: [] room_list_publication_rules: -- cgit 1.5.1 From ac396a0d325eb7fc1311d5bd31b2693ff10fc53c Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Mon, 25 Mar 2019 20:37:08 +1100 Subject: Refactor out state delta handling into its own class (#4917) --- changelog.d/4917.misc | 1 + synapse/handlers/state_deltas.py | 70 ++++++++++++++++++++++++++++++++++++ synapse/handlers/user_directory.py | 51 +++----------------------- synapse/storage/state_deltas.py | 74 ++++++++++++++++++++++++++++++++++++++ synapse/storage/user_directory.py | 66 ++-------------------------------- 5 files changed, 152 insertions(+), 110 deletions(-) create mode 100644 changelog.d/4917.misc create mode 100644 synapse/handlers/state_deltas.py create mode 100644 synapse/storage/state_deltas.py (limited to 'synapse') diff --git a/changelog.d/4917.misc b/changelog.d/4917.misc new file mode 100644 index 0000000000..338d8a9a0c --- /dev/null +++ b/changelog.d/4917.misc @@ -0,0 +1 @@ +Refactor out the state deltas portion of the user directory store and handler. diff --git a/synapse/handlers/state_deltas.py b/synapse/handlers/state_deltas.py new file mode 100644 index 0000000000..b268bbcb2c --- /dev/null +++ b/synapse/handlers/state_deltas.py @@ -0,0 +1,70 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 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. + +import logging + +from twisted.internet import defer + +logger = logging.getLogger(__name__) + + +class StateDeltasHandler(object): + + def __init__(self, hs): + self.store = hs.get_datastore() + + @defer.inlineCallbacks + def _get_key_change(self, prev_event_id, event_id, key_name, public_value): + """Given two events check if the `key_name` field in content changed + from not matching `public_value` to doing so. + + For example, check if `history_visibility` (`key_name`) changed from + `shared` to `world_readable` (`public_value`). + + Returns: + None if the field in the events either both match `public_value` + or if neither do, i.e. there has been no change. + True if it didnt match `public_value` but now does + False if it did match `public_value` but now doesn't + """ + prev_event = None + event = None + if prev_event_id: + prev_event = yield self.store.get_event(prev_event_id, allow_none=True) + + if event_id: + event = yield self.store.get_event(event_id, allow_none=True) + + if not event and not prev_event: + logger.debug("Neither event exists: %r %r", prev_event_id, event_id) + defer.returnValue(None) + + prev_value = None + value = None + + if prev_event: + prev_value = prev_event.content.get(key_name) + + if event: + value = event.content.get(key_name) + + logger.debug("prev_value: %r -> value: %r", prev_value, value) + + if value == public_value and prev_value != public_value: + defer.returnValue(True) + elif value != public_value and prev_value == public_value: + defer.returnValue(False) + else: + defer.returnValue(None) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 7dc0e236e7..b689979b4b 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -21,6 +21,7 @@ from twisted.internet import defer import synapse.metrics from synapse.api.constants import EventTypes, JoinRules, Membership +from synapse.handlers.state_deltas import StateDeltasHandler from synapse.metrics.background_process_metrics import run_as_background_process from synapse.storage.roommember import ProfileInfo from synapse.types import get_localpart_from_id @@ -29,7 +30,7 @@ from synapse.util.metrics import Measure logger = logging.getLogger(__name__) -class UserDirectoryHandler(object): +class UserDirectoryHandler(StateDeltasHandler): """Handles querying of and keeping updated the user_directory. N.B.: ASSUMES IT IS THE ONLY THING THAT MODIFIES THE USER DIRECTORY @@ -41,6 +42,8 @@ class UserDirectoryHandler(object): """ def __init__(self, hs): + super(UserDirectoryHandler, self).__init__(hs) + self.store = hs.get_datastore() self.state = hs.get_state_handler() self.server_name = hs.hostname @@ -360,7 +363,7 @@ class UserDirectoryHandler(object): @defer.inlineCallbacks def _handle_remove_user(self, room_id, user_id): - """Called when we might need to remove user to directory + """Called when we might need to remove user from directory Args: room_id (str): room_id that user left or stopped being public that @@ -402,47 +405,3 @@ class UserDirectoryHandler(object): if prev_name != new_name or prev_avatar != new_avatar: yield self.store.update_profile_in_user_dir(user_id, new_name, new_avatar) - - @defer.inlineCallbacks - def _get_key_change(self, prev_event_id, event_id, key_name, public_value): - """Given two events check if the `key_name` field in content changed - from not matching `public_value` to doing so. - - For example, check if `history_visibility` (`key_name`) changed from - `shared` to `world_readable` (`public_value`). - - Returns: - None if the field in the events either both match `public_value` - or if neither do, i.e. there has been no change. - True if it didnt match `public_value` but now does - False if it did match `public_value` but now doesn't - """ - prev_event = None - event = None - if prev_event_id: - prev_event = yield self.store.get_event(prev_event_id, allow_none=True) - - if event_id: - event = yield self.store.get_event(event_id, allow_none=True) - - if not event and not prev_event: - logger.debug("Neither event exists: %r %r", prev_event_id, event_id) - defer.returnValue(None) - - prev_value = None - value = None - - if prev_event: - prev_value = prev_event.content.get(key_name) - - if event: - value = event.content.get(key_name) - - logger.debug("prev_value: %r -> value: %r", prev_value, value) - - if value == public_value and prev_value != public_value: - defer.returnValue(True) - elif value != public_value and prev_value == public_value: - defer.returnValue(False) - else: - defer.returnValue(None) diff --git a/synapse/storage/state_deltas.py b/synapse/storage/state_deltas.py new file mode 100644 index 0000000000..57bc45cdb9 --- /dev/null +++ b/synapse/storage/state_deltas.py @@ -0,0 +1,74 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 Vector Creations 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.storage._base import SQLBaseStore + +logger = logging.getLogger(__name__) + + +class StateDeltasStore(SQLBaseStore): + + def get_current_state_deltas(self, prev_stream_id): + prev_stream_id = int(prev_stream_id) + if not self._curr_state_delta_stream_cache.has_any_entity_changed(prev_stream_id): + return [] + + def get_current_state_deltas_txn(txn): + # First we calculate the max stream id that will give us less than + # N results. + # We arbitarily limit to 100 stream_id entries to ensure we don't + # select toooo many. + sql = """ + SELECT stream_id, count(*) + FROM current_state_delta_stream + WHERE stream_id > ? + GROUP BY stream_id + ORDER BY stream_id ASC + LIMIT 100 + """ + txn.execute(sql, (prev_stream_id,)) + + total = 0 + max_stream_id = prev_stream_id + for max_stream_id, count in txn: + total += count + if total > 100: + # We arbitarily limit to 100 entries to ensure we don't + # select toooo many. + break + + # Now actually get the deltas + sql = """ + SELECT stream_id, room_id, type, state_key, event_id, prev_event_id + FROM current_state_delta_stream + WHERE ? < stream_id AND stream_id <= ? + ORDER BY stream_id ASC + """ + txn.execute(sql, (prev_stream_id, max_stream_id,)) + return self.cursor_to_dict(txn) + + return self.runInteraction( + "get_current_state_deltas", get_current_state_deltas_txn + ) + + def get_max_stream_id_in_current_state_deltas(self): + return self._simple_select_one_onecol( + table="current_state_delta_stream", + keyvalues={}, + retcol="COALESCE(MAX(stream_id), -1)", + desc="get_max_stream_id_in_current_state_deltas", + ) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index d360e857d1..65bdb1b4a5 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -22,6 +22,7 @@ from synapse.api.constants import EventTypes, JoinRules from synapse.storage.background_updates import BackgroundUpdateStore from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.storage.state import StateFilter +from synapse.storage.state_deltas import StateDeltasStore from synapse.types import get_domain_from_id, get_localpart_from_id from synapse.util.caches.descriptors import cached @@ -31,7 +32,7 @@ logger = logging.getLogger(__name__) TEMP_TABLE = "_temp_populate_user_directory" -class UserDirectoryStore(BackgroundUpdateStore): +class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): # How many records do we calculate before sending it to # add_users_who_share_private_rooms? @@ -488,16 +489,6 @@ class UserDirectoryStore(BackgroundUpdateStore): defer.returnValue(user_ids) - @defer.inlineCallbacks - def get_all_local_users(self): - """Get all local users - """ - sql = """ - SELECT name FROM users - """ - rows = yield self._execute("get_all_local_users", None, sql) - defer.returnValue([name for name, in rows]) - def add_users_who_share_private_room(self, room_id, user_id_tuples): """Insert entries into the users_who_share_private_rooms table. The first user should be a local user. @@ -675,59 +666,6 @@ class UserDirectoryStore(BackgroundUpdateStore): desc="update_user_directory_stream_pos", ) - def get_current_state_deltas(self, prev_stream_id): - prev_stream_id = int(prev_stream_id) - if not self._curr_state_delta_stream_cache.has_any_entity_changed( - prev_stream_id - ): - return [] - - def get_current_state_deltas_txn(txn): - # First we calculate the max stream id that will give us less than - # N results. - # We arbitarily limit to 100 stream_id entries to ensure we don't - # select toooo many. - sql = """ - SELECT stream_id, count(*) - FROM current_state_delta_stream - WHERE stream_id > ? - GROUP BY stream_id - ORDER BY stream_id ASC - LIMIT 100 - """ - txn.execute(sql, (prev_stream_id,)) - - total = 0 - max_stream_id = prev_stream_id - for max_stream_id, count in txn: - total += count - if total > 100: - # We arbitarily limit to 100 entries to ensure we don't - # select toooo many. - break - - # Now actually get the deltas - sql = """ - SELECT stream_id, room_id, type, state_key, event_id, prev_event_id - FROM current_state_delta_stream - WHERE ? < stream_id AND stream_id <= ? - ORDER BY stream_id ASC - """ - txn.execute(sql, (prev_stream_id, max_stream_id)) - return self.cursor_to_dict(txn) - - return self.runInteraction( - "get_current_state_deltas", get_current_state_deltas_txn - ) - - def get_max_stream_id_in_current_state_deltas(self): - return self._simple_select_one_onecol( - table="current_state_delta_stream", - keyvalues={}, - retcol="COALESCE(MAX(stream_id), -1)", - desc="get_max_stream_id_in_current_state_deltas", - ) - @defer.inlineCallbacks def search_user_dir(self, user_id, search_term, limit): """Searches for users in directory -- cgit 1.5.1 From 9bde730ef821a20f6a785813b19953a9ba187ce7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 25 Mar 2019 16:38:05 +0000 Subject: Fix bug where read-receipts lost their timestamps (#4927) Make sure that they are sent correctly over the replication stream. Fixes: #4898 --- changelog.d/4927.feature | 1 + synapse/replication/tcp/protocol.py | 27 +++++++--- synapse/replication/tcp/streams.py | 11 ++-- synapse/storage/receipts.py | 4 +- tests/replication/tcp/__init__.py | 14 +++++ tests/replication/tcp/streams/__init__.py | 14 +++++ tests/replication/tcp/streams/_base.py | 74 ++++++++++++++++++++++++++ tests/replication/tcp/streams/test_receipts.py | 46 ++++++++++++++++ 8 files changed, 179 insertions(+), 12 deletions(-) create mode 100644 changelog.d/4927.feature create mode 100644 tests/replication/tcp/__init__.py create mode 100644 tests/replication/tcp/streams/__init__.py create mode 100644 tests/replication/tcp/streams/_base.py create mode 100644 tests/replication/tcp/streams/test_receipts.py (limited to 'synapse') diff --git a/changelog.d/4927.feature b/changelog.d/4927.feature new file mode 100644 index 0000000000..8d74262250 --- /dev/null +++ b/changelog.d/4927.feature @@ -0,0 +1 @@ +Batch up outgoing read-receipts to reduce federation traffic. diff --git a/synapse/replication/tcp/protocol.py b/synapse/replication/tcp/protocol.py index 55630ba9a7..e16fad5261 100644 --- a/synapse/replication/tcp/protocol.py +++ b/synapse/replication/tcp/protocol.py @@ -223,14 +223,25 @@ class BaseReplicationStreamProtocol(LineOnlyReceiver): return # Now lets try and call on_ function - try: - run_as_background_process( - "replication-" + cmd.get_logcontext_id(), - getattr(self, "on_%s" % (cmd_name,)), - cmd, - ) - except Exception: - logger.exception("[%s] Failed to handle line: %r", self.id(), line) + run_as_background_process( + "replication-" + cmd.get_logcontext_id(), + self.handle_command, + cmd, + ) + + def handle_command(self, cmd): + """Handle a command we have received over the replication stream. + + By default delegates to on_ + + Args: + cmd (synapse.replication.tcp.commands.Command): received command + + Returns: + Deferred + """ + handler = getattr(self, "on_%s" % (cmd.NAME,)) + return handler(cmd) def close(self): logger.warn("[%s] Closing connection", self.id()) diff --git a/synapse/replication/tcp/streams.py b/synapse/replication/tcp/streams.py index c1e626be3f..e23084baae 100644 --- a/synapse/replication/tcp/streams.py +++ b/synapse/replication/tcp/streams.py @@ -23,7 +23,7 @@ Each stream is defined by the following information: current_token: The function that returns the current token for the stream update_function: The function that returns a list of updates between two tokens """ - +import itertools import logging from collections import namedtuple @@ -195,8 +195,8 @@ class Stream(object): limit=MAX_EVENTS_BEHIND + 1, ) - if len(rows) >= MAX_EVENTS_BEHIND: - raise Exception("stream %s has fallen behind" % (self.NAME)) + # never turn more than MAX_EVENTS_BEHIND + 1 into updates. + rows = itertools.islice(rows, MAX_EVENTS_BEHIND + 1) else: rows = yield self.update_function( from_token, current_token, @@ -204,6 +204,11 @@ class Stream(object): updates = [(row[0], self.ROW_TYPE(*row[1:])) for row in rows] + # check we didn't get more rows than the limit. + # doing it like this allows the update_function to be a generator. + if self._LIMITED and len(updates) >= MAX_EVENTS_BEHIND: + raise Exception("stream %s has fallen behind" % (self.NAME)) + defer.returnValue((updates, current_token)) def current_token(self): diff --git a/synapse/storage/receipts.py b/synapse/storage/receipts.py index 0fd1ccc40a..89a1f7e3d7 100644 --- a/synapse/storage/receipts.py +++ b/synapse/storage/receipts.py @@ -301,7 +301,9 @@ class ReceiptsWorkerStore(SQLBaseStore): args.append(limit) txn.execute(sql, args) - return txn.fetchall() + return ( + r[0:5] + (json.loads(r[5]), ) for r in txn + ) return self.runInteraction( "get_all_updated_receipts", get_all_updated_receipts_txn ) diff --git a/tests/replication/tcp/__init__.py b/tests/replication/tcp/__init__.py new file mode 100644 index 0000000000..1453d04571 --- /dev/null +++ b/tests/replication/tcp/__init__.py @@ -0,0 +1,14 @@ +# -*- coding: utf-8 -*- +# 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. diff --git a/tests/replication/tcp/streams/__init__.py b/tests/replication/tcp/streams/__init__.py new file mode 100644 index 0000000000..1453d04571 --- /dev/null +++ b/tests/replication/tcp/streams/__init__.py @@ -0,0 +1,14 @@ +# -*- coding: utf-8 -*- +# 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. diff --git a/tests/replication/tcp/streams/_base.py b/tests/replication/tcp/streams/_base.py new file mode 100644 index 0000000000..38b368a972 --- /dev/null +++ b/tests/replication/tcp/streams/_base.py @@ -0,0 +1,74 @@ +# -*- coding: utf-8 -*- +# 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. +from synapse.replication.tcp.commands import ReplicateCommand +from synapse.replication.tcp.protocol import ClientReplicationStreamProtocol +from synapse.replication.tcp.resource import ReplicationStreamProtocolFactory + +from tests import unittest +from tests.server import FakeTransport + + +class BaseStreamTestCase(unittest.HomeserverTestCase): + """Base class for tests of the replication streams""" + def prepare(self, reactor, clock, hs): + # build a replication server + server_factory = ReplicationStreamProtocolFactory(self.hs) + self.streamer = server_factory.streamer + server = server_factory.buildProtocol(None) + + # build a replication client, with a dummy handler + self.test_handler = TestReplicationClientHandler() + self.client = ClientReplicationStreamProtocol( + "client", "test", clock, self.test_handler + ) + + # wire them together + self.client.makeConnection(FakeTransport(server, reactor)) + server.makeConnection(FakeTransport(self.client, reactor)) + + def replicate(self): + """Tell the master side of replication that something has happened, and then + wait for the replication to occur. + """ + self.streamer.on_notifier_poke() + self.pump(0.1) + + def replicate_stream(self, stream, token="NOW"): + """Make the client end a REPLICATE command to set up a subscription to a stream""" + self.client.send_command(ReplicateCommand(stream, token)) + + +class TestReplicationClientHandler(object): + """Drop-in for ReplicationClientHandler which just collects RDATA rows""" + def __init__(self): + self.received_rdata_rows = [] + + def get_streams_to_replicate(self): + return {} + + def get_currently_syncing_users(self): + return [] + + def update_connection(self, connection): + pass + + def finished_connecting(self): + pass + + def on_rdata(self, stream_name, token, rows): + for r in rows: + self.received_rdata_rows.append( + (stream_name, token, r) + ) diff --git a/tests/replication/tcp/streams/test_receipts.py b/tests/replication/tcp/streams/test_receipts.py new file mode 100644 index 0000000000..9aa9dfe82e --- /dev/null +++ b/tests/replication/tcp/streams/test_receipts.py @@ -0,0 +1,46 @@ +# -*- coding: utf-8 -*- +# 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. +from synapse.replication.tcp.streams import ReceiptsStreamRow + +from tests.replication.tcp.streams._base import BaseStreamTestCase + +USER_ID = "@feeling:blue" +ROOM_ID = "!room:blue" +EVENT_ID = "$event:blue" + + +class ReceiptsStreamTestCase(BaseStreamTestCase): + def test_receipt(self): + # make the client subscribe to the receipts stream + self.replicate_stream("receipts", "NOW") + + # tell the master to send a new receipt + self.get_success( + self.hs.get_datastore().insert_receipt( + ROOM_ID, "m.read", USER_ID, [EVENT_ID], {"a": 1} + ) + ) + self.replicate() + + # there should be one RDATA command + rdata_rows = self.test_handler.received_rdata_rows + self.assertEqual(1, len(rdata_rows)) + self.assertEqual(rdata_rows[0][0], "receipts") + row = rdata_rows[0][2] # type: ReceiptsStreamRow + self.assertEqual(ROOM_ID, row.room_id) + self.assertEqual("m.read", row.receipt_type) + self.assertEqual(USER_ID, row.user_id) + self.assertEqual(EVENT_ID, row.event_id) + self.assertEqual({"a": 1}, row.data) -- cgit 1.5.1 From 8cbbedaa2b459ce17a294535b434ad808963bd8f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 25 Mar 2019 16:41:51 +0000 Subject: Fix ClientReplicationStreamProtocol.__str__ (#4929) `__str__` depended on `self.addr`, which was absent from ClientReplicationStreamProtocol, so attempting to call str on such an object would raise an exception. We can calculate the peer addr from the transport, so there is no need for addr anyway. --- changelog.d/4929.misc | 1 + synapse/replication/tcp/protocol.py | 8 +++++--- synapse/replication/tcp/resource.py | 1 - 3 files changed, 6 insertions(+), 4 deletions(-) create mode 100644 changelog.d/4929.misc (limited to 'synapse') diff --git a/changelog.d/4929.misc b/changelog.d/4929.misc new file mode 100644 index 0000000000..aaf02078b9 --- /dev/null +++ b/changelog.d/4929.misc @@ -0,0 +1 @@ +Fix `ClientReplicationStreamProtocol.__str__()`. diff --git a/synapse/replication/tcp/protocol.py b/synapse/replication/tcp/protocol.py index e16fad5261..02e5bf6cc8 100644 --- a/synapse/replication/tcp/protocol.py +++ b/synapse/replication/tcp/protocol.py @@ -375,8 +375,11 @@ class BaseReplicationStreamProtocol(LineOnlyReceiver): self.transport.unregisterProducer() def __str__(self): + addr = None + if self.transport: + addr = str(self.transport.getPeer()) return "ReplicationConnection" % ( - self.name, self.conn_id, self.addr, + self.name, self.conn_id, addr, ) def id(self): @@ -392,12 +395,11 @@ class ServerReplicationStreamProtocol(BaseReplicationStreamProtocol): VALID_INBOUND_COMMANDS = VALID_CLIENT_COMMANDS VALID_OUTBOUND_COMMANDS = VALID_SERVER_COMMANDS - def __init__(self, server_name, clock, streamer, addr): + def __init__(self, server_name, clock, streamer): BaseReplicationStreamProtocol.__init__(self, clock) # Old style class self.server_name = server_name self.streamer = streamer - self.addr = addr # The streams the client has subscribed to and is up to date with self.replication_streams = set() diff --git a/synapse/replication/tcp/resource.py b/synapse/replication/tcp/resource.py index 47cdf30bd3..7fc346c7b6 100644 --- a/synapse/replication/tcp/resource.py +++ b/synapse/replication/tcp/resource.py @@ -57,7 +57,6 @@ class ReplicationStreamProtocolFactory(Factory): self.server_name, self.clock, self.streamer, - addr ) -- cgit 1.5.1 From 4a125be13815548762f0d4cf63e7913a09314197 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 26 Mar 2019 11:35:29 +0000 Subject: Make federation endpoints more tolerant of trailing slashes v2 (#4935) Redo of https://github.com/matrix-org/synapse/pull/4840 --- changelog.d/4793.feature | 1 + synapse/federation/transport/client.py | 2 +- synapse/federation/transport/server.py | 14 +++++++------- synapse/http/matrixfederationclient.py | 1 + tests/handlers/test_typing.py | 6 +++--- 5 files changed, 13 insertions(+), 11 deletions(-) create mode 100644 changelog.d/4793.feature (limited to 'synapse') diff --git a/changelog.d/4793.feature b/changelog.d/4793.feature new file mode 100644 index 0000000000..90dba7d122 --- /dev/null +++ b/changelog.d/4793.feature @@ -0,0 +1 @@ +Synapse is now permissive about trailing slashes on some of its federation endpoints, allowing zero or more to be present. \ No newline at end of file diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 0cdb31178f..e424c40fdf 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -173,7 +173,7 @@ class TransportLayerClient(object): # generated by the json_data_callback. json_data = transaction.get_dict() - path = _create_v1_path("/send/%s/", transaction.transaction_id) + path = _create_v1_path("/send/%s", transaction.transaction_id) response = yield self.client.put_json( transaction.destination, diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 96d680a5ad..efb6bdca48 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -312,7 +312,7 @@ class BaseFederationServlet(object): class FederationSendServlet(BaseFederationServlet): - PATH = "/send/(?P[^/]*)/" + PATH = "/send/(?P[^/]*)/?" def __init__(self, handler, server_name, **kwargs): super(FederationSendServlet, self).__init__( @@ -378,7 +378,7 @@ class FederationSendServlet(BaseFederationServlet): class FederationEventServlet(BaseFederationServlet): - PATH = "/event/(?P[^/]*)/" + PATH = "/event/(?P[^/]*)/?" # This is when someone asks for a data item for a given server data_id pair. def on_GET(self, origin, content, query, event_id): @@ -386,7 +386,7 @@ class FederationEventServlet(BaseFederationServlet): class FederationStateServlet(BaseFederationServlet): - PATH = "/state/(?P[^/]*)/" + PATH = "/state/(?P[^/]*)/?" # This is when someone asks for all data for a given context. def on_GET(self, origin, content, query, context): @@ -398,7 +398,7 @@ class FederationStateServlet(BaseFederationServlet): class FederationStateIdsServlet(BaseFederationServlet): - PATH = "/state_ids/(?P[^/]*)/" + PATH = "/state_ids/(?P[^/]*)/?" def on_GET(self, origin, content, query, room_id): return self.handler.on_state_ids_request( @@ -409,7 +409,7 @@ class FederationStateIdsServlet(BaseFederationServlet): class FederationBackfillServlet(BaseFederationServlet): - PATH = "/backfill/(?P[^/]*)/" + PATH = "/backfill/(?P[^/]*)/?" def on_GET(self, origin, content, query, context): versions = [x.decode('ascii') for x in query[b"v"]] @@ -1080,7 +1080,7 @@ class FederationGroupsCategoriesServlet(BaseFederationServlet): """Get all categories for a group """ PATH = ( - "/groups/(?P[^/]*)/categories/" + "/groups/(?P[^/]*)/categories/?" ) @defer.inlineCallbacks @@ -1150,7 +1150,7 @@ class FederationGroupsRolesServlet(BaseFederationServlet): """Get roles in a group """ PATH = ( - "/groups/(?P[^/]*)/roles/" + "/groups/(?P[^/]*)/roles/?" ) @defer.inlineCallbacks diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8e855d13d6..ff63d0b2a8 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -231,6 +231,7 @@ class MatrixFederationHttpClient(object): # Retry with a trailing slash if we received a 400 with # 'M_UNRECOGNIZED' which some endpoints can return when omitting a # trailing slash on Synapse <= v0.99.3. + logger.info("Retrying request with trailing slash") request.path += "/" response = yield self._send_request( diff --git a/tests/handlers/test_typing.py b/tests/handlers/test_typing.py index 7decb22933..6460cbc708 100644 --- a/tests/handlers/test_typing.py +++ b/tests/handlers/test_typing.py @@ -180,7 +180,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", - path="/_matrix/federation/v1/send/1000000/", + path="/_matrix/federation/v1/send/1000000", data=_expect_edu_transaction( "m.typing", content={ @@ -202,7 +202,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): (request, channel) = self.make_request( "PUT", - "/_matrix/federation/v1/send/1000000/", + "/_matrix/federation/v1/send/1000000", _make_edu_transaction_json( "m.typing", content={ @@ -258,7 +258,7 @@ class TypingNotificationsTestCase(unittest.HomeserverTestCase): put_json = self.hs.get_http_client().put_json put_json.assert_called_once_with( "farm", - path="/_matrix/federation/v1/send/1000000/", + path="/_matrix/federation/v1/send/1000000", data=_expect_edu_transaction( "m.typing", content={ -- cgit 1.5.1 From 903f04c21fec3bbc1337bc9552f223e17b87f6bf Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 27 Mar 2019 02:49:28 +1100 Subject: Use the state event amount for userdir import batching, not room count (#4944) --- changelog.d/4944.feature | 1 + synapse/storage/user_directory.py | 28 ++++++++++++++++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) create mode 100644 changelog.d/4944.feature (limited to 'synapse') diff --git a/changelog.d/4944.feature b/changelog.d/4944.feature new file mode 100644 index 0000000000..8f792b8890 --- /dev/null +++ b/changelog.d/4944.feature @@ -0,0 +1 @@ +The user directory has been rewritten to make it faster, with less chance of falling behind on a large server. diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 65bdb1b4a5..4d60a5726f 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -135,7 +135,12 @@ class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): @defer.inlineCallbacks def _populate_user_directory_process_rooms(self, progress, batch_size): - + """ + Args: + progress (dict) + batch_size (int): Maximum number of state events to process + per cycle. + """ state = self.hs.get_state_handler() # If we don't have progress filed, delete everything. @@ -143,13 +148,14 @@ class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): yield self.delete_all_from_user_dir() def _get_next_batch(txn): + # Only fetch 250 rooms, so we don't fetch too many at once, even + # if those 250 rooms have less than batch_size state events. sql = """ - SELECT room_id FROM %s + SELECT room_id, events FROM %s ORDER BY events DESC - LIMIT %s + LIMIT 250 """ % ( TEMP_TABLE + "_rooms", - str(batch_size), ) txn.execute(sql) rooms_to_work_on = txn.fetchall() @@ -157,8 +163,6 @@ class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): if not rooms_to_work_on: return None - rooms_to_work_on = [x[0] for x in rooms_to_work_on] - # Get how many are left to process, so we can give status on how # far we are in processing txn.execute("SELECT COUNT(*) FROM " + TEMP_TABLE + "_rooms") @@ -180,7 +184,9 @@ class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): % (len(rooms_to_work_on), progress["remaining"]) ) - for room_id in rooms_to_work_on: + processed_event_count = 0 + + for room_id, event_count in rooms_to_work_on: is_in_room = yield self.is_host_joined(room_id, self.server_name) if is_in_room: @@ -247,7 +253,13 @@ class UserDirectoryStore(StateDeltasStore, BackgroundUpdateStore): progress, ) - defer.returnValue(len(rooms_to_work_on)) + processed_event_count += event_count + + if processed_event_count > batch_size: + # Don't process any more rooms, we've hit our batch size. + defer.returnValue(processed_event_count) + + defer.returnValue(processed_event_count) @defer.inlineCallbacks def _populate_user_directory_process_users(self, progress, batch_size): -- cgit 1.5.1 From bbd244c7b202319f7642f151e099761024327fa2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 26 Mar 2019 17:48:30 +0000 Subject: Support 3PID login in password providers (#4931) Adds a new method, check_3pid_auth, which gives password providers the chance to allow authentication with third-party identifiers such as email or msisdn. --- changelog.d/4931.feature | 1 + docs/password_auth_providers.rst | 14 ++++++++++++ synapse/api/auth.py | 22 +++++++++--------- synapse/handlers/auth.py | 39 +++++++++++++++++++++++++++++++- synapse/handlers/profile.py | 10 ++++++-- synapse/handlers/register.py | 10 ++++---- synapse/module_api/__init__.py | 18 ++++++++++++--- synapse/rest/client/v1/login.py | 49 ++++++++++++++++++++++++++++++++++++---- 8 files changed, 137 insertions(+), 26 deletions(-) create mode 100644 changelog.d/4931.feature (limited to 'synapse') diff --git a/changelog.d/4931.feature b/changelog.d/4931.feature new file mode 100644 index 0000000000..5d34d16800 --- /dev/null +++ b/changelog.d/4931.feature @@ -0,0 +1 @@ +Add ability for password providers to login/register a user via 3PID (email, phone). \ No newline at end of file diff --git a/docs/password_auth_providers.rst b/docs/password_auth_providers.rst index d8a7b61cdc..6149ba7458 100644 --- a/docs/password_auth_providers.rst +++ b/docs/password_auth_providers.rst @@ -75,6 +75,20 @@ Password auth provider classes may optionally provide the following methods. result from the ``/login`` call (including ``access_token``, ``device_id``, etc.) +``someprovider.check_3pid_auth``\(*medium*, *address*, *password*) + + This method, if implemented, is called when a user attempts to register or + log in with a third party identifier, such as email. It is passed the + medium (ex. "email"), an address (ex. "jdoe@example.com") and the user's + password. + + The method should return a Twisted ``Deferred`` object, which resolves to + a ``str`` containing the user's (canonical) User ID if authentication was + successful, and ``None`` if not. + + As with ``check_auth``, the ``Deferred`` may alternatively resolve to a + ``(user_id, callback)`` tuple. + ``someprovider.check_password``\(*user_id*, *password*) This method provides a simpler interface than ``get_supported_login_types`` diff --git a/synapse/api/auth.py b/synapse/api/auth.py index ee646a97e8..e8112d5f05 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -621,13 +621,13 @@ class Auth(object): Returns: True if the the sender is allowed to redact the target event if the - target event was created by them. + target event was created by them. False if the sender is allowed to redact the target event with no - further checks. + further checks. Raises: AuthError if the event sender is definitely not allowed to redact - the target event. + the target event. """ return event_auth.check_redaction(room_version, event, auth_events) @@ -743,9 +743,9 @@ class Auth(object): Returns: Deferred[tuple[str, str|None]]: Resolves to the current membership of - the user in the room and the membership event ID of the user. If - the user is not in the room and never has been, then - `(Membership.JOIN, None)` is returned. + the user in the room and the membership event ID of the user. If + the user is not in the room and never has been, then + `(Membership.JOIN, None)` is returned. """ try: @@ -777,13 +777,13 @@ class Auth(object): Args: user_id(str|None): If present, checks for presence against existing - MAU cohort + MAU cohort threepid(dict|None): If present, checks for presence against configured - reserved threepid. Used in cases where the user is trying register - with a MAU blocked server, normally they would be rejected but their - threepid is on the reserved list. user_id and - threepid should never be set at the same time. + reserved threepid. Used in cases where the user is trying register + with a MAU blocked server, normally they would be rejected but their + threepid is on the reserved list. user_id and + threepid should never be set at the same time. """ # Never fail an auth check for the server notices users or support user diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index caad9ae2dd..4544de821d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -745,6 +745,42 @@ class AuthHandler(BaseHandler): errcode=Codes.FORBIDDEN ) + @defer.inlineCallbacks + def check_password_provider_3pid(self, medium, address, password): + """Check if a password provider is able to validate a thirdparty login + + Args: + medium (str): The medium of the 3pid (ex. email). + address (str): The address of the 3pid (ex. jdoe@example.com). + password (str): The password of the user. + + Returns: + Deferred[(str|None, func|None)]: A tuple of `(user_id, + callback)`. If authentication is successful, `user_id` is a `str` + containing the authenticated, canonical user ID. `callback` is + then either a function to be later run after the server has + completed login/registration, or `None`. If authentication was + unsuccessful, `user_id` and `callback` are both `None`. + """ + for provider in self.password_providers: + if hasattr(provider, "check_3pid_auth"): + # This function is able to return a deferred that either + # resolves None, meaning authentication failure, or upon + # success, to a str (which is the user_id) or a tuple of + # (user_id, callback_func), where callback_func should be run + # after we've finished everything else + result = yield provider.check_3pid_auth( + medium, address, password, + ) + if result: + # Check if the return value is a str or a tuple + if isinstance(result, str): + # If it's a str, set callback function to None + result = (result, None) + defer.returnValue(result) + + defer.returnValue((None, None)) + @defer.inlineCallbacks def _check_local_password(self, user_id, password): """Authenticate a user against the local password database. @@ -756,7 +792,8 @@ class AuthHandler(BaseHandler): user_id (unicode): complete @user:id password (unicode): the provided password Returns: - (unicode) the canonical_user_id, or None if unknown user / bad password + Deferred[unicode] the canonical_user_id, or Deferred[None] if + unknown user/bad password Raises: LimitExceededError if the ratelimiter's login requests count for this diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 1dfbde84fd..a65c98ff5c 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -147,8 +147,14 @@ class BaseProfileHandler(BaseHandler): @defer.inlineCallbacks def set_displayname(self, target_user, requester, new_displayname, by_admin=False): - """target_user is the user whose displayname is to be changed; - auth_user is the user attempting to make this change.""" + """Set the displayname of a user + + Args: + target_user (UserID): the user whose displayname is to be changed. + requester (Requester): The user attempting to make this change. + new_displayname (str): The displayname to give this user. + by_admin (bool): Whether this change was made by an administrator. + """ if not self.hs.is_mine(target_user): raise SynapseError(400, "User is not hosted on this Home Server") diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 68f73d3793..58940e0320 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -171,7 +171,7 @@ class RegistrationHandler(BaseHandler): api.constants.UserTypes, or None for a normal user. default_display_name (unicode|None): if set, the new user's displayname will be set to this. Defaults to 'localpart'. - address (str|None): the IP address used to perform the regitration. + address (str|None): the IP address used to perform the registration. Returns: A tuple of (user_id, access_token). Raises: @@ -623,7 +623,7 @@ class RegistrationHandler(BaseHandler): admin (boolean): is an admin user? user_type (str|None): type of user. One of the values from api.constants.UserTypes, or None for a normal user. - address (str|None): the IP address used to perform the regitration. + address (str|None): the IP address used to perform the registration. Returns: Deferred @@ -721,9 +721,9 @@ class RegistrationHandler(BaseHandler): access_token (str|None): The access token of the newly logged in device, or None if `inhibit_login` enabled. bind_email (bool): Whether to bind the email with the identity - server + server. bind_msisdn (bool): Whether to bind the msisdn with the identity - server + server. """ if self.hs.config.worker_app: yield self._post_registration_client( @@ -765,7 +765,7 @@ class RegistrationHandler(BaseHandler): """A user consented to the terms on registration Args: - user_id (str): The user ID that consented + user_id (str): The user ID that consented. consent_version (str): version of the policy the user has consented to. """ diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index fc9a20ff59..235ce8334e 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -73,14 +73,26 @@ class ModuleApi(object): """ return self._auth_handler.check_user_exists(user_id) - def register(self, localpart): - """Registers a new user with given localpart + @defer.inlineCallbacks + def register(self, localpart, displayname=None): + """Registers a new user with given localpart and optional + displayname. + + Args: + localpart (str): The localpart of the new user. + displayname (str|None): The displayname of the new user. If None, + the user's displayname will default to `localpart`. Returns: Deferred: a 2-tuple of (user_id, access_token) """ + # Register the user reg = self.hs.get_registration_handler() - return reg.register(localpart=localpart) + user_id, access_token = yield reg.register( + localpart=localpart, default_display_name=displayname, + ) + + defer.returnValue((user_id, access_token)) @defer.inlineCallbacks def invalidate_access_token(self, access_token): diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index 8d56effbb8..5180e9eaf1 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -201,6 +201,24 @@ class LoginRestServlet(ClientV1RestServlet): # We store all email addreses as lowercase in the DB. # (See add_threepid in synapse/handlers/auth.py) address = address.lower() + + # Check for login providers that support 3pid login types + canonical_user_id, callback_3pid = ( + yield self.auth_handler.check_password_provider_3pid( + medium, + address, + login_submission["password"], + ) + ) + if canonical_user_id: + # Authentication through password provider and 3pid succeeded + result = yield self._register_device_with_callback( + canonical_user_id, login_submission, callback_3pid, + ) + defer.returnValue(result) + + # No password providers were able to handle this 3pid + # Check local store user_id = yield self.hs.get_datastore().get_user_id_by_threepid( medium, address, ) @@ -223,20 +241,43 @@ class LoginRestServlet(ClientV1RestServlet): if "user" not in identifier: raise SynapseError(400, "User identifier is missing 'user' key") - auth_handler = self.auth_handler - canonical_user_id, callback = yield auth_handler.validate_login( + canonical_user_id, callback = yield self.auth_handler.validate_login( identifier["user"], login_submission, ) + result = yield self._register_device_with_callback( + canonical_user_id, login_submission, callback, + ) + defer.returnValue(result) + + @defer.inlineCallbacks + def _register_device_with_callback( + self, + user_id, + login_submission, + callback=None, + ): + """ Registers a device with a given user_id. Optionally run a callback + function after registration has completed. + + Args: + user_id (str): ID of the user to register. + login_submission (dict): Dictionary of login information. + callback (func|None): Callback function to run after registration. + + Returns: + result (Dict[str,str]): Dictionary of account information after + successful registration. + """ device_id = login_submission.get("device_id") initial_display_name = login_submission.get("initial_device_display_name") device_id, access_token = yield self.registration_handler.register_device( - canonical_user_id, device_id, initial_display_name, + user_id, device_id, initial_display_name, ) result = { - "user_id": canonical_user_id, + "user_id": user_id, "access_token": access_token, "home_server": self.hs.hostname, "device_id": device_id, -- cgit 1.5.1 From 4aa914369b966a76caa2724fbeee78b0336efdaa Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 27 Mar 2019 10:23:03 +0000 Subject: bump version --- synapse/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/__init__.py b/synapse/__init__.py index 25c10244d3..4a3a3ba236 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -27,4 +27,4 @@ try: except ImportError: pass -__version__ = "0.99.2" +__version__ = "0.99.3rc1" -- cgit 1.5.1 From 35442efb758960d72927d8bd698be657eb0e3037 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 1 Apr 2019 12:49:03 +0000 Subject: 0.99.3 --- CHANGES.md | 6 ++++++ debian/changelog | 8 ++++++-- synapse/__init__.py | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) (limited to 'synapse') diff --git a/CHANGES.md b/CHANGES.md index b13a324037..490c2021e0 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,9 @@ +Synapse 0.99.3 (2019-04-01) +=========================== + +No significant changes. + + Synapse 0.99.3rc1 (2019-03-27) ============================== diff --git a/debian/changelog b/debian/changelog index d84931ec03..03df2e1c00 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,8 +1,12 @@ -matrix-synapse-py3 (0.99.3) UNRELEASED; urgency=medium +matrix-synapse-py3 (0.99.3) stable; urgency=medium + [ Richard van der Hoff ] * Fix warning during preconfiguration. (Fixes: #4819) - -- Richard van der Hoff Thu, 07 Mar 2019 07:17:00 +0000 + [ Synapse Packaging team ] + * New synapse release 0.99.3. + + -- Synapse Packaging team Mon, 01 Apr 2019 12:48:21 +0000 matrix-synapse-py3 (0.99.2) stable; urgency=medium diff --git a/synapse/__init__.py b/synapse/__init__.py index 4a3a3ba236..6bb5a8b24d 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -27,4 +27,4 @@ try: except ImportError: pass -__version__ = "0.99.3rc1" +__version__ = "0.99.3" -- cgit 1.5.1