From 3406333a58d9887cdb79ef73cae218c705ac11b0 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 9 Mar 2017 12:20:46 +0000 Subject: [PATCH 01/13] Factor _get_missing_events_for_pdu out of _handle_new_pdu This should be functionally identical: it just seeks to improve readability by reducing indentation. --- synapse/federation/federation_server.py | 144 ++++++++++++++---------- 1 file changed, 82 insertions(+), 62 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index e922b7ff4..3ef700f7f 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -574,68 +574,9 @@ class FederationServer(FederationBase): pdu.room_id, len(prevs - seen), ) - # We recalculate seen, since it may have changed. - have_seen = yield self.store.have_events(prevs) - seen = set(have_seen.keys()) - - if prevs - seen: - latest = yield self.store.get_latest_event_ids_in_room( - pdu.room_id - ) - - # We add the prev events that we have seen to the latest - # list to ensure the remote server doesn't give them to us - latest = set(latest) - latest |= seen - - logger.info( - "Missing %d events for room %r: %r...", - len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] - ) - - # XXX: we set timeout to 10s to help workaround - # https://github.com/matrix-org/synapse/issues/1733. - # The reason is to avoid holding the linearizer lock - # whilst processing inbound /send transactions, causing - # FDs to stack up and block other inbound transactions - # which empirically can currently take up to 30 minutes. - # - # N.B. this explicitly disables retry attempts. - # - # N.B. this also increases our chances of falling back to - # fetching fresh state for the room if the missing event - # can't be found, which slightly reduces our security. - # it may also increase our DAG extremity count for the room, - # causing additional state resolution? See #1760. - # However, fetching state doesn't hold the linearizer lock - # apparently. - # - # see https://github.com/matrix-org/synapse/pull/1744 - - missing_events = yield self.get_missing_events( - origin, - pdu.room_id, - earliest_events_ids=list(latest), - latest_events=[pdu], - limit=10, - min_depth=min_depth, - timeout=10000, - ) - - # We want to sort these by depth so we process them and - # tell clients about them in order. - missing_events.sort(key=lambda x: x.depth) - - for e in missing_events: - yield self._handle_new_pdu( - origin, - e, - get_missing=False - ) - - have_seen = yield self.store.have_events( - [ev for ev, _ in pdu.prev_events] - ) + yield self._get_missing_events_for_pdu( + origin, pdu, prevs, min_depth + ) prevs = {e_id for e_id, _ in pdu.prev_events} seen = set(have_seen.keys()) @@ -667,6 +608,85 @@ class FederationServer(FederationBase): auth_chain=auth_chain, ) + @defer.inlineCallbacks + def _get_missing_events_for_pdu(self, origin, pdu, prevs, min_depth): + """ + Args: + origin (str): Origin of the pdu. Will be called to get the missing events + pdu: received pdu + prevs (str[]): List of event ids which we are missing + min_depth (int): Minimum depth of events to return. + + Returns: + Deferred: updated have_seen dictionary + """ + # We recalculate seen, since it may have changed. + have_seen = yield self.store.have_events(prevs) + seen = set(have_seen.keys()) + + if not prevs - seen: + # nothing left to do + defer.returnValue(have_seen) + + latest = yield self.store.get_latest_event_ids_in_room( + pdu.room_id + ) + + # We add the prev events that we have seen to the latest + # list to ensure the remote server doesn't give them to us + latest = set(latest) + latest |= seen + + logger.info( + "Missing %d events for room %r: %r...", + len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] + ) + + # XXX: we set timeout to 10s to help workaround + # https://github.com/matrix-org/synapse/issues/1733. + # The reason is to avoid holding the linearizer lock + # whilst processing inbound /send transactions, causing + # FDs to stack up and block other inbound transactions + # which empirically can currently take up to 30 minutes. + # + # N.B. this explicitly disables retry attempts. + # + # N.B. this also increases our chances of falling back to + # fetching fresh state for the room if the missing event + # can't be found, which slightly reduces our security. + # it may also increase our DAG extremity count for the room, + # causing additional state resolution? See #1760. + # However, fetching state doesn't hold the linearizer lock + # apparently. + # + # see https://github.com/matrix-org/synapse/pull/1744 + + missing_events = yield self.get_missing_events( + origin, + pdu.room_id, + earliest_events_ids=list(latest), + latest_events=[pdu], + limit=10, + min_depth=min_depth, + timeout=10000, + ) + + # We want to sort these by depth so we process them and + # tell clients about them in order. + missing_events.sort(key=lambda x: x.depth) + + for e in missing_events: + yield self._handle_new_pdu( + origin, + e, + get_missing=False + ) + + have_seen = yield self.store.have_events( + [ev for ev, _ in pdu.prev_events] + ) + defer.returnValue(have_seen) + def __str__(self): return "" % self.server_name From e8b1721290b196f9190f72517e8136d6af8cad58 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 9 Mar 2017 13:15:27 +0000 Subject: [PATCH 02/13] Move sig check out of _handle_new_pdu When we receive PDUs via `get_missing_events`, we have already checked their sigs, so there is no need to do it again. --- synapse/federation/federation_server.py | 48 ++++++++++++++++++------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 3ef700f7f..d8df6555d 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -165,7 +165,7 @@ class FederationServer(FederationBase): ) try: - yield self._handle_new_pdu(transaction.origin, pdu) + yield self._handle_received_pdu(transaction.origin, pdu) results.append({}) except FederationError as e: self.send_failure(e, transaction.origin) @@ -496,9 +496,44 @@ class FederationServer(FederationBase): destination=None, ) + @defer.inlineCallbacks + def _handle_received_pdu(self, origin, pdu): + """ Process a PDU received in a federation /send/ transaction. + + Args: + origin (str): server which sent the pdu + pdu (FrozenEvent): received pdu + + Returns (Deferred): completes with None + Raises: FederationError if the signatures / hash do not match + """ + # Check signature. + try: + pdu = yield self._check_sigs_and_hash(pdu) + except SynapseError as e: + raise FederationError( + "ERROR", + e.code, + e.msg, + affected=pdu.event_id, + ) + + yield self._handle_new_pdu(origin, pdu, get_missing=True) + @defer.inlineCallbacks @log_function def _handle_new_pdu(self, origin, pdu, get_missing=True): + """ Process a PDU received via a federation /send/ transaction, or + via backfill of missing prev_events + + Args: + origin (str): server which initiated the /send/ transaction. Will + be used to fetch missing events or state. + pdu (FrozenEvent): received PDU + get_missing (bool): True if we should fetch missing prev_events + + Returns (Deferred): completes with None + """ # We reprocess pdus when we have seen them only as outliers existing = yield self._get_persisted_pdu( @@ -518,17 +553,6 @@ class FederationServer(FederationBase): logger.debug("Already seen pdu %s", pdu.event_id) return - # Check signature. - try: - pdu = yield self._check_sigs_and_hash(pdu) - except SynapseError as e: - raise FederationError( - "ERROR", - e.code, - e.msg, - affected=pdu.event_id, - ) - state = None auth_chain = [] From 29235901b81b344fc28ff9f59c36257afecf0265 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 9 Mar 2017 16:20:13 +0000 Subject: [PATCH 03/13] Move FederationServer._handle_new_pdu to FederationHandler Unfortunately this significantly increases the size of the already-rather-big FederationHandler, but the code fits more naturally here, and it paves the way for the tighter integration that I need between handling incoming PDUs and doing the join dance. Other than renaming the existing `FederationHandler.on_receive_pdu` to `_process_received_pdu` to make way for it, this just consists of the move, and replacing `self.handler` with `self` and `self` with `self.replication_layer`. --- synapse/federation/federation_server.py | 194 +---------------------- synapse/handlers/federation.py | 202 +++++++++++++++++++++++- 2 files changed, 198 insertions(+), 198 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index d8df6555d..510a17682 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -52,7 +52,6 @@ class FederationServer(FederationBase): self.auth = hs.get_auth() - self._room_pdu_linearizer = Linearizer("fed_room_pdu") self._server_linearizer = Linearizer("fed_server") # We cache responses to state queries, as they take a while and often @@ -518,198 +517,7 @@ class FederationServer(FederationBase): affected=pdu.event_id, ) - yield self._handle_new_pdu(origin, pdu, get_missing=True) - - @defer.inlineCallbacks - @log_function - def _handle_new_pdu(self, origin, pdu, get_missing=True): - """ Process a PDU received via a federation /send/ transaction, or - via backfill of missing prev_events - - Args: - origin (str): server which initiated the /send/ transaction. Will - be used to fetch missing events or state. - pdu (FrozenEvent): received PDU - get_missing (bool): True if we should fetch missing prev_events - - Returns (Deferred): completes with None - """ - - # We reprocess pdus when we have seen them only as outliers - existing = yield self._get_persisted_pdu( - origin, pdu.event_id, do_auth=False - ) - - # FIXME: Currently we fetch an event again when we already have it - # if it has been marked as an outlier. - - already_seen = ( - existing and ( - not existing.internal_metadata.is_outlier() - or pdu.internal_metadata.is_outlier() - ) - ) - if already_seen: - logger.debug("Already seen pdu %s", pdu.event_id) - return - - state = None - - auth_chain = [] - - have_seen = yield self.store.have_events( - [ev for ev, _ in pdu.prev_events] - ) - - fetch_state = False - - # Get missing pdus if necessary. - if not pdu.internal_metadata.is_outlier(): - # We only backfill backwards to the min depth. - min_depth = yield self.handler.get_min_depth_for_context( - pdu.room_id - ) - - logger.debug( - "_handle_new_pdu min_depth for %s: %d", - pdu.room_id, min_depth - ) - - prevs = {e_id for e_id, _ in pdu.prev_events} - seen = set(have_seen.keys()) - - if min_depth and pdu.depth < min_depth: - # This is so that we don't notify the user about this - # message, to work around the fact that some events will - # reference really really old events we really don't want to - # send to the clients. - pdu.internal_metadata.outlier = True - elif min_depth and pdu.depth > min_depth: - if get_missing and prevs - seen: - # If we're missing stuff, ensure we only fetch stuff one - # at a time. - logger.info( - "Acquiring lock for room %r to fetch %d missing events: %r...", - pdu.room_id, len(prevs - seen), list(prevs - seen)[:5], - ) - with (yield self._room_pdu_linearizer.queue(pdu.room_id)): - logger.info( - "Acquired lock for room %r to fetch %d missing events", - pdu.room_id, len(prevs - seen), - ) - - yield self._get_missing_events_for_pdu( - origin, pdu, prevs, min_depth - ) - - prevs = {e_id for e_id, _ in pdu.prev_events} - seen = set(have_seen.keys()) - if prevs - seen: - logger.info( - "Still missing %d events for room %r: %r...", - len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] - ) - fetch_state = True - - if fetch_state: - # We need to get the state at this event, since we haven't - # processed all the prev events. - logger.debug( - "_handle_new_pdu getting state for %s", - pdu.room_id - ) - try: - state, auth_chain = yield self.get_state_for_room( - origin, pdu.room_id, pdu.event_id, - ) - except: - logger.exception("Failed to get state for event: %s", pdu.event_id) - - yield self.handler.on_receive_pdu( - origin, - pdu, - state=state, - auth_chain=auth_chain, - ) - - @defer.inlineCallbacks - def _get_missing_events_for_pdu(self, origin, pdu, prevs, min_depth): - """ - Args: - origin (str): Origin of the pdu. Will be called to get the missing events - pdu: received pdu - prevs (str[]): List of event ids which we are missing - min_depth (int): Minimum depth of events to return. - - Returns: - Deferred: updated have_seen dictionary - """ - # We recalculate seen, since it may have changed. - have_seen = yield self.store.have_events(prevs) - seen = set(have_seen.keys()) - - if not prevs - seen: - # nothing left to do - defer.returnValue(have_seen) - - latest = yield self.store.get_latest_event_ids_in_room( - pdu.room_id - ) - - # We add the prev events that we have seen to the latest - # list to ensure the remote server doesn't give them to us - latest = set(latest) - latest |= seen - - logger.info( - "Missing %d events for room %r: %r...", - len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] - ) - - # XXX: we set timeout to 10s to help workaround - # https://github.com/matrix-org/synapse/issues/1733. - # The reason is to avoid holding the linearizer lock - # whilst processing inbound /send transactions, causing - # FDs to stack up and block other inbound transactions - # which empirically can currently take up to 30 minutes. - # - # N.B. this explicitly disables retry attempts. - # - # N.B. this also increases our chances of falling back to - # fetching fresh state for the room if the missing event - # can't be found, which slightly reduces our security. - # it may also increase our DAG extremity count for the room, - # causing additional state resolution? See #1760. - # However, fetching state doesn't hold the linearizer lock - # apparently. - # - # see https://github.com/matrix-org/synapse/pull/1744 - - missing_events = yield self.get_missing_events( - origin, - pdu.room_id, - earliest_events_ids=list(latest), - latest_events=[pdu], - limit=10, - min_depth=min_depth, - timeout=10000, - ) - - # We want to sort these by depth so we process them and - # tell clients about them in order. - missing_events.sort(key=lambda x: x.depth) - - for e in missing_events: - yield self._handle_new_pdu( - origin, - e, - get_missing=False - ) - - have_seen = yield self.store.have_events( - [ev for ev, _ in pdu.prev_events] - ) - defer.returnValue(have_seen) + yield self.handler.on_receive_pdu(origin, pdu, get_missing=True) def __str__(self): return "" % self.server_name diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ed0fa51e7..d0c2b4d6e 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -31,7 +31,7 @@ from synapse.util.logcontext import ( ) from synapse.util.metrics import measure_func from synapse.util.logutils import log_function -from synapse.util.async import run_on_reactor +from synapse.util.async import run_on_reactor, Linearizer from synapse.util.frozenutils import unfreeze from synapse.crypto.event_signing import ( compute_event_signature, add_hashes_and_signatures, @@ -79,12 +79,204 @@ class FederationHandler(BaseHandler): # When joining a room we need to queue any events for that room up self.room_queues = {} + self._room_pdu_linearizer = Linearizer("fed_room_pdu") + + @defer.inlineCallbacks + @log_function + def on_receive_pdu(self, origin, pdu, get_missing=True): + """ Process a PDU received via a federation /send/ transaction, or + via backfill of missing prev_events + + Args: + origin (str): server which initiated the /send/ transaction. Will + be used to fetch missing events or state. + pdu (FrozenEvent): received PDU + get_missing (bool): True if we should fetch missing prev_events + + Returns (Deferred): completes with None + """ + + # We reprocess pdus when we have seen them only as outliers + existing = yield self.get_persisted_pdu( + origin, pdu.event_id, do_auth=False + ) + + # FIXME: Currently we fetch an event again when we already have it + # if it has been marked as an outlier. + + already_seen = ( + existing and ( + not existing.internal_metadata.is_outlier() + or pdu.internal_metadata.is_outlier() + ) + ) + if already_seen: + logger.debug("Already seen pdu %s", pdu.event_id) + return + + state = None + + auth_chain = [] + + have_seen = yield self.store.have_events( + [ev for ev, _ in pdu.prev_events] + ) + + fetch_state = False + + # Get missing pdus if necessary. + if not pdu.internal_metadata.is_outlier(): + # We only backfill backwards to the min depth. + min_depth = yield self.get_min_depth_for_context( + pdu.room_id + ) + + logger.debug( + "_handle_new_pdu min_depth for %s: %d", + pdu.room_id, min_depth + ) + + prevs = {e_id for e_id, _ in pdu.prev_events} + seen = set(have_seen.keys()) + + if min_depth and pdu.depth < min_depth: + # This is so that we don't notify the user about this + # message, to work around the fact that some events will + # reference really really old events we really don't want to + # send to the clients. + pdu.internal_metadata.outlier = True + elif min_depth and pdu.depth > min_depth: + if get_missing and prevs - seen: + # If we're missing stuff, ensure we only fetch stuff one + # at a time. + logger.info( + "Acquiring lock for room %r to fetch %d missing events: %r...", + pdu.room_id, len(prevs - seen), list(prevs - seen)[:5], + ) + with (yield self._room_pdu_linearizer.queue(pdu.room_id)): + logger.info( + "Acquired lock for room %r to fetch %d missing events", + pdu.room_id, len(prevs - seen), + ) + + yield self._get_missing_events_for_pdu( + origin, pdu, prevs, min_depth + ) + + prevs = {e_id for e_id, _ in pdu.prev_events} + seen = set(have_seen.keys()) + if prevs - seen: + logger.info( + "Still missing %d events for room %r: %r...", + len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] + ) + fetch_state = True + + if fetch_state: + # We need to get the state at this event, since we haven't + # processed all the prev events. + logger.debug( + "_handle_new_pdu getting state for %s", + pdu.room_id + ) + try: + state, auth_chain = yield self.replication_layer.get_state_for_room( + origin, pdu.room_id, pdu.event_id, + ) + except: + logger.exception("Failed to get state for event: %s", pdu.event_id) + + yield self._process_received_pdu( + origin, + pdu, + state=state, + auth_chain=auth_chain, + ) + + @defer.inlineCallbacks + def _get_missing_events_for_pdu(self, origin, pdu, prevs, min_depth): + """ + Args: + origin (str): Origin of the pdu. Will be called to get the missing events + pdu: received pdu + prevs (str[]): List of event ids which we are missing + min_depth (int): Minimum depth of events to return. + + Returns: + Deferred: updated have_seen dictionary + """ + # We recalculate seen, since it may have changed. + have_seen = yield self.store.have_events(prevs) + seen = set(have_seen.keys()) + + if not prevs - seen: + # nothing left to do + defer.returnValue(have_seen) + + latest = yield self.store.get_latest_event_ids_in_room( + pdu.room_id + ) + + # We add the prev events that we have seen to the latest + # list to ensure the remote server doesn't give them to us + latest = set(latest) + latest |= seen + + logger.info( + "Missing %d events for room %r: %r...", + len(prevs - seen), pdu.room_id, list(prevs - seen)[:5] + ) + + # XXX: we set timeout to 10s to help workaround + # https://github.com/matrix-org/synapse/issues/1733. + # The reason is to avoid holding the linearizer lock + # whilst processing inbound /send transactions, causing + # FDs to stack up and block other inbound transactions + # which empirically can currently take up to 30 minutes. + # + # N.B. this explicitly disables retry attempts. + # + # N.B. this also increases our chances of falling back to + # fetching fresh state for the room if the missing event + # can't be found, which slightly reduces our security. + # it may also increase our DAG extremity count for the room, + # causing additional state resolution? See #1760. + # However, fetching state doesn't hold the linearizer lock + # apparently. + # + # see https://github.com/matrix-org/synapse/pull/1744 + + missing_events = yield self.replication_layer.get_missing_events( + origin, + pdu.room_id, + earliest_events_ids=list(latest), + latest_events=[pdu], + limit=10, + min_depth=min_depth, + timeout=10000, + ) + + # We want to sort these by depth so we process them and + # tell clients about them in order. + missing_events.sort(key=lambda x: x.depth) + + for e in missing_events: + yield self.on_receive_pdu( + origin, + e, + get_missing=False + ) + + have_seen = yield self.store.have_events( + [ev for ev, _ in pdu.prev_events] + ) + defer.returnValue(have_seen) @log_function @defer.inlineCallbacks - def on_receive_pdu(self, origin, pdu, state=None, auth_chain=None): - """ Called by the ReplicationLayer when we have a new pdu. We need to - do auth checks and put it through the StateHandler. + def _process_received_pdu(self, origin, pdu, state=None, auth_chain=None): + """ Called when we have a new pdu. We need to do auth checks and put it + through the StateHandler. auth_chain and state are None if we already have the necessary state and prev_events in the db @@ -738,7 +930,7 @@ class FederationHandler(BaseHandler): continue try: - self.on_receive_pdu(origin, p) + self._process_received_pdu(origin, p) except: logger.exception("Couldn't handle pdu") From 9072a8c6279812e50ad1d3129efc564ce531acbb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 9 Mar 2017 22:54:52 +0000 Subject: [PATCH 04/13] Reread log config on SIGHUP When we are using a log_config file, reread it on SIGHUP. --- synapse/config/logger.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/synapse/config/logger.py b/synapse/config/logger.py index 77ded0ad2..e1f060d40 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -153,14 +153,6 @@ def setup_logging(log_config=None, log_file=None, verbosity=None): logger.info("Closing log file due to SIGHUP") handler.doRollover() logger.info("Opened new log file due to SIGHUP") - - # TODO(paul): obviously this is a terrible mechanism for - # stealing SIGHUP, because it means no other part of synapse - # can use it instead. If we want to catch SIGHUP anywhere - # else as well, I'd suggest we find a nicer way to broadcast - # it around. - if getattr(signal, "SIGHUP"): - signal.signal(signal.SIGHUP, sighup) else: handler = logging.StreamHandler() handler.setFormatter(formatter) @@ -169,8 +161,25 @@ def setup_logging(log_config=None, log_file=None, verbosity=None): logger.addHandler(handler) else: - with open(log_config, 'r') as f: - logging.config.dictConfig(yaml.load(f)) + def load_log_config(): + with open(log_config, 'r') as f: + logging.config.dictConfig(yaml.load(f)) + + def sighup(signum, stack): + # it might be better to use a file watcher or something for this. + logging.info("Reloading log config from %s due to SIGHUP", + log_config) + load_log_config() + + load_log_config() + + # TODO(paul): obviously this is a terrible mechanism for + # stealing SIGHUP, because it means no other part of synapse + # can use it instead. If we want to catch SIGHUP anywhere + # else as well, I'd suggest we find a nicer way to broadcast + # it around. + if getattr(signal, "SIGHUP"): + signal.signal(signal.SIGHUP, sighup) # It's critical to point twisted's internal logging somewhere, otherwise it # stacks up and leaks kup to 64K object; From d84bd51e95149f67e0740657d60b568533a6ea72 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 10 Mar 2017 15:16:50 +0000 Subject: [PATCH 05/13] Refactor logger config for workers - to make it easier to add more config options. --- synapse/app/appservice.py | 2 +- synapse/app/client_reader.py | 2 +- synapse/app/federation_reader.py | 2 +- synapse/app/federation_sender.py | 2 +- synapse/app/homeserver.py | 4 +++- synapse/app/media_repository.py | 2 +- synapse/app/pusher.py | 2 +- synapse/app/synchrotron.py | 2 +- synapse/config/logger.py | 21 ++++++++++++++++----- 9 files changed, 26 insertions(+), 13 deletions(-) diff --git a/synapse/app/appservice.py b/synapse/app/appservice.py index 190093005..83ee3e3ce 100644 --- a/synapse/app/appservice.py +++ b/synapse/app/appservice.py @@ -157,7 +157,7 @@ def start(config_options): assert config.worker_app == "synapse.app.appservice" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/client_reader.py b/synapse/app/client_reader.py index 4d081eccd..7ed0de411 100644 --- a/synapse/app/client_reader.py +++ b/synapse/app/client_reader.py @@ -171,7 +171,7 @@ def start(config_options): assert config.worker_app == "synapse.app.client_reader" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/federation_reader.py b/synapse/app/federation_reader.py index 90a481675..ca742de6b 100644 --- a/synapse/app/federation_reader.py +++ b/synapse/app/federation_reader.py @@ -162,7 +162,7 @@ def start(config_options): assert config.worker_app == "synapse.app.federation_reader" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/federation_sender.py b/synapse/app/federation_sender.py index 411e47d98..0cf5b196e 100644 --- a/synapse/app/federation_sender.py +++ b/synapse/app/federation_sender.py @@ -160,7 +160,7 @@ def start(config_options): assert config.worker_app == "synapse.app.federation_sender" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index e0b87468f..0b9d78c13 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -20,6 +20,8 @@ import gc import logging import os import sys + +import synapse.config.logger from synapse.config._base import ConfigError from synapse.python_dependencies import ( @@ -286,7 +288,7 @@ def setup(config_options): # generating config files and shouldn't try to continue. sys.exit(0) - config.setup_logging() + synapse.config.logger.setup_logging(config, use_worker_options=False) # check any extra requirements we have now we have a config check_requirements(config) diff --git a/synapse/app/media_repository.py b/synapse/app/media_repository.py index ef17b158a..c5579d9e3 100644 --- a/synapse/app/media_repository.py +++ b/synapse/app/media_repository.py @@ -168,7 +168,7 @@ def start(config_options): assert config.worker_app == "synapse.app.media_repository" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/pusher.py b/synapse/app/pusher.py index 073f2c248..b025db54d 100644 --- a/synapse/app/pusher.py +++ b/synapse/app/pusher.py @@ -245,7 +245,7 @@ def start(config_options): assert config.worker_app == "synapse.app.pusher" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/app/synchrotron.py b/synapse/app/synchrotron.py index 3f2959525..29f075aa5 100644 --- a/synapse/app/synchrotron.py +++ b/synapse/app/synchrotron.py @@ -478,7 +478,7 @@ def start(config_options): assert config.worker_app == "synapse.app.synchrotron" - setup_logging(config.worker_log_config, config.worker_log_file) + setup_logging(config, use_worker_options=True) synapse.events.USE_FROZEN_DICTS = config.use_frozen_dicts diff --git a/synapse/config/logger.py b/synapse/config/logger.py index 77ded0ad2..c76fddf11 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -118,11 +118,22 @@ class LoggingConfig(Config): DEFAULT_LOG_CONFIG.substitute(log_file=config["log_file"]) ) - def setup_logging(self): - setup_logging(self.log_config, self.log_file, self.verbosity) +def setup_logging(config, use_worker_options=False): + """ Set up python logging + + Args: + config (LoggingConfig | synapse.config.workers.WorkerConfig): + configuration data + + use_worker_options (bool): True to use 'worker_log_config' and + 'worker_log_file' options instead of 'log_config' and 'log_file'. + """ + log_config = (config.worker_log_config if use_worker_options + else config.log_config) + log_file = (config.worker_log_file if use_worker_options + else config.log_file) -def setup_logging(log_config=None, log_file=None, verbosity=None): log_format = ( "%(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s" " - %(message)s" @@ -131,9 +142,9 @@ def setup_logging(log_config=None, log_file=None, verbosity=None): level = logging.INFO level_for_storage = logging.INFO - if verbosity: + if config.verbosity: level = logging.DEBUG - if verbosity > 1: + if config.verbosity > 1: level_for_storage = logging.DEBUG # FIXME: we need a logging.WARN for a -q quiet option From bcfa5cd00c4d5827a9a525078db32a0c93bc087c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 10 Mar 2017 15:38:29 +0000 Subject: [PATCH 06/13] Add an option to disable stdio redirect This makes it tractable to run synapse under pdb. --- synapse/config/logger.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/synapse/config/logger.py b/synapse/config/logger.py index c76fddf11..a48f7695e 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -68,6 +68,7 @@ class LoggingConfig(Config): def read_config(self, config): self.verbosity = config.get("verbose", 0) + self.no_redirect_stdio = config.get("no_redirect_stdio", False) self.log_config = self.abspath(config.get("log_config")) self.log_file = self.abspath(config.get("log_file")) @@ -90,6 +91,8 @@ class LoggingConfig(Config): def read_arguments(self, args): if args.verbose is not None: self.verbosity = args.verbose + if args.no_redirect_stdio is not None: + self.no_redirect_stdio = args.no_redirect_stdio if args.log_config is not None: self.log_config = args.log_config if args.log_file is not None: @@ -109,6 +112,11 @@ class LoggingConfig(Config): '--log-config', dest="log_config", default=None, help="Python logging config file" ) + logging_group.add_argument( + '-n', '--no-redirect-stdio', + action='store_true', default=None, + help="Do not redirect stdout/stderr to the log" + ) def generate_files(self, config): log_config = config.get("log_config") @@ -194,4 +202,7 @@ def setup_logging(config, use_worker_options=False): # # However this may not be too much of a problem if we are just writing to a file. observer = STDLibLogObserver() - globalLogBeginner.beginLoggingTo([observer]) + globalLogBeginner.beginLoggingTo( + [observer], + redirectStandardIO=not config.no_redirect_stdio, + ) From 7eae6eaa2f12474060f0bae03b4aa5bab9c9e1f7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 13 Mar 2017 09:58:39 +0000 Subject: [PATCH 07/13] Revert "Support registration & login with phone number" --- synapse/api/constants.py | 2 - synapse/handlers/auth.py | 32 ++---- synapse/handlers/identity.py | 37 +------ synapse/http/servlet.py | 10 -- synapse/python_dependencies.py | 2 - synapse/rest/client/v1/login.py | 88 ++--------------- synapse/rest/client/v2_alpha/account.py | 114 ++++----------------- synapse/rest/client/v2_alpha/register.py | 120 +++-------------------- synapse/util/msisdn.py | 40 -------- 9 files changed, 50 insertions(+), 395 deletions(-) delete mode 100644 synapse/util/msisdn.py diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 489efb7f8..ca23c9c46 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- # Copyright 2014-2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -45,7 +44,6 @@ class JoinRules(object): class LoginType(object): PASSWORD = u"m.login.password" EMAIL_IDENTITY = u"m.login.email.identity" - MSISDN = u"m.login.msisdn" RECAPTCHA = u"m.login.recaptcha" DUMMY = u"m.login.dummy" diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index e7a1bb724..fffba3438 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- # Copyright 2014 - 2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -48,7 +47,6 @@ class AuthHandler(BaseHandler): LoginType.PASSWORD: self._check_password_auth, LoginType.RECAPTCHA: self._check_recaptcha, LoginType.EMAIL_IDENTITY: self._check_email_identity, - LoginType.MSISDN: self._check_msisdn, LoginType.DUMMY: self._check_dummy_auth, } self.bcrypt_rounds = hs.config.bcrypt_rounds @@ -309,47 +307,31 @@ class AuthHandler(BaseHandler): defer.returnValue(True) raise LoginError(401, "", errcode=Codes.UNAUTHORIZED) + @defer.inlineCallbacks def _check_email_identity(self, authdict, _): - return self._check_threepid('email', authdict) - - def _check_msisdn(self, authdict, _): - return self._check_threepid('msisdn', authdict) - - @defer.inlineCallbacks - def _check_dummy_auth(self, authdict, _): - yield run_on_reactor() - defer.returnValue(True) - - @defer.inlineCallbacks - def _check_threepid(self, medium, authdict): yield run_on_reactor() if 'threepid_creds' not in authdict: raise LoginError(400, "Missing threepid_creds", Codes.MISSING_PARAM) threepid_creds = authdict['threepid_creds'] - identity_handler = self.hs.get_handlers().identity_handler - logger.info("Getting validated threepid. threepidcreds: %r", (threepid_creds,)) + logger.info("Getting validated threepid. threepidcreds: %r" % (threepid_creds,)) threepid = yield identity_handler.threepid_from_creds(threepid_creds) if not threepid: raise LoginError(401, "", errcode=Codes.UNAUTHORIZED) - if threepid['medium'] != medium: - raise LoginError( - 401, - "Expecting threepid of type '%s', got '%s'" % ( - medium, threepid['medium'], - ), - errcode=Codes.UNAUTHORIZED - ) - threepid['threepid_creds'] = authdict['threepid_creds'] defer.returnValue(threepid) + @defer.inlineCallbacks + def _check_dummy_auth(self, authdict, _): + yield run_on_reactor() + defer.returnValue(True) + def _get_params_recaptcha(self): return {"public_key": self.hs.config.recaptcha_public_key} diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 6a53c5eb4..559e5d5a7 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- # Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -151,7 +150,7 @@ class IdentityHandler(BaseHandler): params.update(kwargs) try: - data = yield self.http_client.post_json_get_json( + data = yield self.http_client.post_urlencoded_get_json( "https://%s%s" % ( id_server, "/_matrix/identity/api/v1/validate/email/requestToken" @@ -162,37 +161,3 @@ class IdentityHandler(BaseHandler): except CodeMessageException as e: logger.info("Proxied requestToken failed: %r", e) raise e - - @defer.inlineCallbacks - def requestMsisdnToken( - self, id_server, country, phone_number, - client_secret, send_attempt, **kwargs - ): - yield run_on_reactor() - - if not self._should_trust_id_server(id_server): - raise SynapseError( - 400, "Untrusted ID server '%s'" % id_server, - Codes.SERVER_NOT_TRUSTED - ) - - params = { - 'country': country, - 'phone_number': phone_number, - 'client_secret': client_secret, - 'send_attempt': send_attempt, - } - params.update(kwargs) - - try: - data = yield self.http_client.post_json_get_json( - "https://%s%s" % ( - id_server, - "/_matrix/identity/api/v1/validate/msisdn/requestToken" - ), - params - ) - defer.returnValue(data) - except CodeMessageException as e: - logger.info("Proxied requestToken failed: %r", e) - raise e diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 9a4c36ad5..8c22d6f00 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -192,16 +192,6 @@ def parse_json_object_from_request(request): return content -def assert_params_in_request(body, required): - absent = [] - for k in required: - if k not in body: - absent.append(k) - - if len(absent) > 0: - raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM) - - class RestServlet(object): """ A Synapse REST Servlet. diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index c4777b2a2..7817b0cd9 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -1,5 +1,4 @@ # Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -38,7 +37,6 @@ REQUIREMENTS = { "pysaml2>=3.0.0,<4.0.0": ["saml2>=3.0.0,<4.0.0"], "pymacaroons-pynacl": ["pymacaroons"], "msgpack-python>=0.3.0": ["msgpack"], - "phonenumbers>=8.2.0": ["phonenumbers"], } CONDITIONAL_REQUIREMENTS = { "web_client": { diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py index c4bbb7027..72057f1b0 100644 --- a/synapse/rest/client/v1/login.py +++ b/synapse/rest/client/v1/login.py @@ -19,7 +19,6 @@ from synapse.api.errors import SynapseError, LoginError, Codes from synapse.types import UserID from synapse.http.server import finish_request from synapse.http.servlet import parse_json_object_from_request -from synapse.util.msisdn import phone_number_to_msisdn from .base import ClientV1RestServlet, client_path_patterns @@ -38,49 +37,6 @@ import xml.etree.ElementTree as ET logger = logging.getLogger(__name__) -def login_submission_legacy_convert(submission): - """ - If the input login submission is an old style object - (ie. with top-level user / medium / address) convert it - to a typed object. - """ - if "user" in submission: - submission["identifier"] = { - "type": "m.id.user", - "user": submission["user"], - } - del submission["user"] - - if "medium" in submission and "address" in submission: - submission["identifier"] = { - "type": "m.id.thirdparty", - "medium": submission["medium"], - "address": submission["address"], - } - del submission["medium"] - del submission["address"] - - -def login_id_thirdparty_from_phone(identifier): - """ - Convert a phone login identifier type to a generic threepid identifier - Args: - identifier(dict): Login identifier dict of type 'm.id.phone' - - Returns: Login identifier dict of type 'm.id.threepid' - """ - if "country" not in identifier or "number" not in identifier: - raise SynapseError(400, "Invalid phone-type identifier") - - msisdn = phone_number_to_msisdn(identifier["country"], identifier["number"]) - - return { - "type": "m.id.thirdparty", - "medium": "msisdn", - "address": msisdn, - } - - class LoginRestServlet(ClientV1RestServlet): PATTERNS = client_path_patterns("/login$") PASS_TYPE = "m.login.password" @@ -161,52 +117,20 @@ class LoginRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def do_password_login(self, login_submission): - if "password" not in login_submission: - raise SynapseError(400, "Missing parameter: password") - - login_submission_legacy_convert(login_submission) - - if "identifier" not in login_submission: - raise SynapseError(400, "Missing param: identifier") - - identifier = login_submission["identifier"] - if "type" not in identifier: - raise SynapseError(400, "Login identifier has no type") - - # convert phone type identifiers to generic threepids - if identifier["type"] == "m.id.phone": - identifier = login_id_thirdparty_from_phone(identifier) - - # convert threepid identifiers to user IDs - if identifier["type"] == "m.id.thirdparty": - if 'medium' not in identifier or 'address' not in identifier: - raise SynapseError(400, "Invalid thirdparty identifier") - - address = identifier['address'] - if identifier['medium'] == 'email': + if 'medium' in login_submission and 'address' in login_submission: + address = login_submission['address'] + if login_submission['medium'] == 'email': # For emails, transform the address to lowercase. # We store all email addreses as lowercase in the DB. # (See add_threepid in synapse/handlers/auth.py) address = address.lower() user_id = yield self.hs.get_datastore().get_user_id_by_threepid( - identifier['medium'], address + login_submission['medium'], address ) if not user_id: raise LoginError(403, "", errcode=Codes.FORBIDDEN) - - identifier = { - "type": "m.id.user", - "user": user_id, - } - - # by this point, the identifier should be an m.id.user: if it's anything - # else, we haven't understood it. - if identifier["type"] != "m.id.user": - raise SynapseError(400, "Unknown login identifier type") - if "user" not in identifier: - raise SynapseError(400, "User identifier is missing 'user' key") - - user_id = identifier["user"] + else: + user_id = login_submission['user'] if not user_id.startswith('@'): user_id = UserID.create( diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index aac76edf1..398e7f5eb 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- # Copyright 2015, 2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -18,11 +17,8 @@ from twisted.internet import defer from synapse.api.constants import LoginType from synapse.api.errors import LoginError, SynapseError, Codes -from synapse.http.servlet import ( - RestServlet, parse_json_object_from_request, assert_params_in_request -) +from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.util.async import run_on_reactor -from synapse.util.msisdn import phone_number_to_msisdn from ._base import client_v2_patterns @@ -32,11 +28,11 @@ import logging logger = logging.getLogger(__name__) -class EmailPasswordRequestTokenRestServlet(RestServlet): +class PasswordRequestTokenRestServlet(RestServlet): PATTERNS = client_v2_patterns("/account/password/email/requestToken$") def __init__(self, hs): - super(EmailPasswordRequestTokenRestServlet, self).__init__() + super(PasswordRequestTokenRestServlet, self).__init__() self.hs = hs self.identity_handler = hs.get_handlers().identity_handler @@ -44,9 +40,14 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): def on_POST(self, request): body = parse_json_object_from_request(request) - assert_params_in_request(body, [ - 'id_server', 'client_secret', 'email', 'send_attempt' - ]) + required = ['id_server', 'client_secret', 'email', 'send_attempt'] + absent = [] + for k in required: + if k not in body: + absent.append(k) + + if absent: + raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM) existingUid = yield self.hs.get_datastore().get_user_id_by_threepid( 'email', body['email'] @@ -59,37 +60,6 @@ class EmailPasswordRequestTokenRestServlet(RestServlet): defer.returnValue((200, ret)) -class MsisdnPasswordRequestTokenRestServlet(RestServlet): - PATTERNS = client_v2_patterns("/account/password/msisdn/requestToken$") - - def __init__(self, hs): - super(MsisdnPasswordRequestTokenRestServlet, self).__init__() - self.hs = hs - self.datastore = self.hs.get_datastore() - self.identity_handler = hs.get_handlers().identity_handler - - @defer.inlineCallbacks - def on_POST(self, request): - body = parse_json_object_from_request(request) - - assert_params_in_request(body, [ - 'id_server', 'client_secret', - 'country', 'phone_number', 'send_attempt', - ]) - - msisdn = phone_number_to_msisdn(body['country'], body['phone_number']) - - existingUid = yield self.datastore.get_user_id_by_threepid( - 'msisdn', msisdn - ) - - if existingUid is None: - raise SynapseError(400, "MSISDN not found", Codes.THREEPID_NOT_FOUND) - - ret = yield self.identity_handler.requestMsisdnToken(**body) - defer.returnValue((200, ret)) - - class PasswordRestServlet(RestServlet): PATTERNS = client_v2_patterns("/account/password$") @@ -98,7 +68,6 @@ class PasswordRestServlet(RestServlet): self.hs = hs self.auth = hs.get_auth() self.auth_handler = hs.get_auth_handler() - self.datastore = self.hs.get_datastore() @defer.inlineCallbacks def on_POST(self, request): @@ -108,8 +77,7 @@ class PasswordRestServlet(RestServlet): authed, result, params, _ = yield self.auth_handler.check_auth([ [LoginType.PASSWORD], - [LoginType.EMAIL_IDENTITY], - [LoginType.MSISDN], + [LoginType.EMAIL_IDENTITY] ], body, self.hs.get_ip_from_request(request)) if not authed: @@ -134,7 +102,7 @@ class PasswordRestServlet(RestServlet): # (See add_threepid in synapse/handlers/auth.py) threepid['address'] = threepid['address'].lower() # if using email, we must know about the email they're authing with! - threepid_user_id = yield self.datastore.get_user_id_by_threepid( + threepid_user_id = yield self.hs.get_datastore().get_user_id_by_threepid( threepid['medium'], threepid['address'] ) if not threepid_user_id: @@ -201,14 +169,13 @@ class DeactivateAccountRestServlet(RestServlet): defer.returnValue((200, {})) -class EmailThreepidRequestTokenRestServlet(RestServlet): +class ThreepidRequestTokenRestServlet(RestServlet): PATTERNS = client_v2_patterns("/account/3pid/email/requestToken$") def __init__(self, hs): self.hs = hs - super(EmailThreepidRequestTokenRestServlet, self).__init__() + super(ThreepidRequestTokenRestServlet, self).__init__() self.identity_handler = hs.get_handlers().identity_handler - self.datastore = self.hs.get_datastore() @defer.inlineCallbacks def on_POST(self, request): @@ -223,7 +190,7 @@ class EmailThreepidRequestTokenRestServlet(RestServlet): if absent: raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM) - existingUid = yield self.datastore.get_user_id_by_threepid( + existingUid = yield self.hs.get_datastore().get_user_id_by_threepid( 'email', body['email'] ) @@ -234,44 +201,6 @@ class EmailThreepidRequestTokenRestServlet(RestServlet): defer.returnValue((200, ret)) -class MsisdnThreepidRequestTokenRestServlet(RestServlet): - PATTERNS = client_v2_patterns("/account/3pid/msisdn/requestToken$") - - def __init__(self, hs): - self.hs = hs - super(MsisdnThreepidRequestTokenRestServlet, self).__init__() - self.identity_handler = hs.get_handlers().identity_handler - self.datastore = self.hs.get_datastore() - - @defer.inlineCallbacks - def on_POST(self, request): - body = parse_json_object_from_request(request) - - required = [ - 'id_server', 'client_secret', - 'country', 'phone_number', 'send_attempt', - ] - absent = [] - for k in required: - if k not in body: - absent.append(k) - - if absent: - raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM) - - msisdn = phone_number_to_msisdn(body['country'], body['phone_number']) - - existingUid = yield self.datastore.get_user_id_by_threepid( - 'msisdn', msisdn - ) - - if existingUid is not None: - raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE) - - ret = yield self.identity_handler.requestEmailToken(**body) - defer.returnValue((200, ret)) - - class ThreepidRestServlet(RestServlet): PATTERNS = client_v2_patterns("/account/3pid$") @@ -281,7 +210,6 @@ class ThreepidRestServlet(RestServlet): self.identity_handler = hs.get_handlers().identity_handler self.auth = hs.get_auth() self.auth_handler = hs.get_auth_handler() - self.datastore = self.hs.get_datastore() @defer.inlineCallbacks def on_GET(self, request): @@ -289,7 +217,7 @@ class ThreepidRestServlet(RestServlet): requester = yield self.auth.get_user_by_req(request) - threepids = yield self.datastore.user_get_threepids( + threepids = yield self.hs.get_datastore().user_get_threepids( requester.user.to_string() ) @@ -330,7 +258,7 @@ class ThreepidRestServlet(RestServlet): if 'bind' in body and body['bind']: logger.debug( - "Binding threepid %s to %s", + "Binding emails %s to %s", threepid, user_id ) yield self.identity_handler.bind_threepid( @@ -374,11 +302,9 @@ class ThreepidDeleteRestServlet(RestServlet): def register_servlets(hs, http_server): - EmailPasswordRequestTokenRestServlet(hs).register(http_server) - MsisdnPasswordRequestTokenRestServlet(hs).register(http_server) + PasswordRequestTokenRestServlet(hs).register(http_server) PasswordRestServlet(hs).register(http_server) DeactivateAccountRestServlet(hs).register(http_server) - EmailThreepidRequestTokenRestServlet(hs).register(http_server) - MsisdnThreepidRequestTokenRestServlet(hs).register(http_server) + ThreepidRequestTokenRestServlet(hs).register(http_server) ThreepidRestServlet(hs).register(http_server) ThreepidDeleteRestServlet(hs).register(http_server) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 7448c1346..ccca5a12d 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- # Copyright 2015 - 2016 OpenMarket Ltd -# Copyright 2017 Vector Creations Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -20,10 +19,7 @@ import synapse from synapse.api.auth import get_access_token_from_request, has_access_token from synapse.api.constants import LoginType from synapse.api.errors import SynapseError, Codes, UnrecognizedRequestError -from synapse.http.servlet import ( - RestServlet, parse_json_object_from_request, assert_params_in_request -) -from synapse.util.msisdn import phone_number_to_msisdn +from synapse.http.servlet import RestServlet, parse_json_object_from_request from ._base import client_v2_patterns @@ -47,7 +43,7 @@ else: logger = logging.getLogger(__name__) -class EmailRegisterRequestTokenRestServlet(RestServlet): +class RegisterRequestTokenRestServlet(RestServlet): PATTERNS = client_v2_patterns("/register/email/requestToken$") def __init__(self, hs): @@ -55,7 +51,7 @@ class EmailRegisterRequestTokenRestServlet(RestServlet): Args: hs (synapse.server.HomeServer): server """ - super(EmailRegisterRequestTokenRestServlet, self).__init__() + super(RegisterRequestTokenRestServlet, self).__init__() self.hs = hs self.identity_handler = hs.get_handlers().identity_handler @@ -63,9 +59,14 @@ class EmailRegisterRequestTokenRestServlet(RestServlet): def on_POST(self, request): body = parse_json_object_from_request(request) - assert_params_in_request(body, [ - 'id_server', 'client_secret', 'email', 'send_attempt' - ]) + required = ['id_server', 'client_secret', 'email', 'send_attempt'] + absent = [] + for k in required: + if k not in body: + absent.append(k) + + if len(absent) > 0: + raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM) existingUid = yield self.hs.get_datastore().get_user_id_by_threepid( 'email', body['email'] @@ -78,43 +79,6 @@ class EmailRegisterRequestTokenRestServlet(RestServlet): defer.returnValue((200, ret)) -class MsisdnRegisterRequestTokenRestServlet(RestServlet): - PATTERNS = client_v2_patterns("/register/msisdn/requestToken$") - - def __init__(self, hs): - """ - Args: - hs (synapse.server.HomeServer): server - """ - super(MsisdnRegisterRequestTokenRestServlet, self).__init__() - self.hs = hs - self.identity_handler = hs.get_handlers().identity_handler - - @defer.inlineCallbacks - def on_POST(self, request): - body = parse_json_object_from_request(request) - - assert_params_in_request(body, [ - 'id_server', 'client_secret', - 'country', 'phone_number', - 'send_attempt', - ]) - - msisdn = phone_number_to_msisdn(body['country'], body['phone_number']) - - existingUid = yield self.hs.get_datastore().get_user_id_by_threepid( - 'msisdn', msisdn - ) - - if existingUid is not None: - raise SynapseError( - 400, "Phone number is already in use", Codes.THREEPID_IN_USE - ) - - ret = yield self.identity_handler.requestMsisdnToken(**body) - defer.returnValue((200, ret)) - - class RegisterRestServlet(RestServlet): PATTERNS = client_v2_patterns("/register$") @@ -239,16 +203,12 @@ class RegisterRestServlet(RestServlet): if self.hs.config.enable_registration_captcha: flows = [ [LoginType.RECAPTCHA], - [LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA], - [LoginType.MSISDN, LoginType.RECAPTCHA], - [LoginType.EMAIL_IDENTITY, LoginType.MSISDN, LoginType.RECAPTCHA], + [LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA] ] else: flows = [ [LoginType.DUMMY], - [LoginType.EMAIL_IDENTITY], - [LoginType.MSISDN], - [LoginType.EMAIL_IDENTITY, LoginType.MSISDN], + [LoginType.EMAIL_IDENTITY] ] authed, auth_result, params, session_id = yield self.auth_handler.check_auth( @@ -264,9 +224,8 @@ class RegisterRestServlet(RestServlet): "Already registered user ID %r for this session", registered_user_id ) - # don't re-register the threepids + # don't re-register the email address add_email = False - add_msisdn = False else: # NB: This may be from the auth handler and NOT from the POST if 'password' not in params: @@ -291,7 +250,6 @@ class RegisterRestServlet(RestServlet): ) add_email = True - add_msisdn = True return_dict = yield self._create_registration_details( registered_user_id, params @@ -304,13 +262,6 @@ class RegisterRestServlet(RestServlet): params.get("bind_email") ) - if add_msisdn and auth_result and LoginType.MSISDN in auth_result: - threepid = auth_result[LoginType.MSISDN] - yield self._register_msisdn_threepid( - registered_user_id, threepid, return_dict["access_token"], - params.get("bind_msisdn") - ) - defer.returnValue((200, return_dict)) def on_OPTIONS(self, _): @@ -372,9 +323,8 @@ class RegisterRestServlet(RestServlet): """ reqd = ('medium', 'address', 'validated_at') if any(x not in threepid for x in reqd): - # This will only happen if the ID server returns a malformed response logger.info("Can't add incomplete 3pid") - return + defer.returnValue() yield self.auth_handler.add_threepid( user_id, @@ -421,43 +371,6 @@ class RegisterRestServlet(RestServlet): else: logger.info("bind_email not specified: not binding email") - @defer.inlineCallbacks - def _register_msisdn_threepid(self, user_id, threepid, token, bind_msisdn): - """Add a phone number as a 3pid identifier - - Also optionally binds msisdn to the given user_id on the identity server - - Args: - user_id (str): id of user - threepid (object): m.login.msisdn auth response - token (str): access_token for the user - bind_email (bool): true if the client requested the email to be - bound at the identity server - Returns: - defer.Deferred: - """ - reqd = ('medium', 'address', 'validated_at') - if any(x not in threepid for x in reqd): - # This will only happen if the ID server returns a malformed response - logger.info("Can't add incomplete 3pid") - defer.returnValue() - - yield self.auth_handler.add_threepid( - user_id, - threepid['medium'], - threepid['address'], - threepid['validated_at'], - ) - - if bind_msisdn: - logger.info("bind_msisdn specified: binding") - logger.debug("Binding msisdn %s to %s", threepid, user_id) - yield self.identity_handler.bind_threepid( - threepid['threepid_creds'], user_id - ) - else: - logger.info("bind_msisdn not specified: not binding msisdn") - @defer.inlineCallbacks def _create_registration_details(self, user_id, params): """Complete registration of newly-registered user @@ -536,6 +449,5 @@ class RegisterRestServlet(RestServlet): def register_servlets(hs, http_server): - EmailRegisterRequestTokenRestServlet(hs).register(http_server) - MsisdnRegisterRequestTokenRestServlet(hs).register(http_server) + RegisterRequestTokenRestServlet(hs).register(http_server) RegisterRestServlet(hs).register(http_server) diff --git a/synapse/util/msisdn.py b/synapse/util/msisdn.py deleted file mode 100644 index 607161e7f..000000000 --- a/synapse/util/msisdn.py +++ /dev/null @@ -1,40 +0,0 @@ -# -*- coding: utf-8 -*- -# Copyright 2017 Vector Creations Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import phonenumbers -from synapse.api.errors import SynapseError - - -def phone_number_to_msisdn(country, number): - """ - Takes an ISO-3166-1 2 letter country code and phone number and - returns an msisdn representing the canonical version of that - phone number. - Args: - country (str): ISO-3166-1 2 letter country code - number (str): Phone number in a national or international format - - Returns: - (str) The canonical form of the phone number, as an msisdn - Raises: - SynapseError if the number could not be parsed. - """ - try: - phoneNumber = phonenumbers.parse(number, country) - except phonenumbers.NumberParseException: - raise SynapseError(400, "Unable to parse phone number") - return phonenumbers.format_number( - phoneNumber, phonenumbers.PhoneNumberFormat.E164 - )[1:] From 6037a9804ca730b0dcdf21c7fbfd0f75ecad7e6a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 10 Mar 2017 15:23:20 +0000 Subject: [PATCH 08/13] Add helpful texts to logger config options --- synapse/config/logger.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/config/logger.py b/synapse/config/logger.py index e5d945e5b..2dbeafa9d 100644 --- a/synapse/config/logger.py +++ b/synapse/config/logger.py @@ -45,7 +45,6 @@ handlers: maxBytes: 104857600 backupCount: 10 filters: [context] - level: INFO console: class: logging.StreamHandler formatter: precise @@ -56,6 +55,8 @@ loggers: level: INFO synapse.storage.SQL: + # beware: increasing this to DEBUG will make synapse log sensitive + # information such as access tokens. level: INFO root: @@ -78,10 +79,10 @@ class LoggingConfig(Config): os.path.join(config_dir_path, server_name + ".log.config") ) return """ - # Logging verbosity level. + # Logging verbosity level. Ignored if log_config is specified. verbose: 0 - # File to write logging to + # File to write logging to. Ignored if log_config is specified. log_file: "%(log_file)s" # A yaml python logging config file @@ -102,11 +103,12 @@ class LoggingConfig(Config): logging_group = parser.add_argument_group("logging") logging_group.add_argument( '-v', '--verbose', dest="verbose", action='count', - help="The verbosity level." + help="The verbosity level. Specify multiple times to increase " + "verbosity. (Ignored if --log-config is specified.)" ) logging_group.add_argument( '-f', '--log-file', dest="log_file", - help="File to log to." + help="File to log to. (Ignored if --log-config is specified.)" ) logging_group.add_argument( '--log-config', dest="log_config", default=None, From 8d86d11fdf54b1aca902d128ca89a8614e8c9a27 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 13 Mar 2017 12:39:09 +0000 Subject: [PATCH 09/13] Bring example log config into line with default --- contrib/example_log_config.yaml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contrib/example_log_config.yaml b/contrib/example_log_config.yaml index 7f7c8ba58..c7aa68abf 100644 --- a/contrib/example_log_config.yaml +++ b/contrib/example_log_config.yaml @@ -39,9 +39,11 @@ loggers: synapse: level: INFO - synapse.storage: + synapse.storage.SQL: + # beware: increasing this to DEBUG will make synapse log sensitive + # information such as access tokens. level: INFO - + # example of enabling debugging for a component: # # synapse.federation.transport.server: From c81f33f73d37fdf5027a356b50cc6ab0f93da3d9 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Mon, 13 Mar 2017 16:33:51 +0000 Subject: [PATCH 10/13] Implement delete_devices API This implements the proposal here https://docs.google.com/document/d/1C-25Gqz3TXy2jIAoeOKxpNtmme0jI4g3yFGqv5GlAAk for deleting multiple devices at once in a single request. --- synapse/rest/client/v2_alpha/devices.py | 47 +++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index a1feaf3d5..2560da141 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -45,6 +45,52 @@ class DevicesRestServlet(servlet.RestServlet): ) defer.returnValue((200, {"devices": devices})) +class DeleteDevicesRestServlet(servlet.RestServlet): + PATTERNS = client_v2_patterns("/delete_devices", releases=[], v2_alpha=False) + + def __init__(self, hs): + """ + Args: + hs (synapse.server.HomeServer): server + """ + super(DeleteDevicesRestServlet, self).__init__() + self.hs = hs + self.auth = hs.get_auth() + self.device_handler = hs.get_device_handler() + self.auth_handler = hs.get_auth_handler() + + @defer.inlineCallbacks + def on_POST(self, request): + try: + body = servlet.parse_json_object_from_request(request) + + except errors.SynapseError as e: + if e.errcode == errors.Codes.NOT_JSON: + # deal with older clients which didn't pass a J*DELETESON dict + # the same as those that pass an empty dict + body = {} + else: + raise + + if 'devices' not in body: + raise errors.SynapseError( + 400, "No devices supplied", errcode=errors.Codes.MISSING_PARAM + ) + + authed, result, params, _ = yield self.auth_handler.check_auth([ + [constants.LoginType.PASSWORD], + ], body, self.hs.get_ip_from_request(request)) + + if not authed: + defer.returnValue((401, result)) + + requester = yield self.auth.get_user_by_req(request) + for d_id in body['devices']: + yield self.device_handler.delete_device( + requester.user.to_string(), + d_id, + ) + defer.returnValue((200, {})) class DeviceRestServlet(servlet.RestServlet): PATTERNS = client_v2_patterns("/devices/(?P[^/]*)$", @@ -111,5 +157,6 @@ class DeviceRestServlet(servlet.RestServlet): def register_servlets(hs, http_server): + DeleteDevicesRestServlet(hs).register(http_server) DevicesRestServlet(hs).register(http_server) DeviceRestServlet(hs).register(http_server) From c077c3277b30968933a394bf8c2675cb4f9bf671 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Mon, 13 Mar 2017 16:45:38 +0000 Subject: [PATCH 11/13] Flake --- synapse/rest/client/v2_alpha/devices.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index 2560da141..fd9516a60 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -45,6 +45,7 @@ class DevicesRestServlet(servlet.RestServlet): ) defer.returnValue((200, {"devices": devices})) + class DeleteDevicesRestServlet(servlet.RestServlet): PATTERNS = client_v2_patterns("/delete_devices", releases=[], v2_alpha=False) @@ -92,6 +93,7 @@ class DeleteDevicesRestServlet(servlet.RestServlet): ) defer.returnValue((200, {})) + class DeviceRestServlet(servlet.RestServlet): PATTERNS = client_v2_patterns("/devices/(?P[^/]*)$", releases=[], v2_alpha=False) From bbeeb97f753e158e9aadd53aff78b076d756917c Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Mon, 13 Mar 2017 17:53:23 +0000 Subject: [PATCH 12/13] Implement _simple_delete_many_txn, use it to delete devices (But this doesn't implement the same for deleting access tokens or e2e keys. Also respond to code review. --- synapse/handlers/device.py | 34 ++++++++++++++++++++ synapse/rest/client/v2_alpha/devices.py | 20 ++++++------ synapse/storage/_base.py | 41 +++++++++++++++++++++++++ synapse/storage/devices.py | 17 ++++++++++ 4 files changed, 101 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index e859b3165..efaa0c8d6 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -169,6 +169,40 @@ class DeviceHandler(BaseHandler): yield self.notify_device_update(user_id, [device_id]) + @defer.inlineCallbacks + def delete_devices(self, user_id, device_ids): + """ Delete several devices + + Args: + user_id (str): + device_ids (str): The list of device IDs to delete + + Returns: + defer.Deferred: + """ + + try: + yield self.store.delete_devices(user_id, device_ids) + except errors.StoreError, e: + if e.code == 404: + # no match + pass + else: + raise + + # Delete access tokens and e2e keys for each device. Not optimised as it is not + # considered as part of a critical path. + for device_id in device_ids: + yield self.store.user_delete_access_tokens( + user_id, device_id=device_id, + delete_refresh_tokens=True, + ) + yield self.store.delete_e2e_keys_by_device( + user_id=user_id, device_id=device_id + ) + + yield self.notify_device_update(user_id, device_ids) + @defer.inlineCallbacks def update_device(self, user_id, device_id, content): """ Update the given device diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index fd9516a60..b57ba95d2 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -47,13 +47,13 @@ class DevicesRestServlet(servlet.RestServlet): class DeleteDevicesRestServlet(servlet.RestServlet): + """ + API for bulk deletion of devices. Accepts a JSON object with a devices + key which lists the device_ids to delete. Requires user interactive auth. + """ PATTERNS = client_v2_patterns("/delete_devices", releases=[], v2_alpha=False) def __init__(self, hs): - """ - Args: - hs (synapse.server.HomeServer): server - """ super(DeleteDevicesRestServlet, self).__init__() self.hs = hs self.auth = hs.get_auth() @@ -64,14 +64,13 @@ class DeleteDevicesRestServlet(servlet.RestServlet): def on_POST(self, request): try: body = servlet.parse_json_object_from_request(request) - except errors.SynapseError as e: if e.errcode == errors.Codes.NOT_JSON: # deal with older clients which didn't pass a J*DELETESON dict # the same as those that pass an empty dict body = {} else: - raise + raise e if 'devices' not in body: raise errors.SynapseError( @@ -86,11 +85,10 @@ class DeleteDevicesRestServlet(servlet.RestServlet): defer.returnValue((401, result)) requester = yield self.auth.get_user_by_req(request) - for d_id in body['devices']: - yield self.device_handler.delete_device( - requester.user.to_string(), - d_id, - ) + yield self.device_handler.delete_devices( + requester.user.to_string(), + body['devices'], + ) defer.returnValue((200, {})) diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py index a7a8ec9b7..13b106bba 100644 --- a/synapse/storage/_base.py +++ b/synapse/storage/_base.py @@ -840,6 +840,47 @@ class SQLBaseStore(object): return txn.execute(sql, keyvalues.values()) + def _simple_delete_many(self, table, column, iterable, keyvalues, desc): + return self.runInteraction( + desc, self._simple_delete_many_txn, table, column, iterable, keyvalues + ) + + @staticmethod + def _simple_delete_many_txn(txn, table, column, iterable, keyvalues): + """Executes a DELETE query on the named table. + + Filters rows by if value of `column` is in `iterable`. + + Args: + txn : Transaction object + table : string giving the table name + column : column name to test for inclusion against `iterable` + iterable : list + keyvalues : dict of column names and values to select the rows with + """ + if not iterable: + return + + sql = "DELETE FROM %s" % table + + clauses = [] + values = [] + clauses.append( + "%s IN (%s)" % (column, ",".join("?" for _ in iterable)) + ) + values.extend(iterable) + + for key, value in keyvalues.items(): + clauses.append("%s = ?" % (key,)) + values.append(value) + + if clauses: + sql = "%s WHERE %s" % ( + sql, + " AND ".join(clauses), + ) + return txn.execute(sql, values) + def _get_cache_dict(self, db_conn, table, entity_column, stream_column, max_value, limit=100000): # Fetch a mapping of room_id -> max stream position for "recent" rooms. diff --git a/synapse/storage/devices.py b/synapse/storage/devices.py index bd56ba251..563071b7a 100644 --- a/synapse/storage/devices.py +++ b/synapse/storage/devices.py @@ -108,6 +108,23 @@ class DeviceStore(SQLBaseStore): desc="delete_device", ) + def delete_devices(self, user_id, device_ids): + """Deletes several devices. + + Args: + user_id (str): The ID of the user which owns the devices + device_ids (list): The IDs of the devices to delete + Returns: + defer.Deferred + """ + return self._simple_delete_many( + table="devices", + column="device_id", + iterable=device_ids, + keyvalues={"user_id": user_id}, + desc="delete_devices", + ) + def update_device(self, user_id, device_id, new_display_name=None): """Update a device. From cc7a294e2ec271126207922ea293e16d2a858943 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 14 Mar 2017 10:54:28 +0000 Subject: [PATCH 13/13] Fix current_state_events table to not lie If we try and persist two state events that have the same ancestor we calculate the wrong current state when persisting those events. --- synapse/storage/events.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/synapse/storage/events.py b/synapse/storage/events.py index db01eb6d1..03881ea3d 100644 --- a/synapse/storage/events.py +++ b/synapse/storage/events.py @@ -433,11 +433,36 @@ class EventsStore(SQLBaseStore): if not new_latest_event_ids: current_state = {} elif was_updated: + # We work out the current state by passing the state sets to the + # state resolution algorithm. It may ask for some events, including + # the events we have yet to persist, so we need a slightly more + # complicated event lookup function than simply looking the events + # up in the db. + events_map = {ev.event_id: ev for ev, _ in events_context} + + @defer.inlineCallbacks + def get_events(ev_ids): + # We get the events by first looking at the list of events we + # are trying to persist, and then fetching the rest from the DB. + db = [] + to_return = {} + for ev_id in ev_ids: + ev = events_map.get(ev_id, None) + if ev: + to_return[ev_id] = ev + else: + db.append(ev_id) + + if db: + evs = yield self.get_events( + ev_ids, get_prev_content=False, check_redacted=False, + ) + to_return.update(evs) + defer.returnValue(to_return) + current_state = yield resolve_events( state_sets, - state_map_factory=lambda ev_ids: self.get_events( - ev_ids, get_prev_content=False, check_redacted=False, - ), + state_map_factory=get_events, ) else: return