From c35a3674b719d45266a0aef86a6265cdfeaf7602 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sun, 31 Oct 2021 21:56:25 +0100 Subject: [PATCH 01/23] Convert delete room admin API to async endpoint --- docs/admin_api/rooms.md | 105 ++++- synapse/handlers/pagination.py | 336 +++++++++++++++- synapse/handlers/room.py | 5 +- synapse/rest/admin/__init__.py | 4 + synapse/rest/admin/rooms.py | 104 ++++- tests/rest/admin/test_room.py | 675 +++++++++++++++++++++++++++++---- 6 files changed, 1147 insertions(+), 82 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 8e524e65090e..79a394ad0654 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -4,6 +4,9 @@ - [Room Members API](#room-members-api) - [Room State API](#room-state-api) - [Delete Room API](#delete-room-api) + * [Version 1 (old version)](#version-1-old-version) + * [Version 2 (new version)](#version-2-new-version) + * [Status of deleting rooms](#status-of-deleting-rooms) * [Undoing room shutdowns](#undoing-room-shutdowns) - [Make Room Admin API](#make-room-admin-api) - [Forward Extremities Admin API](#forward-extremities-admin-api) @@ -403,6 +406,17 @@ several minutes or longer. The local server will only have the power to move local user and room aliases to the new room. Users on other servers will be unaffected. +To use it, you will need to authenticate by providing an ``access_token`` for a +server admin: see [Admin API](../usage/administration/admin_api). + +## Version 1 (old version) + +This version works synchronous. That means you get the response if the server has +finised this action. This may take a long time. If you requests the same action +a second time, if the server is not finished the first one, can the server hang up. +This is fixed in Version 2 of this API. The parameters are the same in both APIs. +This API will become deprecated in the future. + The API is: ``` @@ -421,9 +435,6 @@ with a body of: } ``` -To use it, you will need to authenticate by providing an ``access_token`` for a -server admin: see [Admin API](../usage/administration/admin_api). - A response body like the following is returned: ```json @@ -440,6 +451,38 @@ A response body like the following is returned: } ``` +The parameters and response values have the same expression like in +[version 2](#version-2-new-version) of the API. + +## Version 2 (new version) + +This version works asynchronous. That means you get the response from server immediately. +The server works on that task in background. You can request the status of this action. + +The API is: + +``` +DELETE /_synapse/admin/v2/rooms/ +``` + +with a body of: + +```json +{ + "new_room_user_id": "@someuser:example.com", + "room_name": "Content Violation Notification", + "message": "Bad Room has been shutdown due to content violations on this server. Please review our Terms of Service.", + "block": true, + "purge": true +} +``` + +An empty JSON dict is returned. + +```json +{} +``` + **Parameters** The following parameters should be set in the URL: @@ -470,16 +513,60 @@ The following JSON body parameters are available: The JSON body must not be empty. The body must be at least `{}`. +## Status of deleting rooms + +It is possible to query the status of the background task for deleting rooms. +The status can be queried up to 24 hours after completion of the task +or a restart of Synapse. + +The API is: + +``` +GET /_synapse/admin/v2/rooms//delete_status +``` + +A response body like the following is returned: + +```json +{ + "status": "active", + "result": { + "kicked_users": [ + "@foobar:example.com" + ], + "failed_to_kick_users": [], + "local_aliases": [ + "#badroom:example.com", + "#evilsaloon:example.com" + ], + "new_room_id": "!newroomid:example.com" + } +} +``` + +**Parameters** + +The following parameters should be set in the URL: + +* `room_id` - The ID of the room. + **Response** The following fields are returned in the JSON response body: -* `kicked_users` - An array of users (`user_id`) that were kicked. -* `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. -* `new_room_id` - A string representing the room ID of the new room. - +* `status` - The status will be one of: + - `remove members` - The process is removing users from the `room_id`. + - `active` - The process is purging the room from databse. + - `complete` - The process has completed successfully. + - `failed` - The process is aborted, an error has occurred. +* `result` - An object containing information about the result of shutting down the room. + *Note:* The result is shown after removing the room members. The delete process can + still be running. Please pay attention to the `status`. + - `kicked_users` - An array of users (`user_id`) that were kicked. + - `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. + - `new_room_id` - A string representing the room ID of the new room. ## Undoing room deletions diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index abfe7be0e317..70151622d236 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -19,14 +19,14 @@ from twisted.python.failure import Failure -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, RoomCreationPreset from synapse.api.errors import SynapseError from synapse.api.filtering import Filter from synapse.logging.context import run_in_background from synapse.metrics.background_process_metrics import run_as_background_process from synapse.storage.state import StateFilter from synapse.streams.config import PaginationConfig -from synapse.types import JsonDict, Requester +from synapse.types import JsonDict, Requester, create_requester from synapse.util.async_helpers import ReadWriteLock from synapse.util.stringutils import random_string from synapse.visibility import filter_events_for_client @@ -49,19 +49,30 @@ class PurgeStatus: STATUS_ACTIVE = 0 STATUS_COMPLETE = 1 STATUS_FAILED = 2 + STATUS_REMOVE_MEMBERS = 3 STATUS_TEXT = { STATUS_ACTIVE: "active", STATUS_COMPLETE: "complete", STATUS_FAILED: "failed", + STATUS_REMOVE_MEMBERS: "remove members", } # Tracks whether this request has completed. One of STATUS_{ACTIVE,COMPLETE,FAILED}. status: int = STATUS_ACTIVE + # Saves the result of an action to give it back to REST API + result: Dict = {} + def asdict(self) -> JsonDict: return {"status": PurgeStatus.STATUS_TEXT[self.status]} + def asdict_with_result(self) -> JsonDict: + return { + "status": PurgeStatus.STATUS_TEXT[self.status], + "result": self.result, + } + class PaginationHandler: """Handles pagination and purge history requests. @@ -70,6 +81,12 @@ class PaginationHandler: paginating during a purge. """ + DEFAULT_MESSAGE = ( + "Sharing illegal content on this server is not permitted and rooms in" + " violation will be blocked." + ) + DEFAULT_ROOM_NAME = "Content Violation Notification" + def __init__(self, hs: "HomeServer"): self.hs = hs self.auth = hs.get_auth() @@ -79,6 +96,11 @@ def __init__(self, hs: "HomeServer"): self.clock = hs.get_clock() self._server_name = hs.hostname + self.room_member_handler = hs.get_room_member_handler() + self._room_creation_handler = hs.get_room_creation_handler() + self._replication = hs.get_replication_data_handler() + self.event_creation_handler = hs.get_event_creation_handler() + self.pagination_lock = ReadWriteLock() self._purges_in_progress_by_room: Set[str] = set() # map from purge id to PurgeStatus @@ -314,6 +336,7 @@ def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: async def purge_room(self, room_id: str, force: bool = False) -> None: """Purge the given room from the database. + This function is part the delete room v1 API. Args: room_id: room to be purged @@ -472,3 +495,312 @@ async def get_messages( ) return chunk + + async def _shutdown_and_purge_room( + self, + room_id: str, + requester_user_id: str, + new_room_user_id: Optional[str] = None, + new_room_name: Optional[str] = None, + message: Optional[str] = None, + block: bool = False, + purge: bool = True, + force_purge: bool = False, + ) -> None: + """ + Shuts down and purge 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 leave the room without any information. + After that, the room will be removed from the database. + + The new room will be created with the user specified by the + `new_room_user_id` parameter as room administrator and will contain a + message explaining what happened. Users invited to the new room will + have power level `-10` by default, and thus be unable to speak. + + The local server will only have the power to move local user and room + aliases to the new room. Users on other servers will be unaffected. + + Args: + room_id: The ID of the room to shut down. + requester_user_id: + User who requested the action and put the room on the + blocking list. + new_room_user_id: + If set, a new room will be created with this user ID + as the creator and admin, and all users in the old room will be + moved into that room. If not set, no new room will be created + and the users will just be removed from the old room. + new_room_name: + A string representing the name of the room that new users will + be invited to. Defaults to `Content Violation Notification` + message: + A string containing the first message that will be sent as + `new_room_user_id` in the new room. Ideally this will clearly + convey why the original room was shut down. + Defaults to `Sharing illegal content on this server is not + permitted and rooms in violation will be blocked.` + block: + If set to `true`, this room will be added to a blocking list, + preventing future attempts to join the room. Defaults to `false`. + purge: + If set to `true`, purge the given room from the database. + force_purge: + If set to `true`, the room will be purged from database + also if it fails to remove some users from room. + + Saves a dict containing the following keys in `PurgeStatus`: + kicked_users: An array of users (`user_id`) that were kicked. + 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. + new_room_id: A string representing the room ID of the new room. + """ + + if not new_room_name: + new_room_name = self.DEFAULT_ROOM_NAME + if not message: + message = self.DEFAULT_MESSAGE + + self._purges_in_progress_by_room.add(room_id) + try: + with await self.pagination_lock.write(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. + if block: + await self.store.block_room(room_id, requester_user_id) + + if new_room_user_id is not None: + room_creator_requester = create_requester( + new_room_user_id, authenticated_entity=requester_user_id + ) + + info, stream_id = await self._room_creation_handler.create_room( + room_creator_requester, + config={ + "preset": RoomCreationPreset.PUBLIC_CHAT, + "name": new_room_name, + "power_level_content_override": {"users_default": -10}, + }, + ratelimit=False, + ) + new_room_id = info["room_id"] + + logger.info( + "[shutdown_and_purge] Shutting down room %r, joining to new room: %r", + room_id, + new_room_id, + ) + + # We now wait for the create room to come back in via replication so + # that we can assume that all the joins/invites have propagated before + # we try and auto join below. + await self._replication.wait_for_stream_position( + self.hs.config.worker.events_shard_config.get_instance( + new_room_id + ), + "events", + stream_id, + ) + else: + new_room_id = None + logger.info("[shutdown_and_purge] Shutting down room %r", room_id) + + self._purges_by_id[room_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS + + users = await self.store.get_users_in_room(room_id) + kicked_users = [] + failed_to_kick_users = [] + for user_id in users: + if not self.hs.is_mine_id(user_id): + continue + + logger.info( + "[shutdown_and_purge] Kicking %r from %r...", user_id, room_id + ) + + try: + # Kick users from room + target_requester = create_requester( + user_id, authenticated_entity=requester_user_id + ) + _, stream_id = await self.room_member_handler.update_membership( + requester=target_requester, + target=target_requester.user, + room_id=room_id, + action=Membership.LEAVE, + content={}, + ratelimit=False, + require_consent=False, + ) + + # Wait for leave to come in over replication before trying to forget. + await self._replication.wait_for_stream_position( + self.hs.config.worker.events_shard_config.get_instance( + room_id + ), + "events", + stream_id, + ) + + await self.room_member_handler.forget( + target_requester.user, room_id + ) + + # Join users to new room + if new_room_user_id: + await self.room_member_handler.update_membership( + requester=target_requester, + target=target_requester.user, + room_id=new_room_id, + action=Membership.JOIN, + content={}, + ratelimit=False, + require_consent=False, + ) + + kicked_users.append(user_id) + except Exception: + logger.exception( + "[shutdown_and_purge] Failed to leave old room and join new room for %r", + user_id, + ) + failed_to_kick_users.append(user_id) + + self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE + + # Send message in new room and move aliases + if new_room_user_id: + await self.event_creation_handler.create_and_send_nonmember_event( + room_creator_requester, + { + "type": "m.room.message", + "content": {"body": message, "msgtype": "m.text"}, + "room_id": new_room_id, + "sender": new_room_user_id, + }, + ratelimit=False, + ) + + aliases_for_room = await self.store.get_aliases_for_room(room_id) + + await self.store.update_aliases_for_room( + room_id, new_room_id, requester_user_id + ) + else: + aliases_for_room = [] + + self._purges_by_id[room_id].result = { + "kicked_users": kicked_users, + "failed_to_kick_users": failed_to_kick_users, + "local_aliases": aliases_for_room, + "new_room_id": new_room_id, + } + + if purge: + logger.info( + "[shutdown_and_purge] starting purge room_id %s", room_id + ) + self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE + + # first check that we have no users in this room + if not force_purge: + joined = await self.store.is_host_joined( + room_id, self._server_name + ) + if joined: + raise SynapseError( + 400, "Users are still joined to this room" + ) + + await self.storage.purge_events.purge_room(room_id) + + logger.info("[shutdown_and_purge] complete") + self._purges_by_id[room_id].status = PurgeStatus.STATUS_COMPLETE + except Exception: + f = Failure() + logger.error( + "[shutdown_and_purge] failed", + exc_info=(f.type, f.value, f.getTracebackObject()), # type: ignore + ) + self._purges_by_id[room_id].status = PurgeStatus.STATUS_FAILED + finally: + self._purges_in_progress_by_room.discard(room_id) + + # remove the purge from the list 24 hours after it completes + def clear_purge() -> None: + del self._purges_by_id[room_id] + + self.hs.get_reactor().callLater(24 * 3600, clear_purge) + + def start_shutdown_and_purge_room( + self, + room_id: str, + requester_user_id: str, + new_room_user_id: Optional[str] = None, + new_room_name: Optional[str] = None, + message: Optional[str] = None, + block: bool = False, + purge: bool = True, + force_purge: bool = False, + ) -> None: + """Start off shut down and purge on a room. + + Args: + room_id: The ID of the room to shut down. + requester_user_id: + User who requested the action and put the room on the + blocking list. + new_room_user_id: + If set, a new room will be created with this user ID + as the creator and admin, and all users in the old room will be + moved into that room. If not set, no new room will be created + and the users will just be removed from the old room. + new_room_name: + A string representing the name of the room that new users will + be invited to. Defaults to `Content Violation Notification` + message: + A string containing the first message that will be sent as + `new_room_user_id` in the new room. Ideally this will clearly + convey why the original room was shut down. + Defaults to `Sharing illegal content on this server is not + permitted and rooms in violation will be blocked.` + block: + If set to `true`, this room will be added to a blocking list, + preventing future attempts to join the room. Defaults to `false`. + purge: + If set to `true`, purge the given room from the database. + force_purge: + If set to `true`, the room will be purged from database + also if it fails to remove some users from room. + """ + if room_id in self._purges_in_progress_by_room: + raise SynapseError( + 400, "History purge already in progress for %s" % (room_id,) + ) + + if new_room_user_id is not None: + if not self.hs.is_mine_id(new_room_user_id): + raise SynapseError( + 400, "User must be our own: %s" % (new_room_user_id,) + ) + + # we log the purge_id here so that it can be tied back to the + # request id in the log lines. + logger.info("[shutdown_and_purge] starting shutdown room_id %s", room_id) + + self._purges_by_id[room_id] = PurgeStatus() + run_in_background( + self._shutdown_and_purge_room, + room_id, + requester_user_id, + new_room_user_id, + new_room_name, + message, + block, + purge, + force_purge, + ) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 99e9b3734457..b98e80b975c8 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1276,6 +1276,10 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class RoomShutdownHandler: + """This handler shutdown rooms synchronous and is part of the delete room v1 API. + It will become deprecated in the future. + The handler for asynchronous shudowns is part of the PaginationHandler. + """ DEFAULT_MESSAGE = ( "Sharing illegal content on this server is not permitted and rooms in" @@ -1289,7 +1293,6 @@ def __init__(self, hs: "HomeServer"): self._room_creation_handler = hs.get_room_creation_handler() self._replication = hs.get_replication_data_handler() self.event_creation_handler = hs.get_event_creation_handler() - self.state = hs.get_state_handler() self.store = hs.get_datastore() async def shutdown_room( diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index e1506deb2b35..86abc67770f5 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -43,6 +43,7 @@ ) from synapse.rest.admin.rooms import ( DeleteRoomRestServlet, + DeleteRoomStatusRestServlet, ForwardExtremitiesRestServlet, JoinRoomAliasServlet, ListRoomRestServlet, @@ -50,6 +51,7 @@ RoomEventContextServlet, RoomMembersRestServlet, RoomRestServlet, + RoomRestV2Servlet, RoomStateRestServlet, ) from synapse.rest.admin.server_notice_servlet import SendServerNoticeServlet @@ -220,8 +222,10 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: ListRoomRestServlet(hs).register(http_server) RoomStateRestServlet(hs).register(http_server) RoomRestServlet(hs).register(http_server) + RoomRestV2Servlet(hs).register(http_server) RoomMembersRestServlet(hs).register(http_server) DeleteRoomRestServlet(hs).register(http_server) + DeleteRoomStatusRestServlet(hs).register(http_server) JoinRoomAliasServlet(hs).register(http_server) VersionServlet(hs).register(http_server) UserAdminServlet(hs).register(http_server) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index a4823ca6e714..d174692e7f02 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -34,7 +34,7 @@ assert_user_is_admin, ) from synapse.storage.databases.main.room import RoomSortOrder -from synapse.types import JsonDict, UserID, create_requester +from synapse.types import JsonDict, RoomID, UserID, create_requester from synapse.util import json_decoder if TYPE_CHECKING: @@ -81,6 +81,108 @@ async def on_POST( ) +class RoomRestV2Servlet(RestServlet): + """Delete a room from server asynchronously with a background task. + + It is a combination and improvement of shutdown and purge room. + + Shuts down a room by removing all local users from the room. + Blocking all future invites and joins to the room is optional. + + If desired any local aliases will be repointed to a new room + created by `new_room_user_id` and kicked users will be auto- + joined to the new room. + + If 'purge' is true, it will remove all traces of a room from the database. + """ + + PATTERNS = admin_patterns("/rooms/(?P[^/]+)$", "v2") + + def __init__(self, hs: "HomeServer"): + self.hs = hs + self.auth = hs.get_auth() + self.store = hs.get_datastore() + self.pagination_handler = hs.get_pagination_handler() + + async def on_DELETE( + self, request: SynapseRequest, room_id: str + ) -> Tuple[int, JsonDict]: + + requester = await self.auth.get_user_by_req(request) + await assert_user_is_admin(self.auth, requester.user) + + content = parse_json_object_from_request(request) + + block = content.get("block", False) + if not isinstance(block, bool): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Param 'block' must be a boolean, if given", + Codes.BAD_JSON, + ) + + purge = content.get("purge", True) + if not isinstance(purge, bool): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Param 'purge' must be a boolean, if given", + Codes.BAD_JSON, + ) + + force_purge = content.get("force_purge", False) + if not isinstance(force_purge, bool): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Param 'force_purge' must be a boolean, if given", + Codes.BAD_JSON, + ) + + 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,)) + + self.pagination_handler.start_shutdown_and_purge_room( + room_id=room_id, + new_room_user_id=content.get("new_room_user_id"), + new_room_name=content.get("room_name"), + message=content.get("message"), + requester_user_id=requester.user.to_string(), + block=block, + purge=purge, + force_purge=force_purge, + ) + + return 200, {} + + +class DeleteRoomStatusRestServlet(RestServlet): + """Get the status of the delete room background task.""" + + PATTERNS = admin_patterns("/rooms/(?P[^/]+)/delete_status$", "v2") + + def __init__(self, hs: "HomeServer"): + self.hs = hs + self.auth = hs.get_auth() + self.pagination_handler = hs.get_pagination_handler() + + async def on_GET( + self, request: SynapseRequest, room_id: str + ) -> Tuple[int, JsonDict]: + + await assert_requester_is_admin(self.auth, request) + + if not RoomID.is_valid(room_id): + raise SynapseError(400, "%s is not a legal room ID" % (room_id,)) + + purge_status = self.pagination_handler.get_purge_status(room_id) + if purge_status is None: + raise NotFoundError("No delete task for room_id '%s' found" % room_id) + + return 200, purge_status.asdict_with_result() + + class ListRoomRestServlet(RestServlet): """ List all rooms that are known to the homeserver. Results are returned diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 0fa55e03b45b..1d0dba23c3cc 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -17,7 +17,7 @@ from typing import List, Optional from unittest.mock import Mock -from parameterized import parameterized_class +from parameterized import parameterized, parameterized_class import synapse.rest.admin from synapse.api.constants import EventTypes, Membership @@ -77,11 +77,11 @@ def test_requester_is_no_admin(self): channel = self.make_request( self.method, self.url, - json.dumps({}), + {}, access_token=self.other_user_tok, ) - self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def test_room_does_not_exist(self): @@ -93,11 +93,11 @@ def test_room_does_not_exist(self): channel = self.make_request( self.method, url, - json.dumps({}), + {}, access_token=self.admin_user_tok, ) - self.assertEqual(404, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(404, channel.code, msg=channel.json_body) self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) def test_room_is_not_valid(self): @@ -109,11 +109,11 @@ def test_room_is_not_valid(self): channel = self.make_request( self.method, url, - json.dumps({}), + {}, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( "invalidroom is not a legal room ID", channel.json_body["error"], @@ -128,11 +128,11 @@ def test_new_room_user_does_not_exist(self): channel = self.make_request( self.method, self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertIn("new_room_id", channel.json_body) self.assertIn("kicked_users", channel.json_body) self.assertIn("failed_to_kick_users", channel.json_body) @@ -147,11 +147,11 @@ def test_new_room_user_is_not_local(self): channel = self.make_request( self.method, self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( "User must be our own: @not:exist.bla", channel.json_body["error"], @@ -166,11 +166,11 @@ def test_block_is_not_bool(self): channel = self.make_request( self.method, self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) def test_purge_is_not_bool(self): @@ -182,11 +182,11 @@ def test_purge_is_not_bool(self): channel = self.make_request( self.method, self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) def test_purge_room_and_block(self): @@ -208,11 +208,11 @@ def test_purge_room_and_block(self): channel = self.make_request( self.method, self.url.encode("ascii"), - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(None, channel.json_body["new_room_id"]) self.assertEqual(self.other_user, channel.json_body["kicked_users"][0]) self.assertIn("failed_to_kick_users", channel.json_body) @@ -241,11 +241,11 @@ def test_purge_room_and_not_block(self): channel = self.make_request( self.method, self.url.encode("ascii"), - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(None, channel.json_body["new_room_id"]) self.assertEqual(self.other_user, channel.json_body["kicked_users"][0]) self.assertIn("failed_to_kick_users", channel.json_body) @@ -275,11 +275,11 @@ def test_block_room_and_not_purge(self): channel = self.make_request( self.method, self.url.encode("ascii"), - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(None, channel.json_body["new_room_id"]) self.assertEqual(self.other_user, channel.json_body["kicked_users"][0]) self.assertIn("failed_to_kick_users", channel.json_body) @@ -325,7 +325,7 @@ def test_shutdown_room_consent(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(self.other_user, channel.json_body["kicked_users"][0]) self.assertIn("new_room_id", channel.json_body) self.assertIn("failed_to_kick_users", channel.json_body) @@ -354,7 +354,7 @@ def test_shutdown_room_block_peek(self): json.dumps({"history_visibility": "world_readable"}), access_token=self.other_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Test that room is not purged with self.assertRaises(AssertionError): @@ -371,7 +371,7 @@ def test_shutdown_room_block_peek(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(self.other_user, channel.json_body["kicked_users"][0]) self.assertIn("new_room_id", channel.json_body) self.assertIn("failed_to_kick_users", channel.json_body) @@ -427,17 +427,560 @@ def _assert_peek(self, room_id, expect_code): channel = self.make_request( "GET", url.encode("ascii"), access_token=self.admin_user_tok ) - self.assertEqual( - expect_code, int(channel.result["code"]), msg=channel.result["body"] - ) + self.assertEqual(expect_code, channel.code, msg=channel.json_body) url = "events?timeout=0&room_id=" + room_id channel = self.make_request( "GET", url.encode("ascii"), access_token=self.admin_user_tok ) + self.assertEqual(expect_code, channel.code, msg=channel.json_body) + + +class DeleteRoomV2TestCase(unittest.HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + events.register_servlets, + room.register_servlets, + room.register_deprecated_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.event_creation_handler = hs.get_event_creation_handler() + hs.config.consent.user_consent_version = "1" + + consent_uri_builder = Mock() + consent_uri_builder.build_user_consent_uri.return_value = "http://example.com" + self.event_creation_handler._consent_uri_builder = consent_uri_builder + + self.store = hs.get_datastore() + + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + self.other_user = self.register_user("user", "pass") + self.other_user_tok = self.login("user", "pass") + + # Mark the admin user as having consented + self.get_success(self.store.user_set_consent_version(self.admin_user, "1")) + + self.room_id = self.helper.create_room_as( + self.other_user, tok=self.other_user_tok + ) + self.url = f"/_synapse/admin/v2/rooms/{self.room_id}" + self.url_status = f"/_synapse/admin/v2/rooms/{self.room_id}/delete_status" + + @parameterized.expand( + [ + ("DELETE", "/_synapse/admin/v2/rooms/%s"), + ("GET", "/_synapse/admin/v2/rooms/%s/delete_status"), + ] + ) + def test_requester_is_no_admin(self, method: str, url: str): + """ + If the user is not a server admin, an error 403 is returned. + """ + + channel = self.make_request( + method, + url % self.room_id, + content={}, + access_token=self.other_user_tok, + ) + + self.assertEqual(403, channel.code, msg=channel.json_body) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + @parameterized.expand( + [ + ("DELETE", "/_synapse/admin/v2/rooms/%s"), + ("GET", "/_synapse/admin/v2/rooms/%s/delete_status"), + ] + ) + def test_room_does_not_exist(self, method: str, url: str): + """ + Check that unknown rooms/server return error 404. + """ + + channel = self.make_request( + method, + url % "!unknown:test", + content={}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) + + @parameterized.expand( + [ + ("DELETE", "/_synapse/admin/v2/rooms/%s"), + ("GET", "/_synapse/admin/v2/rooms/%s/delete_status"), + ] + ) + def test_room_is_not_valid(self, method: str, url: str): + """ + Check that invalid room names, return an error 400. + """ + + channel = self.make_request( + method, + url % "invalidroom", + content={}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual( + "invalidroom is not a legal room ID", + channel.json_body["error"], + ) + + def test_new_room_user_does_not_exist(self): + """ + Tests that the user ID must be from local server but it does not have to exist. + """ + + channel = self.make_request( + "DELETE", + self.url, + content={"new_room_user_id": "@unknown:test"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertIn("new_room_id", channel.json_body["result"]) + self.assertIn("kicked_users", channel.json_body["result"]) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + def test_new_room_user_is_not_local(self): + """ + Check that only local users can create new room to move members. + """ + + channel = self.make_request( + "DELETE", + self.url, + content={"new_room_user_id": "@not:exist.bla"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual( + "User must be our own: @not:exist.bla", + channel.json_body["error"], + ) + + def test_block_is_not_bool(self): + """ + If parameter `block` is not boolean, return an error + """ + + channel = self.make_request( + "DELETE", + self.url, + content={"block": "NotBool"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) + + def test_purge_is_not_bool(self): + """ + If parameter `purge` is not boolean, return an error + """ + + channel = self.make_request( + "DELETE", + self.url, + content={"purge": "NotBool"}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) + + def test_delete_expired_status(self): + """Test that the task status is removed after expiration.""" + + channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content={}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + + # get status after more than 24 hours + self.reactor.advance(24 * 3600 + 10) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) + + def test_delete_same_room_twice(self): + """Test that the call for delete a room at second time gives an exception.""" + + body = {"new_room_user_id": self.admin_user} + + # first call to delete room + # and do not wait for finish the task + first_channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content=body, + access_token=self.admin_user_tok, + await_result=False, + ) + + # second call to delete room + second_channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content=body, + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, second_channel.code, msg=second_channel.json_body) + self.assertEqual(Codes.UNKNOWN, second_channel.json_body["errcode"]) + self.assertEqual( + f"History purge already in progress for {self.room_id}", + second_channel.json_body["error"], + ) + + # get result of first call + first_channel.await_result() + self.assertEqual(200, first_channel.code, msg=first_channel.json_body) + + # check status after finish the task + status_channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, status_channel.code, msg=status_channel.json_body) + self.assertEqual("complete", status_channel.json_body["status"]) + + def test_purge_room_and_block(self): + """Test to purge a room and block it. + Members will not be moved to a new room and will not receive a message. + """ + # Test that room is not purged + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + + # Test that room is not blocked + self._is_blocked(self.room_id, expect=False) + + # Assert one user in room + self._is_member(room_id=self.room_id, user_id=self.other_user) + + channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content={"block": True, "purge": True}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertIsNone(channel.json_body["result"]["new_room_id"]) + self.assertEqual( + self.other_user, channel.json_body["result"]["kicked_users"][0] + ) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + self._is_purged(self.room_id) + self._is_blocked(self.room_id, expect=True) + self._has_no_members(self.room_id) + + def test_purge_room_and_not_block(self): + """Test to purge a room and do not block it. + Members will not be moved to a new room and will not receive a message. + """ + # Test that room is not purged + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + + # Test that room is not blocked + self._is_blocked(self.room_id, expect=False) + + # Assert one user in room + self._is_member(room_id=self.room_id, user_id=self.other_user) + + channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content={"block": False, "purge": True}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertIsNone(channel.json_body["result"]["new_room_id"]) + self.assertEqual( + self.other_user, channel.json_body["result"]["kicked_users"][0] + ) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + self._is_purged(self.room_id) + self._is_blocked(self.room_id, expect=False) + self._has_no_members(self.room_id) + + def test_block_room_and_not_purge(self): + """Test to block a room without purging it. + Members will not be moved to a new room and will not receive a message. + The room will not be purged. + """ + # Test that room is not purged + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + + # Test that room is not blocked + self._is_blocked(self.room_id, expect=False) + + # Assert one user in room + self._is_member(room_id=self.room_id, user_id=self.other_user) + + channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content={"block": False, "purge": False}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertIsNone(channel.json_body["result"]["new_room_id"]) + self.assertEqual( + self.other_user, channel.json_body["result"]["kicked_users"][0] + ) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + self._is_blocked(self.room_id, expect=False) + self._has_no_members(self.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 + force part the user from the old room. + Members will be moved to a new room and will receive a message. + """ + self.event_creation_handler._block_events_without_consent_error = None + + # Assert one user in room + users_in_room = self.get_success(self.store.get_users_in_room(self.room_id)) + self.assertEqual([self.other_user], users_in_room) + + # Enable require consent to send events + self.event_creation_handler._block_events_without_consent_error = "Error" + + # Assert that the user is getting consent error + self.helper.send( + self.room_id, body="foo", tok=self.other_user_tok, expect_code=403 + ) + + # Test that room is not purged + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + + # Assert one user in room + self._is_member(room_id=self.room_id, user_id=self.other_user) + + # Test that the admin can still send shutdown + channel = self.make_request( + "DELETE", + self.url, + content={"new_room_user_id": self.admin_user}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) self.assertEqual( - expect_code, int(channel.result["code"]), msg=channel.result["body"] + self.other_user, channel.json_body["result"]["kicked_users"][0] ) + self.assertIn("new_room_id", channel.json_body["result"]) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + # Test that member has moved to new room + self._is_member( + room_id=channel.json_body["result"]["new_room_id"], user_id=self.other_user + ) + + self._is_purged(self.room_id) + self._has_no_members(self.room_id) + + def test_shutdown_room_block_peek(self): + """Test that a world_readable room can no longer be peeked into after + it has been shut down. + Members will be moved to a new room and will receive a message. + """ + self.event_creation_handler._block_events_without_consent_error = None + + # Enable world readable + url = "rooms/%s/state/m.room.history_visibility" % (self.room_id,) + channel = self.make_request( + "PUT", + url.encode("ascii"), + content={"history_visibility": "world_readable"}, + access_token=self.other_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Test that room is not purged + with self.assertRaises(AssertionError): + self._is_purged(self.room_id) + + # Assert one user in room + self._is_member(room_id=self.room_id, user_id=self.other_user) + + # Test that the admin can still send shutdown + channel = self.make_request( + "DELETE", + self.url, + content={"new_room_user_id": self.admin_user}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + channel = self.make_request( + "GET", + self.url_status, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertEqual( + self.other_user, channel.json_body["result"]["kicked_users"][0] + ) + self.assertIn("new_room_id", channel.json_body["result"]) + self.assertIn("failed_to_kick_users", channel.json_body["result"]) + self.assertIn("local_aliases", channel.json_body["result"]) + + # Test that member has moved to new room + self._is_member( + room_id=channel.json_body["result"]["new_room_id"], user_id=self.other_user + ) + + self._is_purged(self.room_id) + self._has_no_members(self.room_id) + + # Assert we can no longer peek into the room + self._assert_peek(self.room_id, expect_code=403) + + def _is_blocked(self, room_id, expect=True): + """Assert that the room is blocked or not""" + d = self.store.is_room_blocked(room_id) + if expect: + self.assertTrue(self.get_success(d)) + else: + self.assertIsNone(self.get_success(d)) + + def _has_no_members(self, room_id): + """Assert there is now no longer anyone in the room""" + users_in_room = self.get_success(self.store.get_users_in_room(room_id)) + self.assertEqual([], users_in_room) + + def _is_member(self, room_id, user_id): + """Test that user is member of the room""" + users_in_room = self.get_success(self.store.get_users_in_room(room_id)) + self.assertIn(user_id, users_in_room) + + def _is_purged(self, room_id): + """Test that the following tables have been purged of all rows related to the room.""" + for table in PURGE_TABLES: + count = self.get_success( + self.store.db_pool.simple_select_one_onecol( + table=table, + keyvalues={"room_id": room_id}, + retcol="COUNT(*)", + desc="test_purge_room", + ) + ) + + self.assertEqual(count, 0, msg=f"Rows not purged in {table}") + + def _assert_peek(self, room_id, expect_code): + """Assert that the admin user can (or cannot) peek into the room.""" + + url = f"rooms/{room_id}/initialSync" + channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok + ) + self.assertEqual(expect_code, channel.code, msg=channel.json_body) + + url = "events?timeout=0&room_id=" + room_id + channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok + ) + self.assertEqual(expect_code, channel.code, msg=channel.json_body) class RoomTestCase(unittest.HomeserverTestCase): @@ -559,9 +1102,7 @@ def test_list_rooms_pagination(self): url.encode("ascii"), access_token=self.admin_user_tok, ) - self.assertEqual( - 200, int(channel.result["code"]), msg=channel.result["body"] - ) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertTrue("rooms" in channel.json_body) for r in channel.json_body["rooms"]: @@ -601,7 +1142,7 @@ def test_list_rooms_pagination(self): url.encode("ascii"), access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) def test_correct_room_attributes(self): """Test the correct attributes for a room are returned""" @@ -624,7 +1165,7 @@ def test_correct_room_attributes(self): {"room_id": room_id}, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Set this new alias as the canonical alias for this room self.helper.send_state( @@ -656,7 +1197,7 @@ def test_correct_room_attributes(self): url.encode("ascii"), access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Check that rooms were returned self.assertTrue("rooms" in channel.json_body) @@ -700,9 +1241,7 @@ def _set_canonical_alias(room_id: str, test_alias: str, admin_user_tok: str): {"room_id": room_id}, access_token=admin_user_tok, ) - self.assertEqual( - 200, int(channel.result["code"]), msg=channel.result["body"] - ) + self.assertEqual(200, channel.code, msg=channel.json_body) # Set this new alias as the canonical alias for this room self.helper.send_state( @@ -1156,11 +1695,11 @@ def test_requester_is_no_admin(self): channel = self.make_request( "POST", self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.second_tok, ) - self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def test_invalid_parameter(self): @@ -1172,11 +1711,11 @@ def test_invalid_parameter(self): channel = self.make_request( "POST", self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual(Codes.MISSING_PARAM, channel.json_body["errcode"]) def test_local_user_does_not_exist(self): @@ -1188,11 +1727,11 @@ def test_local_user_does_not_exist(self): channel = self.make_request( "POST", self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(404, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(404, channel.code, msg=channel.json_body) self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) def test_remote_user(self): @@ -1204,11 +1743,11 @@ def test_remote_user(self): channel = self.make_request( "POST", self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( "This endpoint can only be used with local users", channel.json_body["error"], @@ -1224,11 +1763,11 @@ def test_room_does_not_exist(self): channel = self.make_request( "POST", url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(404, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(404, channel.code, msg=channel.json_body) self.assertEqual("No known servers", channel.json_body["error"]) def test_room_is_not_valid(self): @@ -1241,11 +1780,11 @@ def test_room_is_not_valid(self): channel = self.make_request( "POST", url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( "invalidroom was not legal room ID or room alias", channel.json_body["error"], @@ -1260,11 +1799,11 @@ def test_join_public_room(self): channel = self.make_request( "POST", self.url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(self.public_room_id, channel.json_body["room_id"]) # Validate if user is a member of the room @@ -1274,7 +1813,7 @@ def test_join_public_room(self): "/_matrix/client/r0/joined_rooms", access_token=self.second_tok, ) - self.assertEquals(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEquals(200, channel.code, msg=channel.json_body) self.assertEqual(self.public_room_id, channel.json_body["joined_rooms"][0]) def test_join_private_room_if_not_member(self): @@ -1291,11 +1830,11 @@ def test_join_private_room_if_not_member(self): channel = self.make_request( "POST", url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def test_join_private_room_if_member(self): @@ -1323,7 +1862,7 @@ def test_join_private_room_if_member(self): "/_matrix/client/r0/joined_rooms", access_token=self.admin_user_tok, ) - self.assertEquals(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEquals(200, channel.code, msg=channel.json_body) self.assertEqual(private_room_id, channel.json_body["joined_rooms"][0]) # Join user to room. @@ -1334,10 +1873,10 @@ def test_join_private_room_if_member(self): channel = self.make_request( "POST", url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(private_room_id, channel.json_body["room_id"]) # Validate if user is a member of the room @@ -1347,7 +1886,7 @@ def test_join_private_room_if_member(self): "/_matrix/client/r0/joined_rooms", access_token=self.second_tok, ) - self.assertEquals(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEquals(200, channel.code, msg=channel.json_body) self.assertEqual(private_room_id, channel.json_body["joined_rooms"][0]) def test_join_private_room_if_owner(self): @@ -1364,11 +1903,11 @@ def test_join_private_room_if_owner(self): channel = self.make_request( "POST", url, - content=body.encode(encoding="utf_8"), + content=body, access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual(private_room_id, channel.json_body["room_id"]) # Validate if user is a member of the room @@ -1378,7 +1917,7 @@ def test_join_private_room_if_owner(self): "/_matrix/client/r0/joined_rooms", access_token=self.second_tok, ) - self.assertEquals(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEquals(200, channel.code, msg=channel.json_body) self.assertEqual(private_room_id, channel.json_body["joined_rooms"][0]) def test_context_as_non_admin(self): @@ -1412,9 +1951,7 @@ def test_context_as_non_admin(self): % (room_id, events[midway]["event_id"]), access_token=tok, ) - self.assertEquals( - 403, int(channel.result["code"]), msg=channel.result["body"] - ) + self.assertEquals(403, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) def test_context_as_admin(self): @@ -1444,7 +1981,7 @@ def test_context_as_admin(self): % (room_id, events[midway]["event_id"]), access_token=self.admin_user_tok, ) - self.assertEquals(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEquals(200, channel.code, msg=channel.json_body) self.assertEquals( channel.json_body["event"]["event_id"], events[midway]["event_id"] ) @@ -1503,7 +2040,7 @@ def test_public_room(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Now we test that we can join the room and ban a user. self.helper.join(room_id, self.admin_user, tok=self.admin_user_tok) @@ -1530,7 +2067,7 @@ def test_private_room(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Now we test that we can join the room (we should have received an # invite) and can ban a user. @@ -1556,7 +2093,7 @@ def test_other_user(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(200, channel.code, msg=channel.json_body) # Now we test that we can join the room and ban a user. self.helper.join(room_id, self.second_user_id, tok=self.second_tok) @@ -1594,7 +2131,7 @@ def test_not_enough_power(self): # # (Note we assert the error message to ensure that it's not denied for # some other reason) - self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual( channel.json_body["error"], "No local admin user in room with power to update power levels.", From 69bba2437a5b4ef28ea45e8e4a67f0b16480f5bf Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sun, 31 Oct 2021 22:24:44 +0100 Subject: [PATCH 02/23] newsfile --- changelog.d/11223.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11223.feature diff --git a/changelog.d/11223.feature b/changelog.d/11223.feature new file mode 100644 index 000000000000..55ea693dcd7d --- /dev/null +++ b/changelog.d/11223.feature @@ -0,0 +1 @@ +Add a new version of delete room admin API `DELETE /_synapse/admin/v2/rooms/` to run it in background. Contributed by @dklimpel. \ No newline at end of file From a9f9e3a891788b430667e67b8cd9a1135d2dfed6 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sun, 31 Oct 2021 23:14:39 +0100 Subject: [PATCH 03/23] disable flaky test --- tests/rest/admin/test_room.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 1d0dba23c3cc..066f5eca62bd 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -687,6 +687,8 @@ def test_delete_same_room_twice(self): self.assertEqual(200, status_channel.code, msg=status_channel.json_body) self.assertEqual("complete", status_channel.json_body["status"]) + test_delete_same_room_twice.skip = "Disabled by default because it's flaky" + def test_purge_room_and_block(self): """Test to purge a room and block it. Members will not be moved to a new room and will not receive a message. From 6ddcc8e68b982bc1049cf4d9257c08c09137ed96 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 1 Nov 2021 16:45:51 +0100 Subject: [PATCH 04/23] Fix wrong unit test `test_block_room_and_not_purge` found there: https://github.com/matrix-org/synapse/pull/7964#discussion_r740278700 --- tests/rest/admin/test_room.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 7030dd4a30a2..6b0cc6f9078a 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -784,7 +784,7 @@ def test_block_room_and_not_purge(self): channel = self.make_request( "DELETE", self.url.encode("ascii"), - content={"block": False, "purge": False}, + content={"block": True, "purge": False}, access_token=self.admin_user_tok, ) @@ -807,7 +807,7 @@ def test_block_room_and_not_purge(self): with self.assertRaises(AssertionError): self._is_purged(self.room_id) - self._is_blocked(self.room_id, expect=False) + self._is_blocked(self.room_id, expect=True) self._has_no_members(self.room_id) def test_shutdown_room_consent(self): From cf8b612187c09524eaf750d4fda454aff0e82bbd Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 1 Nov 2021 19:30:02 +0100 Subject: [PATCH 05/23] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- docs/admin_api/rooms.md | 21 +++++++++++---------- synapse/handlers/pagination.py | 4 +++- synapse/handlers/room.py | 2 +- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 70a932525830..44a3f54eed20 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -411,10 +411,10 @@ server admin: see [Admin API](../usage/administration/admin_api). ## Version 1 (old version) -This version works synchronous. That means you get the response if the server has -finised this action. This may take a long time. If you requests the same action -a second time, if the server is not finished the first one, can the server hang up. -This is fixed in Version 2 of this API. The parameters are the same in both APIs. +This version works synchronously. That means you only get the response once the server has +finished the action, which may take a long time. If you request the same action +a second time, and the server has not finished the first one, the second request will block. +This is fixed in version 2 of this API. The parameters are the same in both APIs. This API will become deprecated in the future. The API is: @@ -451,13 +451,14 @@ A response body like the following is returned: } ``` -The parameters and response values have the same expression like in +The parameters and response values have the same format as [version 2](#version-2-new-version) of the API. ## Version 2 (new version) -This version works asynchronous. That means you get the response from server immediately. -The server works on that task in background. You can request the status of this action. +This version works asynchronously, meaning you get the response from server immediately +while the server works on that task in background. You can then request the status of the action +to check if it has completed. The API is: @@ -516,8 +517,8 @@ The JSON body must not be empty. The body must be at least `{}`. ## Status of deleting rooms It is possible to query the status of the background task for deleting rooms. -The status can be queried up to 24 hours after completion of the task -or a restart of Synapse. +The status can be queried up to 24 hours after completion of the task, +or until Synapse is restarted (whichever happens first). The API is: @@ -556,7 +557,7 @@ The following fields are returned in the JSON response body: * `status` - The status will be one of: - `remove members` - The process is removing users from the `room_id`. - - `active` - The process is purging the room from databse. + - `active` - The process is purging the room from database. - `complete` - The process has completed successfully. - `failed` - The process is aborted, an error has occurred. * `result` - An object containing information about the result of shutting down the room. diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 70151622d236..523d30ed9204 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -508,7 +508,9 @@ async def _shutdown_and_purge_room( force_purge: bool = False, ) -> None: """ - Shuts down and purge a room. Moves all local users and room aliases + Shuts down and purges 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 leave the room without any information. After that, the room will be removed from the database. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index b98e80b975c8..56303791334c 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1276,7 +1276,7 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class RoomShutdownHandler: - """This handler shutdown rooms synchronous and is part of the delete room v1 API. + """This handles synchronous room shutdowns and is part of the delete room v1 API. It will become deprecated in the future. The handler for asynchronous shudowns is part of the PaginationHandler. """ From a37b09c1ea4958748213faf94fe68ebff8a73a80 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 2 Nov 2021 08:15:09 +0100 Subject: [PATCH 06/23] update docstrings and mark private fields with `_` and use `run_as_background_process` --- synapse/handlers/pagination.py | 24 +++++++++++++----------- synapse/rest/admin/rooms.py | 24 +++++++++++------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 523d30ed9204..7ce2d7b718eb 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -40,7 +40,7 @@ @attr.s(slots=True, auto_attribs=True) class PurgeStatus: - """Object tracking the status of a purge request + """Object tracking the status of a purge and shut down room request This class contains information on the progress of a purge request, for return by get_purge_status. @@ -58,7 +58,8 @@ class PurgeStatus: STATUS_REMOVE_MEMBERS: "remove members", } - # Tracks whether this request has completed. One of STATUS_{ACTIVE,COMPLETE,FAILED}. + # Tracks whether this request has completed. + # One of STATUS_{ACTIVE,COMPLETE,FAILED, STATUS_REMOVE_MEMBERS}. status: int = STATUS_ACTIVE # Saves the result of an action to give it back to REST API @@ -96,10 +97,10 @@ def __init__(self, hs: "HomeServer"): self.clock = hs.get_clock() self._server_name = hs.hostname - self.room_member_handler = hs.get_room_member_handler() + self._room_member_handler = hs.get_room_member_handler() self._room_creation_handler = hs.get_room_creation_handler() self._replication = hs.get_replication_data_handler() - self.event_creation_handler = hs.get_event_creation_handler() + self._event_creation_handler = hs.get_event_creation_handler() self.pagination_lock = ReadWriteLock() self._purges_in_progress_by_room: Set[str] = set() @@ -508,8 +509,8 @@ async def _shutdown_and_purge_room( force_purge: bool = False, ) -> None: """ - Shuts down and purges a room. - + Shuts down and purges 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 leave the room without any information. @@ -629,7 +630,7 @@ async def _shutdown_and_purge_room( target_requester = create_requester( user_id, authenticated_entity=requester_user_id ) - _, stream_id = await self.room_member_handler.update_membership( + _, stream_id = await self._room_member_handler.update_membership( requester=target_requester, target=target_requester.user, room_id=room_id, @@ -648,13 +649,13 @@ async def _shutdown_and_purge_room( stream_id, ) - await self.room_member_handler.forget( + await self._room_member_handler.forget( target_requester.user, room_id ) # Join users to new room if new_room_user_id: - await self.room_member_handler.update_membership( + await self._room_member_handler.update_membership( requester=target_requester, target=target_requester.user, room_id=new_room_id, @@ -676,7 +677,7 @@ async def _shutdown_and_purge_room( # Send message in new room and move aliases if new_room_user_id: - await self.event_creation_handler.create_and_send_nonmember_event( + await self._event_creation_handler.create_and_send_nonmember_event( room_creator_requester, { "type": "m.room.message", @@ -795,7 +796,8 @@ def start_shutdown_and_purge_room( logger.info("[shutdown_and_purge] starting shutdown room_id %s", room_id) self._purges_by_id[room_id] = PurgeStatus() - run_in_background( + run_as_background_process( + "_shutdown_and_purge_room", self._shutdown_and_purge_room, room_id, requester_user_id, diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 737cd7bab934..abe423dc11dd 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -64,17 +64,16 @@ class RoomRestV2Servlet(RestServlet): PATTERNS = admin_patterns("/rooms/(?P[^/]+)$", "v2") def __init__(self, hs: "HomeServer"): - self.hs = hs - self.auth = hs.get_auth() - self.store = hs.get_datastore() - self.pagination_handler = hs.get_pagination_handler() + self._auth = hs.get_auth() + self._store = hs.get_datastore() + self._pagination_handler = hs.get_pagination_handler() async def on_DELETE( self, request: SynapseRequest, room_id: str ) -> Tuple[int, JsonDict]: - requester = await self.auth.get_user_by_req(request) - await assert_user_is_admin(self.auth, requester.user) + requester = await self._auth.get_user_by_req(request) + await assert_user_is_admin(self._auth, requester.user) content = parse_json_object_from_request(request) @@ -105,10 +104,10 @@ async def on_DELETE( 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): + if not await self._store.get_room(room_id): raise NotFoundError("Unknown room id %s" % (room_id,)) - self.pagination_handler.start_shutdown_and_purge_room( + self._pagination_handler.start_shutdown_and_purge_room( room_id=room_id, new_room_user_id=content.get("new_room_user_id"), new_room_name=content.get("room_name"), @@ -128,20 +127,19 @@ class DeleteRoomStatusRestServlet(RestServlet): PATTERNS = admin_patterns("/rooms/(?P[^/]+)/delete_status$", "v2") def __init__(self, hs: "HomeServer"): - self.hs = hs - self.auth = hs.get_auth() - self.pagination_handler = hs.get_pagination_handler() + self._auth = hs.get_auth() + self._pagination_handler = hs.get_pagination_handler() async def on_GET( self, request: SynapseRequest, room_id: str ) -> Tuple[int, JsonDict]: - await assert_requester_is_admin(self.auth, request) + await assert_requester_is_admin(self._auth, request) if not RoomID.is_valid(room_id): raise SynapseError(400, "%s is not a legal room ID" % (room_id,)) - purge_status = self.pagination_handler.get_purge_status(room_id) + purge_status = self._pagination_handler.get_purge_status(room_id) if purge_status is None: raise NotFoundError("No delete task for room_id '%s' found" % room_id) From 3f9a4f420a584b00fd69b3cbb12e69f6b1436d8c Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 2 Nov 2021 09:19:02 +0100 Subject: [PATCH 07/23] Reuse `RoomShutdownHandler.shutdown_room` --- synapse/handlers/pagination.py | 173 ++++----------------------------- synapse/handlers/room.py | 4 +- 2 files changed, 23 insertions(+), 154 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 7ce2d7b718eb..a66d8d49816a 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -19,14 +19,14 @@ from twisted.python.failure import Failure -from synapse.api.constants import EventTypes, Membership, RoomCreationPreset +from synapse.api.constants import EventTypes, Membership from synapse.api.errors import SynapseError from synapse.api.filtering import Filter from synapse.logging.context import run_in_background from synapse.metrics.background_process_metrics import run_as_background_process from synapse.storage.state import StateFilter from synapse.streams.config import PaginationConfig -from synapse.types import JsonDict, Requester, create_requester +from synapse.types import JsonDict, Requester from synapse.util.async_helpers import ReadWriteLock from synapse.util.stringutils import random_string from synapse.visibility import filter_events_for_client @@ -82,12 +82,6 @@ class PaginationHandler: paginating during a purge. """ - DEFAULT_MESSAGE = ( - "Sharing illegal content on this server is not permitted and rooms in" - " violation will be blocked." - ) - DEFAULT_ROOM_NAME = "Content Violation Notification" - def __init__(self, hs: "HomeServer"): self.hs = hs self.auth = hs.get_auth() @@ -96,11 +90,7 @@ def __init__(self, hs: "HomeServer"): self.state_store = self.storage.state self.clock = hs.get_clock() self._server_name = hs.hostname - - self._room_member_handler = hs.get_room_member_handler() - self._room_creation_handler = hs.get_room_creation_handler() - self._replication = hs.get_replication_data_handler() - self._event_creation_handler = hs.get_event_creation_handler() + self._room_shutdown_handler = hs.get_room_shutdown_handler() self.pagination_lock = ReadWriteLock() self._purges_in_progress_by_room: Set[str] = set() @@ -562,151 +552,25 @@ async def _shutdown_and_purge_room( new_room_id: A string representing the room ID of the new room. """ - if not new_room_name: - new_room_name = self.DEFAULT_ROOM_NAME - if not message: - message = self.DEFAULT_MESSAGE - self._purges_in_progress_by_room.add(room_id) try: with await self.pagination_lock.write(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. - if block: - await self.store.block_room(room_id, requester_user_id) - - if new_room_user_id is not None: - room_creator_requester = create_requester( - new_room_user_id, authenticated_entity=requester_user_id - ) - - info, stream_id = await self._room_creation_handler.create_room( - room_creator_requester, - config={ - "preset": RoomCreationPreset.PUBLIC_CHAT, - "name": new_room_name, - "power_level_content_override": {"users_default": -10}, - }, - ratelimit=False, - ) - new_room_id = info["room_id"] - - logger.info( - "[shutdown_and_purge] Shutting down room %r, joining to new room: %r", - room_id, - new_room_id, - ) - - # We now wait for the create room to come back in via replication so - # that we can assume that all the joins/invites have propagated before - # we try and auto join below. - await self._replication.wait_for_stream_position( - self.hs.config.worker.events_shard_config.get_instance( - new_room_id - ), - "events", - stream_id, - ) - else: - new_room_id = None - logger.info("[shutdown_and_purge] Shutting down room %r", room_id) - self._purges_by_id[room_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS - - users = await self.store.get_users_in_room(room_id) - kicked_users = [] - failed_to_kick_users = [] - for user_id in users: - if not self.hs.is_mine_id(user_id): - continue - - logger.info( - "[shutdown_and_purge] Kicking %r from %r...", user_id, room_id - ) - - try: - # Kick users from room - target_requester = create_requester( - user_id, authenticated_entity=requester_user_id - ) - _, stream_id = await self._room_member_handler.update_membership( - requester=target_requester, - target=target_requester.user, - room_id=room_id, - action=Membership.LEAVE, - content={}, - ratelimit=False, - require_consent=False, - ) - - # Wait for leave to come in over replication before trying to forget. - await self._replication.wait_for_stream_position( - self.hs.config.worker.events_shard_config.get_instance( - room_id - ), - "events", - stream_id, - ) - - await self._room_member_handler.forget( - target_requester.user, room_id - ) - - # Join users to new room - if new_room_user_id: - await self._room_member_handler.update_membership( - requester=target_requester, - target=target_requester.user, - room_id=new_room_id, - action=Membership.JOIN, - content={}, - ratelimit=False, - require_consent=False, - ) - - kicked_users.append(user_id) - except Exception: - logger.exception( - "[shutdown_and_purge] Failed to leave old room and join new room for %r", - user_id, - ) - failed_to_kick_users.append(user_id) - + self._purges_by_id[ + room_id + ].result = await self._room_shutdown_handler.shutdown_room( + room_id=room_id, + requester_user_id=requester_user_id, + new_room_user_id=new_room_user_id, + new_room_name=new_room_name, + message=message, + block=block, + ) self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE - # Send message in new room and move aliases - if new_room_user_id: - await self._event_creation_handler.create_and_send_nonmember_event( - room_creator_requester, - { - "type": "m.room.message", - "content": {"body": message, "msgtype": "m.text"}, - "room_id": new_room_id, - "sender": new_room_user_id, - }, - ratelimit=False, - ) - - aliases_for_room = await self.store.get_aliases_for_room(room_id) - - await self.store.update_aliases_for_room( - room_id, new_room_id, requester_user_id - ) - else: - aliases_for_room = [] - - self._purges_by_id[room_id].result = { - "kicked_users": kicked_users, - "failed_to_kick_users": failed_to_kick_users, - "local_aliases": aliases_for_room, - "new_room_id": new_room_id, - } - if purge: - logger.info( - "[shutdown_and_purge] starting purge room_id %s", room_id - ) + logger.info("starting purge room_id %s", room_id) self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE # first check that we have no users in this room @@ -721,12 +585,12 @@ async def _shutdown_and_purge_room( await self.storage.purge_events.purge_room(room_id) - logger.info("[shutdown_and_purge] complete") + logger.info("complete") self._purges_by_id[room_id].status = PurgeStatus.STATUS_COMPLETE except Exception: f = Failure() logger.error( - "[shutdown_and_purge] failed", + "failed", exc_info=(f.type, f.value, f.getTracebackObject()), # type: ignore ) self._purges_by_id[room_id].status = PurgeStatus.STATUS_FAILED @@ -785,6 +649,9 @@ def start_shutdown_and_purge_room( 400, "History purge already in progress for %s" % (room_id,) ) + # This check is double to `RoomShutdownHandler.shutdown_room` + # But here the requester get a direct response / error with HTTP request + # and do not have to check the purge status if new_room_user_id is not None: if not self.hs.is_mine_id(new_room_user_id): raise SynapseError( @@ -793,7 +660,7 @@ def start_shutdown_and_purge_room( # we log the purge_id here so that it can be tied back to the # request id in the log lines. - logger.info("[shutdown_and_purge] starting shutdown room_id %s", room_id) + logger.info("[_shutdown_and_purge_room] starting shutdown room_id %s", room_id) self._purges_by_id[room_id] = PurgeStatus() run_as_background_process( diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 56303791334c..7a26ad4ef8b9 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1278,7 +1278,9 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class RoomShutdownHandler: """This handles synchronous room shutdowns and is part of the delete room v1 API. It will become deprecated in the future. - The handler for asynchronous shudowns is part of the PaginationHandler. + + The handler for asynchronous shudowns is part of the `PaginationHandler`. + If this handler is removed, `shutdown_room` must to be migrated to `PaginationHandler` """ DEFAULT_MESSAGE = ( From 2f1f483942e4632ae1ecb2fc2e8ef40d89c5d75e Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 3 Nov 2021 10:32:15 +0100 Subject: [PATCH 08/23] try flaky unit test --- tests/rest/admin/test_room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 8dc45e635673..ec941b3752b7 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -680,7 +680,7 @@ def test_delete_same_room_twice(self): self.assertEqual(200, status_channel.code, msg=status_channel.json_body) self.assertEqual("complete", status_channel.json_body["status"]) - test_delete_same_room_twice.skip = "Disabled by default because it's flaky" + # test_delete_same_room_twice.skip = "Disabled by default because it's flaky" def test_purge_room_and_block(self): """Test to purge a room and block it. From 60ef11eccb901d1398bc0b1a3d86b2397e97dcbf Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 4 Nov 2021 16:20:44 +0100 Subject: [PATCH 09/23] rework request of purge status --- docs/admin_api/rooms.md | 81 ++++++++++----- synapse/handlers/pagination.py | 62 +++++++++--- synapse/rest/admin/rooms.py | 20 +++- tests/rest/admin/test_room.py | 179 +++++++++++++++++++++------------ 4 files changed, 235 insertions(+), 107 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index ed6c44de85a0..58a6e69e7eaa 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -461,6 +461,8 @@ The parameters and response values have the same format as ## Version 2 (new version) +**Note**: This API is new, experimental and "subject to change". + This version works asynchronously, meaning you get the response from server immediately while the server works on that task in background. You can then request the status of the action to check if it has completed. @@ -483,10 +485,13 @@ with a body of: } ``` -An empty JSON dict is returned. +The API starts the shut down and purge running, and returns immediately with a JSON body with +a purge id: ```json -{} +{ + "purge_id": "" +} ``` **Parameters** @@ -521,10 +526,16 @@ The JSON body must not be empty. The body must be at least `{}`. ## Status of deleting rooms +**Note**: This API is new, experimental and "subject to change". + It is possible to query the status of the background task for deleting rooms. The status can be queried up to 24 hours after completion of the task, or until Synapse is restarted (whichever happens first). +With this API you can get the status of all tasks the last 24h for this `room_id`. +If you want only get the status of one specific task, you can also use +the [Purge status query API](purge_history_api.md#Purge-status-query). + The API is: ``` @@ -535,18 +546,32 @@ A response body like the following is returned: ```json { - "status": "active", - "result": { - "kicked_users": [ - "@foobar:example.com" - ], - "failed_to_kick_users": [], - "local_aliases": [ - "#badroom:example.com", - "#evilsaloon:example.com" - ], - "new_room_id": "!newroomid:example.com" - } + "delete_status": [ + { + "purge_id": "purgeid1", + "status": "failed", + "result": { + "kicked_users": [], + "failed_to_kick_users": [], + "local_aliases": [], + "new_room_id": null + } + }, { + "purge_id": "purgeid2", + "status": "active", + "result": { + "kicked_users": [ + "@foobar:example.com" + ], + "failed_to_kick_users": [], + "local_aliases": [ + "#badroom:example.com", + "#evilsaloon:example.com" + ], + "new_room_id": "!newroomid:example.com" + } + } + ] } ``` @@ -560,19 +585,21 @@ The following parameters should be set in the URL: The following fields are returned in the JSON response body: -* `status` - The status will be one of: - - `remove members` - The process is removing users from the `room_id`. - - `active` - The process is purging the room from database. - - `complete` - The process has completed successfully. - - `failed` - The process is aborted, an error has occurred. -* `result` - An object containing information about the result of shutting down the room. - *Note:* The result is shown after removing the room members. The delete process can - still be running. Please pay attention to the `status`. - - `kicked_users` - An array of users (`user_id`) that were kicked. - - `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. - - `new_room_id` - A string representing the room ID of the new room. +- `delete_status` - An array of objects, each containing information about one task. + Task objects contain the following fields: + - `status` - The status will be one of: + - `remove members` - The process is removing users from the `room_id`. + - `active` - The process is purging the room from database. + - `complete` - The process has completed successfully. + - `failed` - The process is aborted, an error has occurred. + - `result` - An object containing information about the result of shutting down the room. + *Note:* The result is shown after removing the room members. The delete process can + still be running. Please pay attention to the `status`. + - `kicked_users` - An array of users (`user_id`) that were kicked. + - `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. + - `new_room_id` - A string representing the room ID of the new room. ## Undoing room deletions diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index a66d8d49816a..501979314746 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import TYPE_CHECKING, Any, Dict, Optional, Set +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set import attr @@ -82,6 +82,9 @@ class PaginationHandler: paginating during a purge. """ + # remove the purge from the list 24 hours after it completes + CLEAR_PURGE_TIME = 3600 * 24 + def __init__(self, hs: "HomeServer"): self.hs = hs self.auth = hs.get_auth() @@ -96,6 +99,9 @@ def __init__(self, hs: "HomeServer"): self._purges_in_progress_by_room: Set[str] = set() # map from purge id to PurgeStatus self._purges_by_id: Dict[str, PurgeStatus] = {} + # map from room id to purge ids + # Dict[`room_id`, List[`purge_id`]] + self._purges_by_room: Dict[str, List[str]] = {} self._event_serializer = hs.get_event_client_serializer() self._retention_default_max_lifetime = ( @@ -315,7 +321,9 @@ async def _purge_history( def clear_purge() -> None: del self._purges_by_id[purge_id] - self.hs.get_reactor().callLater(24 * 3600, clear_purge) + self.hs.get_reactor().callLater( + PaginationHandler.CLEAR_PURGE_TIME, clear_purge + ) def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: """Get the current status of an active purge @@ -325,6 +333,14 @@ def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: """ return self._purges_by_id.get(purge_id) + def get_purge_ids_by_room(self, room_id: str) -> Optional[List[str]]: + """Get all active purge ids by room + + Args: + room_id: room_id that is purged + """ + return self._purges_by_room.get(room_id) + async def purge_room(self, room_id: str, force: bool = False) -> None: """Purge the given room from the database. This function is part the delete room v1 API. @@ -489,6 +505,7 @@ async def get_messages( async def _shutdown_and_purge_room( self, + purge_id: str, room_id: str, requester_user_id: str, new_room_user_id: Optional[str] = None, @@ -515,6 +532,7 @@ async def _shutdown_and_purge_room( aliases to the new room. Users on other servers will be unaffected. Args: + purge_id: The id for this purge room_id: The ID of the room to shut down. requester_user_id: User who requested the action and put the room on the @@ -556,9 +574,9 @@ async def _shutdown_and_purge_room( try: with await self.pagination_lock.write(room_id): - self._purges_by_id[room_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS self._purges_by_id[ - room_id + purge_id ].result = await self._room_shutdown_handler.shutdown_room( room_id=room_id, requester_user_id=requester_user_id, @@ -567,11 +585,11 @@ async def _shutdown_and_purge_room( message=message, block=block, ) - self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_ACTIVE if purge: logger.info("starting purge room_id %s", room_id) - self._purges_by_id[room_id].status = PurgeStatus.STATUS_ACTIVE + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_ACTIVE # first check that we have no users in this room if not force_purge: @@ -586,22 +604,28 @@ async def _shutdown_and_purge_room( await self.storage.purge_events.purge_room(room_id) logger.info("complete") - self._purges_by_id[room_id].status = PurgeStatus.STATUS_COMPLETE + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_COMPLETE except Exception: f = Failure() logger.error( "failed", exc_info=(f.type, f.value, f.getTracebackObject()), # type: ignore ) - self._purges_by_id[room_id].status = PurgeStatus.STATUS_FAILED + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_FAILED finally: self._purges_in_progress_by_room.discard(room_id) # remove the purge from the list 24 hours after it completes def clear_purge() -> None: - del self._purges_by_id[room_id] + del self._purges_by_id[purge_id] + self._purges_by_room[room_id].remove(purge_id) + if not self._purges_by_room[room_id]: + del self._purges_by_room[room_id] - self.hs.get_reactor().callLater(24 * 3600, clear_purge) + self.hs.get_reactor().callLater( + PaginationHandler.CLEAR_PURGE_TIME, + clear_purge, + ) def start_shutdown_and_purge_room( self, @@ -613,7 +637,7 @@ def start_shutdown_and_purge_room( block: bool = False, purge: bool = True, force_purge: bool = False, - ) -> None: + ) -> str: """Start off shut down and purge on a room. Args: @@ -643,6 +667,9 @@ def start_shutdown_and_purge_room( force_purge: If set to `true`, the room will be purged from database also if it fails to remove some users from room. + + Returns: + unique ID for this purge transaction. """ if room_id in self._purges_in_progress_by_room: raise SynapseError( @@ -658,14 +685,22 @@ def start_shutdown_and_purge_room( 400, "User must be our own: %s" % (new_room_user_id,) ) + purge_id = random_string(16) + # we log the purge_id here so that it can be tied back to the # request id in the log lines. - logger.info("[_shutdown_and_purge_room] starting shutdown room_id %s", room_id) + logger.info( + "[_shutdown_and_purge_room] starting shutdown room_id %s with purge_id %s", + room_id, + purge_id, + ) - self._purges_by_id[room_id] = PurgeStatus() + self._purges_by_id[purge_id] = PurgeStatus() + self._purges_by_room.setdefault(room_id, []).append(purge_id) run_as_background_process( "_shutdown_and_purge_room", self._shutdown_and_purge_room, + purge_id, room_id, requester_user_id, new_room_user_id, @@ -675,3 +710,4 @@ def start_shutdown_and_purge_room( purge, force_purge, ) + return purge_id diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index abe423dc11dd..5b91c2be37cd 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -107,7 +107,7 @@ async def on_DELETE( if not await self._store.get_room(room_id): raise NotFoundError("Unknown room id %s" % (room_id,)) - self._pagination_handler.start_shutdown_and_purge_room( + purge_id = self._pagination_handler.start_shutdown_and_purge_room( room_id=room_id, new_room_user_id=content.get("new_room_user_id"), new_room_name=content.get("room_name"), @@ -118,7 +118,7 @@ async def on_DELETE( force_purge=force_purge, ) - return 200, {} + return 200, {"purge_id": purge_id} class DeleteRoomStatusRestServlet(RestServlet): @@ -139,11 +139,21 @@ async def on_GET( if not RoomID.is_valid(room_id): raise SynapseError(400, "%s is not a legal room ID" % (room_id,)) - purge_status = self._pagination_handler.get_purge_status(room_id) - if purge_status is None: + purge_ids = self._pagination_handler.get_purge_ids_by_room(room_id) + if purge_ids is None: raise NotFoundError("No delete task for room_id '%s' found" % room_id) - return 200, purge_status.asdict_with_result() + response = [] + for purge_id in purge_ids: + purge = self._pagination_handler.get_purge_status(purge_id) + if purge: + response += [ + { + "purge_id": purge_id, + **purge.asdict_with_result(), + } + ] + return 200, {"delete_status": response} class ListRoomRestServlet(RestServlet): diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index ec941b3752b7..ce640a10309a 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -22,9 +22,11 @@ import synapse.rest.admin from synapse.api.constants import EventTypes, Membership from synapse.api.errors import Codes +from synapse.handlers.pagination import PaginationHandler from synapse.rest.client import directory, events, login, room from tests import unittest +from tests.server import FakeChannel """Tests admin REST events for /rooms paths.""" @@ -542,6 +544,8 @@ def test_new_room_user_does_not_exist(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -549,12 +553,7 @@ def test_new_room_user_does_not_exist(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertIn("new_room_id", channel.json_body["result"]) - self.assertIn("kicked_users", channel.json_body["result"]) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user, expect_new_room=True) def test_new_room_user_is_not_local(self): """ @@ -607,14 +606,50 @@ def test_purge_is_not_bool(self): def test_delete_expired_status(self): """Test that the task status is removed after expiration.""" + # first task, do not purge, that we can create a second task channel = self.make_request( "DELETE", self.url.encode("ascii"), - content={}, + content={"purge": False}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id1 = channel.json_body["purge_id"] + + # go ahead + self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) + + # second task + channel = self.make_request( + "DELETE", + self.url.encode("ascii"), + content={"purge": True}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id2 = channel.json_body["purge_id"] + + # get status + channel = self.make_request( + "GET", + self.url_status, access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(2, len(channel.json_body["delete_status"])) + self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) + self.assertEqual("complete", channel.json_body["delete_status"][1]["status"]) + self.assertEqual(purge_id1, channel.json_body["delete_status"][0]["purge_id"]) + self.assertEqual(purge_id2, channel.json_body["delete_status"][1]["purge_id"]) + + # get status after more than clearing time for first task + # second task is not cleared + self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) channel = self.make_request( "GET", @@ -623,10 +658,12 @@ def test_delete_expired_status(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) + self.assertEqual(1, len(channel.json_body["delete_status"])) + self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) + self.assertEqual(purge_id2, channel.json_body["delete_status"][0]["purge_id"]) - # get status after more than 24 hours - self.reactor.advance(24 * 3600 + 10) + # get status after more than clearing time for all tasks + self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) channel = self.make_request( "GET", @@ -670,6 +707,7 @@ def test_delete_same_room_twice(self): # get result of first call first_channel.await_result() self.assertEqual(200, first_channel.code, msg=first_channel.json_body) + self.assertIn("purge_id", first_channel.json_body) # check status after finish the task status_channel = self.make_request( @@ -677,10 +715,12 @@ def test_delete_same_room_twice(self): self.url_status, access_token=self.admin_user_tok, ) - self.assertEqual(200, status_channel.code, msg=status_channel.json_body) - self.assertEqual("complete", status_channel.json_body["status"]) - - # test_delete_same_room_twice.skip = "Disabled by default because it's flaky" + self._test_result( + status_channel, + first_channel.json_body["purge_id"], + self.other_user, + expect_new_room=True, + ) def test_purge_room_and_block(self): """Test to purge a room and block it. @@ -704,6 +744,8 @@ def test_purge_room_and_block(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -711,14 +753,7 @@ def test_purge_room_and_block(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertIsNone(channel.json_body["result"]["new_room_id"]) - self.assertEqual( - self.other_user, channel.json_body["result"]["kicked_users"][0] - ) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=True) @@ -746,6 +781,8 @@ def test_purge_room_and_not_block(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -753,14 +790,7 @@ def test_purge_room_and_not_block(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertIsNone(channel.json_body["result"]["new_room_id"]) - self.assertEqual( - self.other_user, channel.json_body["result"]["kicked_users"][0] - ) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=False) @@ -789,6 +819,8 @@ def test_block_room_and_not_purge(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -796,14 +828,7 @@ def test_block_room_and_not_purge(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertIsNone(channel.json_body["result"]["new_room_id"]) - self.assertEqual( - self.other_user, channel.json_body["result"]["kicked_users"][0] - ) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user) with self.assertRaises(AssertionError): self._is_purged(self.room_id) @@ -846,6 +871,8 @@ def test_shutdown_room_consent(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -853,18 +880,12 @@ def test_shutdown_room_consent(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertEqual( - self.other_user, channel.json_body["result"]["kicked_users"][0] - ) - self.assertIn("new_room_id", channel.json_body["result"]) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user, expect_new_room=True) # Test that member has moved to new room self._is_member( - room_id=channel.json_body["result"]["new_room_id"], user_id=self.other_user + room_id=channel.json_body["delete_status"][0]["result"]["new_room_id"], + user_id=self.other_user, ) self._is_purged(self.room_id) @@ -903,6 +924,8 @@ def test_shutdown_room_block_peek(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] channel = self.make_request( "GET", @@ -910,18 +933,12 @@ def test_shutdown_room_block_peek(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual("complete", channel.json_body["status"]) - self.assertEqual( - self.other_user, channel.json_body["result"]["kicked_users"][0] - ) - self.assertIn("new_room_id", channel.json_body["result"]) - self.assertIn("failed_to_kick_users", channel.json_body["result"]) - self.assertIn("local_aliases", channel.json_body["result"]) + self._test_result(channel, purge_id, self.other_user, expect_new_room=True) # Test that member has moved to new room self._is_member( - room_id=channel.json_body["result"]["new_room_id"], user_id=self.other_user + room_id=channel.json_body["delete_status"][0]["result"]["new_room_id"], + user_id=self.other_user, ) self._is_purged(self.room_id) @@ -930,7 +947,7 @@ def test_shutdown_room_block_peek(self): # Assert we can no longer peek into the room self._assert_peek(self.room_id, expect_code=403) - def _is_blocked(self, room_id, expect=True): + def _is_blocked(self, room_id: str, expect: bool = True) -> None: """Assert that the room is blocked or not""" d = self.store.is_room_blocked(room_id) if expect: @@ -938,17 +955,17 @@ def _is_blocked(self, room_id, expect=True): else: self.assertIsNone(self.get_success(d)) - def _has_no_members(self, room_id): + def _has_no_members(self, room_id: str) -> None: """Assert there is now no longer anyone in the room""" users_in_room = self.get_success(self.store.get_users_in_room(room_id)) self.assertEqual([], users_in_room) - def _is_member(self, room_id, user_id): + def _is_member(self, room_id: str, user_id: str) -> None: """Test that user is member of the room""" users_in_room = self.get_success(self.store.get_users_in_room(room_id)) self.assertIn(user_id, users_in_room) - def _is_purged(self, room_id): + def _is_purged(self, room_id: str) -> None: """Test that the following tables have been purged of all rows related to the room.""" for table in PURGE_TABLES: count = self.get_success( @@ -962,7 +979,7 @@ def _is_purged(self, room_id): self.assertEqual(count, 0, msg=f"Rows not purged in {table}") - def _assert_peek(self, room_id, expect_code): + def _assert_peek(self, room_id: str, expect_code: int) -> None: """Assert that the admin user can (or cannot) peek into the room.""" url = f"rooms/{room_id}/initialSync" @@ -977,6 +994,44 @@ def _assert_peek(self, room_id, expect_code): ) self.assertEqual(expect_code, channel.code, msg=channel.json_body) + def _test_result( + self, + channel: FakeChannel, + purge_id, + kicked_user: str, + expect_new_room: bool = False, + ) -> None: + """ + Test that the result is the expected + + Args: + channel: channel that should validated + purge_id: id of this purge + kicked_user: a user_id which is kicked from the room + expect_new_room: if we expect that a new room was created + """ + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(1, len(channel.json_body["delete_status"])) + self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) + self.assertEqual(purge_id, channel.json_body["delete_status"][0]["purge_id"]) + self.assertEqual( + kicked_user, + channel.json_body["delete_status"][0]["result"]["kicked_users"][0], + ) + self.assertIn( + "failed_to_kick_users", channel.json_body["delete_status"][0]["result"] + ) + self.assertIn("local_aliases", channel.json_body["delete_status"][0]["result"]) + + if expect_new_room: + self.assertIsNotNone( + channel.json_body["delete_status"][0]["result"]["new_room_id"] + ) + else: + self.assertIsNone( + channel.json_body["delete_status"][0]["result"]["new_room_id"] + ) + class RoomTestCase(unittest.HomeserverTestCase): """Test /room admin API.""" @@ -1013,7 +1068,7 @@ def test_list_rooms(self): ) # Check request completed successfully - self.assertEqual(200, int(channel.code), msg=channel.json_body) + self.assertEqual(200, channel.code, msg=channel.json_body) # Check that response json body contains a "rooms" key self.assertTrue( From 76a62d510b3892d1d716a54abc57b5d39ed63598 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 8 Nov 2021 15:50:00 +0100 Subject: [PATCH 10/23] add error message if delete fails --- docs/admin_api/rooms.md | 2 ++ synapse/handlers/pagination.py | 11 +++++++++-- tests/rest/admin/test_room.py | 3 ++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 58a6e69e7eaa..b977295aedab 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -550,6 +550,7 @@ A response body like the following is returned: { "purge_id": "purgeid1", "status": "failed", + "error": "error message", "result": { "kicked_users": [], "failed_to_kick_users": [], @@ -592,6 +593,7 @@ The following fields are returned in the JSON response body: - `active` - The process is purging the room from database. - `complete` - The process has completed successfully. - `failed` - The process is aborted, an error has occurred. + - `error` - A string that shows an error message if `status` is `failed`. Otherwise this field is hidden. - `result` - An object containing information about the result of shutting down the room. *Note:* The result is shown after removing the room members. The delete process can still be running. Please pay attention to the `status`. diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 501979314746..315b3326a19e 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -59,9 +59,12 @@ class PurgeStatus: } # Tracks whether this request has completed. - # One of STATUS_{ACTIVE,COMPLETE,FAILED, STATUS_REMOVE_MEMBERS}. + # One of STATUS_{ACTIVE,COMPLETE,FAILED,REMOVE_MEMBERS}. status: int = STATUS_ACTIVE + # Save the error message if an error occurs + error: str = "" + # Saves the result of an action to give it back to REST API result: Dict = {} @@ -69,10 +72,13 @@ def asdict(self) -> JsonDict: return {"status": PurgeStatus.STATUS_TEXT[self.status]} def asdict_with_result(self) -> JsonDict: - return { + ret = { "status": PurgeStatus.STATUS_TEXT[self.status], "result": self.result, } + if self.error: + ret["error"] = self.error + return ret class PaginationHandler: @@ -612,6 +618,7 @@ async def _shutdown_and_purge_room( exc_info=(f.type, f.value, f.getTracebackObject()), # type: ignore ) self._purges_by_id[purge_id].status = PurgeStatus.STATUS_FAILED + self._purges_by_id[purge_id].error = f.getErrorMessage() finally: self._purges_in_progress_by_room.discard(room_id) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index ce640a10309a..ef9d77748997 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -997,7 +997,7 @@ def _assert_peek(self, room_id: str, expect_code: int) -> None: def _test_result( self, channel: FakeChannel, - purge_id, + purge_id: str, kicked_user: str, expect_new_room: bool = False, ) -> None: @@ -1022,6 +1022,7 @@ def _test_result( "failed_to_kick_users", channel.json_body["delete_status"][0]["result"] ) self.assertIn("local_aliases", channel.json_body["delete_status"][0]["result"]) + self.assertNotIn("error", channel.json_body["delete_status"][0]) if expect_new_room: self.assertIsNotNone( From 7de0bf1705581055cc70665ef7676a1fe8437859 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 8 Nov 2021 15:53:19 +0100 Subject: [PATCH 11/23] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/pagination.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 315b3326a19e..d4f9e7972a96 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -40,10 +40,10 @@ @attr.s(slots=True, auto_attribs=True) class PurgeStatus: - """Object tracking the status of a purge and shut down room request + """Object tracking the status of a purge events/delete room request - This class contains information on the progress of a purge request, for - return by get_purge_status. + This class contains information on the progress of a purge events/delete room request, for + return by get_purge_status or delete_status. """ STATUS_ACTIVE = 0 @@ -339,7 +339,7 @@ def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: """ return self._purges_by_id.get(purge_id) - def get_purge_ids_by_room(self, room_id: str) -> Optional[List[str]]: + def get_purge_ids_by_room(self, room_id: str) -> Optional[Collection[str]]: """Get all active purge ids by room Args: @@ -541,7 +541,7 @@ async def _shutdown_and_purge_room( purge_id: The id for this purge room_id: The ID of the room to shut down. requester_user_id: - User who requested the action and put the room on the + User who requested the action. Will be recorded as putting the room on the blocking list. new_room_user_id: If set, a new room will be created with this user ID @@ -697,7 +697,7 @@ def start_shutdown_and_purge_room( # we log the purge_id here so that it can be tied back to the # request id in the log lines. logger.info( - "[_shutdown_and_purge_room] starting shutdown room_id %s with purge_id %s", + "starting shutdown room_id %s with purge_id %s", room_id, purge_id, ) @@ -705,7 +705,7 @@ def start_shutdown_and_purge_room( self._purges_by_id[purge_id] = PurgeStatus() self._purges_by_room.setdefault(room_id, []).append(purge_id) run_as_background_process( - "_shutdown_and_purge_room", + "shutdown_and_purge_room", self._shutdown_and_purge_room, purge_id, room_id, From a61895887963f2015dbeeb1f97e3e325040c81c8 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 8 Nov 2021 17:18:07 +0100 Subject: [PATCH 12/23] add simple unit test for `/purge_history_status` - update doc - remove `asdict_with_result` - reuse `_clear_purge` - remove `run_in_background` - add normal purges to task list --- docs/admin_api/purge_history_api.md | 1 + synapse/handlers/pagination.py | 49 +++++++++++++++++------------ synapse/rest/admin/__init__.py | 4 ++- synapse/rest/admin/rooms.py | 2 +- tests/rest/admin/test_admin.py | 49 +++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 22 deletions(-) diff --git a/docs/admin_api/purge_history_api.md b/docs/admin_api/purge_history_api.md index bd29e29ab8b4..dd43cfc35eca 100644 --- a/docs/admin_api/purge_history_api.md +++ b/docs/admin_api/purge_history_api.md @@ -69,6 +69,7 @@ This API returns a JSON body like the following: ``` The status will be one of `active`, `complete`, or `failed`. +If `status` is `failed` there will be a string `error` with the error message. ## Reclaim disk space (Postgres) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index d4f9e7972a96..7d2063562d32 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -13,7 +13,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, Set +from typing import TYPE_CHECKING, Any, Collection, Dict, List, Optional, Set import attr @@ -22,7 +22,6 @@ from synapse.api.constants import EventTypes, Membership from synapse.api.errors import SynapseError from synapse.api.filtering import Filter -from synapse.logging.context import run_in_background from synapse.metrics.background_process_metrics import run_as_background_process from synapse.storage.state import StateFilter from synapse.streams.config import PaginationConfig @@ -69,9 +68,6 @@ class PurgeStatus: result: Dict = {} def asdict(self) -> JsonDict: - return {"status": PurgeStatus.STATUS_TEXT[self.status]} - - def asdict_with_result(self) -> JsonDict: ret = { "status": PurgeStatus.STATUS_TEXT[self.status], "result": self.result, @@ -290,8 +286,14 @@ def start_purge_history( logger.info("[purge] starting purge_id %s", purge_id) self._purges_by_id[purge_id] = PurgeStatus() - run_in_background( - self._purge_history, purge_id, room_id, token, delete_local_events + self._purges_by_room.setdefault(room_id, []).append(purge_id) + run_as_background_process( + "purge_history", + self._purge_history, + purge_id, + room_id, + token, + delete_local_events, ) return purge_id @@ -323,12 +325,11 @@ async def _purge_history( finally: self._purges_in_progress_by_room.discard(room_id) - # remove the purge from the list 24 hours after it completes - def clear_purge() -> None: - del self._purges_by_id[purge_id] - self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_TIME, clear_purge + PaginationHandler.CLEAR_PURGE_TIME, + self._clear_purge, + purge_id, + room_id, ) def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: @@ -622,16 +623,11 @@ async def _shutdown_and_purge_room( finally: self._purges_in_progress_by_room.discard(room_id) - # remove the purge from the list 24 hours after it completes - def clear_purge() -> None: - del self._purges_by_id[purge_id] - self._purges_by_room[room_id].remove(purge_id) - if not self._purges_by_room[room_id]: - del self._purges_by_room[room_id] - self.hs.get_reactor().callLater( PaginationHandler.CLEAR_PURGE_TIME, - clear_purge, + self._clear_purge, + purge_id, + room_id, ) def start_shutdown_and_purge_room( @@ -718,3 +714,16 @@ def start_shutdown_and_purge_room( force_purge, ) return purge_id + + def _clear_purge(self, purge_id: str, room_id: str) -> None: + """ + Remove the purge from the list 24 hours after it completes + + Args: + purge_id: The id for this purge + room_id: The ID of the room to shut down. + """ + del self._purges_by_id[purge_id] + self._purges_by_room[room_id].remove(purge_id) + if not self._purges_by_room[room_id]: + del self._purges_by_room[room_id] diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index fffa2cd2d8b3..c4e8ace361cf 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -194,7 +194,9 @@ async def on_GET( if purge_status is None: raise NotFoundError("purge id '%s' not found" % purge_id) - return 200, purge_status.asdict() + response = purge_status.asdict() + del response["result"] + return 200, response ######################################################################################## diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 5b91c2be37cd..a0f0a3b5f1e2 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -150,7 +150,7 @@ async def on_GET( response += [ { "purge_id": purge_id, - **purge.asdict_with_result(), + **purge.asdict(), } ] return 200, {"delete_status": response} diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 192073c5203f..884d4fdf6d42 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -474,3 +474,52 @@ def test_cannot_quarantine_safe_media(self): % server_and_media_id_2 ), ) + + +class PurgeHistoryTestCase(unittest.HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + self.other_user = self.register_user("user", "pass") + self.other_user_tok = self.login("user", "pass") + + self.room_id = self.helper.create_room_as( + self.other_user, tok=self.other_user_tok + ) + self.url = f"/_synapse/admin/v1/purge_history/{self.room_id}" + self.url_status = "/_synapse/admin/v1/purge_history_status/" + + def test_purge_history(self): + """ + Simple test of purge history API. + Test only that is is possible to call, get status 200 and purge_id. + """ + + channel = self.make_request( + "POST", + self.url, + content={"delete_local_events": True, "purge_up_to_ts": 0}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertIn("purge_id", channel.json_body) + purge_id = channel.json_body["purge_id"] + + # get status + channel = self.make_request( + "GET", + self.url_status + purge_id, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual("complete", channel.json_body["status"]) + self.assertNotIn("result", channel.json_body) From 4f42348848a163041aca6ee3ddaf7cb3df596912 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 8 Nov 2021 18:43:38 +0100 Subject: [PATCH 13/23] rename response dicts --- docs/admin_api/rooms.md | 18 +++++++++------- synapse/handlers/pagination.py | 3 ++- synapse/rest/admin/__init__.py | 2 +- synapse/rest/admin/rooms.py | 2 +- tests/rest/admin/test_room.py | 38 +++++++++++++++++----------------- 5 files changed, 33 insertions(+), 30 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index b977295aedab..1c009988b696 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -531,6 +531,7 @@ The JSON body must not be empty. The body must be at least `{}`. It is possible to query the status of the background task for deleting rooms. The status can be queried up to 24 hours after completion of the task, or until Synapse is restarted (whichever happens first). +The result also includes the tasks that have been performed by retention policy. With this API you can get the status of all tasks the last 24h for this `room_id`. If you want only get the status of one specific task, you can also use @@ -546,12 +547,12 @@ A response body like the following is returned: ```json { - "delete_status": [ + "results": [ { "purge_id": "purgeid1", "status": "failed", "error": "error message", - "result": { + "shutdown_room": { "kicked_users": [], "failed_to_kick_users": [], "local_aliases": [], @@ -560,7 +561,7 @@ A response body like the following is returned: }, { "purge_id": "purgeid2", "status": "active", - "result": { + "shutdown_room": { "kicked_users": [ "@foobar:example.com" ], @@ -586,17 +587,18 @@ The following parameters should be set in the URL: The following fields are returned in the JSON response body: -- `delete_status` - An array of objects, each containing information about one task. +- `results` - An array of objects, each containing information about one task. Task objects contain the following fields: - `status` - The status will be one of: - `remove members` - The process is removing users from the `room_id`. - `active` - The process is purging the room from database. - `complete` - The process has completed successfully. - `failed` - The process is aborted, an error has occurred. - - `error` - A string that shows an error message if `status` is `failed`. Otherwise this field is hidden. - - `result` - An object containing information about the result of shutting down the room. - *Note:* The result is shown after removing the room members. The delete process can - still be running. Please pay attention to the `status`. + - `error` - A string that shows an error message if `status` is `failed`. + Otherwise this field is hidden. + - `shutdown_room` - An object containing information about the result of shutting down the room. + *Note:* The result is shown after removing the room members. + The delete process can still be running. Please pay attention to the `status`. - `kicked_users` - An array of users (`user_id`) that were kicked. - `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 diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 7d2063562d32..ecb4bc8e50ba 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -70,7 +70,7 @@ class PurgeStatus: def asdict(self) -> JsonDict: ret = { "status": PurgeStatus.STATUS_TEXT[self.status], - "result": self.result, + "shutdown_room": self.result, } if self.error: ret["error"] = self.error @@ -243,6 +243,7 @@ async def purge_history_for_rooms_in_range( purge_id = random_string(16) self._purges_by_id[purge_id] = PurgeStatus() + self._purges_by_room.setdefault(room_id, []).append(purge_id) logger.info( "Starting purging events in room %s (purge_id %s)" % (room_id, purge_id) diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index c4e8ace361cf..bdcfe2409a20 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -195,7 +195,7 @@ async def on_GET( raise NotFoundError("purge id '%s' not found" % purge_id) response = purge_status.asdict() - del response["result"] + del response["shutdown_room"] return 200, response diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index a0f0a3b5f1e2..c5fcf644d37d 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -153,7 +153,7 @@ async def on_GET( **purge.asdict(), } ] - return 200, {"delete_status": response} + return 200, {"results": response} class ListRoomRestServlet(RestServlet): diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index ef9d77748997..1c1288185462 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -641,11 +641,11 @@ def test_delete_expired_status(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(2, len(channel.json_body["delete_status"])) - self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) - self.assertEqual("complete", channel.json_body["delete_status"][1]["status"]) - self.assertEqual(purge_id1, channel.json_body["delete_status"][0]["purge_id"]) - self.assertEqual(purge_id2, channel.json_body["delete_status"][1]["purge_id"]) + self.assertEqual(2, len(channel.json_body["results"])) + self.assertEqual("complete", channel.json_body["results"][0]["status"]) + self.assertEqual("complete", channel.json_body["results"][1]["status"]) + self.assertEqual(purge_id1, channel.json_body["results"][0]["purge_id"]) + self.assertEqual(purge_id2, channel.json_body["results"][1]["purge_id"]) # get status after more than clearing time for first task # second task is not cleared @@ -658,9 +658,9 @@ def test_delete_expired_status(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(1, len(channel.json_body["delete_status"])) - self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) - self.assertEqual(purge_id2, channel.json_body["delete_status"][0]["purge_id"]) + self.assertEqual(1, len(channel.json_body["results"])) + self.assertEqual("complete", channel.json_body["results"][0]["status"]) + self.assertEqual(purge_id2, channel.json_body["results"][0]["purge_id"]) # get status after more than clearing time for all tasks self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) @@ -884,7 +884,7 @@ def test_shutdown_room_consent(self): # Test that member has moved to new room self._is_member( - room_id=channel.json_body["delete_status"][0]["result"]["new_room_id"], + room_id=channel.json_body["results"][0]["shutdown_room"]["new_room_id"], user_id=self.other_user, ) @@ -937,7 +937,7 @@ def test_shutdown_room_block_peek(self): # Test that member has moved to new room self._is_member( - room_id=channel.json_body["delete_status"][0]["result"]["new_room_id"], + room_id=channel.json_body["results"][0]["shutdown_room"]["new_room_id"], user_id=self.other_user, ) @@ -1011,26 +1011,26 @@ def _test_result( expect_new_room: if we expect that a new room was created """ self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(1, len(channel.json_body["delete_status"])) - self.assertEqual("complete", channel.json_body["delete_status"][0]["status"]) - self.assertEqual(purge_id, channel.json_body["delete_status"][0]["purge_id"]) + self.assertEqual(1, len(channel.json_body["results"])) + self.assertEqual("complete", channel.json_body["results"][0]["status"]) + self.assertEqual(purge_id, channel.json_body["results"][0]["purge_id"]) self.assertEqual( kicked_user, - channel.json_body["delete_status"][0]["result"]["kicked_users"][0], + channel.json_body["results"][0]["shutdown_room"]["kicked_users"][0], ) self.assertIn( - "failed_to_kick_users", channel.json_body["delete_status"][0]["result"] + "failed_to_kick_users", channel.json_body["results"][0]["shutdown_room"] ) - self.assertIn("local_aliases", channel.json_body["delete_status"][0]["result"]) - self.assertNotIn("error", channel.json_body["delete_status"][0]) + self.assertIn("local_aliases", channel.json_body["results"][0]["shutdown_room"]) + self.assertNotIn("error", channel.json_body["results"][0]) if expect_new_room: self.assertIsNotNone( - channel.json_body["delete_status"][0]["result"]["new_room_id"] + channel.json_body["results"][0]["shutdown_room"]["new_room_id"] ) else: self.assertIsNone( - channel.json_body["delete_status"][0]["result"]["new_room_id"] + channel.json_body["results"][0]["shutdown_room"]["new_room_id"] ) From 5a764c8c6d95b88123662a49b05233442a27a9cf Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Mon, 8 Nov 2021 23:26:16 +0100 Subject: [PATCH 14/23] add new status API `/rooms/delete_status/` --- synapse/handlers/pagination.py | 7 +- synapse/rest/admin/__init__.py | 6 +- synapse/rest/admin/rooms.py | 24 ++++++- tests/rest/admin/test_admin.py | 2 +- tests/rest/admin/test_room.py | 125 +++++++++++++++------------------ 5 files changed, 88 insertions(+), 76 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index ecb4bc8e50ba..080634b88bfe 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -65,12 +65,12 @@ class PurgeStatus: error: str = "" # Saves the result of an action to give it back to REST API - result: Dict = {} + shutdown_room: Dict = {} def asdict(self) -> JsonDict: ret = { "status": PurgeStatus.STATUS_TEXT[self.status], - "shutdown_room": self.result, + "shutdown_room": self.shutdown_room, } if self.error: ret["error"] = self.error @@ -585,7 +585,7 @@ async def _shutdown_and_purge_room( self._purges_by_id[purge_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS self._purges_by_id[ purge_id - ].result = await self._room_shutdown_handler.shutdown_room( + ].shutdown_room = await self._room_shutdown_handler.shutdown_room( room_id=room_id, requester_user_id=requester_user_id, new_room_user_id=new_room_user_id, @@ -597,7 +597,6 @@ async def _shutdown_and_purge_room( if purge: logger.info("starting purge room_id %s", room_id) - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_ACTIVE # first check that we have no users in this room if not force_purge: diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index bdcfe2409a20..c07ce216f488 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -42,7 +42,8 @@ RegistrationTokenRestServlet, ) from synapse.rest.admin.rooms import ( - DeleteRoomStatusRestServlet, + DeleteRoomStatusByPurgeIdRestServlet, + DeleteRoomStatusByRoomIdRestServlet, ForwardExtremitiesRestServlet, JoinRoomAliasServlet, ListRoomRestServlet, @@ -225,7 +226,8 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: RoomRestServlet(hs).register(http_server) RoomRestV2Servlet(hs).register(http_server) RoomMembersRestServlet(hs).register(http_server) - DeleteRoomStatusRestServlet(hs).register(http_server) + DeleteRoomStatusByPurgeIdRestServlet(hs).register(http_server) + DeleteRoomStatusByRoomIdRestServlet(hs).register(http_server) JoinRoomAliasServlet(hs).register(http_server) VersionServlet(hs).register(http_server) UserAdminServlet(hs).register(http_server) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index c5fcf644d37d..132626eba3d1 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -121,7 +121,7 @@ async def on_DELETE( return 200, {"purge_id": purge_id} -class DeleteRoomStatusRestServlet(RestServlet): +class DeleteRoomStatusByRoomIdRestServlet(RestServlet): """Get the status of the delete room background task.""" PATTERNS = admin_patterns("/rooms/(?P[^/]+)/delete_status$", "v2") @@ -156,6 +156,28 @@ async def on_GET( return 200, {"results": response} +class DeleteRoomStatusByPurgeIdRestServlet(RestServlet): + """Get the status of the delete room background task.""" + + PATTERNS = admin_patterns("/rooms/delete_status/(?P[^/]+)$", "v2") + + def __init__(self, hs: "HomeServer"): + self._auth = hs.get_auth() + self._pagination_handler = hs.get_pagination_handler() + + async def on_GET( + self, request: SynapseRequest, purge_id: str + ) -> Tuple[int, JsonDict]: + + await assert_requester_is_admin(self._auth, request) + + purge_status = self._pagination_handler.get_purge_status(purge_id) + if purge_status is None: + raise NotFoundError("purge id '%s' not found" % purge_id) + + return 200, purge_status.asdict() + + class ListRoomRestServlet(RestServlet): """ List all rooms that are known to the homeserver. Results are returned diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 884d4fdf6d42..2666f4d2f0a1 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -522,4 +522,4 @@ def test_purge_history(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("complete", channel.json_body["status"]) - self.assertNotIn("result", channel.json_body) + self.assertNotIn("shutdown_room", channel.json_body) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 1c1288185462..c32078178980 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -26,7 +26,6 @@ from synapse.rest.client import directory, events, login, room from tests import unittest -from tests.server import FakeChannel """Tests admin REST events for /rooms paths.""" @@ -463,12 +462,16 @@ def prepare(self, reactor, clock, hs): self.other_user, tok=self.other_user_tok ) self.url = f"/_synapse/admin/v2/rooms/{self.room_id}" - self.url_status = f"/_synapse/admin/v2/rooms/{self.room_id}/delete_status" + self.url_status_by_room_id = ( + f"/_synapse/admin/v2/rooms/{self.room_id}/delete_status" + ) + self.url_status_by_purge_id = "/_synapse/admin/v2/rooms/delete_status/" @parameterized.expand( [ ("DELETE", "/_synapse/admin/v2/rooms/%s"), ("GET", "/_synapse/admin/v2/rooms/%s/delete_status"), + ("GET", "/_synapse/admin/v2/rooms/delete_status/%s"), ] ) def test_requester_is_no_admin(self, method: str, url: str): @@ -490,6 +493,7 @@ def test_requester_is_no_admin(self, method: str, url: str): [ ("DELETE", "/_synapse/admin/v2/rooms/%s"), ("GET", "/_synapse/admin/v2/rooms/%s/delete_status"), + ("GET", "/_synapse/admin/v2/rooms/delete_status/%s"), ] ) def test_room_does_not_exist(self, method: str, url: str): @@ -547,13 +551,7 @@ def test_new_room_user_does_not_exist(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] - channel = self.make_request( - "GET", - self.url_status, - access_token=self.admin_user_tok, - ) - - self._test_result(channel, purge_id, self.other_user, expect_new_room=True) + self._test_result(purge_id, self.other_user, expect_new_room=True) def test_new_room_user_is_not_local(self): """ @@ -636,7 +634,7 @@ def test_delete_expired_status(self): # get status channel = self.make_request( "GET", - self.url_status, + self.url_status_by_room_id, access_token=self.admin_user_tok, ) @@ -653,7 +651,7 @@ def test_delete_expired_status(self): channel = self.make_request( "GET", - self.url_status, + self.url_status_by_room_id, access_token=self.admin_user_tok, ) @@ -667,7 +665,7 @@ def test_delete_expired_status(self): channel = self.make_request( "GET", - self.url_status, + self.url_status_by_room_id, access_token=self.admin_user_tok, ) @@ -710,13 +708,7 @@ def test_delete_same_room_twice(self): self.assertIn("purge_id", first_channel.json_body) # check status after finish the task - status_channel = self.make_request( - "GET", - self.url_status, - access_token=self.admin_user_tok, - ) self._test_result( - status_channel, first_channel.json_body["purge_id"], self.other_user, expect_new_room=True, @@ -747,13 +739,7 @@ def test_purge_room_and_block(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] - channel = self.make_request( - "GET", - self.url_status, - access_token=self.admin_user_tok, - ) - - self._test_result(channel, purge_id, self.other_user) + self._test_result(purge_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=True) @@ -784,13 +770,7 @@ def test_purge_room_and_not_block(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] - channel = self.make_request( - "GET", - self.url_status, - access_token=self.admin_user_tok, - ) - - self._test_result(channel, purge_id, self.other_user) + self._test_result(purge_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=False) @@ -822,13 +802,7 @@ def test_block_room_and_not_purge(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] - channel = self.make_request( - "GET", - self.url_status, - access_token=self.admin_user_tok, - ) - - self._test_result(channel, purge_id, self.other_user) + self._test_result(purge_id, self.other_user) with self.assertRaises(AssertionError): self._is_purged(self.room_id) @@ -874,13 +848,15 @@ def test_shutdown_room_consent(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] + self._test_result(purge_id, self.other_user, expect_new_room=True) + channel = self.make_request( "GET", - self.url_status, + self.url_status_by_room_id, access_token=self.admin_user_tok, ) - - self._test_result(channel, purge_id, self.other_user, expect_new_room=True) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(1, len(channel.json_body["results"])) # Test that member has moved to new room self._is_member( @@ -927,13 +903,15 @@ def test_shutdown_room_block_peek(self): self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] + self._test_result(purge_id, self.other_user, expect_new_room=True) + channel = self.make_request( "GET", - self.url_status, + self.url_status_by_room_id, access_token=self.admin_user_tok, ) - - self._test_result(channel, purge_id, self.other_user, expect_new_room=True) + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(1, len(channel.json_body["results"])) # Test that member has moved to new room self._is_member( @@ -996,42 +974,53 @@ def _assert_peek(self, room_id: str, expect_code: int) -> None: def _test_result( self, - channel: FakeChannel, purge_id: str, kicked_user: str, expect_new_room: bool = False, ) -> None: """ - Test that the result is the expected + Test that the result is the expected. + Uses both APIs (status by room_id and purge_id) Args: - channel: channel that should validated purge_id: id of this purge kicked_user: a user_id which is kicked from the room expect_new_room: if we expect that a new room was created """ - self.assertEqual(200, channel.code, msg=channel.json_body) - self.assertEqual(1, len(channel.json_body["results"])) - self.assertEqual("complete", channel.json_body["results"][0]["status"]) - self.assertEqual(purge_id, channel.json_body["results"][0]["purge_id"]) - self.assertEqual( - kicked_user, - channel.json_body["results"][0]["shutdown_room"]["kicked_users"][0], - ) - self.assertIn( - "failed_to_kick_users", channel.json_body["results"][0]["shutdown_room"] + + # get information by room_id + channel_room_id = self.make_request( + "GET", + self.url_status_by_room_id, + access_token=self.admin_user_tok, ) - self.assertIn("local_aliases", channel.json_body["results"][0]["shutdown_room"]) - self.assertNotIn("error", channel.json_body["results"][0]) + self.assertEqual(200, channel_room_id.code, msg=channel_room_id.json_body) + self.assertEqual(1, len(channel_room_id.json_body["results"])) + self.assertEqual(purge_id, channel_room_id.json_body["results"][0]["purge_id"]) - if expect_new_room: - self.assertIsNotNone( - channel.json_body["results"][0]["shutdown_room"]["new_room_id"] - ) - else: - self.assertIsNone( - channel.json_body["results"][0]["shutdown_room"]["new_room_id"] - ) + # get information by purge_id + channel_purge_id = self.make_request( + "GET", + self.url_status_by_purge_id + purge_id, + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel_purge_id.code, msg=channel_purge_id.json_body) + + # test values that are the same in both responses + for content in [ + channel_room_id.json_body["results"][0], + channel_purge_id.json_body, + ]: + self.assertEqual("complete", content["status"]) + self.assertEqual(kicked_user, content["shutdown_room"]["kicked_users"][0]) + self.assertIn("failed_to_kick_users", content["shutdown_room"]) + self.assertIn("local_aliases", content["shutdown_room"]) + self.assertNotIn("error", content) + + if expect_new_room: + self.assertIsNotNone(content["shutdown_room"]["new_room_id"]) + else: + self.assertIsNone(content["shutdown_room"]["new_room_id"]) class RoomTestCase(unittest.HomeserverTestCase): From 32c06947082950578ff095d6b09151d03d88657e Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 00:01:38 +0100 Subject: [PATCH 15/23] update doc --- docs/admin_api/rooms.md | 50 ++++++++++++++++++++++++++++++---- synapse/handlers/pagination.py | 6 ++-- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 1c009988b696..6baedea328c5 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -405,7 +405,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 @@ -533,9 +533,9 @@ The status can be queried up to 24 hours after completion of the task, or until Synapse is restarted (whichever happens first). The result also includes the tasks that have been performed by retention policy. +### Query by `room_id` + With this API you can get the status of all tasks the last 24h for this `room_id`. -If you want only get the status of one specific task, you can also use -the [Purge status query API](purge_history_api.md#Purge-status-query). The API is: @@ -583,12 +583,49 @@ The following parameters should be set in the URL: * `room_id` - The ID of the room. -**Response** +### Query by `purge_id` + +With this API you can get the status of one specific task by `purge_id`. + +The API is: + +``` +GET /_synapse/admin/v2/rooms/delete_status/ +``` + +A response body like the following is returned: + +```json +{ + "status": "active", + "shutdown_room": { + "kicked_users": [ + "@foobar:example.com" + ], + "failed_to_kick_users": [], + "local_aliases": [ + "#badroom:example.com", + "#evilsaloon:example.com" + ], + "new_room_id": "!newroomid:example.com" + } +} +``` + +**Parameters** + +The following parameters should be set in the URL: + +* `purge_id` - The ID for this purge. + +### Response The following fields are returned in the JSON response body: - `results` - An array of objects, each containing information about one task. + This field is omitted from the result when you query by `purge_id`. Task objects contain the following fields: + - `purge_id` - The ID for this purge if you query by `room_id`. - `status` - The status will be one of: - `remove members` - The process is removing users from the `room_id`. - `active` - The process is purging the room from database. @@ -602,8 +639,9 @@ The following fields are returned in the JSON response body: - `kicked_users` - An array of users (`user_id`) that were kicked. - `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. - - `new_room_id` - A string representing the room ID of the new room. + migrated from the old room to the new. + - `new_room_id` - A string representing the room ID of the new room, or `null` if + no such room was created. ## Undoing room deletions diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 080634b88bfe..afb9f0c242b8 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -304,7 +304,7 @@ async def _purge_history( """Carry out a history purge on a room. Args: - purge_id: The id for this purge + purge_id: The ID for this purge. room_id: The room to purge from token: topological token to delete events before delete_local_events: True to delete local events as well as remote ones @@ -540,7 +540,7 @@ async def _shutdown_and_purge_room( aliases to the new room. Users on other servers will be unaffected. Args: - purge_id: The id for this purge + purge_id: The ID for this purge. room_id: The ID of the room to shut down. requester_user_id: User who requested the action. Will be recorded as putting the room on the @@ -720,7 +720,7 @@ def _clear_purge(self, purge_id: str, room_id: str) -> None: Remove the purge from the list 24 hours after it completes Args: - purge_id: The id for this purge + purge_id: The ID for this purge. room_id: The ID of the room to shut down. """ del self._purges_by_id[purge_id] From 7821ed2c76940b2d26cc8a1658f47f1f0b2d8b33 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 00:12:42 +0100 Subject: [PATCH 16/23] update docstring --- synapse/handlers/pagination.py | 13 +------------ synapse/handlers/room.py | 11 ++++------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index afb9f0c242b8..dbb6885c9957 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -526,18 +526,7 @@ async def _shutdown_and_purge_room( """ Shuts down and purges 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 leave the room without any information. - After that, the room will be removed from the database. - - The new room will be created with the user specified by the - `new_room_user_id` parameter as room administrator and will contain a - message explaining what happened. Users invited to the new room will - have power level `-10` by default, and thus be unable to speak. - - The local server will only have the power to move local user and room - aliases to the new room. Users on other servers will be unaffected. + See `RoomShutdownHandler.shutdown_room` for details of creation of the new room Args: purge_id: The ID for this purge. diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index ff21a9cd69be..a6e657cbc271 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1276,13 +1276,6 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class RoomShutdownHandler: - """This handles synchronous room shutdowns and is part of the delete room v1 API. - It will become deprecated in the future. - - The handler for asynchronous shudowns is part of the `PaginationHandler`. - If this handler is removed, `shutdown_room` must to be migrated to `PaginationHandler` - """ - DEFAULT_MESSAGE = ( "Sharing illegal content on this server is not permitted and rooms in" " violation will be blocked." @@ -1307,6 +1300,10 @@ async def shutdown_room( block: bool = False, ) -> dict: """ + This handles synchronous room shutdowns and is part of the delete room v1 API. + The handler for asynchronous shudowns is part of the `PaginationHandler`. + If this handler is removed, `shutdown_room` must to be migrated to `PaginationHandler` + 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 leave the room without any information. From 3b3cf2aa886e1575582c6ddec71b5cf033a107b4 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 20:05:07 +0100 Subject: [PATCH 17/23] fix merge --- synapse/handlers/pagination.py | 17 ++++++++--------- synapse/handlers/room.py | 10 ++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 9b6ad144d472..55b38afe2a6c 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -22,6 +22,7 @@ from synapse.api.constants import EventTypes, Membership from synapse.api.errors import SynapseError from synapse.api.filtering import Filter +from synapse.handlers.room import ShutdownRoomResponse from synapse.metrics.background_process_metrics import run_as_background_process from synapse.storage.state import StateFilter from synapse.streams.config import PaginationConfig @@ -65,7 +66,12 @@ class PurgeStatus: error: str = "" # Saves the result of an action to give it back to REST API - shutdown_room: Dict = {} + shutdown_room: ShutdownRoomResponse = { + "kicked_users": [], + "failed_to_kick_users": [], + "local_aliases": [], + "new_room_id": None, + } def asdict(self) -> JsonDict: ret = { @@ -557,14 +563,7 @@ async def _shutdown_and_purge_room( If set to `true`, the room will be purged from database also if it fails to remove some users from room. - Saves a dict containing the following keys in `PurgeStatus`: - kicked_users: An array of users (`user_id`) that were kicked. - 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. - new_room_id: A string representing the room ID of the new room. + Saves a `RoomShutdownHandler.ShutdownRoomResponse` in `PurgeStatus`: """ self._purges_in_progress_by_room.add(room_id) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index bfe70be8af6c..44b5154454da 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1279,6 +1279,16 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class ShutdownRoomResponse(TypedDict): + """ + kicked_users: An array of users (`user_id`) that were kicked. + 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. + new_room_id: A string representing the room ID of the new room. + """ + kicked_users: List[str] failed_to_kick_users: List[str] local_aliases: List[str] From bd70ffd998dad897f98fd2cda91fb73573fc3a67 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 20:18:17 +0100 Subject: [PATCH 18/23] update test response code, introduced in #11228 --- tests/rest/admin/test_room.py | 56 ++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 6a955f39fe0d..2e23000af12a 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -512,7 +512,7 @@ def test_requester_is_no_admin(self, method: str, url: str): access_token=self.other_user_tok, ) - self.assertEqual(403, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.FORBIDDEN, channel.code, msg=channel.json_body) self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) @parameterized.expand( @@ -534,7 +534,7 @@ def test_room_does_not_exist(self, method: str, url: str): access_token=self.admin_user_tok, ) - self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, msg=channel.json_body) self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) @parameterized.expand( @@ -555,7 +555,7 @@ def test_room_is_not_valid(self, method: str, url: str): access_token=self.admin_user_tok, ) - self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) self.assertEqual( "invalidroom is not a legal room ID", channel.json_body["error"], @@ -573,7 +573,7 @@ def test_new_room_user_does_not_exist(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -591,7 +591,7 @@ def test_new_room_user_is_not_local(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) self.assertEqual( "User must be our own: @not:exist.bla", channel.json_body["error"], @@ -609,7 +609,7 @@ def test_block_is_not_bool(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) def test_purge_is_not_bool(self): @@ -624,7 +624,7 @@ def test_purge_is_not_bool(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, msg=channel.json_body) self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"]) def test_delete_expired_status(self): @@ -638,7 +638,7 @@ def test_delete_expired_status(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id1 = channel.json_body["purge_id"] @@ -653,7 +653,7 @@ def test_delete_expired_status(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id2 = channel.json_body["purge_id"] @@ -664,7 +664,7 @@ def test_delete_expired_status(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual(2, len(channel.json_body["results"])) self.assertEqual("complete", channel.json_body["results"][0]["status"]) self.assertEqual("complete", channel.json_body["results"][1]["status"]) @@ -681,7 +681,7 @@ def test_delete_expired_status(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual(1, len(channel.json_body["results"])) self.assertEqual("complete", channel.json_body["results"][0]["status"]) self.assertEqual(purge_id2, channel.json_body["results"][0]["purge_id"]) @@ -695,7 +695,7 @@ def test_delete_expired_status(self): access_token=self.admin_user_tok, ) - self.assertEqual(404, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, msg=channel.json_body) self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"]) def test_delete_same_room_twice(self): @@ -721,7 +721,7 @@ def test_delete_same_room_twice(self): access_token=self.admin_user_tok, ) - self.assertEqual(400, second_channel.code, msg=second_channel.json_body) + self.assertEqual(HTTPStatus.BAD_REQUEST, second_channel.code, msg=second_channel.json_body) self.assertEqual(Codes.UNKNOWN, second_channel.json_body["errcode"]) self.assertEqual( f"History purge already in progress for {self.room_id}", @@ -730,7 +730,7 @@ def test_delete_same_room_twice(self): # get result of first call first_channel.await_result() - self.assertEqual(200, first_channel.code, msg=first_channel.json_body) + self.assertEqual(HTTPStatus.OK, first_channel.code, msg=first_channel.json_body) self.assertIn("purge_id", first_channel.json_body) # check status after finish the task @@ -761,7 +761,7 @@ def test_purge_room_and_block(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -792,7 +792,7 @@ def test_purge_room_and_not_block(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -824,7 +824,7 @@ def test_block_room_and_not_purge(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -870,7 +870,7 @@ def test_shutdown_room_consent(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -881,7 +881,7 @@ def test_shutdown_room_consent(self): self.url_status_by_room_id, access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual(1, len(channel.json_body["results"])) # Test that member has moved to new room @@ -908,7 +908,7 @@ def test_shutdown_room_block_peek(self): content={"history_visibility": "world_readable"}, access_token=self.other_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) # Test that room is not purged with self.assertRaises(AssertionError): @@ -925,7 +925,7 @@ def test_shutdown_room_block_peek(self): access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertIn("purge_id", channel.json_body) purge_id = channel.json_body["purge_id"] @@ -936,7 +936,7 @@ def test_shutdown_room_block_peek(self): self.url_status_by_room_id, access_token=self.admin_user_tok, ) - self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual(1, len(channel.json_body["results"])) # Test that member has moved to new room @@ -1020,7 +1020,11 @@ def _test_result( self.url_status_by_room_id, access_token=self.admin_user_tok, ) - self.assertEqual(200, channel_room_id.code, msg=channel_room_id.json_body) + self.assertEqual( + HTTPStatus.OK, + channel_room_id.code, + msg=channel_room_id.json_body + ) self.assertEqual(1, len(channel_room_id.json_body["results"])) self.assertEqual(purge_id, channel_room_id.json_body["results"][0]["purge_id"]) @@ -1030,7 +1034,11 @@ def _test_result( self.url_status_by_purge_id + purge_id, access_token=self.admin_user_tok, ) - self.assertEqual(200, channel_purge_id.code, msg=channel_purge_id.json_body) + self.assertEqual( + HTTPStatus.OK, + channel_purge_id.code, + msg=channel_purge_id.json_body, + ) # test values that are the same in both responses for content in [ From 05dd106898162ceb7c58591fa1e06260a79b7731 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 21:16:47 +0100 Subject: [PATCH 19/23] make compatible with #11228 --- docs/admin_api/rooms.md | 11 ++++++----- synapse/rest/admin/rooms.py | 4 ++-- tests/rest/admin/test_room.py | 8 ++++---- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 0c9dfcf7201a..c47c23387525 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -400,10 +400,10 @@ as room administrator and will contain a message explaining what happened. Users to the new room will have power level `-10` by default, and thus be unable to speak. If `block` is `true`, users will be prevented from joining the old room. -This option can also be used to pre-emptively block a room, even if it's unknown -to this homeserver. In this case, the room will be blocked, and no further action -will be taken. If `block` is `false`, attempting to delete an unknown room is -invalid and will be rejected as a bad request. +This option can in [Version 1](#version-1-old-version) also be used to pre-emptively +block a room, even if it's unknown to this homeserver. In this case, the room will be +blocked, and no further action will be taken. If `block` is `false`, attempting to +delete an unknown room is invalid and will be rejected as a bad request. 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 @@ -519,7 +519,8 @@ The following JSON body parameters are available: is not permitted and rooms in violation will be blocked.` * `block` - Optional. If set to `true`, this room will be added to a blocking list, preventing future attempts to join the room. Rooms can be blocked - even if they're not yet known to the homeserver. Defaults to `false`. + even if they're not yet known to the homeserver (only with + [Version 1](#version-1-old-version) of the API). Defaults to `false`. * `purge` - Optional. If set to `true`, it will remove all traces of the room from your database. Defaults to `true`. * `force_purge` - Optional, and ignored unless `purge` is `true`. If set to `true`, it diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 05fc192eb780..2eecd39d6f04 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -153,7 +153,7 @@ async def on_GET( **purge.asdict(), } ] - return 200, {"results": response} + return 200, {"results": cast(JsonDict, response)} class DeleteRoomStatusByPurgeIdRestServlet(RestServlet): @@ -175,7 +175,7 @@ async def on_GET( if purge_status is None: raise NotFoundError("purge id '%s' not found" % purge_id) - return 200, purge_status.asdict() + return 200, cast(JsonDict, purge_status.asdict()) class ListRoomRestServlet(RestServlet): diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 2e23000af12a..4257066b9ff8 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -721,7 +721,9 @@ def test_delete_same_room_twice(self): access_token=self.admin_user_tok, ) - self.assertEqual(HTTPStatus.BAD_REQUEST, second_channel.code, msg=second_channel.json_body) + self.assertEqual( + HTTPStatus.BAD_REQUEST, second_channel.code, msg=second_channel.json_body + ) self.assertEqual(Codes.UNKNOWN, second_channel.json_body["errcode"]) self.assertEqual( f"History purge already in progress for {self.room_id}", @@ -1021,9 +1023,7 @@ def _test_result( access_token=self.admin_user_tok, ) self.assertEqual( - HTTPStatus.OK, - channel_room_id.code, - msg=channel_room_id.json_body + HTTPStatus.OK, channel_room_id.code, msg=channel_room_id.json_body ) self.assertEqual(1, len(channel_room_id.json_body["results"])) self.assertEqual(purge_id, channel_room_id.json_body["results"][0]["purge_id"]) From 11d9e87eedb5bf93f8d5ca8a246e6d218616b712 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 9 Nov 2021 21:36:27 +0100 Subject: [PATCH 20/23] rename status --- docs/admin_api/purge_history_api.md | 4 ++-- docs/admin_api/rooms.md | 8 ++++---- synapse/handlers/pagination.py | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/docs/admin_api/purge_history_api.md b/docs/admin_api/purge_history_api.md index dd43cfc35eca..71c2b367a0f7 100644 --- a/docs/admin_api/purge_history_api.md +++ b/docs/admin_api/purge_history_api.md @@ -64,11 +64,11 @@ This API returns a JSON body like the following: ```json { - "status": "active" + "status": "purging" } ``` -The status will be one of `active`, `complete`, or `failed`. +The status will be one of `purging`, `complete`, or `failed`. If `status` is `failed` there will be a string `error` with the error message. ## Reclaim disk space (Postgres) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index c47c23387525..87570f4e2750 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -566,7 +566,7 @@ A response body like the following is returned: } }, { "purge_id": "purgeid2", - "status": "active", + "status": "purging", "shutdown_room": { "kicked_users": [ "@foobar:example.com" @@ -603,7 +603,7 @@ A response body like the following is returned: ```json { - "status": "active", + "status": "purging", "shutdown_room": { "kicked_users": [ "@foobar:example.com" @@ -633,8 +633,8 @@ The following fields are returned in the JSON response body: Task objects contain the following fields: - `purge_id` - The ID for this purge if you query by `room_id`. - `status` - The status will be one of: - - `remove members` - The process is removing users from the `room_id`. - - `active` - The process is purging the room from database. + - `shutting_down` - The process is removing users from the room. + - `purging` - The process is purging the room and event data from database. - `complete` - The process has completed successfully. - `failed` - The process is aborted, an error has occurred. - `error` - A string that shows an error message if `status` is `failed`. diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 55b38afe2a6c..d92d986c6788 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -46,21 +46,21 @@ class PurgeStatus: return by get_purge_status or delete_status. """ - STATUS_ACTIVE = 0 + STATUS_PURGING = 0 STATUS_COMPLETE = 1 STATUS_FAILED = 2 - STATUS_REMOVE_MEMBERS = 3 + STATUS_SHUTTING_DOWN = 3 STATUS_TEXT = { - STATUS_ACTIVE: "active", + STATUS_PURGING: "purging", STATUS_COMPLETE: "complete", STATUS_FAILED: "failed", - STATUS_REMOVE_MEMBERS: "remove members", + STATUS_SHUTTING_DOWN: "shutting_down", } # Tracks whether this request has completed. - # One of STATUS_{ACTIVE,COMPLETE,FAILED,REMOVE_MEMBERS}. - status: int = STATUS_ACTIVE + # One of STATUS_{PURGING,COMPLETE,FAILED,SHUTTING_DOWN}. + status: int = STATUS_PURGING # Save the error message if an error occurs error: str = "" @@ -570,7 +570,7 @@ async def _shutdown_and_purge_room( try: with await self.pagination_lock.write(room_id): - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_REMOVE_MEMBERS + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_SHUTTING_DOWN self._purges_by_id[ purge_id ].shutdown_room = await self._room_shutdown_handler.shutdown_room( @@ -581,7 +581,7 @@ async def _shutdown_and_purge_room( message=message, block=block, ) - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_ACTIVE + self._purges_by_id[purge_id].status = PurgeStatus.STATUS_PURGING if purge: logger.info("starting purge room_id %s", room_id) From fea716f2279f33196bb81e6101e2b9c56669c867 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 10 Nov 2021 23:07:23 +0100 Subject: [PATCH 21/23] separate `purge` and `delete` --- docs/admin_api/purge_history_api.md | 4 +- docs/admin_api/rooms.md | 19 ++-- synapse/handlers/pagination.py | 138 +++++++++++++++++----------- synapse/rest/admin/__init__.py | 8 +- synapse/rest/admin/rooms.py | 32 +++---- tests/rest/admin/test_admin.py | 1 - tests/rest/admin/test_room.py | 78 ++++++++-------- 7 files changed, 155 insertions(+), 125 deletions(-) diff --git a/docs/admin_api/purge_history_api.md b/docs/admin_api/purge_history_api.md index 71c2b367a0f7..dd43cfc35eca 100644 --- a/docs/admin_api/purge_history_api.md +++ b/docs/admin_api/purge_history_api.md @@ -64,11 +64,11 @@ This API returns a JSON body like the following: ```json { - "status": "purging" + "status": "active" } ``` -The status will be one of `purging`, `complete`, or `failed`. +The status will be one of `active`, `complete`, or `failed`. If `status` is `failed` there will be a string `error` with the error message. ## Reclaim disk space (Postgres) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 87570f4e2750..1d14a5afc712 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -494,7 +494,7 @@ a purge id: ```json { - "purge_id": "" + "delete_id": "" } ``` @@ -537,7 +537,6 @@ The JSON body must not be empty. The body must be at least `{}`. It is possible to query the status of the background task for deleting rooms. The status can be queried up to 24 hours after completion of the task, or until Synapse is restarted (whichever happens first). -The result also includes the tasks that have been performed by retention policy. ### Query by `room_id` @@ -555,7 +554,7 @@ A response body like the following is returned: { "results": [ { - "purge_id": "purgeid1", + "delete_id": "delete_id1", "status": "failed", "error": "error message", "shutdown_room": { @@ -565,7 +564,7 @@ A response body like the following is returned: "new_room_id": null } }, { - "purge_id": "purgeid2", + "delete_id": "delete_id2", "status": "purging", "shutdown_room": { "kicked_users": [ @@ -589,14 +588,14 @@ The following parameters should be set in the URL: * `room_id` - The ID of the room. -### Query by `purge_id` +### Query by `delete_id` -With this API you can get the status of one specific task by `purge_id`. +With this API you can get the status of one specific task by `delete_id`. The API is: ``` -GET /_synapse/admin/v2/rooms/delete_status/ +GET /_synapse/admin/v2/rooms/delete_status/ ``` A response body like the following is returned: @@ -622,16 +621,16 @@ A response body like the following is returned: The following parameters should be set in the URL: -* `purge_id` - The ID for this purge. +* `delete_id` - The ID for this delete. ### Response The following fields are returned in the JSON response body: - `results` - An array of objects, each containing information about one task. - This field is omitted from the result when you query by `purge_id`. + This field is omitted from the result when you query by `delete_id`. Task objects contain the following fields: - - `purge_id` - The ID for this purge if you query by `room_id`. + - `delete_id` - The ID for this purge if you query by `room_id`. - `status` - The status will be one of: - `shutting_down` - The process is removing users from the room. - `purging` - The process is purging the room and event data from database. diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index d92d986c6788..d2c4bf7e58a8 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -40,10 +40,41 @@ @attr.s(slots=True, auto_attribs=True) class PurgeStatus: - """Object tracking the status of a purge events/delete room request + """Object tracking the status of a purge request - This class contains information on the progress of a purge events/delete room request, for - return by get_purge_status or delete_status. + This class contains information on the progress of a purge request, for + return by get_purge_status. + """ + + STATUS_ACTIVE = 0 + STATUS_COMPLETE = 1 + STATUS_FAILED = 2 + + STATUS_TEXT = { + STATUS_ACTIVE: "active", + STATUS_COMPLETE: "complete", + STATUS_FAILED: "failed", + } + + # Save the error message if an error occurs + error: str = "" + + # Tracks whether this request has completed. One of STATUS_{ACTIVE,COMPLETE,FAILED}. + status: int = STATUS_ACTIVE + + def asdict(self) -> JsonDict: + ret = {"status": PurgeStatus.STATUS_TEXT[self.status]} + if self.error: + ret["error"] = self.error + return ret + + +@attr.s(slots=True, auto_attribs=True) +class DeleteStatus: + """Object tracking the status of a delete room request + + This class contains information on the progress of a delete room request, for + return by get_delete_status. """ STATUS_PURGING = 0 @@ -75,7 +106,7 @@ class PurgeStatus: def asdict(self) -> JsonDict: ret = { - "status": PurgeStatus.STATUS_TEXT[self.status], + "status": DeleteStatus.STATUS_TEXT[self.status], "shutdown_room": self.shutdown_room, } if self.error: @@ -107,9 +138,11 @@ def __init__(self, hs: "HomeServer"): self._purges_in_progress_by_room: Set[str] = set() # map from purge id to PurgeStatus self._purges_by_id: Dict[str, PurgeStatus] = {} - # map from room id to purge ids - # Dict[`room_id`, List[`purge_id`]] - self._purges_by_room: Dict[str, List[str]] = {} + # map from purge id to DeleteStatus + self._delete_by_id: Dict[str, DeleteStatus] = {} + # map from room id to delete ids + # Dict[`room_id`, List[`delete_id`]] + self._delete_by_room: Dict[str, List[str]] = {} self._event_serializer = hs.get_event_client_serializer() self._retention_default_max_lifetime = ( @@ -249,7 +282,6 @@ async def purge_history_for_rooms_in_range( purge_id = random_string(16) self._purges_by_id[purge_id] = PurgeStatus() - self._purges_by_room.setdefault(room_id, []).append(purge_id) logger.info( "Starting purging events in room %s (purge_id %s)" % (room_id, purge_id) @@ -293,7 +325,6 @@ def start_purge_history( logger.info("[purge] starting purge_id %s", purge_id) self._purges_by_id[purge_id] = PurgeStatus() - self._purges_by_room.setdefault(room_id, []).append(purge_id) run_as_background_process( "purge_history", self._purge_history, @@ -329,14 +360,16 @@ async def _purge_history( "[purge] failed", exc_info=(f.type, f.value, f.getTracebackObject()) # type: ignore ) self._purges_by_id[purge_id].status = PurgeStatus.STATUS_FAILED + self._purges_by_id[purge_id].error = f.getErrorMessage() finally: self._purges_in_progress_by_room.discard(room_id) + # remove the purge from the list 24 hours after it completes + def clear_purge() -> None: + del self._purges_by_id[purge_id] + self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_TIME, - self._clear_purge, - purge_id, - room_id, + PaginationHandler.CLEAR_PURGE_TIME, clear_purge ) def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: @@ -347,13 +380,21 @@ def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: """ return self._purges_by_id.get(purge_id) - def get_purge_ids_by_room(self, room_id: str) -> Optional[Collection[str]]: - """Get all active purge ids by room + def get_delete_status(self, delete_id: str) -> Optional[DeleteStatus]: + """Get the current status of an active deleting Args: - room_id: room_id that is purged + delete_id: delete_id returned by start_shutdown_and_purge_room """ - return self._purges_by_room.get(room_id) + return self._delete_by_id.get(delete_id) + + def get_delete_ids_by_room(self, room_id: str) -> Optional[Collection[str]]: + """Get all active delete ids by room + + Args: + room_id: room_id that is deleted + """ + return self._delete_by_room.get(room_id) async def purge_room(self, room_id: str, force: bool = False) -> None: """Purge the given room from the database. @@ -519,7 +560,7 @@ async def get_messages( async def _shutdown_and_purge_room( self, - purge_id: str, + delete_id: str, room_id: str, requester_user_id: str, new_room_user_id: Optional[str] = None, @@ -535,7 +576,7 @@ async def _shutdown_and_purge_room( See `RoomShutdownHandler.shutdown_room` for details of creation of the new room Args: - purge_id: The ID for this purge. + delete_id: The ID for this delete. room_id: The ID of the room to shut down. requester_user_id: User who requested the action. Will be recorded as putting the room on the @@ -563,16 +604,16 @@ async def _shutdown_and_purge_room( If set to `true`, the room will be purged from database also if it fails to remove some users from room. - Saves a `RoomShutdownHandler.ShutdownRoomResponse` in `PurgeStatus`: + Saves a `RoomShutdownHandler.ShutdownRoomResponse` in `DeleteStatus`: """ self._purges_in_progress_by_room.add(room_id) try: with await self.pagination_lock.write(room_id): - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_SHUTTING_DOWN - self._purges_by_id[ - purge_id + self._delete_by_id[delete_id].status = DeleteStatus.STATUS_SHUTTING_DOWN + self._delete_by_id[ + delete_id ].shutdown_room = await self._room_shutdown_handler.shutdown_room( room_id=room_id, requester_user_id=requester_user_id, @@ -581,7 +622,7 @@ async def _shutdown_and_purge_room( message=message, block=block, ) - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_PURGING + self._delete_by_id[delete_id].status = DeleteStatus.STATUS_PURGING if purge: logger.info("starting purge room_id %s", room_id) @@ -599,23 +640,27 @@ async def _shutdown_and_purge_room( await self.storage.purge_events.purge_room(room_id) logger.info("complete") - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_COMPLETE + self._delete_by_id[delete_id].status = DeleteStatus.STATUS_COMPLETE except Exception: f = Failure() logger.error( "failed", exc_info=(f.type, f.value, f.getTracebackObject()), # type: ignore ) - self._purges_by_id[purge_id].status = PurgeStatus.STATUS_FAILED - self._purges_by_id[purge_id].error = f.getErrorMessage() + self._delete_by_id[delete_id].status = DeleteStatus.STATUS_FAILED + self._delete_by_id[delete_id].error = f.getErrorMessage() finally: self._purges_in_progress_by_room.discard(room_id) + # remove the delete from the list 24 hours after it completes + def clear_delete() -> None: + del self._delete_by_id[delete_id] + self._delete_by_room[room_id].remove(delete_id) + if not self._delete_by_room[room_id]: + del self._delete_by_room[room_id] + self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_TIME, - self._clear_purge, - purge_id, - room_id, + PaginationHandler.CLEAR_PURGE_TIME, clear_delete ) def start_shutdown_and_purge_room( @@ -660,7 +705,7 @@ def start_shutdown_and_purge_room( also if it fails to remove some users from room. Returns: - unique ID for this purge transaction. + unique ID for this delete transaction. """ if room_id in self._purges_in_progress_by_room: raise SynapseError( @@ -676,22 +721,22 @@ def start_shutdown_and_purge_room( 400, "User must be our own: %s" % (new_room_user_id,) ) - purge_id = random_string(16) + delete_id = random_string(16) - # we log the purge_id here so that it can be tied back to the + # we log the delete_id here so that it can be tied back to the # request id in the log lines. logger.info( - "starting shutdown room_id %s with purge_id %s", + "starting shutdown room_id %s with delete_id %s", room_id, - purge_id, + delete_id, ) - self._purges_by_id[purge_id] = PurgeStatus() - self._purges_by_room.setdefault(room_id, []).append(purge_id) + self._delete_by_id[delete_id] = DeleteStatus() + self._delete_by_room.setdefault(room_id, []).append(delete_id) run_as_background_process( "shutdown_and_purge_room", self._shutdown_and_purge_room, - purge_id, + delete_id, room_id, requester_user_id, new_room_user_id, @@ -701,17 +746,4 @@ def start_shutdown_and_purge_room( purge, force_purge, ) - return purge_id - - def _clear_purge(self, purge_id: str, room_id: str) -> None: - """ - Remove the purge from the list 24 hours after it completes - - Args: - purge_id: The ID for this purge. - room_id: The ID of the room to shut down. - """ - del self._purges_by_id[purge_id] - self._purges_by_room[room_id].remove(purge_id) - if not self._purges_by_room[room_id]: - del self._purges_by_room[room_id] + return delete_id diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index 4981fe1af04b..d78fe406c40a 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -46,7 +46,7 @@ RegistrationTokenRestServlet, ) from synapse.rest.admin.rooms import ( - DeleteRoomStatusByPurgeIdRestServlet, + DeleteRoomStatusByDeleteIdRestServlet, DeleteRoomStatusByRoomIdRestServlet, ForwardExtremitiesRestServlet, JoinRoomAliasServlet, @@ -199,9 +199,7 @@ async def on_GET( if purge_status is None: raise NotFoundError("purge id '%s' not found" % purge_id) - response = purge_status.asdict() - del response["shutdown_room"] - return 200, response + return 200, purge_status.asdict() ######################################################################################## @@ -230,7 +228,7 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: RoomRestServlet(hs).register(http_server) RoomRestV2Servlet(hs).register(http_server) RoomMembersRestServlet(hs).register(http_server) - DeleteRoomStatusByPurgeIdRestServlet(hs).register(http_server) + DeleteRoomStatusByDeleteIdRestServlet(hs).register(http_server) DeleteRoomStatusByRoomIdRestServlet(hs).register(http_server) JoinRoomAliasServlet(hs).register(http_server) VersionServlet(hs).register(http_server) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 2eecd39d6f04..37cb4d079618 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -107,7 +107,7 @@ async def on_DELETE( if not await self._store.get_room(room_id): raise NotFoundError("Unknown room id %s" % (room_id,)) - purge_id = self._pagination_handler.start_shutdown_and_purge_room( + delete_id = self._pagination_handler.start_shutdown_and_purge_room( room_id=room_id, new_room_user_id=content.get("new_room_user_id"), new_room_name=content.get("room_name"), @@ -118,7 +118,7 @@ async def on_DELETE( force_purge=force_purge, ) - return 200, {"purge_id": purge_id} + return 200, {"delete_id": delete_id} class DeleteRoomStatusByRoomIdRestServlet(RestServlet): @@ -139,43 +139,43 @@ async def on_GET( if not RoomID.is_valid(room_id): raise SynapseError(400, "%s is not a legal room ID" % (room_id,)) - purge_ids = self._pagination_handler.get_purge_ids_by_room(room_id) - if purge_ids is None: + delete_ids = self._pagination_handler.get_delete_ids_by_room(room_id) + if delete_ids is None: raise NotFoundError("No delete task for room_id '%s' found" % room_id) response = [] - for purge_id in purge_ids: - purge = self._pagination_handler.get_purge_status(purge_id) - if purge: + for delete_id in delete_ids: + delete = self._pagination_handler.get_delete_status(delete_id) + if delete: response += [ { - "purge_id": purge_id, - **purge.asdict(), + "delete_id": delete_id, + **delete.asdict(), } ] return 200, {"results": cast(JsonDict, response)} -class DeleteRoomStatusByPurgeIdRestServlet(RestServlet): +class DeleteRoomStatusByDeleteIdRestServlet(RestServlet): """Get the status of the delete room background task.""" - PATTERNS = admin_patterns("/rooms/delete_status/(?P[^/]+)$", "v2") + PATTERNS = admin_patterns("/rooms/delete_status/(?P[^/]+)$", "v2") def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() self._pagination_handler = hs.get_pagination_handler() async def on_GET( - self, request: SynapseRequest, purge_id: str + self, request: SynapseRequest, delete_id: str ) -> Tuple[int, JsonDict]: await assert_requester_is_admin(self._auth, request) - purge_status = self._pagination_handler.get_purge_status(purge_id) - if purge_status is None: - raise NotFoundError("purge id '%s' not found" % purge_id) + delete_status = self._pagination_handler.get_delete_status(delete_id) + if delete_status is None: + raise NotFoundError("delete id '%s' not found" % delete_id) - return 200, cast(JsonDict, purge_status.asdict()) + return 200, cast(JsonDict, delete_status.asdict()) class ListRoomRestServlet(RestServlet): diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 2666f4d2f0a1..af849bd47138 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -522,4 +522,3 @@ def test_purge_history(self): self.assertEqual(200, channel.code, msg=channel.json_body) self.assertEqual("complete", channel.json_body["status"]) - self.assertNotIn("shutdown_room", channel.json_body) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 4257066b9ff8..d72b930d9337 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -491,7 +491,7 @@ def prepare(self, reactor, clock, hs): self.url_status_by_room_id = ( f"/_synapse/admin/v2/rooms/{self.room_id}/delete_status" ) - self.url_status_by_purge_id = "/_synapse/admin/v2/rooms/delete_status/" + self.url_status_by_delete_id = "/_synapse/admin/v2/rooms/delete_status/" @parameterized.expand( [ @@ -574,10 +574,10 @@ def test_new_room_user_does_not_exist(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user, expect_new_room=True) + self._test_result(delete_id, self.other_user, expect_new_room=True) def test_new_room_user_is_not_local(self): """ @@ -639,8 +639,8 @@ def test_delete_expired_status(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id1 = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id1 = channel.json_body["delete_id"] # go ahead self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) @@ -654,8 +654,8 @@ def test_delete_expired_status(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id2 = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id2 = channel.json_body["delete_id"] # get status channel = self.make_request( @@ -668,8 +668,8 @@ def test_delete_expired_status(self): self.assertEqual(2, len(channel.json_body["results"])) self.assertEqual("complete", channel.json_body["results"][0]["status"]) self.assertEqual("complete", channel.json_body["results"][1]["status"]) - self.assertEqual(purge_id1, channel.json_body["results"][0]["purge_id"]) - self.assertEqual(purge_id2, channel.json_body["results"][1]["purge_id"]) + self.assertEqual(delete_id1, channel.json_body["results"][0]["delete_id"]) + self.assertEqual(delete_id2, channel.json_body["results"][1]["delete_id"]) # get status after more than clearing time for first task # second task is not cleared @@ -684,7 +684,7 @@ def test_delete_expired_status(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual(1, len(channel.json_body["results"])) self.assertEqual("complete", channel.json_body["results"][0]["status"]) - self.assertEqual(purge_id2, channel.json_body["results"][0]["purge_id"]) + self.assertEqual(delete_id2, channel.json_body["results"][0]["delete_id"]) # get status after more than clearing time for all tasks self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) @@ -733,11 +733,11 @@ def test_delete_same_room_twice(self): # get result of first call first_channel.await_result() self.assertEqual(HTTPStatus.OK, first_channel.code, msg=first_channel.json_body) - self.assertIn("purge_id", first_channel.json_body) + self.assertIn("delete_id", first_channel.json_body) # check status after finish the task self._test_result( - first_channel.json_body["purge_id"], + first_channel.json_body["delete_id"], self.other_user, expect_new_room=True, ) @@ -764,10 +764,10 @@ def test_purge_room_and_block(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user) + self._test_result(delete_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=True) @@ -795,10 +795,10 @@ def test_purge_room_and_not_block(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user) + self._test_result(delete_id, self.other_user) self._is_purged(self.room_id) self._is_blocked(self.room_id, expect=False) @@ -827,10 +827,10 @@ def test_block_room_and_not_purge(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user) + self._test_result(delete_id, self.other_user) with self.assertRaises(AssertionError): self._is_purged(self.room_id) @@ -873,10 +873,10 @@ def test_shutdown_room_consent(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user, expect_new_room=True) + self._test_result(delete_id, self.other_user, expect_new_room=True) channel = self.make_request( "GET", @@ -928,10 +928,10 @@ def test_shutdown_room_block_peek(self): ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) - self.assertIn("purge_id", channel.json_body) - purge_id = channel.json_body["purge_id"] + self.assertIn("delete_id", channel.json_body) + delete_id = channel.json_body["delete_id"] - self._test_result(purge_id, self.other_user, expect_new_room=True) + self._test_result(delete_id, self.other_user, expect_new_room=True) channel = self.make_request( "GET", @@ -1002,16 +1002,16 @@ def _assert_peek(self, room_id: str, expect_code: int) -> None: def _test_result( self, - purge_id: str, + delete_id: str, kicked_user: str, expect_new_room: bool = False, ) -> None: """ Test that the result is the expected. - Uses both APIs (status by room_id and purge_id) + Uses both APIs (status by room_id and delete_id) Args: - purge_id: id of this purge + delete_id: id of this purge kicked_user: a user_id which is kicked from the room expect_new_room: if we expect that a new room was created """ @@ -1026,24 +1026,26 @@ def _test_result( HTTPStatus.OK, channel_room_id.code, msg=channel_room_id.json_body ) self.assertEqual(1, len(channel_room_id.json_body["results"])) - self.assertEqual(purge_id, channel_room_id.json_body["results"][0]["purge_id"]) + self.assertEqual( + delete_id, channel_room_id.json_body["results"][0]["delete_id"] + ) - # get information by purge_id - channel_purge_id = self.make_request( + # get information by delete_id + channel_delete_id = self.make_request( "GET", - self.url_status_by_purge_id + purge_id, + self.url_status_by_delete_id + delete_id, access_token=self.admin_user_tok, ) self.assertEqual( HTTPStatus.OK, - channel_purge_id.code, - msg=channel_purge_id.json_body, + channel_delete_id.code, + msg=channel_delete_id.json_body, ) # test values that are the same in both responses for content in [ channel_room_id.json_body["results"][0], - channel_purge_id.json_body, + channel_delete_id.json_body, ]: self.assertEqual("complete", content["status"]) self.assertEqual(kicked_user, content["shutdown_room"]["kicked_users"][0]) From 1e5c5a4808b05450105ce309a0d27fdc1cad0694 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 11 Nov 2021 18:54:51 +0100 Subject: [PATCH 22/23] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- docs/admin_api/purge_history_api.md | 1 + docs/admin_api/rooms.md | 3 ++- synapse/handlers/pagination.py | 10 +++++----- synapse/handlers/room.py | 19 ++++++++----------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/docs/admin_api/purge_history_api.md b/docs/admin_api/purge_history_api.md index dd43cfc35eca..277e28d9cbcb 100644 --- a/docs/admin_api/purge_history_api.md +++ b/docs/admin_api/purge_history_api.md @@ -69,6 +69,7 @@ This API returns a JSON body like the following: ``` The status will be one of `active`, `complete`, or `failed`. + If `status` is `failed` there will be a string `error` with the error message. ## Reclaim disk space (Postgres) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 1d14a5afc712..6a6ae92d66ba 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -540,7 +540,8 @@ or until Synapse is restarted (whichever happens first). ### Query by `room_id` -With this API you can get the status of all tasks the last 24h for this `room_id`. +With this API you can get the status of all active deletion tasks, and all those completed in the last 24h, +for the given `room_id`. The API is: diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index d2c4bf7e58a8..9c1a96ae703c 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -121,8 +121,8 @@ class PaginationHandler: paginating during a purge. """ - # remove the purge from the list 24 hours after it completes - CLEAR_PURGE_TIME = 3600 * 24 + # when to remove a completed deletion/purge from the results map + CLEAR_PURGE_AFTER_MS = 1000 * 3600 * 24 # 24 hours def __init__(self, hs: "HomeServer"): self.hs = hs @@ -135,6 +135,7 @@ def __init__(self, hs: "HomeServer"): self._room_shutdown_handler = hs.get_room_shutdown_handler() self.pagination_lock = ReadWriteLock() + # IDs of rooms in which there currently an active purge *or delete* operation. self._purges_in_progress_by_room: Set[str] = set() # map from purge id to PurgeStatus self._purges_by_id: Dict[str, PurgeStatus] = {} @@ -369,7 +370,7 @@ def clear_purge() -> None: del self._purges_by_id[purge_id] self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_TIME, clear_purge + PaginationHandler.CLEAR_PURGE_AFTER_MS/1000, clear_purge ) def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: @@ -610,7 +611,6 @@ async def _shutdown_and_purge_room( self._purges_in_progress_by_room.add(room_id) try: with await self.pagination_lock.write(room_id): - self._delete_by_id[delete_id].status = DeleteStatus.STATUS_SHUTTING_DOWN self._delete_by_id[ delete_id @@ -660,7 +660,7 @@ def clear_delete() -> None: del self._delete_by_room[room_id] self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_TIME, clear_delete + PaginationHandler.CLEAR_PURGE_AFTER_MS/1000, clear_delete ) def start_shutdown_and_purge_room( diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 44b5154454da..f9a099c4f3ef 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -1280,13 +1280,14 @@ def get_current_key_for_room(self, room_id: str) -> Awaitable[str]: class ShutdownRoomResponse(TypedDict): """ - kicked_users: An array of users (`user_id`) that were kicked. - 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. - new_room_id: A string representing the room ID of the new room. + Attributes: + kicked_users: An array of users (`user_id`) that were kicked. + 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. + new_room_id: A string representing the room ID of the new room. """ kicked_users: List[str] @@ -1320,10 +1321,6 @@ async def shutdown_room( block: bool = False, ) -> ShutdownRoomResponse: """ - This handles synchronous room shutdowns and is part of the delete room v1 API. - The handler for asynchronous shudowns is part of the `PaginationHandler`. - If this handler is removed, `shutdown_room` must to be migrated to `PaginationHandler` - 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 leave the room without any information. From 949e55f480c892e4d9669bfee2f7a3e7bc0950b7 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 11 Nov 2021 20:14:35 +0100 Subject: [PATCH 23/23] lint and small fixes --- synapse/handlers/pagination.py | 6 +++--- tests/rest/admin/test_room.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 9c1a96ae703c..cd64142735de 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -122,7 +122,7 @@ class PaginationHandler: """ # when to remove a completed deletion/purge from the results map - CLEAR_PURGE_AFTER_MS = 1000 * 3600 * 24 # 24 hours + CLEAR_PURGE_AFTER_MS = 1000 * 3600 * 24 # 24 hours def __init__(self, hs: "HomeServer"): self.hs = hs @@ -370,7 +370,7 @@ def clear_purge() -> None: del self._purges_by_id[purge_id] self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_AFTER_MS/1000, clear_purge + PaginationHandler.CLEAR_PURGE_AFTER_MS / 1000, clear_purge ) def get_purge_status(self, purge_id: str) -> Optional[PurgeStatus]: @@ -660,7 +660,7 @@ def clear_delete() -> None: del self._delete_by_room[room_id] self.hs.get_reactor().callLater( - PaginationHandler.CLEAR_PURGE_AFTER_MS/1000, clear_delete + PaginationHandler.CLEAR_PURGE_AFTER_MS / 1000, clear_delete ) def start_shutdown_and_purge_room( diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index d72b930d9337..b48fc12e5f91 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -643,7 +643,7 @@ def test_delete_expired_status(self): delete_id1 = channel.json_body["delete_id"] # go ahead - self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) + self.reactor.advance(PaginationHandler.CLEAR_PURGE_AFTER_MS / 1000 / 2) # second task channel = self.make_request( @@ -673,7 +673,7 @@ def test_delete_expired_status(self): # get status after more than clearing time for first task # second task is not cleared - self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) + self.reactor.advance(PaginationHandler.CLEAR_PURGE_AFTER_MS / 1000 / 2) channel = self.make_request( "GET", @@ -687,7 +687,7 @@ def test_delete_expired_status(self): self.assertEqual(delete_id2, channel.json_body["results"][0]["delete_id"]) # get status after more than clearing time for all tasks - self.reactor.advance(PaginationHandler.CLEAR_PURGE_TIME / 2) + self.reactor.advance(PaginationHandler.CLEAR_PURGE_AFTER_MS / 1000 / 2) channel = self.make_request( "GET",