From d27023b415c5a0da32573c08b1afd6943488e153 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 18 Apr 2024 14:58:57 +0100 Subject: [PATCH 1/8] Remove tautologous test, and fix up doc-comments We've just checked `membership=="leave"` here, so clearly it's not join. --- synapse/visibility.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index d1d478129fd..4be4ac81e72 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -396,9 +396,13 @@ def _check_client_allowed_to_see_event( @attr.s(frozen=True, slots=True, auto_attribs=True) class _CheckMembershipReturn: - "Return value of _check_membership" + """Return value of _check_membership""" + allowed: bool + """Whether the user should be allowed to see the event""" + joined: bool + """Whether the user was joined at the event""" def _check_membership( @@ -409,10 +413,6 @@ def _check_membership( is_peeking: bool, ) -> _CheckMembershipReturn: """Check whether the user can see the event due to their membership - - Returns: - True if they can, False if they can't, plus the membership of the user - at the event. """ # If the event is the user's own membership event, use the 'most joined' # membership @@ -435,7 +435,7 @@ def _check_membership( if membership == "leave" and ( prev_membership == "join" or prev_membership == "invite" ): - return _CheckMembershipReturn(True, membership == Membership.JOIN) + return _CheckMembershipReturn(True, False) new_priority = MEMBERSHIP_PRIORITY.index(membership) old_priority = MEMBERSHIP_PRIORITY.index(prev_membership) From 33c3bb739410551ac69a33029b96115a4a1debcd Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 18 Apr 2024 15:53:31 +0100 Subject: [PATCH 2/8] test_visibility: use shared injection methods --- tests/test_visibility.py | 174 ++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 84 deletions(-) diff --git a/tests/test_visibility.py b/tests/test_visibility.py index e51f72d65fb..429773c6af9 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -24,10 +24,11 @@ from synapse.api.room_versions import RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.events.snapshot import EventContext -from synapse.types import JsonDict, create_requester +from synapse.server import HomeServer +from synapse.types import create_requester from synapse.visibility import filter_events_for_client, filter_events_for_server - from tests import unittest +from tests.test_utils.event_injection import inject_event, inject_member_event from tests.utils import create_room logger = logging.getLogger(__name__) @@ -56,15 +57,31 @@ def test_filtering(self) -> None: # # before we do that, we persist some other events to act as state. - self._inject_visibility("@admin:hs", "joined") + self.get_success( + inject_visibility_event(self.hs, TEST_ROOM_ID, "@admin:hs", "joined") + ) for i in range(10): - self._inject_room_member("@resident%i:hs" % i) + self.get_success( + inject_member_event( + self.hs, + TEST_ROOM_ID, + "@resident%i:hs" % i, + "join", + ) + ) events_to_filter = [] for i in range(10): - user = "@user%i:%s" % (i, "test_server" if i == 5 else "other_server") - evt = self._inject_room_member(user, extra_content={"a": "b"}) + evt = self.get_success( + inject_member_event( + self.hs, + TEST_ROOM_ID, + "@user%i:%s" % (i, "test_server" if i == 5 else "other_server"), + "join", + extra_content={"a": "b"}, + ) + ) events_to_filter.append(evt) filtered = self.get_success( @@ -90,8 +107,19 @@ def test_filtering(self) -> None: def test_filter_outlier(self) -> None: # outlier events must be returned, for the good of the collective federation - self._inject_room_member("@resident:remote_hs") - self._inject_visibility("@resident:remote_hs", "joined") + self.get_success( + inject_member_event( + self.hs, + TEST_ROOM_ID, + "@resident:remote_hs", + "join", + ) + ) + self.get_success( + inject_visibility_event( + self.hs, TEST_ROOM_ID, "@resident:remote_hs", "joined" + ) + ) outlier = self._inject_outlier() self.assertEqual( @@ -110,7 +138,9 @@ def test_filter_outlier(self) -> None: ) # it should also work when there are other events in the list - evt = self._inject_message("@unerased:local_hs") + evt = self.get_success( + inject_message_event(self.hs, TEST_ROOM_ID, "@unerased:local_hs") + ) filtered = self.get_success( filter_events_for_server( @@ -150,19 +180,34 @@ def test_erased_user(self) -> None: # change in the middle of them. events_to_filter = [] - evt = self._inject_message("@unerased:local_hs") + evt = self.get_success( + inject_message_event(self.hs, TEST_ROOM_ID, "@unerased:local_hs") + ) events_to_filter.append(evt) - evt = self._inject_message("@erased:local_hs") + evt = self.get_success( + inject_message_event(self.hs, TEST_ROOM_ID, "@erased:local_hs") + ) events_to_filter.append(evt) - evt = self._inject_room_member("@joiner:remote_hs") + evt = self.get_success( + inject_member_event( + self.hs, + TEST_ROOM_ID, + "@joiner:remote_hs", + "join", + ) + ) events_to_filter.append(evt) - evt = self._inject_message("@unerased:local_hs") + evt = self.get_success( + inject_message_event(self.hs, TEST_ROOM_ID, "@unerased:local_hs") + ) events_to_filter.append(evt) - evt = self._inject_message("@erased:local_hs") + evt = self.get_success( + inject_message_event(self.hs, TEST_ROOM_ID, "@erased:local_hs") + ) events_to_filter.append(evt) # the erasey user gets erased @@ -200,76 +245,6 @@ def test_erased_user(self) -> None: for i in (1, 4): self.assertNotIn("body", filtered[i].content) - def _inject_visibility(self, user_id: str, visibility: str) -> EventBase: - content = {"history_visibility": visibility} - builder = self.event_builder_factory.for_room_version( - RoomVersions.V1, - { - "type": "m.room.history_visibility", - "sender": user_id, - "state_key": "", - "room_id": TEST_ROOM_ID, - "content": content, - }, - ) - - event, unpersisted_context = self.get_success( - self.event_creation_handler.create_new_client_event(builder) - ) - context = self.get_success(unpersisted_context.persist(event)) - self.get_success(self._persistence.persist_event(event, context)) - return event - - def _inject_room_member( - self, - user_id: str, - membership: str = "join", - extra_content: Optional[JsonDict] = None, - ) -> EventBase: - content = {"membership": membership} - content.update(extra_content or {}) - builder = self.event_builder_factory.for_room_version( - RoomVersions.V1, - { - "type": "m.room.member", - "sender": user_id, - "state_key": user_id, - "room_id": TEST_ROOM_ID, - "content": content, - }, - ) - - event, unpersisted_context = self.get_success( - self.event_creation_handler.create_new_client_event(builder) - ) - context = self.get_success(unpersisted_context.persist(event)) - - self.get_success(self._persistence.persist_event(event, context)) - return event - - def _inject_message( - self, user_id: str, content: Optional[JsonDict] = None - ) -> EventBase: - if content is None: - content = {"body": "testytest", "msgtype": "m.text"} - builder = self.event_builder_factory.for_room_version( - RoomVersions.V1, - { - "type": "m.room.message", - "sender": user_id, - "room_id": TEST_ROOM_ID, - "content": content, - }, - ) - - event, unpersisted_context = self.get_success( - self.event_creation_handler.create_new_client_event(builder) - ) - context = self.get_success(unpersisted_context.persist(event)) - - self.get_success(self._persistence.persist_event(event, context)) - return event - def _inject_outlier(self) -> EventBase: builder = self.event_builder_factory.for_room_version( RoomVersions.V1, @@ -363,3 +338,34 @@ def test_out_of_band_invite_rejection(self) -> None: ), [], ) + + +async def inject_visibility_event( + hs: HomeServer, + room_id: str, + sender: str, + visibility: str, +) -> EventBase: + return await inject_event( + hs, + type="m.room.history_visibility", + sender=sender, + state_key="", + room_id=room_id, + content={"history_visibility": visibility}, + ) + + +async def inject_message_event( + hs: HomeServer, + room_id: str, + sender: str, + body: Optional[str] = "testytest", +) -> EventBase: + return await inject_event( + hs, + type="m.room.message", + sender=sender, + room_id=room_id, + content={"body": body, "msgtype": "m.text"}, + ) From 296f651eb2668d6d234ce7f44b904ed807e384ca Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 18 Apr 2024 16:34:20 +0100 Subject: [PATCH 3/8] Add a test for client event filtering --- tests/test_visibility.py | 73 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 429773c6af9..c67a169ebd9 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -24,11 +24,14 @@ from synapse.api.room_versions import RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.events.snapshot import EventContext +from synapse.rest import admin +from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.types import create_requester from synapse.visibility import filter_events_for_client, filter_events_for_server from tests import unittest from tests.test_utils.event_injection import inject_event, inject_member_event +from tests.unittest import HomeserverTestCase from tests.utils import create_room logger = logging.getLogger(__name__) @@ -267,7 +270,75 @@ def _inject_outlier(self) -> EventBase: return event -class FilterEventsForClientTestCase(unittest.FederatingHomeserverTestCase): +class FilterEventsForClientTestCase(HomeserverTestCase): + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def test_joined_history_visibility(self) -> None: + # User joins and leaves room. Should be able to seem the join and leave, + # and messages sent between the two, but not before or after. + + self.register_user("resident", "p1") + resident_token = self.login("resident", "p1") + room_id = self.helper.create_room_as("resident", tok=resident_token) + + self.get_success( + inject_visibility_event(self.hs, room_id, "@resident:hs", "joined") + ) + before_event = self.get_success( + inject_message_event(self.hs, room_id, "@resident:hs", body="before") + ) + join_event = self.get_success( + inject_member_event(self.hs, room_id, "@joiner:hs", "join") + ) + during_event = self.get_success( + inject_message_event(self.hs, room_id, "@resident:hs", body="during") + ) + leave_event = self.get_success( + inject_member_event(self.hs, room_id, "@joiner:hs", "leave") + ) + after_event = self.get_success( + inject_message_event(self.hs, room_id, "@resident:hs", body="after") + ) + + # We have to reload the events from the db, to ensure that prev_content is + # populated. + events_to_filter = [ + self.get_success( + self.hs.get_storage_controllers().main.get_event( + e.event_id, + get_prev_content=True, + ) + ) + for e in [ + before_event, + join_event, + during_event, + leave_event, + after_event, + ] + ] + + filtered_events = self.get_success( + filter_events_for_client( + self.hs.get_storage_controllers(), + "@joiner:hs", + events_to_filter, + ) + ) + + self.assertEqual( + [e.event_id for e in [join_event, during_event, leave_event]], + [e.event_id for e in filtered_events], + ) + + +class FilterEventsOutOfBandEventsForClientTestCase( + unittest.FederatingHomeserverTestCase +): def test_out_of_band_invite_rejection(self) -> None: # this is where we have received an invite event over federation, and then # rejected it. From b9a49e8f8551b0283321cc63a7b4117608d4751b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 18 Apr 2024 19:14:44 +0100 Subject: [PATCH 4/8] Return membership on each event returned to the client --- synapse/api/constants.py | 7 +++ synapse/config/experimental.py | 4 ++ synapse/events/utils.py | 23 ++++++- synapse/handlers/admin.py | 6 +- synapse/handlers/events.py | 7 ++- synapse/handlers/initial_sync.py | 7 ++- synapse/handlers/pagination.py | 1 + synapse/handlers/relations.py | 3 + synapse/handlers/room.py | 1 + synapse/handlers/search.py | 20 +++++-- synapse/handlers/sync.py | 2 + synapse/notifier.py | 1 + synapse/push/mailer.py | 5 +- synapse/visibility.py | 59 +++++++++++++++--- tests/events/test_utils.py | 13 ++++ tests/rest/client/test_retention.py | 7 ++- tests/test_visibility.py | 93 +++++++++++++++++++++++------ 17 files changed, 221 insertions(+), 38 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 98884b4967c..0a9123c56bb 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -234,6 +234,13 @@ class EventContentFields: TO_DEVICE_MSGID: Final = "org.matrix.msgid" +class EventUnsignedContentFields: + """Fields found inside the 'unsigned' data on events""" + + # Requesting user's membership, per MSC4115 + MSC4115_MEMBERSHIP: Final = "io.element.msc4115.membership" + + class RoomTypes: """Understood values of the room_type field of m.room.create events.""" diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 353ae23f910..23f9e77aa88 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -422,3 +422,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "MSC4108 requires MSC3861 to be enabled", ("experimental", "msc4108_delegation_endpoint"), ) + + self.msc4115_membership_on_events = experimental.get( + "msc4115_membership_on_events", False + ) diff --git a/synapse/events/utils.py b/synapse/events/utils.py index e0613d0dbc2..8b7c5885dbb 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -49,7 +49,7 @@ from synapse.api.room_versions import RoomVersion from synapse.types import JsonDict, Requester -from . import EventBase +from . import EventBase, make_event_from_dict if TYPE_CHECKING: from synapse.handlers.relations import BundledAggregations @@ -82,8 +82,6 @@ def prune_event(event: EventBase) -> EventBase: """ pruned_event_dict = prune_event_dict(event.room_version, event.get_dict()) - from . import make_event_from_dict - pruned_event = make_event_from_dict( pruned_event_dict, event.room_version, event.internal_metadata.get_dict() ) @@ -101,6 +99,25 @@ def prune_event(event: EventBase) -> EventBase: return pruned_event +def clone_event(event: EventBase) -> EventBase: + """Take a copy of the event. + + This is mostly useful because it does a *shallow* copy of the `unsigned` data, + which means it can then be updated without corrupting the in-memory cache. + """ + new_event = make_event_from_dict( + event.get_dict(), event.room_version, event.internal_metadata.get_dict() + ) + + # copy the internal fields + new_event.internal_metadata.stream_ordering = ( + event.internal_metadata.stream_ordering + ) + new_event.internal_metadata.outlier = event.internal_metadata.outlier + + return new_event + + def prune_event_dict(room_version: RoomVersion, event_dict: JsonDict) -> JsonDict: """Redacts the event_dict in the same way as `prune_event`, except it operates on dicts rather than event objects diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 360614e25b0..702d40332c4 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -42,6 +42,7 @@ def __init__(self, hs: "HomeServer"): self._device_handler = hs.get_device_handler() self._storage_controllers = hs.get_storage_controllers() self._state_storage_controller = self._storage_controllers.state + self._hs_config = hs.config self._msc3866_enabled = hs.config.experimental.msc3866.enabled async def get_whois(self, user: UserID) -> JsonMapping: @@ -217,7 +218,10 @@ async def export_user_data(self, user_id: str, writer: "ExfiltrationWriter") -> ) events = await filter_events_for_client( - self._storage_controllers, user_id, events + self._storage_controllers, + user_id, + events, + msc4115_membership_on_events=self._hs_config.experimental.msc4115_membership_on_events, ) writer.write_events(room_id, events) diff --git a/synapse/handlers/events.py b/synapse/handlers/events.py index c3fee74a98c..09d553cff1e 100644 --- a/synapse/handlers/events.py +++ b/synapse/handlers/events.py @@ -148,6 +148,7 @@ class EventHandler: def __init__(self, hs: "HomeServer"): self.store = hs.get_datastores().main self._storage_controllers = hs.get_storage_controllers() + self._config = hs.config async def get_event( self, @@ -189,7 +190,11 @@ async def get_event( is_peeking = not is_user_in_room filtered = await filter_events_for_client( - self._storage_controllers, user.to_string(), [event], is_peeking=is_peeking + self._storage_controllers, + user.to_string(), + [event], + is_peeking=is_peeking, + msc4115_membership_on_events=self._config.experimental.msc4115_membership_on_events, ) if not filtered: diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py index bcc5b285ac7..d99fc4bec04 100644 --- a/synapse/handlers/initial_sync.py +++ b/synapse/handlers/initial_sync.py @@ -221,7 +221,10 @@ async def handle_room(event: RoomsForUser) -> None: ).addErrback(unwrapFirstError) messages = await filter_events_for_client( - self._storage_controllers, user_id, messages + self._storage_controllers, + user_id, + messages, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) start_token = now_token.copy_and_replace(StreamKeyType.ROOM, token) @@ -380,6 +383,7 @@ async def _room_initial_sync_parted( requester.user.to_string(), messages, is_peeking=is_peeking, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) start_token = StreamToken.START.copy_and_replace(StreamKeyType.ROOM, token) @@ -494,6 +498,7 @@ async def get_receipts() -> List[JsonMapping]: requester.user.to_string(), messages, is_peeking=is_peeking, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) start_token = now_token.copy_and_replace(StreamKeyType.ROOM, token) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index cd3a9088cd3..6617105cdbd 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -623,6 +623,7 @@ async def get_messages( user_id, events, is_peeking=(member_event_id is None), + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) # if after the filter applied there are no more events diff --git a/synapse/handlers/relations.py b/synapse/handlers/relations.py index 931ac0c8133..c5cee8860b6 100644 --- a/synapse/handlers/relations.py +++ b/synapse/handlers/relations.py @@ -95,6 +95,7 @@ def __init__(self, hs: "HomeServer"): self._event_handler = hs.get_event_handler() self._event_serializer = hs.get_event_client_serializer() self._event_creation_handler = hs.get_event_creation_handler() + self._config = hs.config async def get_relations( self, @@ -163,6 +164,7 @@ async def get_relations( user_id, events, is_peeking=(member_event_id is None), + msc4115_membership_on_events=self._config.experimental.msc4115_membership_on_events, ) # The relations returned for the requested event do include their @@ -608,6 +610,7 @@ async def get_threads( user_id, events, is_peeking=(member_event_id is None), + msc4115_membership_on_events=self._config.experimental.msc4115_membership_on_events, ) aggregations = await self.get_bundled_aggregations( diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 5e81a51638a..51739a2653d 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1476,6 +1476,7 @@ async def filter_evts(events: List[EventBase]) -> List[EventBase]: user.to_string(), events, is_peeking=is_peeking, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) event = await self.store.get_event( diff --git a/synapse/handlers/search.py b/synapse/handlers/search.py index 19c5a2f257b..fdbe98de3b0 100644 --- a/synapse/handlers/search.py +++ b/synapse/handlers/search.py @@ -480,7 +480,10 @@ async def _search_by_rank( filtered_events = await search_filter.filter([r["event"] for r in results]) events = await filter_events_for_client( - self._storage_controllers, user.to_string(), filtered_events + self._storage_controllers, + user.to_string(), + filtered_events, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) events.sort(key=lambda e: -rank_map[e.event_id]) @@ -579,7 +582,10 @@ async def _search_by_recent( filtered_events = await search_filter.filter([r["event"] for r in results]) events = await filter_events_for_client( - self._storage_controllers, user.to_string(), filtered_events + self._storage_controllers, + user.to_string(), + filtered_events, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) room_events.extend(events) @@ -664,11 +670,17 @@ async def _calculate_event_contexts( ) events_before = await filter_events_for_client( - self._storage_controllers, user.to_string(), res.events_before + self._storage_controllers, + user.to_string(), + res.events_before, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) events_after = await filter_events_for_client( - self._storage_controllers, user.to_string(), res.events_after + self._storage_controllers, + user.to_string(), + res.events_after, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) context: JsonDict = { diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index a6d54ee4b81..8ff45a3353b 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -596,6 +596,7 @@ async def _load_filtered_recents( sync_config.user.to_string(), recents, always_include_ids=current_state_ids, + msc4115_membership_on_events=self.hs_config.experimental.msc4115_membership_on_events, ) log_kv({"recents_after_visibility_filtering": len(recents)}) else: @@ -681,6 +682,7 @@ async def _load_filtered_recents( sync_config.user.to_string(), loaded_recents, always_include_ids=current_state_ids, + msc4115_membership_on_events=self.hs_config.experimental.msc4115_membership_on_events, ) loaded_recents = [] diff --git a/synapse/notifier.py b/synapse/notifier.py index e87333a80aa..7c1cd3b5f2f 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -721,6 +721,7 @@ async def check_for_updates( user.to_string(), new_events, is_peeking=is_peeking, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) elif keyname == StreamKeyType.PRESENCE: now = self.clock.time_msec() diff --git a/synapse/push/mailer.py b/synapse/push/mailer.py index f1ffc8115f5..d5079652e4d 100644 --- a/synapse/push/mailer.py +++ b/synapse/push/mailer.py @@ -513,7 +513,10 @@ async def _get_notif_vars( } the_events = await filter_events_for_client( - self._storage_controllers, user_id, results.events_before + self._storage_controllers, + user_id, + results.events_before, + msc4115_membership_on_events=self.hs.config.experimental.msc4115_membership_on_events, ) the_events.append(notif_event) diff --git a/synapse/visibility.py b/synapse/visibility.py index 4be4ac81e72..17ec15f42cb 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -36,10 +36,15 @@ import attr -from synapse.api.constants import EventTypes, HistoryVisibility, Membership +from synapse.api.constants import ( + EventTypes, + EventUnsignedContentFields, + HistoryVisibility, + Membership, +) from synapse.events import EventBase from synapse.events.snapshot import EventContext -from synapse.events.utils import prune_event +from synapse.events.utils import clone_event, prune_event from synapse.logging.opentracing import trace from synapse.storage.controllers import StorageControllers from synapse.storage.databases.main import DataStore @@ -77,6 +82,7 @@ async def filter_events_for_client( is_peeking: bool = False, always_include_ids: FrozenSet[str] = frozenset(), filter_send_to_client: bool = True, + msc4115_membership_on_events: bool = False, ) -> List[EventBase]: """ Check which events a user is allowed to see. If the user can see the event but its @@ -95,9 +101,12 @@ async def filter_events_for_client( filter_send_to_client: Whether we're checking an event that's going to be sent to a client. This might not always be the case since this function can also be called to check whether a user can see the state at a given point. + msc4115_membership_on_events: Whether to include the requesting user's + membership in the "unsigned" data, per MSC4115. Returns: - The filtered events. + The filtered events. If `msc4115_membership_on_events` is true, the `unsigned` + data is annotated with the membership state of `user_id` at each event. """ # Filter out events that have been soft failed so that we don't relay them # to clients. @@ -134,7 +143,8 @@ async def filter_events_for_client( ) def allowed(event: EventBase) -> Optional[EventBase]: - return _check_client_allowed_to_see_event( + state_after_event = event_id_to_state.get(event.event_id) + filtered = _check_client_allowed_to_see_event( user_id=user_id, event=event, clock=storage.main.clock, @@ -142,13 +152,45 @@ def allowed(event: EventBase) -> Optional[EventBase]: sender_ignored=event.sender in ignore_list, always_include_ids=always_include_ids, retention_policy=retention_policies[room_id], - state=event_id_to_state.get(event.event_id), + state=state_after_event, is_peeking=is_peeking, sender_erased=erased_senders.get(event.sender, False), ) + if filtered is None: + return None + + if not msc4115_membership_on_events: + return filtered + + # Annotate the event with the user's membership after the event. + # + # Normally we just look in `state_after_event`, but if the event is an outlier + # we won't have such a state. The only outliers that are returned here are the + # user's own membership event, so we can just inspect that. + + user_membership_event: Optional[EventBase] + if event.type == EventTypes.Member and event.state_key == user_id: + user_membership_event = event + elif state_after_event is not None: + user_membership_event = state_after_event.get((EventTypes.Member, user_id)) + else: + # unreachable! + raise Exception("Missing state for event that is not user's own membership") + + user_membership = ( + user_membership_event.membership + if user_membership_event + else Membership.LEAVE + ) - # Check each event: gives an iterable of None or (a potentially modified) - # EventBase. + # Copy the event before updating the unsigned data: this shouldn't be persisted + # to the cache! + cloned = clone_event(filtered) + cloned.unsigned[EventUnsignedContentFields.MSC4115_MEMBERSHIP] = user_membership + + return cloned + + # Check each event: gives an iterable of None or (a modified) EventBase. filtered_events = map(allowed, events) # Turn it into a list and remove None entries before returning. @@ -412,8 +454,7 @@ def _check_membership( state: StateMap[EventBase], is_peeking: bool, ) -> _CheckMembershipReturn: - """Check whether the user can see the event due to their membership - """ + """Check whether the user can see the event due to their membership""" # If the event is the user's own membership event, use the 'most joined' # membership membership = None diff --git a/tests/events/test_utils.py b/tests/events/test_utils.py index cf81bcf52c5..975c285ec7d 100644 --- a/tests/events/test_utils.py +++ b/tests/events/test_utils.py @@ -32,6 +32,7 @@ PowerLevelsContent, SerializeEventConfig, _split_field, + clone_event, copy_and_fixup_power_levels_contents, maybe_upsert_event_field, prune_event, @@ -611,6 +612,18 @@ def test_relations(self) -> None: ) +class CloneEventTestCase(stdlib_unittest.TestCase): + def test_unsigned_is_copied(self) -> None: + original = make_event_from_dict( + {"type": "A", "event_id": "$test:domain", "unsigned": {"a": 1, "b": 2}} + ) + cloned = clone_event(original) + cloned.unsigned["b"] = 3 + + self.assertEqual(original.unsigned, {"a": 1, "b": 2}) + self.assertEqual(cloned.unsigned, {"a": 1, "b": 3}) + + class SerializeEventTestCase(stdlib_unittest.TestCase): def serialize(self, ev: EventBase, fields: Optional[List[str]]) -> JsonDict: return serialize_event( diff --git a/tests/rest/client/test_retention.py b/tests/rest/client/test_retention.py index 09a5d643495..ceae40498e1 100644 --- a/tests/rest/client/test_retention.py +++ b/tests/rest/client/test_retention.py @@ -163,7 +163,12 @@ def test_visibility(self) -> None: ) self.assertEqual(2, len(events), "events retrieved from database") filtered_events = self.get_success( - filter_events_for_client(storage_controllers, self.user_id, events) + filter_events_for_client( + storage_controllers, + self.user_id, + events, + msc4115_membership_on_events=True, + ) ) # We should only get one event back. diff --git a/tests/test_visibility.py b/tests/test_visibility.py index c67a169ebd9..1f7bc81ef61 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -21,6 +21,7 @@ from typing import Optional from unittest.mock import patch +from synapse.api.constants import EventUnsignedContentFields from synapse.api.room_versions import RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.events.snapshot import EventContext @@ -29,6 +30,7 @@ from synapse.server import HomeServer from synapse.types import create_requester from synapse.visibility import filter_events_for_client, filter_events_for_server + from tests import unittest from tests.test_utils.event_injection import inject_event, inject_member_event from tests.unittest import HomeserverTestCase @@ -286,22 +288,22 @@ def test_joined_history_visibility(self) -> None: room_id = self.helper.create_room_as("resident", tok=resident_token) self.get_success( - inject_visibility_event(self.hs, room_id, "@resident:hs", "joined") + inject_visibility_event(self.hs, room_id, "@resident:test", "joined") ) before_event = self.get_success( - inject_message_event(self.hs, room_id, "@resident:hs", body="before") + inject_message_event(self.hs, room_id, "@resident:test", body="before") ) join_event = self.get_success( - inject_member_event(self.hs, room_id, "@joiner:hs", "join") + inject_member_event(self.hs, room_id, "@joiner:test", "join") ) during_event = self.get_success( - inject_message_event(self.hs, room_id, "@resident:hs", body="during") + inject_message_event(self.hs, room_id, "@resident:test", body="during") ) leave_event = self.get_success( - inject_member_event(self.hs, room_id, "@joiner:hs", "leave") + inject_member_event(self.hs, room_id, "@joiner:test", "leave") ) after_event = self.get_success( - inject_message_event(self.hs, room_id, "@resident:hs", body="after") + inject_message_event(self.hs, room_id, "@resident:test", body="after") ) # We have to reload the events from the db, to ensure that prev_content is @@ -322,17 +324,64 @@ def test_joined_history_visibility(self) -> None: ] ] - filtered_events = self.get_success( + # Now run the events through the filter, and check that we can see the events + # we expect, and that the membership prop is as expected. + # + # We deliberately do the queries for both users upfront; this simulates + # concurrent queries on the server, and helps ensure that we aren't + # accidentally serving the same event object (with the same unsigned.membership + # property) to both users. + joiner_filtered_events = self.get_success( + filter_events_for_client( + self.hs.get_storage_controllers(), + "@joiner:test", + events_to_filter, + msc4115_membership_on_events=True, + ) + ) + resident_filtered_events = self.get_success( filter_events_for_client( self.hs.get_storage_controllers(), - "@joiner:hs", + "@resident:test", events_to_filter, + msc4115_membership_on_events=True, ) ) + # The joiner should be able to seem the join and leave, + # and messages sent between the two, but not before or after. self.assertEqual( [e.event_id for e in [join_event, during_event, leave_event]], - [e.event_id for e in filtered_events], + [e.event_id for e in joiner_filtered_events], + ) + self.assertEqual( + ["join", "join", "leave"], + [ + e.unsigned[EventUnsignedContentFields.MSC4115_MEMBERSHIP] + for e in joiner_filtered_events + ], + ) + + # The resident user should see all the events. + self.assertEqual( + [ + e.event_id + for e in [ + before_event, + join_event, + during_event, + leave_event, + after_event, + ] + ], + [e.event_id for e in resident_filtered_events], + ) + self.assertEqual( + ["join", "join", "join", "join", "join"], + [ + e.unsigned[EventUnsignedContentFields.MSC4115_MEMBERSHIP] + for e in resident_filtered_events + ], ) @@ -387,15 +436,24 @@ def test_out_of_band_invite_rejection(self) -> None: ) # the invited user should be able to see both the invite and the rejection + filtered_events = self.get_success( + filter_events_for_client( + self.hs.get_storage_controllers(), + "@user:test", + [invite_event, reject_event], + msc4115_membership_on_events=True, + ) + ) self.assertEqual( - self.get_success( - filter_events_for_client( - self.hs.get_storage_controllers(), - "@user:test", - [invite_event, reject_event], - ) - ), - [invite_event, reject_event], + [e.event_id for e in filtered_events], + [e.event_id for e in [invite_event, reject_event]], + ) + self.assertEqual( + ["invite", "leave"], + [ + e.unsigned[EventUnsignedContentFields.MSC4115_MEMBERSHIP] + for e in filtered_events + ], ) # other users should see neither @@ -405,6 +463,7 @@ def test_out_of_band_invite_rejection(self) -> None: self.hs.get_storage_controllers(), "@other:test", [invite_event, reject_event], + msc4115_membership_on_events=True, ) ), [], From 0ca0cd70e6fa29bf25246db7259c9f12c1176e6a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 18 Apr 2024 19:17:08 +0100 Subject: [PATCH 5/8] changelog --- changelog.d/17104.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17104.feature diff --git a/changelog.d/17104.feature b/changelog.d/17104.feature new file mode 100644 index 00000000000..1c2355e1559 --- /dev/null +++ b/changelog.d/17104.feature @@ -0,0 +1 @@ +Add support for MSC4115 (membership metadata on events). From b2b59c400f779ba47dffa14ede269d5e14ebf46e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 22 Apr 2024 18:41:28 +0100 Subject: [PATCH 6/8] Enable MSC4115 in complement tests --- docker/complement/conf/workers-shared-extra.yaml.j2 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docker/complement/conf/workers-shared-extra.yaml.j2 b/docker/complement/conf/workers-shared-extra.yaml.j2 index 32eada44197..a2c378f5474 100644 --- a/docker/complement/conf/workers-shared-extra.yaml.j2 +++ b/docker/complement/conf/workers-shared-extra.yaml.j2 @@ -92,8 +92,6 @@ allow_device_name_lookup_over_federation: true ## Experimental Features ## experimental_features: - # client-side support for partial state in /send_join responses - faster_joins: true # Enable support for polls msc3381_polls_enabled: true # Enable deleting device-specific notification settings stored in account data @@ -105,6 +103,8 @@ experimental_features: # no UIA for x-signing upload for the first time msc3967_enabled: true + msc4115_membership_on_events: true + server_notices: system_mxid_localpart: _server system_mxid_display_name: "Server Alert" From 88f9ba5d48daac851a4b3a72d9559a309c213121 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:45:37 +0100 Subject: [PATCH 7/8] Update tests/test_visibility.py Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- tests/test_visibility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 1f7bc81ef61..3e2100eab47 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -280,7 +280,7 @@ class FilterEventsForClientTestCase(HomeserverTestCase): ] def test_joined_history_visibility(self) -> None: - # User joins and leaves room. Should be able to seem the join and leave, + # User joins and leaves room. Should be able to see the join and leave, # and messages sent between the two, but not before or after. self.register_user("resident", "p1") From 7da98babd2fa9381009056db0511fa4779120479 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 26 Apr 2024 14:56:05 +0100 Subject: [PATCH 8/8] Add comments and address review comments --- rust/src/events/internal_metadata.rs | 9 +++++++-- synapse/events/utils.py | 11 +++++++---- synapse/visibility.py | 10 ++++++---- tests/events/test_utils.py | 13 ++++++++++++- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/rust/src/events/internal_metadata.rs b/rust/src/events/internal_metadata.rs index a53601862dc..53c7b1ba610 100644 --- a/rust/src/events/internal_metadata.rs +++ b/rust/src/events/internal_metadata.rs @@ -20,8 +20,10 @@ //! Implements the internal metadata class attached to events. //! -//! The internal metadata is a bit like a `TypedDict`, in that it is stored as a -//! JSON dict in the DB. Most events have zero, or only a few, of these keys +//! The internal metadata is a bit like a `TypedDict`, in that most of +//! it is stored as a JSON dict in the DB (the exceptions being `outlier` +//! and `stream_ordering` which have their own columns in the database). +//! Most events have zero, or only a few, of these keys //! set. Therefore, since we care more about memory size than performance here, //! we store these fields in a mapping. //! @@ -234,6 +236,9 @@ impl EventInternalMetadata { self.clone() } + /// Get a dict holding the data stored in the `internal_metadata` column in the database. + /// + /// Note that `outlier` and `stream_ordering` are stored in separate columns so are not returned here. fn get_dict(&self, py: Python<'_>) -> PyResult { let dict = PyDict::new(py); diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 8b7c5885dbb..07724723125 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -86,11 +86,10 @@ def prune_event(event: EventBase) -> EventBase: pruned_event_dict, event.room_version, event.internal_metadata.get_dict() ) - # copy the internal fields + # Copy the bits of `internal_metadata` that aren't returned by `get_dict` pruned_event.internal_metadata.stream_ordering = ( event.internal_metadata.stream_ordering ) - pruned_event.internal_metadata.outlier = event.internal_metadata.outlier # Mark the event as redacted @@ -103,13 +102,17 @@ def clone_event(event: EventBase) -> EventBase: """Take a copy of the event. This is mostly useful because it does a *shallow* copy of the `unsigned` data, - which means it can then be updated without corrupting the in-memory cache. + which means it can then be updated without corrupting the in-memory cache. Note that + other properties of the event, such as `content`, are *not* (currently) copied here. """ + # XXX: We rely on at least one of `event.get_dict()` and `make_event_from_dict()` + # making a copy of `unsigned`. Currently, both do, though I don't really know why. + # Still, as long as they do, there's not much point doing yet another copy here. new_event = make_event_from_dict( event.get_dict(), event.room_version, event.internal_metadata.get_dict() ) - # copy the internal fields + # Copy the bits of `internal_metadata` that aren't returned by `get_dict`. new_event.internal_metadata.stream_ordering = ( event.internal_metadata.stream_ordering ) diff --git a/synapse/visibility.py b/synapse/visibility.py index 17ec15f42cb..09a947ef15e 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -438,13 +438,15 @@ def _check_client_allowed_to_see_event( @attr.s(frozen=True, slots=True, auto_attribs=True) class _CheckMembershipReturn: - """Return value of _check_membership""" + """Return value of `_check_membership`. - allowed: bool - """Whether the user should be allowed to see the event""" + Attributes: + allowed: Whether the user should be allowed to see the event. + joined: Whether the user was joined to the room at the event. + """ + allowed: bool joined: bool - """Whether the user was joined at the event""" def _check_membership( diff --git a/tests/events/test_utils.py b/tests/events/test_utils.py index 975c285ec7d..d5ac66a6ed9 100644 --- a/tests/events/test_utils.py +++ b/tests/events/test_utils.py @@ -615,13 +615,24 @@ def test_relations(self) -> None: class CloneEventTestCase(stdlib_unittest.TestCase): def test_unsigned_is_copied(self) -> None: original = make_event_from_dict( - {"type": "A", "event_id": "$test:domain", "unsigned": {"a": 1, "b": 2}} + { + "type": "A", + "event_id": "$test:domain", + "unsigned": {"a": 1, "b": 2}, + }, + RoomVersions.V1, + {"txn_id": "txn"}, ) + original.internal_metadata.stream_ordering = 1234 + self.assertEqual(original.internal_metadata.stream_ordering, 1234) + cloned = clone_event(original) cloned.unsigned["b"] = 3 self.assertEqual(original.unsigned, {"a": 1, "b": 2}) self.assertEqual(cloned.unsigned, {"a": 1, "b": 3}) + self.assertEqual(cloned.internal_metadata.stream_ordering, 1234) + self.assertEqual(cloned.internal_metadata.txn_id, "txn") class SerializeEventTestCase(stdlib_unittest.TestCase):