Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement MSC2175 to stop adding the creator field. #15394

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Implement MSC2175 to stop adding the creator field. #15394

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
04ce3e3
Implement MSC2175 to stop adding the creator field.
clokep Apr 5, 2023
f5c325b
Inline confusing function which doesn't have to do with the create ev…
clokep Apr 5, 2023
fd8f45a
Newsfragment
clokep Apr 5, 2023
811df39
Tweak event auth rules to use sender in most places.
clokep Apr 5, 2023
48ef4f9
Update other references to the room creator.
clokep Apr 5, 2023
06b6fd8
Review comments.
clokep Apr 5, 2023
a353870
Update check against creator.
clokep Apr 5, 2023
c66df21
Also update power level check.
clokep Apr 5, 2023
b18ab18
Add back logic to use creator field.
clokep Apr 6, 2023
b682443
Fix-up one more spot.
clokep Apr 6, 2023
96598a5
Lint
clokep Apr 6, 2023
9403987
Merge remote-tracking branch 'origin/develop' into clokep/msc2175
clokep Apr 6, 2023
22a71da
Avoid allocating a dict.
clokep Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15394.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement [MSC2175](https://github.com/matrix-org/matrix-doc/pull/2175) to stop adding `creator` to create events.
2 changes: 2 additions & 0 deletions synapse/api/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ class EventContentFields:
FEDERATE: Final = "m.federate"

# The creator of the room, as used in `m.room.create` events.
#
# This is deprecated in MSC2175.
ROOM_CREATOR: Final = "creator"

# Used in m.room.guest_access events.
Expand Down
16 changes: 16 additions & 0 deletions synapse/api/room_versions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ class RoomVersion:
# MSC2209: Check 'notifications' key while verifying
# m.room.power_levels auth rules.
limit_notifications_power_levels: bool
# MSC2175: No longer include the creator in m.room.create events.
msc2175_implicit_room_creator: bool
# MSC2174/MSC2176: Apply updated redaction rules algorithm.
msc2176_redaction_rules: bool
# MSC3083: Support the 'restricted' join_rule.
Expand Down Expand Up @@ -116,6 +118,7 @@ class RoomVersions:
special_case_aliases_auth=True,
strict_canonicaljson=False,
limit_notifications_power_levels=False,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -135,6 +138,7 @@ class RoomVersions:
special_case_aliases_auth=True,
strict_canonicaljson=False,
limit_notifications_power_levels=False,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -154,6 +158,7 @@ class RoomVersions:
special_case_aliases_auth=True,
strict_canonicaljson=False,
limit_notifications_power_levels=False,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -173,6 +178,7 @@ class RoomVersions:
special_case_aliases_auth=True,
strict_canonicaljson=False,
limit_notifications_power_levels=False,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -192,6 +198,7 @@ class RoomVersions:
special_case_aliases_auth=True,
strict_canonicaljson=False,
limit_notifications_power_levels=False,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -211,6 +218,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -230,6 +238,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=True,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -249,6 +258,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -268,6 +278,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=True,
msc3375_redaction_rules=False,
Expand All @@ -287,6 +298,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=True,
msc3375_redaction_rules=True,
Expand All @@ -306,6 +318,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=True,
msc3375_redaction_rules=True,
Expand All @@ -325,6 +338,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=True,
msc3375_redaction_rules=True,
Expand All @@ -344,6 +358,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=False,
msc3375_redaction_rules=False,
Expand All @@ -364,6 +379,7 @@ class RoomVersions:
special_case_aliases_auth=False,
strict_canonicaljson=True,
limit_notifications_power_levels=True,
msc2175_implicit_room_creator=False,
msc2176_redaction_rules=False,
msc3083_join_rules=True,
msc3375_redaction_rules=True,
Expand Down
15 changes: 11 additions & 4 deletions synapse/event_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,11 @@ def _check_create(event: "EventBase") -> None:
"room appears to have unsupported version %s" % (room_version_prop,),
)

# 1.4 If content has no creator field, reject.
if EventContentFields.ROOM_CREATOR not in event.content:
# 1.4 If content has no creator field, reject if the room version requires it.
if (
not event.room_version.msc2175_implicit_room_creator
and EventContentFields.ROOM_CREATOR not in event.content
clokep marked this conversation as resolved.
Show resolved Hide resolved
):
raise AuthError(403, "Create event lacks a 'creator' property")


