From 68f737702b5ce90425cbb77a3ce175225bf72086 Mon Sep 17 00:00:00 2001 From: rnbdsh Date: Sun, 24 Sep 2017 04:26:23 +0200 Subject: [PATCH 01/32] Remove non-existing files, add stop, use synctl Non-existing files, when running the suggested from https://github.com/matrix-org/synapse#configuring-synapse /etc/synapse/log_config.yaml so the --log-config leads to an error /etc/sysconfig/synapse The environment-file or even the /etc/sysconfig does not exist in arch linux Also instead of calling python2 we use synctl, as this seems to be the proper way to start it, and it gives us a more useful error in the systemctl status. And we now allow stop (and therefore restart). --- contrib/systemd/synapse.service | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/systemd/synapse.service b/contrib/systemd/synapse.service index 92d94b9d58..b71be582c6 100644 --- a/contrib/systemd/synapse.service +++ b/contrib/systemd/synapse.service @@ -9,9 +9,9 @@ Description=Synapse Matrix homeserver Type=simple User=synapse Group=synapse -EnvironmentFile=-/etc/sysconfig/synapse WorkingDirectory=/var/lib/synapse -ExecStart=/usr/bin/python2.7 -m synapse.app.homeserver --config-path=/etc/synapse/homeserver.yaml --log-config=/etc/synapse/log_config.yaml +ExecStart=/usr/bin/synctl start /etc/synapse/homeserver.yaml +ExecStop=/usr/bin/synctl stop /etc/synapse/homeserver.yaml [Install] WantedBy=multi-user.target From b68b0ede7a79c4fe012b239201f71a32d1eb7fd2 Mon Sep 17 00:00:00 2001 From: rnbdsh Date: Sun, 24 Sep 2017 04:55:19 +0200 Subject: [PATCH 02/32] Start traditionally, stop synctl Starting with synctl lead to "no config file found" Stopping also leads to some (code=exited, status=1/FAILURE), but at least now we can stop the service. --- contrib/systemd/synapse.service | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/systemd/synapse.service b/contrib/systemd/synapse.service index b71be582c6..3f037055b9 100644 --- a/contrib/systemd/synapse.service +++ b/contrib/systemd/synapse.service @@ -10,8 +10,9 @@ Type=simple User=synapse Group=synapse WorkingDirectory=/var/lib/synapse -ExecStart=/usr/bin/synctl start /etc/synapse/homeserver.yaml +ExecStart=/usr/bin/python2.7 -m synapse.app.homeserver --config-path=/etc/synapse/homeserver.yaml ExecStop=/usr/bin/synctl stop /etc/synapse/homeserver.yaml [Install] WantedBy=multi-user.target + From 75e67b9ee4526bc8e5ffd9251ad0370604db13cb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 28 Sep 2017 15:24:00 +0100 Subject: [PATCH 03/32] Handle SERVFAILs when doing AAAA lookups for federation (#2477) ... to cope with people with broken dnssec setups, mostly --- synapse/http/endpoint.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/synapse/http/endpoint.py b/synapse/http/endpoint.py index 241b17f2cb..a97532162f 100644 --- a/synapse/http/endpoint.py +++ b/synapse/http/endpoint.py @@ -354,16 +354,28 @@ def _get_hosts_for_srv_record(dns_client, host): return res[0] - def eb(res): - res.trap(DNSNameError) - return [] + def eb(res, record_type): + if res.check(DNSNameError): + return [] + logger.warn("Error looking up %s for %s: %s", + record_type, host, res, res.value) + return res # no logcontexts here, so we can safely fire these off and gatherResults d1 = dns_client.lookupAddress(host).addCallbacks(cb, eb) d2 = dns_client.lookupIPV6Address(host).addCallbacks(cb, eb) - results = yield defer.gatherResults([d1, d2], consumeErrors=True) + results = yield defer.DeferredList( + [d1, d2], consumeErrors=True) + + # if all of the lookups failed, raise an exception rather than blowing out + # the cache with an empty result. + if results and all(s == defer.FAILURE for (s, _) in results): + defer.returnValue(results[0][1]) + + for (success, result) in results: + if success == defer.FAILURE: + continue - for result in results: for answer in result: if not answer.payload: continue From e43de3ae4b33fb2fad7a4db042f413ecd7448545 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 28 Sep 2017 13:44:47 +0100 Subject: [PATCH 04/32] Improve logging of failures in matrixfederationclient * don't log exception types twice * not all exceptions have a meaningful 'message'. Use the repr rather than attempting to build a string ourselves. --- synapse/http/matrixfederationclient.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 747a791f83..6fc3a41c29 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -204,18 +204,15 @@ class MatrixFederationHttpClient(object): raise logger.warn( - "{%s} Sending request failed to %s: %s %s: %s - %s", + "{%s} Sending request failed to %s: %s %s: %s", txn_id, destination, method, url_bytes, - type(e).__name__, _flatten_response_never_received(e), ) - log_result = "%s - %s" % ( - type(e).__name__, _flatten_response_never_received(e), - ) + log_result = _flatten_response_never_received(e) if retries_left and not timeout: if long_retries: @@ -578,12 +575,14 @@ class _JsonProducer(object): def _flatten_response_never_received(e): if hasattr(e, "reasons"): - return ", ".join( + reasons = ", ".join( _flatten_response_never_received(f.value) for f in e.reasons ) + + return "%s:[%s]" % (type(e).__name__, reasons) else: - return "%s: %s" % (type(e).__name__, e.message,) + return repr(e) def check_content_type_is_json(headers): From d5694ac5fa3266a777fa171f33bebc0d7477c12a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 28 Sep 2017 16:08:08 +0100 Subject: [PATCH 05/32] Only log if we've removed media --- synapse/rest/media/v1/preview_url_resource.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/preview_url_resource.py b/synapse/rest/media/v1/preview_url_resource.py index 2300c263e0..895b480d5c 100644 --- a/synapse/rest/media/v1/preview_url_resource.py +++ b/synapse/rest/media/v1/preview_url_resource.py @@ -365,7 +365,8 @@ class PreviewUrlResource(Resource): yield self.store.delete_url_cache(removed_media) - logger.info("Deleted %d entries from url cache", len(removed_media)) + if removed_media: + logger.info("Deleted %d entries from url cache", len(removed_media)) # Now we delete old images associated with the url cache. # These may be cached for a bit on the client (i.e., they @@ -412,7 +413,8 @@ class PreviewUrlResource(Resource): yield self.store.delete_url_cache_media(removed_media) - logger.info("Deleted %d media from url cache", len(removed_media)) + if removed_media: + logger.info("Deleted %d media from url cache", len(removed_media)) def decode_and_calc_og(body, media_uri, request_encoding=None): From 7fc1aad195b01c4ffe990fc705ff61d128dc0190 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 2 Oct 2017 00:53:32 +0100 Subject: [PATCH 06/32] Drop search values with nul characters https://github.com/matrix-org/synapse/issues/2187 contains a report of a port failing due to nul characters somewhere in the search table. Let's try dropping the offending rows. --- scripts/synapse_port_db | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/synapse_port_db b/scripts/synapse_port_db index bc167b59af..dc7fe940e8 100755 --- a/scripts/synapse_port_db +++ b/scripts/synapse_port_db @@ -376,10 +376,13 @@ class Porter(object): " VALUES (?,?,?,?,to_tsvector('english', ?),?,?)" ) - rows_dict = [ - dict(zip(headers, row)) - for row in rows - ] + rows_dict = [] + for row in rows: + d = dict(zip(headers, row)) + if "\0" in d['value']: + logger.warn('dropping search row %s', d) + else: + rows_dict.append(d) txn.executemany(sql, [ ( From e4a709eda3a21de41a2e6921674bb65b89f212a2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Oct 2017 13:51:38 +0100 Subject: [PATCH 07/32] Bump version and change log --- CHANGES.rst | 6 ++++++ synapse/__init__.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 6291fedb9a..4be6604ddd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,3 +1,9 @@ +Changes in synapse v0.23.0 (2017-10-02) +======================================= + +No changes since v0.23.0-rc2 + + Changes in synapse v0.23.0-rc2 (2017-09-26) =========================================== diff --git a/synapse/__init__.py b/synapse/__init__.py index ec83e6adb7..97d6c4094d 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -16,4 +16,4 @@ """ This is a reference implementation of a Matrix home server. """ -__version__ = "0.23.0-rc2" +__version__ = "0.23.0" From 3fed5bb25f92586a6467494d4673627abcd25665 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Oct 2017 17:59:34 +0100 Subject: [PATCH 08/32] Move quit_with_error --- synapse/app/_base.py | 10 ++++++++++ synapse/app/homeserver.py | 11 +---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index cd0e815919..e1ff8f9b7c 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -97,3 +97,13 @@ def start_reactor( daemon.start() else: run() + + +def quit_with_error(error_string): + message_lines = error_string.split("\n") + line_length = max([len(l) for l in message_lines if len(l) < 80]) + 2 + sys.stderr.write("*" * line_length + '\n') + for line in message_lines: + sys.stderr.write(" %s\n" % (line.rstrip(),)) + sys.stderr.write("*" * line_length + '\n') + sys.exit(1) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 84ad8f04a0..3adf72e141 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -25,6 +25,7 @@ from synapse.api.urls import CONTENT_REPO_PREFIX, FEDERATION_PREFIX, \ LEGACY_MEDIA_PREFIX, MEDIA_PREFIX, SERVER_KEY_PREFIX, SERVER_KEY_V2_PREFIX, \ STATIC_PREFIX, WEB_CLIENT_PREFIX from synapse.app import _base +from synapse.app._base import quit_with_error from synapse.config._base import ConfigError from synapse.config.homeserver import HomeServerConfig from synapse.crypto import context_factory @@ -249,16 +250,6 @@ class SynapseHomeServer(HomeServer): return db_conn -def quit_with_error(error_string): - message_lines = error_string.split("\n") - line_length = max([len(l) for l in message_lines if len(l) < 80]) + 2 - sys.stderr.write("*" * line_length + '\n') - for line in message_lines: - sys.stderr.write(" %s\n" % (line.rstrip(),)) - sys.stderr.write("*" * line_length + '\n') - sys.exit(1) - - def setup(config_options): """ Args: From ea87cb1ba5f0a2614043be6f4499cfe842b9b8eb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Oct 2017 18:03:59 +0100 Subject: [PATCH 09/32] Make 'affinity' package optional --- synapse/app/_base.py | 15 ++++++++++++++- synapse/python_dependencies.py | 4 +++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index e1ff8f9b7c..cf4730730d 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -12,10 +12,16 @@ # 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 gc import logging +import sys + +try: + import affinity +except: + affinity = None -import affinity from daemonize import Daemonize from synapse.util import PreserveLoggingContext from synapse.util.rlimit import change_resource_limit @@ -78,6 +84,13 @@ def start_reactor( with PreserveLoggingContext(): logger.info("Running") if cpu_affinity is not None: + if not affinity: + quit_with_error( + "Missing package 'affinity' required for cpu_affinity\n" + "option\n\n" + "Install by running:\n\n" + " pip install affinity\n\n" + ) logger.info("Setting CPU affinity to %s" % cpu_affinity) affinity.set_process_affinity_mask(0, cpu_affinity) change_resource_limit(soft_file_limit) diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index 630e92c90e..7052333c19 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -40,7 +40,6 @@ REQUIREMENTS = { "pymacaroons-pynacl": ["pymacaroons"], "msgpack-python>=0.3.0": ["msgpack"], "phonenumbers>=8.2.0": ["phonenumbers"], - "affinity": ["affinity"], } CONDITIONAL_REQUIREMENTS = { "web_client": { @@ -59,6 +58,9 @@ CONDITIONAL_REQUIREMENTS = { "psutil": { "psutil>=2.0.0": ["psutil>=2.0.0"], }, + "affinity": { + "affinity": ["affinity"], + }, } From 6c1bb1601e43c89637ae5bd8720c255646ca8141 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Oct 2017 18:05:17 +0100 Subject: [PATCH 10/32] Bump version and changelog --- CHANGES.rst | 8 ++++++++ synapse/__init__.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 4be6604ddd..f1529e79bd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,3 +1,11 @@ +Changes in synapse v0.23.1 (2017-10-02) +======================================= + +Changes: + +* Make 'affinity' package optional, as it is not supported on some platforms + + Changes in synapse v0.23.0 (2017-10-02) ======================================= diff --git a/synapse/__init__.py b/synapse/__init__.py index 97d6c4094d..bee4aba625 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -16,4 +16,4 @@ """ This is a reference implementation of a Matrix home server. """ -__version__ = "0.23.0" +__version__ = "0.23.1" From 84716d267c6d93cfe759e8da336efb3136dc1560 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 13:53:09 +0100 Subject: [PATCH 11/32] Allow spam checker to reject invites too --- synapse/handlers/federation.py | 4 ++++ synapse/handlers/room_member.py | 20 ++++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 18f87cad67..32078fde3c 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -77,6 +77,7 @@ class FederationHandler(BaseHandler): self.action_generator = hs.get_action_generator() self.is_mine_id = hs.is_mine_id self.pusher_pool = hs.get_pusherpool() + self.spam_checker = hs.get_spam_checker() self.replication_layer.set_handler(self) @@ -1077,6 +1078,9 @@ class FederationHandler(BaseHandler): if self.hs.config.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") + if not self.spam_checker.user_may_invite(requester.user): + raise SynapseError(403, "This user is not permitted to send invites to this server") + membership = event.content.get("membership") if event.type != EventTypes.Member or membership != Membership.INVITE: raise SynapseError(400, "The event was not an m.room.member invite event") diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 9a498c2d3e..61b0140e69 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -48,6 +48,7 @@ class RoomMemberHandler(BaseHandler): self.member_linearizer = Linearizer(name="member") self.clock = hs.get_clock() + self.spam_checker = hs.get_spam_checker() self.distributor = hs.get_distributor() self.distributor.declare("user_joined_room") @@ -210,12 +211,19 @@ class RoomMemberHandler(BaseHandler): if is_blocked: raise SynapseError(403, "This room has been blocked on this server") - if (effective_membership_state == "invite" and - self.hs.config.block_non_admin_invites): - is_requester_admin = yield self.auth.is_server_admin( - requester.user, - ) - if not is_requester_admin: + if effective_membership_state == "invite": + block_invite = False + if self.hs.config.block_non_admin_invites: + is_requester_admin = yield self.auth.is_server_admin( + requester.user, + ) + if not is_requester_admin: + block_invite = True + + if not self.spam_checker.user_may_invite(requester.user): + block_invite = True + + if block_invite: raise SynapseError( 403, "Invites have been disabled on this server", ) From 2a7ed700d51f0a81f563298c78cd4566994ddbab Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 14:04:10 +0100 Subject: [PATCH 12/32] Fix param name & lint --- synapse/handlers/federation.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 32078fde3c..8571350cc8 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1078,8 +1078,10 @@ class FederationHandler(BaseHandler): if self.hs.config.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") - if not self.spam_checker.user_may_invite(requester.user): - raise SynapseError(403, "This user is not permitted to send invites to this server") + if not self.spam_checker.user_may_invite(event.sender): + raise SynapseError( + 403, "This user is not permitted to send invites to this server" + ) membership = event.content.get("membership") if event.type != EventTypes.Member or membership != Membership.INVITE: From 41fd9989a28cfd6cc0b401677be61270f3959cfa Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 14:17:44 +0100 Subject: [PATCH 13/32] Skip spam check for admin users --- synapse/handlers/room_member.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 61b0140e69..e88ba0e3a6 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -213,16 +213,16 @@ class RoomMemberHandler(BaseHandler): if effective_membership_state == "invite": block_invite = False - if self.hs.config.block_non_admin_invites: - is_requester_admin = yield self.auth.is_server_admin( - requester.user, - ) - if not is_requester_admin: + is_requester_admin = yield self.auth.is_server_admin( + requester.user, + ) + if not is_requester_admin: + if ( + self.hs.config.block_non_admin_invites or + not self.spam_checker.user_may_invite(requester.user) + ): block_invite = True - if not self.spam_checker.user_may_invite(requester.user): - block_invite = True - if block_invite: raise SynapseError( 403, "Invites have been disabled on this server", From 537088e7dceff8af4b283e11e46d7df7e2f38065 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 14:28:12 +0100 Subject: [PATCH 14/32] Actually write warpper function --- synapse/events/spamcheck.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index e739f105b2..605261f4b5 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -45,3 +45,19 @@ class SpamChecker(object): return False return self.spam_checker.check_event_for_spam(event) + + def user_may_invite(self, userid): + """Checks if a given user may send an invite + + If this method returns false, the invite will be rejected. + + Args: + userid (string): The sender's user ID + + Returns: + bool: True if the user may send an invite, otherwise False + """ + if self.spam_checker is None: + return True + + return self.spam_checker.user_may_invite(userid) From bd769a81e12ea710accdebbaa296db1c1a625f75 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 15:16:40 +0100 Subject: [PATCH 15/32] better logging --- synapse/handlers/room_member.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index e88ba0e3a6..76e46d93fe 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -217,10 +217,15 @@ class RoomMemberHandler(BaseHandler): requester.user, ) if not is_requester_admin: - if ( - self.hs.config.block_non_admin_invites or - not self.spam_checker.user_may_invite(requester.user) - ): + if self.hs.config.block_non_admin_invites: + logger.debug( + "Blocking invite: user is not admin and non-admin " + "invites disabled" + ) + block_invite = True + + if not self.spam_checker.user_may_invite(requester.user): + logger.debug("Blocking invite due to spam checker") block_invite = True if block_invite: From c46a0d7eb4704c6532a611040a591633dac02b1a Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 15:20:14 +0100 Subject: [PATCH 16/32] this shouldn't be debug --- synapse/handlers/room_member.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 76e46d93fe..77e5b95e8a 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -218,14 +218,14 @@ class RoomMemberHandler(BaseHandler): ) if not is_requester_admin: if self.hs.config.block_non_admin_invites: - logger.debug( + logger.info( "Blocking invite: user is not admin and non-admin " "invites disabled" ) block_invite = True if not self.spam_checker.user_may_invite(requester.user): - logger.debug("Blocking invite due to spam checker") + logger.info("Blocking invite due to spam checker") block_invite = True if block_invite: From c2c188b699e555376912dfea49c42b02c4168270 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 15:46:19 +0100 Subject: [PATCH 17/32] Federation was passing strings anyway so pass string everywhere --- synapse/handlers/room_member.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 77e5b95e8a..a33a8ad42b 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -224,7 +224,7 @@ class RoomMemberHandler(BaseHandler): ) block_invite = True - if not self.spam_checker.user_may_invite(requester.user): + if not self.spam_checker.user_may_invite(requester.user.to_string()): logger.info("Blocking invite due to spam checker") block_invite = True From 1e375468de914fdefc7c0b4b65217c4ec95784a4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 17:13:14 +0100 Subject: [PATCH 18/32] pass room id too --- synapse/events/spamcheck.py | 4 ++-- synapse/handlers/federation.py | 2 +- synapse/handlers/room_member.py | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 605261f4b5..fe2d22a6f2 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -46,7 +46,7 @@ class SpamChecker(object): return self.spam_checker.check_event_for_spam(event) - def user_may_invite(self, userid): + def user_may_invite(self, userid, roomid): """Checks if a given user may send an invite If this method returns false, the invite will be rejected. @@ -60,4 +60,4 @@ class SpamChecker(object): if self.spam_checker is None: return True - return self.spam_checker.user_may_invite(userid) + return self.spam_checker.user_may_invite(userid, roomid) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 8571350cc8..737fe518ef 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1078,7 +1078,7 @@ class FederationHandler(BaseHandler): if self.hs.config.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") - if not self.spam_checker.user_may_invite(event.sender): + if not self.spam_checker.user_may_invite(event.sender, event.room_id): raise SynapseError( 403, "This user is not permitted to send invites to this server" ) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index a33a8ad42b..37985fa1f9 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -224,7 +224,9 @@ class RoomMemberHandler(BaseHandler): ) block_invite = True - if not self.spam_checker.user_may_invite(requester.user.to_string()): + if not self.spam_checker.user_may_invite( + requester.user.to_string(), room_id, + ): logger.info("Blocking invite due to spam checker") block_invite = True From 1e2ac543516baaec06d4e0ebdd2dbbe003e1f73b Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Oct 2017 17:41:38 +0100 Subject: [PATCH 19/32] s/roomid/room_id/ --- synapse/events/spamcheck.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index fe2d22a6f2..8b01c091e9 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -46,7 +46,7 @@ class SpamChecker(object): return self.spam_checker.check_event_for_spam(event) - def user_may_invite(self, userid, roomid): + def user_may_invite(self, userid, room_id): """Checks if a given user may send an invite If this method returns false, the invite will be rejected. @@ -60,4 +60,4 @@ class SpamChecker(object): if self.spam_checker is None: return True - return self.spam_checker.user_may_invite(userid, roomid) + return self.spam_checker.user_may_invite(userid, room_id) From 197c14dbcfa9bc5bb281833a91ee035cb154216d Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Oct 2017 10:47:54 +0100 Subject: [PATCH 20/32] Add room creation checks to spam checker Lets the spam checker deny attempts to create rooms and add aliases to them. --- synapse/events/spamcheck.py | 32 ++++++++++++++++++++++++++++++++ synapse/handlers/directory.py | 7 +++++++ synapse/handlers/room.py | 8 ++++++++ 3 files changed, 47 insertions(+) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 8b01c091e9..7cb3468df4 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -61,3 +61,35 @@ class SpamChecker(object): return True return self.spam_checker.user_may_invite(userid, room_id) + + def user_may_create_room(self, userid): + """Checks if a given user may create a room + + If this method returns false, the creation request will be rejected. + + Args: + userid (string): The sender's user ID + + Returns: + bool: True if the user may create a room, otherwise False + """ + if self.spam_checker is None: + return True + + return self.spam_checker.user_may_create_room(userid) + + def user_may_create_room_alias(self, userid, room_alias): + """Checks if a given user may create a room alias + + If this method returns false, the association request will be rejected. + + Args: + userid (string): The sender's user ID + + Returns: + bool: True if the user may create a room alias, otherwise False + """ + if self.spam_checker is None: + return True + + return self.spam_checker.user_may_create_room_alias(userid, room_alias) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 943554ce98..ed18bb20bb 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -40,6 +40,8 @@ class DirectoryHandler(BaseHandler): "directory", self.on_directory_query ) + self.spam_checker = hs.get_spam_checker() + @defer.inlineCallbacks def _create_association(self, room_alias, room_id, servers=None, creator=None): # general association creation for both human users and app services @@ -73,6 +75,11 @@ class DirectoryHandler(BaseHandler): # association creation for human users # TODO(erikj): Do user auth. + if not self.spam_checker.user_may_create_room_alias(user_id, room_alias): + raise SynapseError( + 403, "This user is not permitted to create this alias", + ) + can_create = yield self.can_modify_alias( room_alias, user_id=user_id diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 5698d28088..f909ea04f0 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -60,6 +60,11 @@ class RoomCreationHandler(BaseHandler): }, } + def __init__(self, hs): + super(RoomCreationHandler, self).__init__(hs) + + self.spam_checker = hs.get_spam_checker() + @defer.inlineCallbacks def create_room(self, requester, config, ratelimit=True): """ Creates a new room. @@ -75,6 +80,9 @@ class RoomCreationHandler(BaseHandler): """ user_id = requester.user.to_string() + if not self.spam_checker.user_may_create_room(user_id): + raise SynapseError(403, "You are not permitted to create rooms") + if ratelimit: yield self.ratelimit(requester) From 78d4ced82941ba249b7b16ea72684ade69c6a0d2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Oct 2017 12:44:27 +0100 Subject: [PATCH 21/32] un-double indent --- synapse/handlers/room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index f909ea04f0..535ba9517c 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -81,7 +81,7 @@ class RoomCreationHandler(BaseHandler): user_id = requester.user.to_string() if not self.spam_checker.user_may_create_room(user_id): - raise SynapseError(403, "You are not permitted to create rooms") + raise SynapseError(403, "You are not permitted to create rooms") if ratelimit: yield self.ratelimit(requester) From d8ce68b09b0966330b4da720eeb41719c7c61be6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Oct 2017 14:29:33 +0100 Subject: [PATCH 22/32] spam check room publishing --- synapse/events/spamcheck.py | 18 ++++++++++++++++++ synapse/handlers/directory.py | 8 ++++++++ 2 files changed, 26 insertions(+) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 7cb3468df4..595b1760f8 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -85,6 +85,7 @@ class SpamChecker(object): Args: userid (string): The sender's user ID + room_alias (string): The alias to be created Returns: bool: True if the user may create a room alias, otherwise False @@ -93,3 +94,20 @@ class SpamChecker(object): return True return self.spam_checker.user_may_create_room_alias(userid, room_alias) + + def user_may_publish_room(self, userid, room_id): + """Checks if a given user may publish a room to the directory + + If this method returns false, the publish request will be rejected. + + Args: + userid (string): The sender's user ID + room_id (string): The ID of the room that would be published + + Returns: + bool: True if the user may publish the room, otherwise False + """ + if self.spam_checker is None: + return True + + return self.spam_checker.user_may_publish_room(userid, room_id) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index ed18bb20bb..a0464ae5c0 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -334,6 +334,14 @@ class DirectoryHandler(BaseHandler): room_id (str) visibility (str): "public" or "private" """ + if not self.spam_checker.user_may_publish_room( + requester.user.to_string(), room_id + ): + raise AuthError( + 403, + "This user is not permitted to publish rooms to the room list" + ) + if requester.is_guest: raise AuthError(403, "Guests cannot edit the published room list") From 6748f0a57962fb9657cab60083d94b4c97a0526c Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Oct 2017 11:33:30 +0100 Subject: [PATCH 23/32] Fix notif kws that start/end with non-word chars Only prepend / append word bounary characters if the search expression starts or ends with a word character, otherwise they don't work because there's no word bounary between whitespace and a non-word char. --- synapse/push/push_rule_evaluator.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 172c27c137..5a34d60abb 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -26,6 +26,8 @@ logger = logging.getLogger(__name__) GLOB_REGEX = re.compile(r'\\\[(\\\!|)(.*)\\\]') IS_GLOB = re.compile(r'[\?\*\[\]]') INEQUALITY_EXPR = re.compile("^([=<>]*)([0-9]*)$") +STARTS_WITH_WORD_CHAR_REGEX = re.compile(r"^\w") +ENDS_WITH_WORD_CHAR_REGEX = re.compile(r"\w$") def _room_member_count(ev, condition, room_member_count): @@ -183,7 +185,7 @@ def _glob_to_re(glob, word_boundary): r, ) if word_boundary: - r = r"\b%s\b" % (r,) + r = _re_word_boundary(r) return re.compile(r, flags=re.IGNORECASE) else: @@ -192,13 +194,30 @@ def _glob_to_re(glob, word_boundary): return re.compile(r, flags=re.IGNORECASE) elif word_boundary: r = re.escape(glob) - r = r"\b%s\b" % (r,) + r = _re_word_boundary(r) return re.compile(r, flags=re.IGNORECASE) else: r = "^" + re.escape(glob) + "$" return re.compile(r, flags=re.IGNORECASE) +def _re_word_boundary(r): + """ + Adds word boundary characters to the start and end of an + expression to require that the match occur as a whole word, + but do so respecting the fact that strings starting or ending + with non-word characters will change word boundaries. + """ + # Matching a regex string aginst a regex, since by definition + # \b is the boundary between a \w and a \W, so match \w at the + # start or end of the expression (although this will miss, eg. + # "[dl]og") + if STARTS_WITH_WORD_CHAR_REGEX.search(r): + r = r"\b%s" % (r,) + if ENDS_WITH_WORD_CHAR_REGEX.search(r): + r = r"%s\b" % (r,) + return r + def _flatten_dict(d, prefix=[], result=None): if result is None: From cbe3c3fdd49b87a452a9a9a229abfdf8dbe45922 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Oct 2017 11:43:10 +0100 Subject: [PATCH 24/32] pep8 --- synapse/push/push_rule_evaluator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 5a34d60abb..b78f2d90d7 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -201,6 +201,7 @@ def _glob_to_re(glob, word_boundary): r = "^" + re.escape(glob) + "$" return re.compile(r, flags=re.IGNORECASE) + def _re_word_boundary(r): """ Adds word boundary characters to the start and end of an From 0c8da8b519fbd8bca984117e354fe57c3a76e154 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Oct 2017 11:57:43 +0100 Subject: [PATCH 25/32] Use better method for word boundary searching From https://github.com/matrix-org/matrix-js-sdk/commit/ebc95667b8a5777d13e5d3c679972bedae022fd5 --- synapse/push/push_rule_evaluator.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index b78f2d90d7..65f9a63fd8 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -26,8 +26,6 @@ logger = logging.getLogger(__name__) GLOB_REGEX = re.compile(r'\\\[(\\\!|)(.*)\\\]') IS_GLOB = re.compile(r'[\?\*\[\]]') INEQUALITY_EXPR = re.compile("^([=<>]*)([0-9]*)$") -STARTS_WITH_WORD_CHAR_REGEX = re.compile(r"^\w") -ENDS_WITH_WORD_CHAR_REGEX = re.compile(r"\w$") def _room_member_count(ev, condition, room_member_count): @@ -209,15 +207,9 @@ def _re_word_boundary(r): but do so respecting the fact that strings starting or ending with non-word characters will change word boundaries. """ - # Matching a regex string aginst a regex, since by definition - # \b is the boundary between a \w and a \W, so match \w at the - # start or end of the expression (although this will miss, eg. - # "[dl]og") - if STARTS_WITH_WORD_CHAR_REGEX.search(r): - r = r"\b%s" % (r,) - if ENDS_WITH_WORD_CHAR_REGEX.search(r): - r = r"%s\b" % (r,) - return r + # we can't use \b as it chokes on unicode. however \W seems to be okay + # as shorthand for [^0-9A-Za-z_]. + return r"(^|\W)%s(\W|$)" % (r,) def _flatten_dict(d, prefix=[], result=None): From f878e6f8af9e80cfa4be717c03cc4f9853a93794 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 5 Oct 2017 14:02:28 +0100 Subject: [PATCH 26/32] Spam checking: add the invitee to user_may_invite --- synapse/events/spamcheck.py | 4 ++-- synapse/handlers/federation.py | 12 +++++++----- synapse/handlers/room_member.py | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 595b1760f8..dccc579eac 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -46,7 +46,7 @@ class SpamChecker(object): return self.spam_checker.check_event_for_spam(event) - def user_may_invite(self, userid, room_id): + def user_may_invite(self, inviter_userid, invitee_userid, room_id): """Checks if a given user may send an invite If this method returns false, the invite will be rejected. @@ -60,7 +60,7 @@ class SpamChecker(object): if self.spam_checker is None: return True - return self.spam_checker.user_may_invite(userid, room_id) + return self.spam_checker.user_may_invite(inviter_userid, invitee_userid, room_id) def user_may_create_room(self, userid): """Checks if a given user may create a room diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 737fe518ef..8fccf8bab3 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1071,6 +1071,9 @@ class FederationHandler(BaseHandler): """ event = pdu + if event.state_key is None: + raise SynapseError(400, "The invite event did not have a state key") + is_blocked = yield self.store.is_room_blocked(event.room_id) if is_blocked: raise SynapseError(403, "This room has been blocked on this server") @@ -1078,9 +1081,11 @@ class FederationHandler(BaseHandler): if self.hs.config.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") - if not self.spam_checker.user_may_invite(event.sender, event.room_id): + if not self.spam_checker.user_may_invite( + event.sender, event.state_key, event.room_id, + ): raise SynapseError( - 403, "This user is not permitted to send invites to this server" + 403, "This user is not permitted to send invites to this server/user" ) membership = event.content.get("membership") @@ -1091,9 +1096,6 @@ class FederationHandler(BaseHandler): if sender_domain != origin: raise SynapseError(400, "The invite event was not from the server sending it") - if event.state_key is None: - raise SynapseError(400, "The invite event did not have a state key") - if not self.is_mine_id(event.state_key): raise SynapseError(400, "The invite event must be for this server") diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 37985fa1f9..36a8ef8ce0 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -225,7 +225,7 @@ class RoomMemberHandler(BaseHandler): block_invite = True if not self.spam_checker.user_may_invite( - requester.user.to_string(), room_id, + requester.user.to_string(), target.to_string(), room_id, ): logger.info("Blocking invite due to spam checker") block_invite = True From 3ddda939d35951896faa48631a3fe023e89e13e1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 5 Oct 2017 14:58:17 +0100 Subject: [PATCH 27/32] some comments in the state res code --- synapse/state.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/synapse/state.py b/synapse/state.py index 390799fbd5..dcdcdef65e 100644 --- a/synapse/state.py +++ b/synapse/state.py @@ -288,6 +288,9 @@ class StateHandler(object): """ logger.debug("resolve_state_groups event_ids %s", event_ids) + # map from state group id to the state in that state group (where + # 'state' is a map from state key to event id) + # dict[int, dict[(str, str), str]] state_groups_ids = yield self.store.get_state_groups_ids( room_id, event_ids ) @@ -320,11 +323,15 @@ class StateHandler(object): "Resolving state for %s with %d groups", room_id, len(state_groups_ids) ) + # build a map from state key to the event_ids which set that state. + # dict[(str, str), set[str]) state = {} for st in state_groups_ids.values(): for key, e_id in st.items(): state.setdefault(key, set()).add(e_id) + # build a map from state key to the event_ids which set that state, + # including only those where there are state keys in conflict. conflicted_state = { k: list(v) for k, v in state.items() @@ -494,8 +501,14 @@ def _resolve_with_state_fac(unconflicted_state, conflicted_state, logger.info("Asking for %d conflicted events", len(needed_events)) + # dict[str, FrozenEvent]: a map from state event id to event. Only includes + # the state events which are in conflict. state_map = yield state_map_factory(needed_events) + # get the ids of the auth events which allow us to authenticate the + # conflicted state, picking only from the unconflicting state. + # + # dict[(str, str), str]: a map from state key to event id auth_events = _create_auth_events_from_maps( unconflicted_state, conflicted_state, state_map ) From c8f568ddf9e8827d8971e14137663fa8df5b57d2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 6 Oct 2017 22:14:24 +0100 Subject: [PATCH 28/32] Fix up deferred handling in federation.py * Avoid preserve_context_over_deferred, which is broken * set consumeErrors=True on defer.gatherResults, to avoid spurious "unhandled failure" erros --- synapse/handlers/federation.py | 45 ++++++++++++++++------------------ 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 8fccf8bab3..63c56a4a32 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -14,7 +14,6 @@ # limitations under the License. """Contains handlers for federation events.""" -import synapse.util.logcontext from signedjson.key import decode_verify_key_bytes from signedjson.sign import verify_signed_json from unpaddedbase64 import decode_base64 @@ -26,10 +25,7 @@ from synapse.api.errors import ( ) from synapse.api.constants import EventTypes, Membership, RejectedReason from synapse.events.validator import EventValidator -from synapse.util import unwrapFirstError -from synapse.util.logcontext import ( - preserve_fn, preserve_context_over_deferred -) +from synapse.util import unwrapFirstError, logcontext from synapse.util.metrics import measure_func from synapse.util.logutils import log_function from synapse.util.async import run_on_reactor, Linearizer @@ -592,9 +588,9 @@ class FederationHandler(BaseHandler): missing_auth - failed_to_fetch ) - results = yield preserve_context_over_deferred(defer.gatherResults( + results = yield logcontext.make_deferred_yieldable(defer.gatherResults( [ - preserve_fn(self.replication_layer.get_pdu)( + logcontext.preserve_fn(self.replication_layer.get_pdu)( [dest], event_id, outlier=True, @@ -786,10 +782,14 @@ class FederationHandler(BaseHandler): event_ids = list(extremities.keys()) logger.debug("calling resolve_state_groups in _maybe_backfill") - states = yield preserve_context_over_deferred(defer.gatherResults([ - preserve_fn(self.state_handler.resolve_state_groups)(room_id, [e]) - for e in event_ids - ])) + states = yield logcontext.make_deferred_yieldable(defer.gatherResults( + [ + logcontext.preserve_fn(self.state_handler.resolve_state_groups)( + room_id, [e] + ) + for e in event_ids + ], consumeErrors=True, + )) states = dict(zip(event_ids, [s.state for s in states])) state_map = yield self.store.get_events( @@ -942,9 +942,7 @@ class FederationHandler(BaseHandler): # lots of requests for missing prev_events which we do actually # have. Hence we fire off the deferred, but don't wait for it. - synapse.util.logcontext.preserve_fn(self._handle_queued_pdus)( - room_queue - ) + logcontext.preserve_fn(self._handle_queued_pdus)(room_queue) defer.returnValue(True) @@ -1438,7 +1436,7 @@ class FederationHandler(BaseHandler): if not backfilled: # this intentionally does not yield: we don't care about the result # and don't need to wait for it. - preserve_fn(self.pusher_pool.on_new_notifications)( + logcontext.preserve_fn(self.pusher_pool.on_new_notifications)( event_stream_id, max_stream_id ) @@ -1451,16 +1449,16 @@ class FederationHandler(BaseHandler): a bunch of outliers, but not a chunk of individual events that depend on each other for state calculations. """ - contexts = yield preserve_context_over_deferred(defer.gatherResults( + contexts = yield logcontext.make_deferred_yieldable(defer.gatherResults( [ - preserve_fn(self._prep_event)( + logcontext.preserve_fn(self._prep_event)( origin, ev_info["event"], state=ev_info.get("state"), auth_events=ev_info.get("auth_events"), ) for ev_info in event_infos - ] + ], consumeErrors=True, )) yield self.store.persist_events( @@ -1768,18 +1766,17 @@ class FederationHandler(BaseHandler): # Do auth conflict res. logger.info("Different auth: %s", different_auth) - different_events = yield preserve_context_over_deferred(defer.gatherResults( - [ - preserve_fn(self.store.get_event)( + different_events = yield logcontext.make_deferred_yieldable( + defer.gatherResults([ + logcontext.preserve_fn(self.store.get_event)( d, allow_none=True, allow_rejected=False, ) for d in different_auth if d in have_events and not have_events[d] - ], - consumeErrors=True - )).addErrback(unwrapFirstError) + ], consumeErrors=True) + ).addErrback(unwrapFirstError) if different_events: local_view = dict(auth_events) From 148428ce763978583da2b1d3c435ec321df45855 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 6 Oct 2017 22:24:28 +0100 Subject: [PATCH 29/32] Fix logcontext handling for concurrently_execute Avoid preserve_context_over_deferred, which is broken. --- synapse/util/async.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/util/async.py b/synapse/util/async.py index 1453faf0ef..bb252f75d7 100644 --- a/synapse/util/async.py +++ b/synapse/util/async.py @@ -19,7 +19,7 @@ from twisted.internet import defer, reactor from .logcontext import ( PreserveLoggingContext, preserve_fn, preserve_context_over_deferred, ) -from synapse.util import unwrapFirstError +from synapse.util import logcontext, unwrapFirstError from contextlib import contextmanager @@ -155,7 +155,7 @@ def concurrently_execute(func, args, limit): except StopIteration: pass - return preserve_context_over_deferred(defer.gatherResults([ + return logcontext.make_deferred_yieldable(defer.gatherResults([ preserve_fn(_concurrently_execute_inner)() for _ in xrange(limit) ], consumeErrors=True)).addErrback(unwrapFirstError) From 01bbacf3c49f4311fddb61ef1ff98ee4f55fc44b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 6 Oct 2017 15:12:43 +0100 Subject: [PATCH 30/32] Fix up logcontext handling in (federation) TransactionQueue Avoid using preserve_context_over_function, which has problems with respect to logcontexts. --- synapse/federation/transaction_queue.py | 48 ++++++++++++++++--------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/synapse/federation/transaction_queue.py b/synapse/federation/transaction_queue.py index 003eaba893..7a3c9cbb70 100644 --- a/synapse/federation/transaction_queue.py +++ b/synapse/federation/transaction_queue.py @@ -20,8 +20,8 @@ from .persistence import TransactionActions from .units import Transaction, Edu from synapse.api.errors import HttpResponseException +from synapse.util import logcontext from synapse.util.async import run_on_reactor -from synapse.util.logcontext import preserve_context_over_fn, preserve_fn from synapse.util.retryutils import NotRetryingDestination, get_retry_limiter from synapse.util.metrics import measure_func from synapse.handlers.presence import format_user_presence_state, get_interested_remotes @@ -231,11 +231,9 @@ class TransactionQueue(object): (pdu, order) ) - preserve_context_over_fn( - self._attempt_new_transaction, destination - ) + self._attempt_new_transaction(destination) - @preserve_fn # the caller should not yield on this + @logcontext.preserve_fn # the caller should not yield on this @defer.inlineCallbacks def send_presence(self, states): """Send the new presence states to the appropriate destinations. @@ -299,7 +297,7 @@ class TransactionQueue(object): state.user_id: state for state in states }) - preserve_fn(self._attempt_new_transaction)(destination) + self._attempt_new_transaction(destination) def send_edu(self, destination, edu_type, content, key=None): edu = Edu( @@ -321,9 +319,7 @@ class TransactionQueue(object): else: self.pending_edus_by_dest.setdefault(destination, []).append(edu) - preserve_context_over_fn( - self._attempt_new_transaction, destination - ) + self._attempt_new_transaction(destination) def send_failure(self, failure, destination): if destination == self.server_name or destination == "localhost": @@ -336,9 +332,7 @@ class TransactionQueue(object): destination, [] ).append(failure) - preserve_context_over_fn( - self._attempt_new_transaction, destination - ) + self._attempt_new_transaction(destination) def send_device_messages(self, destination): if destination == self.server_name or destination == "localhost": @@ -347,15 +341,24 @@ class TransactionQueue(object): if not self.can_send_to(destination): return - preserve_context_over_fn( - self._attempt_new_transaction, destination - ) + self._attempt_new_transaction(destination) def get_current_token(self): return 0 - @defer.inlineCallbacks def _attempt_new_transaction(self, destination): + """Try to start a new transaction to this destination + + If there is already a transaction in progress to this destination, + returns immediately. Otherwise kicks off the process of sending a + transaction in the background. + + Args: + destination (str): + + Returns: + None + """ # list of (pending_pdu, deferred, order) if destination in self.pending_transactions: # XXX: pending_transactions can get stuck on by a never-ending @@ -368,6 +371,19 @@ class TransactionQueue(object): ) return + logger.debug("TX [%s] Starting transaction loop", destination) + + # Drop the logcontext before starting the transaction. It doesn't + # really make sense to log all the outbound transactions against + # whatever path led us to this point: that's pretty arbitrary really. + # + # (this also means we can fire off _perform_transaction without + # yielding) + with logcontext.PreserveLoggingContext(): + self._transaction_transmission_loop(destination) + + @defer.inlineCallbacks + def _transaction_transmission_loop(self, destination): pending_pdus = [] try: self.pending_transactions[destination] = 1 From a6e3222fe5abf5b65b53678d1208c4c58f97b391 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 6 Oct 2017 14:24:06 +0100 Subject: [PATCH 31/32] Fed server: Move origin-check code to _handle_received_pdu The response-building code expects there to be an entry in the `results` list for each entry in the pdu_list, so the early `continue` was messing this up. That doesn't really matter, because all that the federation client does is log any errors, but it's pretty poor form. --- synapse/federation/federation_server.py | 48 ++++++++++++------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 51e3fdea06..e791a1266d 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -143,30 +143,6 @@ class FederationServer(FederationBase): results = [] for pdu in pdu_list: - # check that it's actually being sent from a valid destination to - # workaround bug #1753 in 0.18.5 and 0.18.6 - if transaction.origin != get_domain_from_id(pdu.event_id): - # We continue to accept join events from any server; this is - # necessary for the federation join dance to work correctly. - # (When we join over federation, the "helper" server is - # responsible for sending out the join event, rather than the - # origin. See bug #1893). - if not ( - pdu.type == 'm.room.member' and - pdu.content and - pdu.content.get("membership", None) == 'join' - ): - logger.info( - "Discarding PDU %s from invalid origin %s", - pdu.event_id, transaction.origin - ) - continue - else: - logger.info( - "Accepting join PDU %s from %s", - pdu.event_id, transaction.origin - ) - try: yield self._handle_received_pdu(transaction.origin, pdu) results.append({}) @@ -520,6 +496,30 @@ class FederationServer(FederationBase): Returns (Deferred): completes with None Raises: FederationError if the signatures / hash do not match """ + # check that it's actually being sent from a valid destination to + # workaround bug #1753 in 0.18.5 and 0.18.6 + if origin != get_domain_from_id(pdu.event_id): + # We continue to accept join events from any server; this is + # necessary for the federation join dance to work correctly. + # (When we join over federation, the "helper" server is + # responsible for sending out the join event, rather than the + # origin. See bug #1893). + if not ( + pdu.type == 'm.room.member' and + pdu.content and + pdu.content.get("membership", None) == 'join' + ): + logger.info( + "Discarding PDU %s from invalid origin %s", + pdu.event_id, origin + ) + return + else: + logger.info( + "Accepting join PDU %s from %s", + pdu.event_id, origin + ) + # Check signature. try: pdu = yield self._check_sigs_and_hash(pdu) From ba5b9b80a56a449ffab44afaf4661d5b44277898 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 6 Oct 2017 15:18:58 +0100 Subject: [PATCH 32/32] fed server: refactor on_incoming_transaction Move as much as possible to after the have_responded check, and reduce the number of times we iterate over the pdu list. --- synapse/federation/federation_server.py | 53 ++++++++++++++----------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index e791a1266d..fa4ec2ad3c 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -109,23 +109,12 @@ class FederationServer(FederationBase): @defer.inlineCallbacks @log_function def on_incoming_transaction(self, transaction_data): + # keep this as early as possible to make the calculated origin ts as + # accurate as possible. + request_time = int(self._clock.time_msec()) + transaction = Transaction(**transaction_data) - received_pdus_counter.inc_by(len(transaction.pdus)) - - for p in transaction.pdus: - if "unsigned" in p: - unsigned = p["unsigned"] - if "age" in unsigned: - p["age"] = unsigned["age"] - if "age" in p: - p["age_ts"] = int(self._clock.time_msec()) - int(p["age"]) - del p["age"] - - pdu_list = [ - self.event_from_pdu_json(p) for p in transaction.pdus - ] - logger.debug("[%s] Got transaction", transaction.transaction_id) response = yield self.transaction_actions.have_responded(transaction) @@ -140,17 +129,35 @@ class FederationServer(FederationBase): logger.debug("[%s] Transaction is new", transaction.transaction_id) - results = [] + received_pdus_counter.inc_by(len(transaction.pdus)) + + pdu_list = [] + + for p in transaction.pdus: + if "unsigned" in p: + unsigned = p["unsigned"] + if "age" in unsigned: + p["age"] = unsigned["age"] + if "age" in p: + p["age_ts"] = request_time - int(p["age"]) + del p["age"] + + event = self.event_from_pdu_json(p) + pdu_list.append(event) + + pdu_results = {} for pdu in pdu_list: + event_id = pdu.event_id try: yield self._handle_received_pdu(transaction.origin, pdu) - results.append({}) + pdu_results[event_id] = {} except FederationError as e: + logger.warn("Error handling PDU %s: %s", event_id, e) self.send_failure(e, transaction.origin) - results.append({"error": str(e)}) + pdu_results[event_id] = {"error": str(e)} except Exception as e: - results.append({"error": str(e)}) + pdu_results[event_id] = {"error": str(e)} logger.exception("Failed to handle PDU") if hasattr(transaction, "edus"): @@ -164,14 +171,12 @@ class FederationServer(FederationBase): for failure in getattr(transaction, "pdu_failures", []): logger.info("Got failure %r", failure) - logger.debug("Returning: %s", str(results)) - response = { - "pdus": dict(zip( - (p.event_id for p in pdu_list), results - )), + "pdus": pdu_results, } + logger.debug("Returning: %s", str(response)) + yield self.transaction_actions.set_response( transaction, 200, response