Show all joinable rooms in the spaces summary. (#10298)

Previously only world-readable rooms were shown. This means that
rooms which are public, knockable, or invite-only with a pending invitation,
are included in a space summary. It also applies the same logic to
the experimental room version from MSC3083 -- if a user has access
to the proper allowed rooms then it is shown in the spaces summary.

This change is made per MSC3173 allowing stripped state of a room to
be shown to any potential room joiner.

changelog.d/10298.feature

@@ -0,0 +1 @@
+The spaces summary API now returns any joinable rooms, not only rooms which are world-readable.

changelog.d/10305.feature

@@ -0,0 +1 @@
+The spaces summary API now returns any joinable rooms, not only rooms which are world-readable.

changelog.d/10305.misc

@@ -1 +0,0 @@
-Additional unit tests for the spaces summary API.

synapse/handlers/space_summary.py

@@ -24,6 +24,7 @@ from synapse.api.constants import (
     EventContentFields,
     EventTypes,
     HistoryVisibility,
+    JoinRules,
     Membership,
     RoomTypes,
 )
@@ -150,14 +151,21 @@ class SpaceSummaryHandler:
                     # 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 is in a space that has been granted access to
+                    #     c. the user could join the room, e.g. the join rules
-                    #        the room.
+                    #        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.
-                    include_room = room.get("world_readable") is True
+
+                    # 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.
@@ -420,9 +428,8 @@ class SpaceSummaryHandler:
 
         It should be included if:
 
-        * The requester is joined or invited to the room.
+        * The requester is joined or can join the room (per MSC3173).
-        * The requester can join without an invite (per MSC3083).
+        * The origin server has any user that is joined or can join the room.
-        * The origin server has any user that is joined or invited to the room.
         * The history visibility is set to world readable.
 
         Args:
@@ -441,13 +448,39 @@ class SpaceSummaryHandler:
 
         # If there's no state for the room, it isn't known.
         if not state_ids:
+            # The user might have a pending invite for the room.
+            if requester and await self._store.get_invite_for_local_user_in_room(
+                requester, room_id
+            ):
+                return True
+
             logger.info("room %s is unknown, omitting from summary", room_id)
             return False
 
         room_version = await self._store.get_room_version(room_id)
 
-        # if we have an authenticated requesting user, first check if they are able to view
+        # Include the room if it has join rules of public or knock.
-        # stripped state in the room.
+        join_rules_event_id = state_ids.get((EventTypes.JoinRules, ""))
+        if join_rules_event_id:
+            join_rules_event = await self._store.get_event(join_rules_event_id)
+            join_rule = join_rules_event.content.get("join_rule")
+            if join_rule == JoinRules.PUBLIC or (
+                room_version.msc2403_knocking and join_rule == JoinRules.KNOCK
+            ):
+                return True
+
+        # Include the room if it is peekable.
+        hist_vis_event_id = state_ids.get((EventTypes.RoomHistoryVisibility, ""))
+        if hist_vis_event_id:
+            hist_vis_ev = await self._store.get_event(hist_vis_event_id)
+            hist_vis = hist_vis_ev.content.get("history_visibility")
+            if hist_vis == HistoryVisibility.WORLD_READABLE:
+                return True
+
+        # Otherwise we need to check information specific to the user or server.
+
+        # If we have an authenticated requesting user, check if they are a member
+        # of the room (or can join the room).
         if requester:
             member_event_id = state_ids.get((EventTypes.Member, requester), None)
 
@@ -470,9 +503,11 @@ class SpaceSummaryHandler:
                     return True
 
         # If this is a request over federation, check if the host is in the room or
-        # is in one of the spaces specified via the join rules.
+        # has a user who could join the room.
         elif origin:
-            if await self._event_auth_handler.check_host_in_room(room_id, origin):
+            if await self._event_auth_handler.check_host_in_room(
+                room_id, origin
+            ) or await self._store.is_host_invited(room_id, origin):
                 return True
 
             # Alternately, if the host has a user in any of the spaces specified
@@ -490,18 +525,10 @@ class SpaceSummaryHandler:
                     ):
                         return True
 
-        # otherwise, check if the room is peekable
-        hist_vis_event_id = state_ids.get((EventTypes.RoomHistoryVisibility, ""), None)
-        if hist_vis_event_id:
-            hist_vis_ev = await self._store.get_event(hist_vis_event_id)
-            hist_vis = hist_vis_ev.content.get("history_visibility")
-            if hist_vis == HistoryVisibility.WORLD_READABLE:
-                return True
-
         logger.info(
-            "room %s is unpeekable and user %s is not a member / not allowed to join, omitting from summary",
+            "room %s is unpeekable and requester %s is not a member / not allowed to join, omitting from summary",
             room_id,
-            requester,
+            requester or origin,
         )
         return False
 
@@ -535,6 +562,7 @@ class SpaceSummaryHandler:
             "canonical_alias": stats["canonical_alias"],
             "num_joined_members": stats["joined_members"],
             "avatar_url": stats["avatar"],
+            "join_rules": stats["join_rules"],
             "world_readable": (
                 stats["history_visibility"] == HistoryVisibility.WORLD_READABLE
             ),

