From bce96c2ff9d0f5fbf13f1d1c56399d8ec066bd20 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 23 Apr 2021 10:22:51 -0400 Subject: [PATCH 01/17] Allow a user who could join a restricted room to see it in spaces summary. Allows discovery of restricted rooms via the intersection of MSC2946 and MSC3083. --- changelog.d/9922.feature | 1 + synapse/handlers/space_summary.py | 37 ++++++++++++++++++++++--------- 2 files changed, 27 insertions(+), 11 deletions(-) create mode 100644 changelog.d/9922.feature diff --git a/changelog.d/9922.feature b/changelog.d/9922.feature new file mode 100644 index 000000000000..2c655350c04f --- /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/handlers/space_summary.py b/synapse/handlers/space_summary.py index eb80a5ad67db..fb422a9d399c 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -20,8 +20,12 @@ import attr -from synapse.api.constants import EventContentFields, EventTypes, HistoryVisibility -from synapse.api.errors import AuthError +from synapse.api.constants import ( + EventContentFields, + EventTypes, + HistoryVisibility, + Membership, +) from synapse.events import EventBase from synapse.events.utils import format_event_for_client_v2 from synapse.types import JsonDict @@ -47,6 +51,7 @@ def __init__(self, hs: "HomeServer"): 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 @@ -339,6 +344,7 @@ async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> b It should be included if: * The requester is joined or invited to the room. + * The requester can join without an invite (per MSC3083). * The history visibility is set to world readable. Args: @@ -350,27 +356,36 @@ async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> b Returns: True if the room should be included in the spaces summary. """ + state_ids = await self._store.get_current_state_ids(room_id) # 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) + member_event_id = state_ids.get((EventTypes.Member, requester), None) + + # If they're in the room they can see info on it. + 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, if they should be allowed access via membership in a space. + room_version = await self._store.get_room_version(room_id) + if await self._event_auth_handler.can_join_without_invite( + state_ids, room_version, requester + ): return True - except AuthError: - pass # 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, ) From 0a881ac8ea8362576a0129dae04a3adbe9bde644 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 28 Apr 2021 09:30:30 -0400 Subject: [PATCH 02/17] Check if the origin server can access a room when queried over federation. --- synapse/federation/transport/server.py | 2 +- synapse/handlers/space_summary.py | 25 +++++++++++++++++++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index e1b746247469..c17a085a4f93 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -1428,7 +1428,7 @@ async def on_POST( ) 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/space_summary.py b/synapse/handlers/space_summary.py index fb422a9d399c..0aa611f38565 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -117,7 +117,7 @@ async def get_space_summary( if is_in_room: rooms, events = await self._summarize_local_room( - requester, room_id, suggested_only, max_children + requester, None, room_id, suggested_only, max_children ) else: rooms, events = await self._summarize_remote_room( @@ -167,6 +167,7 @@ async def get_space_summary( async def federation_space_summary( self, + origin: str, room_id: str, suggested_only: bool, max_rooms_per_space: Optional[int], @@ -176,6 +177,8 @@ async def federation_space_summary( 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" @@ -211,7 +214,7 @@ async def federation_space_summary( logger.debug("Processing room %s", room_id) rooms, events = await self._summarize_local_room( - None, room_id, suggested_only, max_rooms_per_space + None, origin, room_id, suggested_only, max_rooms_per_space ) processed_rooms.add(room_id) @@ -227,6 +230,7 @@ async def federation_space_summary( async def _summarize_local_room( self, requester: Optional[str], + origin: Optional[str], room_id: str, suggested_only: bool, max_children: Optional[int], @@ -238,6 +242,9 @@ async def _summarize_local_room( 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. @@ -252,7 +259,7 @@ async def _summarize_local_room( 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): + if not await self._is_room_accessible(room_id, requester, origin): return (), () room_entry = await self._build_room_entry(room_id) @@ -337,7 +344,9 @@ async def _summarize_remote_room( 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. @@ -345,6 +354,7 @@ async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> b * 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: @@ -352,6 +362,9 @@ async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> b 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. @@ -376,6 +389,10 @@ async def _is_room_accessible(self, room_id: str, requester: Optional[str]) -> b ): return True + # If this is a request over federation, check if the host is in the room. + elif origin and await self._auth.check_host_in_room(room_id, origin): + return True + # otherwise, check if the room is peekable hist_vis_event_id = state_ids.get((EventTypes.RoomHistoryVisibility, ""), None) if hist_vis_event_id: From 45ad823d637688c847a94f71ae8ac87f154401be Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 28 Apr 2021 09:32:01 -0400 Subject: [PATCH 03/17] Check if an origin server has access to any spaces for a restricted room when queried over federation. --- synapse/handlers/event_auth.py | 67 ++++++++++++++++++++++--------- synapse/handlers/space_summary.py | 44 +++++++++++++++++--- 2 files changed, 88 insertions(+), 23 deletions(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index eff639f40760..ad9099c9de76 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 +from typing import TYPE_CHECKING, Collection, Tuple from synapse.api.constants import EventTypes, JoinRules from synapse.api.room_versions import RoomVersion @@ -29,46 +29,42 @@ class EventAuthHandler: def __init__(self, hs: "HomeServer"): self._store = hs.get_datastore() - async def can_join_without_invite( - self, state_ids: StateMap[str], room_version: RoomVersion, user_id: str - ) -> bool: + async def get_allowed_spaces( + self, state_ids: StateMap[str], room_version: RoomVersion + ) -> Tuple[bool, Collection[str]]: """ - Check whether a user can join a room without an invite. - - When joining a room with restricted joined rules (as defined in MSC3083), - the membership of spaces must be checked during join. + Generate a list of spaces allow access to a room. Args: state_ids: The state of the room as it currently is. room_version: The room version of the room being joined. - user_id: The user joining the room. Returns: - True if the user can join the room, false otherwise. + A tuple: + True if spaces give access to the room. + A collection of spaces (if any) which provide membership to the room. """ # This only applies to room versions which support the new join rule. if not room_version.msc3083_join_rules: - return True + 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 True + 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) if join_rules_event.content.get("join_rule") != JoinRules.MSC3083_RESTRICTED: - return True + return False, () # 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): - 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) + return True, () - # Pull out the other room IDs, invalid data gets filtered. + # Pull out the room IDs, invalid data gets filtered. + result = [] for space in allowed_spaces: if not isinstance(space, dict): continue @@ -79,6 +75,41 @@ async def can_join_without_invite( # The user was joined to one of the spaces specified, they can join # this room! + result.append(space_id) + + return True, result + + async def can_join_without_invite( + self, state_ids: StateMap[str], room_version: RoomVersion, user_id: str + ) -> bool: + """ + Check whether a user can join a room without an invite. + + When joining a room with restricted joined rules (as defined in MSC3083), + the membership of spaces must be checked during join. + + Args: + state_ids: The state of the room as it currently is. + room_version: The room version of the room being joined. + user_id: The user joining the room. + + Returns: + True if the user can join the room, false otherwise. + """ + # Get whether spaces allow access to the room and the allowed spaces. + allow_via_spaces, allowed_spaces = await self.get_allowed_spaces( + state_ids, room_version + ) + # Access via spaces doesn't apply to this room. + if not allow_via_spaces: + return True + + # Get the list of joined rooms and see if there's an overlap. + joined_rooms = await self._store.get_rooms_for_user(user_id) + + # If the user was joined to one of the spaces specified, they can join + # this room! + for space_id in allowed_spaces: if space_id in joined_rooms: return True diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 0aa611f38565..680466e92bd5 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -120,13 +120,31 @@ async def get_space_summary( requester, None, room_id, suggested_only, max_children ) 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, ) + # 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. Handle that filtering. + room_ids = set() + rooms = [] + events = [] + for room in fed_rooms: + fed_room_id = room.get("room_id") + if fed_room_id and await self._is_room_accessible( + fed_room_id, requester, None + ): + rooms.append(room) + room_ids.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", queue_entry.room_id, @@ -370,6 +388,7 @@ async def _is_room_accessible( True if the room should be included in the spaces summary. """ state_ids = await self._store.get_current_state_ids(room_id) + 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. @@ -383,15 +402,30 @@ async def _is_room_accessible( return True # Otherwise, if they should be allowed access via membership in a space. - room_version = await self._store.get_room_version(room_id) if await self._event_auth_handler.can_join_without_invite( state_ids, room_version, requester ): return True - # If this is a request over federation, check if the host is in the room. - elif origin and await self._auth.check_host_in_room(room_id, origin): - 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). + ( + allow_via_spaces, + allowed_spaces, + ) = await self._event_auth_handler.get_allowed_spaces( + state_ids, room_version + ) + if allow_via_spaces: + 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_event_id = state_ids.get((EventTypes.RoomHistoryVisibility, ""), None) From 5225ef125a2b8de06ce732a5b08173195fa3f55c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 28 Apr 2021 09:59:48 -0400 Subject: [PATCH 04/17] Do not revisit any of the rooms received over federation. --- synapse/handlers/space_summary.py | 49 ++++++++++++++++++------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 680466e92bd5..aff39c7a888f 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -116,9 +116,18 @@ async def get_space_summary( max_children = max_rooms_per_space if processed_rooms else None if is_in_room: - rooms, events = await self._summarize_local_room( + 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: fed_rooms, fed_events = await self._summarize_remote_room( queue_entry, @@ -131,31 +140,30 @@ async def get_space_summary( # as the requesting server, are allowed to see, but the requesting # user is not permitted see. Handle that filtering. room_ids = set() - rooms = [] + rooms = [] # type: List[JsonDict] events = [] for room in fed_rooms: fed_room_id = room.get("room_id") if fed_room_id and await self._is_room_accessible( fed_room_id, requester, None ): - rooms.append(room) + rooms_result.append(room) room_ids.add(fed_room_id) + # any rooms returned don't need visiting again (even if the user + # didn't have access to them). + processed_rooms.add(cast(str, 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", - 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) + logger.debug( + "Query of %s returned rooms %s, events %s", + room_id, + [room.get("room_id") for room in rooms], + ["%s->%s" % (ev["room_id"], ev["state_key"]) for ev in events], + ) # the room we queried may or may not have been returned, but don't process # it again, anyway. @@ -231,14 +239,15 @@ async def federation_space_summary( logger.debug("Processing room %s", room_id) - rooms, events = await self._summarize_local_room( + 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) @@ -252,7 +261,7 @@ async def _summarize_local_room( 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. @@ -278,7 +287,7 @@ async def _summarize_local_room( to a maximum size or may include all children. """ if not await self._is_room_accessible(room_id, requester, origin): - return (), () + return None, () room_entry = await self._build_room_entry(room_id) @@ -302,7 +311,7 @@ async def _summarize_local_room( event_format=format_event_for_client_v2, ) ) - return (room_entry,), events_result + return room_entry, events_result async def _summarize_remote_room( self, From 31bb2ad69cc252fc8faa7104104f72be01766f35 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 3 May 2021 12:09:22 -0400 Subject: [PATCH 05/17] Fix filtering for world readable rooms. --- synapse/handlers/space_summary.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index aff39c7a888f..eab72a0780aa 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -138,15 +138,25 @@ async def get_space_summary( # 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. Handle that filtering. + # user is not permitted see. + # + # A room may be visible if it is world readable, or if it is accessible + # to the specific user. room_ids = set() rooms = [] # type: List[JsonDict] events = [] for room in fed_rooms: + # Pull whether it is world readable from the returned information + # since we may not have the state of this room. + include_room = room.get("world_readable") is True fed_room_id = room.get("room_id") - if fed_room_id and await self._is_room_accessible( - fed_room_id, requester, None - ): + + if not include_room and fed_room_id: + include_room = await self._is_room_accessible( + fed_room_id, requester, None + ) + + if include_room: rooms_result.append(room) room_ids.add(fed_room_id) @@ -397,6 +407,12 @@ async def _is_room_accessible( 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 From 417aa6cddb853d172c92bd1404b24a7df74e8e0d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 12 May 2021 15:34:26 -0400 Subject: [PATCH 06/17] Abstract out a helper for checking if a space is in a list. --- synapse/handlers/event_auth.py | 37 +++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index ad9099c9de76..f01bb6bd7ebe 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -37,7 +37,7 @@ async def get_allowed_spaces( Args: state_ids: The state of the room as it currently is. - room_version: The room version of the room being joined. + room_version: The room version of the room to query. Returns: A tuple: @@ -79,6 +79,31 @@ async def get_allowed_spaces( return True, result + async def 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 rooms. + return False + async def can_join_without_invite( self, state_ids: StateMap[str], room_version: RoomVersion, user_id: str ) -> bool: @@ -104,14 +129,6 @@ async def can_join_without_invite( if not allow_via_spaces: return True - # Get the list of joined rooms and see if there's an overlap. - joined_rooms = await self._store.get_rooms_for_user(user_id) - # If the user was joined to one of the spaces specified, they can join # this room! - for space_id in allowed_spaces: - if space_id in joined_rooms: - return True - - # The user was not in any of the required spaces. - return False + return await self.user_in_rooms(allowed_spaces, user_id) From 36d3932d16ebbde7eb25cc5d8a92e91106e9d246 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 12 May 2021 15:34:34 -0400 Subject: [PATCH 07/17] Return the allowed spaces over federation and filter on it. --- synapse/handlers/space_summary.py | 34 +++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index eab72a0780aa..9bd579ef5f4d 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -146,12 +146,24 @@ async def get_space_summary( rooms = [] # type: List[JsonDict] events = [] for room in fed_rooms: - # Pull whether it is world readable from the returned information - # since we may not have the state of this room. - include_room = room.get("world_readable") is True fed_room_id = room.get("room_id") + if not fed_room_id: + continue + + # Check whether the room is world readable from the response. + include_room = room.get("world_readable") is True + + # If the room is not world-readable, get the allowed spaces from + # the response. + allowed_spaces = room.get("allowed_spaces") + if not include_room and allowed_spaces: + include_room = await self._event_auth_handler.user_in_rooms( + allowed_spaces, requester + ) - if not include_room and fed_room_id: + # Finally, check ourselves if we can access the room (this will + # fail if we don't have any state for this room). + if not include_room: include_room = await self._is_room_accessible( fed_room_id, requester, None ) @@ -199,6 +211,11 @@ async def get_space_summary( ) 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( @@ -486,6 +503,14 @@ async def _build_room_entry(self, room_id: str) -> JsonDict: if not room_type: room_type = create_event.content.get(EventContentFields.MSC1772_ROOM_TYPE) + room_version = await self._store.get_room_version(room_id) + ( + allow_via_spaces, + allowed_spaces, + ) = await self._event_auth_handler.get_allowed_spaces( + current_state_ids, room_version + ) + entry = { "room_id": stats["room_id"], "name": stats["name"], @@ -499,6 +524,7 @@ async def _build_room_entry(self, room_id: str) -> JsonDict: "guest_can_join": stats["guest_access"] == "can_join", "creation_ts": create_event.origin_server_ts, "room_type": room_type, + "allowed_spaces": allowed_spaces if allow_via_spaces else None, } # Filter out Nones – rather omit the field altogether From 9157200a76b1ae14268b6b4d295acb9bf0bf1d53 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 17 May 2021 13:24:49 -0400 Subject: [PATCH 08/17] Remove unused variable. --- synapse/handlers/space_summary.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 9bd579ef5f4d..8bc8ccb6e5cc 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -143,7 +143,6 @@ async def get_space_summary( # A room may be visible if it is world readable, or if it is accessible # to the specific user. room_ids = set() - rooms = [] # type: List[JsonDict] events = [] for room in fed_rooms: fed_room_id = room.get("room_id") @@ -183,8 +182,8 @@ async def get_space_summary( logger.debug( "Query of %s returned rooms %s, events %s", room_id, - [room.get("room_id") for room in rooms], - ["%s->%s" % (ev["room_id"], ev["state_key"]) for ev in events], + [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 From c97917b0cee1db80ee5adc69a7413f3057ae8e32 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 17 May 2021 14:08:01 -0400 Subject: [PATCH 09/17] Fix typo. Co-authored-by: Sumner Evans --- synapse/handlers/event_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index f01bb6bd7ebe..f07c5e7d08d0 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -33,7 +33,7 @@ async def get_allowed_spaces( self, state_ids: StateMap[str], room_version: RoomVersion ) -> Tuple[bool, Collection[str]]: """ - Generate a list of spaces allow access to a room. + Generate a list of spaces which allow access to a room. Args: state_ids: The state of the room as it currently is. From ce8efa95892c5d615f55080d013d2d4e358830f0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 18 May 2021 11:12:53 -0400 Subject: [PATCH 10/17] Split the method in two. --- synapse/handlers/event_auth.py | 68 ++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index 5b2fe103e742..eea6f13b1d0e 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,67 @@ async def check_restricted_join_rules( ): return + # Get the spaces which allow access to this room, if applicable. + allowed_spaces = await self.get_spaces_that_allow_join(state_ids, room_version) + if allowed_spaces is None: + return + + # 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 = () + + # Pull out the other room IDs, invalid data gets filtered. + for space_id in allowed_spaces: + # The user was joined to one of the spaces specified, they can join + # this room! + if space_id in joined_rooms: + return + + # 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.", + ) + + async def get_spaces_that_allow_join( + self, state_ids: StateMap[str], room_version: RoomVersion + ) -> Optional[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. + room_version: The room version of the room to query. + + Returns: + None if room access via spaces doesn't apply for this room. + + Otherwise, a collection of spaces (if any) which provide membership + to the room. + """ # This only applies to room versions which support the new join rule. if not room_version.msc3083_join_rules: - return + return None # 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 None # 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 + return None # 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 +128,6 @@ async def check_restricted_join_rules( 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) - # 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.", - ) + return result From 83974e8a4878d092c40cd86568ae49a63ad5037a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 18 May 2021 11:18:19 -0400 Subject: [PATCH 11/17] Further split the methods. --- synapse/handlers/event_auth.py | 47 ++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index eea6f13b1d0e..484eb3834740 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -64,24 +64,12 @@ async def check_restricted_join_rules( if allowed_spaces is None: return - # 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 = () - - # Pull out the other room IDs, invalid data gets filtered. - for space_id in allowed_spaces: - # The user was joined to one of the spaces specified, they can join - # this room! - if space_id in joined_rooms: - return - # 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.", - ) + 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 get_spaces_that_allow_join( self, state_ids: StateMap[str], room_version: RoomVersion @@ -131,3 +119,28 @@ async def get_spaces_that_allow_join( 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 rooms. + return False From 225e06d2774877ee47606c435be328c445e85ba4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 09:36:16 -0400 Subject: [PATCH 12/17] Clarify comment. Co-authored-by: Erik Johnston --- synapse/handlers/event_auth.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index 484eb3834740..107ffe7bab09 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -62,6 +62,11 @@ async def check_restricted_join_rules( # Get the spaces which allow access to this room, if applicable. allowed_spaces = await self.get_spaces_that_allow_join(state_ids, room_version) if allowed_spaces is None: + # 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. return # The user was not in any of the required spaces. From 7c28513101e879b82df7f7fa6f509c152bc6860f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 10:24:15 -0400 Subject: [PATCH 13/17] Clarify comments about when a room can be included. --- synapse/handlers/space_summary.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index c6e0e52009e3..bbf0a899f1f7 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -140,8 +140,7 @@ async def get_space_summary( # as the requesting server, are allowed to see, but the requesting # user is not permitted see. # - # A room may be visible if it is world readable, or if it is accessible - # to the specific user. + # Filter the returned results to only what is accessible o the user. room_ids = set() events = [] for room in fed_rooms: @@ -149,29 +148,39 @@ async def get_space_summary( if not fed_room_id: continue - # Check whether the room is world readable from the response. + # 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 - # If the room is not world-readable, get the allowed spaces from - # the response. + # 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: include_room = await self._event_auth_handler.is_user_in_rooms( allowed_spaces, requester ) - # Finally, check ourselves if we can access the room (this will - # fail if we don't have any state for this room). - if not include_room: + # 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) - # any rooms returned don't need visiting again (even if the user + # All rooms returned don't need visiting again (even if the user # didn't have access to them). processed_rooms.add(cast(str, room_id)) From 7c0ec1ad3a014ab7e69cefbb378c73752eb69894 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 10:24:33 -0400 Subject: [PATCH 14/17] Fix-up some type hints (and fix a bug about processed rooms). --- synapse/handlers/space_summary.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index bbf0a899f1f7..35ec992f77e9 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -16,7 +16,7 @@ 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 @@ -145,7 +145,7 @@ async def get_space_summary( events = [] for room in fed_rooms: fed_room_id = room.get("room_id") - if not fed_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: @@ -163,7 +163,11 @@ async def get_space_summary( # 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: + 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 ) @@ -182,7 +186,7 @@ async def get_space_summary( # All rooms returned don't need visiting again (even if the user # didn't have access to them). - processed_rooms.add(cast(str, room_id)) + processed_rooms.add(fed_room_id) for event in fed_events: if event.get("room_id") in room_ids: From 4888f02ab51b38fb93d8bc861f44fc053b8e9650 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 11:06:02 -0400 Subject: [PATCH 15/17] Further split the join rules checks. --- synapse/handlers/event_auth.py | 56 ++++++++++++++++++++----------- synapse/handlers/space_summary.py | 31 +++++++++++------ 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index 107ffe7bab09..a0df16a32f68 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -59,52 +59,68 @@ async def check_restricted_join_rules( ): return - # Get the spaces which allow access to this room, if applicable. - allowed_spaces = await self.get_spaces_that_allow_join(state_ids, room_version) - if allowed_spaces is None: - # 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. + # 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 - # The user was not in any of the required spaces. + # 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 get_spaces_that_allow_join( + async def has_restricted_join_rules( self, state_ids: StateMap[str], room_version: RoomVersion - ) -> Optional[Collection[str]]: + ) -> bool: """ - Generate a list of spaces which allow access to a room. + 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: - None if room access via spaces doesn't apply for this room. - - Otherwise, a collection of spaces (if any) which provide membership - to the room. + 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 None + 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 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 None + 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 None # If allowed is of the wrong form, then only allow invited users. allowed_spaces = join_rules_event.content.get("allow", []) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 35ec992f77e9..b14e39b8b97a 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -457,13 +457,16 @@ async def _is_room_accessible( # Otherwise, if they should be allowed access via membership in a space. # Get the spaces which allow access to this room, if applicable. - allowed_spaces = await self._event_auth_handler.get_spaces_that_allow_join( + if await self._event_auth_handler.has_restricted_join_rules( state_ids, room_version - ) - if allowed_spaces and await self._event_auth_handler.is_user_in_rooms( - allowed_spaces, requester ): - return True + allowed_spaces = ( + await self._event_auth_handler.get_spaces_that_allow_join(state_ids) + ) + if await self._event_auth_handler.is_user_in_rooms( + allowed_spaces, requester + ): + 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. @@ -474,10 +477,12 @@ async def _is_room_accessible( # 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). - allowed_spaces = await self._event_auth_handler.get_spaces_that_allow_join( + if await self._event_auth_handler.has_restricted_join_rules( state_ids, room_version - ) - if allowed_spaces: + ): + 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 @@ -517,9 +522,13 @@ async def _build_room_entry(self, room_id: str) -> JsonDict: room_type = create_event.content.get(EventContentFields.MSC1772_ROOM_TYPE) room_version = await self._store.get_room_version(room_id) - allowed_spaces = await self._event_auth_handler.get_spaces_that_allow_join( + 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"], @@ -534,7 +543,7 @@ async def _build_room_entry(self, room_id: str) -> JsonDict: "guest_can_join": stats["guest_access"] == "can_join", "creation_ts": create_event.origin_server_ts, "room_type": room_type, - "allowed_spaces": allowed_spaces if allowed_spaces else None, + "allowed_spaces": allowed_spaces, } # Filter out Nones – rather omit the field altogether From 9ba347bebfb1132253965a7bd3d7ba03b91c5a7a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 12:38:14 -0400 Subject: [PATCH 16/17] Fix typo. Co-authored-by: Erik Johnston --- synapse/handlers/space_summary.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index b14e39b8b97a..4c63e192f0c3 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -140,7 +140,7 @@ async def get_space_summary( # 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 o the user. + # Filter the returned results to only what is accessible to the user. room_ids = set() events = [] for room in fed_rooms: From fe1eb1809e85904117b98ffd7fd0fe690c77346a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 19 May 2021 13:54:41 -0400 Subject: [PATCH 17/17] Remove some duplicated code. --- synapse/handlers/space_summary.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 4c63e192f0c3..8d49ba81646e 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -26,6 +26,7 @@ HistoryVisibility, Membership, ) +from synapse.api.errors import AuthError from synapse.events import EventBase from synapse.events.utils import format_event_for_client_v2 from synapse.types import JsonDict @@ -450,23 +451,23 @@ async def _is_room_accessible( 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, if they should be allowed access via membership in a space. - # Get the spaces which allow access to this room, if applicable. - 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) + # Otherwise, check if they should be allowed access via membership in a space. + try: + await self._event_auth_handler.check_restricted_join_rules( + state_ids, room_version, requester, member_event ) - if await self._event_auth_handler.is_user_in_rooms( - allowed_spaces, requester - ): - return True + 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.