From a61dd408ed78db974a4d78f0708cc9405e36df64 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 12 Mar 2017 19:30:45 +0000 Subject: enable guest access for the 3pl/3pid APIs --- synapse/rest/client/v2_alpha/thirdparty.py | 8 -------- 1 file changed, 8 deletions(-) (limited to 'synapse') diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index 31f94bc6e9..ee2f158d03 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -36,8 +36,6 @@ class ThirdPartyProtocolsServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request): - yield self.auth.get_user_by_req(request) - protocols = yield self.appservice_handler.get_3pe_protocols() defer.returnValue((200, protocols)) @@ -54,8 +52,6 @@ class ThirdPartyProtocolServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): - yield self.auth.get_user_by_req(request) - protocols = yield self.appservice_handler.get_3pe_protocols( only_protocol=protocol, ) @@ -77,8 +73,6 @@ class ThirdPartyUserServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): - yield self.auth.get_user_by_req(request) - fields = request.args fields.pop("access_token", None) @@ -101,8 +95,6 @@ class ThirdPartyLocationServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): - yield self.auth.get_user_by_req(request) - fields = request.args fields.pop("access_token", None) -- cgit 1.4.1 From e0ff66251f71cb46aea30a187edc1dc027760b9e Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 15 Mar 2017 12:22:18 +0000 Subject: add setting (on by default) to support TURN for guests --- docs/turn-howto.rst | 38 ++++++++++++++++++++++++++++++++++---- synapse/config/voip.py | 8 ++++++++ synapse/rest/client/v1/voip.py | 5 ++++- 3 files changed, 46 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/docs/turn-howto.rst b/docs/turn-howto.rst index 04c0100715..e48628ce6e 100644 --- a/docs/turn-howto.rst +++ b/docs/turn-howto.rst @@ -50,14 +50,37 @@ You may be able to setup coturn via your package manager, or set it up manually pwgen -s 64 1 - 5. Ensure youe firewall allows traffic into the TURN server on + 5. Consider your security settings. TURN lets users request a relay + which will connect to arbitrary IP addresses and ports. At the least + we recommend: + + # VoIP traffic is all UDP. There is no reason to let users connect to arbitrary TCP endpoints via the relay. + no-tcp-relay + + # don't let the relay ever try to connect to private IP address ranges within your network (if any) + # given the turn server is likely behind your firewall, remember to include any privileged public IPs too. + denied-peer-ip=10.0.0.0-10.255.255.255 + denied-peer-ip=192.168.0.0-192.168.255.255 + denied-peer-ip=172.16.0.0-172.31.255.255 + + # special case the turn server itself so that client->TURN->TURN->client flows work + allowed-peer-ip=10.0.0.1 + + # consider whether you want to limit the quota of relayed streams per user (or total) to avoid risk of DoS. + user-quota=12 # 4 streams per video call, so 12 streams = 3 simultaneous relayed calls per user. + total-quota=1200 + + Ideally coturn should refuse to relay traffic which isn't SRTP; + see https://github.com/matrix-org/synapse/issues/2009 + + 6. Ensure your firewall allows traffic into the TURN server on the ports you've configured it to listen on (remember to allow - both TCP and UDP if you've enabled both). + both TCP and UDP TURN traffic) - 6. If you've configured coturn to support TLS/DTLS, generate or + 7. If you've configured coturn to support TLS/DTLS, generate or import your private key and certificate. - 7. Start the turn server:: + 8. Start the turn server:: bin/turnserver -o @@ -83,12 +106,19 @@ Your home server configuration file needs the following extra keys: to refresh credentials. The TURN REST API specification recommends one day (86400000). + 4. "turn_allow_guests": Whether to allow guest users to use the TURN + server. This is enabled by default, as otherwise VoIP will not + work reliably for guests. However, it does introduce a security risk + as it lets guests connect to arbitrary endpoints without having gone + through a CAPTCHA or similar to register a real account. + As an example, here is the relevant section of the config file for matrix.org:: turn_uris: [ "turn:turn.matrix.org:3478?transport=udp", "turn:turn.matrix.org:3478?transport=tcp" ] turn_shared_secret: n0t4ctuAllymatr1Xd0TorgSshar3d5ecret4obvIousreAsons turn_user_lifetime: 86400000 + turn_allow_guests: True Now, restart synapse:: diff --git a/synapse/config/voip.py b/synapse/config/voip.py index eeb693027b..c93c92d177 100644 --- a/synapse/config/voip.py +++ b/synapse/config/voip.py @@ -23,6 +23,7 @@ class VoipConfig(Config): self.turn_username = config.get("turn_username") self.turn_password = config.get("turn_password") self.turn_user_lifetime = self.parse_duration(config["turn_user_lifetime"]) + self.turn_allow_guests = config.get("turn_allow_guests") or True def default_config(self, **kwargs): return """\ @@ -41,4 +42,11 @@ class VoipConfig(Config): # How long generated TURN credentials last turn_user_lifetime: "1h" + + # Whether guests should be allowed to use the TURN server. + # This is defaults to True, otherwise VoIP will be unreliable for guests. + # However, it does introduce a slight security risk as it allows users to + # connect to arbitrary endpoints without having first signed up for a + # valid account (e.g. by passing a CAPTCHA). + turn_allow_guests: True """ diff --git a/synapse/rest/client/v1/voip.py b/synapse/rest/client/v1/voip.py index 03141c623c..c43b30b73a 100644 --- a/synapse/rest/client/v1/voip.py +++ b/synapse/rest/client/v1/voip.py @@ -28,7 +28,10 @@ class VoipRestServlet(ClientV1RestServlet): @defer.inlineCallbacks def on_GET(self, request): - requester = yield self.auth.get_user_by_req(request) + requester = yield self.auth.get_user_by_req( + request, + self.hs.config.turn_allow_guests + ) turnUris = self.hs.config.turn_uris turnSecret = self.hs.config.turn_shared_secret -- cgit 1.4.1 From 5aa42d429262fe44a4f02ea55c885eb1b3402359 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 15 Mar 2017 12:40:13 +0000 Subject: set default for turn_allow_guests correctly --- synapse/config/voip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/config/voip.py b/synapse/config/voip.py index c93c92d177..cf4274b618 100644 --- a/synapse/config/voip.py +++ b/synapse/config/voip.py @@ -23,7 +23,7 @@ class VoipConfig(Config): self.turn_username = config.get("turn_username") self.turn_password = config.get("turn_password") self.turn_user_lifetime = self.parse_duration(config["turn_user_lifetime"]) - self.turn_allow_guests = config.get("turn_allow_guests") or True + self.turn_allow_guests = config.get("turn_allow_guests", True) def default_config(self, **kwargs): return """\ -- cgit 1.4.1 From 0970e0307e52d4c7c666eded9955c423ef56b7c2 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 15 Mar 2017 12:40:42 +0000 Subject: typo --- synapse/config/voip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/config/voip.py b/synapse/config/voip.py index cf4274b618..3a4e16fa96 100644 --- a/synapse/config/voip.py +++ b/synapse/config/voip.py @@ -44,7 +44,7 @@ class VoipConfig(Config): turn_user_lifetime: "1h" # Whether guests should be allowed to use the TURN server. - # This is defaults to True, otherwise VoIP will be unreliable for guests. + # This defaults to True, otherwise VoIP will be unreliable for guests. # However, it does introduce a slight security risk as it allows users to # connect to arbitrary endpoints without having first signed up for a # valid account (e.g. by passing a CAPTCHA). -- cgit 1.4.1 From 650f0e69f2ff1c15739868c0e1a639d70ac13dbf Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 28 Mar 2017 13:03:50 +0100 Subject: Compile the regex's used in ASes --- synapse/appservice/__init__.py | 14 +++++--------- tests/appservice/test_appservice.py | 4 +++- 2 files changed, 8 insertions(+), 10 deletions(-) (limited to 'synapse') diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py index b0106a3597..1e298ccf36 100644 --- a/synapse/appservice/__init__.py +++ b/synapse/appservice/__init__.py @@ -124,22 +124,18 @@ class ApplicationService(object): raise ValueError( "Expected bool for 'exclusive' in ns '%s'" % ns ) - if not isinstance(regex_obj.get("regex"), basestring): + regex = regex_obj.get("regex") + if isinstance(regex, basestring): + regex_obj["regex"] = re.compile(regex) + else: raise ValueError( "Expected string for 'regex' in ns '%s'" % ns ) return namespaces def _matches_regex(self, test_string, namespace_key, return_obj=False): - if not isinstance(test_string, basestring): - logger.error( - "Expected a string to test regex against, but got %s", - test_string - ) - return False - for regex_obj in self.namespaces[namespace_key]: - if re.match(regex_obj["regex"], test_string): + if regex_obj["regex"].match(test_string): if return_obj: return regex_obj return True diff --git a/tests/appservice/test_appservice.py b/tests/appservice/test_appservice.py index aa8cc50550..7586ea9053 100644 --- a/tests/appservice/test_appservice.py +++ b/tests/appservice/test_appservice.py @@ -19,10 +19,12 @@ from twisted.internet import defer from mock import Mock from tests import unittest +import re + def _regex(regex, exclusive=True): return { - "regex": regex, + "regex": re.compile(regex), "exclusive": exclusive } -- cgit 1.4.1 From 30f5ffdca2a610c7b47dd9aaa02f1fa91976775f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 28 Mar 2017 13:20:15 +0100 Subject: Remove param and cast at call site --- synapse/appservice/__init__.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'synapse') diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py index 1e298ccf36..885d14fa91 100644 --- a/synapse/appservice/__init__.py +++ b/synapse/appservice/__init__.py @@ -133,16 +133,14 @@ class ApplicationService(object): ) return namespaces - def _matches_regex(self, test_string, namespace_key, return_obj=False): + def _matches_regex(self, test_string, namespace_key): for regex_obj in self.namespaces[namespace_key]: if regex_obj["regex"].match(test_string): - if return_obj: - return regex_obj - return True - return False + return regex_obj + return None def _is_exclusive(self, ns_key, test_string): - regex_obj = self._matches_regex(test_string, ns_key, return_obj=True) + regex_obj = self._matches_regex(test_string, ns_key) if regex_obj: return regex_obj["exclusive"] return False @@ -215,10 +213,10 @@ class ApplicationService(object): ) def is_interested_in_alias(self, alias): - return self._matches_regex(alias, ApplicationService.NS_ALIASES) + return bool(self._matches_regex(alias, ApplicationService.NS_ALIASES)) def is_interested_in_room(self, room_id): - return self._matches_regex(room_id, ApplicationService.NS_ROOMS) + return bool(self._matches_regex(room_id, ApplicationService.NS_ROOMS)) def is_exclusive_user(self, user_id): return ( -- cgit 1.4.1 From 51b156d48a14f1f3c8b03a6901317b0330cd368b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 28 Mar 2017 13:25:18 +0100 Subject: Cache whether an AS is interested based on members --- synapse/appservice/__init__.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py index 885d14fa91..48791f0d9b 100644 --- a/synapse/appservice/__init__.py +++ b/synapse/appservice/__init__.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. from synapse.api.constants import EventTypes +from synapse.util.caches.descriptors import cachedInlineCallbacks from twisted.internet import defer @@ -160,7 +161,14 @@ class ApplicationService(object): if not store: defer.returnValue(False) - member_list = yield store.get_users_in_room(event.room_id) + does_match = yield self._matches_user_in_member_list(event.room_id, store) + defer.returnValue(does_match) + + @cachedInlineCallbacks(num_args=1, cache_context=True) + def _matches_user_in_member_list(self, room_id, store, cache_context): + member_list = yield store.get_users_in_room( + room_id, on_invalidate=cache_context.invalidate + ) # check joined member events for user_id in member_list: -- cgit 1.4.1 From 69efd7774935c1dd6a0330f114c7fca00db959c0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 29 Mar 2017 09:50:05 +0100 Subject: Add comment --- synapse/appservice/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py index 48791f0d9b..7346206bb1 100644 --- a/synapse/appservice/__init__.py +++ b/synapse/appservice/__init__.py @@ -127,7 +127,7 @@ class ApplicationService(object): ) regex = regex_obj.get("regex") if isinstance(regex, basestring): - regex_obj["regex"] = re.compile(regex) + regex_obj["regex"] = re.compile(regex) # Pre-compile regex else: raise ValueError( "Expected string for 'regex' in ns '%s'" % ns -- cgit 1.4.1 From a3810136fe107415543ae92ec21fdbeb22b049b8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 29 Mar 2017 15:53:14 +0100 Subject: Cache glob to regex at a higher level for push --- synapse/push/push_rule_evaluator.py | 104 ++++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 47 deletions(-) (limited to 'synapse') diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py index 4db76f18bd..4d88046579 100644 --- a/synapse/push/push_rule_evaluator.py +++ b/synapse/push/push_rule_evaluator.py @@ -17,6 +17,7 @@ import logging import re from synapse.types import UserID +from synapse.util.caches import CACHE_SIZE_FACTOR, register_cache from synapse.util.caches.lrucache import LruCache logger = logging.getLogger(__name__) @@ -125,6 +126,11 @@ class PushRuleEvaluatorForEvent(object): return self._value_cache.get(dotted_key, None) +# Caches (glob, word_boundary) -> regex for push. See _glob_matches +regex_cache = LruCache(50000 * CACHE_SIZE_FACTOR) +register_cache("regex_push_cache", regex_cache) + + def _glob_matches(glob, value, word_boundary=False): """Tests if value matches glob. @@ -137,46 +143,63 @@ def _glob_matches(glob, value, word_boundary=False): Returns: bool """ - try: - if IS_GLOB.search(glob): - r = re.escape(glob) - - r = r.replace(r'\*', '.*?') - r = r.replace(r'\?', '.') - - # handle [abc], [a-z] and [!a-z] style ranges. - r = GLOB_REGEX.sub( - lambda x: ( - '[%s%s]' % ( - x.group(1) and '^' or '', - x.group(2).replace(r'\\\-', '-') - ) - ), - r, - ) - if word_boundary: - r = r"\b%s\b" % (r,) - r = _compile_regex(r) - - return r.search(value) - else: - r = r + "$" - r = _compile_regex(r) - - return r.match(value) - elif word_boundary: - r = re.escape(glob) - r = r"\b%s\b" % (r,) - r = _compile_regex(r) - return r.search(value) - else: - return value.lower() == glob.lower() + try: + r = regex_cache.get((glob, word_boundary), None) + if not r: + r = _glob_to_re(glob, word_boundary) + regex_cache[(glob, word_boundary)] = r + return r.search(value) except re.error: logger.warn("Failed to parse glob to regex: %r", glob) return False +def _glob_to_re(glob, word_boundary): + """Generates regex for a given glob. + + Args: + glob (string) + word_boundary (bool): Whether to match against word boundaries or entire + string. Defaults to False. + + Returns: + regex object + """ + if IS_GLOB.search(glob): + r = re.escape(glob) + + r = r.replace(r'\*', '.*?') + r = r.replace(r'\?', '.') + + # handle [abc], [a-z] and [!a-z] style ranges. + r = GLOB_REGEX.sub( + lambda x: ( + '[%s%s]' % ( + x.group(1) and '^' or '', + x.group(2).replace(r'\\\-', '-') + ) + ), + r, + ) + if word_boundary: + r = r"\b%s\b" % (r,) + + return re.compile(r, flags=re.IGNORECASE) + else: + r = "^" + r + "$" + + return re.compile(r, flags=re.IGNORECASE) + elif word_boundary: + r = re.escape(glob) + r = r"\b%s\b" % (r,) + + return re.compile(r, flags=re.IGNORECASE) + else: + r = "^" + re.escape(glob) + "$" + return re.compile(r, flags=re.IGNORECASE) + + def _flatten_dict(d, prefix=[], result={}): for key, value in d.items(): if isinstance(value, basestring): @@ -185,16 +208,3 @@ def _flatten_dict(d, prefix=[], result={}): _flatten_dict(value, prefix=(prefix + [key]), result=result) return result - - -regex_cache = LruCache(5000) - - -def _compile_regex(regex_str): - r = regex_cache.get(regex_str, None) - if r: - return r - - r = re.compile(regex_str, flags=re.IGNORECASE) - regex_cache[regex_str] = r - return r -- cgit 1.4.1 From 305d16d61200362f807fa5d97d415043f2a09315 Mon Sep 17 00:00:00 2001 From: Anant Prakash Date: Wed, 29 Mar 2017 18:17:42 +0530 Subject: add user friendly report of assertion error in synctl.py Signed-off-by: Anant Prakash --- synapse/app/synctl.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/app/synctl.py b/synapse/app/synctl.py index c045588866..81510bc5c1 100755 --- a/synapse/app/synctl.py +++ b/synapse/app/synctl.py @@ -175,7 +175,8 @@ def main(): worker_app = worker_config["worker_app"] worker_pidfile = worker_config["worker_pid_file"] worker_daemonize = worker_config["worker_daemonize"] - assert worker_daemonize # TODO print something more user friendly + assert worker_daemonize, "In config %r: expected '%s' to be True" % ( + worker_configfile, "worker_daemonize") worker_cache_factor = worker_config.get("synctl_cache_factor") workers.append(Worker( worker_app, worker_configfile, worker_pidfile, worker_cache_factor, -- cgit 1.4.1 From f9b4bb05e05694f3000df2bc5331b1aaa501575c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 30 Mar 2017 13:22:24 +0100 Subject: Fix the logcontext handling in the cache wrappers (#2077) The cache wrappers had a habit of leaking the logcontext into the reactor while the lookup function was running, and then not restoring it correctly when the lookup function had completed. It's all the fault of `preserve_context_over_{fn,deferred}` which are basically a bit broken. --- docs/log_contexts.rst | 11 +++-- synapse/util/caches/descriptors.py | 30 ++++++------ synapse/util/logcontext.py | 23 +++++++++ tests/util/caches/test_descriptors.py | 91 +++++++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 19 deletions(-) (limited to 'synapse') diff --git a/docs/log_contexts.rst b/docs/log_contexts.rst index 8d04a973de..eb1784e700 100644 --- a/docs/log_contexts.rst +++ b/docs/log_contexts.rst @@ -204,9 +204,14 @@ That doesn't follow the rules, but we can fix it by wrapping it with This technique works equally for external functions which return deferreds, or deferreds we have made ourselves. -XXX: think this is what ``preserve_context_over_deferred`` is supposed to do, -though it is broken, in that it only restores the logcontext for the duration -of the callbacks, which doesn't comply with the logcontext rules. +You can also use ``logcontext.make_deferred_yieldable``, which just does the +boilerplate for you, so the above could be written: + +.. code:: python + + def sleep(seconds): + return logcontext.make_deferred_yieldable(get_sleep_deferred(seconds)) + Fire-and-forget --------------- diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 19595df422..5c30ed235d 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -15,12 +15,9 @@ import logging from synapse.util.async import ObservableDeferred -from synapse.util import unwrapFirstError +from synapse.util import unwrapFirstError, logcontext from synapse.util.caches.lrucache import LruCache from synapse.util.caches.treecache import TreeCache, iterate_tree_cache_entry -from synapse.util.logcontext import ( - PreserveLoggingContext, preserve_context_over_deferred, preserve_context_over_fn -) from . import DEBUG_CACHES, register_cache @@ -328,11 +325,9 @@ class CacheDescriptor(_CacheDescriptorBase): defer.returnValue(cached_result) observer.addCallback(check_result) - return preserve_context_over_deferred(observer) except KeyError: ret = defer.maybeDeferred( - preserve_context_over_fn, - self.function_to_call, + logcontext.preserve_fn(self.function_to_call), obj, *args, **kwargs ) @@ -342,10 +337,11 @@ class CacheDescriptor(_CacheDescriptorBase): ret.addErrback(onErr) - ret = ObservableDeferred(ret, consumeErrors=True) - cache.set(cache_key, ret, callback=invalidate_callback) + result_d = ObservableDeferred(ret, consumeErrors=True) + cache.set(cache_key, result_d, callback=invalidate_callback) + observer = result_d.observe() - return preserve_context_over_deferred(ret.observe()) + return logcontext.make_deferred_yieldable(observer) wrapped.invalidate = cache.invalidate wrapped.invalidate_all = cache.invalidate_all @@ -362,7 +358,11 @@ class CacheListDescriptor(_CacheDescriptorBase): """Wraps an existing cache to support bulk fetching of keys. Given a list of keys it looks in the cache to find any hits, then passes - the list of missing keys to the wrapped fucntion. + the list of missing keys to the wrapped function. + + Once wrapped, the function returns either a Deferred which resolves to + the list of results, or (if all results were cached), just the list of + results. """ def __init__(self, orig, cached_method_name, list_name, num_args=None, @@ -433,8 +433,7 @@ class CacheListDescriptor(_CacheDescriptorBase): args_to_call[self.list_name] = missing ret_d = defer.maybeDeferred( - preserve_context_over_fn, - self.function_to_call, + logcontext.preserve_fn(self.function_to_call), **args_to_call ) @@ -443,8 +442,7 @@ class CacheListDescriptor(_CacheDescriptorBase): # We need to create deferreds for each arg in the list so that # we can insert the new deferred into the cache. for arg in missing: - with PreserveLoggingContext(): - observer = ret_d.observe() + observer = ret_d.observe() observer.addCallback(lambda r, arg: r.get(arg, None), arg) observer = ObservableDeferred(observer) @@ -471,7 +469,7 @@ class CacheListDescriptor(_CacheDescriptorBase): results.update(res) return results - return preserve_context_over_deferred(defer.gatherResults( + return logcontext.make_deferred_yieldable(defer.gatherResults( cached_defers.values(), consumeErrors=True, ).addCallback(update_results_dict).addErrback( diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index ff67b1d794..857afee7cb 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -310,6 +310,10 @@ def preserve_context_over_fn(fn, *args, **kwargs): def preserve_context_over_deferred(deferred, context=None): """Given a deferred wrap it such that any callbacks added later to it will be invoked with the current context. + + Deprecated: this almost certainly doesn't do want you want, ie make + the deferred follow the synapse logcontext rules: try + ``make_deferred_yieldable`` instead. """ if context is None: context = LoggingContext.current_context() @@ -359,6 +363,25 @@ def preserve_fn(f): return g +@defer.inlineCallbacks +def make_deferred_yieldable(deferred): + """Given a deferred, make it follow the Synapse logcontext rules: + + If the deferred has completed (or is not actually a Deferred), essentially + does nothing (just returns another completed deferred with the + result/failure). + + If the deferred has not yet completed, resets the logcontext before + returning a deferred. Then, when the deferred completes, restores the + current logcontext before running callbacks/errbacks. + + (This is more-or-less the opposite operation to preserve_fn.) + """ + with PreserveLoggingContext(): + r = yield deferred + defer.returnValue(r) + + # modules to ignore in `logcontext_tracer` _to_ignore = [ "synapse.util.logcontext", diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py index 419281054d..4414e86771 100644 --- a/tests/util/caches/test_descriptors.py +++ b/tests/util/caches/test_descriptors.py @@ -12,11 +12,18 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import logging + import mock +from synapse.api.errors import SynapseError +from synapse.util import async +from synapse.util import logcontext from twisted.internet import defer from synapse.util.caches import descriptors from tests import unittest +logger = logging.getLogger(__name__) + class DescriptorTestCase(unittest.TestCase): @defer.inlineCallbacks @@ -84,3 +91,87 @@ class DescriptorTestCase(unittest.TestCase): r = yield obj.fn(2, 5) self.assertEqual(r, 'chips') obj.mock.assert_not_called() + + def test_cache_logcontexts(self): + """Check that logcontexts are set and restored correctly when + using the cache.""" + + complete_lookup = defer.Deferred() + + class Cls(object): + @descriptors.cached() + def fn(self, arg1): + @defer.inlineCallbacks + def inner_fn(): + with logcontext.PreserveLoggingContext(): + yield complete_lookup + defer.returnValue(1) + + return inner_fn() + + @defer.inlineCallbacks + def do_lookup(): + with logcontext.LoggingContext() as c1: + c1.name = "c1" + r = yield obj.fn(1) + self.assertEqual(logcontext.LoggingContext.current_context(), + c1) + defer.returnValue(r) + + def check_result(r): + self.assertEqual(r, 1) + + obj = Cls() + + # set off a deferred which will do a cache lookup + d1 = do_lookup() + self.assertEqual(logcontext.LoggingContext.current_context(), + logcontext.LoggingContext.sentinel) + d1.addCallback(check_result) + + # and another + d2 = do_lookup() + self.assertEqual(logcontext.LoggingContext.current_context(), + logcontext.LoggingContext.sentinel) + d2.addCallback(check_result) + + # let the lookup complete + complete_lookup.callback(None) + + return defer.gatherResults([d1, d2]) + + def test_cache_logcontexts_with_exception(self): + """Check that the cache sets and restores logcontexts correctly when + the lookup function throws an exception""" + + class Cls(object): + @descriptors.cached() + def fn(self, arg1): + @defer.inlineCallbacks + def inner_fn(): + yield async.run_on_reactor() + raise SynapseError(400, "blah") + + return inner_fn() + + @defer.inlineCallbacks + def do_lookup(): + with logcontext.LoggingContext() as c1: + c1.name = "c1" + try: + yield obj.fn(1) + self.fail("No exception thrown") + except SynapseError: + pass + + self.assertEqual(logcontext.LoggingContext.current_context(), + c1) + + obj = Cls() + + # set off a deferred which will do a cache lookup + d1 = do_lookup() + self.assertEqual(logcontext.LoggingContext.current_context(), + logcontext.LoggingContext.sentinel) + + return d1 -- cgit 1.4.1 From 86780a8bc3eac566d2c03a601f84b5ccf5737ceb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 28 Mar 2017 10:41:08 +0100 Subject: Don't convert to deferreds when not necessary --- synapse/push/push_tools.py | 7 ++----- synapse/util/async.py | 2 +- synapse/util/caches/descriptors.py | 5 ++++- synapse/util/logcontext.py | 3 +++ synapse/visibility.py | 3 ++- 5 files changed, 12 insertions(+), 8 deletions(-) (limited to 'synapse') diff --git a/synapse/push/push_tools.py b/synapse/push/push_tools.py index 287df94b4f..6835f54e97 100644 --- a/synapse/push/push_tools.py +++ b/synapse/push/push_tools.py @@ -17,15 +17,12 @@ from twisted.internet import defer from synapse.push.presentable_names import ( calculate_room_name, name_from_member_event ) -from synapse.util.logcontext import preserve_fn, preserve_context_over_deferred @defer.inlineCallbacks def get_badge_count(store, user_id): - invites, joins = yield preserve_context_over_deferred(defer.gatherResults([ - preserve_fn(store.get_invited_rooms_for_user)(user_id), - preserve_fn(store.get_rooms_for_user)(user_id), - ], consumeErrors=True)) + invites = yield store.get_invited_rooms_for_user(user_id) + joins = yield store.get_rooms_for_user(user_id) my_receipts_by_room = yield store.get_receipts_for_user( user_id, "m.read", diff --git a/synapse/util/async.py b/synapse/util/async.py index 35380bf8ed..8495de496a 100644 --- a/synapse/util/async.py +++ b/synapse/util/async.py @@ -101,7 +101,7 @@ class ObservableDeferred(object): return d else: success, res = self._result - return defer.succeed(res) if success else defer.fail(res) + return res if success else defer.fail(res) def observers(self): return self._observers diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 5c30ed235d..1607978e29 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -341,7 +341,10 @@ class CacheDescriptor(_CacheDescriptorBase): cache.set(cache_key, result_d, callback=invalidate_callback) observer = result_d.observe() - return logcontext.make_deferred_yieldable(observer) + if isinstance(observer, defer.Deferred): + return logcontext.make_deferred_yieldable(observer) + else: + return observer wrapped.invalidate = cache.invalidate wrapped.invalidate_all = cache.invalidate_all diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 857afee7cb..183d9cf62f 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -315,6 +315,9 @@ def preserve_context_over_deferred(deferred, context=None): the deferred follow the synapse logcontext rules: try ``make_deferred_yieldable`` instead. """ + if not isinstance(deferred, defer.Deferred): + return deferred + if context is None: context = LoggingContext.current_context() d = _PreservingContextDeferred(context) diff --git a/synapse/visibility.py b/synapse/visibility.py index 31659156ae..c4dd9ae2c7 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -56,7 +56,8 @@ def filter_events_for_clients(store, user_tuples, events, event_id_to_state): events ([synapse.events.EventBase]): list of events to filter """ forgotten = yield preserve_context_over_deferred(defer.gatherResults([ - preserve_fn(store.who_forgot_in_room)( + defer.maybeDeferred( + preserve_fn(store.who_forgot_in_room), room_id, ) for room_id in frozenset(e.room_id for e in events) -- cgit 1.4.1 From 014fee93b38ea93ee7dd9f9f9211895272e50834 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 28 Mar 2017 11:14:15 +0100 Subject: Manually calculate cache key as getcallargs is expensive This is because getcallargs recomputes the getargspec, amongst other things, which we don't need to do as its already been done --- synapse/util/caches/descriptors.py | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 1607978e29..eed60d567e 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -197,6 +197,7 @@ class _CacheDescriptorBase(object): arg_spec = inspect.getargspec(orig) all_args = arg_spec.args + self.arg_spec = arg_spec if "cache_context" in all_args: if not cache_context: @@ -226,6 +227,14 @@ class _CacheDescriptorBase(object): self.num_args = num_args self.arg_names = all_args[1:num_args + 1] + if arg_spec.defaults: + self.arg_defaults = dict(zip( + all_args[-len(arg_spec.defaults):], + arg_spec.defaults + )) + else: + self.arg_defaults = {} + if "cache_context" in self.arg_names: raise Exception( "cache_context arg cannot be included among the cache keys" @@ -289,18 +298,31 @@ class CacheDescriptor(_CacheDescriptorBase): iterable=self.iterable, ) + def get_cache_key(args, kwargs): + """Given some args/kwargs return a generator that resolves into + the cache_key. + + We loop through each arg name, looking up if its in the `kwargs`, + otherwise using the next argument in `args`. If there are no more + args then we try looking the arg name up in the defaults + """ + pos = 0 + for nm in self.arg_names: + if nm in kwargs: + yield kwargs[nm] + elif pos < len(args): + yield args[pos] + pos += 1 + else: + yield self.arg_defaults[nm] + @functools.wraps(self.orig) def wrapped(*args, **kwargs): # If we're passed a cache_context then we'll want to call its invalidate() # whenever we are invalidated invalidate_callback = kwargs.pop("on_invalidate", None) - # Add temp cache_context so inspect.getcallargs doesn't explode - if self.add_cache_context: - kwargs["cache_context"] = None - - arg_dict = inspect.getcallargs(self.orig, obj, *args, **kwargs) - cache_key = tuple(arg_dict[arg_nm] for arg_nm in self.arg_names) + cache_key = tuple(get_cache_key(args, kwargs)) # Add our own `cache_context` to argument list if the wrapped function # has asked for one -- cgit 1.4.1 From 6194a64ae913aa400e19c3a9bd9348ce2bc11305 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 Mar 2017 14:19:10 +0100 Subject: Doc new instance variables --- synapse/util/caches/descriptors.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index eed60d567e..1f02cca8a5 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -197,7 +197,6 @@ class _CacheDescriptorBase(object): arg_spec = inspect.getargspec(orig) all_args = arg_spec.args - self.arg_spec = arg_spec if "cache_context" in all_args: if not cache_context: @@ -225,8 +224,16 @@ class _CacheDescriptorBase(object): ) self.num_args = num_args + + # list of the names of the args used as the cache key self.arg_names = all_args[1:num_args + 1] + # The arg spec of the wrapped function, see `inspect.getargspec` for + # the type. + self.arg_spec = arg_spec + + # self.arg_defaults is a map of arg name to its default value for each + # argument that has a default value if arg_spec.defaults: self.arg_defaults = dict(zip( all_args[-len(arg_spec.defaults):], -- cgit 1.4.1 From 9ff4e0e91bc877b05dc06b69f60fc031fe8dcaac Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 Mar 2017 16:37:40 +0100 Subject: Bump version and changelog --- CHANGES.rst | 44 ++++++++++++++++++++++++++++++++++++++++++++ synapse/__init__.py | 2 +- 2 files changed, 45 insertions(+), 1 deletion(-) (limited to 'synapse') diff --git a/CHANGES.rst b/CHANGES.rst index 2a46af52a8..6659c6671a 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,3 +1,47 @@ +Changes in synapse v0.20.0-rc1 (2017-03-30) +=========================================== + +Features: + +* Add delete_devices API (PR #1993) +* Add phone number registration/login support (PR #1994, #2055) + + +Changes: + +* Use JSONSchema for validation of filters. Thanks @pik! (PR #1783) +* Reread log config on SIGHUP (PR #1982) +* Speed up public room list (PR #1989) +* Add helpful texts to logger config options (PR #1990) +* Minor ``/sync`` performance improvements. (PR #2002, #2013, #2022) +* Add some debug to help diagnose weird federation issue (PR #2035) +* Correctly limit retries for all federation requests (PR #2050, #2061) +* Don't lock table when persisting new one time keys (PR #2053) +* Reduce some CPU work on DB threads (PR #2054) +* Cache hosts in room (PR #2060) +* Batch sending of device list pokes (PR #2063) +* Speed up persist event path in certain edge cases (PR #2070) + + +Bug fixes: + +* Fix bug where current_state_events renamed to current_state_ids (PR #1849) +* Fix routing loop when fetching remote media (PR #1992) +* Fix current_state_events table to not lie (PR #1996) +* Fix CAS login to handle PartialDownloadError (PR #1997) +* Fix assertion to stop transaction queue getting wedged (PR #2010) +* Fix presence to fallback to last_active_ts if it beats the last sync time. + Thanks @Half-Shot! (PR #2014) +* Fix bug when federation received a PDU while a room join is in progress (PR + #2016) +* Fix resetting state on rejected events (PR #2025) +* Fix installation issues in readme. Thanks @ricco386 (PR #2037) +* Fix caching of remote servers' signature keys (PR #2042) +* Fix some leaking log context (PR #2048, #2049, #2057, #2058) +* Fix rejection of invites not reaching sync (PR #2056) + + + Changes in synapse v0.19.3 (2017-03-20) ======================================= diff --git a/synapse/__init__.py b/synapse/__init__.py index 7628e7c505..580927abf4 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -16,4 +16,4 @@ """ This is a reference implementation of a Matrix home server. """ -__version__ = "0.19.3" +__version__ = "0.20.0-rc1" -- cgit 1.4.1 From b282fe7170142191c8ce795270422754ab4bc58e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 Mar 2017 17:03:59 +0100 Subject: Revert log context change --- synapse/util/logcontext.py | 3 --- 1 file changed, 3 deletions(-) (limited to 'synapse') diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 183d9cf62f..857afee7cb 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -315,9 +315,6 @@ def preserve_context_over_deferred(deferred, context=None): the deferred follow the synapse logcontext rules: try ``make_deferred_yieldable`` instead. """ - if not isinstance(deferred, defer.Deferred): - return deferred - if context is None: context = LoggingContext.current_context() d = _PreservingContextDeferred(context) -- cgit 1.4.1 From 5b5b171f3e9223351f72150ea73e1c9797144eae Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 Mar 2017 17:05:53 +0100 Subject: Docs --- synapse/util/async.py | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'synapse') diff --git a/synapse/util/async.py b/synapse/util/async.py index 8495de496a..1453faf0ef 100644 --- a/synapse/util/async.py +++ b/synapse/util/async.py @@ -89,6 +89,11 @@ class ObservableDeferred(object): deferred.addCallbacks(callback, errback) def observe(self): + """Observe the underlying deferred. + + Can return either a deferred if the underlying deferred is still pending + (or has failed), or the actual value. Callers may need to use maybeDeferred. + """ if not self._result: d = defer.Deferred() -- cgit 1.4.1 From 27b1b4a2c958b04f37732d19f163dcfab12ad0a7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 Mar 2017 17:50:31 +0100 Subject: Speed up copy_and_replace --- synapse/types.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'synapse') diff --git a/synapse/types.py b/synapse/types.py index 9666f9d73f..c87ed813b9 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -216,9 +216,7 @@ class StreamToken( return self def copy_and_replace(self, key, new_value): - d = self._asdict() - d[key] = new_value - return StreamToken(**d) + return self._replace(**{key: new_value}) StreamToken.START = StreamToken( -- cgit 1.4.1 From 4d17add8de6d1c3bcd073246519f3cdaa5063bed Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 31 Mar 2017 09:38:27 +0100 Subject: Remove unused instance variable --- synapse/util/caches/descriptors.py | 4 ---- 1 file changed, 4 deletions(-) (limited to 'synapse') diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 1f02cca8a5..9d0d0be1f9 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -228,10 +228,6 @@ class _CacheDescriptorBase(object): # list of the names of the args used as the cache key self.arg_names = all_args[1:num_args + 1] - # The arg spec of the wrapped function, see `inspect.getargspec` for - # the type. - self.arg_spec = arg_spec - # self.arg_defaults is a map of arg name to its default value for each # argument that has a default value if arg_spec.defaults: -- cgit 1.4.1 From 9ee397b440c01f2dd170c7f7341cb47b90cf2762 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Fri, 31 Mar 2017 13:54:26 +0100 Subject: switch to allow_guest=True for authing 3Ps as per PR feedback --- synapse/rest/client/v2_alpha/thirdparty.py | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'synapse') diff --git a/synapse/rest/client/v2_alpha/thirdparty.py b/synapse/rest/client/v2_alpha/thirdparty.py index ee2f158d03..6fceb23e26 100644 --- a/synapse/rest/client/v2_alpha/thirdparty.py +++ b/synapse/rest/client/v2_alpha/thirdparty.py @@ -36,6 +36,8 @@ class ThirdPartyProtocolsServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request): + yield self.auth.get_user_by_req(request, allow_guest=True) + protocols = yield self.appservice_handler.get_3pe_protocols() defer.returnValue((200, protocols)) @@ -52,6 +54,8 @@ class ThirdPartyProtocolServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): + yield self.auth.get_user_by_req(request, allow_guest=True) + protocols = yield self.appservice_handler.get_3pe_protocols( only_protocol=protocol, ) @@ -73,6 +77,8 @@ class ThirdPartyUserServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): + yield self.auth.get_user_by_req(request, allow_guest=True) + fields = request.args fields.pop("access_token", None) @@ -95,6 +101,8 @@ class ThirdPartyLocationServlet(RestServlet): @defer.inlineCallbacks def on_GET(self, request, protocol): + yield self.auth.get_user_by_req(request, allow_guest=True) + fields = request.args fields.pop("access_token", None) -- cgit 1.4.1 From 773e1c6d68223c787bff1da78baef519a70f8c3d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:14:11 +0100 Subject: Remove spurious @preserve_fn decorators Remove `@preserve_fn` decorators on `on_new_room_event`, `_notify_pending_new_room_events`, `_on_new_room_event`, `on_new_event`, and `on_new_replication_data` - none of these functions return a deferred, and the decorator does nothing unless the wrapped function returns a deferred, so the decorator was a no-op. --- synapse/notifier.py | 5 ----- 1 file changed, 5 deletions(-) (limited to 'synapse') diff --git a/synapse/notifier.py b/synapse/notifier.py index 7eeba6d28e..3b206bb96b 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -202,7 +202,6 @@ class Notifier(object): lambda: len(self.user_to_user_stream), ) - @preserve_fn def on_new_room_event(self, event, room_stream_id, max_room_stream_id, extra_users=[]): """ Used by handlers to inform the notifier something has happened @@ -224,7 +223,6 @@ class Notifier(object): self.notify_replication() - @preserve_fn def _notify_pending_new_room_events(self, max_room_stream_id): """Notify for the room events that were queued waiting for a previous event to be persisted. @@ -242,7 +240,6 @@ class Notifier(object): else: self._on_new_room_event(event, room_stream_id, extra_users) - @preserve_fn def _on_new_room_event(self, event, room_stream_id, extra_users=[]): """Notify any user streams that are interested in this room event""" # poke any interested application service. @@ -260,7 +257,6 @@ class Notifier(object): rooms=[event.room_id], ) - @preserve_fn def on_new_event(self, stream_key, new_token, users=[], rooms=[]): """ Used to inform listeners that something has happend event wise. @@ -287,7 +283,6 @@ class Notifier(object): self.notify_replication() - @preserve_fn def on_new_replication_data(self): """Used to inform replication listeners that something has happend without waking up any of the normal user event streams""" -- cgit 1.4.1 From e2eebf16963d9580a581a15308d2771dce875a83 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:38:02 +0100 Subject: Fix fixme in preserve_fn `preserve_fn` is no longer used as a decorator anywhere, so we can safely fix a fixme therein. --- synapse/util/logcontext.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/util/logcontext.py b/synapse/util/logcontext.py index 857afee7cb..990216145e 100644 --- a/synapse/util/logcontext.py +++ b/synapse/util/logcontext.py @@ -334,12 +334,8 @@ def preserve_fn(f): LoggingContext.set_current_context(LoggingContext.sentinel) return result - # XXX: why is this here rather than inside g? surely we want to preserve - # the context from the time the function was called, not when it was - # wrapped? - current = LoggingContext.current_context() - def g(*args, **kwargs): + current = LoggingContext.current_context() res = f(*args, **kwargs) if isinstance(res, defer.Deferred) and not res.called: # The function will have reset the context before returning, so -- cgit 1.4.1 From feb496056ee1a6d30174a2594dbe01e24dd4fb25 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:41:17 +0100 Subject: preserve_fn some deferred-returning things In `Notifier._on_new_room_event`, `preserve_fn` around its subroutines which return deferreds, so that it is safe to call it with an active logcontext. --- synapse/notifier.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'synapse') diff --git a/synapse/notifier.py b/synapse/notifier.py index 3b206bb96b..e8177452a8 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -243,10 +243,13 @@ class Notifier(object): def _on_new_room_event(self, event, room_stream_id, extra_users=[]): """Notify any user streams that are interested in this room event""" # poke any interested application service. - self.appservice_handler.notify_interested_services(room_stream_id) + preserve_fn(self.appservice_handler.notify_interested_services)( + room_stream_id) if self.federation_sender: - self.federation_sender.notify_new_events(room_stream_id) + preserve_fn(self.federation_sender.notify_new_events)( + room_stream_id + ) if event.type == EventTypes.Member and event.membership == Membership.JOIN: self._user_joined_room(event.state_key, event.room_id) -- cgit 1.4.1 From 65e1683680b656accd46f531e00d69b68a09c49e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:42:38 +0100 Subject: Remove spurious PreserveLoggingContext In `on_new_room_event`, remove `PreserveLoggingContext` - we can call its subroutines with the logcontext set. --- synapse/notifier.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/notifier.py b/synapse/notifier.py index e8177452a8..57d6a8cfe3 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -215,13 +215,12 @@ class Notifier(object): until all previous events have been persisted before notifying the client streams. """ - with PreserveLoggingContext(): - self.pending_new_room_events.append(( - room_stream_id, event, extra_users - )) - self._notify_pending_new_room_events(max_room_stream_id) + self.pending_new_room_events.append(( + room_stream_id, event, extra_users + )) + self._notify_pending_new_room_events(max_room_stream_id) - self.notify_replication() + self.notify_replication() def _notify_pending_new_room_events(self, max_room_stream_id): """Notify for the room events that were queued waiting for a previous -- cgit 1.4.1 From 0b08c48fc5269a04325791c96ddd389a7cfe502a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:43:37 +0100 Subject: Remove more spurious `PreserveLoggingContext`s Remove `PreserveLoggingContext` around calls to `Notifier.on_new_room_event`; there is no problem if the logcontext is set when calling it. --- synapse/handlers/federation.py | 43 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 24 deletions(-) (limited to 'synapse') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 888dd01240..737e2f7160 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -28,7 +28,7 @@ from synapse.api.constants import EventTypes, Membership, RejectedReason from synapse.events.validator import EventValidator from synapse.util import unwrapFirstError from synapse.util.logcontext import ( - PreserveLoggingContext, preserve_fn, preserve_context_over_deferred + preserve_fn, preserve_context_over_deferred ) from synapse.util.metrics import measure_func from synapse.util.logutils import log_function @@ -394,11 +394,10 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=extra_users + ) if event.type == EventTypes.Member: if event.membership == Membership.JOIN: @@ -916,11 +915,10 @@ class FederationHandler(BaseHandler): origin, auth_chain, state, event ) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=[joinee] - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=[joinee] + ) logger.debug("Finished joining %s to %s", joinee, room_id) finally: @@ -1025,10 +1023,9 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, extra_users=extra_users + ) if event.type == EventTypes.Member: if event.content["membership"] == Membership.JOIN: @@ -1074,11 +1071,10 @@ class FederationHandler(BaseHandler): ) target_user = UserID.from_string(event.state_key) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, - extra_users=[target_user], - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, + extra_users=[target_user], + ) defer.returnValue(event) @@ -1236,10 +1232,9 @@ class FederationHandler(BaseHandler): target_user = UserID.from_string(target_user_id) extra_users.append(target_user) - with PreserveLoggingContext(): - self.notifier.on_new_room_event( - event, event_stream_id, max_stream_id, extra_users=extra_users - ) + self.notifier.on_new_room_event( + event, event_stream_id, max_stream_id, extra_users=extra_users + ) defer.returnValue(None) -- cgit 1.4.1 From 7eb9f34cc3c2845a0ef35c9524f7ecc14339f7c1 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:44:19 +0100 Subject: Remove spurious yield In `MessageHandler`, remove `yield` on call to `Notifier.on_new_room_event`: it doesn't return anything anyway. --- synapse/handlers/message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'synapse') diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 7a498af5a2..348056add5 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -612,7 +612,7 @@ class MessageHandler(BaseHandler): @defer.inlineCallbacks def _notify(): yield run_on_reactor() - yield self.notifier.on_new_room_event( + self.notifier.on_new_room_event( event, event_stream_id, max_stream_id, extra_users=extra_users ) -- cgit 1.4.1 From 30bcbf775abbf8582a6fac2ac1b23a220508ea62 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Apr 2017 15:58:07 +0100 Subject: Accept join events from all servers Make sure that we accept join events from any server, rather than just the origin server, to make the federation join dance work correctly. (Fixes #1893). --- synapse/federation/federation_server.py | 8 ++++++-- synapse/handlers/federation.py | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 510a176821..bc20b9c201 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -146,11 +146,15 @@ class FederationServer(FederationBase): # check that it's actually being sent from a valid destination to # workaround bug #1753 in 0.18.5 and 0.18.6 if transaction.origin != get_domain_from_id(pdu.event_id): + # We continue to accept join events from any server; this is + # necessary for the federation join dance to work correctly. + # (When we join over federation, the "helper" server is + # responsible for sending out the join event, rather than the + # origin. See bug #1893). if not ( pdu.type == 'm.room.member' and pdu.content and - pdu.content.get("membership", None) == 'join' and - self.hs.is_mine_id(pdu.state_key) + pdu.content.get("membership", None) == 'join' ): logger.info( "Discarding PDU %s from invalid origin %s", diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 888dd01240..2ecc0087b8 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1004,9 +1004,19 @@ class FederationHandler(BaseHandler): ) event.internal_metadata.outlier = False - # Send this event on behalf of the origin server since they may not - # have an up to data view of the state of the room at this event so - # will not know which servers to send the event to. + # Send this event on behalf of the origin server. + # + # The reasons we have the destination server rather than the origin + # server send it are slightly mysterious: the origin server should have + # all the neccessary state once it gets the response to the send_join, + # so it could send the event itself if it wanted to. It may be that + # doing it this way reduces failure modes, or avoids certain attacks + # where a new server selectively tells a subset of the federation that + # it has joined. + # + # The fact is that, as of the current writing, Synapse doesn't send out + # the join event over federation after joining, and changing it now + # would introduce the danger of backwards-compatibility problems. event.internal_metadata.send_on_behalf_of = origin context, event_stream_id, max_stream_id = yield self._handle_new_event( -- cgit 1.4.1