Fix exception thrown when attempting to add an appservice sender to user_directory

.github/workflows/tests.yml

@@ -122,6 +122,7 @@ jobs:
         # Note: Dumps to workflow logs instead of using actions/upload-artifact
         #       This keeps logs colocated with failing jobs
         #       It also ignores find's exit code; this is a best effort affair
+        if: ${{ always() }}
         run: >-
           find _trial_temp -name '*.log'
           -exec echo "::group::{}" \;
@@ -146,6 +147,7 @@ jobs:
         # Note: Dumps to workflow logs instead of using actions/upload-artifact
         #       This keeps logs colocated with failing jobs
         #       It also ignores find's exit code; this is a best effort affair
+        if: ${{ always() }}
         run: >-
           find _trial_temp -name '*.log'
           -exec echo "::group::{}" \;
@@ -176,6 +178,7 @@ jobs:
         # Note: Dumps to workflow logs instead of using actions/upload-artifact
         #       This keeps logs colocated with failing jobs
         #       It also ignores find's exit code; this is a best effort affair
+        if: ${{ always() }}
         run: >-
           find _trial_temp -name '*.log'
           -exec echo "::group::{}" \;

.github/workflows/twisted_trunk.yml

@@ -36,6 +36,7 @@ jobs:
         # Note: Dumps to workflow logs instead of using actions/upload-artifact
         #       This keeps logs colocated with failing jobs
         #       It also ignores find's exit code; this is a best effort affair
+        if: ${{ always() }}
         run: >-
           find _trial_temp -name '*.log'
           -exec echo "::group::{}" \;

changelog.d/11026.bugfix

@@ -0,0 +1 @@
+Fix a long-standing bug where rebuilding the user directory wouldn't exclude support and disabled users.

docs/user_directory.md

@@ -5,11 +5,6 @@ The user directory is currently maintained based on the 'visible' users
 on this particular server - i.e. ones which your account shares a room with, or
 who are present in a publicly viewable room present on the server.
 
-The directory info is stored in various tables, which can (typically after
-DB corruption) get stale or out of sync.  If this happens, for now the
-solution to fix it is to execute the SQL [here](https://github.com/matrix-org/synapse/blob/master/synapse/storage/schema/main/delta/53/user_dir_populate.sql)
-and then restart synapse. This should then start a background task to
-flush the current tables and regenerate the directory.
 
 Data model
 ----------
@@ -21,7 +16,8 @@ see who.
 
 From all of these tables we exclude three types of local user:
   - support users
-  - appservice users
+  - appservice users (e.g. people using IRC)
+    - but not the "appservice sender" (e.g. the bot which bridges Matrix to IRC).
   - deactivated users
 
 * `user_directory`. This contains the user_id, display name and avatar we'll
@@ -47,3 +43,36 @@ From all of these tables we exclude three types of local user:
 * `users_who_share_private_rooms`. Rows are triples `(L, M, room id)` where `L`
    is a local user and `M` is a local or remote user. `L` and `M` should be
    different, but this isn't enforced by a constraint.
+
+
+Rebuilding the directory
+------------------------
+
+The directory info is stored in various tables, which can (typically after
+DB corruption) get stale or out of sync.  If this happens, for now the
+solution to fix it is to execute the following SQL and then restart Synapse.
+
+```sql
+-- Set up staging tables
+INSERT INTO background_updates (update_name, progress_json) VALUES
+    ('populate_user_directory_createtables', '{}');
+
+-- Run through each room and update the room sharing tables.
+-- Also add directory entries for remote users.
+INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
+    ('populate_user_directory_process_rooms', '{}', 'populate_user_directory_createtables');
+
+-- Insert directory entries for all local users.
+INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
+    ('populate_user_directory_process_users', '{}', 'populate_user_directory_process_rooms');
+
+-- Insert directory entries for all appservice senders.
+INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
+    ('populate_user_directory_process_appservice_senders', '{}', 'populate_user_directory_process_users');
+
+-- Clean up staging tables
+INSERT INTO background_updates (update_name, progress_json, depends_on) VALUES
+    ('populate_user_directory_cleanup', '{}', 'populate_user_directory_process_appservice_senders');
+```
+This should then start a background task to
+flush the current tables and regenerate the directory.

synapse/storage/databases/main/user_directory.py

@@ -28,6 +28,7 @@
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
+    from synapse.appservice import ApplicationService
 
 from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules
 from synapse.storage.database import DatabasePool, LoggingTransaction
@@ -70,6 +71,10 @@ def __init__(
             "populate_user_directory_process_users",
             self._populate_user_directory_process_users,
         )
+        self.db_pool.updates.register_background_update_handler(
+            "populate_user_directory_process_appservice_senders",
+            self._populate_user_directory_process_appservice_senders,
+        )
         self.db_pool.updates.register_background_update_handler(
             "populate_user_directory_cleanup", self._populate_user_directory_cleanup
         )
@@ -379,11 +384,40 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]:
 
         return len(users_to_work_on)
 
