From a50cf929c13044f25c3776802287458fe5695c37 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 23 Jan 2019 20:21:33 +0000 Subject: [PATCH 01/15] Require event format version to parse or create events --- synapse/events/__init__.py | 24 ++++++++- synapse/events/builder.py | 51 +++++++++++++++++- synapse/federation/federation_base.py | 9 ++-- synapse/federation/federation_client.py | 60 +++++++++++++-------- synapse/federation/federation_server.py | 33 +++++++----- synapse/federation/transport/server.py | 4 +- synapse/handlers/federation.py | 72 +++++++++++++++---------- synapse/handlers/message.py | 10 +++- 8 files changed, 191 insertions(+), 72 deletions(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 38470ad176..3fe52aaa45 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -18,7 +18,11 @@ from distutils.util import strtobool import six -from synapse.api.constants import KNOWN_ROOM_VERSIONS, EventFormatVersions +from synapse.api.constants import ( + KNOWN_EVENT_FORMAT_VERSIONS, + KNOWN_ROOM_VERSIONS, + EventFormatVersions, +) from synapse.util.caches import intern_dict from synapse.util.frozenutils import freeze @@ -256,3 +260,21 @@ def room_version_to_event_format(room_version): raise RuntimeError("Unrecognized room version %s" % (room_version,)) return EventFormatVersions.V1 + + +def event_type_from_format_version(format_version): + """Returns the python type to use to construct an Event object for the + given event format version. + + Args: + format_version (int): The event format version + + Returns: + type: A type that can be initialized as per the initializer of + `FrozenEvent` + """ + if format_version not in KNOWN_EVENT_FORMAT_VERSIONS: + raise Exception( + "No event format %r" % (format_version,) + ) + return FrozenEvent diff --git a/synapse/events/builder.py b/synapse/events/builder.py index e662eaef10..7e63371095 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -15,12 +15,39 @@ import copy +from synapse.api.constants import RoomVersions from synapse.types import EventID from synapse.util.stringutils import random_string from . import EventBase, FrozenEvent, _event_dict_property +def get_event_builder(room_version, key_values={}, internal_metadata_dict={}): + """Generate an event builder appropriate for the given room version + + Args: + room_version (str): Version of the room that we're creating an + event builder for + key_values (dict): Fields used as the basis of the new event + internal_metadata_dict (dict): Used to create the `_EventInternalMetadata` + object. + + Returns: + EventBuilder + """ + if room_version in { + RoomVersions.V1, + RoomVersions.V2, + RoomVersions.VDH_TEST, + RoomVersions.STATE_V2_TEST, + }: + return EventBuilder(key_values, internal_metadata_dict) + else: + raise Exception( + "No event format defined for version %r" % (room_version,) + ) + + class EventBuilder(EventBase): def __init__(self, key_values={}, internal_metadata_dict={}): signatures = copy.deepcopy(key_values.pop("signatures", {})) @@ -58,7 +85,29 @@ class EventBuilderFactory(object): return e_id.to_string() - def new(self, key_values={}): + def new(self, room_version, key_values={}): + """Generate an event builder appropriate for the given room version + + Args: + room_version (str): Version of the room that we're creating an + event builder for + key_values (dict): Fields used as the basis of the new event + + Returns: + EventBuilder + """ + + # There's currently only the one event version defined + if room_version not in { + RoomVersions.V1, + RoomVersions.V2, + RoomVersions.VDH_TEST, + RoomVersions.STATE_V2_TEST, + }: + raise Exception( + "No event format defined for version %r" % (room_version,) + ) + key_values["event_id"] = self.create_event_id() time_now = int(self.clock.time_msec()) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index d749bfdd3a..5c31e5f85f 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -23,7 +23,7 @@ from twisted.internet.defer import DeferredList from synapse.api.constants import MAX_DEPTH, EventTypes, Membership from synapse.api.errors import Codes, SynapseError from synapse.crypto.event_signing import check_event_content_hash -from synapse.events import FrozenEvent +from synapse.events import event_type_from_format_version from synapse.events.utils import prune_event from synapse.http.servlet import assert_params_in_dict from synapse.types import get_domain_from_id @@ -302,11 +302,12 @@ def _is_invite_via_3pid(event): ) -def event_from_pdu_json(pdu_json, outlier=False): +def event_from_pdu_json(pdu_json, event_format_version, outlier=False): """Construct a FrozenEvent from an event json received over federation Args: pdu_json (object): pdu as received over federation + event_format_version (int): The event format version outlier (bool): True to mark this event as an outlier Returns: @@ -330,8 +331,8 @@ def event_from_pdu_json(pdu_json, outlier=False): elif depth > MAX_DEPTH: raise SynapseError(400, "Depth too large", Codes.BAD_JSON) - event = FrozenEvent( - pdu_json + event = event_type_from_format_version(event_format_version)( + pdu_json, ) event.internal_metadata.outlier = outlier diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 33ecabca29..71809893c5 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -170,13 +170,13 @@ class FederationClient(FederationBase): @defer.inlineCallbacks @log_function - def backfill(self, dest, context, limit, extremities): + def backfill(self, dest, room_id, limit, extremities): """Requests some more historic PDUs for the given context from the given destination server. Args: dest (str): The remote home server to ask. - context (str): The context to backfill. + room_id (str): The room_id to backfill. limit (int): The maximum number of PDUs to return. extremities (list): List of PDU id and origins of the first pdus we have seen from the context @@ -191,12 +191,15 @@ class FederationClient(FederationBase): return transaction_data = yield self.transport_layer.backfill( - dest, context, extremities, limit) + dest, room_id, extremities, limit) logger.debug("backfill transaction_data=%s", repr(transaction_data)) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + pdus = [ - event_from_pdu_json(p, outlier=False) + event_from_pdu_json(p, format_ver, outlier=False) for p in transaction_data["pdus"] ] @@ -240,6 +243,8 @@ class FederationClient(FederationBase): pdu_attempts = self.pdu_destination_tried.setdefault(event_id, {}) + format_ver = room_version_to_event_format(room_version) + signed_pdu = None for destination in destinations: now = self._clock.time_msec() @@ -255,7 +260,7 @@ class FederationClient(FederationBase): logger.debug("transaction_data %r", transaction_data) pdu_list = [ - event_from_pdu_json(p, outlier=outlier) + event_from_pdu_json(p, format_ver, outlier=outlier) for p in transaction_data["pdus"] ] @@ -349,12 +354,16 @@ class FederationClient(FederationBase): destination, room_id, event_id=event_id, ) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + pdus = [ - event_from_pdu_json(p, outlier=True) for p in result["pdus"] + event_from_pdu_json(p, format_ver, outlier=True) + for p in result["pdus"] ] auth_chain = [ - event_from_pdu_json(p, outlier=True) + event_from_pdu_json(p, format_ver, outlier=True) for p in result.get("auth_chain", []) ] @@ -362,8 +371,6 @@ class FederationClient(FederationBase): ev.event_id for ev in itertools.chain(pdus, auth_chain) ]) - room_version = yield self.store.get_room_version(room_id) - signed_pdus = yield self._check_sigs_and_hash_and_fetch( destination, [p for p in pdus if p.event_id not in seen_events], @@ -462,13 +469,14 @@ class FederationClient(FederationBase): destination, room_id, event_id, ) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + auth_chain = [ - event_from_pdu_json(p, outlier=True) + event_from_pdu_json(p, format_ver, outlier=True) for p in res["auth_chain"] ] - room_version = yield self.store.get_room_version(room_id) - signed_auth = yield self._check_sigs_and_hash_and_fetch( destination, auth_chain, outlier=True, room_version=room_version, @@ -605,7 +613,7 @@ class FederationClient(FederationBase): pdu_dict.pop("origin_server_ts", None) pdu_dict.pop("unsigned", None) - builder = self.event_builder_factory.new(pdu_dict) + builder = self.event_builder_factory.new(room_version, pdu_dict) add_hashes_and_signatures( builder, self.hs.hostname, @@ -621,7 +629,7 @@ class FederationClient(FederationBase): "make_" + membership, destinations, send_request, ) - def send_join(self, destinations, pdu): + def send_join(self, destinations, pdu, event_format_version): """Sends a join event to one of a list of homeservers. Doing so will cause the remote server to add the event to the graph, @@ -631,6 +639,7 @@ class FederationClient(FederationBase): destinations (str): Candidate homeservers which are probably participating in the room. pdu (BaseEvent): event to be sent + event_format_version (int): The event format version Return: Deferred: resolves to a dict with members ``origin`` (a string @@ -676,12 +685,12 @@ class FederationClient(FederationBase): logger.debug("Got content: %s", content) state = [ - event_from_pdu_json(p, outlier=True) + event_from_pdu_json(p, event_format_version, outlier=True) for p in content.get("state", []) ] auth_chain = [ - event_from_pdu_json(p, outlier=True) + event_from_pdu_json(p, event_format_version, outlier=True) for p in content.get("auth_chain", []) ] @@ -759,7 +768,10 @@ class FederationClient(FederationBase): logger.debug("Got response to send_invite: %s", pdu_dict) - pdu = event_from_pdu_json(pdu_dict) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + + pdu = event_from_pdu_json(pdu_dict, format_ver) # Check signatures are correct. pdu = yield self._check_sigs_and_hash(pdu) @@ -837,13 +849,14 @@ class FederationClient(FederationBase): content=send_content, ) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + auth_chain = [ - event_from_pdu_json(e) + event_from_pdu_json(e, format_ver) for e in content["auth_chain"] ] - room_version = yield self.store.get_room_version(room_id) - signed_auth = yield self._check_sigs_and_hash_and_fetch( destination, auth_chain, outlier=True, room_version=room_version, ) @@ -887,13 +900,14 @@ class FederationClient(FederationBase): timeout=timeout, ) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + events = [ - event_from_pdu_json(e) + event_from_pdu_json(e, format_ver) for e in content.get("events", []) ] - room_version = yield self.store.get_room_version(room_id) - signed_events = yield self._check_sigs_and_hash_and_fetch( destination, events, outlier=False, room_version=room_version, ) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index dde166e295..4aa04b9588 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -34,6 +34,7 @@ from synapse.api.errors import ( SynapseError, ) from synapse.crypto.event_signing import compute_event_signature +from synapse.events import room_version_to_event_format from synapse.federation.federation_base import FederationBase, event_from_pdu_json from synapse.federation.persistence import TransactionActions from synapse.federation.units import Edu, Transaction @@ -178,14 +179,13 @@ class FederationServer(FederationBase): continue try: - # In future we will actually use the room version to parse the - # PDU into an event. - yield self.store.get_room_version(room_id) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) except NotFoundError: logger.info("Ignoring PDU for unknown room_id: %s", room_id) continue - event = event_from_pdu_json(p) + event = event_from_pdu_json(p, format_ver) pdus_by_room.setdefault(room_id, []).append(event) pdu_results = {} @@ -370,7 +370,9 @@ class FederationServer(FederationBase): @defer.inlineCallbacks def on_invite_request(self, origin, content, room_version): - pdu = event_from_pdu_json(content) + format_ver = room_version_to_event_format(room_version) + + pdu = event_from_pdu_json(content, format_ver) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) ret_pdu = yield self.handler.on_invite_request(origin, pdu) @@ -378,9 +380,12 @@ class FederationServer(FederationBase): defer.returnValue({"event": ret_pdu.get_pdu_json(time_now)}) @defer.inlineCallbacks - def on_send_join_request(self, origin, content): + def on_send_join_request(self, origin, content, room_id): logger.debug("on_send_join_request: content: %s", content) - pdu = event_from_pdu_json(content) + + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + pdu = event_from_pdu_json(content, format_ver) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) @@ -410,9 +415,12 @@ class FederationServer(FederationBase): }) @defer.inlineCallbacks - def on_send_leave_request(self, origin, content): + def on_send_leave_request(self, origin, content, room_id): logger.debug("on_send_leave_request: content: %s", content) - pdu = event_from_pdu_json(content) + + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + pdu = event_from_pdu_json(content, format_ver) origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, pdu.room_id) @@ -458,13 +466,14 @@ class FederationServer(FederationBase): origin_host, _ = parse_server_name(origin) yield self.check_server_matches_acl(origin_host, room_id) + room_version = yield self.store.get_room_version(room_id) + format_ver = room_version_to_event_format(room_version) + auth_chain = [ - event_from_pdu_json(e) + event_from_pdu_json(e, format_ver) for e in content["auth_chain"] ] - room_version = yield self.store.get_room_version(room_id) - signed_auth = yield self._check_sigs_and_hash_and_fetch( origin, auth_chain, outlier=True, room_version=room_version, ) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 4557a9e66e..67ae0212c3 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -469,7 +469,7 @@ class FederationSendLeaveServlet(BaseFederationServlet): @defer.inlineCallbacks def on_PUT(self, origin, content, query, room_id, event_id): - content = yield self.handler.on_send_leave_request(origin, content) + content = yield self.handler.on_send_leave_request(origin, content, room_id) defer.returnValue((200, content)) @@ -487,7 +487,7 @@ class FederationSendJoinServlet(BaseFederationServlet): def on_PUT(self, origin, content, query, context, event_id): # TODO(paul): assert that context/event_id parsed from path actually # match those given in content - content = yield self.handler.on_send_join_request(origin, content) + content = yield self.handler.on_send_join_request(origin, content, context) defer.returnValue((200, content)) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index c52dca1b81..a4b771049c 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1061,7 +1061,7 @@ class FederationHandler(BaseHandler): """ logger.debug("Joining %s to %s", joinee, room_id) - origin, event = yield self._make_and_verify_event( + origin, event, event_format_version = yield self._make_and_verify_event( target_hosts, room_id, joinee, @@ -1091,7 +1091,9 @@ class FederationHandler(BaseHandler): target_hosts.insert(0, origin) except ValueError: pass - ret = yield self.federation_client.send_join(target_hosts, event) + ret = yield self.federation_client.send_join( + target_hosts, event, event_format_version, + ) origin = ret["origin"] state = ret["state"] @@ -1164,13 +1166,18 @@ class FederationHandler(BaseHandler): """ event_content = {"membership": Membership.JOIN} - builder = self.event_builder_factory.new({ - "type": EventTypes.Member, - "content": event_content, - "room_id": room_id, - "sender": user_id, - "state_key": user_id, - }) + room_version = yield self.store.get_room_version(room_id) + + builder = self.event_builder_factory.new( + room_version, + { + "type": EventTypes.Member, + "content": event_content, + "room_id": room_id, + "sender": user_id, + "state_key": user_id, + } + ) try: event, context = yield self.event_creation_handler.create_new_client_event( @@ -1304,7 +1311,7 @@ class FederationHandler(BaseHandler): @defer.inlineCallbacks def do_remotely_reject_invite(self, target_hosts, room_id, user_id): - origin, event = yield self._make_and_verify_event( + origin, event, event_format_version = yield self._make_and_verify_event( target_hosts, room_id, user_id, @@ -1336,7 +1343,7 @@ class FederationHandler(BaseHandler): @defer.inlineCallbacks def _make_and_verify_event(self, target_hosts, room_id, user_id, membership, content={}, params=None): - origin, pdu, _ = yield self.federation_client.make_membership_event( + origin, event, format_ver = yield self.federation_client.make_membership_event( target_hosts, room_id, user_id, @@ -1345,9 +1352,7 @@ class FederationHandler(BaseHandler): params=params, ) - logger.debug("Got response to make_%s: %s", membership, pdu) - - event = pdu + logger.debug("Got response to make_%s: %s", membership, event) # We should assert some things. # FIXME: Do this in a nicer way @@ -1355,7 +1360,7 @@ class FederationHandler(BaseHandler): assert(event.user_id == user_id) assert(event.state_key == user_id) assert(event.room_id == room_id) - defer.returnValue((origin, event)) + defer.returnValue((origin, event, format_ver)) @defer.inlineCallbacks @log_function @@ -1364,13 +1369,17 @@ class FederationHandler(BaseHandler): leave event for the room and return that. We do *not* persist or process it until the other server has signed it and sent it back. """ - builder = self.event_builder_factory.new({ - "type": EventTypes.Member, - "content": {"membership": Membership.LEAVE}, - "room_id": room_id, - "sender": user_id, - "state_key": user_id, - }) + room_version = yield self.store.get_room_version(room_id) + builder = self.event_builder_factory.new( + room_version, + { + "type": EventTypes.Member, + "content": {"membership": Membership.LEAVE}, + "room_id": room_id, + "sender": user_id, + "state_key": user_id, + } + ) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, @@ -2266,14 +2275,16 @@ class FederationHandler(BaseHandler): } if (yield self.auth.check_host_in_room(room_id, self.hs.hostname)): - builder = self.event_builder_factory.new(event_dict) + room_version = yield self.store.get_room_version(room_id) + builder = self.event_builder_factory.new(room_version, event_dict) + EventValidator().validate_new(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder ) event, context = yield self.add_display_name_to_third_party_invite( - event_dict, event, context + room_version, event_dict, event, context ) try: @@ -2304,14 +2315,18 @@ class FederationHandler(BaseHandler): Returns: Deferred: resolves (to None) """ - builder = self.event_builder_factory.new(event_dict) + room_version = yield self.store.get_room_version(room_id) + + # NB: event_dict has a particular specced format we might need to fudge + # if we change event formats too much. + builder = self.event_builder_factory.new(room_version, event_dict) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, ) event, context = yield self.add_display_name_to_third_party_invite( - event_dict, event, context + room_version, event_dict, event, context ) try: @@ -2331,7 +2346,8 @@ class FederationHandler(BaseHandler): yield member_handler.send_membership_event(None, event, context) @defer.inlineCallbacks - def add_display_name_to_third_party_invite(self, event_dict, event, context): + def add_display_name_to_third_party_invite(self, room_version, event_dict, + event, context): key = ( EventTypes.ThirdPartyInvite, event.content["third_party_invite"]["signed"]["token"] @@ -2355,7 +2371,7 @@ class FederationHandler(BaseHandler): # auth checks. If we need the invite and don't have it then the # auth check code will explode appropriately. - builder = self.event_builder_factory.new(event_dict) + builder = self.event_builder_factory.new(room_version, event_dict) EventValidator().validate_new(builder) event, context = yield self.event_creation_handler.create_new_client_event( builder=builder, diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index a7cd779b02..7aaa4fba33 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -278,7 +278,15 @@ class EventCreationHandler(object): """ yield self.auth.check_auth_blocking(requester.user.to_string()) - builder = self.event_builder_factory.new(event_dict) + if event_dict["type"] == EventTypes.Create and event_dict["state_key"] == "": + room_version = event_dict["content"]["room_version"] + else: + try: + room_version = yield self.store.get_room_version(event_dict["room_id"]) + except NotFoundError: + raise AuthError(403, "Unknown room") + + builder = self.event_builder_factory.new(room_version, event_dict) self.validator.validate_new(builder) From 9770ed91c223051d940f773e0b1fa2746003eb4e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 24 Jan 2019 09:28:16 +0000 Subject: [PATCH 02/15] Fix tests --- tests/storage/test_redaction.py | 5 ++++- tests/storage/test_roommember.py | 3 ++- tests/storage/test_state.py | 3 ++- tests/test_visibility.py | 4 ++++ tests/utils.py | 3 ++- 5 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/storage/test_redaction.py b/tests/storage/test_redaction.py index 02bf975fbf..3957561b1e 100644 --- a/tests/storage/test_redaction.py +++ b/tests/storage/test_redaction.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -52,6 +52,7 @@ class RedactionTestCase(unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), @@ -74,6 +75,7 @@ class RedactionTestCase(unittest.TestCase): self.depth += 1 builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Message, "sender": user.to_string(), @@ -94,6 +96,7 @@ class RedactionTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_redaction(self, room, event_id, user, reason): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Redaction, "sender": user.to_string(), diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 978c66133d..7fa2f4fd70 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -18,7 +18,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.types import RoomID, UserID from tests import unittest @@ -50,6 +50,7 @@ class RoomMemberStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def inject_room_member(self, room, user, membership, replaces_state=None): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Member, "sender": user.to_string(), diff --git a/tests/storage/test_state.py b/tests/storage/test_state.py index 086a39d834..a1f99134dc 100644 --- a/tests/storage/test_state.py +++ b/tests/storage/test_state.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomVersions from synapse.storage.state import StateFilter from synapse.types import RoomID, UserID @@ -52,6 +52,7 @@ class StateStoreTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def inject_state_event(self, room, sender, typ, state_key, content): builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": typ, "sender": sender.to_string(), diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 2eea3b098b..82d63ce00e 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -17,6 +17,7 @@ import logging from twisted.internet import defer from twisted.internet.defer import succeed +from synapse.api.constants import RoomVersions from synapse.events import FrozenEvent from synapse.visibility import filter_events_for_server @@ -124,6 +125,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): def inject_visibility(self, user_id, visibility): content = {"history_visibility": visibility} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.history_visibility", "sender": user_id, @@ -144,6 +146,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): content = {"membership": membership} content.update(extra_content) builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.member", "sender": user_id, @@ -165,6 +168,7 @@ class FilterEventsForServerTestCase(tests.unittest.TestCase): if content is None: content = {"body": "testytest"} builder = self.event_builder_factory.new( + RoomVersions.V1, { "type": "m.room.message", "sender": user_id, diff --git a/tests/utils.py b/tests/utils.py index df73c539c3..2dfcb70a93 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -26,7 +26,7 @@ from six.moves.urllib import parse as urlparse from twisted.internet import defer, reactor -from synapse.api.constants import EventTypes +from synapse.api.constants import EventTypes, RoomVersions from synapse.api.errors import CodeMessageException, cs_error from synapse.config.server import ServerConfig from synapse.federation.transport import server @@ -624,6 +624,7 @@ def create_room(hs, room_id, creator_id): event_creation_handler = hs.get_event_creation_handler() builder = event_builder_factory.new( + RoomVersions.V1, { "type": EventTypes.Create, "state_key": "", From 678a92cb56d547dcadffa723e29b4855a27d0901 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 24 Jan 2019 11:14:07 +0000 Subject: [PATCH 03/15] Replace missed usages of FrozenEvent --- synapse/replication/http/federation.py | 8 ++++++-- synapse/replication/http/send_event.py | 8 ++++++-- synapse/storage/events_worker.py | 8 ++------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/synapse/replication/http/federation.py b/synapse/replication/http/federation.py index 64a79da162..2e16c69666 100644 --- a/synapse/replication/http/federation.py +++ b/synapse/replication/http/federation.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.events import FrozenEvent +from synapse.events import event_type_from_format_version from synapse.events.snapshot import EventContext from synapse.http.servlet import parse_json_object_from_request from synapse.replication.http._base import ReplicationEndpoint @@ -70,6 +70,7 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): event_payloads.append({ "event": event.get_pdu_json(), + "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), "rejected_reason": event.rejected_reason, "context": serialized_context, @@ -94,9 +95,12 @@ class ReplicationFederationSendEventsRestServlet(ReplicationEndpoint): event_and_contexts = [] for event_payload in event_payloads: event_dict = event_payload["event"] + format_ver = content["event_format_version"] internal_metadata = event_payload["internal_metadata"] rejected_reason = event_payload["rejected_reason"] - event = FrozenEvent(event_dict, internal_metadata, rejected_reason) + + EventType = event_type_from_format_version(format_ver) + event = EventType(event_dict, internal_metadata, rejected_reason) context = yield EventContext.deserialize( self.store, event_payload["context"], diff --git a/synapse/replication/http/send_event.py b/synapse/replication/http/send_event.py index 5b52c91650..3635015eda 100644 --- a/synapse/replication/http/send_event.py +++ b/synapse/replication/http/send_event.py @@ -17,7 +17,7 @@ import logging from twisted.internet import defer -from synapse.events import FrozenEvent +from synapse.events import event_type_from_format_version from synapse.events.snapshot import EventContext from synapse.http.servlet import parse_json_object_from_request from synapse.replication.http._base import ReplicationEndpoint @@ -74,6 +74,7 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): payload = { "event": event.get_pdu_json(), + "event_format_version": event.format_version, "internal_metadata": event.internal_metadata.get_dict(), "rejected_reason": event.rejected_reason, "context": serialized_context, @@ -90,9 +91,12 @@ class ReplicationSendEventRestServlet(ReplicationEndpoint): content = parse_json_object_from_request(request) event_dict = content["event"] + format_ver = content["event_format_version"] internal_metadata = content["internal_metadata"] rejected_reason = content["rejected_reason"] - event = FrozenEvent(event_dict, internal_metadata, rejected_reason) + + EventType = event_type_from_format_version(format_ver) + event = EventType(event_dict, internal_metadata, rejected_reason) requester = Requester.deserialize(self.store, content["requester"]) context = yield EventContext.deserialize(self.store, content["context"]) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index 599f892858..0a0ca58fc4 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -23,7 +23,7 @@ from twisted.internet import defer from synapse.api.constants import EventFormatVersions from synapse.api.errors import NotFoundError -from synapse.events import FrozenEvent +from synapse.events import FrozenEvent, event_type_from_format_version # noqa: F401 # these are only included to make the type annotations work from synapse.events.snapshot import EventContext # noqa: F401 from synapse.events.utils import prune_event @@ -412,11 +412,7 @@ class EventsWorkerStore(SQLBaseStore): # of a event format version, so it must be a V1 event. format_version = EventFormatVersions.V1 - # TODO: When we implement new event formats we'll need to use a - # different event python type - assert format_version == EventFormatVersions.V1 - - original_ev = FrozenEvent( + original_ev = event_type_from_format_version(format_version)( event_dict=d, internal_metadata_dict=internal_metadata, rejected_reason=rejected_reason, From 5d881cbcb8199d0b933050462a580175fc859e70 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 25 Jan 2019 10:37:13 +0000 Subject: [PATCH 04/15] Newsfile --- changelog.d/4470.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4470.misc diff --git a/changelog.d/4470.misc b/changelog.d/4470.misc new file mode 100644 index 0000000000..43f8963614 --- /dev/null +++ b/changelog.d/4470.misc @@ -0,0 +1 @@ +Add infrastructure to support different event formats From 19530671362021b2ef53f4926c278e346406997b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 25 Jan 2019 10:46:49 +0000 Subject: [PATCH 05/15] Disable native upserts for sqlite, as they don't work --- synapse/storage/engines/sqlite.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index c64d73ff21..206919fcd5 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -34,10 +34,14 @@ class Sqlite3Engine(object): @property def can_native_upsert(self): """ - Do we support native UPSERTs? This requires SQLite3 3.24+, plus some - more work we haven't done yet to tell what was inserted vs updated. + Do we support native UPSERTs? """ - return sqlite_version_info >= (3, 24, 0) + # SQLite3 3.24+ supports them, but empirically the unit tests don't work + # when its enabled. + # FIXME: Figure out what is wrong so we can re-enable native upserts + + # return sqlite_version_info >= (3, 24, 0) + return False def check_database(self, txn): pass From 8dcfa6e75cb1c8e5afc62c1d964e1c752dd1917b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 25 Jan 2019 10:48:00 +0000 Subject: [PATCH 06/15] Newsfile --- changelog.d/4306.misc | 2 +- changelog.d/4471.misc | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/4471.misc diff --git a/changelog.d/4306.misc b/changelog.d/4306.misc index 58130b6190..7f48b02fbf 100644 --- a/changelog.d/4306.misc +++ b/changelog.d/4306.misc @@ -1 +1 @@ -Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. diff --git a/changelog.d/4471.misc b/changelog.d/4471.misc new file mode 100644 index 0000000000..7f48b02fbf --- /dev/null +++ b/changelog.d/4471.misc @@ -0,0 +1 @@ +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+. From 0b3fd1401fdebb944729cf46d6de9c3bff482933 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Fri, 25 Jan 2019 11:25:02 +0000 Subject: [PATCH 07/15] Don't require sqlite3 when using postgres (#4466) --- changelog.d/4466.misc | 1 + synapse/storage/engines/sqlite.py | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 changelog.d/4466.misc diff --git a/changelog.d/4466.misc b/changelog.d/4466.misc new file mode 100644 index 0000000000..58130b6190 --- /dev/null +++ b/changelog.d/4466.misc @@ -0,0 +1 @@ +Synapse will now take advantage of native UPSERT functionality in PostgreSQL 9.5+ and SQLite 3.24+. diff --git a/synapse/storage/engines/sqlite.py b/synapse/storage/engines/sqlite.py index c64d73ff21..059ab81055 100644 --- a/synapse/storage/engines/sqlite.py +++ b/synapse/storage/engines/sqlite.py @@ -15,7 +15,6 @@ import struct import threading -from sqlite3 import sqlite_version_info from synapse.storage.prepare_database import prepare_database @@ -37,7 +36,7 @@ class Sqlite3Engine(object): Do we support native UPSERTs? This requires SQLite3 3.24+, plus some more work we haven't done yet to tell what was inserted vs updated. """ - return sqlite_version_info >= (3, 24, 0) + return self.module.sqlite_version_info >= (3, 24, 0) def check_database(self, txn): pass From 8520bc3109c2de6175391497941f3fc0b74c08e5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 25 Jan 2019 12:38:16 +0000 Subject: [PATCH 08/15] Fix Host header sent by MatrixFederationAgent (#4468) Move the Host header logic down here so that (a) it is used if we reuse the agent elsewhere, and (b) we can mess about with it with .well-known. --- changelog.d/4468.misc | 1 + .../http/federation/matrix_federation_agent.py | 10 ++++++++++ synapse/http/matrixfederationclient.py | 1 - .../federation/test_matrix_federation_agent.py | 16 ++++++++++++++++ tests/http/test_fedclient.py | 2 +- 5 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 changelog.d/4468.misc diff --git a/changelog.d/4468.misc b/changelog.d/4468.misc new file mode 100644 index 0000000000..9a51434755 --- /dev/null +++ b/changelog.d/4468.misc @@ -0,0 +1 @@ +Move SRV logic into the Agent layer diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 0ec28c6696..1788e9a34a 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -19,6 +19,7 @@ from zope.interface import implementer from twisted.internet import defer from twisted.internet.endpoints import HostnameEndpoint, wrapClientTLS from twisted.web.client import URI, Agent, HTTPConnectionPool +from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent from synapse.http.endpoint import parse_server_name @@ -109,6 +110,15 @@ class MatrixFederationAgent(object): else: target = pick_server_from_list(server_list) + # make sure that the Host header is set correctly + if headers is None: + headers = Headers() + else: + headers = headers.copy() + + if not headers.hasHeader(b'host'): + headers.addRawHeader(b'host', server_name_bytes) + class EndpointFactory(object): @staticmethod def endpointForURI(_uri): diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 980e912348..bb2e64ed80 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -255,7 +255,6 @@ class MatrixFederationHttpClient(object): headers_dict = { b"User-Agent": [self.version_string_bytes], - b"Host": [destination_bytes], } with limiter: diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index b32d7566a5..261afb5f41 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -131,6 +131,10 @@ class MatrixFederationAgentTests(TestCase): request = http_server.requests[0] self.assertEqual(request.method, b'GET') self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'testserv:8448'] + ) content = request.content.read() self.assertEqual(content, b'') @@ -195,6 +199,10 @@ class MatrixFederationAgentTests(TestCase): request = http_server.requests[0] self.assertEqual(request.method, b'GET') self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'1.2.3.4'], + ) # finish the request request.finish() @@ -235,6 +243,10 @@ class MatrixFederationAgentTests(TestCase): request = http_server.requests[0] self.assertEqual(request.method, b'GET') self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'testserv'], + ) # finish the request request.finish() @@ -276,6 +288,10 @@ class MatrixFederationAgentTests(TestCase): request = http_server.requests[0] self.assertEqual(request.method, b'GET') self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'testserv'], + ) # finish the request request.finish() diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py index d37f8f9981..018c77ebcd 100644 --- a/tests/http/test_fedclient.py +++ b/tests/http/test_fedclient.py @@ -49,7 +49,6 @@ class FederationClientTests(HomeserverTestCase): return hs def prepare(self, reactor, clock, homeserver): - self.cl = MatrixFederationHttpClient(self.hs) self.reactor.lookups["testserv"] = "1.2.3.4" @@ -95,6 +94,7 @@ class FederationClientTests(HomeserverTestCase): # that should have made it send the request to the transport self.assertRegex(transport.value(), b"^GET /foo/bar") + self.assertRegex(transport.value(), b"Host: testserv:8008") # Deferred is still without a result self.assertNoResult(test_d) From 4a3f1388328f8181c33b74a6a777ab2073d400d8 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 25 Jan 2019 13:57:52 +0000 Subject: [PATCH 09/15] Fix quoting for allowed_local_3pids example config (#4476) If you use double-quotes here, you have to escape your backslashes. It's much easier with single-quotes. (Note that the existing double-backslashes are already interpreted by python's """ parsing.) --- changelog.d/4476.misc | 1 + synapse/config/registration.py | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) create mode 100644 changelog.d/4476.misc diff --git a/changelog.d/4476.misc b/changelog.d/4476.misc new file mode 100644 index 0000000000..a070e10c7c --- /dev/null +++ b/changelog.d/4476.misc @@ -0,0 +1 @@ +Fix quoting for allowed_local_3pids example config diff --git a/synapse/config/registration.py b/synapse/config/registration.py index fe520d6855..d808a989f3 100644 --- a/synapse/config/registration.py +++ b/synapse/config/registration.py @@ -84,11 +84,11 @@ class RegistrationConfig(Config): # # allowed_local_3pids: # - medium: email - # pattern: ".*@matrix\\.org" + # pattern: '.*@matrix\\.org' # - medium: email - # pattern: ".*@vector\\.im" + # pattern: '.*@vector\\.im' # - medium: msisdn - # pattern: "\\+44" + # pattern: '\\+44' # If set, allows registration by anyone who also has the shared # secret, even if registration is otherwise disabled. From d840019192769f900f6a1a5c768368a080e651cd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 28 Jan 2019 09:56:59 +0000 Subject: [PATCH 10/15] Fix idna and ipv6 literal handling in MatrixFederationAgent (#4487) Turns out that the library does a better job of parsing URIs than our reinvented wheel. Who knew. There are two things going on here. The first is that, unlike parse_server_name, URI.fromBytes will strip off square brackets from IPv6 literals, which means that it is valid input to ClientTLSOptionsFactory and HostnameEndpoint. The second is that we stay in `bytes` throughout (except for the argument to ClientTLSOptionsFactory), which avoids the weirdness of (sometimes) ending up with idna-encoded values being held in `unicode` variables. TBH it probably would have been ok but it made the tests fragile. --- changelog.d/4487.misc | 1 + .../federation/matrix_federation_agent.py | 23 +-- .../test_matrix_federation_agent.py | 181 +++++++++++++++++- 3 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 changelog.d/4487.misc diff --git a/changelog.d/4487.misc b/changelog.d/4487.misc new file mode 100644 index 0000000000..79de8eb3ad --- /dev/null +++ b/changelog.d/4487.misc @@ -0,0 +1 @@ +Fix idna and ipv6 literal handling in MatrixFederationAgent diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 1788e9a34a..9526f39cca 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -22,7 +22,6 @@ from twisted.web.client import URI, Agent, HTTPConnectionPool from twisted.web.http_headers import Headers from twisted.web.iweb import IAgent -from synapse.http.endpoint import parse_server_name from synapse.http.federation.srv_resolver import SrvResolver, pick_server_from_list from synapse.util.logcontext import make_deferred_yieldable @@ -87,9 +86,7 @@ class MatrixFederationAgent(object): from being sent). """ - parsed_uri = URI.fromBytes(uri) - server_name_bytes = parsed_uri.netloc - host, port = parse_server_name(server_name_bytes.decode("ascii")) + parsed_uri = URI.fromBytes(uri, defaultPort=-1) # XXX disabling TLS is really only supported here for the benefit of the # unit tests. We should make the UTs cope with TLS rather than having to make @@ -97,16 +94,20 @@ class MatrixFederationAgent(object): if self._tls_client_options_factory is None: tls_options = None else: - tls_options = self._tls_client_options_factory.get_options(host) + tls_options = self._tls_client_options_factory.get_options( + parsed_uri.host.decode("ascii") + ) - if port is not None: - target = (host, port) + if parsed_uri.port != -1: + # there was an explicit port in the URI + target = parsed_uri.host, parsed_uri.port else: - service_name = b"_matrix._tcp.%s" % (server_name_bytes, ) + service_name = b"_matrix._tcp.%s" % (parsed_uri.host, ) server_list = yield self._srv_resolver.resolve_service(service_name) if not server_list: - target = (host, 8448) - logger.debug("No SRV record for %s, using %s", host, target) + target = (parsed_uri.host, 8448) + logger.debug( + "No SRV record for %s, using %s", service_name, target) else: target = pick_server_from_list(server_list) @@ -117,7 +118,7 @@ class MatrixFederationAgent(object): headers = headers.copy() if not headers.hasHeader(b'host'): - headers.addRawHeader(b'host', server_name_bytes) + headers.addRawHeader(b'host', parsed_uri.netloc) class EndpointFactory(object): @staticmethod diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index 261afb5f41..f144092a51 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -209,6 +209,95 @@ class MatrixFederationAgentTests(TestCase): self.reactor.pump((0.1,)) self.successResultOf(test_d) + def test_get_ipv6_address(self): + """ + Test the behaviour when the server name contains an explicit IPv6 address + (with no port) + """ + + # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) + self.mock_resolver.resolve_service.side_effect = lambda _: [] + + # then there will be a getaddrinfo on the IP + self.reactor.lookups["::1"] = "::1" + + test_d = self._make_get_request(b"matrix://[::1]/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + self.mock_resolver.resolve_service.assert_called_once_with( + b"_matrix._tcp.::1", + ) + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, '::1') + self.assertEqual(port, 8448) + + # make a test server, and wire up the client + http_server = self._make_connection( + client_factory, + expected_sni=None, + ) + + self.assertEqual(len(http_server.requests), 1) + request = http_server.requests[0] + self.assertEqual(request.method, b'GET') + self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'[::1]'], + ) + + # finish the request + request.finish() + self.reactor.pump((0.1,)) + self.successResultOf(test_d) + + def test_get_ipv6_address_with_port(self): + """ + Test the behaviour when the server name contains an explicit IPv6 address + (with explicit port) + """ + + # there will be a getaddrinfo on the IP + self.reactor.lookups["::1"] = "::1" + + test_d = self._make_get_request(b"matrix://[::1]:80/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, '::1') + self.assertEqual(port, 80) + + # make a test server, and wire up the client + http_server = self._make_connection( + client_factory, + expected_sni=None, + ) + + self.assertEqual(len(http_server.requests), 1) + request = http_server.requests[0] + self.assertEqual(request.method, b'GET') + self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'[::1]:80'], + ) + + # finish the request + request.finish() + self.reactor.pump((0.1,)) + self.successResultOf(test_d) + def test_get_hostname_no_srv(self): """ Test the behaviour when the server name has no port, and no SRV record @@ -258,7 +347,7 @@ class MatrixFederationAgentTests(TestCase): Test the behaviour when there is a single SRV record """ self.mock_resolver.resolve_service.side_effect = lambda _: [ - Server(host="srvtarget", port=8443) + Server(host=b"srvtarget", port=8443) ] self.reactor.lookups["srvtarget"] = "1.2.3.4" @@ -298,6 +387,96 @@ class MatrixFederationAgentTests(TestCase): self.reactor.pump((0.1,)) self.successResultOf(test_d) + def test_idna_servername(self): + """test the behaviour when the server name has idna chars in""" + + self.mock_resolver.resolve_service.side_effect = lambda _: [] + + # hostnameendpoint does the lookup on the unicode value (getaddrinfo encodes + # it back to idna) + self.reactor.lookups[u"bücher.com"] = "1.2.3.4" + + # this is idna for bücher.com + test_d = self._make_get_request(b"matrix://xn--bcher-kva.com/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + self.mock_resolver.resolve_service.assert_called_once_with( + b"_matrix._tcp.xn--bcher-kva.com", + ) + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, '1.2.3.4') + self.assertEqual(port, 8448) + + # make a test server, and wire up the client + http_server = self._make_connection( + client_factory, + expected_sni=b'xn--bcher-kva.com', + ) + + self.assertEqual(len(http_server.requests), 1) + request = http_server.requests[0] + self.assertEqual(request.method, b'GET') + self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'xn--bcher-kva.com'], + ) + + # finish the request + request.finish() + self.reactor.pump((0.1,)) + self.successResultOf(test_d) + + def test_idna_srv_target(self): + """test the behaviour when the target of a SRV record has idna chars""" + + self.mock_resolver.resolve_service.side_effect = lambda _: [ + Server(host=b"xn--trget-3qa.com", port=8443) # târget.com + ] + self.reactor.lookups[u"târget.com"] = "1.2.3.4" + + test_d = self._make_get_request(b"matrix://xn--bcher-kva.com/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + self.mock_resolver.resolve_service.assert_called_once_with( + b"_matrix._tcp.xn--bcher-kva.com", + ) + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, '1.2.3.4') + self.assertEqual(port, 8443) + + # make a test server, and wire up the client + http_server = self._make_connection( + client_factory, + expected_sni=b'xn--bcher-kva.com', + ) + + self.assertEqual(len(http_server.requests), 1) + request = http_server.requests[0] + self.assertEqual(request.method, b'GET') + self.assertEqual(request.path, b'/foo/bar') + self.assertEqual( + request.requestHeaders.getRawHeaders(b'host'), + [b'xn--bcher-kva.com'], + ) + + # finish the request + request.finish() + self.reactor.pump((0.1,)) + self.successResultOf(test_d) + def _check_logcontext(context): current = LoggingContext.current_context() From 51958df766667a75236764c9df57cfd60d57bd5e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 27 Jan 2019 23:24:17 +0000 Subject: [PATCH 11/15] MatrixFederationAgent: factor out routing logic This is going to get too big and unmanageable. --- .../federation/matrix_federation_agent.py | 80 ++++++++++++++----- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 9526f39cca..2a8bceed9a 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -14,6 +14,7 @@ # limitations under the License. import logging +import attr from zope.interface import implementer from twisted.internet import defer @@ -85,9 +86,11 @@ class MatrixFederationAgent(object): response from being received (including problems that prevent the request from being sent). """ - parsed_uri = URI.fromBytes(uri, defaultPort=-1) + res = yield self._route_matrix_uri(parsed_uri) + # set up the TLS connection params + # # XXX disabling TLS is really only supported here for the benefit of the # unit tests. We should make the UTs cope with TLS rather than having to make # the code support the unit tests. @@ -95,22 +98,9 @@ class MatrixFederationAgent(object): tls_options = None else: tls_options = self._tls_client_options_factory.get_options( - parsed_uri.host.decode("ascii") + res.tls_server_name.decode("ascii") ) - if parsed_uri.port != -1: - # there was an explicit port in the URI - target = parsed_uri.host, parsed_uri.port - else: - service_name = b"_matrix._tcp.%s" % (parsed_uri.host, ) - server_list = yield self._srv_resolver.resolve_service(service_name) - if not server_list: - target = (parsed_uri.host, 8448) - logger.debug( - "No SRV record for %s, using %s", service_name, target) - else: - target = pick_server_from_list(server_list) - # make sure that the Host header is set correctly if headers is None: headers = Headers() @@ -118,13 +108,13 @@ class MatrixFederationAgent(object): headers = headers.copy() if not headers.hasHeader(b'host'): - headers.addRawHeader(b'host', parsed_uri.netloc) + headers.addRawHeader(b'host', res.host_header) class EndpointFactory(object): @staticmethod def endpointForURI(_uri): - logger.info("Connecting to %s:%s", target[0], target[1]) - ep = HostnameEndpoint(self._reactor, host=target[0], port=target[1]) + logger.info("Connecting to %s:%s", res.target_host, res.target_port) + ep = HostnameEndpoint(self._reactor, res.target_host, res.target_port) if tls_options is not None: ep = wrapClientTLS(tls_options, ep) return ep @@ -134,3 +124,57 @@ class MatrixFederationAgent(object): agent.request(method, uri, headers, bodyProducer) ) defer.returnValue(res) + + @defer.inlineCallbacks + def _route_matrix_uri(self, parsed_uri): + """Helper for `request`: determine the routing for a Matrix URI + + Args: + parsed_uri (twisted.web.client.URI): uri to route. Note that it should be + parsed with URI.fromBytes(uri, defaultPort=-1) to set the `port` to -1 + if there is no explicit port given. + + Returns: + Deferred[_RoutingResult] + """ + if parsed_uri.port != -1: + # there is an explicit port + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=parsed_uri.host, + target_port=parsed_uri.port, + )) + + # try a SRV lookup + service_name = b"_matrix._tcp.%s" % (parsed_uri.host,) + server_list = yield self._srv_resolver.resolve_service(service_name) + + if not server_list: + target_host = parsed_uri.host + port = 8448 + logger.debug( + "No SRV record for %s, using %s:%i", + parsed_uri.host.decode("ascii"), target_host.decode("ascii"), port, + ) + else: + target_host, port = pick_server_from_list(server_list) + logger.debug( + "Picked %s:%i from SRV records for %s", + target_host.decode("ascii"), port, parsed_uri.host.decode("ascii"), + ) + + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=target_host, + target_port=port, + )) + + +@attr.s +class _RoutingResult(object): + host_header = attr.ib() + tls_server_name = attr.ib() + target_host = attr.ib() + target_port = attr.ib() From 0fd5b3b53e312a48d98afec27ff3686d8ffce199 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 26 Jan 2019 21:48:50 +0000 Subject: [PATCH 12/15] Handle IP literals explicitly We don't want to be doing .well-known lookups on these guys. --- .../federation/matrix_federation_agent.py | 19 +++++++++++++++++++ .../test_matrix_federation_agent.py | 19 ++----------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 2a8bceed9a..4d674cdb93 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -15,6 +15,7 @@ import logging import attr +from netaddr import IPAddress from zope.interface import implementer from twisted.internet import defer @@ -137,6 +138,24 @@ class MatrixFederationAgent(object): Returns: Deferred[_RoutingResult] """ + # check for an IP literal + try: + ip_address = IPAddress(parsed_uri.host.decode("ascii")) + except Exception: + # not an IP address + ip_address = None + + if ip_address: + port = parsed_uri.port + if port == -1: + port = 8448 + defer.returnValue(_RoutingResult( + host_header=parsed_uri.netloc, + tls_server_name=parsed_uri.host, + target_host=parsed_uri.host, + target_port=port, + )) + if parsed_uri.port != -1: # there is an explicit port defer.returnValue(_RoutingResult( diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index f144092a51..8257594fb8 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -166,11 +166,7 @@ class MatrixFederationAgentTests(TestCase): """ Test the behaviour when the server name contains an explicit IP (with no port) """ - - # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) - self.mock_resolver.resolve_service.side_effect = lambda _: [] - - # then there will be a getaddrinfo on the IP + # there will be a getaddrinfo on the IP self.reactor.lookups["1.2.3.4"] = "1.2.3.4" test_d = self._make_get_request(b"matrix://1.2.3.4/foo/bar") @@ -178,10 +174,6 @@ class MatrixFederationAgentTests(TestCase): # Nothing happened yet self.assertNoResult(test_d) - self.mock_resolver.resolve_service.assert_called_once_with( - b"_matrix._tcp.1.2.3.4", - ) - # Make sure treq is trying to connect clients = self.reactor.tcpClients self.assertEqual(len(clients), 1) @@ -215,10 +207,7 @@ class MatrixFederationAgentTests(TestCase): (with no port) """ - # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) - self.mock_resolver.resolve_service.side_effect = lambda _: [] - - # then there will be a getaddrinfo on the IP + # there will be a getaddrinfo on the IP self.reactor.lookups["::1"] = "::1" test_d = self._make_get_request(b"matrix://[::1]/foo/bar") @@ -226,10 +215,6 @@ class MatrixFederationAgentTests(TestCase): # Nothing happened yet self.assertNoResult(test_d) - self.mock_resolver.resolve_service.assert_called_once_with( - b"_matrix._tcp.::1", - ) - # Make sure treq is trying to connect clients = self.reactor.tcpClients self.assertEqual(len(clients), 1) From ff05ad147a2b62da9a4b7f00c8e8c298da06fa52 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 27 Jan 2019 23:54:50 +0000 Subject: [PATCH 13/15] changelog --- changelog.d/4488.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4488.feature diff --git a/changelog.d/4488.feature b/changelog.d/4488.feature new file mode 100644 index 0000000000..bda713adf9 --- /dev/null +++ b/changelog.d/4488.feature @@ -0,0 +1 @@ +Implement MSC1708 (.well-known routing for server-server federation) \ No newline at end of file From 3bd0f1a4a3b735794f4a19d352b36c2e86e86dc0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 28 Jan 2019 12:43:09 +0000 Subject: [PATCH 14/15] docstrings for _RoutingResult --- .../federation/matrix_federation_agent.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/synapse/http/federation/matrix_federation_agent.py b/synapse/http/federation/matrix_federation_agent.py index 4d674cdb93..4a6f634c8b 100644 --- a/synapse/http/federation/matrix_federation_agent.py +++ b/synapse/http/federation/matrix_federation_agent.py @@ -193,7 +193,43 @@ class MatrixFederationAgent(object): @attr.s class _RoutingResult(object): + """The result returned by `_route_matrix_uri`. + + Contains the parameters needed to direct a federation connection to a particular + server. + + Where a SRV record points to several servers, this object contains a single server + chosen from the list. + """ + host_header = attr.ib() + """ + The value we should assign to the Host header (host:port from the matrix + URI, or .well-known). + + :type: bytes + """ + tls_server_name = attr.ib() + """ + The server name we should set in the SNI (typically host, without port, from the + matrix URI or .well-known) + + :type: bytes + """ + target_host = attr.ib() + """ + The hostname (or IP literal) we should route the TCP connection to (the target of the + SRV record, or the hostname from the URL/.well-known) + + :type: bytes + """ + target_port = attr.ib() + """ + The port we should route the TCP connection to (the target of the SRV record, or + the port from the URL/.well-known, or 8448) + + :type: int + """ From 57a3e96e8ebf4a424e973c325e3b4e25cbdc27f1 Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Mon, 28 Jan 2019 07:54:25 -0600 Subject: [PATCH 15/15] Remove --process-dependency-links from UPGRADE.rst (#4485) * Remove --process-dependency-links from UPGRADE.rst Signed-off-by: Aaron Raimist * Add changelog Signed-off-by: Aaron Raimist --- UPGRADE.rst | 2 +- changelog.d/4485.misc | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/4485.misc diff --git a/UPGRADE.rst b/UPGRADE.rst index b214a15526..47a8cb9c88 100644 --- a/UPGRADE.rst +++ b/UPGRADE.rst @@ -18,7 +18,7 @@ instructions that may be required are listed later in this document. .. code:: bash - pip install --upgrade --process-dependency-links matrix-synapse + pip install --upgrade matrix-synapse # restart synapse synctl restart diff --git a/changelog.d/4485.misc b/changelog.d/4485.misc new file mode 100644 index 0000000000..8aa0aeab2a --- /dev/null +++ b/changelog.d/4485.misc @@ -0,0 +1 @@ +Remove deprecated --process-dependency-links option from UPGRADE.rst