Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow admins to proactively block rooms #11228

Merged
merged 15 commits into from
Nov 9, 2021
1 change: 1 addition & 0 deletions changelog.d/11228.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow the admin [Delete Room API](https://matrix-org.github.io/synapse/latest/admin_api/rooms.html#delete-room-api) to block a room before this server joins it.
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions docs/admin_api/rooms.md
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ This API will remove all trace of the old room from your database after removing
all local users. If `purge` is `true` (the default), all traces of the old room will
be removed from your database after removing all local users. If you do not want
this to happen, set `purge` to `false`.
Depending on the amount of history being purged a call to the API may take
Depending on the amount of history being purged, a call to the API may take
several minutes or longer.

The local server will only have the power to move local user and room aliases to
Expand Down Expand Up @@ -478,7 +478,8 @@ The following fields are returned in the JSON response body:
* `failed_to_kick_users` - An array of users (`user_id`) that that were not kicked.
* `local_aliases` - An array of strings representing the local aliases that were migrated from
the old room to the new.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
* `new_room_id` - A string representing the room ID of the new room.
* `new_room_id` - A string representing the room ID of the new room, or `null` if
no such room was created.


## Undoing room deletions
Expand Down
40 changes: 31 additions & 9 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

"""Contains functions for performing events on rooms."""

"""Contains functions for performing actions on rooms."""
import itertools
import logging
import math
Expand All @@ -31,6 +30,8 @@
Tuple,
)

from typing_extensions import TypedDict

from synapse.api.constants import (
EventContentFields,
EventTypes,
Expand Down Expand Up @@ -1275,6 +1276,13 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]:
return self.store.get_room_events_max_id(room_id)


class ShutdownRoomResponse(TypedDict):
kicked_users: List[str]
failed_to_kick_users: List[str]
local_aliases: List[str]
new_room_id: Optional[str]


class RoomShutdownHandler:

DEFAULT_MESSAGE = (
Expand All @@ -1300,7 +1308,7 @@ async def shutdown_room(
new_room_name: Optional[str] = None,
message: Optional[str] = None,
block: bool = False,
) -> dict:
) -> ShutdownRoomResponse:
"""
Shuts down a room. Moves all local users and room aliases automatically
to a new room if `new_room_user_id` is set. Otherwise local users only
Expand Down Expand Up @@ -1344,7 +1352,9 @@ async def shutdown_room(
local_aliases:
An array of strings representing the local aliases that were
migrated from the old room to the new.
new_room_id: A string representing the room ID of the new room.
new_room_id:
A string representing the room ID of the new room, or None if
no such room was created.
"""

if not new_room_name:
Expand All @@ -1355,14 +1365,26 @@ async def shutdown_room(
if not RoomID.is_valid(room_id):
raise SynapseError(400, "%s is not a legal room ID" % (room_id,))

if not await self.store.get_room(room_id):
raise NotFoundError("Unknown room id %s" % (room_id,))

# This will work even if the room is already blocked, but that is
# desirable in case the first attempt at blocking the room failed below.
# Action the block first (even if the room doesn't exist yet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost feel as though we want to pull the room blocking bit out of this function and put the logic into _delete_room. Then only call shutdown_room and purge_room if the room is known. That may make the control flow slightly simpler?

Copy link
Contributor Author

@DMRobertson DMRobertson Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little reluctant here.

I didn't like the idea that the block could succeed only for the shutdown/purge to fail; I'd rather the whole thing succeed or fail atomically. But that's the situation today and wouldn't be introduced by your change. It would maybe mean checking twice that a room exists, which is a bit sad.

I'm kinda of the view that this is best left alone for now---I feel like blocking might merit being its own separate API call. How does that sit with you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if a simple block needs a new API. But I suspect it.
I am try to bring the delete API to a background task. #11223
I do not know how to handle a simple block in V2 API. A block request bring to background is not a good solution. If I am a user, I would except to get a response immediately.
An alternative for V2 API is to response for a simple block other than a shutdown or purge. But I do not like an API with two different return types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is related to #5575.
Maybe should the API return if the blocked room is known or unknown for the Homeserver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda of the view that this is best left alone for now---I feel like blocking might merit being its own separate API call. How does that sit with you?

I think shutting down a room should still block it, but separating a single block out into its own API call does sound like it'd be cleaner and better long-term. Leaving this alone for now with the hope of refactoring it all later sounds fine. Can you write up a ticket for the idea?

@dklimpel placing the shutdown room handling into a background process does sound like something that should happen regardless. I'm not sure how it relates to moving blocking of a room into a separate API however. Room blocking should be a relatively quick process.

if block:
# This will work even if the room is already blocked, but that is
# desirable in case the first attempt at blocking the room failed below.
await self.store.block_room(room_id, requester_user_id)
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

if not await self.store.get_room(room_id):
if block:
# We allow you to block an unknown room.
return {
"kicked_users": [],
"failed_to_kick_users": [],
"local_aliases": [],
"new_room_id": None,
}
else:
# But if you don't want to preventatively block another room,
# this function can't do anything useful.
raise NotFoundError("Unknown room id %s" % (room_id,))
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

if new_room_user_id is not None:
if not self.hs.is_mine_id(new_room_user_id):
raise SynapseError(
Expand Down
19 changes: 15 additions & 4 deletions synapse/rest/admin/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.
import logging
from http import HTTPStatus
from typing import TYPE_CHECKING, List, Optional, Tuple
from typing import TYPE_CHECKING, List, Optional, Tuple, cast
from urllib import parse as urlparse

from synapse.api.constants import EventTypes, JoinRules, Membership
Expand Down Expand Up @@ -239,9 +239,20 @@ async def _delete_room(

# Purge room
if purge:
await pagination_handler.purge_room(room_id, force=force_purge)

return 200, ret
try:
await pagination_handler.purge_room(room_id, force=force_purge)
except NotFoundError:
if block:
# We can block unknown rooms with this endpoint, in which case
# a failed purge is expected.
pass
else:
# But otherwise, we expect this purge to have succeeded.
raise

# Cast safety: I'm casting away the knowledge that this is a TypedDict.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
# `ret` is an opaque dictionary blob as far as the rest of the app cares.
return 200, cast(JsonDict, ret)


class RoomMembersRestServlet(RestServlet):
Expand Down
28 changes: 28 additions & 0 deletions tests/rest/admin/test_room.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@

import json
import urllib.parse
from http import HTTPStatus
from typing import List, Optional
from unittest.mock import Mock

from parameterized import parameterized

import synapse.rest.admin
from synapse.api.constants import EventTypes, Membership
from synapse.api.errors import Codes
Expand Down Expand Up @@ -281,6 +284,31 @@ def test_block_room_and_not_purge(self):
self._is_blocked(self.room_id, expect=True)
self._has_no_members(self.room_id)

@parameterized.expand([(True,), (False,)])
def test_block_unknown_room(self, purge: bool) -> None:
"""
We can block an unknown room. In this case, the `purge` argument
should be ignored.
"""
room_id = "!unknown:test"

# The room isn't already in the blocked rooms table
self._is_blocked(room_id, expect=False)

# Request the room be blocked.
channel = self.make_request(
"DELETE",
f"/_synapse/admin/v1/rooms/{room_id}",
{"block": True, "purge": purge},
access_token=self.admin_user_tok,
)

# The room is now blocked.
self.assertEqual(
HTTPStatus.OK, int(channel.result["code"]), msg=channel.result["body"]
)
self._is_blocked(room_id)

def test_shutdown_room_consent(self):
"""Test that we can shutdown rooms with local users who have not
yet accepted the privacy policy. This used to fail when we tried to
Expand Down