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

Add an option to disable purge in delete room admin API #7964

Merged
merged 5 commits into from
Jul 28, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7964.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add an option to purge room or not with delete room admin endpoint (`POST /_synapse/admin/v1/rooms/<room_id>/delete`). Contributed by @dklimpel.
11 changes: 7 additions & 4 deletions docs/admin_api/rooms.md
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ to the new room will have power level `-10` by default, and thus be unable to sp
If `block` is `True` it prevents new joins to the old room.

This API will remove all trace of the old room from your database after removing
all local users.
all local users. If you do not like to remove the trace set `purge` to `False`.
Copy link
Member

Choose a reason for hiding this comment

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

I would probably rewrite this sentence as something like:

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
several minutes or longer.

Expand All @@ -388,7 +388,8 @@ with a body of:
"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
"block": true,
"purge": true
}
```

Expand Down Expand Up @@ -430,8 +431,10 @@ The following JSON body parameters are available:
`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` - Optional. If set to `true`, this room will be added to a blocking list, preventing future attempts to
join the room. Defaults to `false`.
* `block` - Optional. If set to `true`, this room will be added to a blocking list, preventing
future attempts to join the room. Defaults to `false`.
* `purge` - Optional. If set to `true`, it will remove all trace of a room from your database.
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
Defaults to `true`.

The JSON body must not be empty. The body must be at least `{}`.

Expand Down
11 changes: 10 additions & 1 deletion synapse/rest/admin/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ async def on_POST(self, request, room_id):
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,
)

ret = await self.room_shutdown_handler.shutdown_room(
room_id=room_id,
new_room_user_id=content.get("new_room_user_id"),
Expand All @@ -113,7 +121,8 @@ async def on_POST(self, request, room_id):
)

# Purge room
await self.pagination_handler.purge_room(room_id)
if purge:
await self.pagination_handler.purge_room(room_id)

return (200, ret)

Expand Down
56 changes: 54 additions & 2 deletions tests/rest/admin/test_room.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,23 @@ def test_block_is_not_bool(self):
self.assertEqual(400, int(channel.result["code"]), msg=channel.result["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
"""
body = json.dumps({"purge": "NotBool"})

request, channel = self.make_request(
"POST",
self.url,
content=body.encode(encoding="utf_8"),
access_token=self.admin_user_tok,
)
self.render(request)

self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
self.assertEqual(Codes.BAD_JSON, channel.json_body["errcode"])

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.
Expand All @@ -297,7 +314,7 @@ def test_purge_room_and_block(self):
# Assert one user in room
self._is_member(room_id=self.room_id, user_id=self.other_user)

body = json.dumps({"block": True})
body = json.dumps({"block": True, "purge": True})

request, channel = self.make_request(
"POST",
Expand Down Expand Up @@ -331,7 +348,7 @@ def test_purge_room_and_not_block(self):
# Assert one user in room
self._is_member(room_id=self.room_id, user_id=self.other_user)

body = json.dumps({"block": False})
body = json.dumps({"block": False, "purge": True})

request, channel = self.make_request(
"POST",
Expand All @@ -351,6 +368,41 @@ def test_purge_room_and_not_block(self):
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 and do not purge it.
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
Members will not be moved to a new room and will not receive a message.
dklimpel marked this conversation as resolved.
Show resolved Hide resolved
"""
# 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)

body = json.dumps({"block": False, "purge": False})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo? Should this have been "block": True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I think so.
Neither block nor purge is certainly not a good use case.
My PR #11223 needs an update, too.

Copy link
Contributor

@DMRobertson DMRobertson Nov 1, 2021

Choose a reason for hiding this comment

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

Neither block nor purge is certainly not a good use case.

To check my understanding, I think no block and no purge would

  • kick all local users out of the room
  • retain events for that room
  • all users would be free rejoin

I agree that this isn't a useful mode of operation! Sounds like it ought to caught and rejected with 400 bad request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think you have to check everything. Maybe it's a edge case.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that someone might accidentally use the API in this way, and not realise that the room can come back to haunt them.

(Is there a use case for shutting down the room with block: False at all?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea for an use case {"block": False, "purge": False} and remove all users.


request, channel = self.make_request(
"POST",
self.url.encode("ascii"),
content=body.encode(encoding="utf_8"),
access_token=self.admin_user_tok,
)
self.render(request)

self.assertEqual(200, int(channel.result["code"]), msg=channel.result["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)
self.assertIn("local_aliases", channel.json_body)

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
Expand Down