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

Don't remove local users from dir when the leave their last room #11103

Merged
merged 4 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from all 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/11103.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix local users who left all their rooms being removed from the user directory, even if the "search_all_users" config option was enabled.
13 changes: 8 additions & 5 deletions synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,16 +415,19 @@ async def _handle_remove_user(self, room_id: str, user_id: str) -> None:
room_id: The room ID that user left or stopped being public that
user_id
"""
logger.debug("Removing user %r", user_id)
logger.debug("Removing user %r from room %r", user_id, room_id)

# Remove user from sharing tables
await self.store.remove_user_who_share_room(user_id, room_id)

# Are they still in any rooms? If not, remove them entirely.
rooms_user_is_in = await self.store.get_user_dir_rooms_user_is_in(user_id)
# Additionally, if they're a remote user and we're no longer joined
# to any rooms they're in, remove them from the user directory.
if not self.is_mine_id(user_id):
rooms_user_is_in = await self.store.get_user_dir_rooms_user_is_in(user_id)

if len(rooms_user_is_in) == 0:
await self.store.remove_from_user_dir(user_id)
if len(rooms_user_is_in) == 0:
logger.debug("Removing user %r from directory", user_id)
await self.store.remove_from_user_dir(user_id)

async def _handle_possible_remote_profile_change(
self,
Expand Down
50 changes: 50 additions & 0 deletions tests/handlers/test_user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,56 @@ def _add_user_to_room(
self.hs.get_storage().persistence.persist_event(event, context)
)

def test_local_user_leaving_room_remains_in_user_directory(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Is there already a test that remote users are removed from the user directory when they leave all rooms?

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 there's any tests of how remote users are handled in the directory at alL!

I considered writing one here, but tbh I've been putting that off. I'm aiming to get the handling of local users into a good place so that I can address remote users (and the complications of federation!) afterwards. I think it'll be easier to deal with the remote case holistically rather than piecemeal.

Copy link
Member

Choose a reason for hiding this comment

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

I think it'll be easier to deal with the remote case holistically rather than piecemeal.

Can you file an issue about adding tests for remote user in the room directory? At least so we can track this work!

"""We've chosen to simplify the user directory's implementation by
always including local users. Ensure this invariant is maintained when
a local user
- leaves a room, and
- leaves the last room they're in which is visible to this server.

This is user-visible if the "search_all_users" config option is on: the
local user who left a room would no longer be searchable if this test fails!
"""
alice = self.register_user("alice", "pass")
alice_token = self.login(alice, "pass")
bob = self.register_user("bob", "pass")
bob_token = self.login(bob, "pass")

# Alice makes two public rooms, which Bob joins.
room1 = self.helper.create_room_as(alice, is_public=True, tok=alice_token)
room2 = self.helper.create_room_as(alice, is_public=True, tok=alice_token)
self.helper.join(room1, bob, tok=bob_token)
self.helper.join(room2, bob, tok=bob_token)

# The user directory tables are updated.
users, in_public, in_private = self.get_success(
self.user_dir_helper.get_tables()
)
self.assertEqual(users, {alice, bob})
self.assertEqual(
in_public, {(alice, room1), (alice, room2), (bob, room1), (bob, room2)}
)
self.assertEqual(in_private, set())

# Alice leaves one room. She should still be in the directory.
self.helper.leave(room1, alice, tok=alice_token)
users, in_public, in_private = self.get_success(
self.user_dir_helper.get_tables()
)
self.assertEqual(users, {alice, bob})
self.assertEqual(in_public, {(alice, room2), (bob, room1), (bob, room2)})
self.assertEqual(in_private, set())

# Alice leaves the other. She should still be in the directory.
self.helper.leave(room2, alice, tok=alice_token)
self.wait_for_background_updates()
users, in_public, in_private = self.get_success(
self.user_dir_helper.get_tables()
)
self.assertEqual(users, {alice, bob})
self.assertEqual(in_public, {(bob, room1), (bob, room2)})
self.assertEqual(in_private, set())


class TestUserDirSearchDisabled(unittest.HomeserverTestCase):
servlets = [
Expand Down