synapse/storage/databases/main/roommember.py

@@ -703,13 +703,22 @@ class RoomMemberWorkerStore(EventsWorkerStore):
 
     @cached(max_entries=10000)
     async def is_host_joined(self, room_id: str, host: str) -> bool:
+        return await self._check_host_room_membership(room_id, host, Membership.JOIN)
+
+    @cached(max_entries=10000)
+    async def is_host_invited(self, room_id: str, host: str) -> bool:
+        return await self._check_host_room_membership(room_id, host, Membership.INVITE)
+
+    async def _check_host_room_membership(
+        self, room_id: str, host: str, membership: str
+    ) -> bool:
         if "%" in host or "_" in host:
             raise Exception("Invalid host name")
 
         sql = """
             SELECT state_key FROM current_state_events AS c
             INNER JOIN room_memberships AS m USING (event_id)
-            WHERE m.membership = 'join'
+            WHERE m.membership = ?
                 AND type = 'm.room.member'
                 AND c.room_id = ?
                 AND state_key LIKE ?
@@ -722,7 +731,7 @@ class RoomMemberWorkerStore(EventsWorkerStore):
         like_clause = "%:" + host
 
         rows = await self.db_pool.execute(
-            "is_host_joined", None, sql, room_id, like_clause
+            "is_host_joined", None, sql, membership, room_id, like_clause
         )
 
         if not rows:

tests/handlers/test_space_summary.py

@@ -14,8 +14,18 @@
 from typing import Any, Iterable, Optional, Tuple
 from unittest import mock
 
-from synapse.api.constants import EventContentFields, JoinRules, RoomTypes
+from synapse.api.constants import (
+    EventContentFields,
+    EventTypes,
+    HistoryVisibility,
+    JoinRules,
+    Membership,
+    RestrictedJoinRuleTypes,
+    RoomTypes,
+)
 from synapse.api.errors import AuthError
+from synapse.api.room_versions import RoomVersions
+from synapse.events import make_event_from_dict
 from synapse.handlers.space_summary import _child_events_comparison_key
 from synapse.rest import admin
 from synapse.rest.client.v1 import login, room
@@ -117,7 +127,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         """Add a child room to a space."""
         self.helper.send_state(
             space_id,
-            event_type="m.space.child",
+            event_type=EventTypes.SpaceChild,
             body={"via": [self.hs.hostname]},
             tok=token,
             state_key=room_id,
@@ -155,29 +165,129 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         # The user cannot see the space.
         self.get_failure(self.handler.get_space_summary(user2, self.space), AuthError)
 
-        # Joining the room causes it to be visible.
+        # If the space is made world-readable it should return a result.
+        self.helper.send_state(
+            self.space,
+            event_type=EventTypes.RoomHistoryVisibility,
+            body={"history_visibility": HistoryVisibility.WORLD_READABLE},
+            tok=self.token,
+        )
+        result = self.get_success(self.handler.get_space_summary(user2, self.space))
+        self._assert_rooms(result, [self.space, self.room])
+        self._assert_events(result, [(self.space, self.room)])
+
+        # Make it not world-readable again and confirm it results in an error.
+        self.helper.send_state(
+            self.space,
+            event_type=EventTypes.RoomHistoryVisibility,
+            body={"history_visibility": HistoryVisibility.JOINED},
+            tok=self.token,
+        )
+        self.get_failure(self.handler.get_space_summary(user2, self.space), AuthError)
+
+        # Join the space and results should be returned.
+        self.helper.join(self.space, user2, tok=token2)
+        result = self.get_success(self.handler.get_space_summary(user2, self.space))
+        self._assert_rooms(result, [self.space, self.room])
+        self._assert_events(result, [(self.space, self.room)])
+
+    def _create_room_with_join_rule(
+        self, join_rule: str, room_version: Optional[str] = None, **extra_content
+    ) -> str:
+        """Create a room with the given join rule and add it to the space."""
+        room_id = self.helper.create_room_as(
+            self.user,
+            room_version=room_version,
+            tok=self.token,
+            extra_content={
+                "initial_state": [
+                    {
+                        "type": EventTypes.JoinRules,
+                        "state_key": "",
+                        "content": {
+                            "join_rule": join_rule,
+                            **extra_content,
+                        },
+                    }
+                ]
+            },
+        )
+        self._add_child(self.space, room_id, self.token)
+        return room_id
+
+    def test_filtering(self):
+        """
+        Rooms should be properly filtered to only include rooms the user has access to.
+        """
+        user2 = self.register_user("user2", "pass")
+        token2 = self.login("user2", "pass")
+
+        # Create a few rooms which will have different properties.
+        public_room = self._create_room_with_join_rule(JoinRules.PUBLIC)
+        knock_room = self._create_room_with_join_rule(
+            JoinRules.KNOCK, room_version=RoomVersions.V7.identifier
+        )
+        not_invited_room = self._create_room_with_join_rule(JoinRules.INVITE)
+        invited_room = self._create_room_with_join_rule(JoinRules.INVITE)
+        self.helper.invite(invited_room, targ=user2, tok=self.token)
+        restricted_room = self._create_room_with_join_rule(
+            JoinRules.MSC3083_RESTRICTED,
+            room_version=RoomVersions.MSC3083.identifier,
+            allow=[],
+        )
+        restricted_accessible_room = self._create_room_with_join_rule(
+            JoinRules.MSC3083_RESTRICTED,
+            room_version=RoomVersions.MSC3083.identifier,
+            allow=[
+                {
+                    "type": RestrictedJoinRuleTypes.ROOM_MEMBERSHIP,
+                    "room_id": self.space,
+                    "via": [self.hs.hostname],
+                }
+            ],
+        )
+        world_readable_room = self._create_room_with_join_rule(JoinRules.INVITE)
+        self.helper.send_state(
+            world_readable_room,
+            event_type=EventTypes.RoomHistoryVisibility,
+            body={"history_visibility": HistoryVisibility.WORLD_READABLE},
+            tok=self.token,
+        )
+        joined_room = self._create_room_with_join_rule(JoinRules.INVITE)
+        self.helper.invite(joined_room, targ=user2, tok=self.token)
+        self.helper.join(joined_room, user2, tok=token2)
+
+        # Join the space.
         self.helper.join(self.space, user2, tok=token2)
         result = self.get_success(self.handler.get_space_summary(user2, self.space))
 
-        # The result should only have the space, but includes the link to the room.
+        self._assert_rooms(
-        self._assert_rooms(result, [self.space])
+            result,
-        self._assert_events(result, [(self.space, self.room)])
+            [
-
-    def test_world_readable(self):
-        """A world-readable room is visible to everyone."""
-        self.helper.send_state(
                 self.space,
-            event_type="m.room.history_visibility",
+                self.room,
-            body={"history_visibility": "world_readable"},
+                public_room,
-            tok=self.token,
+                knock_room,
+                invited_room,
+                restricted_accessible_room,
+                world_readable_room,
+                joined_room,
+            ],
+        )
+        self._assert_events(
+            result,
+            [
+                (self.space, self.room),
+                (self.space, public_room),
+                (self.space, knock_room),
+                (self.space, not_invited_room),
+                (self.space, invited_room),
+                (self.space, restricted_room),
+                (self.space, restricted_accessible_room),
+                (self.space, world_readable_room),
+                (self.space, joined_room),
+            ],
         )
-
-        user2 = self.register_user("user2", "pass")
-
-        # The space should be visible, as well as the link to the room.
-        result = self.get_success(self.handler.get_space_summary(user2, self.space))
-        self._assert_rooms(result, [self.space])
-        self._assert_events(result, [(self.space, self.room)])
 
     def test_complex_space(self):
         """
