Fix an edge-case with invited rooms over federation in the spaces summary. (#10560)

If a room which the requesting user was invited to was queried over
federation it will now properly appear in the spaces summary (instead
of being stripped out by the requesting server).

3 files changed, 138 insertions, 62 deletions

diff --git a/changelog.d/10560.feature b/changelog.d/10560.feature
new file mode 100644
index 0000000000..ffc4e4289c
--- /dev/null
+++ b/changelog.d/10560.feature
@@ -0,0 +1 @@
+Add pagination to the spaces summary based on updates to [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946).
diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py
index 2517f278b6..d04afe6c31 100644
--- a/synapse/handlers/space_summary.py
+++ b/synapse/handlers/space_summary.py
@@ -158,48 +158,10 @@ class SpaceSummaryHandler:
                     room = room_entry.room
                     fed_room_id = room_entry.room_id
 
-                    # The room should only be included in the summary if:
-                    #     a. the user is in the room;
-                    #     b. the room is world readable; or
-                    #     c. the user could join the room, e.g. the join rules
-                    #        are set to public or the user is in a space that
-                    #        has been granted access to the room.
-                    #
-                    # Note that we know the user is not in the root room (which is
-                    # why the remote call was made in the first place), but the user
-                    # could be in one of the children rooms and we just didn't know
-                    # about the link.
-
-                    # The API doesn't return the room version so assume that a
-                    # join rule of knock is valid.
-                    include_room = (
-                        room.get("join_rules") in (JoinRules.PUBLIC, JoinRules.KNOCK)
-                        or room.get("world_readable") is True
-                    )
-
-                    # Check if the user is a member of any of the allowed spaces
-                    # from the response.
-                    allowed_rooms = room.get("allowed_room_ids") or room.get(
-                        "allowed_spaces"
-                    )
-                    if (
-                        not include_room
-                        and allowed_rooms
-                        and isinstance(allowed_rooms, list)
-                    ):
-                        include_room = await self._event_auth_handler.is_user_in_rooms(
-                            allowed_rooms, requester
-                        )
-
-                    # Finally, if this isn't the requested room, check ourselves
-                    # if we can access the room.
-                    if not include_room and fed_room_id != queue_entry.room_id:
-                        include_room = await self._is_room_accessible(
-                            fed_room_id, requester, None
-                        )
-
                     # The user can see the room, include it!
-                    if include_room:
+                    if await self._is_remote_room_accessible(
+                        requester, fed_room_id, room
+                    ):
                         # Before returning to the client, remove the allowed_room_ids
                         # and allowed_spaces keys.
                         room.pop("allowed_room_ids", None)
@@ -336,7 +298,7 @@ class SpaceSummaryHandler:
         Returns:
             A room entry if the room should be returned. None, otherwise.
         """
-        if not await self._is_room_accessible(room_id, requester, origin):
+        if not await self._is_local_room_accessible(room_id, requester, origin):
             return None
 
         room_entry = await self._build_room_entry(room_id, for_federation=bool(origin))
@@ -438,7 +400,7 @@ class SpaceSummaryHandler:
 
         return results
 
-    async def _is_room_accessible(
+    async def _is_local_room_accessible(
         self, room_id: str, requester: Optional[str], origin: Optional[str]
     ) -> bool:
         """
@@ -550,6 +512,51 @@ class SpaceSummaryHandler:
         )
         return False
 
