Consistently exclude from user_directory, round 2

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

@@ -238,6 +238,10 @@ def _get_next_batch(
 
                 # Update each user in the user directory.
                 for user_id, profile in users_with_profile.items():
+                    if self.hs.is_mine_id(
+                        user_id
+                    ) and not await self.should_include_local_user_in_dir(user_id):
+                        continue
                     await self.update_profile_in_user_dir(
                         user_id, profile.display_name, profile.avatar_url
                     )
@@ -246,7 +250,9 @@ 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 self.hs.is_mine_id(
+                            user_id
+                        ) and not await self.should_include_local_user_in_dir(user_id):
                             continue
 
                         to_insert.add(user_id)
@@ -259,7 +265,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 not await self.should_include_local_user_in_dir(user_id):
                             continue
 
                         for other_user_id in users_with_profile:
@@ -349,10 +355,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 +376,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"""
 

tests/handlers/test_user_directory.py

@@ -483,8 +483,6 @@ def _add_user_to_room(
 
 
 class TestUserDirSearchDisabled(unittest.HomeserverTestCase):
-    user_id = "@test:test"
-
     servlets = [
         user_directory.register_servlets,
         room.register_servlets,
@@ -504,24 +502,32 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
     def test_disabling_room_list(self) -> None:
         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)
+        # Create two users and put them in the same room.
+        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")
+
+        room = self.helper.create_room_as(u1, tok=u1_token)
+        self.helper.join(room, user=u2, tok=u2_token)
 
-        # Assert user directory is not empty
+        # Each should see the other when searching the user directory.
         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)
 
         # 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)

tests/rest/client/test_login.py

@@ -1064,13 +1064,6 @@ class AppserviceLoginRestServletTestCase(unittest.HomeserverTestCase):
         register.register_servlets,
     ]
 
-    def register_as_user(self, username):
-        self.make_request(
-            b"POST",
-            "/_matrix/client/r0/register?access_token=%s" % (self.service.token,),
-            {"username": username},
-        )
-
     def make_homeserver(self, reactor, clock):
         self.hs = self.setup_test_homeserver()
 
@@ -1107,7 +1100,7 @@ def make_homeserver(self, reactor, clock):
 
     def test_login_appservice_user(self):
         """Test that an appservice user can use /login"""
-        self.register_as_user(AS_USER)
+        self.register_appservice_user(AS_USER, self.service.token)
 
         params = {
             "type": login.LoginRestServlet.APPSERVICE_TYPE,
@@ -1121,7 +1114,7 @@ def test_login_appservice_user(self):
 
     def test_login_appservice_user_bot(self):
         """Test that the appservice bot can use /login"""
-        self.register_as_user(AS_USER)
+        self.register_appservice_user(AS_USER, self.service.token)
 
         params = {
             "type": login.LoginRestServlet.APPSERVICE_TYPE,
@@ -1135,7 +1128,7 @@ def test_login_appservice_user_bot(self):
 
     def test_login_appservice_wrong_user(self):
         """Test that non-as users cannot login with the as token"""
-        self.register_as_user(AS_USER)
+        self.register_appservice_user(AS_USER, self.service.token)
 
         params = {
             "type": login.LoginRestServlet.APPSERVICE_TYPE,
@@ -1149,7 +1142,7 @@ def test_login_appservice_wrong_user(self):
 
     def test_login_appservice_wrong_as(self):
         """Test that as users cannot login with wrong as token"""
-        self.register_as_user(AS_USER)
+        self.register_appservice_user(AS_USER, self.service.token)
 
         params = {
             "type": login.LoginRestServlet.APPSERVICE_TYPE,
@@ -1165,7 +1158,7 @@ def test_login_appservice_no_token(self):
         """Test that users must provide a token when using the appservice
         login method
         """
-        self.register_as_user(AS_USER)
+        self.register_appservice_user(AS_USER, self.service.token)
 
         params = {
             "type": login.LoginRestServlet.APPSERVICE_TYPE,

tests/unittest.py

@@ -22,6 +22,7 @@
 import time
 from typing import Callable, Dict, Iterable, Optional, Tuple, Type, TypeVar, Union
 from unittest.mock import Mock, patch
+from urllib.parse import quote
 
 from canonicaljson import json
 
@@ -596,6 +597,25 @@ def register_user(
         user_id = channel.json_body["user_id"]
         return user_id
 
+    def register_appservice_user(
+        self,
+        username: str,
+        appservice_token: str,
+    ) -> str:
+        """Register an appservice user. Requires the client-facing registration API be registered.
+
+        Returns the MXID of the new user."""
+        urlencoded_token = quote(appservice_token)
+        channel = self.make_request(
+            "POST",
+            f"/_matrix/client/r0/register?access_token={urlencoded_token}",
+            {
+                "username": username,
+                "type": "m.login.application_service",
+            },
+        )
+        return channel.json_body["user_id"]
+
     def login(
         self,
         username,