From f2c6215bd02b90fe98888a4ec5e733ce924621ae Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 30 Mar 2021 14:25:13 -0400 Subject: [PATCH 01/11] Check for proper membership during a local join. --- synapse/handlers/room_member.py | 66 ++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 1cf12f3255bc..9e2bd1d251af 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -20,7 +20,7 @@ from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple from synapse import types -from synapse.api.constants import AccountDataTypes, EventTypes, Membership +from synapse.api.constants import AccountDataTypes, EventTypes, JoinRules, Membership from synapse.api.errors import ( AuthError, Codes, @@ -178,6 +178,60 @@ async def ratelimit_invite( await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id) + async def _can_join_restricted_room( + self, state_ids: StateMap[str], room_id: str, user_id: str + ) -> bool: + """ + Check whether a user can join a restricted room. + + Args: + state_ids: The state of the room as it currently is. + room_id: The room being joined. + user_id: The user joining the room. + + Returns: + True if the user can join the room, false otherwise. + """ + # This only applies to room versions which support the new join rule. + room_version = await self.store.get_room_version(room_id) + if not room_version.msc3083_join_rules: + return True + + # If there's no join rule, then it defaults to public (so this doesn't apply). + join_rules_event_id = state_ids.get((EventTypes.JoinRules, ""), None) + if not join_rules_event_id: + return True + + # 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 + + # If allowed is of the wrong form, then ignore it (and treat the room as public). + allowed_spaces = join_rules_event.content.get("allow", []) + if not isinstance(allowed_spaces, list): + 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) + + # Pull out the other room IDs, invalid data gets filtered. + for space in allowed_spaces: + if not isinstance(space, dict): + continue + + soace_id = space.get("space") + if not isinstance(soace_id, str): + continue + + # The user was joined to one of the spaces specified, they can join + # this room! + if soace_id in joined_rooms: + return True + + # The user was not in any of the required spaces. + return False + async def _local_membership_update( self, requester: Requester, @@ -239,6 +293,16 @@ async def _local_membership_update( prev_member_event = await self.store.get_event(prev_member_event_id) newly_joined = prev_member_event.membership != Membership.JOIN + # If the member is not already in the room, check if they should be + # allowed access via membership in a space. + if newly_joined and not await self._can_join_restricted_room( + prev_state_ids, room_id, user_id + ): + raise AuthError( + 403, + "You do not belong to any of the required spaces to join this room.", + ) + # Only rate-limit if the user actually joined the room, otherwise we'll end # up blocking profile updates. if newly_joined and ratelimit: From 98840cf5263fef343a450ff66629fc3012018f6f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 1 Apr 2021 16:48:55 -0400 Subject: [PATCH 02/11] Enable additional complement tests. --- scripts-dev/complement.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 31cc20a826b9..b77187472ffe 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -46,4 +46,4 @@ if [[ -n "$1" ]]; then fi # Run the tests! -COMPLEMENT_BASE_IMAGE=complement-synapse go test -v -tags synapse_blacklist -count=1 $EXTRA_COMPLEMENT_ARGS ./tests +COMPLEMENT_BASE_IMAGE=complement-synapse go test -v -tags synapse_blacklist,msc3083 -count=1 $EXTRA_COMPLEMENT_ARGS ./tests From 194aa751cfe2271a801918d57327b48679d2c35c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 2 Apr 2021 08:16:42 -0400 Subject: [PATCH 03/11] Newsfragment --- changelog.d/9735.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9735.feature diff --git a/changelog.d/9735.feature b/changelog.d/9735.feature new file mode 100644 index 000000000000..c2c74f13d578 --- /dev/null +++ b/changelog.d/9735.feature @@ -0,0 +1 @@ +Add experimental support for [MSC3083](https://github.com/matrix-org/matrix-doc/pull/3083): restricting room access via group membership. From db83bbd4ca101495b0361edde80d1afbb50adb39 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 2 Apr 2021 09:38:02 -0400 Subject: [PATCH 04/11] Don't do auth checks if user is invited. --- synapse/handlers/room_member.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 9e2bd1d251af..f997f4886466 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -289,14 +289,20 @@ async def _local_membership_update( if event.membership == Membership.JOIN: newly_joined = True + is_invite = False if prev_member_event_id: prev_member_event = await self.store.get_event(prev_member_event_id) newly_joined = prev_member_event.membership != Membership.JOIN + is_invite = prev_member_event.membership == Membership.INVITE # If the member is not already in the room, check if they should be # allowed access via membership in a space. - if newly_joined and not await self._can_join_restricted_room( - prev_state_ids, room_id, user_id + if ( + newly_joined + and not is_invite + and not await self._can_join_restricted_room( + prev_state_ids, room_id, user_id + ) ): raise AuthError( 403, From 0523fce12694996c65aa487c009642080f887251 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Apr 2021 09:49:18 -0400 Subject: [PATCH 05/11] Directly use the room version instead of re-fetching it. --- synapse/handlers/room_member.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index f997f4886466..7352aa3cce7d 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -29,6 +29,7 @@ SynapseError, ) from synapse.api.ratelimiting import Ratelimiter +from synapse.api.room_versions import RoomVersion from synapse.events import EventBase from synapse.events.snapshot import EventContext from synapse.types import JsonDict, Requester, RoomAlias, RoomID, StateMap, UserID @@ -179,7 +180,7 @@ async def ratelimit_invite( await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id) async def _can_join_restricted_room( - self, state_ids: StateMap[str], room_id: str, user_id: str + self, state_ids: StateMap[str], room_version: RoomVersion, user_id: str ) -> bool: """ Check whether a user can join a restricted room. @@ -193,7 +194,6 @@ async def _can_join_restricted_room( True if the user can join the room, false otherwise. """ # This only applies to room versions which support the new join rule. - room_version = await self.store.get_room_version(room_id) if not room_version.msc3083_join_rules: return True @@ -301,7 +301,7 @@ async def _local_membership_update( newly_joined and not is_invite and not await self._can_join_restricted_room( - prev_state_ids, room_id, user_id + prev_state_ids, event.room_version, user_id ) ): raise AuthError( From fad13e0911cb6d0f0673c76e4610c5a075546ffc Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Apr 2021 13:00:18 -0400 Subject: [PATCH 06/11] Update comment. --- synapse/handlers/room_member.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 7352aa3cce7d..fe9c0acdf18a 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -187,7 +187,7 @@ async def _can_join_restricted_room( Args: state_ids: The state of the room as it currently is. - room_id: The room being joined. + room_version: The room version of the room being joined. user_id: The user joining the room. Returns: From bdc6ae1f38f1847f911d0124eec2d9e0c986b842 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 5 Apr 2021 13:29:30 -0400 Subject: [PATCH 07/11] Update a comment. --- synapse/handlers/room_member.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index fe9c0acdf18a..5620a7600a56 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -295,8 +295,8 @@ async def _local_membership_update( newly_joined = prev_member_event.membership != Membership.JOIN is_invite = prev_member_event.membership == Membership.INVITE - # If the member is not already in the room, check if they should be - # allowed access via membership in a space. + # If the member is not already in the room and is not accepting an invite, + # check if they should be allowed access via membership in a space. if ( newly_joined and not is_invite From c0a5192779379c76a1b5972491f5133667dad949 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 7 Apr 2021 11:10:51 -0400 Subject: [PATCH 08/11] Rename variables. --- synapse/handlers/room_member.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 5620a7600a56..22b02e907978 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -220,13 +220,13 @@ async def _can_join_restricted_room( if not isinstance(space, dict): continue - soace_id = space.get("space") - if not isinstance(soace_id, str): + space_id = space.get("space") + if not isinstance(space_id, str): continue # The user was joined to one of the spaces specified, they can join # this room! - if soace_id in joined_rooms: + if space_id in joined_rooms: return True # The user was not in any of the required spaces. @@ -289,17 +289,17 @@ async def _local_membership_update( if event.membership == Membership.JOIN: newly_joined = True - is_invite = False + user_is_invited = False if prev_member_event_id: prev_member_event = await self.store.get_event(prev_member_event_id) newly_joined = prev_member_event.membership != Membership.JOIN - is_invite = prev_member_event.membership == Membership.INVITE + user_is_invited = prev_member_event.membership == Membership.INVITE # If the member is not already in the room and is not accepting an invite, # check if they should be allowed access via membership in a space. if ( newly_joined - and not is_invite + and not user_is_invited and not await self._can_join_restricted_room( prev_state_ids, event.room_version, user_id ) From 04fdfe5d09fceb3722b44d58f8b35a80e999424b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 7 Apr 2021 11:12:34 -0400 Subject: [PATCH 09/11] Tweak the method name. --- synapse/handlers/room_member.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 22b02e907978..acb4304fa6f9 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -179,11 +179,14 @@ async def ratelimit_invite( await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id) - async def _can_join_restricted_room( + 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 restricted room. + 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. @@ -300,7 +303,7 @@ async def _local_membership_update( if ( newly_joined and not user_is_invited - and not await self._can_join_restricted_room( + and not await self._can_join_without_invite( prev_state_ids, event.room_version, user_id ) ): From 7bd595d14887fc65c6260e54ce878155a74acccb Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 7 Apr 2021 12:10:26 -0400 Subject: [PATCH 10/11] Modify the behavior of how bad data is handled to fail closed. --- synapse/handlers/room_member.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index acb4304fa6f9..5ad040050ee4 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -210,10 +210,10 @@ async def _can_join_without_invite( if join_rules_event.content.get("join_rule") != JoinRules.MSC3083_RESTRICTED: return True - # If allowed is of the wrong form, then ignore it (and treat the room as public). + # If allowed is of the wrong form, then only allows invites. allowed_spaces = join_rules_event.content.get("allow", []) if not isinstance(allowed_spaces, list): - return True + 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) From d5c9c3664fcddcbab3ba053cec7498fe42731952 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 8 Apr 2021 07:57:09 -0400 Subject: [PATCH 11/11] Clarify comment. Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/room_member.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 5ad040050ee4..894ef859f4d4 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -210,7 +210,7 @@ async def _can_join_without_invite( if join_rules_event.content.get("join_rule") != JoinRules.MSC3083_RESTRICTED: return True - # If allowed is of the wrong form, then only allows invites. + # 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