From da02259e2dae9a9daf03a19f2ae360d2f38237a6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 16:03:59 +0100 Subject: [PATCH 1/9] Consistently exclude support, deactivated, and appservice users --- synapse/handlers/user_directory.py | 25 ++++++------------- .../storage/databases/main/user_directory.py | 20 +++++++++++++-- 2 files changed, 25 insertions(+), 20 deletions(-) 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..a0576a565d21 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -246,7 +246,7 @@ 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 await self.is_excluded_from_user_dir(user_id): continue to_insert.add(user_id) @@ -259,7 +259,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: @@ -369,6 +369,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""" From c9913cf19a083f3cca4de1d72d5944c100c754fe Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 16:58:38 +0100 Subject: [PATCH 2/9] Test user dir rebuilding in storage, not handlers To do this: - move required helpers to the handlers test dir and pull them in via inheritance (ew) Additionally, we no longer rebuild the user dir the the user dir handlers tests. Instead, tests register their own users and that creates the required user_dir entries. --- tests/handlers/test_user_directory.py | 162 ++---------------------- tests/storage/test_user_directory.py | 174 ++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 153 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 266333c5532c..c0c5053df8e6 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -25,12 +25,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 = [ @@ -317,133 +323,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 +336,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 +400,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( diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 222e5d129d73..b0576452dbfc 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,9 +11,183 @@ # 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, Dict +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 room, login +from synapse.storage import DataStore +from synapse.types import UserID, create_requester +from tests.test_utils.event_injection import inject_member_event 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}) + ALICE = "@alice:a" BOB = "@bob:b" BOBBY = "@bobby:a" From ad7ea64398cdfd4b080b1660bb615da7b6a8a68d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:12:16 +0100 Subject: [PATCH 3/9] Fix test_disabling_room_list I think this now fails because I've changed the user directory code to require that users are not deactivated. I think this test was relying on some magic to sort of create a user based on the test case attribute "user_id". I don't think it was doing it properly by actually registering a user proper. --- tests/handlers/test_user_directory.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index c0c5053df8e6..13120cc72209 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -444,8 +444,6 @@ def _add_user_to_room( class TestUserDirSearchDisabled(unittest.HomeserverTestCase): - user_id = "@test:test" - servlets = [ user_directory.register_servlets, room.register_servlets, @@ -465,16 +463,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) @@ -482,7 +486,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) From acf94bfe74be899fd7f3f49d37c9b7d32993bfd1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:14:06 +0100 Subject: [PATCH 4/9] Test that exclusions apply when rebuilding dir --- tests/storage/test_user_directory.py | 49 ++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index b0576452dbfc..0b90d1c4b9b2 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -188,6 +188,55 @@ def test_populates_local_users(self): # 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 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 add AS 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_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" From 556566ff36a7648d4f7134e9def8486de55c3f5f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:45:04 +0100 Subject: [PATCH 5/9] Fix type hint for `user_directory_stream_pos` --- synapse/storage/databases/main/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index a0576a565d21..bde268cd7cd7 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -543,7 +543,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={}, From 969d0d13c45500570d734bfd9b0ac9ee0f25010e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:49:10 +0100 Subject: [PATCH 6/9] Use patch rather than assigning mocks This makes mypy happier --- tests/handlers/test_user_directory.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 13120cc72209..b06cc13451b5 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -12,7 +12,7 @@ # 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 @@ -125,19 +125,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)) + 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)) + mock.called_once_with(r_user_id) def test_reactivation_makes_regular_user_searchable(self): user = self.register_user("regular", "pass") From ff89fb8164ca583239d5aceaee2bbd90bba60b45 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:50:34 +0100 Subject: [PATCH 7/9] LintyMcLintFace --- tests/handlers/test_user_directory.py | 1 - tests/storage/test_user_directory.py | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index b06cc13451b5..0691944bc303 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,7 +11,6 @@ # 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 patch from urllib.parse import quote diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 0b90d1c4b9b2..78d6c1ffb2ab 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,16 +11,15 @@ # 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, Dict +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 room, login +from synapse.rest.client import login, room from synapse.storage import DataStore -from synapse.types import UserID, create_requester -from tests.test_utils.event_injection import inject_member_event + from tests.unittest import HomeserverTestCase, override_config @@ -237,6 +236,8 @@ def test_population_excludes_deactivated_user(self): 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" From ff177a868276d1dce44cd2876445e9f2ecda5bb2 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 17:57:16 +0100 Subject: [PATCH 8/9] Ch-ch-ch-ch-changelog --- changelog.d/10914.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10914.bugfix 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 From 2e32e1c1dd9461161d2cd8e21512c030c6aa8ab0 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 24 Sep 2021 18:43:11 +0100 Subject: [PATCH 9/9] Fix failing unit tests --- synapse/storage/databases/main/user_directory.py | 16 +++++++++------- tests/handlers/test_user_directory.py | 4 ++-- tests/storage/test_user_directory.py | 5 +++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index bde268cd7cd7..d6e33573f80d 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -238,9 +238,10 @@ 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() @@ -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( diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 0691944bc303..bfcb58791b7a 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -128,7 +128,7 @@ def test_handle_user_deactivated_support_user(self): 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)) - mock.not_called() + self.get_success(mock.not_called()) def test_handle_user_deactivated_regular_user(self): r_user_id = "@regular:test" @@ -139,7 +139,7 @@ def test_handle_user_deactivated_regular_user(self): 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)) - mock.called_once_with(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") diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 78d6c1ffb2ab..88bc6bd8ce36 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -213,6 +213,7 @@ def test_population_excludes_appservice_user(self): 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() @@ -220,8 +221,8 @@ def test_population_excludes_appservice_user(self): with patch.object(self.store, "exclusive_user_regex", mock_regex): self._purge_and_rebuild_user_dir() - # TODO add AS user to a public and private room. Check that - # users_in_public_rooms and users_who_share_private_rooms is empty. + # 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):