From e5d3bfba30351f4f9c2bcf89a2b002c6be9ee099 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 2 Dec 2024 10:17:55 -0600 Subject: [PATCH] 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 | 1 + synapse/api/constants.py | 2 + synapse/handlers/sliding_sync/__init__.py | 12 +- synapse/types/handlers/sliding_sync.py | 11 +- .../sliding_sync/test_rooms_required_state.py | 166 +++++++++++++++++- 5 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 changelog.d/17947.feature diff --git a/changelog.d/17947.feature b/changelog.d/17947.feature new file mode 100644 index 0000000000..2d1b99cec2 --- /dev/null +++ b/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. diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 8db302b3d8..1206d1e00f 100644 --- a/synapse/api/constants.py +++ b/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" diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 85cfbc6dbf..4f4faef524 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/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: diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index aae60fddea..3ebd334a6d 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/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 - # `state_key`. + # `sender` or membership change target in the timeline). We only give special + # 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 - # timeline but since we had to auth those timeline events, we will have the - # membership state for them (including from remote senders). + # Lazy-loading means we include membership for any event `sender` or + # membership change target in the timeline but since we had to auth those + # 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: diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index ecea5f2d5b..b4869d5fa3 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -11,6 +11,7 @@ # See the GNU Affero General Public License for more details: # . # +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