Merge branches 'matthew/msc4233-knock-via' and 'matthew/auto-join-on-knock-accept' into matthew/knock-push-rules

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.

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.

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`.

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: >-

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)

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
         )

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:

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

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"),

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

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)