From c6e994fffc83579adb7956a1a0b88edec21de5bb Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 23 Jul 2025 15:31:07 +0100 Subject: [PATCH 01/19] Fix structure of initial_state when calling user_may_create_room module API callback --- synapse/handlers/room.py | 5 +++- tests/module_api/test_spamchecker.py | 38 +++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index d8c4d0c20e7..8dad914774b 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -586,7 +586,10 @@ async def clone_existing_room( user_id, { "creation_content": creation_content, - "initial_state": list(initial_state.items()), + "initial_state": [ + {"type": state[0][0], "state_key": state[0][1], "content": state[1]} + for state in initial_state.items() + ], }, ) if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 926fe30b435..8ac70ebe7f9 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -72,6 +72,41 @@ async def user_may_create_room( self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_config["foo"], "baa") + def test_may_user_create_room_with_initial_state(self) -> None: + """Test that the may_user_create_room callback is called when a user + creates a room with some initial state events, and that it receives the correct parameters. + """ + + async def user_may_create_room( + user_id: str, room_config: JsonDict + ) -> Union[Literal["NOT_SPAM"], Codes]: + self.last_room_config = room_config + self.last_user_id = user_id + return "NOT_SPAM" + + self._module_api.register_spam_checker_callbacks( + user_may_create_room=user_may_create_room + ) + + channel = self.create_room( + { + "foo": "baa", + "initial_state": [ + {"type": "m.room.topic", "content": {"topic": "foo"}} + ], + } + ) + self.assertEqual(channel.code, 200) + self.assertEqual(self.last_user_id, self.user_id) + self.assertEqual(self.last_room_config["foo"], "baa") + self.assertTrue( + any( + event.get("type") == "m.room.topic" + and event.get("content").get("topic") == "foo" + for event in self.last_room_config["initial_state"] + ) + ) + def test_may_user_create_room_on_upgrade(self) -> None: """Test that the may_user_create_room callback is called when a room is upgraded.""" @@ -107,7 +142,8 @@ async def user_may_create_room( # Check that the initial state received by callback contains the topic event. self.assertTrue( any( - event[0][0] == "m.room.topic" and event[1].get("topic") == "foo" + event.get("type") == "m.room.topic" + and event.get("content").get("topic") == "foo" for event in self.last_room_config["initial_state"] ) ) From 9f2539c4aea8f45d1e808dc4070d5ed13e124e3c Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 23 Jul 2025 15:32:35 +0100 Subject: [PATCH 02/19] Changelog --- changelog.d/18721.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18721.bugfix diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix new file mode 100644 index 00000000000..b369c9c3224 --- /dev/null +++ b/changelog.d/18721.bugfix @@ -0,0 +1 @@ +Fix structure of initial_state when calling user_may_create_room module API callback. From 99f5de089123cf6775af31e6888c7e2c0b5380d7 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 25 Jul 2025 17:00:11 +0100 Subject: [PATCH 03/19] Apply suggestions from code review Co-authored-by: Eric Eastwood --- changelog.d/18721.bugfix | 2 +- tests/module_api/test_spamchecker.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix index b369c9c3224..a67829bdbd9 100644 --- a/changelog.d/18721.bugfix +++ b/changelog.d/18721.bugfix @@ -1 +1 @@ -Fix structure of initial_state when calling user_may_create_room module API callback. +Fix `room_config` `initial_state` being mangled when the `user_may_create_room` spam-checker callback was called for room upgrades. diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 8ac70ebe7f9..7e7d61fd45a 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -72,8 +72,8 @@ async def user_may_create_room( self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_config["foo"], "baa") - def test_may_user_create_room_with_initial_state(self) -> None: - """Test that the may_user_create_room callback is called when a user + def test_user_may_create_room_with_initial_state(self) -> None: + """Test that the user_may_create_room callback is called when a user creates a room with some initial state events, and that it receives the correct parameters. """ @@ -142,8 +142,8 @@ async def user_may_create_room( # Check that the initial state received by callback contains the topic event. self.assertTrue( any( - event.get("type") == "m.room.topic" - and event.get("content").get("topic") == "foo" + event.get("type") == EventTypes.Topic + and event.get("content").get(EventContentFields.TOPIC) == "foo" for event in self.last_room_config["initial_state"] ) ) From 831773d8c1dcaf4bb0bf11dbebd2262d633aa18b Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 25 Jul 2025 17:01:32 +0100 Subject: [PATCH 04/19] More suggested changes from review --- tests/module_api/test_spamchecker.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 7e7d61fd45a..2554f4883f9 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -16,6 +16,7 @@ from twisted.test.proto_helpers import MemoryReactor +from synapse.api.constants import EventContentFields, EventTypes from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.rest import admin, login, room, room_upgrade_rest_servlet from synapse.server import HomeServer @@ -51,8 +52,8 @@ def create_room(self, content: JsonDict) -> FakeChannel: return channel - def test_may_user_create_room(self) -> None: - """Test that the may_user_create_room callback is called when a user + def test_user_may_create_room(self) -> None: + """Test that the user_may_create_room callback is called when a user creates a room, and that it receives the correct parameters. """ @@ -92,7 +93,10 @@ async def user_may_create_room( { "foo": "baa", "initial_state": [ - {"type": "m.room.topic", "content": {"topic": "foo"}} + { + "type": EventTypes.Topic, + "content": {EventContentFields.TOPIC: "foo"}, + } ], } ) @@ -101,17 +105,17 @@ async def user_may_create_room( self.assertEqual(self.last_room_config["foo"], "baa") self.assertTrue( any( - event.get("type") == "m.room.topic" - and event.get("content").get("topic") == "foo" + event.get("type") == EventTypes.Topic + and event.get("content").get(EventContentFields.TOPIC) == "foo" for event in self.last_room_config["initial_state"] ) ) - def test_may_user_create_room_on_upgrade(self) -> None: - """Test that the may_user_create_room callback is called when a room is upgraded.""" + def test_user_may_create_room_on_upgrade(self) -> None: + """Test that the user_may_create_room callback is called when a room is upgraded.""" # First, create a room to upgrade. - channel = self.create_room({"topic": "foo"}) + channel = self.create_room({EventContentFields.TOPIC: "foo"}) self.assertEqual(channel.code, 200) room_id = channel.json_body["room_id"] @@ -148,8 +152,8 @@ async def user_may_create_room( ) ) - def test_may_user_create_room_disallowed(self) -> None: - """Test that the codes response from may_user_create_room callback is respected + def test_user_may_create_room_disallowed(self) -> None: + """Test that the codes response from user_may_create_room callback is respected and returned via the API. """ @@ -170,8 +174,8 @@ async def user_may_create_room( self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_config["foo"], "baa") - def test_may_user_create_room_compatibility(self) -> None: - """Test that the may_user_create_room callback is called when a user + def test_user_may_create_room_compatibility(self) -> None: + """Test that the user_may_create_room callback is called when a user creates a room for a module that uses the old callback signature (without the `room_config` parameter) """ From 137da57650634609579230a58e153040dafffb64 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 25 Jul 2025 17:10:41 +0100 Subject: [PATCH 05/19] More suggestions from code review --- synapse/handlers/room.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 8dad914774b..1bd18efb888 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -488,7 +488,7 @@ async def clone_existing_room( # If so, mark the new room as non-federatable as well creation_content[EventContentFields.FEDERATE] = False - initial_state = {} + initial_state: MutableStateMap = {} # Replicate relevant room events types_to_copy: List[Tuple[str, Optional[str]]] = [ @@ -587,8 +587,12 @@ async def clone_existing_room( { "creation_content": creation_content, "initial_state": [ - {"type": state[0][0], "state_key": state[0][1], "content": state[1]} - for state in initial_state.items() + { + "type": state_key[0], + "state_key": state_key[1], + "content": event_content, + } + for state_key, event_content in initial_state.items() ], }, ) From fa4278395e4ebe7aa66351dec048950ad6145b4a Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 25 Jul 2025 17:24:06 +0100 Subject: [PATCH 06/19] More suggestions from review --- tests/module_api/test_spamchecker.py | 44 +++++++++++++--------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 2554f4883f9..7c2817c0a50 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -68,10 +68,12 @@ async def user_may_create_room( user_may_create_room=user_may_create_room ) - channel = self.create_room({"foo": "baa"}) + expected_room_config = {"foo": "baa"} + + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) - self.assertEqual(self.last_room_config["foo"], "baa") + self.assertEqual(self.last_room_config, expected_room_config) def test_user_may_create_room_with_initial_state(self) -> None: """Test that the user_may_create_room callback is called when a user @@ -89,27 +91,20 @@ async def user_may_create_room( user_may_create_room=user_may_create_room ) - channel = self.create_room( - { - "foo": "baa", - "initial_state": [ - { - "type": EventTypes.Topic, - "content": {EventContentFields.TOPIC: "foo"}, - } - ], - } - ) + expected_room_config = { + "foo": "baa", + "initial_state": [ + { + "type": EventTypes.Topic, + "content": {EventContentFields.TOPIC: "foo"}, + } + ], + } + + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) - self.assertEqual(self.last_room_config["foo"], "baa") - self.assertTrue( - any( - event.get("type") == EventTypes.Topic - and event.get("content").get(EventContentFields.TOPIC) == "foo" - for event in self.last_room_config["initial_state"] - ) - ) + self.assertEqual(self.last_room_config, expected_room_config) def test_user_may_create_room_on_upgrade(self) -> None: """Test that the user_may_create_room callback is called when a room is upgraded.""" @@ -147,6 +142,7 @@ async def user_may_create_room( self.assertTrue( any( event.get("type") == EventTypes.Topic + and event.get("state_key") == "" and event.get("content").get(EventContentFields.TOPIC) == "foo" for event in self.last_room_config["initial_state"] ) @@ -168,11 +164,13 @@ async def user_may_create_room( user_may_create_room=user_may_create_room ) - channel = self.create_room({"foo": "baa"}) + expected_room_config = {"foo": "baa"} + + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.UNAUTHORIZED) self.assertEqual(self.last_user_id, self.user_id) - self.assertEqual(self.last_room_config["foo"], "baa") + self.assertEqual(self.last_room_config, expected_room_config) def test_user_may_create_room_compatibility(self) -> None: """Test that the user_may_create_room callback is called when a user From 920301674c894f45cad9fa50ccdcbdaa9829853a Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Mon, 28 Jul 2025 15:21:03 +0100 Subject: [PATCH 07/19] Pass `room_config` = `None` for a room upgrade and clarify documentation --- changelog.d/18721.bugfix | 2 +- docs/modules/spam_checker_callbacks.md | 6 +++--- synapse/handlers/room.py | 16 ++------------ .../callbacks/spamchecker_callbacks.py | 6 +++--- tests/module_api/test_spamchecker.py | 21 +++++++------------ 5 files changed, 16 insertions(+), 35 deletions(-) diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix index a67829bdbd9..adfaacfdb6d 100644 --- a/changelog.d/18721.bugfix +++ b/changelog.d/18721.bugfix @@ -1 +1 @@ -Fix `room_config` `initial_state` being mangled when the `user_may_create_room` spam-checker callback was called for room upgrades. +Set `room_config` param to `None` when the `user_may_create_room` spam-checker callback was called for room upgrades. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 49b7e06bb3f..d940e172af1 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -192,15 +192,15 @@ _Changed in Synapse v1.62.0: `synapse.module_api.NOT_SPAM` and `synapse.module_a _Changed in Synapse v1.132.0: Added the `room_config` argument. Callbacks that only expect a single `user_id` argument are still supported._ ```python -async def user_may_create_room(user_id: str, room_config: synapse.module_api.JsonDict) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_create_room(user_id: str, room_config: Optional[synapse.module_api.JsonDict]) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` -Called when processing a room creation request. +Called when processing a room creation or room upgrade request. The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). -* `room_config`: The contents of the body of a [/createRoom request](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3createroom) as a dictionary. +* `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3createroom) as a dictionary. In the case of a room upgrade this is `None`. The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 1bd18efb888..8cdb2c7250e 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -579,22 +579,10 @@ async def clone_existing_room( if current_power_level_int < needed_power_level: user_power_levels[user_id] = needed_power_level - # We construct what the body of a call to /createRoom would look like for passing - # to the spam checker. We don't include a preset here, as we expect the - # initial state to contain everything we need. + # Note that we don't attempt to pass the room config as this is an upgrade spam_check = await self._spam_checker_module_callbacks.user_may_create_room( user_id, - { - "creation_content": creation_content, - "initial_state": [ - { - "type": state_key[0], - "state_key": state_key[1], - "content": event_content, - } - for state_key, event_content in initial_state.items() - ], - }, + None, ) if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: raise SynapseError( diff --git a/synapse/module_api/callbacks/spamchecker_callbacks.py b/synapse/module_api/callbacks/spamchecker_callbacks.py index 428e733979c..2184ddb02a2 100644 --- a/synapse/module_api/callbacks/spamchecker_callbacks.py +++ b/synapse/module_api/callbacks/spamchecker_callbacks.py @@ -150,7 +150,7 @@ ] USER_MAY_CREATE_ROOM_CALLBACK = Union[ Callable[ - [str, JsonDict], + [str, Optional[JsonDict]], Awaitable[USER_MAY_CREATE_ROOM_CALLBACK_RETURN_VALUE], ], Callable[ # Single argument variant for backwards compatibility @@ -738,7 +738,7 @@ async def user_may_send_3pid_invite( return self.NOT_SPAM async def user_may_create_room( - self, userid: str, room_config: JsonDict + self, userid: str, room_config: Optional[JsonDict] ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room @@ -758,7 +758,7 @@ async def user_may_create_room( if len(checker_args.parameters) == 2: callback_with_requester_id = cast( Callable[ - [str, JsonDict], + [str, Optional[JsonDict]], Awaitable[USER_MAY_CREATE_ROOM_CALLBACK_RETURN_VALUE], ], callback, diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 7c2817c0a50..79a401a6039 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -12,7 +12,7 @@ # . # # -from typing import Literal, Union +from typing import Literal, Optional, Union from twisted.test.proto_helpers import MemoryReactor @@ -58,7 +58,7 @@ def test_user_may_create_room(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: JsonDict + user_id: str, room_config: Optional[JsonDict] ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -81,7 +81,7 @@ def test_user_may_create_room_with_initial_state(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: JsonDict + user_id: str, room_config: Optional[JsonDict] ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -115,7 +115,7 @@ def test_user_may_create_room_on_upgrade(self) -> None: room_id = channel.json_body["room_id"] async def user_may_create_room( - user_id: str, room_config: JsonDict + user_id: str, room_config: Optional[JsonDict] ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -138,15 +138,8 @@ async def user_may_create_room( # Check that the callback was called and the room was upgraded. self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) - # Check that the initial state received by callback contains the topic event. - self.assertTrue( - any( - event.get("type") == EventTypes.Topic - and event.get("state_key") == "" - and event.get("content").get(EventContentFields.TOPIC) == "foo" - for event in self.last_room_config["initial_state"] - ) - ) + # Check that the room config is None. + self.assertIsNone(self.last_room_config) def test_user_may_create_room_disallowed(self) -> None: """Test that the codes response from user_may_create_room callback is respected @@ -154,7 +147,7 @@ def test_user_may_create_room_disallowed(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: JsonDict + user_id: str, room_config: Optional[JsonDict] ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id From 005a61f26dce0df5c8465bddab80bebde6e5c200 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Mon, 28 Jul 2025 15:37:09 +0100 Subject: [PATCH 08/19] Suggestions from review Re: https://github.com/element-hq/synapse/pull/18721#discussion_r2231795769 --- tests/module_api/test_spamchecker.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 79a401a6039..80924eecfc2 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -69,7 +69,6 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} - channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -100,7 +99,6 @@ async def user_may_create_room( } ], } - channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -134,7 +132,6 @@ async def user_may_create_room( content={"new_version": DEFAULT_ROOM_VERSION}, access_token=self.token, ) - # Check that the callback was called and the room was upgraded. self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -158,7 +155,6 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} - channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.UNAUTHORIZED) @@ -226,7 +222,6 @@ async def user_may_send_state_event( content={"foo": "bar"}, access_token=self.token, ) - self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_id, room_id) @@ -270,6 +265,5 @@ async def user_may_send_state_event( content={"foo": "bar"}, access_token=self.token, ) - self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.FORBIDDEN) From 2e115b7d0d382e3ff6e3fc656233272aba1b21e0 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Mon, 28 Jul 2025 16:03:22 +0100 Subject: [PATCH 09/19] Revert the logic back to how it was pre adding room_config https://github.com/element-hq/synapse/pull/18486 --- synapse/handlers/room.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 8cdb2c7250e..3c6610cbb01 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -473,6 +473,19 @@ async def clone_existing_room( """ user_id = requester.user.to_string() + # Note that we don't attempt to pass the room config as this is an upgrade + spam_check = await self._spam_checker_module_callbacks.user_may_create_room( + user_id, + None, + ) + if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: + raise SynapseError( + 403, + "You are not permitted to create rooms", + errcode=spam_check[0], + additional_fields=spam_check[1], + ) + creation_content: JsonDict = { "room_version": new_room_version.identifier, "predecessor": {"room_id": old_room_id, "event_id": tombstone_event_id}, @@ -579,19 +592,6 @@ async def clone_existing_room( if current_power_level_int < needed_power_level: user_power_levels[user_id] = needed_power_level - # Note that we don't attempt to pass the room config as this is an upgrade - spam_check = await self._spam_checker_module_callbacks.user_may_create_room( - user_id, - None, - ) - if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: - raise SynapseError( - 403, - "You are not permitted to create rooms", - errcode=spam_check[0], - additional_fields=spam_check[1], - ) - await self._send_events_for_new_room( requester, new_room_id, From 9c13a60b0da1109313afc20c2c451de95e1d2936 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 29 Aug 2025 13:46:41 +0100 Subject: [PATCH 10/19] Revert "Revert the logic back to how it was pre adding room_config" This reverts commit 2e115b7d0d382e3ff6e3fc656233272aba1b21e0. --- synapse/handlers/room.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 3c6610cbb01..8cdb2c7250e 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -473,19 +473,6 @@ async def clone_existing_room( """ user_id = requester.user.to_string() - # Note that we don't attempt to pass the room config as this is an upgrade - spam_check = await self._spam_checker_module_callbacks.user_may_create_room( - user_id, - None, - ) - if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: - raise SynapseError( - 403, - "You are not permitted to create rooms", - errcode=spam_check[0], - additional_fields=spam_check[1], - ) - creation_content: JsonDict = { "room_version": new_room_version.identifier, "predecessor": {"room_id": old_room_id, "event_id": tombstone_event_id}, @@ -592,6 +579,19 @@ async def clone_existing_room( if current_power_level_int < needed_power_level: user_power_levels[user_id] = needed_power_level + # Note that we don't attempt to pass the room config as this is an upgrade + spam_check = await self._spam_checker_module_callbacks.user_may_create_room( + user_id, + None, + ) + if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: + raise SynapseError( + 403, + "You are not permitted to create rooms", + errcode=spam_check[0], + additional_fields=spam_check[1], + ) + await self._send_events_for_new_room( requester, new_room_id, From 3d17a8854cd99ae65b2d7ac04e1a0f705f0c6985 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 29 Aug 2025 13:46:43 +0100 Subject: [PATCH 11/19] Revert "Suggestions from review" This reverts commit 005a61f26dce0df5c8465bddab80bebde6e5c200. --- tests/module_api/test_spamchecker.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 80924eecfc2..79a401a6039 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -69,6 +69,7 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -99,6 +100,7 @@ async def user_may_create_room( } ], } + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -132,6 +134,7 @@ async def user_may_create_room( content={"new_version": DEFAULT_ROOM_VERSION}, access_token=self.token, ) + # Check that the callback was called and the room was upgraded. self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -155,6 +158,7 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} + channel = self.create_room(expected_room_config) self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.UNAUTHORIZED) @@ -222,6 +226,7 @@ async def user_may_send_state_event( content={"foo": "bar"}, access_token=self.token, ) + self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_id, room_id) @@ -265,5 +270,6 @@ async def user_may_send_state_event( content={"foo": "bar"}, access_token=self.token, ) + self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.FORBIDDEN) From 44a0e76e278e4bdf59d5cb389e3c44fb862a5b03 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 29 Aug 2025 13:46:45 +0100 Subject: [PATCH 12/19] Revert "Pass `room_config` = `None` for a room upgrade and clarify documentation" This reverts commit 920301674c894f45cad9fa50ccdcbdaa9829853a. --- changelog.d/18721.bugfix | 2 +- docs/modules/spam_checker_callbacks.md | 6 +++--- synapse/handlers/room.py | 16 ++++++++++++-- .../callbacks/spamchecker_callbacks.py | 6 +++--- tests/module_api/test_spamchecker.py | 21 ++++++++++++------- 5 files changed, 35 insertions(+), 16 deletions(-) diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix index adfaacfdb6d..a67829bdbd9 100644 --- a/changelog.d/18721.bugfix +++ b/changelog.d/18721.bugfix @@ -1 +1 @@ -Set `room_config` param to `None` when the `user_may_create_room` spam-checker callback was called for room upgrades. +Fix `room_config` `initial_state` being mangled when the `user_may_create_room` spam-checker callback was called for room upgrades. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index d940e172af1..49b7e06bb3f 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -192,15 +192,15 @@ _Changed in Synapse v1.62.0: `synapse.module_api.NOT_SPAM` and `synapse.module_a _Changed in Synapse v1.132.0: Added the `room_config` argument. Callbacks that only expect a single `user_id` argument are still supported._ ```python -async def user_may_create_room(user_id: str, room_config: Optional[synapse.module_api.JsonDict]) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] +async def user_may_create_room(user_id: str, room_config: synapse.module_api.JsonDict) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` -Called when processing a room creation or room upgrade request. +Called when processing a room creation request. The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). -* `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3createroom) as a dictionary. In the case of a room upgrade this is `None`. +* `room_config`: The contents of the body of a [/createRoom request](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3createroom) as a dictionary. The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 8cdb2c7250e..1bd18efb888 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -579,10 +579,22 @@ async def clone_existing_room( if current_power_level_int < needed_power_level: user_power_levels[user_id] = needed_power_level - # Note that we don't attempt to pass the room config as this is an upgrade + # We construct what the body of a call to /createRoom would look like for passing + # to the spam checker. We don't include a preset here, as we expect the + # initial state to contain everything we need. spam_check = await self._spam_checker_module_callbacks.user_may_create_room( user_id, - None, + { + "creation_content": creation_content, + "initial_state": [ + { + "type": state_key[0], + "state_key": state_key[1], + "content": event_content, + } + for state_key, event_content in initial_state.items() + ], + }, ) if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: raise SynapseError( diff --git a/synapse/module_api/callbacks/spamchecker_callbacks.py b/synapse/module_api/callbacks/spamchecker_callbacks.py index 2184ddb02a2..428e733979c 100644 --- a/synapse/module_api/callbacks/spamchecker_callbacks.py +++ b/synapse/module_api/callbacks/spamchecker_callbacks.py @@ -150,7 +150,7 @@ ] USER_MAY_CREATE_ROOM_CALLBACK = Union[ Callable[ - [str, Optional[JsonDict]], + [str, JsonDict], Awaitable[USER_MAY_CREATE_ROOM_CALLBACK_RETURN_VALUE], ], Callable[ # Single argument variant for backwards compatibility @@ -738,7 +738,7 @@ async def user_may_send_3pid_invite( return self.NOT_SPAM async def user_may_create_room( - self, userid: str, room_config: Optional[JsonDict] + self, userid: str, room_config: JsonDict ) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]: """Checks if a given user may create a room @@ -758,7 +758,7 @@ async def user_may_create_room( if len(checker_args.parameters) == 2: callback_with_requester_id = cast( Callable[ - [str, Optional[JsonDict]], + [str, JsonDict], Awaitable[USER_MAY_CREATE_ROOM_CALLBACK_RETURN_VALUE], ], callback, diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 79a401a6039..7c2817c0a50 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -12,7 +12,7 @@ # . # # -from typing import Literal, Optional, Union +from typing import Literal, Union from twisted.test.proto_helpers import MemoryReactor @@ -58,7 +58,7 @@ def test_user_may_create_room(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: Optional[JsonDict] + user_id: str, room_config: JsonDict ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -81,7 +81,7 @@ def test_user_may_create_room_with_initial_state(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: Optional[JsonDict] + user_id: str, room_config: JsonDict ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -115,7 +115,7 @@ def test_user_may_create_room_on_upgrade(self) -> None: room_id = channel.json_body["room_id"] async def user_may_create_room( - user_id: str, room_config: Optional[JsonDict] + user_id: str, room_config: JsonDict ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id @@ -138,8 +138,15 @@ async def user_may_create_room( # Check that the callback was called and the room was upgraded. self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) - # Check that the room config is None. - self.assertIsNone(self.last_room_config) + # Check that the initial state received by callback contains the topic event. + self.assertTrue( + any( + event.get("type") == EventTypes.Topic + and event.get("state_key") == "" + and event.get("content").get(EventContentFields.TOPIC) == "foo" + for event in self.last_room_config["initial_state"] + ) + ) def test_user_may_create_room_disallowed(self) -> None: """Test that the codes response from user_may_create_room callback is respected @@ -147,7 +154,7 @@ def test_user_may_create_room_disallowed(self) -> None: """ async def user_may_create_room( - user_id: str, room_config: Optional[JsonDict] + user_id: str, room_config: JsonDict ) -> Union[Literal["NOT_SPAM"], Codes]: self.last_room_config = room_config self.last_user_id = user_id From 8693873952186634b9a1a2c22a99dbd63233bcca Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 29 Aug 2025 14:20:09 +0100 Subject: [PATCH 13/19] Update documentation --- changelog.d/18721.bugfix | 2 +- docs/modules/spam_checker_callbacks.md | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix index a67829bdbd9..0aa0b3962d6 100644 --- a/changelog.d/18721.bugfix +++ b/changelog.d/18721.bugfix @@ -1 +1 @@ -Fix `room_config` `initial_state` being mangled when the `user_may_create_room` spam-checker callback was called for room upgrades. +Fix room upgrade `room_config` argument and documentation for `user_may_create_room` spam-checker callback. diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 49b7e06bb3f..320a4e15e21 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -195,12 +195,13 @@ _Changed in Synapse v1.132.0: Added the `room_config` argument. Callbacks that o async def user_may_create_room(user_id: str, room_config: synapse.module_api.JsonDict) -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", bool] ``` -Called when processing a room creation request. +Called when processing a room creation or room upgrade request. The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). -* `room_config`: The contents of the body of a [/createRoom request](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3createroom) as a dictionary. +* `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3createroom) as a dictionary. + For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent /createRoom request would have looked like. Specifically it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a subset of the state of the previous room). The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still From a165f99964aa170b18b43f99db15c7cf6f781317 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 17 Sep 2025 20:38:26 +0100 Subject: [PATCH 14/19] Another attempt at line spacing in tests Try again to follow @MadLittleMods's instructions: > expected_room_config = { and self.create_room should be next to each other with a newline after that grouping. If this is still wrong then please point to an example of it correctly (I see inconsitencies when I look at other tests in the codebase) --- tests/module_api/test_spamchecker.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 63c81567464..51cb99e84b7 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -69,8 +69,8 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} - channel = self.create_room(expected_room_config) + self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_config, expected_room_config) @@ -100,8 +100,8 @@ async def user_may_create_room( } ], } - channel = self.create_room(expected_room_config) + self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) self.assertEqual(self.last_room_config, expected_room_config) @@ -111,6 +111,7 @@ def test_user_may_create_room_on_upgrade(self) -> None: # First, create a room to upgrade. channel = self.create_room({EventContentFields.TOPIC: "foo"}) + self.assertEqual(channel.code, 200) room_id = channel.json_body["room_id"] @@ -165,8 +166,8 @@ async def user_may_create_room( ) expected_room_config = {"foo": "baa"} - channel = self.create_room(expected_room_config) + self.assertEqual(channel.code, 403) self.assertEqual(channel.json_body["errcode"], Codes.UNAUTHORIZED) self.assertEqual(self.last_user_id, self.user_id) @@ -189,6 +190,7 @@ async def user_may_create_room( ) channel = self.create_room({"foo": "baa"}) + self.assertEqual(channel.code, 200) self.assertEqual(self.last_user_id, self.user_id) @@ -216,6 +218,7 @@ async def user_may_send_state_event( ) channel = self.create_room({}) + self.assertEqual(channel.code, 200) room_id = channel.json_body["room_id"] @@ -260,6 +263,7 @@ async def user_may_send_state_event( ) channel = self.create_room({}) + self.assertEqual(channel.code, 200) room_id = channel.json_body["room_id"] From dcc5ce3ac2210381afb30b8d658f27e624e304b4 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 17 Sep 2025 20:39:42 +0100 Subject: [PATCH 15/19] Update docs/modules/spam_checker_callbacks.md Co-authored-by: Eric Eastwood --- docs/modules/spam_checker_callbacks.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 320a4e15e21..42b4aa4fe18 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -201,7 +201,7 @@ The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). * `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3createroom) as a dictionary. - For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent /createRoom request would have looked like. Specifically it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a subset of the state of the previous room). + For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent /createRoom request would have looked like. Specifically, it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a subset of the state of the previous room). The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still From 91beccaa2dcb6ea3e76ed0ebcbb0bfb8de2e3f6e Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 17 Sep 2025 20:40:24 +0100 Subject: [PATCH 16/19] Linewrap --- docs/modules/spam_checker_callbacks.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index 42b4aa4fe18..f6dba2ab931 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -201,7 +201,9 @@ The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). * `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3createroom) as a dictionary. - For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent /createRoom request would have looked like. Specifically, it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a subset of the state of the previous room). + For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent + /createRoom request would have looked like. Specifically, it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a + subset of the state of the previous room). The callback must return one of: - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still From 14895d386cbcc586fe0cb15a58607276f5d05087 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 24 Sep 2025 18:35:22 +0100 Subject: [PATCH 17/19] Apply suggestions from code review Co-authored-by: Eric Eastwood --- docs/modules/spam_checker_callbacks.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md index f6dba2ab931..0f15a9dcc55 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -200,9 +200,9 @@ Called when processing a room creation or room upgrade request. The arguments passed to this callback are: * `user_id`: The Matrix user ID of the user (e.g. `@alice:example.com`). -* `room_config`: The contents of the body of the [/createRoom request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3createroom) as a dictionary. +* `room_config`: The contents of the body of the [`/createRoom` request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3createroom) as a dictionary. For a [room upgrade request](https://spec.matrix.org/v1.15/client-server-api/#post_matrixclientv3roomsroomidupgrade) it is a synthesised subset of what an equivalent - /createRoom request would have looked like. Specifically, it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a + `/createRoom` request would have looked like. Specifically, it contains the `creation_content` (linking to the previous room) and `initial_state` (containing a subset of the state of the previous room). The callback must return one of: From 1f9b3e3637733f40715b1363912c9110c4b7bfd1 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 24 Sep 2025 18:41:57 +0100 Subject: [PATCH 18/19] Improve comments for spam checker logic --- synapse/handlers/room.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index aebf9bde81f..51ec9e36b09 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -693,9 +693,10 @@ async def clone_existing_room( additional_creators, ) - # We construct what the body of a call to /createRoom would look like for passing - # to the spam checker. We don't include a preset here, as we expect the + # We construct a subset of what the body of a call to /createRoom would look like + # for passing to the spam checker. We don't include a preset here, as we expect the # initial state to contain everything we need. + # TODO: given we are upgrading, it would make sense to pass the room_version spam_check = await self._spam_checker_module_callbacks.user_may_create_room( user_id, { From 4c6942ce49376163a1c96b5636d444e937cb6269 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Wed, 24 Sep 2025 18:44:44 +0100 Subject: [PATCH 19/19] Add TODO comments regarding preset for spam checker. --- synapse/handlers/room.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 51ec9e36b09..db6dc5efd04 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -697,6 +697,7 @@ async def clone_existing_room( # for passing to the spam checker. We don't include a preset here, as we expect the # initial state to contain everything we need. # TODO: given we are upgrading, it would make sense to pass the room_version + # TODO: the preset might be useful too spam_check = await self._spam_checker_module_callbacks.user_may_create_room( user_id, {