diff --git a/changelog.d/19727.bugfix b/changelog.d/19727.bugfix new file mode 100644 index 0000000000..a535bd6aa4 --- /dev/null +++ b/changelog.d/19727.bugfix @@ -0,0 +1 @@ +Fix a bug where when upgrading a room to v12 the power level event in the old room got mutated to remove the user upgrading the room's power. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index f110be0a2f..6a5c76c667 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -27,6 +27,7 @@ import math import random import string from collections import OrderedDict +from collections.abc import Mapping from http import HTTPStatus from typing import ( TYPE_CHECKING, @@ -67,7 +68,11 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion from synapse.event_auth import validate_event_for_room_version from synapse.events import EventBase, event_exists_in_state_dag from synapse.events.snapshot import UnpersistedEventContext -from synapse.events.utils import FilteredEvent, copy_and_fixup_power_levels_contents +from synapse.events.utils import ( + FilteredEvent, + PowerLevelsContent, + copy_and_fixup_power_levels_contents, +) from synapse.handlers.relations import BundledAggregations from synapse.rest.admin._base import assert_user_is_admin from synapse.streams import EventSource @@ -500,10 +505,12 @@ class RoomCreationHandler: except AuthError as e: logger.warning("Unable to update PLs in old room: %s", e) + power_levels_content: JsonMapping = old_room_pl_state.content + new_room_version = await self.store.get_room_version(new_room_id) if new_room_version.msc4289_creator_power_enabled: - self._remove_creators_from_pl_users_map( - old_room_pl_state.content.get("users", {}), + power_levels_content = self._copy_and_remove_creators_from_pl_users_map( + power_levels_content, requester.user.to_string(), additional_creators, ) @@ -515,9 +522,7 @@ class RoomCreationHandler: "state_key": "", "room_id": new_room_id, "sender": requester.user.to_string(), - "content": copy_and_fixup_power_levels_contents( - old_room_pl_state.content - ), + "content": copy_and_fixup_power_levels_contents(power_levels_content), }, ratelimit=False, ) @@ -686,11 +691,12 @@ class RoomCreationHandler: if new_room_version.msc4289_creator_power_enabled: # the creator(s) cannot be in the users map - self._remove_creators_from_pl_users_map( - user_power_levels, + fixed_power_levels = self._copy_and_remove_creators_from_pl_users_map( + power_levels, user_id, additional_creators, ) + initial_state[(EventTypes.PowerLevels, "")] = fixed_power_levels # 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 @@ -1829,12 +1835,19 @@ class RoomCreationHandler: ) return preset_name, preset_config - def _remove_creators_from_pl_users_map( + def _copy_and_remove_creators_from_pl_users_map( self, - users_map: dict[str, int], + power_levels_content: PowerLevelsContent, creator: str, additional_creators: list[str] | None, - ) -> None: + ) -> PowerLevelsContent: + users_map = power_levels_content.get("users", {}) + if not users_map: + return power_levels_content + + assert isinstance(users_map, Mapping) + users_map = dict(users_map) + creators = [creator] if additional_creators: creators.extend(additional_creators) @@ -1842,6 +1855,9 @@ class RoomCreationHandler: # the creator(s) cannot be in the users map users_map.pop(creator, None) + power_levels_content = {**power_levels_content, "users": users_map} + return power_levels_content + def _generate_room_id(self) -> str: """Generates a random room ID. diff --git a/tests/rest/client/test_upgrade_room.py b/tests/rest/client/test_upgrade_room.py index ee26492909..6cb85c94c4 100644 --- a/tests/rest/client/test_upgrade_room.py +++ b/tests/rest/client/test_upgrade_room.py @@ -23,6 +23,7 @@ from unittest.mock import patch from twisted.internet.testing import MemoryReactor from synapse.api.constants import EventContentFields, EventTypes, Membership, RoomTypes +from synapse.api.room_versions import RoomVersions from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.rest import admin from synapse.rest.client import login, room, room_upgrade_rest_servlet @@ -58,6 +59,7 @@ class UpgradeRoomTest(unittest.HomeserverTestCase): token: str | None = None, room_id: str | None = None, expire_cache: bool = True, + new_version: str = DEFAULT_ROOM_VERSION, ) -> FakeChannel: if expire_cache: # We don't want a cached response. @@ -70,7 +72,7 @@ class UpgradeRoomTest(unittest.HomeserverTestCase): "POST", f"/_matrix/client/r0/rooms/{room_id}/upgrade", # This will upgrade a room to the same version, but that's fine. - content={"new_version": DEFAULT_ROOM_VERSION}, + content={"new_version": new_version}, access_token=token or self.creator_token, ) @@ -431,3 +433,51 @@ class UpgradeRoomTest(unittest.HomeserverTestCase): tok=self.creator_token, ) self.assertEqual(content[EventContentFields.MEMBERSHIP], Membership.BAN) + + def test_creator_removed_from_powerlevels_v12(self) -> None: + """ + Test that the creator is removed from the power levels users map when + upgrading to a room version with MSC4289. + """ + # Create a room on room version 11, which doesn't have MSC4289. + room_id = self.helper.create_room_as( + self.creator, tok=self.creator_token, room_version="11" + ) + self.helper.join(room_id, self.other, tok=self.other_token) + + # Retrieve the room's current power levels. + old_power_level_event = self.get_success( + self.hs.get_storage_controllers().state.get_current_state_event( + room_id, "m.room.power_levels", "" + ) + ) + assert old_power_level_event is not None + + # The creator should be in the users map with power level 100. + self.assertEqual(old_power_level_event.content["users"][self.creator], 100) + + # Upgrade the room to version 12, which has MSC4289. + channel = self._upgrade_room( + room_id=room_id, new_version=RoomVersions.V12.identifier + ) + self.assertEqual(200, channel.code, channel.result) + + # Extract the new room ID. + new_room_id = channel.json_body["replacement_room"] + + # Fetch the new room's power level event. + new_power_levels = self.helper.get_state( + new_room_id, + "m.room.power_levels", + tok=self.creator_token, + ) + + # The creator should no longer be in the users map. + self.assertNotIn(self.creator, new_power_levels["users"]) + + # The creator should still be in the old power levels event with power + # level 100. + # + # This is a regression test where previously Synapse would accidentally + # mutate the old power levels event. + self.assertEqual(old_power_level_event.content["users"][self.creator], 100)