From a5c26750b50563f2edda8b5d37c70b1d49e5f34c Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Mon, 16 May 2022 14:06:04 +0100 Subject: [PATCH] Fix room upgrades creating an empty room when auth fails (#12696) Signed-off-by: Sean Quah --- changelog.d/12696.bugfix | 1 + synapse/handlers/room.py | 137 +++++++++++------- .../test_sharded_event_persister.py | 14 +- 3 files changed, 90 insertions(+), 62 deletions(-) create mode 100644 changelog.d/12696.bugfix diff --git a/changelog.d/12696.bugfix b/changelog.d/12696.bugfix new file mode 100644 index 0000000000..e410184a22 --- /dev/null +++ b/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. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index e71c78adad..23baa50d03 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -33,6 +33,7 @@ from typing import ( import attr from typing_extensions import TypedDict +import synapse.events.snapshot from synapse.api.constants import ( EventContentFields, EventTypes, @@ -77,7 +78,6 @@ from synapse.types import ( create_requester, ) from synapse.util import stringutils -from synapse.util.async_helpers import Linearizer from synapse.util.caches.response_cache import ResponseCache from synapse.util.stringutils import parse_and_validate_server_name from synapse.visibility import filter_events_for_client @@ -155,9 +155,6 @@ class RoomCreationHandler: self._replication = hs.get_replication_data_handler() - # linearizer to stop two upgrades happening at once - self._upgrade_linearizer = Linearizer("room_upgrade_linearizer") - # If a user tries to update the same room multiple times in quick # succession, only process the first attempt and return its result to # subsequent requests @@ -200,50 +197,17 @@ class RoomCreationHandler: 400, "An upgrade for this room is currently in progress" ) - # Upgrade the room - # - # If this user has sent multiple upgrade requests for the same room - # and one of them is not complete yet, cache the response and - # return it to all subsequent requests - ret = await self._upgrade_response_cache.wrap( - (old_room_id, user_id), - self._upgrade_room, - requester, - old_room_id, - new_version, # args for _upgrade_room - ) - - return ret - - async def _upgrade_room( - self, requester: Requester, old_room_id: str, new_version: RoomVersion - ) -> 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 - - Raises: - ShadowBanError if the requester is shadow-banned. - """ - user_id = requester.user.to_string() - assert self.hs.is_mine_id(user_id), "User must be our own: %s" % (user_id,) - - # start by allocating a new room id - r = await self.store.get_room(old_room_id) - if r is None: + # Check whether the room exists and 404 if it doesn't. + # We could go straight for the auth check, but that will raise a 403 instead. + old_room = await self.store.get_room(old_room_id) + if old_room is None: raise NotFoundError("Unknown room id %s" % (old_room_id,)) - new_room_id = await self._generate_room_id( - creator_id=user_id, - is_public=r["is_public"], - room_version=new_version, - ) - logger.info("Creating new room %s to replace %s", new_room_id, old_room_id) + new_room_id = self._generate_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. + # Check whether the user has the power level to carry out the upgrade. + # `check_auth_rules_from_context` will check that they are in the room and have + # the required power level to send the tombstone event. ( tombstone_event, tombstone_context, @@ -266,6 +230,63 @@ class RoomCreationHandler: old_room_version, tombstone_event, tombstone_context ) + # Upgrade the room + # + # If this user has sent multiple upgrade requests for the same room + # and one of them is not complete yet, cache the response and + # return it to all subsequent requests + ret = await self._upgrade_response_cache.wrap( + (old_room_id, user_id), + self._upgrade_room, + requester, + old_room_id, + old_room, # args for _upgrade_room + new_room_id, + new_version, + tombstone_event, + tombstone_context, + ) + + return ret + + async def _upgrade_room( + self, + requester: Requester, + old_room_id: str, + old_room: Dict[str, Any], + new_room_id: str, + new_version: RoomVersion, + tombstone_event: EventBase, + tombstone_context: synapse.events.snapshot.EventContext, + ) -> str: + """ + Args: + requester: the user requesting the upgrade + old_room_id: the id of the room to be replaced + old_room: a dict containing room information for the room to be replaced, + as returned by `RoomWorkerStore.get_room`. + new_room_id: the id of the replacement room + new_version: the version to upgrade the room to + tombstone_event: the tombstone event to send to the old room + tombstone_context: the context for the tombstone event + + Raises: + ShadowBanError if the requester is shadow-banned. + """ + user_id = requester.user.to_string() + assert self.hs.is_mine_id(user_id), "User must be our own: %s" % (user_id,) + + logger.info("Creating new room %s to replace %s", new_room_id, old_room_id) + + # create the new room. may raise a `StoreError` in the exceedingly unlikely + # event of a room ID collision. + await self.store.store_room( + room_id=new_room_id, + room_creator_user_id=user_id, + is_public=old_room["is_public"], + room_version=new_version, + ) + await self.clone_existing_room( requester, old_room_id=old_room_id, @@ -782,7 +803,7 @@ class RoomCreationHandler: visibility = config.get("visibility", "private") is_public = visibility == "public" - room_id = await self._generate_room_id( + room_id = await self._generate_and_create_room_id( creator_id=user_id, is_public=is_public, room_version=room_version, @@ -1104,7 +1125,26 @@ class RoomCreationHandler: return last_sent_stream_id - async def _generate_room_id( + def _generate_room_id(self) -> str: + """Generates a random room ID. + + Room IDs look like "!opaque_id:domain" and are case-sensitive as per the spec + at https://spec.matrix.org/v1.2/appendices/#room-ids-and-event-ids. + + Does not check for collisions with existing rooms or prevent future calls from + returning the same room ID. To ensure the uniqueness of a new room ID, use + `_generate_and_create_room_id` instead. + + Synapse's room IDs are 18 [a-zA-Z] characters long, which comes out to around + 102 bits. + + Returns: + A random room ID of the form "!opaque_id:domain". + """ + random_string = stringutils.random_string(18) + return RoomID(random_string, self.hs.hostname).to_string() + + async def _generate_and_create_room_id( self, creator_id: str, is_public: bool, @@ -1115,8 +1155,7 @@ class RoomCreationHandler: attempts = 0 while attempts < 5: try: - random_string = stringutils.random_string(18) - gen_room_id = RoomID(random_string, self.hs.hostname).to_string() + gen_room_id = self._generate_room_id() await self.store.store_room( room_id=gen_room_id, room_creator_user_id=creator_id, diff --git a/tests/replication/test_sharded_event_persister.py b/tests/replication/test_sharded_event_persister.py index 5f142e84c3..a7ca68069e 100644 --- a/tests/replication/test_sharded_event_persister.py +++ b/tests/replication/test_sharded_event_persister.py @@ -14,7 +14,6 @@ import logging from unittest.mock import patch -from synapse.api.room_versions import RoomVersion from synapse.rest import admin from synapse.rest.client import login, room, sync from synapse.storage.util.id_generators import MultiWriterIdGenerator @@ -64,21 +63,10 @@ class EventPersisterShardTestCase(BaseMultiWorkerStreamTestCase): # We control the room ID generation by patching out the # `_generate_room_id` method - async def generate_room( - creator_id: str, is_public: bool, room_version: RoomVersion - ): - await self.store.store_room( - room_id=room_id, - room_creator_user_id=creator_id, - is_public=is_public, - room_version=room_version, - ) - return room_id - with patch( "synapse.handlers.room.RoomCreationHandler._generate_room_id" ) as mock: - mock.side_effect = generate_room + mock.side_effect = lambda: room_id self.helper.create_room_as(user_id, tok=tok) def test_basic(self):