Do not mutate power levels on upgrade to v12 room (#19727)

When upgrading a room to v12, we accidentally ended up mutating the
content of the old power level. Since we cache events, this meant any
future usage of the old power level event would see the wrong content
(until it dropped from the cache).

This meant that the creator of the new room would not be able to perform
admin actions on the old room. Any federation requests for the event
would fail the hash checks, since the content had been changed.

All in all, quite a nasty bug.

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.

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.
 

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)