Prevent local user profiles leaking when a "per-room profile" is changed

changelog.d/10695.bugfix

@@ -0,0 +1 @@
+TODO write a proper changelog dmr.

synapse/handlers/deactivate_account.py

@@ -131,7 +131,7 @@ async def deactivate_account(
         await self.store.add_user_pending_deactivation(user_id)
 
         # delete from user directory
-        await self.user_directory_handler.handle_user_deactivated(user_id)
+        await self.user_directory_handler.handle_local_user_deactivated(user_id)
 
         # Mark the user as erased, if they asked for that
         if erase_data:
@@ -248,7 +248,7 @@ async def activate_account(self, user_id: str) -> None:
         This marks the user as active and not erased in the database, but does
         not attempt to rejoin rooms, re-add threepids, etc.
 
-        If enabled, the user will be re-added to the user directory.
+        The user will be re-added to the user directory.
 
         The user will also need a password hash set to actually login.
 
@@ -257,11 +257,8 @@ async def activate_account(self, user_id: str) -> None:
         """
         # Add the user to the directory, if necessary.
         user = UserID.from_string(user_id)
-        if self.hs.config.user_directory_search_all_users:
-            profile = await self.store.get_profileinfo(user.localpart)
-            await self.user_directory_handler.handle_local_profile_change(
-                user_id, profile
-            )
+        profile = await self.store.get_profileinfo(user.localpart)
+        await self.user_directory_handler.handle_local_profile_change(user_id, profile)
 
         # Ensure the user is not marked as erased.
         await self.store.mark_user_not_erased(user_id)

synapse/handlers/profile.py

@@ -214,11 +214,10 @@ async def set_displayname(
             target_user.localpart, displayname_to_set
         )
 
-        if self.hs.config.user_directory_search_all_users:
-            profile = await self.store.get_profileinfo(target_user.localpart)
-            await self.user_directory_handler.handle_local_profile_change(
-                target_user.to_string(), profile
-            )
+        profile = await self.store.get_profileinfo(target_user.localpart)
+        await self.user_directory_handler.handle_local_profile_change(
+            target_user.to_string(), profile
+        )
 
         await self._update_join_states(requester, target_user)
 
@@ -300,11 +299,10 @@ async def set_avatar_url(
             target_user.localpart, avatar_url_to_set
         )
 
-        if self.hs.config.user_directory_search_all_users:
-            profile = await self.store.get_profileinfo(target_user.localpart)
-            await self.user_directory_handler.handle_local_profile_change(
-                target_user.to_string(), profile
-            )
+        profile = await self.store.get_profileinfo(target_user.localpart)
+        await self.user_directory_handler.handle_local_profile_change(
+            target_user.to_string(), profile
+        )
 
         await self._update_join_states(requester, target_user)
 

synapse/handlers/register.py

@@ -289,11 +289,10 @@ async def register_user(
                 shadow_banned=shadow_banned,
             )
 
-            if self.hs.config.user_directory_search_all_users:
-                profile = await self.store.get_profileinfo(localpart)
-                await self.user_directory_handler.handle_local_profile_change(
-                    user_id, profile
-                )
+            profile = await self.store.get_profileinfo(localpart)
+            await self.user_directory_handler.handle_local_profile_change(
+                user_id, profile
+            )
 
         else:
             # autogen a sequential user ID

synapse/handlers/state_deltas.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import logging
+from enum import Enum, auto
 from typing import TYPE_CHECKING, Optional
 
 if TYPE_CHECKING:
@@ -21,6 +22,12 @@
 logger = logging.getLogger(__name__)
 
 
+class MatchChange(Enum):
+    no_change = auto()
+    now_true = auto()
+    now_false = auto()
+
+
 class StateDeltasHandler:
     def __init__(self, hs: "HomeServer"):
         self.store = hs.get_datastore()
@@ -31,18 +38,12 @@ async def _get_key_change(
         event_id: Optional[str],
         key_name: str,
         public_value: str,
-    ) -> Optional[bool]:
+    ) -> MatchChange:
         """Given two events check if the `key_name` field in content changed
         from not matching `public_value` to doing so.
 
         For example, check if `history_visibility` (`key_name`) changed from
         `shared` to `world_readable` (`public_value`).
-
-        Returns:
-            None if the field in the events either both match `public_value`
-            or if neither do, i.e. there has been no change.
-            True if it didn't match `public_value` but now does
-            False if it did match `public_value` but now doesn't
         """
         prev_event = None
         event = None
@@ -54,7 +55,7 @@ async def _get_key_change(
 
         if not event and not prev_event:
             logger.debug("Neither event exists: %r %r", prev_event_id, event_id)
-            return None
+            return MatchChange.no_change
 
         prev_value = None
         value = None
@@ -68,8 +69,8 @@ async def _get_key_change(
         logger.debug("prev_value: %r -> value: %r", prev_value, value)
 
         if value == public_value and prev_value != public_value:
-            return True
+            return MatchChange.now_true
         elif value != public_value and prev_value == public_value:
-            return False
+            return MatchChange.now_false
         else:
-            return None
+            return MatchChange.no_change

synapse/handlers/user_directory.py

@@ -17,7 +17,7 @@
 
 import synapse.metrics
 from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules, Membership
-from synapse.handlers.state_deltas import StateDeltasHandler
+from synapse.handlers.state_deltas import MatchChange, StateDeltasHandler
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.storage.roommember import ProfileInfo
 from synapse.types import JsonDict
@@ -30,14 +30,26 @@
 
 
 class UserDirectoryHandler(StateDeltasHandler):
-    """Handles querying of and keeping updated the user_directory.
+    """Handles queries and updates for the user_directory.
 
     N.B.: ASSUMES IT IS THE ONLY THING THAT MODIFIES THE USER DIRECTORY
 
-    The user directory is filled with users who this server can see are joined to a
-    world_readable or publicly joinable room. We keep a database table up to date
-    by streaming changes of the current state and recalculating whether users should
-    be in the directory or not when necessary.
+    When a local user searches the user_directory, we report two kinds of users:
+
+    - users this server can see are joined to a world_readable or publicly
+      joinable room, and
+    - users belonging to a private room that contains a local user.
+
+    The two cases are tracked separately in the `users_in_public_rooms` and
+    `users_who_share_private_rooms` tables. Both kinds of users have their
+    username and avatar tracked in a `user_directory` table.
+
+    This handler has three responsibilities:
+    1. Forwarding requests to `/user_directory/search` to the UserDirectoryStore.
+    2. Providing hooks for the application to call when local users are added,
+       removed, or have their profile changed.
+    3. Listening for room state changes that indicate remote users have
+       joined or left a room, or that their profile has changed.
     """
 
     def __init__(self, hs: "HomeServer"):
@@ -94,23 +106,6 @@ async def search_users(
 
         return results
 
-    def notify_new_event(self) -> None:
-        """Called when there may be more deltas to process"""
-        if not self.update_user_directory:
-            return
-
-        if self._is_processing:
-            return
-
-        async def process():
-            try:
-                await self._unsafe_process()
-            finally:
-                self._is_processing = False
-
-        self._is_processing = True
-        run_as_background_process("user_directory.notify_new_event", process)
-
     async def handle_local_profile_change(
         self, user_id: str, profile: ProfileInfo
     ) -> None:
@@ -130,12 +125,29 @@ async def handle_local_profile_change(
                 user_id, profile.display_name, profile.avatar_url
             )
 
-    async def handle_user_deactivated(self, user_id: str) -> None:
+    async def handle_local_user_deactivated(self, user_id: str) -> None:
         """Called when a user ID is deactivated"""
         # FIXME(#3714): We should probably do this in the same worker as all
         # the other changes.
         await self.store.remove_from_user_dir(user_id)
 
+    def notify_new_event(self) -> None:
+        """Called when there may be more deltas to process"""
+        if not self.update_user_directory:
+            return
+
+        if self._is_processing:
+            return
+
+        async def process():
+            try:
+                await self._unsafe_process()
+            finally:
+                self._is_processing = False
+
+        self._is_processing = True
+        run_as_background_process("user_directory.notify_new_event", process)
+
     async def _unsafe_process(self) -> None:
         # If self.pos is None then means we haven't fetched it from DB
         if self.pos is None:
@@ -189,14 +201,14 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None:
                     room_id, prev_event_id, event_id, typ
                 )
             elif typ == EventTypes.Member:
-                change = await self._get_key_change(
+                joined = await self._get_key_change(
                     prev_event_id,
                     event_id,
                     key_name="membership",
                     public_value=Membership.JOIN,
                 )
 
-                if change is False:
+                if joined is MatchChange.now_false:
                     # Need to check if the server left the room entirely, if so
                     # we might need to remove all the users in that room
                     is_in_room = await self.store.is_host_joined(
@@ -217,30 +229,32 @@ 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:
-                    if change is None:
-                        # Handle any profile changes
+                is_support_user = await self.store.is_support_user(state_key)
+                if is_support_user:
+                    continue
+
+                if joined is MatchChange.no_change:
+                    # Handle any profile changes for remote users
+                    if not self.is_mine_id(state_key):
                         await self._handle_profile_change(
                             state_key, room_id, prev_event_id, event_id
                         )
-                        continue
 
-                    if change:  # The user joined
-                        event = await self.store.get_event(event_id, allow_none=True)
-                        # It isn't expected for this event to not exist, but we
-                        # don't want the entire background process to break.
-                        if event is None:
-                            continue
+                elif joined is MatchChange.now_true:  # The user joined
+                    event = await self.store.get_event(event_id, allow_none=True)
+                    # It isn't expected for this event to not exist, but we
+                    # don't want the entire background process to break.
+                    if event is None:
+                        continue
 
-                        profile = ProfileInfo(
-                            avatar_url=event.content.get("avatar_url"),
-                            display_name=event.content.get("displayname"),
-                        )
+                    profile = ProfileInfo(
+                        avatar_url=event.content.get("avatar_url"),
+                        display_name=event.content.get("displayname"),
+                    )
 
-                        await self._handle_new_user(room_id, state_key, profile)
-                    else:  # The user left
-                        await self._handle_remove_user(room_id, state_key)
+                    await self._handle_new_user(room_id, state_key, profile)
+                else:  # The user left
+                    await self._handle_remove_user(room_id, state_key)
             else:
                 logger.debug("Ignoring irrelevant type: %r", typ)
 
@@ -263,14 +277,14 @@ async def _handle_room_publicity_change(
         logger.debug("Handling change for %s: %s", typ, room_id)
 
         if typ == EventTypes.RoomHistoryVisibility:
-            change = await self._get_key_change(
+            publicity = await self._get_key_change(
                 prev_event_id,
                 event_id,
                 key_name="history_visibility",
                 public_value=HistoryVisibility.WORLD_READABLE,
             )
         elif typ == EventTypes.JoinRules:
-            change = await self._get_key_change(
+            publicity = await self._get_key_change(
                 prev_event_id,
                 event_id,
                 key_name="join_rule",
@@ -280,7 +294,7 @@ async def _handle_room_publicity_change(
             raise Exception("Invalid event type")
         # If change is None, no change. True => become world_readable/public,
         # False => was world_readable/public
-        if change is None:
+        if publicity is MatchChange.no_change:
             logger.debug("No change")
             return
 
@@ -290,13 +304,13 @@ async def _handle_room_publicity_change(
             room_id
         )
 
-        logger.debug("Change: %r, is_public: %r", change, is_public)
+        logger.debug("Change: %r, is_public: %r", publicity, is_public)
 
-        if change and not is_public:
+        if publicity is MatchChange.now_true and not is_public:
             # If we became world readable but room isn't currently public then
             # we ignore the change
             return
-        elif not change and is_public:
+        elif publicity is MatchChange.now_false and is_public:
             # If we stopped being world readable but are still public,
             # ignore the change
             return