Fix room upgrades creating an empty room when auth fails

changelog.d/12696.bugfix

@@ -0,0 +1 @@
+Fix a long-standing bug where an empty room would be created when a user with an insufficient power level tried to upgrade a room.

synapse/events/builder.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Any, Collection, Dict, List, Optional, Tuple, Union
 
 import attr
 from signedjson.types import SigningKey
@@ -101,8 +101,8 @@ def is_state(self) -> bool:
 
     async def build(
         self,
-        prev_event_ids: List[str],
-        auth_event_ids: Optional[List[str]],
+        prev_event_ids: Collection[str],
+        auth_event_ids: Optional[Collection[str]],
         depth: Optional[int] = None,
     ) -> EventBase:
         """Transform into a fully signed and hashed event
@@ -135,8 +135,8 @@ async def build(
             auth_events = await self._store.add_event_hashes(auth_event_ids)
             prev_events = await self._store.add_event_hashes(prev_event_ids)
         else:
-            auth_events = auth_event_ids
-            prev_events = prev_event_ids
+            auth_events = list(auth_event_ids)
+            prev_events = list(prev_event_ids)
 
         # Otherwise, progress the depth as normal
         if depth is None:

synapse/handlers/message.py

@@ -17,7 +17,7 @@
 import logging
 import random
 from http import HTTPStatus
-from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional, Tuple
+from typing import TYPE_CHECKING, Any, Collection, Dict, List, Mapping, Optional, Tuple
 
 from canonicaljson import encode_canonical_json
 
@@ -487,9 +487,9 @@ async def create_event(
         event_dict: dict,
         txn_id: Optional[str] = None,
         allow_no_prev_events: bool = False,
-        prev_event_ids: Optional[List[str]] = None,
-        auth_event_ids: Optional[List[str]] = None,
-        state_event_ids: Optional[List[str]] = None,
+        prev_event_ids: Optional[Collection[str]] = None,
+        auth_event_ids: Optional[Collection[str]] = None,
+        state_event_ids: Optional[Collection[str]] = None,
         require_consent: bool = True,
         outlier: bool = False,
         historical: bool = False,
@@ -901,9 +901,9 @@ async def create_new_client_event(
         builder: EventBuilder,
         requester: Optional[Requester] = None,
         allow_no_prev_events: bool = False,
-        prev_event_ids: Optional[List[str]] = None,
-        auth_event_ids: Optional[List[str]] = None,
-        state_event_ids: Optional[List[str]] = None,
+        prev_event_ids: Optional[Collection[str]] = None,
+        auth_event_ids: Optional[Collection[str]] = None,
+        state_event_ids: Optional[Collection[str]] = None,
         depth: Optional[int] = None,
     ) -> Tuple[EventBase, EventContext]:
         """Create a new event for a local client

synapse/handlers/room.py

@@ -33,6 +33,7 @@
 import attr
 from typing_extensions import TypedDict
 
