From f9fe3066b4004fe6db1516e74339a3c618177ff7 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 30 Sep 2021 14:14:38 +0100 Subject: [PATCH] Introduce `should_include_local_users_in_dir` We exclude three kinds of local users from the user_directory tables. At present we don't consistently exclude all three in the same places. This commit introduces a new function to gather those exclusion conditions together. Because we have to handle local and remote users in different ways, I've made that function only consider the case of remote users. It's the callers responsibility to make the local versus remote distinction clear and correct. A test fixup is required. The test now hits a path which makes db queries against the users table. The expected rows were missing, because we were using a dummy user that hadn't actually been registered. Broken-off from #10914. Changes By my reading this makes these changes: Incremental updates: * When an app service user registers or changes their profile, they will _not_ be added to the user directory. (Previously only support users were excluded). This is consistent with the logic that rebuilds the user directory. See also [the discussion here](https://github.com/matrix-org/synapse/pull/10914#discussion_r716859548). * When a deactivated user joins a room, they will _not_ be added to the user directory. Previously they were. (This probably never happens, but the previous source code allows it.) * When a room changes from public to private or vice versa, any deactivated or support users will _not_ be added to the directory. Previously they were. Rebuild: * When rebuilding the directory, exclude support and disabled users from room sharing tables. Previously only appservice users were excluded. * Exclude all three categories of local users when rebuilding the directory. Previously `_populate_user_directory_process_users` didn't do any exclusion. --- synapse/handlers/user_directory.py | 27 ++++++---------- .../storage/databases/main/user_directory.py | 31 +++++++++++++++++-- tests/handlers/test_user_directory.py | 26 ++++++++++------ 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f4430ce3c9aa..18d8c8744e75 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -132,12 +132,7 @@ async def handle_local_profile_change( # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - # Support users are for diagnostics and should not appear in the user directory. - is_support = await self.store.is_support_user(user_id) - # When change profile information of deactivated user it should not appear in the user directory. - is_deactivated = await self.store.get_user_deactivated_status(user_id) - - if not (is_support or is_deactivated): + if await self.store.should_include_local_user_in_dir(user_id): await self.store.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url ) @@ -229,8 +224,10 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None: else: logger.debug("Server is still in room: %r", room_id) - is_support = await self.store.is_support_user(state_key) - if not is_support: + include_in_dir = not self.is_mine_id( + state_key + ) or await self.store.should_include_local_user_in_dir(state_key) + if include_in_dir: if change is MatchChange.no_change: # Handle any profile changes await self._handle_profile_change( @@ -356,13 +353,7 @@ async def _handle_new_user( # First, if they're our user then we need to update for every user if self.is_mine_id(user_id): - - is_appservice = self.store.get_if_app_services_interested_in_user( - user_id - ) - - # We don't care about appservice users. - if not is_appservice: + if await self.store.should_include_local_user_in_dir(user_id): for other_user_id in other_users_in_room: if user_id == other_user_id: continue @@ -374,10 +365,10 @@ async def _handle_new_user( if user_id == other_user_id: continue - is_appservice = self.store.get_if_app_services_interested_in_user( + include_other_user = self.is_mine_id( other_user_id - ) - if self.is_mine_id(other_user_id) and not is_appservice: + ) and await self.store.should_include_local_user_in_dir(other_user_id) + if include_other_user: to_insert.add((other_user_id, user_id)) if to_insert: diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index c26e3e066f9d..4a93bd4b804c 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -238,6 +238,10 @@ def _get_next_batch( # Update each user in the user directory. for user_id, profile in users_with_profile.items(): + if self.hs.is_mine_id( + user_id + ) and not await self.should_include_local_user_in_dir(user_id): + continue await self.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url ) @@ -246,7 +250,9 @@ def _get_next_batch( if is_public: for user_id in users_with_profile: - if self.get_if_app_services_interested_in_user(user_id): + if self.hs.is_mine_id( + user_id + ) and not await self.should_include_local_user_in_dir(user_id): continue to_insert.add(user_id) @@ -259,7 +265,7 @@ def _get_next_batch( if not self.hs.is_mine_id(user_id): continue - if self.get_if_app_services_interested_in_user(user_id): + if not await self.should_include_local_user_in_dir(user_id): continue for other_user_id in users_with_profile: @@ -349,6 +355,9 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]: ) for user_id in users_to_work_on: + if not await self.should_include_local_user_in_dir(user_id): + continue + profile = await self.get_profileinfo(get_localpart_from_id(user_id)) await self.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url @@ -369,6 +378,24 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]: return len(users_to_work_on) + async def should_include_local_user_in_dir(self, user: str) -> bool: + """Certain classes of local user are omitted from the user directory. + Is this user one of them? + """ + # App service users aren't usually contactable, so exclude them. + if self.get_if_app_services_interested_in_user(user): + # TODO we might want to make this configurable for each app service + return False + + # Support users are for diagnostics and should not appear in the user directory. + if await self.is_support_user(user): + return False + + # Deactivated users aren't contactable, so should not appear in the user directory. + if await self.get_user_deactivated_status(user): + return False + return True + async def is_room_world_readable_or_publicly_joinable(self, room_id: str) -> bool: """Check if the room is either world_readable or publically joinable""" diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 2988befb21b4..866b418367a4 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -483,8 +483,6 @@ def _add_user_to_room( class TestUserDirSearchDisabled(unittest.HomeserverTestCase): - user_id = "@test:test" - servlets = [ user_directory.register_servlets, room.register_servlets, @@ -504,16 +502,21 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: def test_disabling_room_list(self) -> None: self.config.userdirectory.user_directory_search_enabled = True - # First we create a room with another user so that user dir is non-empty - # for our user - self.helper.create_room_as(self.user_id) + # Create two users and put them in the same room. + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") u2 = self.register_user("user2", "pass") - room = self.helper.create_room_as(self.user_id) - self.helper.join(room, user=u2) + u2_token = self.login(u2, "pass") + + room = self.helper.create_room_as(u1, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) - # Assert user directory is not empty + # Each should see the other when searching the user directory. channel = self.make_request( - "POST", b"user_directory/search", b'{"search_term":"user2"}' + "POST", + b"user_directory/search", + b'{"search_term":"user2"}', + access_token=u1_token, ) self.assertEquals(200, channel.code, channel.result) self.assertTrue(len(channel.json_body["results"]) > 0) @@ -521,7 +524,10 @@ def test_disabling_room_list(self) -> None: # Disable user directory and check search returns nothing self.config.userdirectory.user_directory_search_enabled = False channel = self.make_request( - "POST", b"user_directory/search", b'{"search_term":"user2"}' + "POST", + b"user_directory/search", + b'{"search_term":"user2"}', + access_token=u1_token, ) self.assertEquals(200, channel.code, channel.result) self.assertTrue(len(channel.json_body["results"]) == 0)