Ensure we don't respond with profile fields the client didn't ask for

synapse/handlers/sync.py

@@ -2329,12 +2329,21 @@ class SyncHandler:
 
         # Process field updates and users who have events in the sync response
         if users:
-            user_fields: dict[str, set[str]] = {}
+            updated_user_fields: dict[str, set[str]] = {}
             # Set fields from updates
             for update in updates:
-                if not update.field_name or update.user_id not in users:
+                # Skip the update if there is no field update (a joined or left room
+                # action), the client didn't ask for this field, or we're not
+                # interested in this user.
+                if (
+                    not update.field_name
+                    or update.field_name not in profile_fields
+                    or update.user_id not in users
+                ):
                     continue
-                user_fields.setdefault(update.user_id, set()).add(update.field_name)
+                updated_user_fields.setdefault(update.user_id, set()).add(
+                    update.field_name
+                )
 
             # Note: there's a small race condition here where a profile update may
             # occur between fetching `now_token` above and reaching this step. In
@@ -2357,10 +2366,11 @@ class SyncHandler:
 
                 per_user_updates: dict[str, JsonValue | dict[str, JsonValue]] = {}
                 if include_users and other_user_id in include_users:
-                    # Include the full profile as this user has events in
-                    # a lazy loaded sync response, except for fields we've recently
-                    # sent in a previous lazy loaded sync response
-                    for field_name in profile_data.keys():
+                    # Include all the fields the client asked for, as this user
+                    # has events in a lazy loaded sync response, except for
+                    # fields we've recently sent in a previous lazy loaded sync response
+                    fields = set(profile_data.keys()).intersection(profile_fields)
+                    for field_name in fields:
                         cache_key = (
                             sync_config.user.to_string(),
                             sync_config.device_id,
@@ -2388,12 +2398,15 @@ class SyncHandler:
                                     cast(str, profile_data.get(field_name)),
                                 )
                 else:
-                    # Include only the diff, unless the user recently joined
-                    # We don't use a cache here as changes are always sent
+                    # Include only the diff, unless the user recently joined,
+                    # then send all the fields the client asked for.
+                    # We don't use a cache here as for non-lazy sync we always
+                    # send changes and/or fields the client asked for, if relevant
+                    # as above joined condition.
                     fields = (
-                        list(profile_data.keys())
+                        profile_fields
                         if other_user_id in joined_room_user_ids
-                        else user_fields.get(other_user_id, [])
+                        else set(updated_user_fields.get(other_user_id, []))
                     )
                     for field_name in fields:
                         per_user_updates[field_name] = profile_data.get(field_name)

tests/handlers/test_sync.py

@@ -1239,8 +1239,8 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
     @override_config({"include_profile_updates_in_sync": True})
     def test_initial_sync_responds_with_tracked_profile_updates(self) -> None:
         """Test that with MSC4429 enabled the initial sync response does
-        contain profile updates for users who share rooms, including our
-        syncing user."""
+        contain profile updates for users who share rooms, for the fields the
+        client requests. This response should include our syncing user."""
         self.get_success(
             self.profile_handler.set_field(
                 target_user=UserID.from_string(self.other_user),
@@ -1249,6 +1249,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                 new_value={"text": "On holiday", "emoji": "🏖"},
             )
         )
+        # Also set a field the client doesn't want
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(self.other_user),
+                requester=create_requester(self.other_user),
+                field_name="displayname",
+                new_value="New displayname",
+            )
+        )
 
         requester = create_requester(self.user)
         initial_result = self.get_success(
@@ -1259,9 +1268,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                     filter_collection=FilterCollection(
                         hs=self.hs,
                         filter_json={
-                            "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
-                            }
+                            "org.matrix.msc4429.profile_fields": {"ids": ["m.status"]}
                         },
                     ),
                 ),
@@ -1274,6 +1281,9 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             initial_result.profile_updates["@other_user:test"]["m.status"],
             {"text": "On holiday", "emoji": "🏖"},
         )
+        self.assertFalse(
+            "displayname" in initial_result.profile_updates["@other_user:test"].keys(),
+        )
         self.assertCountEqual(
             initial_result.profile_updates.keys(),
             [
@@ -1409,11 +1419,11 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         )
 
     @override_config({"include_profile_updates_in_sync": True})
-    def test_incremental_sync_sends_down_profile_updates(
+    def test_incremental_sync_sends_down_profile_update_diffs(
         self,
     ) -> None:
         """Test that with MSC4429 enabled the incremental sync response does
