diff --git a/changelog.d/18721.bugfix b/changelog.d/18721.bugfix new file mode 100644 index 00000000000..0aa0b3962d6 --- /dev/null +++ b/changelog.d/18721.bugfix @@ -0,0 +1 @@ +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..0f15a9dcc55 100644 --- a/docs/modules/spam_checker_callbacks.md +++ b/docs/modules/spam_checker_callbacks.md @@ -195,12 +195,15 @@ _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 diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 47bd139ca71..db6dc5efd04 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -597,7 +597,7 @@ async def clone_existing_room( new_room_version, additional_creators=additional_creators, ) - initial_state = {} + initial_state: MutableStateMap = {} # Replicate relevant room events types_to_copy: List[Tuple[str, Optional[str]]] = [ @@ -693,14 +693,23 @@ 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 + # TODO: the preset might be useful too spam_check = await self._spam_checker_module_callbacks.user_may_create_room( user_id, { "creation_content": creation_content, - "initial_state": list(initial_state.items()), + "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: diff --git a/tests/module_api/test_spamchecker.py b/tests/module_api/test_spamchecker.py index 3f4d1d9d5f0..d461d6cea29 100644 --- a/tests/module_api/test_spamchecker.py +++ b/tests/module_api/test_spamchecker.py @@ -16,6 +16,7 @@ from twisted.internet.testing 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. """ @@ -67,16 +68,50 @@ 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_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_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. + """ + + 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 + ) + + 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, 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.""" # 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"] @@ -107,13 +142,15 @@ 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") == 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_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. """ @@ -128,14 +165,16 @@ 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_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) """ @@ -151,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) @@ -178,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"] @@ -222,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"]