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.

Author: erikjohnston

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 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.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 @@ async def _update_upgraded_room_pls(
             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 @@ async def _update_upgraded_room_pls(
                 "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 @@ async def clone_existing_room(
 
         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,19 +1835,29 @@ def _room_preset_config(self, room_config: JsonDict) -> tuple[str, dict]:
             )
         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)
         for creator in creators:
             # 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 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 @@ def _upgrade_room(
         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 @@ def _upgrade_room(
             "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 @@ def test_bans(self) -> None:
                 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)