Sliding Sync: Include invite, ban, kick, targets when $LAZY-loading room members (#17947)

Part of https://github.com/element-hq/synapse/issues/17929

changelog.d/17947.feature

@@ -0,0 +1 @@
+Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members.

synapse/api/constants.py

@@ -231,6 +231,8 @@ class EventContentFields:
     ROOM_NAME: Final = "name"
 
     MEMBERSHIP: Final = "membership"
+    MEMBERSHIP_DISPLAYNAME: Final = "displayname"
+    MEMBERSHIP_AVATAR_URL: Final = "avatar_url"
 
     # Used in m.room.guest_access events.
     GUEST_ACCESS: Final = "guest_access"

synapse/handlers/sliding_sync/__init__.py

@@ -955,15 +955,21 @@ class SlidingSyncHandler:
                             and state_key == StateValues.LAZY
                         ):
                             lazy_load_room_members = True
+
                             # Everyone in the timeline is relevant
-                            #
-                            # FIXME: We probably also care about invite, ban, kick, targets, etc
-                            # but the spec only mentions "senders".
                             timeline_membership: Set[str] = set()
                             if timeline_events is not None:
                                 for timeline_event in timeline_events:
+                                    # Anyone who sent a message is relevant
                                     timeline_membership.add(timeline_event.sender)
 
+                                    # We also care about invite, ban, kick, targets,
+                                    # etc.
+                                    if timeline_event.type == EventTypes.Member:
+                                        timeline_membership.add(
+                                            timeline_event.state_key
+                                        )
+
                             # Update the required state filter so we pick up the new
                             # membership
                             for user_id in timeline_membership:

synapse/types/handlers/sliding_sync.py

@@ -407,8 +407,8 @@ class StateValues:
     # Include all state events of the given type
     WILDCARD: Final = "*"
     # Lazy-load room membership events (include room membership events for any event
-    # `sender` in the timeline). We only give special meaning to this value when it's a
+    # `sender` or membership change target in the timeline). We only give special
-    # `state_key`.
+    # meaning to this value when it's a `state_key`.
     LAZY: Final = "$LAZY"
     # Subsitute with the requester's user ID. Typically used by clients to get
     # the user's membership.
@@ -641,9 +641,10 @@ class RoomSyncConfig:
             if user_id == StateValues.ME:
                 continue
             # We're lazy-loading membership so we can just return the state we have.
-            # Lazy-loading means we include membership for any event `sender` in the
+            # Lazy-loading means we include membership for any event `sender` or
-            # timeline but since we had to auth those timeline events, we will have the
+            # membership change target in the timeline but since we had to auth those
-            # membership state for them (including from remote senders).
+            # timeline events, we will have the membership state for them (including
+            # from remote senders).
             elif user_id == StateValues.LAZY:
                 continue
             elif user_id == StateValues.WILDCARD:

tests/rest/client/sliding_sync/test_rooms_required_state.py

@@ -11,6 +11,7 @@
 # See the GNU Affero General Public License for more details:
 # <https://www.gnu.org/licenses/agpl-3.0.html>.
 #
+import enum
 import logging
 
 from parameterized import parameterized, parameterized_class
@@ -18,9 +19,9 @@ from parameterized import parameterized, parameterized_class
 from twisted.test.proto_helpers import MemoryReactor
 
 import synapse.rest.admin
-from synapse.api.constants import EventTypes, Membership
+from synapse.api.constants import EventContentFields, EventTypes, JoinRules, Membership
 from synapse.handlers.sliding_sync import StateValues
-from synapse.rest.client import login, room, sync
+from synapse.rest.client import knock, login, room, sync
 from synapse.server import HomeServer
 from synapse.util import Clock
 
@@ -30,6 +31,17 @@ from tests.test_utils.event_injection import mark_event_as_partial_state
 logger = logging.getLogger(__name__)
 
 
+# Inherit from `str` so that they show up in the test description when we
+# `@parameterized.expand(...)` the first parameter
+class MembershipAction(str, enum.Enum):
+    INVITE = "invite"
+    JOIN = "join"
+    KNOCK = "knock"
+    LEAVE = "leave"
+    BAN = "ban"
+    KICK = "kick"
+
+
 # FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the
 # foreground update for
 # `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by
@@ -52,6 +64,7 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
     servlets = [
         synapse.rest.admin.register_servlets,
         login.register_servlets,
+        knock.register_servlets,
         room.register_servlets,
         sync.register_servlets,
     ]
@@ -496,6 +509,153 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
         )
         self.assertIsNone(response_body["rooms"][room_id1].get("invite_state"))
 
