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

Allow admins to proactively block rooms #11228

Merged
merged 15 commits into from
Nov 9, 2021
Merged

Allow admins to proactively block rooms #11228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7e6e9da
Fix doc typo
Nov 1, 2021
33afbeb
Note that new_room_id may be null
Nov 1, 2021
47fd059
Avoid confusion with other meanings of "event".
Nov 1, 2021
033adb7
Enforce the type of the shutdown_room response
Nov 1, 2021
0ed7a31
Test that you can block an unknown room
Nov 1, 2021
933197b
Allow blocking an unknown room
Nov 1, 2021
3bd4984
Changelog
Nov 1, 2021
92efc62
Update tests/rest/admin/test_room.py
Nov 2, 2021
b820b60
Update changelog.d/11228.feature
Nov 5, 2021
8f9bf40
Update synapse/handlers/room.py
Nov 5, 2021
a2da438
Cast safety comment
Nov 5, 2021
de6aa37
Update docs(trings)
Nov 5, 2021
c5fccfa
Lint
Nov 5, 2021
e397068
Update docs/admin_api/rooms.md
Nov 9, 2021
5402a69
Update docstring to match
Nov 9, 2021
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
22 changes: 17 additions & 5 deletions synapse/handlers/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,14 +1365,26 @@ async def shutdown_room(
if not RoomID.is_valid(room_id):
raise SynapseError(400, "%s is not a legal room ID" % (room_id,))

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@DMRobertson DMRobertson Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I was a little reluctant here.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

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

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

if new_room_user_id is not None:
if not self.hs.is_mine_id(new_room_user_id):
raise SynapseError(
Expand Down
11 changes: 10 additions & 1 deletion synapse/rest/admin/rooms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,16 @@ async def _delete_room(

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

# Cast safety: I'm casting away the knowledge that this is a TypedDict.
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
# `ret` is an opaque dictionary blob as far as the rest of the app cares.
Expand Down