+    async def _is_remote_room_accessible(
+        self, requester: str, room_id: str, room: JsonDict
+    ) -> bool:
+        """
+        Calculate whether the room received over federation should be shown in the spaces summary.
+
+        It should be included if:
+
+        * The requester is joined or can join the room (per MSC3173).
+        * The history visibility is set to world readable.
+
+        Note that the local server is not in the requested room (which is why the
+        remote call was made in the first place), but the user could have access
+        due to an invite, etc.
+
+        Args:
+            requester: The user requesting the summary.
+            room_id: The room ID returned over federation.
+            room: The summary of the child room returned over federation.
+
+        Returns:
+            True if the room should be included in the spaces summary.
+        """
+        # The API doesn't return the room version so assume that a
+        # join rule of knock is valid.
+        if (
+            room.get("join_rules") in (JoinRules.PUBLIC, JoinRules.KNOCK)
+            or room.get("world_readable") is True
+        ):
+            return True
+
+        # Check if the user is a member of any of the allowed spaces
+        # from the response.
+        allowed_rooms = room.get("allowed_room_ids") or room.get("allowed_spaces")
+        if allowed_rooms and isinstance(allowed_rooms, list):
+            if await self._event_auth_handler.is_user_in_rooms(
+                allowed_rooms, requester
+            ):
+                return True
+
+        # Finally, check locally if we can access the room. The user might
+        # already be in the room (if it was a child room), or there might be a
+        # pending invite, etc.
+        return await self._is_local_room_accessible(room_id, requester, None)
+
     async def _build_room_entry(self, room_id: str, for_federation: bool) -> JsonDict:
         """
         Generate en entry suitable for the 'rooms' list in the summary response.
diff --git a/tests/handlers/test_space_summary.py b/tests/handlers/test_space_summary.py
index 6cc1a02e12..f470c81ea2 100644
--- a/tests/handlers/test_space_summary.py
+++ b/tests/handlers/test_space_summary.py
@@ -30,7 +30,7 @@ from synapse.handlers.space_summary import _child_events_comparison_key, _RoomEn
 from synapse.rest import admin
 from synapse.rest.client.v1 import login, room
 from synapse.server import HomeServer
-from synapse.types import JsonDict
+from synapse.types import JsonDict, UserID
 
 from tests import unittest
 
@@ -149,6 +149,36 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             events,
         )
 
+    def _poke_fed_invite(self, room_id: str, from_user: str) -> None:
+        """
+        Creates a invite (as if received over federation) for the room from the
+        given hostname.
+
+        Args:
+            room_id: The room ID to issue an invite for.
+            fed_hostname: The user to invite from.
+        """
+        # Poke an invite over federation into the database.
+        fed_handler = self.hs.get_federation_handler()
+        fed_hostname = UserID.from_string(from_user).domain
+        event = make_event_from_dict(
+            {
+                "room_id": room_id,
+                "event_id": "!abcd:" + fed_hostname,
+                "type": EventTypes.Member,
+                "sender": from_user,
+                "state_key": self.user,
+                "content": {"membership": Membership.INVITE},
+                "prev_events": [],
+                "auth_events": [],
+                "depth": 1,
+                "origin_server_ts": 1234,
+            }
+        )
+        self.get_success(
+            fed_handler.on_invite_request(fed_hostname, event, RoomVersions.V6)
+        )
+
     def test_simple_space(self):
         """Test a simple space with a single room."""
         result = self.get_success(self.handler.get_space_summary(self.user, self.space))
@@ -416,24 +446,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         joined_room = self.helper.create_room_as(self.user, tok=self.token)
 
         # Poke an invite over federation into the database.
-        fed_handler = self.hs.get_federation_handler()
-        event = make_event_from_dict(
-            {
-                "room_id": invited_room,
-                "event_id": "!abcd:" + fed_hostname,
-                "type": EventTypes.Member,
-                "sender": "@remote:" + fed_hostname,
-                "state_key": self.user,
-                "content": {"membership": Membership.INVITE},
-                "prev_events": [],
-                "auth_events": [],
-                "depth": 1,
-                "origin_server_ts": 1234,
-            }
-        )
-        self.get_success(
-            fed_handler.on_invite_request(fed_hostname, event, RoomVersions.V6)
-        )
+        self._poke_fed_invite(invited_room, "@remote:" + fed_hostname)
 
         async def summarize_remote_room(
             _self, room, suggested_only, max_children, exclude_rooms
@@ -570,3 +583,58 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
                 (subspace, joined_room),
             ],
         )
+
+    def test_fed_invited(self):
+        """
+        A room which the user was invited to should be included in the response.
+
+        This differs from test_fed_filtering in that the room itself is being
+        queried over federation, instead of it being included as a sub-room of
+        a space in the response.
+        """
+        fed_hostname = self.hs.hostname + "2"
+        fed_room = "#subroom:" + fed_hostname
+
+        # Poke an invite over federation into the database.
+        self._poke_fed_invite(fed_room, "@remote:" + fed_hostname)
+
+        async def summarize_remote_room(
+            _self, room, suggested_only, max_children, exclude_rooms
+        ):
+            return [
+                _RoomEntry(
+                    fed_room,
+                    {
+                        "room_id": fed_room,
+                        "world_readable": False,
+                        "join_rules": JoinRules.INVITE,
+                    },
+                ),
+            ]
+
+        # Add a room to the space which is on another server.
+        self._add_child(self.space, fed_room, self.token)
+
+        with mock.patch(
+            "synapse.handlers.space_summary.SpaceSummaryHandler._summarize_remote_room",
+            new=summarize_remote_room,
+        ):
+            result = self.get_success(
+                self.handler.get_space_summary(self.user, self.space)
+            )
+
+        self._assert_rooms(
+            result,
+            [
+                self.space,
+                self.room,
+                fed_room,
+            ],
+        )
+        self._assert_events(
+            result,
+            [
+                (self.space, self.room),
+                (self.space, fed_room),
+            ],
+        )