+import synapse.events.snapshot
 from synapse.api.constants import (
     EventContentFields,
     EventTypes,
@@ -196,6 +197,18 @@ async def upgrade_room(
                     400, "An upgrade for this room is currently in progress"
                 )
 
+        # Before starting the room upgrade, check whether the user has the power level
+        # to carry out the upgrade, by creating a dummy tombstone event. The auth checks
+        # will check for room membership and the required power level to send the
+        # tombstone. We reuse the prev events and auth events from the dummy event for
+        # the real tombstone. That way if the dummy event passes the auth checks, the
+        # real tombstone should pass auth checks too.
+        tombstone_event, _ = await self._create_tombstone_and_check_auth(
+            requester,
+            old_room_id,
+            new_room_id=None,
+        )
+
         # Upgrade the room
         #
         # If this user has sent multiple upgrade requests for the same room
@@ -207,18 +220,26 @@ async def upgrade_room(
             requester,
             old_room_id,
             new_version,  # args for _upgrade_room
+            tombstone_auth_template=tombstone_event,
         )
 
         return ret
 
     async def _upgrade_room(
-        self, requester: Requester, old_room_id: str, new_version: RoomVersion
+        self,
+        requester: Requester,
+        old_room_id: str,
+        new_version: RoomVersion,
+        tombstone_auth_template: EventBase,
     ) -> str:
         """
         Args:
             requester: the user requesting the upgrade
             old_room_id: the id of the room to be replaced
             new_versions: the version to upgrade the room to
+            tombstone_auth_template: a template for the tombstone event to send to the
+                old room. `tombstone_auth_template`'s prev and auth events will be used
+                to create the tombstone event.
 
         Raises:
             ShadowBanError if the requester is shadow-banned.
@@ -238,28 +259,14 @@ async def _upgrade_room(
 
         logger.info("Creating new room %s to replace %s", new_room_id, old_room_id)
 
-        # we create and auth the tombstone event before properly creating the new
-        # room, to check our user has perms in the old room.
         (
             tombstone_event,
             tombstone_context,
-        ) = await self.event_creation_handler.create_event(
+        ) = await self._create_tombstone_and_check_auth(
             requester,
-            {
-                "type": EventTypes.Tombstone,
-                "state_key": "",
-                "room_id": old_room_id,
-                "sender": user_id,
-                "content": {
-                    "body": "This room has been replaced",
-                    "replacement_room": new_room_id,
-                },
-            },
-        )
-        old_room_version = await self.store.get_room_version(old_room_id)
-        validate_event_for_room_version(old_room_version, tombstone_event)
-        await self._event_auth_handler.check_auth_rules_from_context(
-            old_room_version, tombstone_event, tombstone_context
+            old_room_id,
+            new_room_id,
+            copy_auth_from_event=tombstone_auth_template,
         )
 
         await self.clone_existing_room(
@@ -303,6 +310,61 @@ async def _upgrade_room(
 
         return new_room_id
 
+    async def _create_tombstone_and_check_auth(
+        self,
+        requester: Requester,
+        old_room_id: str,
+        new_room_id: Optional[str],
+        copy_auth_from_event: Optional[EventBase] = None,
+    ) -> Tuple[EventBase, synapse.events.snapshot.EventContext]:
+        """Creates a tombstone event for a room upgrade and checks that it can be sent.
+
+        Args:
+            requester: the user requesting the upgrade
+            old_room_id: the id of the room to be replaced
+            new_room_id: the id of the replacement room, or `None` if not known yet
+            copy_auth_from_event: an event whose prev and auth events are to be used for
+                the tombstone event, if provided
+
+        Returns:
+            A tuple containing the tombstone event and its context.
+
+        Raises:
+            AuthError if the requester cannot send the tombstone event, eg. if they are
+                not in the room or do not have the required power level.
+        """
+        user_id = requester.user.to_string()
+
+        (
+            tombstone_event,
+            tombstone_context,
+        ) = await self.event_creation_handler.create_event(
+            requester,
+            {
+                "type": EventTypes.Tombstone,
+                "state_key": "",
+                "room_id": old_room_id,
+                "sender": user_id,
+                "content": {
+                    "body": "This room has been replaced",
+                    "replacement_room": new_room_id,
+                },
+            },
+            prev_event_ids=(
+                copy_auth_from_event.prev_event_ids() if copy_auth_from_event else None
+            ),
+            auth_event_ids=(
+                copy_auth_from_event.auth_event_ids() if copy_auth_from_event else None
+            ),
+        )
+        old_room_version = await self.store.get_room_version(old_room_id)
+        validate_event_for_room_version(old_room_version, tombstone_event)
+        await self._event_auth_handler.check_auth_rules_from_context(
+            old_room_version, tombstone_event, tombstone_context
+        )
+
+        return tombstone_event, tombstone_context
+
     async def _update_upgraded_room_pls(
         self,
         requester: Requester,

synapse/storage/databases/main/event_federation.py

@@ -788,7 +788,7 @@ def get_insertion_event_backward_extremities_in_room_txn(txn, room_id):
             room_id,
         )
 
-    async def get_max_depth_of(self, event_ids: List[str]) -> Tuple[str, int]:
+    async def get_max_depth_of(self, event_ids: Iterable[str]) -> Tuple[str, int]:
         """Returns the event ID and depth for the event that has the max depth from a set of event IDs
 
         Args:
@@ -817,7 +817,7 @@ async def get_max_depth_of(self, event_ids: List[str]) -> Tuple[str, int]:
 
             return max_depth_event_id, current_max_depth
 
-    async def get_min_depth_of(self, event_ids: List[str]) -> Tuple[str, int]:
+    async def get_min_depth_of(self, event_ids: Iterable[str]) -> Tuple[str, int]:
         """Returns the event ID and depth for the event that has the min depth from a set of event IDs
 
         Args: