requested changes

element-hq · Sep 17, 2024 · 22af98d · 22af98d
1 parent c92ca2d
commit 22af98d
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 62 deletions.
diff --git 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.
+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).
diff --git 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._
diff --git 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
@@ -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
@@ -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
@@ -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)