From e83e24f0a16081a502beb8c0d2e315e0f07a98e4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Sep 2021 19:09:50 +0100 Subject: [PATCH 01/15] Improve user directory tests - Distinguish between tests of the incremental updates (handler) versus the initial background process (storage) - Add test cases to futher probe that initial background process - Add test case to check that reactivated users are added back to the directory --- tests/handlers/test_user_directory.py | 202 +++++-------------- tests/storage/test_user_directory.py | 269 ++++++++++++++++++++++++++ 2 files changed, 316 insertions(+), 155 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index a251f77f9c2e..023ba28c6c36 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,24 +11,36 @@ # 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 twisted.internet import defer import synapse.rest.admin -from synapse.api.constants import EventTypes, RoomEncryptionAlgorithms, UserTypes +from synapse.api.constants import ( + EventTypes, + Membership, + RoomEncryptionAlgorithms, + UserTypes, +) from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.rest.client import login, room, user_directory from synapse.storage.roommember import ProfileInfo +from synapse.types import create_requester from tests import unittest -from tests.unittest import override_config +from tests.storage.test_user_directory import GetUserDirectoryTables +from tests.unittest import HomeserverTestCase, override_config -class UserDirectoryTestCase(unittest.HomeserverTestCase): +class UserDirectoryTestCase(GetUserDirectoryTables, 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 = [ @@ -131,6 +143,36 @@ def test_handle_user_deactivated_regular_user(self): self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) self.store.remove_from_user_dir.called_once_with(r_user_id) + def test_reactivation_makes_regular_user_searchable(self): + user = self.register_user("regular", "pass") + user_token = self.login(user, "pass") + admin_user = self.register_user("admin", "pass", admin=True) + + # Ensure the regular user is publicly visible and searchable. + public_room = self.helper.create_room_as(user, is_public=True, tok=user_token) + s = self.get_success(self.handler.search_users(admin_user, user, 10)) + self.assertEqual(len(s["results"]), 1) + self.assertEqual(s["results"][0]["user_id"], user) + + # Deactivate the user and check they're not searchable. + deactivate_handler = self.hs._deactivate_account_handler + self.get_success( + deactivate_handler.deactivate_account( + user, erase_data=False, requester=create_requester(admin_user) + ) + ) + s = self.get_success(self.handler.search_users(admin_user, user, 10)) + self.assertEqual(s["results"], []) + + # Reactivate the user and make them publicly visible again. + self.get_success(deactivate_handler.activate_account(user)) + self.inject_room_member(public_room, user, Membership.JOIN) + + # Check they're searchable. + s = self.get_success(self.handler.search_users(admin_user, user, 10)) + self.assertEqual(len(s["results"]), 1) + self.assertEqual(s["results"][0]["user_id"], user) + def test_private_room(self): """ A user can be searched for only by people that are either in a public @@ -371,134 +413,7 @@ 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): + def test_search_all_users(self): """ Search all users = True means that a user does not have to share a private room with the searching user or be in a public room to be search @@ -511,20 +426,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() @@ -589,15 +490,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..98066b23aa1a 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,9 +11,278 @@ # 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, Set, Tuple +import synapse +from synapse.api.constants import UserTypes +from synapse.appservice import ApplicationService +from synapse.rest.client import login, room +from synapse.storage import DataStore + +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) -> Set[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"] 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, + synapse.rest.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(self.get_users_in_user_directory(), {u1, u2, u3}) + + def test_populates_users_in_zero_rooms(self): + billy_no_mates = self.register_user("user2", "pass") + self._purge_and_rebuild_user_dir() + self.assertEqual(self.get_users_in_user_directory(), {billy_no_mates}) + self.assertEqual(self.get_users_in_public_rooms(), []) + self.assertEqual(self.get_users_who_share_private_rooms(), []) + + 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(), set()) + + 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", + ) + as_user = "@as_user_potato:test" + self.hs.get_datastore().services_cache.append(appservice) + self.get_success(self.store.register_user(user_id=as_user, password_hash=None)) + + 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(), set()) + + 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(), set()) + + def test_populates_remote_and_local_users(self): + """All local users and remote users have entries in the user_directory table. + + Test normal local user in a room is in the user directory. + Test remote user in a public room is in the user directory. + Test remote user in a private room is in the user directory. + """ + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + + remote1 = "@c:other.server" + remote2 = "@d:other.server" + + public_room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.get_success( + inject_member_event( + self.hs, + public_room, + remote1, + "join", + ) + ) + + private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) + self.get_success( + inject_member_event(self.hs, public_room, u1, "invite", remote2) + ) + self.get_success( + inject_member_event( + self.hs, + private_room, + remote2, + "join", + ) + ) + + self._purge_and_rebuild_user_dir() + + users_in_directory = self.get_users_in_user_directory() + # No assertions about displaynames or avatars here. + self.assertEqual(users_in_directory, {u1, u2, remote1, remote2}) + + ALICE = "@alice:a" BOB = "@bob:b" BOBBY = "@bobby:a" From fe335e7af40ea07e9a28704a4c5d2675be910fa1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Sep 2021 21:26:49 +0100 Subject: [PATCH 02/15] Really register user in TestUserDirSearchDisabled I don't fully understand why this was needed, but without this I was missing entries from the users table that upcoming changes are about to expect. (Was getting failures to select from the users table because there were missing rows.) --- tests/handlers/test_user_directory.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 023ba28c6c36..d165715b2a4e 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -594,8 +594,6 @@ def test_making_room_public_doesnt_alter_directory_entry(self): class TestUserDirSearchDisabled(unittest.HomeserverTestCase): - user_id = "@test:test" - servlets = [ user_directory.register_servlets, room.register_servlets, @@ -614,17 +612,22 @@ def make_homeserver(self, reactor, clock): def test_disabling_room_list(self): self.config.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) @@ -632,7 +635,10 @@ def test_disabling_room_list(self): # Disable user directory and check search returns nothing self.config.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 e370bcea818da5b262e99c4cfd2df56011a79128 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Sep 2021 21:30:09 +0100 Subject: [PATCH 03/15] Documentation note --- docs/user_directory.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/user_directory.md b/docs/user_directory.md index 5ff14e334c4b..739d8d9d3a9a 100644 --- a/docs/user_directory.md +++ b/docs/user_directory.md @@ -14,7 +14,15 @@ flush the current tables and regenerate the directory. Data model ---------- -There are five relevant tables: +There are five relevant tables that collectively form the "user directory". +Three of them track a master list of all the users we could search for. +The last two (collectively called the "search tables") track who can +see who. + +From all of these tables we exclude three types of local user: + - "support users" + - appservice users + - deactivated users * `user_directory`. This contains the user_id, display name and avatar we'll return when you search the directory. From 559d5c7c1c75f792a9139aace57e4a52cb4e8546 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sat, 4 Sep 2021 01:48:01 +0100 Subject: [PATCH 04/15] Correct the appservice user test can't see how to correctly configure the homeserver, so let's hack it --- tests/storage/test_user_directory.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 98066b23aa1a..7fc61ac5e7c1 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -12,12 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. from typing import List, Set, Tuple +from unittest.mock import patch, Mock import synapse from synapse.api.constants import UserTypes from synapse.appservice import ApplicationService from synapse.rest.client import login, room from synapse.storage import DataStore +from synapse.storage.databases.main.appservice import _make_exclusive_regex from tests.test_utils.event_injection import inject_member_event from tests.unittest import HomeserverTestCase, override_config @@ -216,11 +218,16 @@ def test_population_excludes_appservice_user(self): namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]}, sender="@as:test", ) + self.store.services_cache.append(appservice) + as_user = "@as_user_potato:test" - self.hs.get_datastore().services_cache.append(appservice) self.get_success(self.store.register_user(user_id=as_user, password_hash=None)) - self._purge_and_rebuild_user_dir() + # 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. From c3bc1d40705793a11d27c86e47780ec54753e5fe Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sat, 4 Sep 2021 01:53:37 +0100 Subject: [PATCH 05/15] Consistently exclude unwanted users from directory --- synapse/handlers/user_directory.py | 42 +++++-------- .../storage/databases/main/user_directory.py | 61 +++++++++++-------- 2 files changed, 50 insertions(+), 53 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 0817a9ad5ad2..15b5e3645624 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -114,13 +114,8 @@ 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): + excluded = await self.store.exclude_from_user_dir(user_id) + if not excluded: await self.store.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url ) @@ -201,7 +196,7 @@ async def _handle_delta(self, delta: Dict[str, Any]) -> None: room_id, prev_event_id, event_id, typ ) elif typ == EventTypes.Member: - if await self._user_omitted_from_directory(state_key): + if await self.store.exclude_from_user_dir(state_key): return joined = await self._get_key_change( @@ -258,16 +253,6 @@ async def _handle_delta(self, delta: Dict[str, Any]) -> None: else: logger.debug("Ignoring irrelevant type: %r", typ) - async def _user_omitted_from_directory(self, user_id: str) -> bool: - """We want to ignore events from "hidden" users who shouldn't be exposed - to real users.""" - if await self.store.is_support_user(user_id): - return True - if self.store.get_if_app_services_interested_in_user(user_id): - return True - - return False - async def _handle_room_publicity_change( self, room_id: str, @@ -340,18 +325,24 @@ async def _handle_room_publicity_change( async def _track_user_joined_room(self, room_id: str, user_id: str) -> None: """Someone's just joined a room. Add to `users_in_public_rooms` and - `users_who_share_private_rooms` if necessary.""" + `users_who_share_private_rooms` if necessary. + + The caller is responsible for ensuring that the given user may be + included in the user directory. + (See UserDirectoryStore.exclude_from_user_dir) + """ is_public = await self.store.is_room_world_readable_or_publicly_joinable( room_id ) if is_public: await self.store.add_users_in_public_rooms(room_id, (user_id,)) else: - other_users_in_room = await self.store.get_users_in_room(room_id) + users_in_room = await self.store.get_users_in_room(room_id) other_users_in_room = [ - other_user - for other_user in other_users_in_room - if not await self._user_omitted_from_directory(other_user) + user + for user in users_in_room + if user != user_id + and not await self.store.exclude_from_user_dir(user) ] to_insert = set() @@ -359,15 +350,10 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None: # First, if they're our user then we need to update for every user if self.is_mine_id(user_id): for other_user_id in other_users_in_room: - if user_id == other_user_id: - continue - to_insert.add((user_id, other_user_id)) # Next we need to update for every local user in the room for other_user_id in other_users_in_room: - if user_id == other_user_id: - continue if self.is_mine_id(other_user_id): to_insert.add((other_user_id, user_id)) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 21b516423120..fe4dd2359911 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -249,13 +249,16 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No if not is_in_room: return - is_public = await self.is_room_world_readable_or_publicly_joinable(room_id) - # TODO: this will leak per-room profiles to the user directory. + # TODO: get_users_in_room_with_profiles returns per-room profiles. Leak! users_with_profile = await self.get_users_in_room_with_profiles(room_id) - - # Update each remote user in the user directory. - # (Entries for local users are managed by the UserDirectoryHandler - # and do not require us to peek at room state/events.) + users_with_profile = { + user_id: profile + for user_id, profile in users_with_profile.items() + if not await self.exclude_from_user_dir(user_id) + } + + # Upsert a user_directory record for each remote user we see. + # (Local users are processed in `_populate_user_directory_users`.) for user_id, profile in users_with_profile.items(): if self.hs.is_mine_id(user_id): continue @@ -263,26 +266,16 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No user_id, profile.display_name, profile.avatar_url ) - to_insert = set() - + is_public = await self.is_room_world_readable_or_publicly_joinable(room_id) if is_public: - for user_id in users_with_profile: - if self.get_if_app_services_interested_in_user(user_id): - continue - - to_insert.add(user_id) - - if to_insert: - await self.add_users_in_public_rooms(room_id, to_insert) - to_insert.clear() + if users_with_profile: + await self.add_users_in_public_rooms(room_id, users_with_profile.keys()) else: + to_insert = set() for user_id in users_with_profile: if not self.hs.is_mine_id(user_id): continue - if self.get_if_app_services_interested_in_user(user_id): - continue - for other_user_id in users_with_profile: if user_id == other_user_id: continue @@ -298,7 +291,6 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No if to_insert: await self.add_users_who_share_private_room(room_id, to_insert) - to_insert.clear() async def _populate_user_directory_process_users( self, progress: ProgressDict, batch_size: int @@ -343,10 +335,11 @@ def _get_next_batch(txn): ) 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.exclude_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( @@ -529,6 +522,24 @@ async def update_user_directory_stream_pos(self, stream_id: int) -> None: desc="update_user_directory_stream_pos", ) + async def exclude_from_user_dir(self, user_id: 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_id): + # Support users are for diagnostics and should not appear in the user directory. + if await self.is_support_user(user_id): + return True + + # Deactivated users aren't contactable, so should not appear in the user directory. + if await self.get_user_deactivated_status(user_id): + return True + + # App service users aren't usually contactable, so exclude them. + if self.get_if_app_services_interested_in_user(user_id): + return True + + return False class UserDirectoryStore(UserDirectoryBackgroundUpdateStore): From ede54d6d27a076268028e5106b35e86c27ba07f1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sat, 4 Sep 2021 02:04:25 +0100 Subject: [PATCH 06/15] typing in main/user_directory.py --- synapse/handlers/user_directory.py | 3 +-- synapse/storage/databases/main/user_directory.py | 12 ++++++++---- tests/storage/test_user_directory.py | 3 +-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 15b5e3645624..c9b7e1cd705d 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -341,8 +341,7 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None: other_users_in_room = [ user for user in users_in_room - if user != user_id - and not await self.store.exclude_from_user_dir(user) + if user != user_id and not await self.store.exclude_from_user_dir(user) ] to_insert = set() diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index fe4dd2359911..77eaf6e4db16 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -20,6 +20,8 @@ from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules from synapse.storage.database import DatabasePool, LoggingTransaction +from synapse.storage.databases.main.appservice import ApplicationServiceWorkerStore +from synapse.storage.databases.main.registration import RegistrationWorkerStore from synapse.storage.databases.main.state import StateFilter from synapse.storage.databases.main.state_deltas import StateDeltasStore from synapse.storage.engines import PostgresEngine, Sqlite3Engine @@ -28,7 +30,6 @@ logger = logging.getLogger(__name__) - TEMP_TABLE = "_temp_populate_user_directory" @@ -37,7 +38,6 @@ class ProgressDict(TypedDict): class UserDirectoryBackgroundUpdateStore(StateDeltasStore): - # How many records do we calculate before sending it to # add_users_who_share_private_rooms? SHARE_PRIVATE_WORKING_SET = 500 @@ -514,7 +514,7 @@ async def get_user_in_directory(self, user_id: str) -> Optional[Dict[str, Any]]: 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={}, @@ -541,8 +541,12 @@ async def exclude_from_user_dir(self, user_id: str) -> bool: return False -class UserDirectoryStore(UserDirectoryBackgroundUpdateStore): +class UserDirectoryStore( + UserDirectoryBackgroundUpdateStore, + ApplicationServiceWorkerStore, + RegistrationWorkerStore, +): # How many records do we calculate before sending it to # add_users_who_share_private_rooms? SHARE_PRIVATE_WORKING_SET = 500 diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 7fc61ac5e7c1..dc10b058c110 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -12,14 +12,13 @@ # See the License for the specific language governing permissions and # limitations under the License. from typing import List, Set, Tuple -from unittest.mock import patch, Mock +from unittest.mock import Mock, patch import synapse from synapse.api.constants import UserTypes from synapse.appservice import ApplicationService from synapse.rest.client import login, room from synapse.storage import DataStore -from synapse.storage.databases.main.appservice import _make_exclusive_regex from tests.test_utils.event_injection import inject_member_event from tests.unittest import HomeserverTestCase, override_config From b74c8226cab93e654a58cfbeba1741d20daa4f94 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sat, 4 Sep 2021 02:29:03 +0100 Subject: [PATCH 07/15] Changelog --- changelog.d/10761.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10761.bugfix diff --git a/changelog.d/10761.bugfix b/changelog.d/10761.bugfix new file mode 100644 index 000000000000..54a4fc1c8e0d --- /dev/null +++ b/changelog.d/10761.bugfix @@ -0,0 +1 @@ +Fix leaking per-room nicknames and avatars to the user directory for local users when the user directory is rebuilt. From 269fa144e8fdffb97442b7cc4e298c1fe0d1103e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Sep 2021 12:40:29 +0100 Subject: [PATCH 08/15] Test leakage of per-room names for local users --- .../storage/databases/main/user_directory.py | 1 + tests/storage/test_user_directory.py | 53 +++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 77eaf6e4db16..d39f30d8c922 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -266,6 +266,7 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No user_id, profile.display_name, profile.avatar_url ) + # Now update the room sharing tables to include this room. is_public = await self.is_room_world_readable_or_publicly_joinable(room_id) if is_public: if users_with_profile: diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index dc10b058c110..2b20f33df823 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,7 +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, Set, Tuple +from typing import Dict, List, Tuple from unittest.mock import Mock, patch import synapse @@ -19,6 +19,7 @@ from synapse.appservice import ApplicationService 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 @@ -51,14 +52,14 @@ def get_users_who_share_private_rooms(self) -> List[Tuple[str, str, str]]: ) ) - def get_users_in_user_directory(self) -> Set[str]: + 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"] for entry in r} + return {entry["user_id"]: entry["display_name"] for entry in r} def _compress_shared(self, shared): """ @@ -186,12 +187,12 @@ def test_populates_local_users(self): ) # All three should have entries in the directory - self.assertEqual(self.get_users_in_user_directory(), {u1, u2, u3}) + self.assertEqual(set(self.get_users_in_user_directory().keys()), {u1, u2, u3}) def test_populates_users_in_zero_rooms(self): billy_no_mates = self.register_user("user2", "pass") self._purge_and_rebuild_user_dir() - self.assertEqual(self.get_users_in_user_directory(), {billy_no_mates}) + self.assertEqual(self.get_users_in_user_directory().keys(), {billy_no_mates}) self.assertEqual(self.get_users_in_public_rooms(), []) self.assertEqual(self.get_users_who_share_private_rooms(), []) @@ -206,7 +207,7 @@ def test_population_excludes_support_user(self): 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(), set()) + self.assertEqual(self.get_users_in_user_directory(), {}) def test_population_excludes_appservice_user(self): as_token = "i_am_an_app_service" @@ -230,7 +231,7 @@ def test_population_excludes_appservice_user(self): # 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(), set()) + self.assertEqual(self.get_users_in_user_directory(), {}) def test_population_excludes_deactivated_user(self): user = self.register_user("rip", "pass") @@ -243,7 +244,7 @@ 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(), set()) + self.assertEqual(self.get_users_in_user_directory(), {}) def test_populates_remote_and_local_users(self): """All local users and remote users have entries in the user_directory table. @@ -284,10 +285,44 @@ def test_populates_remote_and_local_users(self): self._purge_and_rebuild_user_dir() - users_in_directory = self.get_users_in_user_directory() + users_in_directory = set(self.get_users_in_user_directory().keys()) # No assertions about displaynames or avatars here. self.assertEqual(users_in_directory, {u1, u2, remote1, remote2}) + def test_population_of_local_users_ignores_per_room_nicknames(self): + user = self.register_user("user", "pass") + token = self.login(user, "pass") + + # Explictly set a profile name for our userAlice + self.get_success( + self.hs.get_profile_handler().set_displayname( + UserID.from_string(user), + create_requester(user, token), + "Alice Cooper", + ) + ) + + # Alice makes a private room and sets a nickname there + private_room = self.helper.create_room_as(user, is_public=False, tok=token) + self.helper.send_state( + private_room, + "m.room.member", + { + "displayname": "Freddie Mercury", + "membership": "join", + }, + token, + state_key=user, + ) + + # Rebuild the directory + self._purge_and_rebuild_user_dir() + + # Check we only see Alice's profile name in the directory + self.assertEqual( + self.get_users_in_user_directory(), {"@user:test": "Alice Cooper"} + ) + ALICE = "@alice:a" BOB = "@bob:b" From d99aee0517a80726d20661cfb34ac02c825c2a34 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Sep 2021 15:01:48 +0100 Subject: [PATCH 09/15] Remove test related to reactivation Handled in separate PR #10762 --- tests/handlers/test_user_directory.py | 30 --------------------------- 1 file changed, 30 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index d165715b2a4e..118adb771455 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -143,36 +143,6 @@ def test_handle_user_deactivated_regular_user(self): self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) self.store.remove_from_user_dir.called_once_with(r_user_id) - def test_reactivation_makes_regular_user_searchable(self): - user = self.register_user("regular", "pass") - user_token = self.login(user, "pass") - admin_user = self.register_user("admin", "pass", admin=True) - - # Ensure the regular user is publicly visible and searchable. - public_room = self.helper.create_room_as(user, is_public=True, tok=user_token) - s = self.get_success(self.handler.search_users(admin_user, user, 10)) - self.assertEqual(len(s["results"]), 1) - self.assertEqual(s["results"][0]["user_id"], user) - - # Deactivate the user and check they're not searchable. - deactivate_handler = self.hs._deactivate_account_handler - self.get_success( - deactivate_handler.deactivate_account( - user, erase_data=False, requester=create_requester(admin_user) - ) - ) - s = self.get_success(self.handler.search_users(admin_user, user, 10)) - self.assertEqual(s["results"], []) - - # Reactivate the user and make them publicly visible again. - self.get_success(deactivate_handler.activate_account(user)) - self.inject_room_member(public_room, user, Membership.JOIN) - - # Check they're searchable. - s = self.get_success(self.handler.search_users(admin_user, user, 10)) - self.assertEqual(len(s["results"]), 1) - self.assertEqual(s["results"][0]["user_id"], user) - def test_private_room(self): """ A user can be searched for only by people that are either in a public From c5f34ac823b84ec05ca1aee3249ee0c731ce6d83 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Sep 2021 16:39:56 +0100 Subject: [PATCH 10/15] "support users" -> support users Co-authored-by: reivilibre <38398653+reivilibre@users.noreply.github.com> --- docs/user_directory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user_directory.md b/docs/user_directory.md index 739d8d9d3a9a..07fe95489133 100644 --- a/docs/user_directory.md +++ b/docs/user_directory.md @@ -20,7 +20,7 @@ The last two (collectively called the "search tables") track who can see who. From all of these tables we exclude three types of local user: - - "support users" + - support users - appservice users - deactivated users From 61d584e7c77887c77c5ed64b8630dc3b42b64e9e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Sep 2021 16:42:07 +0100 Subject: [PATCH 11/15] exclude_from_user_dir -> is_excluded_from_user_dir --- synapse/handlers/user_directory.py | 6 +++--- synapse/storage/databases/main/user_directory.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index c9b7e1cd705d..fcb6d6637096 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -114,7 +114,7 @@ async def handle_local_profile_change( """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - excluded = await self.store.exclude_from_user_dir(user_id) + excluded = await self.store.is_excluded_from_user_dir(user_id) if not excluded: await self.store.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url @@ -196,7 +196,7 @@ async def _handle_delta(self, delta: Dict[str, Any]) -> None: room_id, prev_event_id, event_id, typ ) elif typ == EventTypes.Member: - if await self.store.exclude_from_user_dir(state_key): + if await self.store.is_excluded_from_user_dir(state_key): return joined = await self._get_key_change( @@ -341,7 +341,7 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None: other_users_in_room = [ user for user in users_in_room - if user != user_id and not await self.store.exclude_from_user_dir(user) + if user != user_id and not await self.store.is_excluded_from_user_dir(user) ] to_insert = set() diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index d39f30d8c922..45cdce6a81cb 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -254,7 +254,7 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No users_with_profile = { user_id: profile for user_id, profile in users_with_profile.items() - if not await self.exclude_from_user_dir(user_id) + if not await self.is_excluded_from_user_dir(user_id) } # Upsert a user_directory record for each remote user we see. @@ -336,7 +336,7 @@ def _get_next_batch(txn): ) for user_id in users_to_work_on: - if not await self.exclude_from_user_dir(user_id): + 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 @@ -523,7 +523,7 @@ async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> No desc="update_user_directory_stream_pos", ) - async def exclude_from_user_dir(self, user_id: str) -> bool: + async def is_excluded_from_user_dir(self, user_id: str) -> bool: """Certain classes of local user are omitted from the user directory. Is this user one of them? """ From eac5c23bf4f3a4a4202fff7222a173c45efbae1d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Sep 2021 16:59:00 +0100 Subject: [PATCH 12/15] Clarify comments --- synapse/storage/databases/main/user_directory.py | 2 ++ tests/storage/test_user_directory.py | 1 + 2 files changed, 3 insertions(+) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 45cdce6a81cb..ff86f2b296a7 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -337,6 +337,8 @@ def _get_next_batch(txn): for user_id in users_to_work_on: if not await self.is_excluded_from_user_dir(user_id): + # Populate this local user's user directory entry with their + # profile information 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 diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 2b20f33df823..a2da30572659 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -287,6 +287,7 @@ def test_populates_remote_and_local_users(self): users_in_directory = set(self.get_users_in_user_directory().keys()) # No assertions about displaynames or avatars here. + # TODO extend this case to do so, or add a new test case to cover it self.assertEqual(users_in_directory, {u1, u2, remote1, remote2}) def test_population_of_local_users_ignores_per_room_nicknames(self): From 6e61645455a7b311e44d0f3fdcabc3f2e8791e10 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Sep 2021 17:00:07 +0100 Subject: [PATCH 13/15] Remove redundant copy of SHARE_PRIVATE_WORKING_SET --- synapse/storage/databases/main/user_directory.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index ff86f2b296a7..a1d045093659 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -550,10 +550,6 @@ class UserDirectoryStore( ApplicationServiceWorkerStore, RegistrationWorkerStore, ): - # How many records do we calculate before sending it to - # add_users_who_share_private_rooms? - SHARE_PRIVATE_WORKING_SET = 500 - def __init__(self, database: DatabasePool, db_conn, hs): super().__init__(database, db_conn, hs) From 039fb4ccf086616ae966c55be2ff90c83f19c39f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Sep 2021 17:08:34 +0100 Subject: [PATCH 14/15] lint --- synapse/handlers/user_directory.py | 3 ++- tests/handlers/test_user_directory.py | 8 +------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index fcb6d6637096..38769c3d1615 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -341,7 +341,8 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None: other_users_in_room = [ user for user in users_in_room - if user != user_id and not await self.store.is_excluded_from_user_dir(user) + if user != user_id + and not await self.store.is_excluded_from_user_dir(user) ] to_insert = set() diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 118adb771455..71e0b0a7c4b2 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -16,16 +16,10 @@ from twisted.internet import defer import synapse.rest.admin -from synapse.api.constants import ( - EventTypes, - Membership, - RoomEncryptionAlgorithms, - UserTypes, -) +from synapse.api.constants import EventTypes, RoomEncryptionAlgorithms, UserTypes from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.rest.client import login, room, user_directory from synapse.storage.roommember import ProfileInfo -from synapse.types import create_requester from tests import unittest from tests.storage.test_user_directory import GetUserDirectoryTables From cfe745815ed876b6c5e1baff07e01476f58b80bc Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 8 Sep 2021 15:52:13 +0100 Subject: [PATCH 15/15] Typing fixes --- .../storage/databases/main/user_directory.py | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index a1d045093659..3f2781e33d3f 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -14,9 +14,7 @@ import logging import re -from typing import Any, Dict, Iterable, Optional, Sequence, Set, Tuple - -from typing_extensions import TypedDict +from typing import Any, Dict, Iterable, Optional, Sequence, Set, Tuple, cast from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules from synapse.storage.database import DatabasePool, LoggingTransaction @@ -25,7 +23,7 @@ from synapse.storage.databases.main.state import StateFilter from synapse.storage.databases.main.state_deltas import StateDeltasStore from synapse.storage.engines import PostgresEngine, Sqlite3Engine -from synapse.types import get_domain_from_id, get_localpart_from_id +from synapse.types import JsonDict, get_domain_from_id, get_localpart_from_id from synapse.util.caches.descriptors import cached logger = logging.getLogger(__name__) @@ -33,10 +31,6 @@ TEMP_TABLE = "_temp_populate_user_directory" -class ProgressDict(TypedDict): - remaining: int - - class UserDirectoryBackgroundUpdateStore(StateDeltasStore): # How many records do we calculate before sending it to # add_users_who_share_private_rooms? @@ -64,7 +58,7 @@ def __init__(self, database: DatabasePool, db_conn, hs): ) async def _populate_user_directory_createtables( - self, progress: Dict, batch_size: str + self, progress: JsonDict, batch_size: str ) -> int: # Get all the rooms that we want to process. @@ -119,7 +113,7 @@ def _make_staging_area(txn): return 1 async def _populate_user_directory_cleanup( - self, progress: Dict, batch_size: str + self, progress: JsonDict, batch_size: str ) -> int: """ Update the user directory stream position, then clean up the old tables. @@ -144,7 +138,7 @@ def _delete_staging_area(txn): return 1 async def _populate_user_directory_process_rooms( - self, progress: ProgressDict, batch_size: int + self, progress: JsonDict, batch_size: int ) -> int: """ Rescan the state of all rooms so we can track @@ -168,7 +162,7 @@ async def _populate_user_directory_process_rooms( def _get_next_batch( txn: LoggingTransaction, - ) -> Optional[Sequence[Tuple[str, str]]]: + ) -> Optional[Sequence[Tuple[str, int]]]: # Only fetch 250 rooms, so we don't fetch too many at once, even # if those 250 rooms have less than batch_size state events. sql = """ @@ -179,7 +173,7 @@ def _get_next_batch( TEMP_TABLE + "_rooms", ) txn.execute(sql) - rooms_to_work_on = txn.fetchall() + rooms_to_work_on = cast(Sequence[Tuple[str, int]], txn.fetchall()) if not rooms_to_work_on: return None @@ -187,7 +181,8 @@ def _get_next_batch( # Get how many are left to process, so we can give status on how # far we are in processing txn.execute("SELECT COUNT(*) FROM " + TEMP_TABLE + "_rooms") - progress["remaining"] = txn.fetchone()[0] + result = cast(Tuple[int], txn.fetchone()) + progress["remaining"] = result[0] return rooms_to_work_on @@ -294,7 +289,7 @@ async def _populate_user_directory_process_single_room(self, room_id: str) -> No await self.add_users_who_share_private_room(room_id, to_insert) async def _populate_user_directory_process_users( - self, progress: ProgressDict, batch_size: int + self, progress: JsonDict, batch_size: int ) -> int: """Upsert a user_directory entry for each local user.""" @@ -758,7 +753,7 @@ async def search_user_dir(self, user_id, search_term, limit): # We allow manipulating the ranking algorithm by injecting statements # based on config options. additional_ordering_statements = [] - ordering_arguments = () + ordering_arguments: Tuple[str, ...] = () if isinstance(self.database_engine, PostgresEngine): full_query, exact_query, prefix_query = _parse_query_postgres(search_term)