Add admin API to remove a local user from a space #11358

squahtx · 2021-11-16T13:53:06Z

~~Includes changes from #11356~~

This PR's a bit on the long side unfortunately. ~~It ought to be reviewable commit by commit.~~

The commit history's become unhelpful. As a rough guide to the changes:

A new SpaceHierarchyHandler is added in synapse/handlers/space_hierarchy.py. It duplicates space-walking functionality in RoomSummaryHandler, but from the all-knowing perspective of the homeserver.
I'm not too happy about this. Then again, there's no easy way to refactor the code in RoomSummaryHandler, since it's very much tailored for pagination and the client-facing endpoint.
Tests for the new SpaceHierarchyHandler are added in tests/handlers/test_space_hierarchy.py
The new admin command is added in synapse/rest/admin/space.py.
Documentation for the new command is added in docs/usage/administration/admin_api/spaces.md.
Tests for the new admin command are added in tests/rest/admin/test_space.py.

This change makes mypy complain if the constants are ever reassigned, and, more usefully, makes mypy type them as `Literal`s instead of `str`s, allowing code of the following form to pass mypy: ```py def do_something(membership: Literal["join", "leave"], ...): ... do_something(Membership.JOIN, ...) ```

This change makes mypy type the constants as `Literal`s instead of `str`s, allowing code of the following form to pass mypy: ```py def do_something( membership: Literal[Membership.JOIN, Membership.LEAVE], ... ): ... do_something(Membership.JOIN, ...) ```

synapse/rest/admin/room_hierarchy.py

synapse/handlers/room_hierarchy.py

reivilibre · 2021-11-16T14:33:21Z

synapse/handlers/room_hierarchy.py

