From bd901fef125fdacf5b1278e25647ac4b8d2e5334 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Wed, 14 Apr 2021 12:32:20 -0400
Subject: [PATCH 1/8] Check for space membership during a remote join of a
 restricted room. (#9763)

When receiving a /send_join request for a room with join rules set to 'restricted',
check if the user is a member of the spaces defined in the 'allow' key of the join
rules.

This only applies to an experimental room version, as defined in MSC3083.
---
 changelog.d/9814.feature        |  1 +
 synapse/handlers/event_auth.py  | 82 +++++++++++++++++++++++++++++++++
 synapse/handlers/federation.py  | 44 ++++++++++++++----
 synapse/handlers/room_member.py | 62 ++-----------------------
 synapse/server.py               |  5 ++
 5 files changed, 126 insertions(+), 68 deletions(-)
 create mode 100644 changelog.d/9814.feature
 create mode 100644 synapse/handlers/event_auth.py

diff --git a/changelog.d/9814.feature b/changelog.d/9814.feature
new file mode 100644
index 000000000000..9404ad2fc047
--- /dev/null
+++ b/changelog.d/9814.feature
@@ -0,0 +1 @@
+Update experimental support for [MSC3083](https://github.com/matrix-org/matrix-doc/pull/3083): restricting room access via group membership.
diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
new file mode 100644
index 000000000000..06da1a93d9f1
--- /dev/null
+++ b/synapse/handlers/event_auth.py
@@ -0,0 +1,82 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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 synapse.api.constants import EventTypes, JoinRules
+from synapse.api.room_versions import RoomVersion
+from synapse.types import StateMap
+
+if TYPE_CHECKING:
+    from synapse.server import HomeServer
+
+
+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:
+        """
+        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.
+        """
+        # This only applies to room versions which support the new join rule.
+        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 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)
+
+        # Pull out the other room IDs, invalid data gets filtered.
+        for space in allowed_spaces:
+            if not isinstance(space, dict):
+                continue
+
+            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 space_id in joined_rooms:
+                return True
+
+        # The user was not in any of the required spaces.
+        return False
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 4b3730aa3b7d..4f3ddc3debb3 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -146,6 +146,7 @@ def __init__(self, hs: "HomeServer"):
         self.is_mine_id = hs.is_mine_id
         self.spam_checker = hs.get_spam_checker()
         self.event_creation_handler = hs.get_event_creation_handler()
+        self.event_auth_handler = hs.get_event_auth_handler()
         self._message_handler = hs.get_message_handler()
         self._server_notices_mxid = hs.config.server_notices_mxid
         self.config = hs.config
@@ -1673,8 +1674,40 @@ async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
         # would introduce the danger of backwards-compatibility problems.
         event.internal_metadata.send_on_behalf_of = origin
 
+        # Calculate the event context.
         context = await self.state_handler.compute_event_context(event)
-        context = await self._auth_and_persist_event(origin, event, context)
+
+        # Get the state before the new event.
+        prev_state_ids = await context.get_prev_state_ids()
+
+        # Check if the user is already in the room or invited to the room.
+        user_id = event.state_key
+        prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None)
+        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, and not invited, check if
+        # they should be allowed access via membership in a space.
+        if (
+            newly_joined
+            and not is_invite
+            and not await self.event_auth_handler.can_join_without_invite(
+                prev_state_ids,
+                event.room_version,
+                user_id,
+            )
+        ):
+            raise SynapseError(
+                400,
+                "You do not belong to any of the required spaces to join this room.",
+            )
+
+        # Persist the event.
+        await self._auth_and_persist_event(origin, event, context)
 
         logger.debug(
             "on_send_join_request: After _auth_and_persist_event: %s, sigs: %s",
@@ -1682,8 +1715,6 @@ async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
             event.signatures,
         )
 
-        prev_state_ids = await context.get_prev_state_ids()
-
         state_ids = list(prev_state_ids.values())
         auth_chain = await self.store.get_auth_chain(event.room_id, state_ids)
 
