diff --git a/changelog.d/16307.bugfix b/changelog.d/16307.bugfix new file mode 100644 index 0000000000..17abfd4c76 --- /dev/null +++ b/changelog.d/16307.bugfix @@ -0,0 +1 @@ +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), instead of requiring the user to accept the resulting invite by hand. Can be disabled with the new `auto_accept_invites.enabled_for_accepted_knocks` option. diff --git a/changelog.d/18031.bugfix b/changelog.d/18031.bugfix new file mode 100644 index 0000000000..045f9135a4 --- /dev/null +++ b/changelog.d/18031.bugfix @@ -0,0 +1 @@ +Reflect out-of-band membership events (invite rejections, knock rescissions and denials) in `state_after` (MSC4222) on `/sync`, so that clients which no longer apply timeline events to room state learn of the membership change. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 92eca4a7ff..8fedda5ec0 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 `true`. + * `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 0e345b7b69..4012278523 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: true only_for_direct_messages: type: boolean description: >- diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 4814370684..fcd97d8fbe 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 d90e13a510..49135b5ae3 100644 --- a/synapse/config/auto_accept_invites.py +++ b/synapse/config/auto_accept_invites.py @@ -32,6 +32,15 @@ class AutoAcceptInvitesConfig(Config): 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 on by default and independent of `enabled` above. + # See https://github.com/element-hq/synapse/issues/16307. + self.enabled_for_accepted_knocks = auto_accept_invites_config.get( + "enabled_for_accepted_knocks", True + ) + 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 4c59f0dffe..5eae169133 100644 --- a/synapse/events/auto_accept_invites.py +++ b/synapse/events/auto_accept_invites.py @@ -36,8 +36,12 @@ class InviteAutoAccepter: 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 +79,34 @@ class InviteAutoAccepter: ): 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 +144,26 @@ class InviteAutoAccepter: 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: diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 9ecfe0da0f..bd3869eef8 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -2870,6 +2870,26 @@ class SyncHandler: # An out of band room won't have any state changes. state = {} + # ...however, clients using `state_after` (MSC4222) no longer + # apply timeline events to room state, so we must reflect the + # out-of-band membership event itself in `state_after`: + # otherwise the client never learns of the membership change + # (e.g. an invite rejection, or a knock rescission or denial). + if sync_config.use_state_after: + for timeline_event in batch.events: + if ( + timeline_event.event.type == EventTypes.Member + and timeline_event.event.state_key + == sync_config.user.to_string() + and timeline_event.event.internal_metadata.is_out_of_band_membership() + ): + state[ + ( + timeline_event.event.type, + timeline_event.event.state_key, + ) + ] = timeline_event.event + summary: JsonDict | None = {} # we include a summary in room responses when we're lazy loading diff --git a/tests/events/test_auto_accept_invites.py b/tests/events/test_auto_accept_invites.py index 632d1dc4f6..69b5367493 100644 --- a/tests/events/test_auto_accept_invites.py +++ b/tests/events/test_auto_accept_invites.py @@ -36,7 +36,7 @@ from synapse.events.auto_accept_invites import 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 +58,114 @@ 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 + + def test_auto_join_on_accepted_knock(self) -> None: + """A user whose knock is accepted (invited by a room member) is + automatically joined to the room, even with no `auto_accept_invites` + configuration at all.""" + ( + 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) + + @override_config( + { + "auto_accept_invites": { + "enabled_for_accepted_knocks": False, + }, + } + ) + def test_no_auto_join_on_accepted_knock_when_disabled(self) -> None: + """With `enabled_for_accepted_knocks` off, 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) + + def test_plain_invite_not_auto_accepted_by_default(self) -> None: + """A plain invite (no prior knock) is not auto-accepted just because + the accepted-knocks logic is enabled by default.""" + 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 +665,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 +881,11 @@ class MockEvent: """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 +914,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/federation/test_federation_knock_msc4233.py b/tests/federation/test_federation_knock_msc4233.py index b90767f751..ef449803d8 100644 --- a/tests/federation/test_federation_knock_msc4233.py +++ b/tests/federation/test_federation_knock_msc4233.py @@ -28,14 +28,13 @@ from twisted.internet.testing import MemoryReactor from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.api.room_versions import RoomVersion, RoomVersions -from synapse.events import EventBase -from synapse.events import builder +from synapse.events import EventBase, builder from synapse.events.snapshot import EventContext from synapse.events.utils import strip_event from synapse.http.matrixfederationclient import ByteParser from synapse.http.types import QueryParams from synapse.rest import admin -from synapse.rest.client import knock, login, room +from synapse.rest.client import knock, login, room, sync from synapse.server import HomeServer from synapse.types import JsonDict from synapse.util.clock import Clock @@ -75,11 +74,18 @@ class KnockViaServerTestCase(unittest.FederatingHomeserverTestCase): knock.register_servlets, room.register_servlets, login.register_servlets, + sync.register_servlets, ] def default_config(self) -> JsonDict: conf = super().default_config() - conf["experimental_features"] = {"msc4233_enabled": True} + conf["experimental_features"] = { + "msc4233_enabled": True, + # For the state_after test: out-of-band leaves must be reflected + # in `state_after` for clients that no longer apply timeline + # events to state. + "msc4222_enabled": True, + } return conf def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: @@ -417,6 +423,60 @@ class KnockViaServerTestCase(unittest.FederatingHomeserverTestCase): break time.sleep(0.1) + def test_denied_knock_in_state_after(self) -> None: + """The out-of-band leave retracting the knock is reflected in + `state_after` (MSC4222) on sync: it is an outlier, so it never enters + the current state delta stream, and clients using `state_after` do + not apply timeline events to state.""" + knock_result = self._knock_on_remote_room() + + # Sync up to just after the knock. + channel = self.make_request( + "GET", + "/_matrix/client/v3/sync?timeout=0&org.matrix.msc4222.use_state_after=true", + access_token=knock_result.local_user1_tok, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + since_token = channel.json_body["next_batch"] + + self._send_denial_over_federation( + knock_result, + auth_events=[ + knock_result.room_create_event.event_id, + knock_result.knock_event_id, + ], + ) + + with test_timeout(3, "Denial of the knock was not processed"): + while True: + membership, _ = self.get_success( + self.store.get_local_current_membership_for_user_in_room( + knock_result.local_user1_id, knock_result.remote_room_id + ) + ) + if membership == Membership.LEAVE: + break + time.sleep(0.1) + + channel = self.make_request( + "GET", + f"/_matrix/client/v3/sync?timeout=0&org.matrix.msc4222.use_state_after=true&since={since_token}", + access_token=knock_result.local_user1_tok, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + leave_room = channel.json_body["rooms"]["leave"][knock_result.remote_room_id] + state_after_events = leave_room["org.matrix.msc4222.state_after"]["events"] + self.assertTrue( + any( + e["type"] == EventTypes.Member + and e["state_key"] == knock_result.local_user1_id + and e["content"]["membership"] == Membership.LEAVE + for e in state_after_events + ), + leave_room, + ) + def test_denied_knock_ignored_without_knock_in_auth_events(self) -> None: """A leave event for our knocked user which does not reference the knock in its auth events is ignored.""" @@ -495,7 +555,7 @@ class DenyKnockFederationSendTestCase(unittest.FederatingHomeserverTestCase): def test_deny_sends_leave_to_knocking_server(self) -> None: """Kicking a remote user whose membership is knock sends the leave event to their (otherwise uninvolved) server.""" - user_id = self.register_user("u1", "pass") + self.register_user("u1", "pass") user_token = self.login("u1", "pass") fake_knocking_user_id = f"@user:{self.OTHER_SERVER_NAME}" @@ -585,9 +645,7 @@ class DenyKnockFederationSendTestCase(unittest.FederatingHomeserverTestCase): while True: leave_pdus = [ pdu - for pdu in sent_pdus_by_destination.get( - self.OTHER_SERVER_NAME, [] - ) + for pdu in sent_pdus_by_destination.get(self.OTHER_SERVER_NAME, []) if pdu.get("type") == EventTypes.Member and pdu.get("state_key") == fake_knocking_user_id and pdu.get("content", {}).get("membership") == Membership.LEAVE diff --git a/tests/server.py b/tests/server.py index 15a7661c34..e65f3044f3 100644 --- a/tests/server.py +++ b/tests/server.py @@ -1465,7 +1465,10 @@ def start_test_homeserver( 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)