From 22af98dfbc5b040d20d9f7c3752bc759f77cf477 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 17 Sep 2024 12:49:33 -0700 Subject: [PATCH] requested changes --- changelog.d/17506.feature | 3 +- docs/admin_api/user_admin_api.md | 10 +-- synapse/handlers/admin.py | 9 ++- synapse/rest/admin/users.py | 13 +++ .../storage/databases/main/events_worker.py | 2 +- tests/rest/admin/test_user.py | 80 +++++++------------ 6 files changed, 55 insertions(+), 62 deletions(-) diff --git a/changelog.d/17506.feature b/changelog.d/17506.feature index ac1e76955c7..dc71e43fe3c 100644 --- a/changelog.d/17506.feature +++ b/changelog.d/17506.feature @@ -1 +1,2 @@ -Add an asynchronous Admin API endpoint to redact all a user's events, and an endpoint to check on the status of that redaction task. \ No newline at end of file +Add an asynchronous Admin API endpoint [to redact all a user's events](https://element-hq.github.io/synapse/v1.116/admin_api/user_admin_api.html#redact-all-the-events-of-a-user), +and [an endpoint to check on the status of that redaction task](https://element-hq.github.io/synapse/v1.116/admin_api/user_admin_api.html#check-the-status-of-a-redaction-process). \ No newline at end of file diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 148bd1ef522..ff74a87a383 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -1400,8 +1400,8 @@ _Added in Synapse 1.114.0._ The following JSON body parameters are optional: -- `reason` - Reason the redaction is being requested, ie "spam", "abuse", etc -- `limit` - a limit on the number of events to redact (events are redacted newest to oldest) in each room, defaults to 1000 if not provided +- `reason` - Reason the redaction is being requested, ie "spam", "abuse", etc. This will be included in the each redaction event, and be visible to users. +- `limit` - a limit on the number of the user's events to search for ones that can be redacted (events are redacted newest to oldest) in each room, defaults to 1000 if not provided ## Check the status of a redaction process @@ -1429,15 +1429,15 @@ A response body like the following is returned: The following parameters should be set in the URL: -* `redact_id` - The ID for this redaction, provided when the redaction was requested. +* `redact_id` - string - The ID for this redaction process, provided when the redaction was requested. **Response** The following fields are returned in the JSON response body: -- status: one of scheduled/active/completed/failed, indicating the status of the redaction job -- failed_redactions: a dict where the keys are event ids the process was unable to redact, if any, and the values are +- `status` - string - one of scheduled/active/completed/failed, indicating the status of the redaction job +- `failed_redactions` - dictionary - the keys of the dict are event ids the process was unable to redact, if any, and the values are the corresponding error that caused the redaction to fail _Added in Synapse 1.116.0._ \ No newline at end of file diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index a28c92d21aa..58d89080ffd 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -382,6 +382,9 @@ async def start_redact_events( 400, "Redact already in progress for user %s" % (user_id,) ) + if not limit: + limit = 1000 + redact_id = await self._task_scheduler.schedule_task( REDACT_ALL_EVENTS_ACTION_NAME, resource_id=user_id, @@ -426,9 +429,7 @@ async def _redact_all_events( reason = task.params.get("reason") limit = task.params.get("limit") - - if not limit: - limit = 1000 + assert limit is not None result: Mapping[str, Any] = ( task.result if task.result else {"failed_redactions": {}} @@ -451,7 +452,7 @@ async def _redact_all_events( if event.type == "m.room.member": content = event.content if content: - if content.get("membership") == "join": + if content.get("membership") == Membership.JOIN: pass else: continue diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 9dcf1164399..81dfb57a95d 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -1435,7 +1435,20 @@ async def on_POST( ) reason = body.get("reason") + if reason: + if not isinstance(reason, str): + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "If a reason is provided it must be a string.", + ) + limit = body.get("limit") + if limit: + if not isinstance(limit, int) or limit <= 0: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "If limit is provided it must be a non-negative integer greater than 0.", + ) if not rooms: rooms = await self._store.get_rooms_for_user(user_id) diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index f27a9358e81..c029228422d 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -2517,7 +2517,7 @@ def _get_events_by_user_in_room_txn( offset = 0 batch_size = 100 - if batch_size < limit: + if batch_size > limit: batch_size = limit selected_ids: List[str] = [] diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index c5ef63334e0..ef918efe495 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -5113,23 +5113,24 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.spam_checker = hs.get_module_api_callbacks().spam_checker - def test_redact_messages_all_rooms(self) -> None: - """ - Test that request to redact events in all rooms user is member of is successful - """ # create rooms - room versions 11+ store the `redacts` key in content while # earlier ones don't so we use a mix of room versions - rm1 = self.helper.create_room_as( + self.rm1 = self.helper.create_room_as( self.admin, tok=self.admin_tok, room_version="7" ) - rm2 = self.helper.create_room_as( + self.rm2 = self.helper.create_room_as(self.admin, tok=self.admin_tok) + self.rm3 = self.helper.create_room_as( self.admin, tok=self.admin_tok, room_version="11" ) - rm3 = self.helper.create_room_as(self.admin, tok=self.admin_tok) + + def test_redact_messages_all_rooms(self) -> None: + """ + Test that request to redact events in all rooms user is member of is successful + """ # join rooms, send some messages originals = [] - for rm in [rm1, rm2, rm3]: + for rm in [self.rm1, self.rm2, self.rm3]: join = self.helper.join(rm, self.bad_user, tok=self.bad_user_tok) originals.append(join["event_id"]) for i in range(15): @@ -5149,7 +5150,7 @@ def test_redact_messages_all_rooms(self) -> None: self.assertEqual(channel.code, 200) matched = [] - for rm in [rm1, rm2, rm3]: + for rm in [self.rm1, self.rm2, self.rm3]: filter = json.dumps({"types": [EventTypes.Redaction]}) channel = self.make_request( "GET", @@ -5171,16 +5172,9 @@ def test_redact_messages_specific_rooms(self) -> None: """ Test that request to redact events in specified rooms user is member of is successful """ - rm1 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="7" - ) - rm2 = self.helper.create_room_as(self.admin, tok=self.admin_tok) - rm3 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="11" - ) originals = [] - for rm in [rm1, rm2, rm3]: + for rm in [self.rm1, self.rm2, self.rm3]: join = self.helper.join(rm, self.bad_user, tok=self.bad_user_tok) originals.append(join["event_id"]) for i in range(15): @@ -5194,13 +5188,13 @@ def test_redact_messages_specific_rooms(self) -> None: channel = self.make_request( "POST", f"/_synapse/admin/v1/user/{self.bad_user}/redact", - content={"rooms": [rm1, rm3]}, + content={"rooms": [self.rm1, self.rm3]}, access_token=self.admin_tok, ) self.assertEqual(channel.code, 200) # messages in requested rooms are redacted - for rm in [rm1, rm3]: + for rm in [self.rm1, self.rm3]: filter = json.dumps({"types": [EventTypes.Redaction]}) channel = self.make_request( "GET", @@ -5221,7 +5215,7 @@ def test_redact_messages_specific_rooms(self) -> None: self.assertEqual(len(matches), 16) channel = self.make_request( - "GET", f"rooms/{rm2}/messages?limit=50", access_token=self.admin_tok + "GET", f"rooms/{self.rm2}/messages?limit=50", access_token=self.admin_tok ) self.assertEqual(channel.code, 200) @@ -5231,32 +5225,24 @@ def test_redact_messages_specific_rooms(self) -> None: self.fail("found redaction in room 2") def test_redact_status(self) -> None: - rm1 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="7" - ) - rm2 = self.helper.create_room_as(self.admin, tok=self.admin_tok) - rm3 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="11" - ) - - originals = [] - for rm in [rm1, rm2, rm3]: + rm2_originals = [] + for rm in [self.rm1, self.rm2, self.rm3]: join = self.helper.join(rm, self.bad_user, tok=self.bad_user_tok) - if rm == rm2: - originals.append(join["event_id"]) + if rm == self.rm2: + rm2_originals.append(join["event_id"]) for i in range(5): event = {"body": f"hello{i}", "msgtype": "m.text"} res = self.helper.send_event( rm, "m.room.message", event, tok=self.bad_user_tok ) - if rm == rm2: - originals.append(res["event_id"]) + if rm == self.rm2: + rm2_originals.append(res["event_id"]) # redact messages in rooms 1 and 3 channel = self.make_request( "POST", f"/_synapse/admin/v1/user/{self.bad_user}/redact", - content={"rooms": [rm1, rm3]}, + content={"rooms": [self.rm1, self.rm3]}, access_token=self.admin_tok, ) self.assertEqual(channel.code, 200) @@ -5280,7 +5266,7 @@ async def check_event_for_spam(event: str) -> str: channel3 = self.make_request( "POST", f"/_synapse/admin/v1/user/{self.bad_user}/redact", - content={"rooms": [rm2]}, + content={"rooms": [self.rm2]}, access_token=self.admin_tok, ) self.assertEqual(channel.code, 200) @@ -5296,22 +5282,14 @@ async def check_event_for_spam(event: str) -> str: failed_redactions = channel4.json_body.get("failed_redactions") assert failed_redactions is not None matched = [] - for original in originals: + for original in rm2_originals: if failed_redactions.get(original) is not None: matched.append(original) - self.assertEqual(len(matched), len(originals)) + self.assertEqual(len(matched), len(rm2_originals)) def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: - rm1 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="7" - ) - rm2 = self.helper.create_room_as(self.admin, tok=self.admin_tok) - rm3 = self.helper.create_room_as( - self.admin, tok=self.admin_tok, room_version="11" - ) - originals = [] - for rm in [rm1, rm2, rm3]: + for rm in [self.rm1, self.rm2, self.rm3]: join = self.helper.join(rm, self.bad_user, tok=self.bad_user_tok) originals.append(join["event_id"]) for i in range(5): @@ -5322,7 +5300,7 @@ def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: originals.append(res["event_id"]) # kick user from rooms 1 and 3 - for r in [rm1, rm2]: + for r in [self.rm1, self.rm2]: channel = self.make_request( "POST", f"/_matrix/client/r0/rooms/{r}/kick", @@ -5335,7 +5313,7 @@ def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: channel1 = self.make_request( "POST", f"/_synapse/admin/v1/user/{self.bad_user}/redact", - content={"rooms": [rm1, rm3]}, + content={"rooms": [self.rm1, self.rm3]}, access_token=self.admin_tok, ) self.assertEqual(channel1.code, 200) @@ -5355,7 +5333,7 @@ def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: # ban user channel3 = self.make_request( "POST", - f"/_matrix/client/r0/rooms/{rm2}/ban", + f"/_matrix/client/r0/rooms/{self.rm2}/ban", content={"reason": "being a bummer", "user_id": self.bad_user}, access_token=self.admin_tok, ) @@ -5365,7 +5343,7 @@ def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: channel4 = self.make_request( "POST", f"/_synapse/admin/v1/user/{self.bad_user}/redact", - content={"rooms": [rm2]}, + content={"rooms": [self.rm2]}, access_token=self.admin_tok, ) self.assertEqual(channel4.code, 200)