Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for MSC4115 #17104

Merged
merged 9 commits into from
Apr 29, 2024
7 changes: 7 additions & 0 deletions synapse/api/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""

Expand Down
4 changes: 4 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
23 changes: 20 additions & 3 deletions synapse/events/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
)
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you not want a deep copy of the unsigned dictionary instead, such that you can update it without affecting the original?

Granted, your unit test proves that this does the right thing!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well. As it happens, we're updating a top-level key in the unsigned dict, so a shallow copy is enough.

I mean doing a shallow copy isn't deliberate on my part, it's just what calling make_event_from_dict(event.get_dict(), ...) happens to do.

Generally this whole thing seems like a big mess, and I kinda hate it.

  • Why do both event.get_dict and make_event_from_dict copy unsigned, when they doesn't copy any of the other properties (like, say, content)?
  • If they are going to copy it, why is it a shallow copy?
  • Why are stream_ordering and outlier part of internal_metadata in the in-memory model when they are separate in the database, so need magic handling?
  • Also, why is the value of unsigned not protected by use_frozen_dicts like the rest of the event, so that I nearly shot my own foot off here?

Nobody knows, but fixing these things aren't really today jobs.

I'll add some more comments which might help here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough explanation. I agree that the state of those functions are a mess, and look like the result of organic growth. They could use a refactor.

Agreed that that's a problem for another day.

"""
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
Expand Down
6 changes: 5 additions & 1 deletion synapse/handlers/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion synapse/handlers/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down
7 changes: 6 additions & 1 deletion synapse/handlers/initial_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions synapse/handlers/pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions synapse/handlers/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
20 changes: 16 additions & 4 deletions synapse/handlers/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 = {
Expand Down
2 changes: 2 additions & 0 deletions synapse/handlers/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 = []
Expand Down
1 change: 1 addition & 0 deletions synapse/notifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
5 changes: 4 additions & 1 deletion synapse/push/mailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
59 changes: 50 additions & 9 deletions synapse/visibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -134,21 +143,54 @@ 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,
filter_send_to_client=filter_send_to_client,
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.
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions tests/events/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
PowerLevelsContent,
SerializeEventConfig,
_split_field,
clone_event,
copy_and_fixup_power_levels_contents,
maybe_upsert_event_field,
prune_event,
Expand Down Expand Up @@ -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(
Expand Down
Loading