From 80d107093c9bb69d9b90ba1515cf886cfd3acc47 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 11:18:06 +0100 Subject: [PATCH 1/8] Improve the reliability of auto-joining remote rooms --- changelog.d/10237.misc | 1 + synapse/handlers/register.py | 31 +++++++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 changelog.d/10237.misc diff --git a/changelog.d/10237.misc b/changelog.d/10237.misc new file mode 100644 index 000000000000..d76c119a4196 --- /dev/null +++ b/changelog.d/10237.misc @@ -0,0 +1 @@ +Improve the reliability of auto-joining remote rooms. diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 4ceef3fab3d9..38d9b49be4fa 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -386,10 +386,25 @@ async def _create_and_join_rooms(self, user_id: str) -> None: room_alias = RoomAlias.from_string(r) if self.hs.hostname != room_alias.domain: - logger.warning( - "Cannot create room alias %s, " - "it does not match server domain", - r, + # If the alias is remote, try to join the room. This might fail + # because the room might be invite only, but we don't have any local + # user in the room to invite this one with, so at this point that's + # the best we can do. + ( + room, + remote_room_hosts, + ) = await room_member_handler.lookup_room_alias(room_alias) + room_id = room.to_string() + + await room_member_handler.update_membership( + requester=create_requester( + user_id, authenticated_entity=self._server_name + ), + target=UserID.from_string(user_id), + room_id=room_id, + remote_room_hosts=remote_room_hosts, + action="join", + ratelimit=False, ) else: # A shallow copy is OK here since the only key that is @@ -465,6 +480,14 @@ async def _join_rooms(self, user_id: str) -> None: join_rule = join_rules_event.content.get("join_rule", None) requires_invite = join_rule and join_rule != JoinRules.PUBLIC + hosts_in_room = ( + await self.state_handler.get_current_hosts_in_room(room_id) + ) + if self.server_name not in hosts_in_room: + # The server isn't in the room, so there's no way for us to craft up + # an invite. Instead, try to join and see what happens. + requires_invite = False + # Send the invite, if necessary. if requires_invite: # If an invite is required, there must be a auto-join user ID. From ec26a51411fa92e867726a26fc77e6299ce10e1d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 11:25:30 +0100 Subject: [PATCH 2/8] Error if the auto join room is invite only but we don't have a user to invite with --- synapse/handlers/register.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 38d9b49be4fa..5f3df78b88e0 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -463,9 +463,15 @@ async def _join_rooms(self, user_id: str) -> None: ) # Calculate whether the room requires an invite or can be - # joined directly. Note that unless a join rule of public exists, - # it is treated as requiring an invite. - requires_invite = True + # joined directly. By default, we consider the room as requiring an + # invite if the homeserver is in the room (unless told otherwise by the + # join rules). Otherwise we consider it as being joinable, at the risk of + # failing to join, but in this case there's little more we can do since + # we don't have a local user in the room to craft up an invite with. + hosts_in_room = ( + await self.state_handler.get_current_hosts_in_room(room_id) + ) + requires_invite = self.server_name in hosts_in_room state = await self.store.get_filtered_current_state_ids( room_id, StateFilter.from_types([(EventTypes.JoinRules, "")]) @@ -480,13 +486,12 @@ async def _join_rooms(self, user_id: str) -> None: join_rule = join_rules_event.content.get("join_rule", None) requires_invite = join_rule and join_rule != JoinRules.PUBLIC - hosts_in_room = ( - await self.state_handler.get_current_hosts_in_room(room_id) - ) - if self.server_name not in hosts_in_room: - # The server isn't in the room, so there's no way for us to craft up - # an invite. Instead, try to join and see what happens. - requires_invite = False + if requires_invite and self.server_name not in hosts_in_room: + raise SynapseError( + 400, + "Auto-join room is invite only but there are no local" + " user to invite with", + ) # Send the invite, if necessary. if requires_invite: From 258dad293ef7b1fffc224c4a08c512fa2a19e359 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 11:32:33 +0100 Subject: [PATCH 3/8] Lint --- synapse/handlers/register.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 5f3df78b88e0..f88894120a45 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -468,8 +468,8 @@ async def _join_rooms(self, user_id: str) -> None: # join rules). Otherwise we consider it as being joinable, at the risk of # failing to join, but in this case there's little more we can do since # we don't have a local user in the room to craft up an invite with. - hosts_in_room = ( - await self.state_handler.get_current_hosts_in_room(room_id) + hosts_in_room = await self.state_handler.get_current_hosts_in_room( + room_id ) requires_invite = self.server_name in hosts_in_room From e2285e33046adff2c0d8b0a3684eac6968437811 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 11:38:07 +0100 Subject: [PATCH 4/8] Still warn when trying to create a room with a remote alias --- synapse/handlers/register.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index f88894120a45..6da49a1a80dd 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -390,6 +390,12 @@ async def _create_and_join_rooms(self, user_id: str) -> None: # because the room might be invite only, but we don't have any local # user in the room to invite this one with, so at this point that's # the best we can do. + logger.warning( + "Cannot automatically create room with alias %s as it isn't" + " local, trying to join the room instead", + r, + ) + ( room, remote_room_hosts, From ca298db31d5218ae98f2b5cb70147b26d5895837 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 11:58:31 +0100 Subject: [PATCH 5/8] Incorporate review --- synapse/handlers/register.py | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 6da49a1a80dd..3e16e530ff62 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -474,30 +474,22 @@ async def _join_rooms(self, user_id: str) -> None: # join rules). Otherwise we consider it as being joinable, at the risk of # failing to join, but in this case there's little more we can do since # we don't have a local user in the room to craft up an invite with. - hosts_in_room = await self.state_handler.get_current_hosts_in_room( - room_id - ) - requires_invite = self.server_name in hosts_in_room - - state = await self.store.get_filtered_current_state_ids( - room_id, StateFilter.from_types([(EventTypes.JoinRules, "")]) - ) + requires_invite = self.store.is_host_joined(room_id, self.server_name) - event_id = state.get((EventTypes.JoinRules, "")) - if event_id: - join_rules_event = await self.store.get_event( - event_id, allow_none=True + if requires_invite: + # If the server is in the room, check if the room is public. + state = await self.store.get_filtered_current_state_ids( + room_id, StateFilter.from_types([(EventTypes.JoinRules, "")]) ) - if join_rules_event: - join_rule = join_rules_event.content.get("join_rule", None) - requires_invite = join_rule and join_rule != JoinRules.PUBLIC - - if requires_invite and self.server_name not in hosts_in_room: - raise SynapseError( - 400, - "Auto-join room is invite only but there are no local" - " user to invite with", - ) + + event_id = state.get((EventTypes.JoinRules, "")) + if event_id: + join_rules_event = await self.store.get_event( + event_id, allow_none=True + ) + if join_rules_event: + join_rule = join_rules_event.content.get("join_rule", None) + requires_invite = join_rule and join_rule != JoinRules.PUBLIC # Send the invite, if necessary. if requires_invite: From 1ac345fe4f50b11377055f0f752e4d2f106625b6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 14:13:03 +0100 Subject: [PATCH 6/8] Test + async fix --- synapse/handlers/register.py | 9 ++++-- tests/handlers/test_register.py | 49 ++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 3e16e530ff62..34509904897a 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -474,7 +474,10 @@ async def _join_rooms(self, user_id: str) -> None: # join rules). Otherwise we consider it as being joinable, at the risk of # failing to join, but in this case there's little more we can do since # we don't have a local user in the room to craft up an invite with. - requires_invite = self.store.is_host_joined(room_id, self.server_name) + requires_invite = await self.store.is_host_joined( + room_id, + self.server_name, + ) if requires_invite: # If the server is in the room, check if the room is public. @@ -489,7 +492,9 @@ async def _join_rooms(self, user_id: str) -> None: ) if join_rules_event: join_rule = join_rules_event.content.get("join_rule", None) - requires_invite = join_rule and join_rule != JoinRules.PUBLIC + requires_invite = ( + join_rule and join_rule != JoinRules.PUBLIC + ) # Send the invite, if necessary. if requires_invite: diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index bd4319052338..9752a148e2ab 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -18,7 +18,7 @@ from synapse.api.constants import UserTypes from synapse.api.errors import Codes, ResourceLimitError, SynapseError from synapse.spam_checker_api import RegistrationBehaviour -from synapse.types import RoomAlias, UserID, create_requester +from synapse.types import RoomAlias, RoomID, UserID, create_requester from tests.test_utils import make_awaitable from tests.unittest import override_config @@ -595,3 +595,50 @@ async def get_or_create_user( ) return user_id, token + + +class RemoteAutoJoinTestCase(unittest.HomeserverTestCase): + """ Tests auto-join on remote rooms. """ + + def make_homeserver(self, reactor, clock): + self.room_id = "!roomid:remotetest" + + async def update_membership(*args, **kwargs): + pass + + async def lookup_room_alias(*args, **kwargs): + return RoomID.from_string(self.room_id), ["remotetest"] + + self.room_member_handler = Mock(spec=["update_membership", "lookup_room_alias"]) + self.room_member_handler.update_membership.side_effect = update_membership + self.room_member_handler.lookup_room_alias.side_effect = lookup_room_alias + + hs = self.setup_test_homeserver(room_member_handler=self.room_member_handler) + return hs + + def prepare(self, reactor, clock, hs): + self.handler = self.hs.get_registration_handler() + self.store = self.hs.get_datastore() + + @override_config({"auto_join_rooms": ["#room:remotetest"]}) + def test_auto_create_auto_join_remote_room(self): + """Tests that we don't attempt to create remote rooms, and that we don't attempt + to invite ourselves to rooms we're not in.""" + + # Register a first user; this should call _create_and_join_rooms + self.get_success(self.handler.register_user(localpart="jeff")) + + _, kwargs = self.room_member_handler.update_membership.call_args + + self.assertEqual(kwargs["room_id"], self.room_id) + self.assertEqual(kwargs["action"], "join") + self.assertEqual(kwargs["remote_room_hosts"], ["remotetest"]) + + # Register a second user; this should call _join_rooms + self.get_success(self.handler.register_user(localpart="jeff2")) + + _, kwargs = self.room_member_handler.update_membership.call_args + + self.assertEqual(kwargs["room_id"], self.room_id) + self.assertEqual(kwargs["action"], "join") + self.assertEqual(kwargs["remote_room_hosts"], ["remotetest"]) From 262b2021b82cfd9188b865460e32b780dd576902 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 15:17:25 +0200 Subject: [PATCH 7/8] Looks like I forgot to update black --- tests/handlers/test_register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 9752a148e2ab..3535ac5cc8c8 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -598,7 +598,7 @@ async def get_or_create_user( class RemoteAutoJoinTestCase(unittest.HomeserverTestCase): - """ Tests auto-join on remote rooms. """ + """Tests auto-join on remote rooms.""" def make_homeserver(self, reactor, clock): self.room_id = "!roomid:remotetest" From 16d889f272db41c71628974bca8d095d834e25a6 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 23 Jun 2021 14:25:35 +0100 Subject: [PATCH 8/8] Downgrade warning to info --- synapse/handlers/register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 34509904897a..ec15245d5d28 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -390,7 +390,7 @@ async def _create_and_join_rooms(self, user_id: str) -> None: # because the room might be invite only, but we don't have any local # user in the room to invite this one with, so at this point that's # the best we can do. - logger.warning( + logger.info( "Cannot automatically create room with alias %s as it isn't" " local, trying to join the room instead", r,