diff --git a/changelog.d/10914.bugfix b/changelog.d/10914.bugfix new file mode 100644 index 000000000000..b4f1c228ea0e --- /dev/null +++ b/changelog.d/10914.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where rebuilding the user directory wouldn't exclude support and disabled users. \ No newline at end of file diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index b91e7cb501c5..78922685771d 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 not await self.store.is_excluded_from_user_dir(user_id): await self.store.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url ) @@ -229,8 +224,8 @@ 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: + excluded = await self.store.is_excluded_from_user_dir(state_key) + if not excluded: if change is MatchChange.no_change: # Handle any profile changes await self._handle_profile_change( @@ -357,12 +352,8 @@ 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: + excluded = await self.store.is_excluded_from_user_dir(user_id) + if not excluded: for other_user_id in other_users_in_room: if user_id == other_user_id: continue @@ -374,10 +365,8 @@ async def _handle_new_user( if user_id == other_user_id: continue - is_appservice = self.store.get_if_app_services_interested_in_user( - other_user_id - ) - if self.is_mine_id(other_user_id) and not is_appservice: + excluded = await self.store.is_excluded_from_user_dir(other_user_id) + if not excluded: 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 90d65edc4267..d6e33573f80d 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -238,15 +238,16 @@ def _get_next_batch( # Update each user in the user directory. for user_id, profile in users_with_profile.items(): - await self.update_profile_in_user_dir( - user_id, profile.display_name, profile.avatar_url - ) + if not await self.is_excluded_from_user_dir(user_id): + await self.update_profile_in_user_dir( + user_id, profile.display_name, profile.avatar_url + ) to_insert = set() if is_public: for user_id in users_with_profile: - if self.get_if_app_services_interested_in_user(user_id): + if await self.is_excluded_from_user_dir(user_id): continue to_insert.add(user_id) @@ -259,7 +260,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 await self.is_excluded_from_user_dir(user_id): continue for other_user_id in users_with_profile: @@ -349,10 +350,11 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]: ) for user_id in users_to_work_on: - 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 - ) + if not await self.is_excluded_from_user_dir(user_id): + 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 + ) # We've finished processing a user. Delete it from the table. await self.db_pool.simple_delete_one( @@ -369,6 +371,22 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]: return len(users_to_work_on) + async def is_excluded_from_user_dir(self, user: str) -> bool: + """Certain classes of local user are omitted from the user directory. + Is this user one of them? + """ + if self.hs.is_mine_id(user): + # Support users are for diagnostics and should not appear in the user directory. + if await self.is_support_user(user): + return True + # Deactivated users aren't contactable, so should not appear in the user directory. + if await self.get_user_deactivated_status(user): + return True + # App service users aren't usually contactable, so exclude them. + if self.get_if_app_services_interested_in_user(user): + return True + return False + 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""" @@ -527,7 +545,7 @@ async def get_user_in_directory(self, user_id: str) -> Optional[Dict[str, str]]: desc="get_user_in_directory", ) - async def update_user_directory_stream_pos(self, stream_id: int) -> None: + async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> None: await self.db_pool.simple_update_one( table="user_directory_stream_pos", keyvalues={}, diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 266333c5532c..bfcb58791b7a 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,8 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import List, Tuple -from unittest.mock import Mock +from unittest.mock import patch from urllib.parse import quote from twisted.internet import defer @@ -25,12 +24,18 @@ from synapse.types import create_requester from tests import unittest +from tests.storage.test_user_directory import GetUserDirectoryTables from tests.unittest import override_config -class UserDirectoryTestCase(unittest.HomeserverTestCase): - """ - Tests the UserDirectoryHandler. +class UserDirectoryTestCase(GetUserDirectoryTables, unittest.HomeserverTestCase): + """Tests the UserDirectoryHandler. + + We're broadly testing two kinds of things here. + + 1. Check that we correctly update the user directory in response + to events (e.g. join a room, leave a room, change name, make public) + 2. Check that the search logic behaves as expected. """ servlets = [ @@ -119,19 +124,22 @@ def test_handle_user_deactivated_support_user(self): user_id=s_user_id, password_hash=None, user_type=UserTypes.SUPPORT ) ) - - self.store.remove_from_user_dir = Mock(return_value=defer.succeed(None)) - self.get_success(self.handler.handle_local_user_deactivated(s_user_id)) - self.store.remove_from_user_dir.not_called() + with patch.object( + self.store, "remove_from_user_dir", return_value=defer.succeed(None) + ) as mock: + self.get_success(self.handler.handle_local_user_deactivated(s_user_id)) + self.get_success(mock.not_called()) def test_handle_user_deactivated_regular_user(self): r_user_id = "@regular:test" self.get_success( self.store.register_user(user_id=r_user_id, password_hash=None) ) - self.store.remove_from_user_dir = Mock(return_value=defer.succeed(None)) - self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) - self.store.remove_from_user_dir.called_once_with(r_user_id) + with patch.object( + self.store, "remove_from_user_dir", return_value=defer.succeed(None) + ) as mock: + self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) + self.get_success(mock.called_once_with(r_user_id)) def test_reactivation_makes_regular_user_searchable(self): user = self.register_user("regular", "pass") @@ -317,133 +325,6 @@ def test_legacy_spam_checker(self): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - def _compress_shared(self, shared): - """ - Compress a list of users who share rooms dicts to a list of tuples. - """ - r = set() - for i in shared: - r.add((i["user_id"], i["other_user_id"], i["room_id"])) - return r - - def get_users_in_public_rooms(self) -> List[Tuple[str, str]]: - r = self.get_success( - self.store.db_pool.simple_select_list( - "users_in_public_rooms", None, ("user_id", "room_id") - ) - ) - retval = [] - for i in r: - retval.append((i["user_id"], i["room_id"])) - return retval - - def get_users_who_share_private_rooms(self) -> List[Tuple[str, str, str]]: - return self.get_success( - self.store.db_pool.simple_select_list( - "users_who_share_private_rooms", - None, - ["user_id", "other_user_id", "room_id"], - ) - ) - - def _add_background_updates(self): - """ - Add the background updates we need to run. - """ - # Ugh, have to reset this flag - self.store.db_pool.updates._all_done = False - - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_createtables", - "progress_json": "{}", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_process_rooms", - "progress_json": "{}", - "depends_on": "populate_user_directory_createtables", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_process_users", - "progress_json": "{}", - "depends_on": "populate_user_directory_process_rooms", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_cleanup", - "progress_json": "{}", - "depends_on": "populate_user_directory_process_users", - }, - ) - ) - - def test_initial(self): - """ - The user directory's initial handler correctly updates the search tables. - """ - u1 = self.register_user("user1", "pass") - u1_token = self.login(u1, "pass") - u2 = self.register_user("user2", "pass") - u2_token = self.login(u2, "pass") - u3 = self.register_user("user3", "pass") - u3_token = self.login(u3, "pass") - - room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) - self.helper.invite(room, src=u1, targ=u2, tok=u1_token) - self.helper.join(room, user=u2, tok=u2_token) - - private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) - self.helper.invite(private_room, src=u1, targ=u3, tok=u1_token) - self.helper.join(private_room, user=u3, tok=u3_token) - - self.get_success(self.store.update_user_directory_stream_pos(None)) - self.get_success(self.store.delete_all_from_user_dir()) - - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() - - # Nothing updated yet - self.assertEqual(shares_private, []) - self.assertEqual(public_users, []) - - # Do the initial population of the user directory via the background update - self._add_background_updates() - - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() - - # User 1 and User 2 are in the same public room - self.assertEqual(set(public_users), {(u1, room), (u2, room)}) - - # User 1 and User 3 share private rooms - self.assertEqual( - self._compress_shared(shares_private), - {(u1, u3, private_room), (u3, u1, private_room)}, - ) - def test_initial_share_all_users(self): """ Search all users = True means that a user does not have to share a @@ -457,20 +338,6 @@ def test_initial_share_all_users(self): self.register_user("user2", "pass") u3 = self.register_user("user3", "pass") - # Wipe the user dir - self.get_success(self.store.update_user_directory_stream_pos(None)) - self.get_success(self.store.delete_all_from_user_dir()) - - # Do the initial population of the user directory via the background update - self._add_background_updates() - - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - shares_private = self.get_users_who_share_private_rooms() public_users = self.get_users_in_public_rooms() @@ -535,15 +402,6 @@ def test_prefer_local_users(self): local_users = [local_user_1, local_user_2, local_user_3] remote_users = [remote_user_1, remote_user_2, remote_user_3] - # Populate the user directory via background update - self._add_background_updates() - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - # The local searching user searches for the term "user", which other users have # in their user id results = self.get_success( @@ -588,8 +446,6 @@ def _add_user_to_room( class TestUserDirSearchDisabled(unittest.HomeserverTestCase): - user_id = "@test:test" - servlets = [ user_directory.register_servlets, room.register_servlets, @@ -609,16 +465,22 @@ def make_homeserver(self, reactor, clock): def test_disabling_room_list(self): 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) + 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") + + # First we create a room with another user u2, so that user dir is + # non-empty for our user u1 + room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) # Assert user directory is not empty 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) @@ -626,7 +488,10 @@ def test_disabling_room_list(self): # 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) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 222e5d129d73..88bc6bd8ce36 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,9 +11,234 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from typing import Dict, List, Tuple +from unittest.mock import Mock, patch + +from synapse.api.constants import UserTypes +from synapse.appservice import ApplicationService +from synapse.rest import admin +from synapse.rest.client import login, room +from synapse.storage import DataStore from tests.unittest import HomeserverTestCase, override_config + +class GetUserDirectoryTables(HomeserverTestCase): + """These helpers aren't present on the store itself. We want to use them + here and in the handler's tests too. + """ + + store: DataStore + + def get_users_in_public_rooms(self) -> List[Tuple[str, str]]: + r = self.get_success( + self.store.db_pool.simple_select_list( + "users_in_public_rooms", None, ("user_id", "room_id") + ) + ) + retval = [] + for i in r: + retval.append((i["user_id"], i["room_id"])) + return retval + + def get_users_who_share_private_rooms(self) -> List[Tuple[str, str, str]]: + return self.get_success( + self.store.db_pool.simple_select_list( + "users_who_share_private_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + ) + + def get_users_in_user_directory(self) -> Dict[str, str]: + # Just the set of usernames for now + r = self.get_success( + self.store.db_pool.simple_select_list( + "user_directory", None, ("user_id", "display_name") + ) + ) + return {entry["user_id"]: entry["display_name"] for entry in r} + + def _compress_shared(self, shared): + """ + Compress a list of users who share rooms dicts to a list of tuples. + """ + r = set() + for i in shared: + r.add((i["user_id"], i["other_user_id"], i["room_id"])) + return r + + +class UserDirectoryInitialPopulationTestcase( + GetUserDirectoryTables, HomeserverTestCase +): + """Ensure that the initial background process creates the user directory data + as intended. + + See also tests/handlers/test_user_directory.py for similar checks. They + test the incremental updates, rather than the big batch of updates. + """ + + servlets = [ + login.register_servlets, + admin.register_servlets_for_client_rest_resource, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + + def _purge_and_rebuild_user_dir(self): + """Nuke the user directory tables, start the background process to + repopulate them, and wait for the process to complete. This allows us + to inspect the outcome of the background process alone, without any of + the other incremental updates. + """ + self.get_success(self.store.update_user_directory_stream_pos(None)) + self.get_success(self.store.delete_all_from_user_dir()) + + shares_private = self.get_users_who_share_private_rooms() + public_users = self.get_users_in_public_rooms() + + # Nothing updated yet + self.assertEqual(shares_private, []) + self.assertEqual(public_users, []) + + # Ugh, have to reset this flag + self.store.db_pool.updates._all_done = False + + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_createtables", + "progress_json": "{}", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_process_rooms", + "progress_json": "{}", + "depends_on": "populate_user_directory_createtables", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_process_users", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_rooms", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_cleanup", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_users", + }, + ) + ) + + while not self.get_success( + self.store.db_pool.updates.has_completed_background_updates() + ): + self.get_success( + self.store.db_pool.updates.do_next_background_update(100), by=0.1 + ) + + def test_populates_local_users(self): + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + u2_token = self.login(u2, "pass") + u3 = self.register_user("user3", "pass") + u3_token = self.login(u3, "pass") + + room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.helper.invite(room, src=u1, targ=u2, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) + + private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) + self.helper.invite(private_room, src=u1, targ=u3, tok=u1_token) + self.helper.join(private_room, user=u3, tok=u3_token) + + self._purge_and_rebuild_user_dir() + + shares_private = self.get_users_who_share_private_rooms() + public_users = self.get_users_in_public_rooms() + + # User 1 and User 2 are in the same public room + self.assertEqual(set(public_users), {(u1, room), (u2, room)}) + + # User 1 and User 3 share private rooms + self.assertEqual( + self._compress_shared(shares_private), + {(u1, u3, private_room), (u3, u1, private_room)}, + ) + + # All three should have entries in the directory + self.assertEqual(set(self.get_users_in_user_directory().keys()), {u1, u2, u3}) + + def test_population_excludes_support_user(self): + support = "@support1:test" + self.get_success( + self.store.register_user( + user_id=support, password_hash=None, user_type=UserTypes.SUPPORT + ) + ) + + self._purge_and_rebuild_user_dir() + # TODO add support user to a public and private room. Check that + # users_in_public_rooms and users_who_share_private_rooms is empty. + self.assertEqual(self.get_users_in_user_directory(), {}) + + def test_population_excludes_appservice_user(self): + as_token = "i_am_an_app_service" + appservice = ApplicationService( + as_token, + self.hs.config.server_name, + id="1234", + namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]}, + sender="@as:test", + ) + self.store.services_cache.append(appservice) + + as_user = "@as_user_potato:test" + self.get_success(self.store.register_user(user_id=as_user, password_hash=None)) + # TODO add AS user to a public and private room. + + # TODO can we configure the app service up front somehow? This is a hack. + mock_regex = Mock() + mock_regex.match = lambda user_id: user_id == as_user + with patch.object(self.store, "exclusive_user_regex", mock_regex): + self._purge_and_rebuild_user_dir() + + # TODO after putting AS userin a room, check that + # users_in_public_rooms and users_who_share_private_rooms is empty. + self.assertEqual(self.get_users_in_user_directory(), {}) + + def test_population_excludes_deactivated_user(self): + user = self.register_user("rip", "pass") + user_token = self.login(user, "pass") + self.helper.create_room_as(user, is_public=True, tok=user_token) + self.helper.create_room_as(user, is_public=False, tok=user_token) + self.get_success(self.store.set_user_deactivated_status(user, True)) + + self._purge_and_rebuild_user_dir() + + self.assertEqual(self.get_users_in_public_rooms(), []) + self.assertEqual(self.get_users_who_share_private_rooms(), []) + self.assertEqual(self.get_users_in_user_directory(), {}) + + ALICE = "@alice:a" BOB = "@bob:b" BOBBY = "@bobby:a"