+    async def _populate_user_directory_process_appservice_senders(
+        self, progress: JsonDict, batch_size: int
+    ) -> int:
+        """Add appservice senders to the `user_directory` table.
+
+        Appservices users don't live in the users table, so they won't be picked up
+        by `_populate_user_directory_process_users`. Process them explicitly here.
+        """
+        service: "ApplicationService"
+        for service in self.services_cache:
+            await self.update_profile_in_user_dir(service.sender, None, None)
+
+        await self.db_pool.updates._end_background_update(
+            "populate_user_directory_process_appservice_senders"
+        )
+        return 1
+
     async def should_include_local_user_in_dir(self, user: str) -> bool:
         """Certain classes of local user are omitted from the user directory.
         Is this user one of them?
         """
-        # App service users aren't usually contactable, so exclude them.
+        # We include the appservice sender in the directory. These typically
+        # aren't covered by `get_if_app_services_interested_in_user`.
+        # We need to handle it here or else we'll get a "row not found" error
+        # in the deactivated user check, because the sender doesn't have an
+        # entry in the `users` table.
+        if self.get_app_service_by_user_id(user) is not None:
+            return True
+
+        # 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

synapse/storage/schema/main/delta/53/user_dir_populate.sql

@@ -13,6 +13,8 @@
  * limitations under the License.
  */
 
+-- For the most up-to-date version of these instructions, see docs/user_directory.md
+
 -- Set up staging tables
 INSERT INTO background_updates (update_name, progress_json) VALUES
     ('populate_user_directory_createtables', '{}');

tests/handlers/test_user_directory.py

@@ -63,7 +63,9 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
             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])
@@ -179,6 +181,26 @@ def test_excludes_appservices_user(self) -> None:
         )
         self._check_only_one_user_in_directory(user, public)
 
+    def test_includes_appservice_sender(self) -> None:
+        user = self.register_user("user", "pass")
+        token = self.login(user, "pass")
+        room = self.helper.create_room_as(
+            self.appservice.sender, is_public=True, tok=self.appservice.token
+        )
+        self.helper.join(room, user, tok=token)
+
+        # Will this make the test pass in CI? It's fine locally.
+        self.wait_for_background_updates()
+        users = self.get_success(self.user_dir_helper.get_users_in_user_directory())
+        in_public = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
+        in_private = self.get_success(
+            self.user_dir_helper.get_users_who_share_private_rooms()
+        )
+
+        self.assertEqual(users, {user, self.appservice.sender})
+        self.assertEqual(set(in_public), {(user, room), (self.appservice.sender, room)})
+        self.assertEqual(in_private, [])
+
     def _create_rooms_and_inject_memberships(
         self, creator: str, token: str, joiner: str
     ) -> Tuple[str, str]:
@@ -230,7 +252,7 @@ def test_handle_local_profile_change_with_support_user(self) -> None:
             )
         )
         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 +286,7 @@ def test_handle_local_profile_change_with_deactivated_user(self) -> None:
 
         # 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 +295,7 @@ def test_handle_local_profile_change_with_deactivated_user(self) -> None:
 
         # 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 +305,7 @@ def test_handle_local_profile_change_with_appservice_user(self) -> None:
 
         # 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 +315,30 @@ def test_handle_local_profile_change_with_appservice_user(self) -> None:
 
         # 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="beep boop")