Expand Down Expand Up @@ -491,7 +494,11 @@ def _is_membership_change_allowed(
key = (EventTypes.Create, "")
create = auth_events.get(key)
if create and event.prev_event_ids()[0] == create.event_id:
if create.content["creator"] == event.state_key:
if room_version.msc2175_implicit_room_creator:
creator = create.sender
else:
creator = create.content[EventContentFields.ROOM_CREATOR]
if creator == event.state_key:
return

target_user_id = event.state_key
Expand Down Expand Up @@ -1004,7 +1011,7 @@ def get_user_power_level(user_id: str, auth_events: StateMap["EventBase"]) -> in
# that.
key = (EventTypes.Create, "")
create_event = auth_events.get(key)
if create_event is not None and create_event.content["creator"] == user_id:
if create_event is not None and create_event.sender == user_id:
return 100
else:
return 0
Expand Down
3 changes: 1 addition & 2 deletions synapse/handlers/federation_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,10 +1515,9 @@ async def _handle_marker_event(self, origin: str, marker_event: EventBase) -> No
# support it or the event is not from the room creator.
room_version = await self._store.get_room_version(marker_event.room_id)
create_event = await self._store.get_create_event_for_room(marker_event.room_id)
room_creator = create_event.content.get(EventContentFields.ROOM_CREATOR)
if not room_version.msc2716_historical and (
not self._config.experimental.msc2716_enabled
or marker_event.sender != room_creator
or marker_event.sender != create_event.sender
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the existing logic correct? Shouldn't it be if not room_version.msc2716_historical OR (...)?

(A => B iff not A or B.)

Additionally: I think (strictly speaking) that this makes MSC2716 depend on MSC2175?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the existing logic correct? Shouldn't it be if not room_version.msc2716_historical OR (...)?

I'm not sure, would need to ask @MadLittleMods.

Additionally: I think (strictly speaking) that this makes MSC2716 depend on MSC2175?

Yes, I guess this should probably use the same or logic that we use in other spots.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I fixed this to break the dependency. 🤷 )

Copy link
Contributor

@MadLittleMods MadLittleMods Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the logic correct. This is taking a lot of squinting so it could probably be clearer but here is the breakdown:

  • If the room version supports MSC2716: we always process marker events (no early return)
  • If the room version does not support MSC2716 (existing room versions), the MSC2716 experimental feature has to be enabled and the event has to be from the room creator

If we used OR here, then the second case with existing room versions would never be work.

synapse/synapse/handlers/federation_event.py

Lines 1522 to 1524 in b682443

if not room_version.msc2716_historical and (
not self._config.experimental.msc2716_enabled
or marker_event.sender != room_creator

If we break these down into conditions, then the current boolean logic looks like: not V and (not E or not C)

is_supported_room_version = room_version.msc2716_historical # V
is_msc2716_enabled = self._config.experimental.msc2716_enabled # E
is_event_from_room_creator = marker_event.sender == room_creator # C

If we add some labels and derive a positive should_process_event condition which we can then later negate, then the boolean logic looks like not (V or (E and C)) which simplifies to the exact same logic above (see alternate form listed on Wolfram Alpha)

This probably does make more sense though (if we find it clearer, feel free to update the 3 spots where we do this):

is_event_eligible_to_process_in_existing_room_versions = is_msc2716_enabled and is_event_from_room_creator
should_process_event = is_supported_room_version or is_event_eligible_to_process_in_existing_room_versions
if not should_process_event:
    return

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, thanks for double checking. (I'd missed that this logic was guarding an early return, amongst other things.)

):
return

Expand Down
3 changes: 1 addition & 2 deletions synapse/handlers/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1909,13 +1909,12 @@ async def persist_and_notify_client_events(
room_version_obj = KNOWN_ROOM_VERSIONS[room_version]

create_event = await self.store.get_create_event_for_room(event.room_id)
room_creator = create_event.content.get(EventContentFields.ROOM_CREATOR)

# Only check an insertion event if the room version
# supports it or the event is from the room creator.
if room_version_obj.msc2716_historical or (
self.config.experimental.msc2716_enabled
and event.sender == room_creator
and event.sender == create_event.sender
):
next_batch_id = event.content.get(
EventContentFields.MSC2716_NEXT_BATCH_ID
Expand Down
22 changes: 12 additions & 10 deletions synapse/handlers/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ async def clone_existing_room(
await self._send_events_for_new_room(
requester,
new_room_id,
new_room_version,
# we expect to override all the presets with initial_state, so this is
# somewhat arbitrary.
room_config={"preset": RoomCreationPreset.PRIVATE_CHAT},
Expand Down Expand Up @@ -922,6 +923,7 @@ async def create_room(
) = await self._send_events_for_new_room(
requester,
room_id,
room_version,
room_config=config,
invite_list=invite_list,
initial_state=initial_state,
Expand Down Expand Up @@ -998,6 +1000,7 @@ async def _send_events_for_new_room(
self,
creator: Requester,
room_id: str,
room_version: RoomVersion,
room_config: JsonDict,
invite_list: List[str],
initial_state: MutableStateMap,
Expand All @@ -1020,6 +1023,8 @@ async def _send_events_for_new_room(
the user requesting the room creation
room_id:
room id for the room being created
room_version:
The room version of the new room.
room_config:
A dict of configuration options. This will be the body of
a /createRoom request; see
Expand Down Expand Up @@ -1053,14 +1058,6 @@ async def _send_events_for_new_room(
# (as this info can't be pulled from the db)
state_map: MutableStateMap[str] = {}

def create_event_dict(etype: str, content: JsonDict, **kwargs: Any) -> JsonDict:
e = {"type": etype, "content": content}

e.update(event_keys)
e.update(kwargs)

return e

async def create_event(
etype: str,
content: JsonDict,
Expand All @@ -1083,7 +1080,10 @@ async def create_event(
nonlocal depth
nonlocal prev_event

event_dict = create_event_dict(etype, content, **kwargs)
# Create the event dictionary.
event_dict = {"type": etype, "content": content}
event_dict.update(event_keys)
event_dict.update(kwargs)

(
new_event,
Expand Down Expand Up @@ -1120,7 +1120,9 @@ async def create_event(
400, f"'{preset_config}' is not a valid preset", errcode=Codes.BAD_JSON
)

creation_content.update({"creator": creator_id})
# MSC2175 removes the creator field from the create event.
if not room_version.msc2175_implicit_room_creator:
creation_content.update({"creator": creator_id})
clokep marked this conversation as resolved.
Show resolved Hide resolved
creation_event, unpersisted_creation_context = await create_event(
EventTypes.Create, creation_content, False
)
Expand Down
12 changes: 4 additions & 8 deletions synapse/storage/databases/main/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1998,6 +1998,9 @@ def _background_populate_rooms_creator_column_txn(
for room_id, event_json in room_id_to_create_event_results:
event_dict = db_to_json(event_json)

# The creator property might not exist in newer room versions, but
# for those versions the creator column should be properly populate
# during room creation.
creator = event_dict.get("content").get(EventContentFields.ROOM_CREATOR)
Comment on lines +2001 to 2004
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: Is the rooms.creator column useful? I guess it avoids looking up the create event and from the state and event_json tables (though I'd bet we're already looking in those tables anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fairly certain that we do use it, but it seems like it has been around forever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, maybe not -- it was added in #10697.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be mentioned on #10737 too.


self.db_pool.simple_update_txn(
Expand Down Expand Up @@ -2132,21 +2135,14 @@ async def upsert_room_on_join(
# invalid, and it would fail auth checks anyway.
raise StoreError(400, "No create event in state")

room_creator = create_event.content.get(EventContentFields.ROOM_CREATOR)

if not isinstance(room_creator, str):
# If the create event does not have a creator then the room is
# invalid, and it would fail auth checks anyway.
raise StoreError(400, "No creator defined on the create event")

await self.db_pool.simple_upsert(
desc="upsert_room_on_join",
table="rooms",
keyvalues={"room_id": room_id},
values={"room_version": room_version.identifier},
insertion_values={
"is_public": False,
"creator": room_creator,
clokep marked this conversation as resolved.
Show resolved Hide resolved
"creator": create_event.sender,
"has_auth_chain_index": has_auth_chain_index,
},
)
Expand Down