From f42150a2be2f4ec60563087abee33c596a77c2fc Mon Sep 17 00:00:00 2001 From: Niklas Tittjung Date: Sun, 19 Jul 2020 16:32:52 +0200 Subject: [PATCH 1/5] Add option for server admins to join too complex rooms. Solves 7901. Signed-off-by: Niklas Tittjung --- synapse/config/server.py | 7 +++++++ synapse/handlers/room_member.py | 23 ++++++++++++++--------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 3586a7d49184..7dfbf39e251f 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -439,6 +439,9 @@ class LimitRemoteRoomsConfig(object): validator=attr.validators.instance_of(str), default=ROOM_COMPLEXITY_TOO_GREAT, ) + admins_can_join = attr.ib( + validator=attr.validators.instance_of(bool), default=False + ) self.limit_remote_rooms = LimitRemoteRoomsConfig( **(config.get("limit_remote_rooms") or {}) @@ -893,6 +896,10 @@ def generate_config_section( # #complexity_error: "This room is too complex." + # allow server admins to join complex rooms. Default is false. + # + #admins_can_join: true + # Whether to require a user to be in the room to add an alias to it. # Defaults to 'true'. # diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index a1a8fa1d3bc7..642f8ece92ed 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -953,16 +953,21 @@ async def _remote_join( raise SynapseError(404, "No known servers") if self.hs.config.limit_remote_rooms.enabled: - # Fetch the room complexity - too_complex = await self._is_remote_room_too_complex( - room_id, remote_room_hosts - ) - if too_complex is True: - raise SynapseError( - code=400, - msg=self.hs.config.limit_remote_rooms.complexity_error, - errcode=Codes.RESOURCE_LIMIT_EXCEEDED, + if self.hs.config.limit_remote_rooms.admins_can_join: + check_complexity = not await self.hs.auth.is_server_admin(user) + else: + check_complexity = True + if check_complexity: + # Fetch the room complexity + too_complex = await self._is_remote_room_too_complex( + room_id, remote_room_hosts ) + if too_complex is True: + raise SynapseError( + code=400, + msg=self.hs.config.limit_remote_rooms.complexity_error, + errcode=Codes.RESOURCE_LIMIT_EXCEEDED, + ) # We don't do an auth check if we are doing an invite # join dance for now, since we're kinda implicitly checking From 75418fa7c6ed3ad3efe078c3bf31ccd4f1e37e58 Mon Sep 17 00:00:00 2001 From: Niklas Tittjung Date: Sun, 19 Jul 2020 16:38:32 +0200 Subject: [PATCH 2/5] Add changelog file Signed-off-by: Niklas Tittjung --- changelog.d/7901.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7901.feature diff --git a/changelog.d/7901.feature b/changelog.d/7901.feature new file mode 100644 index 000000000000..5c31190e614a --- /dev/null +++ b/changelog.d/7901.feature @@ -0,0 +1 @@ +Add option to allow server admins to join too complex rooms. Contributed by @lugino-emeritus. From 3086a493c3fa7bb55694c4b78c2758613846889c Mon Sep 17 00:00:00 2001 From: Niklas Tittjung Date: Sun, 19 Jul 2020 16:59:57 +0200 Subject: [PATCH 3/5] Correct changelog number of pull-request to 7902 Signed-off-by: Niklas Tittjung --- changelog.d/{7901.feature => 7902.feature} | 0 docs/sample_config.yaml | 4 ++++ 2 files changed, 4 insertions(+) rename changelog.d/{7901.feature => 7902.feature} (100%) diff --git a/changelog.d/7901.feature b/changelog.d/7902.feature similarity index 100% rename from changelog.d/7901.feature rename to changelog.d/7902.feature diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 0e83f855bbf6..ae4ca3f25cd6 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -314,6 +314,10 @@ limit_remote_rooms: # #complexity_error: "This room is too complex." + # allow server admins to join complex rooms. Default is false. + # + #admins_can_join: true + # Whether to require a user to be in the room to add an alias to it. # Defaults to 'true'. # From 138339109ea47845e5cff60306348ef13aa9617d Mon Sep 17 00:00:00 2001 From: Niklas Tittjung Date: Mon, 20 Jul 2020 12:23:13 +0200 Subject: [PATCH 4/5] Add tests and solved an exception. Signed-off-by: Niklas Tittjung --- synapse/handlers/room_member.py | 31 +++++---- tests/federation/test_complexity.py | 101 ++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 642f8ece92ed..5a40e8c1446e 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -952,22 +952,21 @@ async def _remote_join( if len(remote_room_hosts) == 0: raise SynapseError(404, "No known servers") - if self.hs.config.limit_remote_rooms.enabled: - if self.hs.config.limit_remote_rooms.admins_can_join: - check_complexity = not await self.hs.auth.is_server_admin(user) - else: - check_complexity = True - if check_complexity: - # Fetch the room complexity - too_complex = await self._is_remote_room_too_complex( - room_id, remote_room_hosts + check_complexity = self.hs.config.limit_remote_rooms.enabled + if check_complexity and self.hs.config.limit_remote_rooms.admins_can_join: + check_complexity = not await self.hs.auth.is_server_admin(user) + + if check_complexity: + # Fetch the room complexity + too_complex = await self._is_remote_room_too_complex( + room_id, remote_room_hosts + ) + if too_complex is True: + raise SynapseError( + code=400, + msg=self.hs.config.limit_remote_rooms.complexity_error, + errcode=Codes.RESOURCE_LIMIT_EXCEEDED, ) - if too_complex is True: - raise SynapseError( - code=400, - msg=self.hs.config.limit_remote_rooms.complexity_error, - errcode=Codes.RESOURCE_LIMIT_EXCEEDED, - ) # We don't do an auth check if we are doing an invite # join dance for now, since we're kinda implicitly checking @@ -980,7 +979,7 @@ async def _remote_join( # Check the room we just joined wasn't too large, if we didn't fetch the # complexity of it before. - if self.hs.config.limit_remote_rooms.enabled: + if check_complexity: if too_complex is False: # We checked, and we're under the limit. return event_id, stream_id diff --git a/tests/federation/test_complexity.py b/tests/federation/test_complexity.py index 0c9987be54e3..6eb022eda4d3 100644 --- a/tests/federation/test_complexity.py +++ b/tests/federation/test_complexity.py @@ -141,3 +141,104 @@ def test_join_too_large_once_joined(self): f = self.get_failure(d, SynapseError) self.assertEqual(f.value.code, 400) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + + def test_join_too_large_admin(self): + u1 = self.register_user("u1", "pass", admin=True) + + handler = self.hs.get_room_member_handler() + fed_transport = self.hs.get_federation_transport_client() + + # Mock out some things, because we don't want to test the whole join + fed_transport.client.get_json = Mock(return_value=defer.succeed({"v1": 9999})) + handler.federation_handler.do_invite_join = Mock( + return_value=defer.succeed(("", 1)) + ) + + d = handler._remote_join( + None, + ["other.example.com"], + "roomid", + UserID.from_string(u1), + {"membership": "join"}, + ) + + self.pump() + + # The request failed with a SynapseError saying the resource limit was + # exceeded. + f = self.get_failure(d, SynapseError) + self.assertEqual(f.value.code, 400, f.value) + self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + + +class RoomComplexityAdminTests(unittest.FederatingHomeserverTestCase): + + servlets = [ + admin.register_servlets, + room.register_servlets, + login.register_servlets, + ] + + def default_config(self): + config = super().default_config() + config["limit_remote_rooms"] = { + "enabled": True, + "complexity": 0.05, + "admins_can_join": True, + } + return config + + def test_join_too_large_noadmin(self): + + u1 = self.register_user("u1", "pass") + + handler = self.hs.get_room_member_handler() + fed_transport = self.hs.get_federation_transport_client() + + # Mock out some things, because we don't want to test the whole join + fed_transport.client.get_json = Mock(return_value=defer.succeed({"v1": 9999})) + handler.federation_handler.do_invite_join = Mock( + return_value=defer.succeed(("", 1)) + ) + + d = handler._remote_join( + None, + ["other.example.com"], + "roomid", + UserID.from_string(u1), + {"membership": "join"}, + ) + + self.pump() + + # The request failed with a SynapseError saying the resource limit was + # exceeded. + f = self.get_failure(d, SynapseError) + self.assertEqual(f.value.code, 400, f.value) + self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + + def test_join_too_large_admin(self): + u1 = self.register_user("u1", "pass", admin=True) + + handler = self.hs.get_room_member_handler() + fed_transport = self.hs.get_federation_transport_client() + + # Mock out some things, because we don't want to test the whole join + fed_transport.client.get_json = Mock(return_value=defer.succeed({"v1": 9999})) + handler.federation_handler.do_invite_join = Mock( + return_value=defer.succeed(("", 1)) + ) + + d = handler._remote_join( + None, + ["other.example.com"], + "roomid", + UserID.from_string(u1), + {"membership": "join"}, + ) + + self.pump() + + # The request success since the user is an admin + res = self.get_success(d) + self.assertEqual(res, ("", 1)) From f54c3d9e4bd36456709d83516de95248ebf94261 Mon Sep 17 00:00:00 2001 From: Niklas Tittjung Date: Fri, 24 Jul 2020 16:22:16 +0200 Subject: [PATCH 5/5] Add test docstrings. Signed-off-by: Niklas Tittjung --- changelog.d/7902.feature | 2 +- tests/federation/test_complexity.py | 62 ++++++++++++++++------------- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/changelog.d/7902.feature b/changelog.d/7902.feature index 5c31190e614a..4feae8cc2955 100644 --- a/changelog.d/7902.feature +++ b/changelog.d/7902.feature @@ -1 +1 @@ -Add option to allow server admins to join too complex rooms. Contributed by @lugino-emeritus. +Add option to allow server admins to join rooms which fail complexity checks. Contributed by @lugino-emeritus. diff --git a/tests/federation/test_complexity.py b/tests/federation/test_complexity.py index 6eb022eda4d3..5cd0510f0d7e 100644 --- a/tests/federation/test_complexity.py +++ b/tests/federation/test_complexity.py @@ -99,37 +99,25 @@ def test_join_too_large(self): self.assertEqual(f.value.code, 400, f.value) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) - def test_join_too_large_once_joined(self): - - u1 = self.register_user("u1", "pass") - u1_token = self.login("u1", "pass") + def test_join_too_large_admin(self): + # Check whether an admin can join if option "admins_can_join" is undefined, + # this option defaults to false, so the join should fail. - # Ok, this might seem a bit weird -- I want to test that we actually - # leave the room, but I don't want to simulate two servers. So, we make - # a local room, which we say we're joining remotely, even if there's no - # remote, because we mock that out. Then, we'll leave the (actually - # local) room, which will be propagated over federation in a real - # scenario. - room_1 = self.helper.create_room_as(u1, tok=u1_token) + u1 = self.register_user("u1", "pass", admin=True) handler = self.hs.get_room_member_handler() fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=defer.succeed(None)) + fed_transport.client.get_json = Mock(return_value=defer.succeed({"v1": 9999})) handler.federation_handler.do_invite_join = Mock( return_value=defer.succeed(("", 1)) ) - # Artificially raise the complexity - self.hs.get_datastore().get_current_state_event_counts = lambda x: defer.succeed( - 600 - ) - d = handler._remote_join( None, ["other.example.com"], - room_1, + "roomid", UserID.from_string(u1), {"membership": "join"}, ) @@ -139,25 +127,40 @@ def test_join_too_large_once_joined(self): # The request failed with a SynapseError saying the resource limit was # exceeded. f = self.get_failure(d, SynapseError) - self.assertEqual(f.value.code, 400) + self.assertEqual(f.value.code, 400, f.value) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) - def test_join_too_large_admin(self): - u1 = self.register_user("u1", "pass", admin=True) + def test_join_too_large_once_joined(self): + + u1 = self.register_user("u1", "pass") + u1_token = self.login("u1", "pass") + + # Ok, this might seem a bit weird -- I want to test that we actually + # leave the room, but I don't want to simulate two servers. So, we make + # a local room, which we say we're joining remotely, even if there's no + # remote, because we mock that out. Then, we'll leave the (actually + # local) room, which will be propagated over federation in a real + # scenario. + room_1 = self.helper.create_room_as(u1, tok=u1_token) handler = self.hs.get_room_member_handler() fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=defer.succeed({"v1": 9999})) + fed_transport.client.get_json = Mock(return_value=defer.succeed(None)) handler.federation_handler.do_invite_join = Mock( return_value=defer.succeed(("", 1)) ) + # Artificially raise the complexity + self.hs.get_datastore().get_current_state_event_counts = lambda x: defer.succeed( + 600 + ) + d = handler._remote_join( None, ["other.example.com"], - "roomid", + room_1, UserID.from_string(u1), {"membership": "join"}, ) @@ -167,11 +170,13 @@ def test_join_too_large_admin(self): # The request failed with a SynapseError saying the resource limit was # exceeded. f = self.get_failure(d, SynapseError) - self.assertEqual(f.value.code, 400, f.value) + self.assertEqual(f.value.code, 400) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) class RoomComplexityAdminTests(unittest.FederatingHomeserverTestCase): + # Test the behavior of joining rooms which exceed the complexity if option + # limit_remote_rooms.admins_can_join is True. servlets = [ admin.register_servlets, @@ -188,7 +193,9 @@ def default_config(self): } return config - def test_join_too_large_noadmin(self): + def test_join_too_large_no_admin(self): + # A user which is not an admin should not be able to join a remote room + # which is too complex. u1 = self.register_user("u1", "pass") @@ -218,6 +225,8 @@ def test_join_too_large_noadmin(self): self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) def test_join_too_large_admin(self): + # An admin should be able to join rooms where a complexity check fails. + u1 = self.register_user("u1", "pass", admin=True) handler = self.hs.get_room_member_handler() @@ -240,5 +249,4 @@ def test_join_too_large_admin(self): self.pump() # The request success since the user is an admin - res = self.get_success(d) - self.assertEqual(res, ("", 1)) + self.get_success(d)