-        contain profile updates."""
+        contain profile update diffs."""
         requester = create_requester(self.user)
         initial_result = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
@@ -1440,6 +1450,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                 new_value={"text": "On holiday", "emoji": "🏖"},
             )
         )
+        # Set a field the client didn't ask for
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(self.other_user),
+                requester=create_requester(self.other_user),
+                field_name="uninterestingfield",
+                new_value="Content",
+            )
+        )
         incremental_result = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
                 requester,
@@ -1464,8 +1483,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             {"text": "On holiday", "emoji": "🏖"},
         )
         # We only send diffs in incremental sync for profile field updates
-        self.assertIsNone(
-            incremental_result.profile_updates["@other_user:test"].get("displayname"),
+        self.assertFalse(
+            "displayname"
+            in incremental_result.profile_updates["@other_user:test"].keys(),
+        )
+        # The client didn't ask for this field
+        self.assertFalse(
+            "uninterestingfield"
+            in incremental_result.profile_updates["@other_user:test"].keys(),
         )
 
     @override_config({"include_profile_updates_in_sync": True})
@@ -1494,7 +1519,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         hs=self.hs,
                         filter_json={
                             "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
+                                "ids": ["m.status", "displayname"]
                             }
                         },
                     ),
@@ -1524,6 +1549,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         self.helper.send_messages(
             room_id=self.joined_room, num_events=10, tok=third_tok
         )
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(third_user),
+                requester=create_requester(third_user),
+                field_name="uninterestingfield",
+                new_value="Content",
+            )
+        )
         # Join a federated user to the room
         self.get_success(
             inject_member_event(
@@ -1543,7 +1576,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         hs=self.hs,
                         filter_json={
                             "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
+                                "ids": ["m.status", "displayname"]
                             },
                             "room": {
                                 "state": {
@@ -1562,6 +1595,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         self.assertFalse(
             "@federateduser:federatedhs" in incremental_result.profile_updates.keys()
         )
+
         # Lazy loading only filters initial sync profile updates. Incremental syncs
         # should include all tracked profile updates for the syncing user.
         self.assertCountEqual(
@@ -1572,22 +1606,31 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             ],
         )
         assert incremental_result.profile_updates["@other_user:test"] is not None
+
         # This is a field update, so should be here
         self.assertEqual(
             incremental_result.profile_updates["@other_user:test"]["m.status"],
             {"text": "On holiday", "emoji": "🏖"},
         )
+
         # We don't have events for this user in this response, so their full profile
         # is not included
-        self.assertIsNone(
-            incremental_result.profile_updates["@other_user:test"].get("displayname"),
+        self.assertFalse(
+            "displayname"
+            in incremental_result.profile_updates["@other_user:test"].keys(),
         )
         assert incremental_result.profile_updates["@third_user:test"] is not None
-        # This user has events in the timeline, thus their full profile is included
+
+        # This user has events in the timeline, thus the fields the client asked for
+        # are included
         self.assertEqual(
             incremental_result.profile_updates["@third_user:test"]["m.status"],
             {"text": "On fire", "emoji": "🔥"},
         )
+        self.assertFalse(
+            "uninterestingfield"
+            in incremental_result.profile_updates["@third_user:test"].keys(),
+        )
         self.assertEqual(
             incremental_result.profile_updates["@third_user:test"]["displayname"],
             "third_user",
@@ -1595,16 +1638,19 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
 
     @parameterized.expand(
         [
-            True,
-            False,
+            [True, True],
+            [False, False],
+            [True, False],
+            [False, True],
         ]
     )
     @override_config({"include_profile_updates_in_sync": True})
-    def test_lazy_loading_sync_filters_out_profile_updates_from_federated_users(
+    def test_sync_filters_out_profile_updates_from_federated_users(
         self,
         is_initial: bool,
+        is_lazy: bool,
     ) -> None:
-        """Test that with MSC4429 enabled lazy loading sync response
+        """Test that with MSC4429 enabled any sync response
         doesn't contain federated users even if there are timeline events from them.
         """
         # Join a federated user to the room, causing a membership event into
@@ -1618,6 +1664,17 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             )
         )
         requester = create_requester(self.user)
+        filter_json: dict[str, dict] = {
+            "org.matrix.msc4429.profile_fields": {
+                "ids": ["m.status", "displayname", "avatar_url"]
+            },
+        }
+        if is_lazy:
+            filter_json["room"] = {
+                "state": {
+                    "lazy_load_members": True,
+                },
+            }
         initial_result = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
                 requester,
@@ -1625,16 +1682,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                     user_id=self.user,
                     filter_collection=FilterCollection(
                         hs=self.hs,
-                        filter_json={
-                            "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
-                            },
-                            "room": {
-                                "state": {
-                                    "lazy_load_members": True,
-                                },
-                            },
-                        },
+                        filter_json=filter_json,
                     ),
                 ),
                 request_key=generate_request_key(),
@@ -1664,16 +1712,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         user_id=self.user,
                         filter_collection=FilterCollection(
                             hs=self.hs,
-                            filter_json={
-                                "org.matrix.msc4429.profile_fields": {
-                                    "ids": ["m.status", "displayname", "avatar_url"]
-                                },
-                                "room": {
-                                    "state": {
-                                        "lazy_load_members": True,
-                                    },
-                                },
-                            },
+                            filter_json=filter_json,
                         ),
                     ),
                     request_key=generate_request_key(),
@@ -1688,13 +1727,21 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             )
 
     @override_config({"include_profile_updates_in_sync": True})
-    def test_incremental_sync_lazy_loading_cache_filters_recently_sent_profiles(
+    def test_incremental_sync_lazy_loading_cache_filters_recently_sent_profiles_and_fields(
         self,
     ) -> None:
         """Test that with MSC4429 enabled the incremental sync lazy loading response
-        filters out unchanged profiles we have recently sent to the client.
+        filters out unchanged profiles or fields we have recently sent to the client.
         """
         requester = create_requester(self.user)
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(self.other_user),
+                requester=create_requester(self.other_user),
+                field_name="sooninterestingfield",
+                new_value="Content",
+            )
+        )
         initial_result = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
                 requester,
@@ -1747,9 +1794,14 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                 "@other_user:test",
             ],
         )
+        assert incremental_result.profile_updates["@other_user:test"] is not None
+        self.assertEqual(
+            incremental_result.profile_updates["@other_user:test"].keys(),
+            ["displayname", "avatar_url"],
+        )
 
         # If we have more events from the other_user, and do another lazy sync,
-        # we don't expect the full profile to be sent again due to our cache
+        # we don't expect the full profile to be sent again due to our cache.
         self.helper.send_messages(
             room_id=self.joined_room, num_events=1, tok=self.other_tok
         )
@@ -1780,6 +1832,50 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             incremental_result.profile_updates.keys(),
             [],
         )
+        # However, if we again add an event, we do expect any fields the client didn't
+        # previously ask for to be there.
+        self.helper.send_messages(
+            room_id=self.joined_room, num_events=1, tok=self.other_tok
+        )
+        incremental_result = self.get_success(
+            self.sync_handler.wait_for_sync_for_user(
+                requester,
+                since_token=incremental_result.next_batch,
+                sync_config=generate_sync_config(
+                    user_id=self.user,
+                    filter_collection=FilterCollection(
+                        hs=self.hs,
+                        filter_json={
+                            "org.matrix.msc4429.profile_fields": {
+                                "ids": [
+                                    "m.status",
+                                    "displayname",
+                                    "avatar_url",
+                                    "sooninterestingfield",
+                                ]
+                            },
+                            "room": {
+                                "state": {
+                                    "lazy_load_members": True,
+                                },
+                            },
+                        },
+                    ),
+                ),
+                request_key=generate_request_key(),
+            )
+        )
+        self.assertCountEqual(
+            incremental_result.profile_updates.keys(),
+            [
+                "@other_user:test",
+            ],
+        )
+        assert incremental_result.profile_updates["@other_user:test"] is not None
+        self.assertEqual(
+            incremental_result.profile_updates["@other_user:test"].keys(),
+            ["sooninterestingfield"],
+        )
 
     @override_config({"include_profile_updates_in_sync": True})
     def test_incremental_sync_sends_down_null_profile_if_user_no_longer_sharing_rooms(
@@ -1832,11 +1928,12 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         )
 
     @override_config({"include_profile_updates_in_sync": True})
-    def test_incremental_sync_sends_down_full_profile_for_users_who_have_joined(
+    def test_incremental_sync_sends_down_all_requested_fields_for_users_who_have_joined(
         self,
     ) -> None:
         """Test that with MSC4429 enabled the incremental sync response
-        includes the full profile of a user who has joined a room with the syncing user.
+        includes all the requested fields of a user who has joined a room with the
+        syncing user.
         """
         requester = create_requester(self.user)
         initial_result = self.get_success(
@@ -1848,7 +1945,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         hs=self.hs,
                         filter_json={
                             "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
+                                "ids": ["displayname", "avatar_url"]
                             },
                         },
                     ),
@@ -1866,7 +1963,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         hs=self.hs,
                         filter_json={
                             "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
+                                "ids": ["displayname", "avatar_url"]
                             },
                         },
                     ),
@@ -1882,7 +1979,15 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             user=third_user,
             tok=third_tok,
         )
-
+        # Set a status field we don't except to see in sync
+        self.get_success(
+            self.profile_handler.set_field(
+                target_user=UserID.from_string(third_user),
+                requester=create_requester(third_user),
+                field_name="m.status",
+                new_value={"text": "On fire", "emoji": "🔥"},
+            )
+        )
         incremental_result = self.get_success(
             self.sync_handler.wait_for_sync_for_user(
                 requester,
@@ -1893,7 +1998,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
                         hs=self.hs,
                         filter_json={
                             "org.matrix.msc4429.profile_fields": {
-                                "ids": ["m.status", "displayname", "avatar_url"]
+                                "ids": ["displayname", "avatar_url"]
                             },
                         },
                     ),
@@ -1913,6 +2018,9 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         self.assertIsNone(
             incremental_result.profile_updates["@third_user:test"]["avatar_url"],
         )
+        self.assertFalse(
+            "m.status" in incremental_result.profile_updates["@third_user:test"].keys(),
+        )
 
     @parameterized.expand(
         [
@@ -1926,9 +2034,7 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
         ones own profile updates."""
         requester = create_requester(self.user)
         filter_json: dict[str, dict] = {
-            "org.matrix.msc4429.profile_fields": {
-                "ids": ["m.status", "displayname", "avatar_url"]
-            }
+            "org.matrix.msc4429.profile_fields": {"ids": ["m.status", "avatar_url"]}
         }
         if is_lazy:
             filter_json["room"] = {
@@ -1976,6 +2082,10 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase):
             incremental_result.profile_updates["@user:test"]["m.status"],
             {"text": "On holiday", "emoji": "🏖"},
         )
+        # We didn't ask for displayname
+        self.assertFalse(
+            "displayname" in incremental_result.profile_updates["@user:test"].keys(),
+        )
 
 
 class SyncStateAfterTestCase(tests.unittest.HomeserverTestCase):