-
Notifications
You must be signed in to change notification settings - Fork 579
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 10 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, | ||
|
|
@@ -1061,3 +1052,12 @@ def parse_stripped_state_event(raw_stripped_event: Any) -> StrippedStateEvent | | |
| ) | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def serialize_stripped_state_event(stripped_event: StrippedStateEvent) -> JsonDict: | ||
|
MadLittleMods marked this conversation as resolved.
Outdated
|
||
| return { | ||
| "type": stripped_event.type, | ||
| "state_key": stripped_event.state_key, | ||
| "sender": stripped_event.sender, | ||
| "content": stripped_event.content, | ||
| } | ||
| 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,10 +1398,18 @@ 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: | ||
| # TODO: MSC4311: The 400 `M_MISSING_PARAM` error SHOULD be translated to a 5xx | ||
| # error by the sending server over the Client-Server API. This is done | ||
| # because there's nothing the client can materially do differently to make | ||
| # the request succeed. | ||
|
|
||
| # If an error is received that is due to an unrecognised endpoint, | ||
| # fallback to the v1 endpoint if the room uses old-style event IDs. | ||
| # Otherwise, consider it a legitimate error and raise. | ||
|
|
@@ -1385,6 +1433,10 @@ async def _do_send_invite( | |
| event_id=pdu.event_id, | ||
| content=pdu.get_pdu_json(time_now), | ||
| ) | ||
| # TODO: MSC4311: The 400 `M_MISSING_PARAM` error SHOULD be translated to a 5xx | ||
| # error by the sending server over the Client-Server API. This is done | ||
| # because there's nothing the client can materially do differently to make | ||
| # the request succeed. | ||
| return content | ||
|
|
||
| async def send_leave(self, destinations: Iterable[str], pdu: EventBase) -> None: | ||
|
|
||
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.