+        self.get_success(
+            self.handler.handle_local_profile_change(
+                self.appservice.sender, profile_info
+            )
+        )
+
+        # profile is now set
+        profile = self.get_success(
+            self.store.get_user_in_directory(self.appservice.sender)
+        )
+        self.assertEqual(
+            profile, {"display_name": "beep boop", "avatar_url": "avatar_url"}
+        )
 
     def test_handle_user_deactivated_support_user(self) -> None:
         s_user_id = "@support:test"

tests/storage/test_user_directory.py

@@ -134,7 +134,9 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
             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])
@@ -205,12 +207,22 @@ def _purge_and_rebuild_user_dir(self) -> None:
             self.store.db_pool.simple_insert(
                 "background_updates",
                 {
-                    "update_name": "populate_user_directory_cleanup",
+                    "update_name": "populate_user_directory_process_appservice_senders",
                     "progress_json": "{}",
                     "depends_on": "populate_user_directory_process_users",
                 },
             )
         )
+        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_appservice_senders",
+                },
+            )
+        )
 
         self.wait_for_background_updates()
 
@@ -254,7 +266,7 @@ def test_initial(self) -> None:
 
         # All three should have entries in the directory
         users = self.get_success(self.user_dir_helper.get_users_in_user_directory())
-        self.assertEqual(users, {u1, u2, u3})
+        self.assertEqual(users, {u1, u2, u3, self.appservice.sender})
 
     # The next three tests (test_population_excludes_*) all set up
     #   - A normal user included in the user dir
@@ -290,7 +302,7 @@ def _check_room_sharing_tables(
     ) -> None:
         # After rebuilding the directory, we should only see the normal user.
         users = self.get_success(self.user_dir_helper.get_users_in_user_directory())
-        self.assertEqual(users, {normal_user})
+        self.assertEqual(users, {normal_user, self.appservice.sender})
         in_public_rooms = self.get_success(
             self.user_dir_helper.get_users_in_public_rooms()
         )
@@ -364,6 +376,38 @@ def test_population_excludes_appservice_user(self) -> None:
         # Check the AS user is not in the directory.
         self._check_room_sharing_tables(user, public, private)
 
+    def test_population_includes_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 in the directory.
+        users = self.get_success(self.user_dir_helper.get_users_in_user_directory())
+        self.assertEqual(users, {user, self.appservice.sender})
+        in_public_rooms = self.get_success(
+            self.user_dir_helper.get_users_in_public_rooms()
+        )
+        self.assertEqual(
+            set(in_public_rooms), {(user, public), (self.appservice.sender, public)}
+        )
+        in_private_rooms = self.get_success(
+            self.user_dir_helper.get_users_who_share_private_rooms()
+        )
+        self.assertEqual(
+            self.user_dir_helper._compress_shared(in_private_rooms),
+            {
+                (user, self.appservice.sender, private),
+                (self.appservice.sender, user, private),
+            },
+        )
+
     def test_population_conceals_private_nickname(self) -> None:
         # Make a private room, and set a nickname within
         user = self.register_user("aaaa", "pass")
@@ -392,15 +436,16 @@ async def mocked_process_users(*args: Any, **kwargs: Any) -> int:
             self._purge_and_rebuild_user_dir()
 
         # Local users are ignored by the scan over rooms
-        users = self.get_success(self.user_dir_helper.get_profiles_in_user_directory())
-        self.assertEqual(users, {})
+        users = self.get_success(self.user_dir_helper.get_users_in_user_directory())
+        self.assertEqual(users, {self.appservice.sender})
 
         # Do a full rebuild including the scan over the `users` table. The local
         # user should appear with their profile name.
         self._purge_and_rebuild_user_dir()
         users = self.get_success(self.user_dir_helper.get_profiles_in_user_directory())
         self.assertEqual(
-            users, {user: ProfileInfo(display_name="aaaa", avatar_url=None)}
+            users[user],
+            ProfileInfo(display_name="aaaa", avatar_url=None),
         )