+    @parameterized.expand(
+        [
+            (MembershipAction.LEAVE,),
+            (MembershipAction.INVITE,),
+            (MembershipAction.KNOCK,),
+            (MembershipAction.JOIN,),
+            (MembershipAction.BAN,),
+            (MembershipAction.KICK,),
+        ]
+    )
+    def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_members_incremental_sync(
+        self,
+        room_membership_action: str,
+    ) -> None:
+        """
+        On incremental sync, test `rooms.required_state` returns people relevant to the
+        timeline when lazy-loading room members, `["m.room.member","$LAZY"]` **including
+        changes to membership**.
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+        user2_id = self.register_user("user2", "pass")
+        user2_tok = self.login(user2_id, "pass")
+        user3_id = self.register_user("user3", "pass")
+        user3_tok = self.login(user3_id, "pass")
+        user4_id = self.register_user("user4", "pass")
+        user4_tok = self.login(user4_id, "pass")
+        user5_id = self.register_user("user5", "pass")
+        user5_tok = self.login(user5_id, "pass")
+
+        room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True)
+        # If we're testing knocks, set the room to knock
+        if room_membership_action == MembershipAction.KNOCK:
+            self.helper.send_state(
+                room_id1,
+                EventTypes.JoinRules,
+                {"join_rule": JoinRules.KNOCK},
+                tok=user2_tok,
+            )
+
+        # Join the test users to the room
+        self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok)
+        self.helper.join(room_id1, user1_id, tok=user1_tok)
+        self.helper.invite(room_id1, src=user2_id, targ=user3_id, tok=user2_tok)
+        self.helper.join(room_id1, user3_id, tok=user3_tok)
+        self.helper.invite(room_id1, src=user2_id, targ=user4_id, tok=user2_tok)
+        self.helper.join(room_id1, user4_id, tok=user4_tok)
+        if room_membership_action in (
+            MembershipAction.LEAVE,
+            MembershipAction.BAN,
+            MembershipAction.JOIN,
+        ):
+            self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok)
+            self.helper.join(room_id1, user5_id, tok=user5_tok)
+
+        # Send some messages to fill up the space
+        self.helper.send(room_id1, "1", tok=user2_tok)
+        self.helper.send(room_id1, "2", tok=user2_tok)
+        self.helper.send(room_id1, "3", tok=user2_tok)
+
+        # Make the Sliding Sync request with lazy loading for the room members
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 1]],
+                    "required_state": [
+                        [EventTypes.Create, ""],
+                        [EventTypes.Member, StateValues.LAZY],
+                    ],
+                    "timeline_limit": 3,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+
+        # Send more timeline events into the room
+        self.helper.send(room_id1, "4", tok=user2_tok)
+        self.helper.send(room_id1, "5", tok=user4_tok)
+        # The third event will be our membership event concerning user5
+        if room_membership_action == MembershipAction.LEAVE:
+            # User 5 leaves
+            self.helper.leave(room_id1, user5_id, tok=user5_tok)
+        elif room_membership_action == MembershipAction.INVITE:
+            # User 5 is invited
+            self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok)
+        elif room_membership_action == MembershipAction.KNOCK:
+            # User 5 knocks
+            self.helper.knock(room_id1, user5_id, tok=user5_tok)
+            # The admin of the room accepts the knock
+            self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok)
+        elif room_membership_action == MembershipAction.JOIN:
+            # Update the display name of user5 (causing a membership change)
+            self.helper.send_state(
+                room_id1,
+                event_type=EventTypes.Member,
+                state_key=user5_id,
+                body={
+                    EventContentFields.MEMBERSHIP: Membership.JOIN,
+                    EventContentFields.MEMBERSHIP_DISPLAYNAME: "quick changer",
+                },
+                tok=user5_tok,
+            )
+        elif room_membership_action == MembershipAction.BAN:
+            self.helper.ban(room_id1, src=user2_id, targ=user5_id, tok=user2_tok)
+        elif room_membership_action == MembershipAction.KICK:
+            # Kick user5 from the room
+            self.helper.change_membership(
+                room=room_id1,
+                src=user2_id,
+                targ=user5_id,
+                tok=user2_tok,
+                membership=Membership.LEAVE,
+                extra_data={
+                    "reason": "Bad manners",
+                },
+            )
+        else:
+            raise AssertionError(
+                f"Unknown room_membership_action: {room_membership_action}"
+            )
+
+        # Make an incremental Sliding Sync request
+        response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
+
+        state_map = self.get_success(
+            self.storage_controllers.state.get_current_state(room_id1)
+        )
+
+        # Only user2, user4, and user5 sent events in the last 3 events we see in the
+        # `timeline`.
+        self._assertRequiredStateIncludes(
+            response_body["rooms"][room_id1]["required_state"],
+            {
+                # This appears because *some* membership in the room changed and the
+                # heroes are recalculated and is thrown in because we have it. But this
+                # is technically optional and not needed because we've already seen user2
+                # in the last sync (and their membership hasn't changed).
+                state_map[(EventTypes.Member, user2_id)],
+                # Appears because there is a message in the timeline from this user
+                state_map[(EventTypes.Member, user4_id)],
+                # Appears because there is a membership event in the timeline from this user
+                state_map[(EventTypes.Member, user5_id)],
+            },
+            exact=True,
+        )
+        self.assertIsNone(response_body["rooms"][room_id1].get("invite_state"))
+
     def test_rooms_required_state_expand_lazy_loading_room_members_incremental_sync(
         self,
     ) -> None:
@@ -1243,7 +1403,7 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
 
         # Update the room name
         self.helper.send_state(
-            room_id1, "m.room.name", {"name": "Bar"}, state_key="", tok=user1_tok
+            room_id1, EventTypes.Name, {"name": "Bar"}, state_key="", tok=user1_tok
         )
 
         # Update the sliding sync requests to exclude the room name again