diff options
| -rw-r--r-- | changelog.d/11053.bugfix | 2 | ||||
| -rw-r--r-- | synapse/storage/databases/main/user_directory.py | 24 | ||||
| -rw-r--r-- | tests/handlers/test_user_directory.py | 65 | ||||
| -rw-r--r-- | tests/storage/test_user_directory.py | 17 |
4 files changed, 98 insertions, 10 deletions
diff --git a/changelog.d/11053.bugfix b/changelog.d/11053.bugfix new file mode 100644 index 0000000000..a59cfac931 --- /dev/null +++ b/changelog.d/11053.bugfix @@ -0,0 +1,2 @@ +Fix a bug introduced in Synapse 1.45.0rc1 where the user directory would stop updating if it processed an event from a +user not in the `users` table. diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 5c713a732e..e98a45b6af 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -26,6 +26,8 @@ from typing import ( cast, ) +from synapse.api.errors import StoreError + if TYPE_CHECKING: from synapse.server import HomeServer @@ -383,7 +385,19 @@ class UserDirectoryBackgroundUpdateStore(StateDeltasStore): """Certain classes of local user are omitted from the user directory. Is this user one of them? """ - # App service users aren't usually contactable, so exclude them. + # We're opting to exclude the appservice sender (user defined by the + # `sender_localpart` in the appservice registration) even though + # technically it could be DM-able. In the future, this could potentially + # be configurable per-appservice whether the appservice sender can be + # contacted. + if self.get_app_service_by_user_id(user) is not None: + return False + + # We're opting to exclude appservice users (anyone matching the user + # namespace regex in the appservice registration) even though technically + # they could be DM-able. In the future, this could potentially + # be configurable per-appservice whether the appservice users can be + # contacted. if self.get_if_app_services_interested_in_user(user): # TODO we might want to make this configurable for each app service return False @@ -393,8 +407,14 @@ class UserDirectoryBackgroundUpdateStore(StateDeltasStore): return False # Deactivated users aren't contactable, so should not appear in the user directory. - if await self.get_user_deactivated_status(user): + try: + if await self.get_user_deactivated_status(user): + return False + except StoreError: + # No such user in the users table. No need to do this when calling + # is_support_user---that returns False if the user is missing. return False + return True async def is_room_world_readable_or_publicly_joinable(self, room_id: str) -> bool: diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index db65253773..0120b4688b 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -63,7 +63,9 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): hostname="test", id="1234", namespaces={"users": [{"regex": r"@as_user.*", "exclusive": True}]}, - sender="@as:test", + # Note: this user does not match the regex above, so that tests + # can distinguish the sender from the AS user. + sender="@as_main:test", ) mock_load_appservices = Mock(return_value=[self.appservice]) @@ -122,7 +124,7 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): {(alice, bob, private), (bob, alice, private)}, ) - # The next three tests (test_population_excludes_*) all setup + # The next four tests (test_excludes_*) all setup # - A normal user included in the user dir # - A public and private room created by that user # - A user excluded from the room dir, belonging to both rooms @@ -179,6 +181,34 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): ) self._check_only_one_user_in_directory(user, public) + def test_excludes_appservice_sender(self) -> None: + user = self.register_user("user", "pass") + token = self.login(user, "pass") + room = self.helper.create_room_as(user, is_public=True, tok=token) + self.helper.join(room, self.appservice.sender, tok=self.appservice.token) + self._check_only_one_user_in_directory(user, room) + + def test_user_not_in_users_table(self) -> None: + """Unclear how it happens, but on matrix.org we've seen join events + for users who aren't in the users table. Test that we don't fall over + when processing such a user. + """ + user1 = self.register_user("user1", "pass") + token1 = self.login(user1, "pass") + room = self.helper.create_room_as(user1, is_public=True, tok=token1) + + # Inject a join event for a user who doesn't exist + self.get_success(inject_member_event(self.hs, room, "@not-a-user:test", "join")) + + # Another new user registers and joins the room + user2 = self.register_user("user2", "pass") + token2 = self.login(user2, "pass") + self.helper.join(room, user2, tok=token2) + + # The dodgy event should not have stopped us from processing user2's join. + in_public = self.get_success(self.user_dir_helper.get_users_in_public_rooms()) + self.assertEqual(set(in_public), {(user1, room), (user2, room)}) + def _create_rooms_and_inject_memberships( self, creator: str, token: str, joiner: str ) -> Tuple[str, str]: @@ -230,7 +260,7 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): ) ) profile = self.get_success(self.store.get_user_in_directory(support_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) display_name = "display_name" profile_info = ProfileInfo(avatar_url="avatar_url", display_name=display_name) @@ -264,7 +294,7 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): # profile is not in directory profile = self.get_success(self.store.get_user_in_directory(r_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) # update profile after deactivation self.get_success( @@ -273,7 +303,7 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): # profile is furthermore not in directory profile = self.get_success(self.store.get_user_in_directory(r_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) def test_handle_local_profile_change_with_appservice_user(self) -> None: # create user @@ -283,7 +313,7 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): # profile is not in directory profile = self.get_success(self.store.get_user_in_directory(as_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) # update profile profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") @@ -293,7 +323,28 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): # profile is still not in directory profile = self.get_success(self.store.get_user_in_directory(as_user_id)) - self.assertTrue(profile is None) + self.assertIsNone(profile) + + def test_handle_local_profile_change_with_appservice_sender(self) -> None: + # profile is not in directory + profile = self.get_success( + self.store.get_user_in_directory(self.appservice.sender) + ) + self.assertIsNone(profile) + + # update profile + profile_info = ProfileInfo(avatar_url="avatar_url", display_name="4L1c3") + self.get_success( + self.handler.handle_local_profile_change( + self.appservice.sender, profile_info + ) + ) + + # profile is still not in directory + profile = self.get_success( + self.store.get_user_in_directory(self.appservice.sender) + ) + self.assertIsNone(profile) def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 9f483ad681..be3ed64f5e 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -256,7 +256,7 @@ class UserDirectoryInitialPopulationTestcase(HomeserverTestCase): users = self.get_success(self.user_dir_helper.get_users_in_user_directory()) self.assertEqual(users, {u1, u2, u3}) - # The next three tests (test_population_excludes_*) all set up + # The next four tests (test_population_excludes_*) all set up # - A normal user included in the user dir # - A public and private room created by that user # - A user excluded from the room dir, belonging to both rooms @@ -364,6 +364,21 @@ class UserDirectoryInitialPopulationTestcase(HomeserverTestCase): # Check the AS user is not in the directory. self._check_room_sharing_tables(user, public, private) + def test_population_excludes_appservice_sender(self) -> None: + user = self.register_user("user", "pass") + token = self.login(user, "pass") + + # Join the AS sender to rooms owned by the normal user. + public, private = self._create_rooms_and_inject_memberships( + user, token, self.appservice.sender + ) + + # Rebuild the directory. + self._purge_and_rebuild_user_dir() + + # Check the AS sender is not in the directory. + self._check_room_sharing_tables(user, public, private) + def test_population_conceals_private_nickname(self) -> None: # Make a private room, and set a nickname within user = self.register_user("aaaa", "pass") |