Consistently exclude from user_directory, round 2

changelog.d/10960.bugfix

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

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 await self.store.should_include_local_user_in_dir(user_id):
             await self.store.update_profile_in_user_dir(
                 user_id, profile.display_name, profile.avatar_url
             )
@@ -229,8 +224,10 @@ 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:
+                include_in_dir = not self.is_mine_id(
+                    state_key
+                ) or await self.store.should_include_local_user_in_dir(state_key)
+                if include_in_dir:
                     if change is MatchChange.no_change:
                         # Handle any profile changes
                         await self._handle_profile_change(
@@ -356,13 +353,7 @@ 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:
+                if await self.store.should_include_local_user_in_dir(user_id):
                     for other_user_id in other_users_in_room:
                         if user_id == other_user_id:
                             continue
@@ -374,10 +365,10 @@ async def _handle_new_user(
                 if user_id == other_user_id:
                     continue
 
-                is_appservice = self.store.get_if_app_services_interested_in_user(
+                include_other_user = self.is_mine_id(
                     other_user_id
-                )
-                if self.is_mine_id(other_user_id) and not is_appservice:
+                ) and await self.store.should_include_local_user_in_dir(other_user_id)
+                if include_other_user:
                     to_insert.add((other_user_id, user_id))
 
             if to_insert:

synapse/storage/databases/main/user_directory.py

@@ -40,12 +40,10 @@
 
 logger = logging.getLogger(__name__)
 
-
 TEMP_TABLE = "_temp_populate_user_directory"
 
 
 class UserDirectoryBackgroundUpdateStore(StateDeltasStore):
-
     # How many records do we calculate before sending it to
     # add_users_who_share_private_rooms?
     SHARE_PRIVATE_WORKING_SET = 500
@@ -235,6 +233,13 @@ def _get_next_batch(
                 )
 
                 users_with_profile = await self.get_users_in_room_with_profiles(room_id)
+                # Throw away users excluded from the directory.
+                users_with_profile = {
+                    user_id: profile
+                    for user_id, profile in users_with_profile.items()
+                    if not self.hs.is_mine_id(user_id)
+                    or await self.should_include_local_user_in_dir(user_id)
+                }
 
                 # Update each user in the user directory.
                 for user_id, profile in users_with_profile.items():
@@ -246,22 +251,19 @@ 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):
-                            continue
-
                         to_insert.add(user_id)
 
                     if to_insert:
                         await self.add_users_in_public_rooms(room_id, to_insert)
                         to_insert.clear()
                 else:
                     for user_id in users_with_profile:
+                        # We want the set of pairs (L, M) where L and M are
+                        # in `users_with_profile` and L is local.
+                        # Do so by looking for the local user L first.
                         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
@@ -349,10 +351,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 await self.should_include_local_user_in_dir(user_id):
+                profile = await self.get_profileinfo(get_localpart_from_id(user_id))
+                await self.update_profile_in_user_dir(
+                    user_id, profile.display_name, profile.avatar_url
+                )
 
             # We've finished processing a user. Delete it from the table.
             await self.db_pool.simple_delete_one(
@@ -369,6 +372,24 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]:
 
         return len(users_to_work_on)
 
+    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.
+        if self.get_if_app_services_interested_in_user(user):
+            # TODO we might want to make this configurable for each app service
+            return False
+
+        # Support users are for diagnostics and should not appear in the user directory.
+        if await self.is_support_user(user):
+            return False
+
+        # Deactivated users aren't contactable, so should not appear in the user directory.
+        if await self.get_user_deactivated_status(user):
+            return False
+        return True
+
     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"""
 
@@ -537,7 +558,6 @@ async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> No
 
 
 class UserDirectoryStore(UserDirectoryBackgroundUpdateStore):
-
     # How many records do we calculate before sending it to
     # add_users_who_share_private_rooms?
     SHARE_PRIVATE_WORKING_SET = 500