diff --git a/changelog.d/19958.bugfix b/changelog.d/19958.bugfix new file mode 100644 index 00000000000..f2376eabe48 --- /dev/null +++ b/changelog.d/19958.bugfix @@ -0,0 +1 @@ +Add a new `auto_accept_invites.enabled_for_accepted_knocks` option which, when enabled, automatically joins a user to a room when an invite arrives for a knock they have pending in that room (i.e. their knock was accepted), instead of requiring the user to accept the resulting invite by hand; the server designates one of the user's devices to download any MSC4268 room key bundle. Also fix the `on_new_event` module callback breaking event persistence when a rejected event was received over federation. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 92eca4a7ffd..c8bf7cf39ca 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4773,6 +4773,8 @@ This setting has the following sub-options: * `enabled` (boolean): Whether to run the auto-accept invites logic. Defaults to `false`. +* `enabled_for_accepted_knocks` (boolean): Whether to automatically join a user to a room when an invite arrives for a knock they have pending in that room (i.e. their knock was accepted). The user already asked to join by knocking, so this applies independently of `enabled` and of the restrictions below. Defaults to `false`. + * `only_for_direct_messages` (boolean): Whether invites should be automatically accepted for all room types, or only for direct messages. Defaults to `false`. * `only_from_local_users` (boolean): Whether to only automatically accept invites from users on this homeserver. Defaults to `false`. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index 0e345b7b69d..981cc36d9b2 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -5865,6 +5865,15 @@ properties: type: boolean description: Whether to run the auto-accept invites logic. default: false + enabled_for_accepted_knocks: + type: boolean + description: >- + Whether to automatically join a user to a room when an invite + arrives for a knock they have pending in that room (i.e. their + knock was accepted). The user already asked to join by knocking, + so this applies independently of `enabled` and of the restrictions + below. + default: false only_for_direct_messages: type: boolean description: >- diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 4814370684a..fcd97d8fbee 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -716,7 +716,10 @@ async def start(hs: "HomeServer", *, freeze: bool = True) -> None: m = module(config, module_api) logger.info("Loaded module %s", m) - if hs.config.auto_accept_invites.enabled: + if ( + hs.config.auto_accept_invites.enabled + or hs.config.auto_accept_invites.enabled_for_accepted_knocks + ): # Start the local auto_accept_invites module. m = InviteAutoAccepter(hs.config.auto_accept_invites, module_api) logger.info("Loaded local module %s", m) diff --git a/synapse/config/auto_accept_invites.py b/synapse/config/auto_accept_invites.py index d90e13a5107..ddaf0b49b71 100644 --- a/synapse/config/auto_accept_invites.py +++ b/synapse/config/auto_accept_invites.py @@ -32,6 +32,16 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.enabled = auto_accept_invites_config.get("enabled", False) + # Whether to automatically join a room on behalf of a user when an + # invite arrives for a knock the user has pending in that room (i.e. + # the knock was accepted). The user already asked to join by knocking, + # so this is independent of `enabled` above and of the DM/local-user + # restrictions below. + # See https://github.com/element-hq/synapse/issues/16307. + self.enabled_for_accepted_knocks = auto_accept_invites_config.get( + "enabled_for_accepted_knocks", False + ) + self.accept_invites_only_for_direct_messages = auto_accept_invites_config.get( "only_for_direct_messages", False ) diff --git a/synapse/events/auto_accept_invites.py b/synapse/events/auto_accept_invites.py index 4c59f0dffeb..2fcaa306d50 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -26,9 +26,14 @@ from synapse.api.errors import SynapseError from synapse.config.auto_accept_invites import AutoAcceptInvitesConfig from synapse.module_api import EventBase, ModuleApi, run_as_background_process +from synapse.types import create_requester logger = logging.getLogger(__name__) +# MSC4509: to-device message designating one of a user's devices as the eager +# downloader of an MSC4268 room key bundle after a server-initiated join. +UNSTABLE_KEY_BUNDLE_CLAIM_TYPE = "org.matrix.msc4509.key_bundle_claim" + class InviteAutoAccepter: def __init__(self, config: AutoAcceptInvitesConfig, api: ModuleApi): @@ -36,8 +41,12 @@ def __init__(self, config: AutoAcceptInvitesConfig, api: ModuleApi): self._api = api self.server_name = api.server_name self._config = config + # This is bundled with Synapse (rather than a true module), so reach + # for the datastore directly: the module API doesn't expose event + # fetching. + self._store = api._store - if not self._config.enabled: + if not self._config.enabled and not self._config.enabled_for_accepted_knocks: return should_run_on_this_worker = config.worker_to_run_on == self._api.worker_name @@ -75,21 +84,34 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: ): return - # Only accept invites for direct messages if the configuration mandates it. - is_direct_message = event.content.get("is_direct", False) - if ( - self._config.accept_invites_only_for_direct_messages - and is_direct_message is False - ): + # An invite following a knock from the user (the knock being accepted) + # is auto-joined regardless of the general auto-accept settings: the + # user already asked to join the room by knocking. + # See https://github.com/element-hq/synapse/issues/16307. + is_accepted_knock = await self._is_accepted_knock(event) + if is_accepted_knock: + if not self._config.enabled_for_accepted_knocks: + return + elif not self._config.enabled: return - # Only accept invites from remote users if the configuration mandates it. - is_from_local_user = self._api.is_mine(event.sender) - if ( - self._config.accept_invites_only_from_local_users - and is_from_local_user is False - ): - return + is_direct_message = event.content.get("is_direct", False) + + if not is_accepted_knock: + # Only accept invites for direct messages if the configuration mandates it. + 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) + if ( + self._config.accept_invites_only_from_local_users + and is_from_local_user is False + ): + return # Check the user is activated. recipient = await self._api.get_userinfo_by_id(event.state_key) @@ -127,6 +149,26 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None: event.state_key, event.sender, event.room_id ) + async def _is_accepted_knock(self, invite_event: EventBase) -> bool: + """Whether the invite is the acceptance of a knock by the invited + user: i.e. the invited user's own knock membership event is among the + invite's auth events. + + This works for over-federation invites too: the knocking server holds + the knock event (it created it), and the resident server necessarily + cited it as the invitee's prior membership when authing the invite. + """ + for auth_event_id in invite_event.auth_event_ids(): + auth_event = await self._store.get_event(auth_event_id, allow_none=True) + if ( + auth_event is not None + and auth_event.type == EventTypes.Member + and auth_event.state_key == invite_event.state_key + and auth_event.membership == Membership.KNOCK + ): + return True + return False + async def _mark_room_as_direct_message( self, user_id: str, dm_user_id: str, room_id: str ) -> None: @@ -214,3 +256,54 @@ async def _retry_make_join( if join_event is not None: break + + if join_event is not None: + # The join was made by the server, so it lands on all of the + # user's devices in the same sync instant. Designate one device to + # eagerly download any MSC4268 room key bundle for the room, lest + # every device does (MSC4509). Best effort: the hint is advisory, + # and without it clients fall back to claiming the bundle lazily. + try: + await self._send_key_bundle_claim_hint(target, room_id) + except Exception as e: + logger.warning( + "Failed to send key bundle claim hint to %s for %s: %s", + target, + room_id, + e, + ) + + async def _send_key_bundle_claim_hint(self, user_id: str, room_id: str) -> None: + """Send an `org.matrix.msc4509.key_bundle_claim` to-device message to + the user's most recently active device, designating it as the one + device which should eagerly download an MSC4268 room key bundle for + the room the user was just joined to. + + Args: + user_id: the (local) user who was joined to the room + room_id: the room they were joined to + """ + # This is bundled with Synapse (rather than a true module), so reach + # into the homeserver for the device machinery: the module API doesn't + # expose device listing or to-device sending. + hs = self._api._hs + devices = await hs.get_device_handler().get_devices_by_user(user_id) + if not devices: + return + + # Pick the device the user is most likely to be actively using. The + # worst a stale pick costs is the bundle being claimed lazily instead. + device = max(devices, key=lambda d: d.get("last_seen_ts") or 0) + device_id = device["device_id"] + + logger.info( + "Designating device %s of %s to claim any key bundle for %s", + device_id, + user_id, + room_id, + ) + await hs.get_device_message_handler().send_device_message( + create_requester(user_id), + UNSTABLE_KEY_BUNDLE_CLAIM_TYPE, + {user_id: {device_id: {"room_id": room_id}}}, + ) diff --git a/synapse/module_api/callbacks/third_party_event_rules_callbacks.py b/synapse/module_api/callbacks/third_party_event_rules_callbacks.py index 68053d65a31..ec6158d8859 100644 --- a/synapse/module_api/callbacks/third_party_event_rules_callbacks.py +++ b/synapse/module_api/callbacks/third_party_event_rules_callbacks.py @@ -416,7 +416,13 @@ async def on_new_event(self, event_id: str) -> None: if len(self._on_new_event_callbacks) == 0: return - event = await self.store.get_event(event_id) + # `allow_rejected` so that a rejected event doesn't raise NotFoundError + # here, which would propagate out of the notifier and break the caller + # (e.g. persisting further federation events). Rejected events are not + # shown to modules. + event = await self.store.get_event(event_id, allow_rejected=True) + if event.rejected_reason is not None: + return # We *don't* want to wait for the full state here, because waiting for full # state will persist event, which in turn will call this method. diff --git a/tests/events/test_auto_accept_invites.py b/tests/events/test_auto_accept_invites.py index 632d1dc4f64..31f0ad4cfc6 100644 --- a/tests/events/test_auto_accept_invites.py +++ b/tests/events/test_auto_accept_invites.py @@ -32,11 +32,14 @@ from synapse.api.errors import SynapseError from synapse.config._base import RootConfig from synapse.config.auto_accept_invites import AutoAcceptInvitesConfig -from synapse.events.auto_accept_invites import InviteAutoAccepter +from synapse.events.auto_accept_invites import ( + UNSTABLE_KEY_BUNDLE_CLAIM_TYPE, + InviteAutoAccepter, +) from synapse.handlers.sync import JoinedSyncResult, SyncRequestKey from synapse.module_api import ModuleApi from synapse.rest import admin -from synapse.rest.client import login, room +from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.types import StreamToken, UserID, UserInfo, create_requester from synapse.util.clock import Clock @@ -58,10 +61,142 @@ class AutoAcceptInvitesTestCase(FederatingHomeserverTestCase): servlets = [ admin.register_servlets, + knock.register_servlets, login.register_servlets, room.register_servlets, ] + def _create_knockable_room_with_pending_knock( + self, + ) -> tuple[str, str, str, str]: + """Create a knockable room and have a second local user knock on it. + + Returns a tuple of (room_id, creator_id, creator_tok, knocker_id). + """ + creator_id = self.register_user("creator", "pass") + creator_tok = self.login("creator", "pass") + + knocker_id = self.register_user("knocker", "pass") + knocker_tok = self.login("knocker", "pass") + + room_id = self.helper.create_room_as( + creator_id, + is_public=False, + tok=creator_tok, + ) + self.helper.send_state( + room_id, + EventTypes.JoinRules, + {"join_rule": "knock"}, + tok=creator_tok, + ) + + self.helper.knock(room=room_id, user=knocker_id, tok=knocker_tok) + + return room_id, creator_id, creator_tok, knocker_id + + @override_config( + { + "auto_accept_invites": { + "enabled_for_accepted_knocks": True, + }, + } + ) + def test_auto_join_on_accepted_knock(self) -> None: + """With `enabled_for_accepted_knocks` on, a user whose knock is + accepted (invited by a room member) is automatically joined to the + room.""" + ( + room_id, + creator_id, + creator_tok, + knocker_id, + ) = self._create_knockable_room_with_pending_knock() + + # The creator accepts the knock by inviting the knocker. + self.helper.invite( + room_id, + creator_id, + knocker_id, + tok=creator_tok, + ) + + # The knocker is automatically joined to the room. + join_updates, _ = sync_join(self, knocker_id) + self.assertEqual(len(join_updates), 1) + self.assertEqual(join_updates[0].room_id, room_id) + + # The join was server-initiated, so the knocker's device should have + # been designated (via a to-device message) to eagerly claim any + # MSC4268 room key bundle for the room. + devices = self.get_success( + self.hs.get_datastores().main.get_devices_by_user(knocker_id) + ) + self.assertEqual(len(devices), 1, devices) + device_id = next(iter(devices)) + to_stream_id = self.hs.get_datastores().main.get_to_device_stream_token() + messages, _ = self.get_success( + self.hs.get_datastores().main.get_messages_for_device( + knocker_id, device_id, 0, to_stream_id + ) + ) + claim_messages = [ + m for m in messages if m["type"] == UNSTABLE_KEY_BUNDLE_CLAIM_TYPE + ] + self.assertEqual(len(claim_messages), 1, messages) + self.assertEqual(claim_messages[0]["content"], {"room_id": room_id}) + self.assertEqual(claim_messages[0]["sender"], knocker_id) + + def test_no_auto_join_on_accepted_knock_by_default(self) -> None: + """With `enabled_for_accepted_knocks` off (the default), an accepted + knock stays an ordinary invite.""" + ( + room_id, + creator_id, + creator_tok, + knocker_id, + ) = self._create_knockable_room_with_pending_knock() + + self.helper.invite( + room_id, + creator_id, + knocker_id, + tok=creator_tok, + ) + + join_updates, _ = sync_join(self, knocker_id) + self.assertEqual(len(join_updates), 0) + + @override_config( + { + "auto_accept_invites": { + "enabled_for_accepted_knocks": True, + }, + } + ) + def test_plain_invite_not_auto_accepted(self) -> None: + """A plain invite (no prior knock) is not auto-accepted just because + the accepted-knocks logic is enabled.""" + inviting_user_id = self.register_user("inviter2", "pass") + inviting_user_tok = self.login("inviter2", "pass") + + invited_user_id = self.register_user("invitee2", "pass") + self.login("invitee2", "pass") + + room_id = self.helper.create_room_as( + inviting_user_id, is_public=False, tok=inviting_user_tok + ) + + self.helper.invite( + room_id, + inviting_user_id, + invited_user_id, + tok=inviting_user_tok, + ) + + join_updates, _ = sync_join(self, invited_user_id) + self.assertEqual(len(join_updates), 0) + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: hs = self.setup_test_homeserver() self.handler = hs.get_federation_handler() @@ -561,7 +696,12 @@ class InviteAutoAccepterInternalTestCase(TestCase): """ def setUp(self) -> None: - self.module = create_module() + # These tests exercise the plain-invite acceptance path, which is + # gated on `enabled` in `on_new_event` (the accepted-knock path is + # what's on by default). + self.module = create_module( + config_override={"auto_accept_invites": {"enabled": True}} + ) self.user_id = "@peter:test" self.invitee = "@lesley:test" self.remote_invitee = "@thomas:remote" @@ -772,6 +912,11 @@ def is_state(self) -> bool: """Checks if the event is a state event by checking if it has a state key.""" return self.state_key is not None + def auth_event_ids(self) -> list[str]: + """The module walks the auth events when checking whether an invite + accepts a knock; a mocked event has none.""" + return [] + @property def membership(self) -> str: """Extracts the membership from the event. Should only be called on an event @@ -800,6 +945,12 @@ def create_module( module_api = Mock(spec=ModuleApi) module_api.is_mine.side_effect = lambda a: a.split(":")[1] == "test" module_api.worker_name = worker_name + # The module reaches into the datastore to walk an invite's auth events; + # none of the mocked events have any. + module_api._store = Mock() + module_api._store.get_event.side_effect = lambda *_args, **_kwargs: ( + make_awaitable(None) + ) module_api.sleep.return_value = lambda *_args, **_kwargs: make_awaitable(None) module_api.get_userinfo_by_id.return_value = UserInfo( user_id=UserID.from_string("@user:test"), diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 0c7edbaa2da..9ae27ac3ebd 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -159,6 +159,62 @@ def test_rejected_message_event_state(self) -> None: self.assertEqual(sg, sg2) + def test_rejected_event_with_on_new_event_callback(self) -> None: + """ + A rejected event must not break the notifier when a module has + registered an `on_new_event` callback (which the notifier invokes for + each new event). + + Regression test: the callback dispatcher used to look the event up + without `allow_rejected=True`, so a rejected event raised + `NotFoundError` inside the federation persist path. + """ + OTHER_SERVER = "otherserver" + OTHER_USER = "@otheruser:" + OTHER_SERVER + + # create the room + user_id = self.register_user("kermit", "test") + tok = self.login("kermit", "test") + room_id = self.helper.create_room_as(room_creator=user_id, tok=tok) + room_version = self.get_success(self.store.get_room_version(room_id)) + + # pretend that another server has joined + join_event = self._build_and_send_join_event(OTHER_SERVER, OTHER_USER, room_id) + + # register an `on_new_event` callback (after the events above, so that + # they don't count towards its calls) + on_new_event = AsyncMock(return_value=None) + self.hs.get_module_api_callbacks().third_party_event_rules._on_new_event_callbacks.append( + on_new_event + ) + + # build and send an event which will be rejected + ev = make_test_pdu_event( + { + "type": EventTypes.Message, + "content": {}, + "room_id": room_id, + "sender": "@yetanotheruser:" + OTHER_SERVER, + "depth": join_event.depth + 1, + "prev_events": [join_event.event_id], + "auth_events": [], + "origin_server_ts": self.clock.time_msec(), + }, + room_version, + ) + + # this should succeed, despite the on_new_event callback + self.get_success( + self.hs.get_federation_event_handler().on_receive_pdu(OTHER_SERVER, ev) + ) + + # that should have been rejected... + e = self.get_success(self.store.get_event(ev.event_id, allow_rejected=True)) + self.assertIsNotNone(e.rejected_reason) + + # ... and the module should not have been told about it + on_new_event.assert_not_called() + def test_rejected_state_event_state(self) -> None: """ Check that we store the state group correctly for rejected state events. diff --git a/tests/server.py b/tests/server.py index 15a7661c345..e65f3044f3a 100644 --- a/tests/server.py +++ b/tests/server.py @@ -1465,7 +1465,10 @@ def thread_pool() -> threadpool.ThreadPool: for module, module_config in hs.config.modules.loaded_modules: module(config=module_config, api=module_api) - if hs.config.auto_accept_invites.enabled: + if ( + hs.config.auto_accept_invites.enabled + or hs.config.auto_accept_invites.enabled_for_accepted_knocks + ): # Start the local auto_accept_invites module. m = InviteAutoAccepter(hs.config.auto_accept_invites, module_api) logger.info("Loaded local module %s", m)