From 1273fcc86e89f780d53574ffdbdb58de9fd7c376 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 10 Jan 2025 14:18:56 +0000 Subject: [PATCH 1/6] Add logic to handle deactivated and suspended users. --- docs/usage/configuration/config_documentation.md | 2 ++ synapse/config/auto_accept_invites.py | 4 ++++ synapse/events/auto_accept_invites.py | 15 +++++++++++++++ 3 files changed, 21 insertions(+) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 74522a1e4e5..8d1a3414c55 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4775,6 +4775,7 @@ are instead automatically joined to a room when receiving an invite. Set the `en enable auto-accepting invites. Defaults to false. This setting has the following sub-options: * `enabled`: Whether to run the auto-accept invites logic. Defaults to false. +* `accept_invites_for_suspended_users`: Whether to automatically accept invites for suspended users. Defaults to false. * `only_for_direct_messages`: Whether invites should be automatically accepted for all room types, or only for direct messages. Defaults to false. * `only_from_local_users`: Whether to only automatically accept invites from users on this homeserver. Defaults to false. @@ -4790,6 +4791,7 @@ Example configuration: ```yaml auto_accept_invites: enabled: true + accept_invites_for_suspended_users: false only_for_direct_messages: true only_from_local_users: true worker_to_run_on: "worker_1" diff --git a/synapse/config/auto_accept_invites.py b/synapse/config/auto_accept_invites.py index d90e13a5107..02e9a98a9d7 100644 --- a/synapse/config/auto_accept_invites.py +++ b/synapse/config/auto_accept_invites.py @@ -40,4 +40,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "only_from_local_users", False ) + self.accept_invites_for_suspended_users = auto_accept_invites_config.get( + "accept_invites_for_suspended_users", False + ) + self.worker_to_run_on = auto_accept_invites_config.get("worker_to_run_on") diff --git a/synapse/events/auto_accept_invites.py b/synapse/events/auto_accept_invites.py index d88ec51d9d1..a90aa12813a 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -87,6 +87,21 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: or is_from_local_user is True ) + # Check the user is activated. + recipient = await self._api.get_userinfo_by_id(event.state_key) + + # Ignore if the user doesn't exist. + if recipient is None: + return + + # Never accept invites for deactivated users. + if recipient.is_deactivated: + return + + # Only accept invites for suspended remote if the configuration mandates it. + if not self._config.accept_invites_for_suspended_users and recipient.suspended: + return + if ( is_invite_for_local_user and is_allowed_by_direct_message_rules From 583129ed477b52a84f31e2287e3214f50607f335 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 10 Jan 2025 14:20:20 +0000 Subject: [PATCH 2/6] Use short circuit logic. --- synapse/events/auto_accept_invites.py | 68 +++++++++++++-------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/synapse/events/auto_accept_invites.py b/synapse/events/auto_accept_invites.py index a90aa12813a..fe391f5e345 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -66,26 +66,29 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: event: The incoming event. """ # Check if the event is an invite for a local user. - is_invite_for_local_user = ( - event.type == EventTypes.Member - and event.is_state() - and event.membership == Membership.INVITE - and self._api.is_mine(event.state_key) - ) + if ( + event.type != EventTypes.Member + or event.is_state() is False + or event.membership != Membership.INVITE + or self._api.is_mine(event.state_key) is False + ): + return # Only accept invites for direct messages if the configuration mandates it. is_direct_message = event.content.get("is_direct", False) - is_allowed_by_direct_message_rules = ( - not self._config.accept_invites_only_for_direct_messages - or is_direct_message is True - ) + if ( + self._config.accept_invites_only_for_direct_messages + and is_direct_message is False + ): + return # Only accept invites from remote users if the configuration mandates it. is_from_local_user = self._api.is_mine(event.sender) - is_allowed_by_local_user_rules = ( - not self._config.accept_invites_only_from_local_users - or is_from_local_user is True - ) + if ( + self._config.accept_invites_only_from_local_users + and is_from_local_user is False + ): + return False # Check the user is activated. recipient = await self._api.get_userinfo_by_id(event.state_key) @@ -102,28 +105,23 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: if not self._config.accept_invites_for_suspended_users and recipient.suspended: return - if ( - is_invite_for_local_user - and is_allowed_by_direct_message_rules - and is_allowed_by_local_user_rules - ): - # Make the user join the room. We run this as a background process to circumvent a race condition - # that occurs when responding to invites over federation (see https://github.com/matrix-org/synapse-auto-accept-invite/issues/12) - run_as_background_process( - "retry_make_join", - self._retry_make_join, - event.state_key, - event.state_key, - event.room_id, - "join", - bg_start_span=False, - ) + # Make the user join the room. We run this as a background process to circumvent a race condition + # that occurs when responding to invites over federation (see https://github.com/matrix-org/synapse-auto-accept-invite/issues/12) + run_as_background_process( + "retry_make_join", + self._retry_make_join, + event.state_key, + event.state_key, + event.room_id, + "join", + bg_start_span=False, + ) - if is_direct_message: - # Mark this room as a direct message! - await self._mark_room_as_direct_message( - event.state_key, event.sender, event.room_id - ) + if is_direct_message: + # Mark this room as a direct message! + await self._mark_room_as_direct_message( + event.state_key, event.sender, event.room_id + ) async def _mark_room_as_direct_message( self, user_id: str, dm_user_id: str, room_id: str From a94b722f901cdd9d38e0d84780925314e5dc78be Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Fri, 10 Jan 2025 14:36:31 +0000 Subject: [PATCH 3/6] Add tests --- synapse/events/auto_accept_invites.py | 2 +- tests/events/test_auto_accept_invites.py | 151 ++++++++++++++++++++++- 2 files changed, 151 insertions(+), 2 deletions(-) diff --git a/synapse/events/auto_accept_invites.py b/synapse/events/auto_accept_invites.py index fe391f5e345..47f1a9f5fbe 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -88,7 +88,7 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: self._config.accept_invites_only_from_local_users and is_from_local_user is False ): - return False + return # Check the user is activated. recipient = await self._api.get_userinfo_by_id(event.state_key) diff --git a/tests/events/test_auto_accept_invites.py b/tests/events/test_auto_accept_invites.py index 7fb4d4fa902..3f4c46e2fe1 100644 --- a/tests/events/test_auto_accept_invites.py +++ b/tests/events/test_auto_accept_invites.py @@ -39,7 +39,7 @@ from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer -from synapse.types import StreamToken, create_requester +from synapse.types import StreamToken, create_requester, UserInfo, UserID from synapse.util import Clock from tests.handlers.test_sync import generate_sync_config @@ -566,6 +566,139 @@ def test_runs_on_only_one_worker(self) -> None: Mock, specified_module._api.register_third_party_rules_callbacks ).assert_called_once() + + async def test_ignore_invite_from_missing_user(self) -> None: + """Tests that receiving an invite for a missing user is ignored.""" + invite = MockEvent( + sender=self.remote_invitee, + state_key=self.invitee, + type="m.room.member", + content={"membership": "invite"}, + ) + self.get_userinfo_by_id.return_value = None + + # Stop mypy from complaining that we give on_new_event a MockEvent rather than an + # EventBase. + await self.module.on_new_event(event=invite) # type: ignore[arg-type] + self.mocked_update_membership.assert_not_called() + + async def test_ignore_invite_from_deactivated_user(self) -> None: + """Tests that receiving an invite for a deactivated user is ignored.""" + invite = MockEvent( + sender=self.remote_invitee, + state_key=self.invitee, + type="m.room.member", + content={"membership": "invite"}, + ) + self.get_userinfo_by_id.return_value = UserInfo( + user_id=UserID.from_string("@user:test"), + is_admin=False, + is_guest=False, + consent_server_notice_sent=None, + consent_ts=None, + consent_version=None, + appservice_id=None, + creation_ts=0, + user_type=None, + is_deactivated=True, + locked=False, + is_shadow_banned=False, + approved=True, + suspended=False, + ) + + # Stop mypy from complaining that we give on_new_event a MockEvent rather than an + # EventBase. + await self.module.on_new_event(event=invite) # type: ignore[arg-type] + self.mocked_update_membership.assert_not_called() + + async def test_ignore_invite_from_suspended_user(self) -> None: + """Tests that receiving an invite for a suspended user is ignored by default.""" + invite = MockEvent( + sender=self.remote_invitee, + state_key=self.invitee, + type="m.room.member", + content={"membership": "invite"}, + ) + self.get_userinfo_by_id.return_value = UserInfo( + user_id=UserID.from_string("@user:test"), + is_admin=False, + is_guest=False, + consent_server_notice_sent=None, + consent_ts=None, + consent_version=None, + appservice_id=None, + creation_ts=0, + user_type=None, + is_deactivated=False, + locked=False, + is_shadow_banned=False, + approved=True, + suspended=True, + ) + + # Stop mypy from complaining that we give on_new_event a MockEvent rather than an + # EventBase. + await self.module.on_new_event(event=invite) # type: ignore[arg-type] + self.mocked_update_membership.assert_not_called() + + async def test_accept_invite_for_suspended_user_if_enabled( + self, + ) -> None: + """Tests that, if the module is configured to accept invites for suspended users, invites + are still automatically accepted. + """ + main_module = create_module( + config_override={"accept_invites_for_suspended_users": True}, + ) + + get_userinfo_by_id: Mock = main_module._api.get_userinfo_by_id # type: ignore[assignment] + get_userinfo_by_id.return_value = UserInfo( + user_id=UserID.from_string("@user:test"), + is_admin=False, + is_guest=False, + consent_server_notice_sent=None, + consent_ts=None, + consent_version=None, + appservice_id=None, + creation_ts=0, + user_type=None, + is_deactivated=False, + locked=False, + is_shadow_banned=False, + approved=True, + suspended=True, + ) + + mocked_update_membership: Mock = main_module._api.update_room_membership # type: ignore[assignment] + join_event = MockEvent( + sender="someone", + state_key="someone", + type="m.room.member", + content={"membership": "join"}, + ) + mocked_update_membership.return_value = make_awaitable(join_event) + + invite = MockEvent( + sender=self.user_id, + state_key=self.invitee, + type="m.room.member", + content={"membership": "invite"}, + ) + + # Stop mypy from complaining that we give on_new_event a MockEvent rather than an + # EventBase. + await module.on_new_event(event=invite) # type: ignore[arg-type] + + await self.retry_assertions( + mocked_update_membership, + 1, + sender=invite.state_key, + target=invite.state_key, + room_id=invite.room_id, + new_membership="join", + ) + async def retry_assertions( self, mock: Mock, call_count: int, **kwargs: Any ) -> None: @@ -647,6 +780,22 @@ def create_module( module_api.is_mine.side_effect = lambda a: a.split(":")[1] == "test" module_api.worker_name = worker_name module_api.sleep.return_value = make_multiple_awaitable(None) + module_api.get_userinfo_by_id.return_value = UserInfo( + user_id=UserID.from_string("@user:test"), + is_admin=False, + is_guest=False, + consent_server_notice_sent=None, + consent_ts=None, + consent_version=None, + appservice_id=None, + creation_ts=0, + user_type=None, + is_deactivated=False, + locked=False, + is_shadow_banned=False, + approved=True, + suspended=False, + ) if config_override is None: config_override = {} From 1d7d14c43defa4ef5e54221ff58728bc29ec3f94 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 13 Jan 2025 10:34:25 +0000 Subject: [PATCH 4/6] Remove suspension config option. --- docs/usage/configuration/config_documentation.md | 2 -- synapse/config/auto_accept_invites.py | 4 ---- synapse/events/auto_accept_invites.py | 4 ++-- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 8d1a3414c55..74522a1e4e5 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4775,7 +4775,6 @@ are instead automatically joined to a room when receiving an invite. Set the `en enable auto-accepting invites. Defaults to false. This setting has the following sub-options: * `enabled`: Whether to run the auto-accept invites logic. Defaults to false. -* `accept_invites_for_suspended_users`: Whether to automatically accept invites for suspended users. Defaults to false. * `only_for_direct_messages`: Whether invites should be automatically accepted for all room types, or only for direct messages. Defaults to false. * `only_from_local_users`: Whether to only automatically accept invites from users on this homeserver. Defaults to false. @@ -4791,7 +4790,6 @@ Example configuration: ```yaml auto_accept_invites: enabled: true - accept_invites_for_suspended_users: false only_for_direct_messages: true only_from_local_users: true worker_to_run_on: "worker_1" diff --git a/synapse/config/auto_accept_invites.py b/synapse/config/auto_accept_invites.py index 02e9a98a9d7..d90e13a5107 100644 --- a/synapse/config/auto_accept_invites.py +++ b/synapse/config/auto_accept_invites.py @@ -40,8 +40,4 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "only_from_local_users", False ) - self.accept_invites_for_suspended_users = auto_accept_invites_config.get( - "accept_invites_for_suspended_users", False - ) - self.worker_to_run_on = auto_accept_invites_config.get("worker_to_run_on") diff --git a/synapse/events/auto_accept_invites.py b/synapse/events/auto_accept_invites.py index 47f1a9f5fbe..8d2a3fbd43a 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -101,8 +101,8 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: if recipient.is_deactivated: return - # Only accept invites for suspended remote if the configuration mandates it. - if not self._config.accept_invites_for_suspended_users and recipient.suspended: + # Never accept invites for suspended users. + if recipient.suspended: return # Make the user join the room. We run this as a background process to circumvent a race condition From fea8b289605f4fb19a7a34d5215f8de5a169580d Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 13 Jan 2025 10:34:29 +0000 Subject: [PATCH 5/6] Make tests work. --- tests/events/test_auto_accept_invites.py | 256 +++++++++++------------ 1 file changed, 122 insertions(+), 134 deletions(-) diff --git a/tests/events/test_auto_accept_invites.py b/tests/events/test_auto_accept_invites.py index 3f4c46e2fe1..18d45929112 100644 --- a/tests/events/test_auto_accept_invites.py +++ b/tests/events/test_auto_accept_invites.py @@ -39,7 +39,7 @@ from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer -from synapse.types import StreamToken, create_requester, UserInfo, UserID +from synapse.types import StreamToken, UserID, UserInfo, create_requester from synapse.util import Clock from tests.handlers.test_sync import generate_sync_config @@ -349,6 +349,127 @@ def test_accept_invite_local_user( join_updates, _ = sync_join(self, invited_user_id) self.assertEqual(len(join_updates), 0) + @override_config( + { + "auto_accept_invites": { + "enabled": True, + }, + } + ) + async def test_ignore_invite_from_missing_user(self) -> None: + """Tests that receiving an invite for a missing user is ignored.""" + inviting_user_id = self.register_user("inviter", "pass") + inviting_user_tok = self.login("inviter", "pass") + + # A local user who receives an invite + invited_user_id = "@fake:" + self.hs.config.server.server_name + + # Create a room and send an invite to the other user + room_id = self.helper.create_room_as( + inviting_user_id, + tok=inviting_user_tok, + ) + + self.helper.invite( + room_id, + inviting_user_id, + invited_user_id, + tok=inviting_user_tok, + ) + + join_updates, _ = sync_join(self, inviting_user_id) + # Assert that the last event in the room was not a member event for the target user. + self.assertEqual( + join_updates[0].timeline.events[-1].content["membership"], "invite" + ) + + @override_config( + { + "auto_accept_invites": { + "enabled": True, + }, + } + ) + async def test_ignore_invite_from_deactivated_user(self) -> None: + """Tests that receiving an invite for a deactivated user is ignored.""" + inviting_user_id = self.register_user("inviter", "pass", admin=True) + inviting_user_tok = self.login("inviter", "pass") + + # A local user who receives an invite + invited_user_id = self.register_user("invitee", "pass") + + # Create a room and send an invite to the other user + room_id = self.helper.create_room_as( + inviting_user_id, + tok=inviting_user_tok, + ) + + channel = self.make_request( + "PUT", + "/_synapse/admin/v2/users/%s" % invited_user_id, + {"deactivated": True}, + access_token=inviting_user_tok, + ) + + assert channel.code == 200 + + self.helper.invite( + room_id, + inviting_user_id, + invited_user_id, + tok=inviting_user_tok, + ) + + # Check that the invite receiving user has automatically joined the room when syncing + join_updates, b = sync_join(self, inviting_user_id) + # Assert that the last event in the room was not a member event for the target user. + self.assertEqual( + join_updates[0].timeline.events[-1].content["membership"], "invite" + ) + + @override_config( + { + "auto_accept_invites": { + "enabled": True, + }, + } + ) + async def test_ignore_invite_from_suspended_user(self) -> None: + """Tests that receiving an invite for a suspended user is ignored.""" + inviting_user_id = self.register_user("inviter", "pass", admin=True) + inviting_user_tok = self.login("inviter", "pass") + + # A local user who receives an invite + invited_user_id = self.register_user("invitee", "pass") + + # Create a room and send an invite to the other user + room_id = self.helper.create_room_as( + inviting_user_id, + tok=inviting_user_tok, + ) + + channel = self.make_request( + "PUT", + f"/_synapse/admin/v1/suspend/{invited_user_id}", + {"suspend": True}, + access_token=inviting_user_tok, + ) + + assert channel.code == 200 + + self.helper.invite( + room_id, + inviting_user_id, + invited_user_id, + tok=inviting_user_tok, + ) + + join_updates, b = sync_join(self, inviting_user_id) + # Assert that the last event in the room was not a member event for the target user. + self.assertEqual( + join_updates[0].timeline.events[-1].content["membership"], "invite" + ) + _request_key = 0 @@ -566,139 +687,6 @@ def test_runs_on_only_one_worker(self) -> None: Mock, specified_module._api.register_third_party_rules_callbacks ).assert_called_once() - - async def test_ignore_invite_from_missing_user(self) -> None: - """Tests that receiving an invite for a missing user is ignored.""" - invite = MockEvent( - sender=self.remote_invitee, - state_key=self.invitee, - type="m.room.member", - content={"membership": "invite"}, - ) - self.get_userinfo_by_id.return_value = None - - # Stop mypy from complaining that we give on_new_event a MockEvent rather than an - # EventBase. - await self.module.on_new_event(event=invite) # type: ignore[arg-type] - self.mocked_update_membership.assert_not_called() - - async def test_ignore_invite_from_deactivated_user(self) -> None: - """Tests that receiving an invite for a deactivated user is ignored.""" - invite = MockEvent( - sender=self.remote_invitee, - state_key=self.invitee, - type="m.room.member", - content={"membership": "invite"}, - ) - self.get_userinfo_by_id.return_value = UserInfo( - user_id=UserID.from_string("@user:test"), - is_admin=False, - is_guest=False, - consent_server_notice_sent=None, - consent_ts=None, - consent_version=None, - appservice_id=None, - creation_ts=0, - user_type=None, - is_deactivated=True, - locked=False, - is_shadow_banned=False, - approved=True, - suspended=False, - ) - - # Stop mypy from complaining that we give on_new_event a MockEvent rather than an - # EventBase. - await self.module.on_new_event(event=invite) # type: ignore[arg-type] - self.mocked_update_membership.assert_not_called() - - async def test_ignore_invite_from_suspended_user(self) -> None: - """Tests that receiving an invite for a suspended user is ignored by default.""" - invite = MockEvent( - sender=self.remote_invitee, - state_key=self.invitee, - type="m.room.member", - content={"membership": "invite"}, - ) - self.get_userinfo_by_id.return_value = UserInfo( - user_id=UserID.from_string("@user:test"), - is_admin=False, - is_guest=False, - consent_server_notice_sent=None, - consent_ts=None, - consent_version=None, - appservice_id=None, - creation_ts=0, - user_type=None, - is_deactivated=False, - locked=False, - is_shadow_banned=False, - approved=True, - suspended=True, - ) - - # Stop mypy from complaining that we give on_new_event a MockEvent rather than an - # EventBase. - await self.module.on_new_event(event=invite) # type: ignore[arg-type] - self.mocked_update_membership.assert_not_called() - - async def test_accept_invite_for_suspended_user_if_enabled( - self, - ) -> None: - """Tests that, if the module is configured to accept invites for suspended users, invites - are still automatically accepted. - """ - main_module = create_module( - config_override={"accept_invites_for_suspended_users": True}, - ) - - get_userinfo_by_id: Mock = main_module._api.get_userinfo_by_id # type: ignore[assignment] - get_userinfo_by_id.return_value = UserInfo( - user_id=UserID.from_string("@user:test"), - is_admin=False, - is_guest=False, - consent_server_notice_sent=None, - consent_ts=None, - consent_version=None, - appservice_id=None, - creation_ts=0, - user_type=None, - is_deactivated=False, - locked=False, - is_shadow_banned=False, - approved=True, - suspended=True, - ) - - mocked_update_membership: Mock = main_module._api.update_room_membership # type: ignore[assignment] - join_event = MockEvent( - sender="someone", - state_key="someone", - type="m.room.member", - content={"membership": "join"}, - ) - mocked_update_membership.return_value = make_awaitable(join_event) - - invite = MockEvent( - sender=self.user_id, - state_key=self.invitee, - type="m.room.member", - content={"membership": "invite"}, - ) - - # Stop mypy from complaining that we give on_new_event a MockEvent rather than an - # EventBase. - await module.on_new_event(event=invite) # type: ignore[arg-type] - - await self.retry_assertions( - mocked_update_membership, - 1, - sender=invite.state_key, - target=invite.state_key, - room_id=invite.room_id, - new_membership="join", - ) - async def retry_assertions( self, mock: Mock, call_count: int, **kwargs: Any ) -> None: From 190e8694a7c7fcb825801a09a1db290fd2810fda Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 13 Jan 2025 10:37:16 +0000 Subject: [PATCH 6/6] changelog --- changelog.d/18073.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18073.bugfix diff --git a/changelog.d/18073.bugfix b/changelog.d/18073.bugfix new file mode 100644 index 00000000000..eeb56a7a618 --- /dev/null +++ b/changelog.d/18073.bugfix @@ -0,0 +1 @@ +Deactivated users will no longer automatically accept an invite when `auto_accept_invites` is enabled. \ No newline at end of file