@@ -2006,7 +2037,7 @@ async def _auth_and_persist_event(
         state: Optional[Iterable[EventBase]] = None,
         auth_events: Optional[MutableStateMap[EventBase]] = None,
         backfilled: bool = False,
-    ) -> EventContext:
+    ) -> None:
         """
         Process an event by performing auth checks and then persisting to the database.
 
@@ -2028,9 +2059,6 @@ async def _auth_and_persist_event(
                 event is an outlier), may be the auth events claimed by the remote
                 server.
             backfilled: True if the event was backfilled.
-
-        Returns:
-             The event context.
         """
         context = await self._check_event_auth(
             origin,
@@ -2060,8 +2088,6 @@ async def _auth_and_persist_event(
             )
             raise
 
-        return context
-
     async def _auth_and_persist_events(
         self,
         origin: str,
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 2bbfac6471e5..2c5bada1d89b 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -19,7 +19,7 @@
 from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple
 
 from synapse import types
-from synapse.api.constants import AccountDataTypes, EventTypes, JoinRules, Membership
+from synapse.api.constants import AccountDataTypes, EventTypes, Membership
 from synapse.api.errors import (
     AuthError,
     Codes,
@@ -28,7 +28,6 @@
     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
@@ -64,6 +63,7 @@ def __init__(self, hs: "HomeServer"):
         self.profile_handler = hs.get_profile_handler()
         self.event_creation_handler = hs.get_event_creation_handler()
         self.account_data_handler = hs.get_account_data_handler()
+        self.event_auth_handler = hs.get_event_auth_handler()
 
         self.member_linearizer = Linearizer(name="member")
 
@@ -178,62 +178,6 @@ async def ratelimit_invite(
 
         await self._invites_per_user_limiter.ratelimit(requester, invitee_user_id)
 
-    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.
-        """
-        # This only applies to room versions which support the new join rule.
-        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 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)
-
-        # Pull out the other room IDs, invalid data gets filtered.
-        for space in allowed_spaces:
-            if not isinstance(space, dict):
-                continue
-
-            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 space_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,
@@ -302,7 +246,7 @@ async def _local_membership_update(
             if (
                 newly_joined
                 and not user_is_invited
-                and not await self._can_join_without_invite(
+                and not await self.event_auth_handler.can_join_without_invite(
                     prev_state_ids, event.room_version, user_id
                 )
             ):
diff --git a/synapse/server.py b/synapse/server.py
index 95a2cd2e5d08..045b8f3fcac3 100644
--- a/synapse/server.py
+++ b/synapse/server.py
@@ -77,6 +77,7 @@
 from synapse.handlers.directory import DirectoryHandler
 from synapse.handlers.e2e_keys import E2eKeysHandler
 from synapse.handlers.e2e_room_keys import E2eRoomKeysHandler
+from synapse.handlers.event_auth import EventAuthHandler
 from synapse.handlers.events import EventHandler, EventStreamHandler
 from synapse.handlers.federation import FederationHandler
 from synapse.handlers.groups_local import GroupsLocalHandler, GroupsLocalWorkerHandler
@@ -749,6 +750,10 @@ def get_account_data_handler(self) -> AccountDataHandler:
     def get_space_summary_handler(self) -> SpaceSummaryHandler:
         return SpaceSummaryHandler(self)
 
+    @cache_in_self
+    def get_event_auth_handler(self) -> EventAuthHandler:
+        return EventAuthHandler(self)
+
     @cache_in_self
     def get_external_cache(self) -> ExternalCache:
         return ExternalCache(self)

From 4c25353ef63e0d3a5b18b1f9e78a4bc520dac667 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 15 Apr 2021 10:13:04 -0400
Subject: [PATCH 2/8] Return auth errors back to the client.

---
 synapse/federation/federation_client.py | 11 +++++++++++
 synapse/handlers/federation.py          |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index f93335edaa1c..5dbc2d8ffeed 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -44,6 +44,7 @@
     Codes,
     FederationDeniedError,
     HttpResponseException,
+    ProxiedRequestError,
     SynapseError,
     UnsupportedRoomVersionError,
 )
@@ -524,7 +525,17 @@ async def _try_destination_list(
                     e.code,
                     e.args[0],
                 )
+            except ProxiedRequestError as e:
+                # If an auth error comes back from a federation endpoint, pass
+                # it back to the client.
+                if e.code == 403:
+                    raise
+
+                logger.warning(
+                    "Failed to %s via %s", description, destination, exc_info=True
+                )
             except Exception:
+                logger.warning("POMP POMP POMP POMP POMP")
                 logger.warning(
                     "Failed to %s via %s", description, destination, exc_info=True
                 )
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 4f3ddc3debb3..9bd69e49be35 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1702,8 +1702,9 @@ async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
             )
         ):
             raise SynapseError(
-                400,
+                403,
                 "You do not belong to any of the required spaces to join this room.",
+                Codes.FORBIDDEN,
             )
 
         # Persist the event.

From af2d1e649a208bbbf2affc44986fd77963300daa Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Tue, 20 Apr 2021 09:48:00 -0400
Subject: [PATCH 3/8] Apply suggestions from code review

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
---
 synapse/handlers/event_auth.py | 2 +-
 synapse/handlers/federation.py | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
index 06da1a93d9f1..24e3f1ebeeec 100644
--- a/synapse/handlers/event_auth.py
+++ b/synapse/handlers/event_auth.py
@@ -46,7 +46,7 @@ async def can_join_without_invite(
         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).
+        # 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
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 9bd69e49be35..870cd3cffca9 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1701,10 +1701,9 @@ async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
                 user_id,
             )
         ):
-            raise SynapseError(
+            raise AuthError(
                 403,
                 "You do not belong to any of the required spaces to join this room.",
-                Codes.FORBIDDEN,
             )
 
         # Persist the event.

From 5350838df13aecf1ec33a7e1d7f0cff425725902 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 20 Apr 2021 09:49:16 -0400
Subject: [PATCH 4/8] Rename variables based on feedback.

---
 synapse/handlers/federation.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 870cd3cffca9..9a3337cd14aa 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -146,7 +146,7 @@ def __init__(self, hs: "HomeServer"):
         self.is_mine_id = hs.is_mine_id
         self.spam_checker = hs.get_spam_checker()
         self.event_creation_handler = hs.get_event_creation_handler()
-        self.event_auth_handler = hs.get_event_auth_handler()
+        self._event_auth_handler = hs.get_event_auth_handler()
         self._message_handler = hs.get_message_handler()
         self._server_notices_mxid = hs.config.server_notices_mxid
         self.config = hs.config
@@ -1684,18 +1684,18 @@ async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
         user_id = event.state_key
         prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None)
         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 not invited, check if
         # they should be allowed access via membership in a space.
         if (
             newly_joined
-            and not is_invite
-            and not await self.event_auth_handler.can_join_without_invite(
+            and not user_is_invited
+            and not await self._event_auth_handler.can_join_without_invite(
                 prev_state_ids,
                 event.room_version,
                 user_id,

From 03a8d43527b2fa435cfa40a5eaeebd9cfeff66e5 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 20 Apr 2021 09:59:36 -0400
Subject: [PATCH 5/8] Fix-up comments.

---
 synapse/api/auth.py            | 1 +
 synapse/handlers/event_auth.py | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 6c13f53957c4..25538b254328 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -65,6 +65,7 @@ class Auth:
     """
     FIXME: This class contains a mix of functions for authenticating users
     of our client-server API and authenticating events added to room graphs.
+    The latter should be moved to synapse.handlers.event_auth.EventAuthHandler.
     """
 
     def __init__(self, hs):
diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
index 24e3f1ebeeec..d17a13add292 100644
--- a/synapse/handlers/event_auth.py
+++ b/synapse/handlers/event_auth.py
@@ -22,6 +22,9 @@
 
 
 class EventAuthHandler:
+    """
+    This class contains methods for authenticating events added to room graphs.
+    """
     def __init__(self, hs: "HomeServer"):
         self._store = hs.get_datastore()
 

From 6e8a50ceb104fbae94b99b549a022d886946708a Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 20 Apr 2021 10:01:56 -0400
Subject: [PATCH 6/8] Remove extraneous log-line.

---
 synapse/federation/federation_client.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index 5dbc2d8ffeed..674124284109 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -535,7 +535,6 @@ async def _try_destination_list(
                     "Failed to %s via %s", description, destination, exc_info=True
                 )
             except Exception:
-                logger.warning("POMP POMP POMP POMP POMP")
                 logger.warning(
                     "Failed to %s via %s", description, destination, exc_info=True
                 )

From 32c9467a6431b6da800f94f77afc33646f3b3774 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 20 Apr 2021 10:47:37 -0400
Subject: [PATCH 7/8] Lint

---
 synapse/handlers/event_auth.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
index d17a13add292..eff639f40760 100644
--- a/synapse/handlers/event_auth.py
+++ b/synapse/handlers/event_auth.py
@@ -25,6 +25,7 @@ class EventAuthHandler:
     """
     This class contains methods for authenticating events added to room graphs.
     """
+
     def __init__(self, hs: "HomeServer"):
         self._store = hs.get_datastore()
 

From 4e37845eb136ff314b38aa10e9b6241ed44014f6 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Thu, 22 Apr 2021 07:48:59 -0400
Subject: [PATCH 8/8] Revert changes to FederationClient.

---
 synapse/federation/federation_client.py | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index 674124284109..f93335edaa1c 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -44,7 +44,6 @@
     Codes,
     FederationDeniedError,
     HttpResponseException,
-    ProxiedRequestError,
     SynapseError,
     UnsupportedRoomVersionError,
 )
@@ -525,15 +524,6 @@ async def _try_destination_list(
                     e.code,
                     e.args[0],
                 )
-            except ProxiedRequestError as e:
-                # If an auth error comes back from a federation endpoint, pass
-                # it back to the client.
-                if e.code == 403:
-                    raise
-
-                logger.warning(
-                    "Failed to %s via %s", description, destination, exc_info=True
-                )
             except Exception:
                 logger.warning(
                     "Failed to %s via %s", description, destination, exc_info=True