+
+        return descendants, inaccessible_room_ids
+
+    async def _get_room_children(


Suggested change

async def _get_room_children(

async def _get_space_children(

perhaps?

or get_rooms_in_space?

get_rooms_in_space doesn't convey the difference between direct children and descendants that I want.

get_space_(children|descendants) sounds plausible. I'm actually confused about the room vs space terminology because in RoomSummaryHandler, we have space summaries and room hierarchies.

get_rooms_in_space doesn't convey the difference between direct children and descendants that I want.

Hm, I guess that's fair enough. I don't think I'd imagine anything other than direct children for that, but it's hard to say I can't see where others would be coming from if they did. Best avoid ambiguous terminology after all..

get_space_(children|descendants) sounds plausible. I'm actually confused about the room vs space terminology because in RoomSummaryHandler, we have space summaries and room hierarchies.

This is probably a typical case of naming is hard and everything is awful :/.
However you are checking that the room is a space (bailing if it's not), so personally speaking I would call it get_space_children (especially since m.space.child exists).

Room hierarchies might be because the leaves in the tree are not spaces, they are merely rooms. Of course, the root node and interior nodes, being spaces, are also rooms, so the 'Room' in 'Room hierarchies' is so because it's the most general. I'm just making this up so might have nothing to do with the real reason...

I renamed a bunch of things in this PR to refer to spaces. Calling them spaces rather than rooms is a lot more intuitive for me at least: I had trouble finding room_summary.py because I was looking for something called space_* first.

Yeah, happy to conclude that it might be the most intuitive name.

reivilibre · 2021-11-16T14:42:07Z

synapse/handlers/room_hierarchy.py

+            # Check the room chunks previously returned over federation to see if we
+            # should really make a request.
+            # `federation_room_chunks` is intentionally not used earlier since we want
+            # to trust local data over data from federation.


I think it should be possible to disable the remote federation requests altogether for the kick-out feature; I think this could be a security issue. Thinking about it, a malicious remote HS could cause your user to leave arbitrary rooms.
To do so, the remote homeserver just needs to create a space and get it blessed by the root space, or one of its descendants. It can then place any room IDs it likes in there (or arbitrarily answer your queries).

If my understanding is wrong then ignore me :-).

That's a very good point. I'll have to think about how we can have the caller decide whether to recurse into spaces, without coupling this API to a particular use case (eg. no federation or no subspaces that a user cannot view)

It may also have an impact on MSC3216 (matrix-org/matrix-spec-proposals#3216)

My specific point here is that you might want a flag that doesn't do any network requests but perhaps returns some kind of placeholder 'You would need to query a remote HS to find out what rooms are in this space' at that point in the tree? (or omits them entirely; not too fussed at this point in time)

synapse/rest/admin/room_hierarchy.py

squahtx · 2021-11-18T16:01:58Z

It might be handy to get #11356 resolved first, so that those changes stop cluttering up the diff

This reverts commit b43d085.

reivilibre · 2021-11-30T11:43:12Z

It might be handy to get #11356 resolved first, so that those changes stop cluttering up the diff

should develop be perhaps merged into this branch to get that part of the diff to go away?

squahtx · 2021-11-30T12:20:01Z

It might be handy to get #11356 resolved first, so that those changes stop cluttering up the diff

should develop be perhaps merged into this branch to get that part of the diff to go away?

Yes, I'll try that!

richvdh

Includes changes from #11356

is this still true? could you update the description if not?

seems to have a conflict?

squahtx · 2021-12-14T18:12:59Z

The commit history's become unhelpful. As a rough guide to the changes:

A new SpaceHierarchyHandler is added in synapse/handlers/space_hierarchy.py. It duplicates space-walking functionality in RoomSummaryHandler, but from the all-knowing perspective of the homeserver.
I'm not too happy about this. Then again, there's no easy way to refactor the code in RoomSummaryHandler, since it's very much tailored for pagination and the client-facing endpoint.
Tests for the new SpaceHierarchyHandler are added in tests/handlers/test_space_hierarchy.py
The new admin command is added in synapse/rest/admin/space.py.
Documentation for the new command is added in docs/usage/administration/admin_api/spaces.md.
Tests for the new admin command are added in tests/rest/admin/test_space.py.

reivilibre

A few points, but otherwise happy for this one to (finally — sorry) make it :).

reivilibre · 2022-01-04T11:30:34Z

docs/usage/administration/admin_api/spaces.md

+`include_remote_spaces` controls whether to process subspaces that the
+local homeserver is not participating in. The listings of such subspaces
+have to be retrieved over federation and their accuracy cannot be
+guaranteed.


perhaps we should call out the security impact a bit more clearly to help security-paranoid people make a judgement.

Also worth noting the default behaviour: if the body is optional, what happens if you don't specify that?

reivilibre · 2022-01-04T11:31:14Z

docs/usage/administration/admin_api/spaces.md

+`left_rooms`: A list of rooms that the user has been made to leave.
+
+`inaccessible_rooms`: A list of rooms and spaces that the local
+homeserver is not in, and may have not been fully processed. Rooms may
+appear here if:
+  * The room is a space that the local homeserver is not in, and so its
+    full list of child rooms could not be determined.
+  * The room is inaccessible to the local homeserver, and it is not
+    known whether the room is a subspace containing further rooms.
+
+`failed_rooms`: A dictionary of errors encountered when leaving rooms.
+The keys of the dictionary are room IDs and the values of the dictionary
+are error messages.


lovely and clear, thanks :)

reivilibre · 2022-01-04T11:32:25Z

synapse/handlers/space_hierarchy.py

+        self,
+        space_id: str,
+        via: Optional[Iterable[str]] = None,
+        enable_federation: Optional[bool] = True,


I would be tempted to make this False by default since it will make it absolutely clear, at the callsites, that there's a danger of trusting remotes.

reivilibre · 2022-01-04T11:34:28Z

synapse/handlers/space_hierarchy.py

+        space_id: str,
+        via: Optional[Iterable[str]] = None,
+        enable_federation: Optional[bool] = True,
+    ) -> Tuple[Sequence[Tuple[str, Iterable[str]]], Sequence[str]]:


I think I would prefer this:

Suggested change

) -> Tuple[Sequence[Tuple[str, Iterable[str]]], Sequence[str]]:

) -> Tuple[Sequence[Tuple[str, Collection[str]]], Sequence[str]]:

I know we don't really have set standards around this but Iterable is potentially only once-iterable and I'd like to avoid conflating it too much.

reivilibre · 2022-01-04T11:52:43Z

synapse/rest/admin/space.py

+            except Exception as e:
+                failures[room_id] = str(e)


wondering if we want to log this exception with its traceback still so we can debug any issues that arise.

I suppose you could check the user's membership at this point to see if they were already kicked like you suggested, but it's unlikely to be worth the effort, so probably no need.

reivilibre · 2022-01-04T11:55:27Z

synapse/rest/admin/space.py

