From 52aefd50860f9b44f48a9b465d42f26faa4eb84f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 7 Oct 2021 12:37:10 +0200 Subject: [PATCH 1/6] Catch AttributeErrors when calling registerProducer (#10995) Looks like the wrong exception type was caught in #10932. --- changelog.d/10995.bugfix | 1 + synapse/http/server.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog.d/10995.bugfix diff --git a/changelog.d/10995.bugfix b/changelog.d/10995.bugfix new file mode 100644 index 0000000000..3eef96f3db --- /dev/null +++ b/changelog.d/10995.bugfix @@ -0,0 +1 @@ +Correct a bugfix introduced in Synapse v1.44.0 that wouldn't catch every error of the connection breaks before a response could be written to it. diff --git a/synapse/http/server.py b/synapse/http/server.py index 0df1bfbeef..897ba5e453 100644 --- a/synapse/http/server.py +++ b/synapse/http/server.py @@ -563,7 +563,10 @@ class _ByteProducer: try: self._request.registerProducer(self, True) - except RuntimeError as e: + except AttributeError as e: + # Calling self._request.registerProducer might raise an AttributeError since + # the underlying Twisted code calls self._request.channel.registerProducer, + # however self._request.channel will be None if the connection was lost. logger.info("Connection disconnected before response was written: %r", e) # We drop our references to data we'll not use. From 86af6b2f0ef92a317900fd4a4f6d3436ff8a011c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 7 Oct 2021 12:20:03 +0100 Subject: [PATCH 2/6] Add a comment in _process_received_pdu (#11011) --- changelog.d/11011.misc | 1 + synapse/handlers/federation_event.py | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 changelog.d/11011.misc diff --git a/changelog.d/11011.misc b/changelog.d/11011.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/11011.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity. diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 243be46267..0645ce9392 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -894,6 +894,9 @@ class FederationEventHandler: backfilled=backfilled, ) except AuthError as e: + # FIXME richvdh 2021/10/07 I don't think this is reachable. Let's log it + # for now + logger.exception("Unexpected AuthError from _check_event_auth") raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) await self._run_push_actions_and_persist_event(event, context, backfilled) From 96fe77c2546598449c1d423c125f84c92620b155 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 7 Oct 2021 12:43:25 +0100 Subject: [PATCH 3/6] Improve the logging in _auth_and_persist_outliers (#11010) Include the event ids being peristed --- changelog.d/11010.misc | 1 + synapse/handlers/federation_event.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11010.misc diff --git a/changelog.d/11010.misc b/changelog.d/11010.misc new file mode 100644 index 0000000000..9a765435db --- /dev/null +++ b/changelog.d/11010.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity. diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 0645ce9392..f640b417b3 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1161,7 +1161,10 @@ class FederationEventHandler: return logger.info( - "Persisting %i of %i remaining events", len(roots), len(event_map) + "Persisting %i of %i remaining outliers: %s", + len(roots), + len(event_map), + shortstr(e.event_id for e in roots), ) await self._auth_and_persist_fetched_events_inner(origin, room_id, roots) From e0bf34dada709776ae00843e47cd811d1cd195c6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Oct 2021 13:26:11 +0100 Subject: [PATCH 4/6] Don't alter directory entries for local users when setting a per-room nickname (#11002) Co-authored-by: Patrick Cloke --- changelog.d/11002.bugfix | 1 + synapse/handlers/user_directory.py | 20 ++++++++++------ tests/handlers/test_user_directory.py | 34 +++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 7 deletions(-) create mode 100644 changelog.d/11002.bugfix diff --git a/changelog.d/11002.bugfix b/changelog.d/11002.bugfix new file mode 100644 index 0000000000..cf894a6314 --- /dev/null +++ b/changelog.d/11002.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where local users' per-room nicknames/avatars were visible to anyone who could see you in the user_directory. diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 97f60b5806..b7b1973346 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -203,6 +203,7 @@ class UserDirectoryHandler(StateDeltasHandler): public_value=Membership.JOIN, ) + is_remote = not self.is_mine_id(state_key) if change is MatchChange.now_false: # Need to check if the server left the room entirely, if so # we might need to remove all the users in that room @@ -224,15 +225,20 @@ class UserDirectoryHandler(StateDeltasHandler): else: logger.debug("Server is still in room: %r", room_id) - include_in_dir = not self.is_mine_id( - state_key - ) or await self.store.should_include_local_user_in_dir(state_key) + include_in_dir = ( + is_remote + or await self.store.should_include_local_user_in_dir(state_key) + ) if include_in_dir: if change is MatchChange.no_change: - # Handle any profile changes - await self._handle_profile_change( - state_key, room_id, prev_event_id, event_id - ) + # Handle any profile changes for remote users. + # (For local users we are not forced to scan membership + # events; instead the rest of the application calls + # `handle_local_profile_change`.) + if is_remote: + await self._handle_profile_change( + state_key, room_id, prev_event_id, event_id + ) continue if change is MatchChange.now_true: # The user joined diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 03fd5a3e2c..47217f0542 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -402,6 +402,40 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): public3 = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) self.assertEqual(set(public3), {(alice, room2), (bob, room2)}) + def test_per_room_profile_doesnt_alter_directory_entry(self) -> None: + alice = self.register_user("alice", "pass") + alice_token = self.login(alice, "pass") + bob = self.register_user("bob", "pass") + + # Alice should have a user directory entry created at registration. + users = self.get_success(self.user_dir_helper.get_profiles_in_user_directory()) + self.assertEqual( + users[alice], ProfileInfo(display_name="alice", avatar_url=None) + ) + + # Alice makes a room for herself. + room = self.helper.create_room_as(alice, is_public=True, tok=alice_token) + + # Alice sets a nickname unique to that room. + self.helper.send_state( + room, + "m.room.member", + { + "displayname": "Freddy Mercury", + "membership": "join", + }, + alice_token, + state_key=alice, + ) + + # Alice's display name remains the same in the user directory. + search_result = self.get_success(self.handler.search_users(bob, alice, 10)) + self.assertEqual( + search_result["results"], + [{"display_name": "alice", "avatar_url": None, "user_id": alice}], + 0, + ) + def test_private_room(self) -> None: """ A user can be searched for only by people that are either in a public From 7301019d48f1a4ca7683b1745be55cecc6fe4be3 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 7 Oct 2021 09:38:31 -0400 Subject: [PATCH 5/6] Ensure each cache config test uses separate state. (#11019) Hopefully this fixes these tests sometimes failing in CI. --- changelog.d/11019.misc | 1 + tests/config/test_cache.py | 20 ++++++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 changelog.d/11019.misc diff --git a/changelog.d/11019.misc b/changelog.d/11019.misc new file mode 100644 index 0000000000..aae5ee62b2 --- /dev/null +++ b/changelog.d/11019.misc @@ -0,0 +1 @@ +Ensure that cache config tests do not share state. diff --git a/tests/config/test_cache.py b/tests/config/test_cache.py index f518abdb7a..79d417568d 100644 --- a/tests/config/test_cache.py +++ b/tests/config/test_cache.py @@ -12,12 +12,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +from unittest.mock import patch + from synapse.config.cache import CacheConfig, add_resizable_cache from synapse.util.caches.lrucache import LruCache from tests.unittest import TestCase +# Patch the global _CACHES so that each test runs against its own state. +@patch("synapse.config.cache._CACHES", new_callable=dict) class CacheConfigTests(TestCase): def setUp(self): # Reset caches before each test @@ -26,7 +30,7 @@ class CacheConfigTests(TestCase): def tearDown(self): self.config.reset() - def test_individual_caches_from_environ(self): + def test_individual_caches_from_environ(self, _caches): """ Individual cache factors will be loaded from the environment. """ @@ -39,7 +43,7 @@ class CacheConfigTests(TestCase): self.assertEqual(dict(self.config.cache_factors), {"something_or_other": 2.0}) - def test_config_overrides_environ(self): + def test_config_overrides_environ(self, _caches): """ Individual cache factors defined in the environment will take precedence over those in the config. @@ -56,7 +60,7 @@ class CacheConfigTests(TestCase): {"foo": 1.0, "bar": 3.0, "something_or_other": 2.0}, ) - def test_individual_instantiated_before_config_load(self): + def test_individual_instantiated_before_config_load(self, _caches): """ If a cache is instantiated before the config is read, it will be given the default cache size in the interim, and then resized once the config @@ -72,7 +76,7 @@ class CacheConfigTests(TestCase): self.assertEqual(cache.max_size, 300) - def test_individual_instantiated_after_config_load(self): + def test_individual_instantiated_after_config_load(self, _caches): """ If a cache is instantiated after the config is read, it will be immediately resized to the correct size given the per_cache_factor if @@ -85,7 +89,7 @@ class CacheConfigTests(TestCase): add_resizable_cache("foo", cache_resize_callback=cache.set_cache_factor) self.assertEqual(cache.max_size, 200) - def test_global_instantiated_before_config_load(self): + def test_global_instantiated_before_config_load(self, _caches): """ If a cache is instantiated before the config is read, it will be given the default cache size in the interim, and then resized to the new @@ -100,7 +104,7 @@ class CacheConfigTests(TestCase): self.assertEqual(cache.max_size, 400) - def test_global_instantiated_after_config_load(self): + def test_global_instantiated_after_config_load(self, _caches): """ If a cache is instantiated after the config is read, it will be immediately resized to the correct size given the global factor if there @@ -113,7 +117,7 @@ class CacheConfigTests(TestCase): add_resizable_cache("foo", cache_resize_callback=cache.set_cache_factor) self.assertEqual(cache.max_size, 150) - def test_cache_with_asterisk_in_name(self): + def test_cache_with_asterisk_in_name(self, _caches): """Some caches have asterisks in their name, test that they are set correctly.""" config = { @@ -139,7 +143,7 @@ class CacheConfigTests(TestCase): add_resizable_cache("*cache_c*", cache_resize_callback=cache_c.set_cache_factor) self.assertEqual(cache_c.max_size, 200) - def test_apply_cache_factor_from_config(self): + def test_apply_cache_factor_from_config(self, _caches): """Caches can disable applying cache factor updates, mainly used by event cache size. """ From e79ee48313404abf8fbb7c88361e4ab1efa29a81 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Oct 2021 19:55:15 +0100 Subject: [PATCH 6/6] disallow-untyped-defs for synapse.server_notices (#11021) --- changelog.d/11021.misc | 1 + mypy.ini | 3 +++ synapse/server_notices/server_notices_manager.py | 8 ++------ 3 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 changelog.d/11021.misc diff --git a/changelog.d/11021.misc b/changelog.d/11021.misc new file mode 100644 index 0000000000..8ac1bfcf22 --- /dev/null +++ b/changelog.d/11021.misc @@ -0,0 +1 @@ +Add additional type hints to `synapse.server_notices`. \ No newline at end of file diff --git a/mypy.ini b/mypy.ini index a052d49c71..68437e5ce1 100644 --- a/mypy.ini +++ b/mypy.ini @@ -99,6 +99,9 @@ disallow_untyped_defs = True [mypy-synapse.rest.*] disallow_untyped_defs = True +[mypy-synapse.server_notices.*] +disallow_untyped_defs = True + [mypy-synapse.state.*] disallow_untyped_defs = True diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index cd1c5ff6f4..0cf60236f8 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -41,12 +41,8 @@ class ServerNoticesManager: self._notifier = hs.get_notifier() self.server_notices_mxid = self._config.servernotices.server_notices_mxid - def is_enabled(self): - """Checks if server notices are enabled on this server. - - Returns: - bool - """ + def is_enabled(self) -> bool: + """Checks if server notices are enabled on this server.""" return self.server_notices_mxid is not None async def send_notice(