Correctly exclude users when making a room public or private

changelog.d/11075.bugfix

@@ -0,0 +1 @@
+Fix a long-standing bug where users excluded from the user directory were added into the directory if they belonged to a room which became public or private.

synapse/handlers/user_directory.py

@@ -273,7 +273,13 @@ async def _handle_room_publicity_change(
         # being added multiple times. The batching upserts shouldn't make this
         # too bad, though.
         for user_id in users_in_room:
-            await self._track_user_joined_room(room_id, user_id)
+            # Check for inclusion here rather than when constructing
+            # users_in_room so that we clean up entries that were erroneously
+            # added.
+            if not self.is_mine_id(
+                user_id
+            ) or await self.store.should_include_local_user_in_dir(user_id):
+                await self._track_user_joined_room(room_id, user_id)
 
     async def _handle_room_membership_event(
         self,
@@ -364,8 +370,8 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None:
         """Someone's just joined a room. Update `users_in_public_rooms` or
         `users_who_share_private_rooms` as appropriate.
 
-        The caller is responsible for ensuring that the given user is not excluded
-        from the user directory.
+        The caller is responsible for ensuring that the given user should be
+        included in the user directory.
         """
         is_public = await self.store.is_room_world_readable_or_publicly_joinable(
             room_id

tests/handlers/test_user_directory.py

@@ -209,6 +209,41 @@ def test_user_not_in_users_table(self) -> None:
         in_public = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
         self.assertEqual(set(in_public), {(user1, room), (user2, room)})
 
+    def test_excludes_users_when_making_room_public(self) -> None:
+        # Create a regular user and a support user.
+        alice = self.register_user("alice", "pass")
+        alice_token = self.login(alice, "pass")
+        support = "@support1:test"
+        self.get_success(
+            self.store.register_user(
+                user_id=support, password_hash=None, user_type=UserTypes.SUPPORT
+            )
+        )
+
+        # Make a public and private room containing Alice and the support user
+        public, initially_private = self._create_rooms_and_inject_memberships(
+            alice, alice_token, support
+        )
+        self._check_only_one_user_in_directory(alice, public)
+
+        # Alice makes the private room public.
+        self.helper.send_state(
+            initially_private,
+            "m.room.join_rules",
+            {"join_rule": "public"},
+            tok=alice_token,
+        )
+
+        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, {alice})
+        self.assertEqual(set(in_public), {(alice, public), (alice, initially_private)})
+        self.assertEqual(in_private, [])
+
     def _create_rooms_and_inject_memberships(
         self, creator: str, token: str, joiner: str
     ) -> Tuple[str, str]:
@@ -232,6 +267,12 @@ def _create_rooms_and_inject_memberships(
         return public_room, private_room
 
     def _check_only_one_user_in_directory(self, user: str, public: str) -> None:
+        """Check that the user directory DB tables show that:
+
+        - only one user is in the user directory
+        - they belong to exactly one public room
+        - they don't share a private room with anyone.
+        """
         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(