+        self._room_member_handler = hs.get_room_member_handler()
+        self._space_hierarchy_handler = hs.get_space_hierarchy_handler()
+
+    async def on_DELETE(


Do we expect this operation to take a long time?

I wonder if we should consider putting a ResponseCache on it (hopefully there are other good examples around) so that an admin API client can just keep retrying the request on timeout and then the final result will be given eventually (much like initial sync and I think a few others).
Mind you, that would be fine as a separate PR since I don't want to bog down this PR any further.
I think this will still work, it's just risking the output taking too long to get back.

Sean Quah added 6 commits November 16, 2021 13:37

Add RoomHierarchyHandler for walking space hierarchies

98873d7

Add tests for RoomHierarchyHandler

f004687

Add admin API to remove a local user from a space

75be1be

Add documentation for new admin API to remove a local user from a space

3371ec0

Add tests for admin API to remove a local user from a space

b8c228a

squahtx requested a review from a team as a code owner November 16, 2021 13:53

Add newsfile

ab89c60

squahtx force-pushed the squah/leave_space_admin_api branch from 61469ff to ab89c60 Compare November 16, 2021 13:55

squahtx commented Nov 16, 2021

View reviewed changes

synapse/rest/admin/room_hierarchy.py Outdated Show resolved Hide resolved

reivilibre reviewed Nov 16, 2021

View reviewed changes

reivilibre requested a review from a team November 16, 2021 16:12

squahtx commented Nov 16, 2021

View reviewed changes

synapse/rest/admin/room_hierarchy.py Outdated Show resolved Hide resolved

dklimpel mentioned this pull request Nov 16, 2021

Create an Admin API to remove a local user from non-public rooms in a space #11275

Open

reivilibre linked an issue Nov 17, 2021 that may be closed by this pull request

Create an Admin API to remove a local user from non-public rooms in a space #11275

Open

Sean Quah added 3 commits November 18, 2021 15:42

Refer to "spaces" instead of "rooms"

8627a45

Add missing type hints to new tests

f77da61

Fix bug/redundant code to do with handling of the root space id

b105bea

Revert "Convert strings in synapse.api.constants to enums or Final"

e5751a6

This reverts commit b43d085.

squahtx added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Nov 18, 2021

Sean Quah added 3 commits November 18, 2021 20:01

Leave rooms in a deterministic order

856656a

Add flag to control whether remote spaces are processed

94586c5

Add tests for remote spaces

64c5617

squahtx removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Nov 23, 2021

Merge branch 'develop' into squah/leave_space_admin_api

07580ac

richvdh suggested changes Dec 7, 2021

View reviewed changes

Merge branch 'develop' into squah/leave_space_admin_api

2e2f5c0

squahtx requested a review from a team December 14, 2021 18:01

reivilibre self-assigned this Jan 4, 2022

reivilibre suggested changes Jan 4, 2022

View reviewed changes

erikjohnston assigned squahtx and unassigned reivilibre Apr 7, 2022

squahtx closed this Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add admin API to remove a local user from a space #11358

Add admin API to remove a local user from a space #11358

squahtx commented Nov 16, 2021 •

edited

Loading

reivilibre Nov 16, 2021

squahtx Nov 16, 2021

reivilibre Nov 17, 2021

reivilibre Nov 17, 2021

squahtx Nov 18, 2021

reivilibre Jan 4, 2022

reivilibre Nov 16, 2021

squahtx Nov 16, 2021 •

edited

Loading

reivilibre Nov 17, 2021

squahtx commented Nov 18, 2021

reivilibre commented Nov 30, 2021

squahtx commented Nov 30, 2021

richvdh left a comment

squahtx commented Dec 14, 2021

reivilibre left a comment

reivilibre Jan 4, 2022

reivilibre Jan 4, 2022

reivilibre Jan 4, 2022

reivilibre Jan 4, 2022

reivilibre Jan 4, 2022

reivilibre Jan 4, 2022


		return descendants, inaccessible_room_ids

		async def _get_room_children(

	) -> Tuple[Sequence[Tuple[str, Iterable[str]]], Sequence[str]]:
	) -> Tuple[Sequence[Tuple[str, Collection[str]]], Sequence[str]]:

Add admin API to remove a local user from a space #11358

Add admin API to remove a local user from a space #11358

Conversation

squahtx commented Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squahtx Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squahtx commented Nov 18, 2021

reivilibre commented Nov 30, 2021

squahtx commented Nov 30, 2021

richvdh left a comment

Choose a reason for hiding this comment

squahtx commented Dec 14, 2021

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squahtx commented Nov 16, 2021 •

edited

Loading

squahtx Nov 16, 2021 •

edited

Loading