From 9752849e2b41968613ca244e86311d805bdd27df Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 17 May 2021 09:01:19 -0400 Subject: Clarify comments in the space summary handler. (#9974) --- synapse/handlers/space_summary.py | 51 +++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 5 deletions(-) (limited to 'synapse/handlers/space_summary.py') diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index e35d91832b..953356f34d 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -32,7 +32,6 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) # number of rooms to return. We'll stop once we hit this limit. -# TODO: allow clients to reduce this with a request param. MAX_ROOMS = 50 # max number of events to return per room. @@ -231,11 +230,15 @@ class SpaceSummaryHandler: Generate a room entry and a list of event entries for a given room. Args: - requester: The requesting user, or None if this is over federation. + requester: + The user requesting the summary, if it is a local request. None + if this is a federation request. room_id: The room ID to summarize. suggested_only: True if only suggested children should be returned. Otherwise, all children are returned. - max_children: The maximum number of children to return for this node. + max_children: + The maximum number of children rooms to include. This is capped + to a server-set limit. Returns: A tuple of: @@ -278,6 +281,26 @@ class SpaceSummaryHandler: max_children: Optional[int], exclude_rooms: Iterable[str], ) -> Tuple[Sequence[JsonDict], Sequence[JsonDict]]: + """ + Request room entries and a list of event entries for a given room by querying a remote server. + + Args: + room: The room to summarize. + suggested_only: True if only suggested children should be returned. + Otherwise, all children are returned. + max_children: + The maximum number of children rooms to include. This is capped + to a server-set limit. + exclude_rooms: + Rooms IDs which do not need to be summarized. + + Returns: + A tuple of: + An iterable of rooms. + + An iterable of the sorted children events. This may be limited + to a maximum size or may include all children. + """ room_id = room.room_id logger.info("Requesting summary for %s via %s", room_id, room.via) @@ -310,8 +333,26 @@ class SpaceSummaryHandler: ) async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> bool: - # if we have an authenticated requesting user, first check if they are in the - # room + """ + Calculate whether the room should be shown in the spaces summary. + + It should be included if: + + * The requester is joined or invited to the room. + * The history visibility is set to world readable. + + Args: + room_id: The room ID to summarize. + requester: + The user requesting the summary, if it is a local request. None + if this is a federation request. + + Returns: + True if the room should be included in the spaces summary. + """ + + # if we have an authenticated requesting user, first check if they are able to view + # stripped state in the room. if requester: try: await self._auth.check_user_in_room(room_id, requester) -- cgit 1.5.1 From 206a7b5f12fd3d88ec24a1f53ce75e5b701faed8 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 17 May 2021 09:59:17 -0400 Subject: Fix the allowed range of valid ordering characters for spaces. (#10002) \x7F was meant to be \0x7E (~) this was originally incorrect in MSC1772. --- changelog.d/10002.bugfix | 1 + synapse/handlers/space_summary.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) create mode 100644 changelog.d/10002.bugfix (limited to 'synapse/handlers/space_summary.py') diff --git a/changelog.d/10002.bugfix b/changelog.d/10002.bugfix new file mode 100644 index 0000000000..1fabdad22e --- /dev/null +++ b/changelog.d/10002.bugfix @@ -0,0 +1 @@ +Fix a validation bug introduced in v1.34.0 in the ordering of spaces in the space summary API. diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 953356f34d..eb80a5ad67 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -471,8 +471,8 @@ def _is_suggested_child_event(edge_event: EventBase) -> bool: return False -# Order may only contain characters in the range of \x20 (space) to \x7F (~). -_INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7F]") +# Order may only contain characters in the range of \x20 (space) to \x7E (~) inclusive. +_INVALID_ORDER_CHARS_RE = re.compile(r"[^\x20-\x7E]") def _child_events_comparison_key(child: EventBase) -> Tuple[bool, Optional[str], str]: -- cgit 1.5.1 From 551d2c3f4b492d59b3c670c1a8e82869b16a594d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 May 2021 11:10:36 -0400 Subject: Allow a user who could join a restricted room to see it in spaces summary. (#9922) This finishes up the experimental implementation of MSC3083 by showing the restricted rooms in the spaces summary (from MSC2946). --- changelog.d/9922.feature | 1 + synapse/federation/transport/server.py | 2 +- synapse/handlers/event_auth.py | 104 +++++++++++++---- synapse/handlers/space_summary.py | 201 +++++++++++++++++++++++++++------ 4 files changed, 254 insertions(+), 54 deletions(-) create mode 100644 changelog.d/9922.feature (limited to 'synapse/handlers/space_summary.py') diff --git a/changelog.d/9922.feature b/changelog.d/9922.feature new file mode 100644 index 0000000000..2c655350c0 --- /dev/null +++ b/changelog.d/9922.feature @@ -0,0 +1 @@ +Experimental support to allow a user who could join a restricted room to view it in the spaces summary. diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index e1b7462474..c17a085a4f 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -1428,7 +1428,7 @@ class FederationSpaceSummaryServlet(BaseFederationServlet): ) return 200, await self.handler.federation_space_summary( - room_id, suggested_only, max_rooms_per_space, exclude_rooms + origin, room_id, suggested_only, max_rooms_per_space, exclude_rooms ) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index 5b2fe103e7..a0df16a32f 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -11,7 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, Collection, Optional from synapse.api.constants import EventTypes, JoinRules, Membership from synapse.api.errors import AuthError @@ -59,32 +59,76 @@ class EventAuthHandler: ): return + # This is not a room with a restricted join rule, so we don't need to do the + # restricted room specific checks. + # + # Note: We'll be applying the standard join rule checks later, which will + # catch the cases of e.g. trying to join private rooms without an invite. + if not await self.has_restricted_join_rules(state_ids, room_version): + return + + # Get the spaces which allow access to this room and check if the user is + # in any of them. + allowed_spaces = await self.get_spaces_that_allow_join(state_ids) + if not await self.is_user_in_rooms(allowed_spaces, user_id): + raise AuthError( + 403, + "You do not belong to any of the required spaces to join this room.", + ) + + async def has_restricted_join_rules( + self, state_ids: StateMap[str], room_version: RoomVersion + ) -> bool: + """ + Return if the room has the proper join rules set for access via spaces. + + Args: + state_ids: The state of the room as it currently is. + room_version: The room version of the room to query. + + Returns: + True if the proper room version and join rules are set for restricted access. + """ # This only applies to room versions which support the new join rule. if not room_version.msc3083_join_rules: - return + return False # If there's no join rule, then it defaults to invite (so this doesn't apply). join_rules_event_id = state_ids.get((EventTypes.JoinRules, ""), None) if not join_rules_event_id: - return + return False + + # If the join rule is not restricted, this doesn't apply. + join_rules_event = await self._store.get_event(join_rules_event_id) + return join_rules_event.content.get("join_rule") == JoinRules.MSC3083_RESTRICTED + + async def get_spaces_that_allow_join( + self, state_ids: StateMap[str] + ) -> Collection[str]: + """ + Generate a list of spaces which allow access to a room. + + Args: + state_ids: The state of the room as it currently is. + + Returns: + A collection of spaces which provide membership to the room. + """ + # If there's no join rule, then it defaults to invite (so this doesn't apply). + join_rules_event_id = state_ids.get((EventTypes.JoinRules, ""), None) + if not join_rules_event_id: + return () # If the join rule is not restricted, this doesn't apply. join_rules_event = await self._store.get_event(join_rules_event_id) - if join_rules_event.content.get("join_rule") != JoinRules.MSC3083_RESTRICTED: - return # If allowed is of the wrong form, then only allow invited users. allowed_spaces = join_rules_event.content.get("allow", []) if not isinstance(allowed_spaces, list): - allowed_spaces = () - - # Get the list of joined rooms and see if there's an overlap. - if allowed_spaces: - joined_rooms = await self._store.get_rooms_for_user(user_id) - else: - joined_rooms = () + return () # Pull out the other room IDs, invalid data gets filtered. + result = [] for space in allowed_spaces: if not isinstance(space, dict): continue @@ -93,13 +137,31 @@ class EventAuthHandler: if not isinstance(space_id, str): continue - # The user was joined to one of the spaces specified, they can join - # this room! - if space_id in joined_rooms: - return + result.append(space_id) + + return result + + async def is_user_in_rooms(self, room_ids: Collection[str], user_id: str) -> bool: + """ + Check whether a user is a member of any of the provided rooms. + + Args: + room_ids: The rooms to check for membership. + user_id: The user to check. + + Returns: + True if the user is in any of the rooms, false otherwise. + """ + if not room_ids: + return False + + # Get the list of joined rooms and see if there's an overlap. + joined_rooms = await self._store.get_rooms_for_user(user_id) + + # Check each room and see if the user is in it. + for room_id in room_ids: + if room_id in joined_rooms: + return True - # The user was not in any of the required spaces. - raise AuthError( - 403, - "You do not belong to any of the required spaces to join this room.", - ) + # The user was not in any of the rooms. + return False diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index eb80a5ad67..8d49ba8164 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -16,11 +16,16 @@ import itertools import logging import re from collections import deque -from typing import TYPE_CHECKING, Iterable, List, Optional, Sequence, Set, Tuple, cast +from typing import TYPE_CHECKING, Iterable, List, Optional, Sequence, Set, Tuple import attr -from synapse.api.constants import EventContentFields, EventTypes, HistoryVisibility +from synapse.api.constants import ( + EventContentFields, + EventTypes, + HistoryVisibility, + Membership, +) from synapse.api.errors import AuthError from synapse.events import EventBase from synapse.events.utils import format_event_for_client_v2 @@ -47,6 +52,7 @@ class SpaceSummaryHandler: self._auth = hs.get_auth() self._room_list_handler = hs.get_room_list_handler() self._state_handler = hs.get_state_handler() + self._event_auth_handler = hs.get_event_auth_handler() self._store = hs.get_datastore() self._event_serializer = hs.get_event_client_serializer() self._server_name = hs.hostname @@ -111,28 +117,88 @@ class SpaceSummaryHandler: max_children = max_rooms_per_space if processed_rooms else None if is_in_room: - rooms, events = await self._summarize_local_room( - requester, room_id, suggested_only, max_children + room, events = await self._summarize_local_room( + requester, None, room_id, suggested_only, max_children ) + + logger.debug( + "Query of local room %s returned events %s", + room_id, + ["%s->%s" % (ev["room_id"], ev["state_key"]) for ev in events], + ) + + if room: + rooms_result.append(room) else: - rooms, events = await self._summarize_remote_room( + fed_rooms, fed_events = await self._summarize_remote_room( queue_entry, suggested_only, max_children, exclude_rooms=processed_rooms, ) - logger.debug( - "Query of %s returned rooms %s, events %s", - queue_entry.room_id, - [room.get("room_id") for room in rooms], - ["%s->%s" % (ev["room_id"], ev["state_key"]) for ev in events], - ) - - rooms_result.extend(rooms) - - # any rooms returned don't need visiting again - processed_rooms.update(cast(str, room.get("room_id")) for room in rooms) + # The results over federation might include rooms that the we, + # as the requesting server, are allowed to see, but the requesting + # user is not permitted see. + # + # Filter the returned results to only what is accessible to the user. + room_ids = set() + events = [] + for room in fed_rooms: + fed_room_id = room.get("room_id") + if not fed_room_id or not isinstance(fed_room_id, str): + continue + + # 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 + # 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 + + # Check if the user is a member of any of the allowed spaces + # from the response. + allowed_spaces = room.get("allowed_spaces") + if ( + not include_room + and allowed_spaces + and isinstance(allowed_spaces, list) + ): + include_room = await self._event_auth_handler.is_user_in_rooms( + allowed_spaces, 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: + rooms_result.append(room) + room_ids.add(fed_room_id) + + # All rooms returned don't need visiting again (even if the user + # didn't have access to them). + processed_rooms.add(fed_room_id) + + for event in fed_events: + if event.get("room_id") in room_ids: + events.append(event) + + logger.debug( + "Query of %s returned rooms %s, events %s", + room_id, + [room.get("room_id") for room in fed_rooms], + ["%s->%s" % (ev["room_id"], ev["state_key"]) for ev in fed_events], + ) # the room we queried may or may not have been returned, but don't process # it again, anyway. @@ -158,10 +224,16 @@ class SpaceSummaryHandler: ) processed_events.add(ev_key) + # Before returning to the client, remove the allowed_spaces key for any + # rooms. + for room in rooms_result: + room.pop("allowed_spaces", None) + return {"rooms": rooms_result, "events": events_result} async def federation_space_summary( self, + origin: str, room_id: str, suggested_only: bool, max_rooms_per_space: Optional[int], @@ -171,6 +243,8 @@ class SpaceSummaryHandler: Implementation of the space summary Federation API Args: + origin: The server requesting the spaces summary. + room_id: room id to start the summary at suggested_only: whether we should only return children with the "suggested" @@ -205,14 +279,15 @@ class SpaceSummaryHandler: logger.debug("Processing room %s", room_id) - rooms, events = await self._summarize_local_room( - None, room_id, suggested_only, max_rooms_per_space + room, events = await self._summarize_local_room( + None, origin, room_id, suggested_only, max_rooms_per_space ) processed_rooms.add(room_id) - rooms_result.extend(rooms) - events_result.extend(events) + if room: + rooms_result.append(room) + events_result.extend(events) # add any children to the queue room_queue.extend(edge_event["state_key"] for edge_event in events) @@ -222,10 +297,11 @@ class SpaceSummaryHandler: async def _summarize_local_room( self, requester: Optional[str], + origin: Optional[str], room_id: str, suggested_only: bool, max_children: Optional[int], - ) -> Tuple[Sequence[JsonDict], Sequence[JsonDict]]: + ) -> Tuple[Optional[JsonDict], Sequence[JsonDict]]: """ Generate a room entry and a list of event entries for a given room. @@ -233,6 +309,9 @@ class SpaceSummaryHandler: requester: The user requesting the summary, if it is a local request. None if this is a federation request. + origin: + The server requesting the summary, if it is a federation request. + None if this is a local request. room_id: The room ID to summarize. suggested_only: True if only suggested children should be returned. Otherwise, all children are returned. @@ -247,8 +326,8 @@ class SpaceSummaryHandler: An iterable of the sorted children events. This may be limited to a maximum size or may include all children. """ - if not await self._is_room_accessible(room_id, requester): - return (), () + if not await self._is_room_accessible(room_id, requester, origin): + return None, () room_entry = await self._build_room_entry(room_id) @@ -272,7 +351,7 @@ class SpaceSummaryHandler: event_format=format_event_for_client_v2, ) ) - return (room_entry,), events_result + return room_entry, events_result async def _summarize_remote_room( self, @@ -332,13 +411,17 @@ class SpaceSummaryHandler: or ev.event_type == EventTypes.SpaceChild ) - async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> bool: + async def _is_room_accessible( + self, room_id: str, requester: Optional[str], origin: Optional[str] + ) -> bool: """ Calculate whether the room should be shown in the spaces summary. It should be included if: * The requester is joined or invited to the room. + * The requester can join without an invite (per MSC3083). + * The origin server has any user that is joined or invited to the room. * The history visibility is set to world readable. Args: @@ -346,31 +429,75 @@ class SpaceSummaryHandler: requester: The user requesting the summary, if it is a local request. None if this is a federation request. + origin: + The server requesting the summary, if it is a federation request. + None if this is a local request. Returns: True if the room should be included in the spaces summary. """ + state_ids = await self._store.get_current_state_ids(room_id) + + # If there's no state for the room, it isn't known. + if not state_ids: + 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 # stripped state in the room. if requester: + member_event_id = state_ids.get((EventTypes.Member, requester), None) + + # If they're in the room they can see info on it. + member_event = None + if member_event_id: + member_event = await self._store.get_event(member_event_id) + if member_event.membership in (Membership.JOIN, Membership.INVITE): + return True + + # Otherwise, check if they should be allowed access via membership in a space. try: - await self._auth.check_user_in_room(room_id, requester) - return True + await self._event_auth_handler.check_restricted_join_rules( + state_ids, room_version, requester, member_event + ) except AuthError: + # The user doesn't have access due to spaces, but might have access + # another way. Keep trying. pass + else: + 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. + elif origin: + if await self._auth.check_host_in_room(room_id, origin): + return True + + # Alternately, if the host has a user in any of the spaces specified + # for access, then the host can see this room (and should do filtering + # if the requester cannot see it). + if await self._event_auth_handler.has_restricted_join_rules( + state_ids, room_version + ): + allowed_spaces = ( + await self._event_auth_handler.get_spaces_that_allow_join(state_ids) + ) + for space_id in allowed_spaces: + if await self._auth.check_host_in_room(space_id, origin): + return True # otherwise, check if the room is peekable - hist_vis_ev = await self._state_handler.get_current_state( - room_id, EventTypes.RoomHistoryVisibility, "" - ) - if hist_vis_ev: + 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, omitting from summary", + "room %s is unpeekable and user %s is not a member / not allowed to join, omitting from summary", room_id, requester, ) @@ -395,6 +522,15 @@ class SpaceSummaryHandler: if not room_type: room_type = create_event.content.get(EventContentFields.MSC1772_ROOM_TYPE) + room_version = await self._store.get_room_version(room_id) + allowed_spaces = None + if await self._event_auth_handler.has_restricted_join_rules( + current_state_ids, room_version + ): + allowed_spaces = await self._event_auth_handler.get_spaces_that_allow_join( + current_state_ids + ) + entry = { "room_id": stats["room_id"], "name": stats["name"], @@ -408,6 +544,7 @@ class SpaceSummaryHandler: "guest_can_join": stats["guest_access"] == "can_join", "creation_ts": create_event.origin_server_ts, "room_type": room_type, + "allowed_spaces": allowed_spaces, } # Filter out Nones – rather omit the field altogether -- cgit 1.5.1 From c5413d0e9ef6afa9cb5140774acfb4954fe0bf37 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 21 May 2021 12:02:01 -0400 Subject: Remove unused properties from the SpaceSummaryHandler. (#10038) --- changelog.d/10038.feature | 1 + synapse/handlers/space_summary.py | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) create mode 100644 changelog.d/10038.feature (limited to 'synapse/handlers/space_summary.py') diff --git a/changelog.d/10038.feature b/changelog.d/10038.feature new file mode 100644 index 0000000000..2c655350c0 --- /dev/null +++ b/changelog.d/10038.feature @@ -0,0 +1 @@ +Experimental support to allow a user who could join a restricted room to view it in the spaces summary. diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 8d49ba8164..abd9ddecca 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -50,8 +50,6 @@ class SpaceSummaryHandler: def __init__(self, hs: "HomeServer"): self._clock = hs.get_clock() self._auth = hs.get_auth() - self._room_list_handler = hs.get_room_list_handler() - self._state_handler = hs.get_state_handler() self._event_auth_handler = hs.get_event_auth_handler() self._store = hs.get_datastore() self._event_serializer = hs.get_event_client_serializer() -- cgit 1.5.1