From 4911f7931d6f5cd65a13f7b1b5d3edecbab7c123 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 15 Jun 2021 08:03:17 -0400 Subject: Remove support for unstable MSC1772 prefixes. (#10161) The stable prefixes have been supported since v1.34.0. The unstable prefixes are not supported by any known clients. --- synapse/handlers/space_summary.py | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) (limited to 'synapse/handlers/space_summary.py') diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 046dba6fd8..73d2aab15c 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -402,10 +402,7 @@ class SpaceSummaryHandler: return (), () return res.rooms, tuple( - ev.data - for ev in res.events - if ev.event_type == EventTypes.MSC1772_SPACE_CHILD - or ev.event_type == EventTypes.SpaceChild + ev.data for ev in res.events if ev.event_type == EventTypes.SpaceChild ) async def _is_room_accessible( @@ -514,11 +511,6 @@ class SpaceSummaryHandler: current_state_ids[(EventTypes.Create, "")] ) - # TODO: update once MSC1772 lands - room_type = create_event.content.get(EventContentFields.ROOM_TYPE) - 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( @@ -540,7 +532,7 @@ class SpaceSummaryHandler: ), "guest_can_join": stats["guest_access"] == "can_join", "creation_ts": create_event.origin_server_ts, - "room_type": room_type, + "room_type": create_event.content.get(EventContentFields.ROOM_TYPE), "allowed_spaces": allowed_spaces, } @@ -569,9 +561,7 @@ class SpaceSummaryHandler: [ event_id for key, event_id in current_state_ids.items() - # TODO: update once MSC1772 has been FCP for a period of time. - if key[0] == EventTypes.MSC1772_SPACE_CHILD - or key[0] == EventTypes.SpaceChild + if key[0] == EventTypes.SpaceChild ] ) -- cgit 1.5.1 From 8c97d5863f352e48cb4e64a5b663411a7779686d Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 17 Jun 2021 12:53:27 -0400 Subject: Update MSC3083 support per changes in the MSC. (#10189) Adds a "type" field and generalize "space" to "room_id". --- changelog.d/10189.misc | 1 + synapse/api/constants.py | 6 ++++++ synapse/handlers/event_auth.py | 45 +++++++++++++++++++++++---------------- synapse/handlers/space_summary.py | 26 +++++++++++----------- 4 files changed, 47 insertions(+), 31 deletions(-) create mode 100644 changelog.d/10189.misc (limited to 'synapse/handlers/space_summary.py') diff --git a/changelog.d/10189.misc b/changelog.d/10189.misc new file mode 100644 index 0000000000..df0e636c7d --- /dev/null +++ b/changelog.d/10189.misc @@ -0,0 +1 @@ +Update MSC3083 support for modifications in the MSC. diff --git a/synapse/api/constants.py b/synapse/api/constants.py index ca13843680..6c3958f7ab 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -65,6 +65,12 @@ class JoinRules: MSC3083_RESTRICTED = "restricted" +class RestrictedJoinRuleTypes: + """Understood types for the allow rules in restricted join rules.""" + + ROOM_MEMBERSHIP = "m.room_membership" + + class LoginType: PASSWORD = "m.login.password" EMAIL_IDENTITY = "m.login.email.identity" diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index a0df16a32f..989996b628 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -13,7 +13,12 @@ # limitations under the License. from typing import TYPE_CHECKING, Collection, Optional -from synapse.api.constants import EventTypes, JoinRules, Membership +from synapse.api.constants import ( + EventTypes, + JoinRules, + Membership, + RestrictedJoinRuleTypes, +) from synapse.api.errors import AuthError from synapse.api.room_versions import RoomVersion from synapse.events import EventBase @@ -42,7 +47,7 @@ class EventAuthHandler: Check whether a user can join a room without an invite due to restricted join rules. When joining a room with restricted joined rules (as defined in MSC3083), - the membership of spaces must be checked during a room join. + the membership of rooms must be checked during a room join. Args: state_ids: The state of the room as it currently is. @@ -67,20 +72,20 @@ class EventAuthHandler: 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 + # Get the rooms 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): + allowed_rooms = await self.get_rooms_that_allow_join(state_ids) + if not await self.is_user_in_rooms(allowed_rooms, user_id): raise AuthError( 403, - "You do not belong to any of the required spaces to join this room.", + "You do not belong to any of the required rooms 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. + Return if the room has the proper join rules set for access via rooms. Args: state_ids: The state of the room as it currently is. @@ -102,17 +107,17 @@ class EventAuthHandler: 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( + async def get_rooms_that_allow_join( self, state_ids: StateMap[str] ) -> Collection[str]: """ - Generate a list of spaces which allow access to a room. + Generate a list of rooms in which membership allows access to a room. Args: - state_ids: The state of the room as it currently is. + state_ids: The current state of the room the user wishes to join Returns: - A collection of spaces which provide membership to the room. + A collection of room IDs. Membership in any of the rooms in the list grants the ability to join the target 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) @@ -123,21 +128,25 @@ class EventAuthHandler: join_rules_event = await self._store.get_event(join_rules_event_id) # 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): + allow_list = join_rules_event.content.get("allow", []) + if not isinstance(allow_list, list): return () # Pull out the other room IDs, invalid data gets filtered. result = [] - for space in allowed_spaces: - if not isinstance(space, dict): + for allow in allow_list: + if not isinstance(allow, dict): + continue + + # If the type is unexpected, skip it. + if allow.get("type") != RestrictedJoinRuleTypes.ROOM_MEMBERSHIP: continue - space_id = space.get("space") - if not isinstance(space_id, str): + room_id = allow.get("room_id") + if not isinstance(room_id, str): continue - result.append(space_id) + result.append(room_id) return result diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 73d2aab15c..e953a8afe6 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -160,14 +160,14 @@ class SpaceSummaryHandler: # Check if the user is a member of any of the allowed spaces # from the response. - allowed_spaces = room.get("allowed_spaces") + allowed_rooms = room.get("allowed_spaces") if ( not include_room - and allowed_spaces - and isinstance(allowed_spaces, list) + and allowed_rooms + and isinstance(allowed_rooms, list) ): include_room = await self._event_auth_handler.is_user_in_rooms( - allowed_spaces, requester + allowed_rooms, requester ) # Finally, if this isn't the requested room, check ourselves @@ -455,11 +455,11 @@ class SpaceSummaryHandler: if 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) + allowed_rooms = ( + await self._event_auth_handler.get_rooms_that_allow_join(state_ids) ) if await self._event_auth_handler.is_user_in_rooms( - allowed_spaces, requester + allowed_rooms, requester ): return True @@ -475,10 +475,10 @@ class SpaceSummaryHandler: 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) + allowed_rooms = ( + await self._event_auth_handler.get_rooms_that_allow_join(state_ids) ) - for space_id in allowed_spaces: + for space_id in allowed_rooms: if await self._auth.check_host_in_room(space_id, origin): return True @@ -512,11 +512,11 @@ class SpaceSummaryHandler: ) room_version = await self._store.get_room_version(room_id) - allowed_spaces = None + allowed_rooms = 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( + allowed_rooms = await self._event_auth_handler.get_rooms_that_allow_join( current_state_ids ) @@ -533,7 +533,7 @@ class SpaceSummaryHandler: "guest_can_join": stats["guest_access"] == "can_join", "creation_ts": create_event.origin_server_ts, "room_type": create_event.content.get(EventContentFields.ROOM_TYPE), - "allowed_spaces": allowed_spaces, + "allowed_spaces": allowed_rooms, } # Filter out Nones – rather omit the field altogether -- cgit 1.5.1 From 0bd968921c03dd61c1487f85dd884c4ed11ff486 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 18 Jun 2021 13:41:33 -0400 Subject: Fix a missing await when in the spaces summary. (#10208) This could cause a minor data leak if someone defined a non-restricted join rule with an allow key or used a restricted join rule in an older room version, but this is unlikely. Additionally this starts adding unit tests to the spaces summary handler. --- changelog.d/10208.bugfix | 1 + synapse/handlers/space_summary.py | 3 +- tests/handlers/test_space_summary.py | 99 +++++++++++++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 changelog.d/10208.bugfix (limited to 'synapse/handlers/space_summary.py') diff --git a/changelog.d/10208.bugfix b/changelog.d/10208.bugfix new file mode 100644 index 0000000000..32b6465717 --- /dev/null +++ b/changelog.d/10208.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.35.1 where an `allow` key of a `m.room.join_rules` event could be applied for incorrect room versions and configurations. diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index e953a8afe6..17fc47ce16 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -445,14 +445,13 @@ class SpaceSummaryHandler: 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. - if self._event_auth_handler.has_restricted_join_rules( + if await self._event_auth_handler.has_restricted_join_rules( state_ids, room_version ): allowed_rooms = ( diff --git a/tests/handlers/test_space_summary.py b/tests/handlers/test_space_summary.py index 2c5e81531b..131d362ccc 100644 --- a/tests/handlers/test_space_summary.py +++ b/tests/handlers/test_space_summary.py @@ -11,10 +11,15 @@ # 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 Any, Optional +from typing import Any, Iterable, Optional, Tuple from unittest import mock +from synapse.api.errors import AuthError from synapse.handlers.space_summary import _child_events_comparison_key +from synapse.rest import admin +from synapse.rest.client.v1 import login, room +from synapse.server import HomeServer +from synapse.types import JsonDict from tests import unittest @@ -79,3 +84,95 @@ class TestSpaceSummarySort(unittest.TestCase): ev1 = _create_event("!abc:test", "a" * 51) self.assertEqual([ev2, ev1], _order(ev1, ev2)) + + +class SpaceSummaryTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets_for_client_rest_resource, + room.register_servlets, + login.register_servlets, + ] + + def prepare(self, reactor, clock, hs: HomeServer): + self.hs = hs + self.handler = self.hs.get_space_summary_handler() + + self.user = self.register_user("user", "pass") + self.token = self.login("user", "pass") + + def _add_child(self, space_id: str, room_id: str, token: str) -> None: + """Add a child room to a space.""" + self.helper.send_state( + space_id, + event_type="m.space.child", + body={"via": [self.hs.hostname]}, + tok=token, + state_key=room_id, + ) + + def _assert_rooms(self, result: JsonDict, rooms: Iterable[str]) -> None: + """Assert that the expected room IDs are in the response.""" + self.assertCountEqual([room.get("room_id") for room in result["rooms"]], rooms) + + def _assert_events( + self, result: JsonDict, events: Iterable[Tuple[str, str]] + ) -> None: + """Assert that the expected parent / child room IDs are in the response.""" + self.assertCountEqual( + [ + (event.get("room_id"), event.get("state_key")) + for event in result["events"] + ], + events, + ) + + def test_simple_space(self): + """Test a simple space with a single room.""" + space = self.helper.create_room_as(self.user, tok=self.token) + room = self.helper.create_room_as(self.user, tok=self.token) + self._add_child(space, room, self.token) + + result = self.get_success(self.handler.get_space_summary(self.user, space)) + # The result should have the space and the room in it, along with a link + # from space -> room. + self._assert_rooms(result, [space, room]) + self._assert_events(result, [(space, room)]) + + def test_visibility(self): + """A user not in a space cannot inspect it.""" + space = self.helper.create_room_as(self.user, tok=self.token) + room = self.helper.create_room_as(self.user, tok=self.token) + self._add_child(space, room, self.token) + + user2 = self.register_user("user2", "pass") + token2 = self.login("user2", "pass") + + # The user cannot see the space. + self.get_failure(self.handler.get_space_summary(user2, space), AuthError) + + # Joining the room causes it to be visible. + self.helper.join(space, user2, tok=token2) + result = self.get_success(self.handler.get_space_summary(user2, space)) + + # The result should only have the space, but includes the link to the room. + self._assert_rooms(result, [space]) + self._assert_events(result, [(space, room)]) + + def test_world_readable(self): + """A world-readable room is visible to everyone.""" + space = self.helper.create_room_as(self.user, tok=self.token) + room = self.helper.create_room_as(self.user, tok=self.token) + self._add_child(space, room, self.token) + self.helper.send_state( + space, + event_type="m.room.history_visibility", + body={"history_visibility": "world_readable"}, + tok=self.token, + ) + + 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, space)) + self._assert_rooms(result, [space]) + self._assert_events(result, [(space, room)]) -- cgit 1.5.1