From 89fc579329d7c81c040b1c178099860e7de37bed Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Fri, 26 Apr 2024 10:52:24 +0100
Subject: [PATCH] Fix filtering of rooms when supplying the `destination` query
 parameter to `/_synapse/admin/v1/federation/destinations/<destination>/rooms`
 (#17077)

---
 changelog.d/17077.bugfix                      |  1 +
 .../storage/databases/main/transactions.py    |  1 +
 tests/rest/admin/test_federation.py           | 67 ++++++++++++++++++-
 3 files changed, 66 insertions(+), 3 deletions(-)
 create mode 100644 changelog.d/17077.bugfix

diff --git a/changelog.d/17077.bugfix b/changelog.d/17077.bugfix
new file mode 100644
index 0000000000..7d8ea37406
--- /dev/null
+++ b/changelog.d/17077.bugfix
@@ -0,0 +1 @@
+Fixes a bug introduced in v1.52.0 where the `destination` query parameter for the  [Destination Rooms Admin API](https://element-hq.github.io/synapse/v1.105/usage/administration/admin_api/federation.html#destination-rooms) failed to actually filter returned rooms.
\ No newline at end of file
diff --git a/synapse/storage/databases/main/transactions.py b/synapse/storage/databases/main/transactions.py
index 08e0241f68..770802483c 100644
--- a/synapse/storage/databases/main/transactions.py
+++ b/synapse/storage/databases/main/transactions.py
@@ -660,6 +660,7 @@ class TransactionWorkerStore(CacheInvalidationWorkerStore):
                     limit=limit,
                     retcols=("room_id", "stream_ordering"),
                     order_direction=order,
+                    keyvalues={"destination": destination},
                 ),
             )
             return rooms, count
diff --git a/tests/rest/admin/test_federation.py b/tests/rest/admin/test_federation.py
index c1d88f0176..c2015774a1 100644
--- a/tests/rest/admin/test_federation.py
+++ b/tests/rest/admin/test_federation.py
@@ -778,20 +778,81 @@ class DestinationMembershipTestCase(unittest.HomeserverTestCase):
         self.assertEqual(number_rooms, len(channel.json_body["rooms"]))
         self._check_fields(channel.json_body["rooms"])
 
-    def _create_destination_rooms(self, number_rooms: int) -> None:
-        """Create a number rooms for destination
+    def test_room_filtering(self) -> None:
+        """Tests that rooms are correctly filtered"""
+
+        # Create two rooms on the homeserver. Each has a different remote homeserver
+        # participating in it.
+        other_destination = "other.destination.org"
+        room_ids_self_dest = self._create_destination_rooms(2, destination=self.dest)
+        room_ids_other_dest = self._create_destination_rooms(
+            1, destination=other_destination
+        )
+
+        # Ask for the rooms that `self.dest` is participating in.
+        channel = self.make_request("GET", self.url, access_token=self.admin_user_tok)
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+
+        # Verify that we received only the rooms that `self.dest` is participating in.
+        # This assertion method name is a bit misleading. It does check that both lists
+        # contain the same items, and the same counts.
+        self.assertCountEqual(
+            [r["room_id"] for r in channel.json_body["rooms"]], room_ids_self_dest
+        )
+        self.assertEqual(channel.json_body["total"], len(room_ids_self_dest))
+
+        # Ask for the rooms that `other_destination` is participating in.
+        channel = self.make_request(
+            "GET",
+            self.url.replace(self.dest, other_destination),
+            access_token=self.admin_user_tok,
+        )
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+
+        # Verify that we received only the rooms that `other_destination` is
+        # participating in.
+        self.assertCountEqual(
+            [r["room_id"] for r in channel.json_body["rooms"]], room_ids_other_dest
+        )
+        self.assertEqual(channel.json_body["total"], len(room_ids_other_dest))
+
+    def _create_destination_rooms(
+        self,
+        number_rooms: int,
+        destination: Optional[str] = None,
+    ) -> List[str]:
+        """
+        Create the given number of rooms. The given `destination` homeserver will
+        be recorded as a participant.
 
         Args:
             number_rooms: Number of rooms to be created
+            destination: The domain of the homeserver that will be considered
+                as a participant in the rooms.
+
+        Returns:
+            The IDs of the rooms that have been created.
         """
+        room_ids = []
+
+        # If no destination was provided, default to `self.dest`.
+        if destination is None:
+            destination = self.dest
+
         for _ in range(number_rooms):
             room_id = self.helper.create_room_as(
                 self.admin_user, tok=self.admin_user_tok
             )
+            room_ids.append(room_id)
+
             self.get_success(
-                self.store.store_destination_rooms_entries((self.dest,), room_id, 1234)
+                self.store.store_destination_rooms_entries(
+                    (destination,), room_id, 1234
+                )
             )
 
+        return room_ids
+
     def _check_fields(self, content: List[JsonDict]) -> None:
         """Checks that the expected room attributes are present in content