-
Notifications
You must be signed in to change notification settings - Fork 592
MSC4311: Use full PDU's in stripped state (like invite_room_state) over federation and always include m.room.create event
#19723
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
base: develop
Are you sure you want to change the base?
Changes from 6 commits
bdfeb90
43a11f5
7f25120
6026aaa
e0eb224
3464ec8
f96c008
a088aa8
336b686
22f4f20
cce5dc7
7e379e7
92d0d8b
76b4905
5103f1b
5c1f4ca
374c4c5
ffe5c4b
2b900b4
5e12cb8
ff533df
de78d9e
4598a43
fbff685
75a53ef
178dd89
1926534
17c3763
75d1935
3888387
259f151
c289e93
ec572c6
4e7f3fc
bf9ab2f
c54c93b
17a4ef6
1cf8282
ce17c93
5cc8753
53a68d7
7488a24
3870034
40d1316
3fc94ba
26bf739
2822f3c
3ce6900
19d8a7a
ecab104
eed3071
f751b5f
e06b311
e301cdd
bc73bb0
0baa67b
4f70688
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Remove flawed [MSC4311](https://github.com/matrix-org/matrix-spec-proposals/pull/4311) partial implementation: Client-side API's like `/sync` should still use stripped events. | ||
|
MadLittleMods marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1019,15 +1019,6 @@ def strip_event(event: EventBase) -> JsonDict: | |
| Stripped state events can only have the `sender`, `type`, `state_key` and `content` | ||
| properties present. | ||
| """ | ||
| # MSC4311: Ensure the create event is available on invites and knocks. | ||
| # TODO: Implement the rest of MSC4311 | ||
| if ( | ||
| event.room_version.msc4291_room_ids_as_hashes | ||
| and event.type == EventTypes.Create | ||
| and event.get_state_key() == "" | ||
| ): | ||
| return event.get_pdu_json() | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return { | ||
| "type": event.type, | ||
| "state_key": event.state_key, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,8 @@ | |
| RoomVersions, | ||
| ) | ||
| from synapse.events import EventBase, builder, make_event_from_dict | ||
| from synapse.events.snapshot import EventContext | ||
| from synapse.events.utils import parse_stripped_state_event | ||
| from synapse.federation.federation_base import ( | ||
| FederationBase, | ||
| InvalidEventSignatureError, | ||
|
|
@@ -71,7 +73,13 @@ | |
| from synapse.http.types import QueryParams | ||
| from synapse.logging.opentracing import SynapseTags, log_kv, set_tag, tag_args, trace | ||
| from synapse.metrics import SERVER_NAME_LABEL | ||
| from synapse.types import JsonDict, StrCollection, UserID, get_domain_from_id | ||
| from synapse.types import ( | ||
| JsonDict, | ||
| StrCollection, | ||
| UserID, | ||
| get_domain_from_id, | ||
| ) | ||
| from synapse.types.state import StateFilter | ||
| from synapse.util.async_helpers import concurrently_execute | ||
| from synapse.util.caches.expiringcache import ExpiringCache | ||
| from synapse.util.duration import Duration | ||
|
|
@@ -1309,12 +1317,12 @@ async def send_invite( | |
| self, | ||
| destination: str, | ||
| room_id: str, | ||
| event_id: str, | ||
| pdu: EventBase, | ||
| context: EventContext, | ||
| ) -> EventBase: | ||
| room_version = await self.store.get_room_version(room_id) | ||
|
|
||
| content = await self._do_send_invite(destination, pdu, room_version) | ||
| content = await self._do_send_invite(destination, pdu, context, room_version) | ||
|
|
||
| pdu_dict = content["event"] | ||
|
|
||
|
|
@@ -1335,7 +1343,11 @@ async def send_invite( | |
| return pdu | ||
|
|
||
| async def _do_send_invite( | ||
| self, destination: str, pdu: EventBase, room_version: RoomVersion | ||
| self, | ||
| destination: str, | ||
| pdu: EventBase, | ||
| context: EventContext, | ||
| room_version: RoomVersion, | ||
| ) -> JsonDict: | ||
| """Actually sends the invite, first trying v2 API and falling back to | ||
| v1 API if necessary. | ||
|
|
@@ -1350,6 +1362,34 @@ async def _do_send_invite( | |
| """ | ||
| time_now = self._clock.time_msec() | ||
|
|
||
| # MSC4311: For the federation API, format events in `invite_room_state` as full | ||
| # PDU's | ||
| # | ||
| # First get all of the expected stripped state events that should be included. | ||
| # We will derive these from the `unsigned` part of the PDU but this doesn't | ||
| # include any event ID information so we need to look it up based on the state | ||
| # at the time of the invite. | ||
| stripped_state_types = [] | ||
| for raw_stripped_event in pdu.unsigned.get("invite_room_state", []): | ||
| stripped_state_event = parse_stripped_state_event(raw_stripped_event) | ||
| # Since this is our own invite, it should always be well-formed | ||
| assert stripped_state_event is not None, ( | ||
| "Unable to parse one of the evnts from the `invite_room_state` as a stripped state event" | ||
| ) | ||
| stripped_state_types.append( | ||
| (stripped_state_event.type, stripped_state_event.state_key) | ||
| ) | ||
|
|
||
| # Find the full events based on the state at the time of the invite | ||
| state_filter = StateFilter.from_types(stripped_state_types) | ||
| state_ids = await self.store.get_stripped_room_state_ids_from_event_context( | ||
| context, state_filter | ||
| ) | ||
| state_events = await self.store.get_events(state_ids) | ||
| assert set(state_ids) == set(state_events.keys()), ( | ||
| "We should have all events available that were set as stripped state." | ||
| ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, this is technically a behaviour change where we are recalculating the stripped state here rather than using the stripped state we calculated at event creation. Generally, I'm wondering if we should change the format of what we store as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 Future plans |
||
|
|
||
| try: | ||
| return await self.transport_layer.send_invite_v2( | ||
| destination=destination, | ||
|
|
@@ -1358,7 +1398,10 @@ async def _do_send_invite( | |
| content={ | ||
| "event": pdu.get_pdu_json(time_now), | ||
| "room_version": room_version.identifier, | ||
| "invite_room_state": pdu.unsigned.get("invite_room_state", []), | ||
| "invite_room_state": [ | ||
| state_event.get_pdu_json(time_now) | ||
| for state_event in state_events.values() | ||
| ], | ||
| }, | ||
| ) | ||
| except HttpResponseException as e: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1137,19 +1137,46 @@ async def get_stripped_room_state_from_event_context( | |||||
| filter = StateFilter.from_types(types) | ||||||
| else: | ||||||
| filter = state_keys_to_include | ||||||
| selected_state_ids = await context.get_current_state_ids(filter) | ||||||
|
|
||||||
| selected_state_ids = await self.get_stripped_room_state_ids_from_event_context( | ||||||
| context, filter | ||||||
| ) | ||||||
|
|
||||||
| state_to_include = await self.get_events(selected_state_ids) | ||||||
|
|
||||||
| return [strip_event(e) for e in state_to_include.values()] | ||||||
|
|
||||||
| async def get_stripped_room_state_ids_from_event_context( | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split some logic out of This way we can use the same event selection logic but fetch/format them however we see fit. |
||||||
| self, | ||||||
| context: EventContext, | ||||||
| state_keys_to_include: StateFilter, | ||||||
| ) -> list[str]: | ||||||
| """ | ||||||
| Retrieve the stripped state IDs for an event, given an event context to retrieve state | ||||||
| from as well as the state types to include. Optionally, include the membership | ||||||
| events from a specific user. | ||||||
|
|
||||||
| "Stripped" state means that only the `type`, `state_key`, `content` and `sender` keys | ||||||
| are included from each state event. | ||||||
|
Comment on lines
+1166
to
+1167
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really relevant to this function which is only about getting the state event IDs.
Suggested change
|
||||||
|
|
||||||
| Args: | ||||||
| context: The event context to retrieve state of the room from. | ||||||
| state_keys_to_include: The state events to include, for each event type. | ||||||
|
|
||||||
| Returns: | ||||||
| A list of event_ids, each representing the stripped state event to include for this event | ||||||
| """ | ||||||
| selected_state_ids = await context.get_current_state_ids(state_keys_to_include) | ||||||
|
|
||||||
| # We know this event is not an outlier, so this must be | ||||||
| # non-None. | ||||||
| assert selected_state_ids is not None | ||||||
|
|
||||||
| # Confusingly, get_current_state_events may return events that are discarded by | ||||||
| # the filter, if they're in context._state_delta_due_to_event. Strip these away. | ||||||
| selected_state_ids = filter.filter_state(selected_state_ids) | ||||||
|
|
||||||
| state_to_include = await self.get_events(selected_state_ids.values()) | ||||||
| # Confusingly, `get_current_state_ids` may return events that are discarded by | ||||||
| # the filter, if they're in `context._state_delta_due_to_event`. Strip these away. | ||||||
| selected_state_ids = state_keys_to_include.filter_state(selected_state_ids) | ||||||
|
|
||||||
| return [strip_event(e) for e in state_to_include.values()] | ||||||
| return list(selected_state_ids.values()) | ||||||
|
|
||||||
| def _maybe_start_fetch_thread(self) -> None: | ||||||
| """Starts an event fetch thread if we are not yet at the maximum number.""" | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Complement tests are expected to fail ❌ as we removed the flawed partial implementation in this PR.
The Complement tests have been updated in matrix-org/complement#796 and pass locally. We will merge both PRs at the same time.