@@ -186,7 +296,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         # Create an inaccessible room.
         user2 = self.register_user("user2", "pass")
         token2 = self.login("user2", "pass")
-        room2 = self.helper.create_room_as(user2, tok=token2)
+        room2 = self.helper.create_room_as(user2, is_public=False, tok=token2)
         # This is a bit odd as "user" is adding a room they don't know about, but
         # it works for the tests.
         self._add_child(self.space, room2, self.token)
@@ -292,16 +402,60 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         subspace = "#subspace:" + fed_hostname
 
         # Create a few rooms which will have different properties.
+        public_room = "#public:" + fed_hostname
+        knock_room = "#knock:" + fed_hostname
+        not_invited_room = "#not_invited:" + fed_hostname
+        invited_room = "#invited:" + fed_hostname
         restricted_room = "#restricted:" + fed_hostname
         restricted_accessible_room = "#restricted_accessible:" + fed_hostname
         world_readable_room = "#world_readable:" + fed_hostname
         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)
+        )
+
         async def summarize_remote_room(
             _self, room, suggested_only, max_children, exclude_rooms
         ):
             # Note that these entries are brief, but should contain enough info.
             rooms = [
+                {
+                    "room_id": public_room,
+                    "world_readable": False,
+                    "join_rules": JoinRules.PUBLIC,
+                },
+                {
+                    "room_id": knock_room,
+                    "world_readable": False,
+                    "join_rules": JoinRules.KNOCK,
+                },
+                {
+                    "room_id": not_invited_room,
+                    "world_readable": False,
+                    "join_rules": JoinRules.INVITE,
+                },
+                {
+                    "room_id": invited_room,
+                    "world_readable": False,
+                    "join_rules": JoinRules.INVITE,
+                },
                 {
                     "room_id": restricted_room,
                     "world_readable": False,
@@ -364,6 +518,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
                 self.space,
                 self.room,
                 subspace,
+                public_room,
+                knock_room,
+                invited_room,
                 restricted_accessible_room,
                 world_readable_room,
                 joined_room,
@@ -374,6 +531,10 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             [
                 (self.space, self.room),
                 (self.space, subspace),
+                (subspace, public_room),
+                (subspace, knock_room),
+                (subspace, not_invited_room),
+                (subspace, invited_room),
                 (subspace, restricted_room),
                 (subspace, restricted_accessible_room),
                 (subspace, world_readable_room),