From 5ff06684b67db8fa285584b6da647b0496cf045b Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 27 Sep 2021 19:50:09 +0100
Subject: [PATCH 1/8] Factor `check_auth_rules_for_event` out of
 `event_auth.check`

If we're going to skip most of the method, we may as well just run the bits we
care about.
---
 synapse/event_auth.py    |  63 +++++++++++++++++------
 synapse/state/v1.py      |   8 +--
 synapse/state/v2.py      |   4 +-
 tests/test_event_auth.py | 108 +++++++++++++--------------------------
 4 files changed, 86 insertions(+), 97 deletions(-)

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index 5d7c6fa858fb..b2b1da570098 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -64,28 +64,12 @@ def check(
     Returns:
          if the auth checks pass.
     """
-    assert isinstance(auth_events, dict)
-
     if do_size_check:
         _check_size_limits(event)
 
     if not hasattr(event, "room_id"):
         raise AuthError(500, "Event has no room_id: %s" % event)
 
-    room_id = event.room_id
-
-    # We need to ensure that the auth events are actually for the same room, to
-    # stop people from using powers they've been granted in other rooms for
-    # example.
-    for auth_event in auth_events.values():
-        if auth_event.room_id != room_id:
-            raise AuthError(
-                403,
-                "During auth for event %s in room %s, found event %s in the state "
-                "which is in room %s"
-                % (event.event_id, room_id, auth_event.event_id, auth_event.room_id),
-            )
-
     if do_sig_check:
         sender_domain = get_domain_from_id(event.sender)
 
@@ -125,6 +109,53 @@ def check(
             if not event.signatures.get(authoriser_domain):
                 raise AuthError(403, "Event not signed by authorising server")
 
+    check_auth_rules_for_event(room_version_obj, event, auth_events)
+
+
+def check_auth_rules_for_event(
+    room_version_obj: RoomVersion, event: EventBase, auth_events: StateMap[EventBase]
+) -> None:
+    """Check that an event complies with the auth rules
+
+    Checks whether an event passes the auth rules with a given set of state events
+
+    Assumes that we have already checked that the event is the right shape (it has
+    enough signatures, has a room ID, etc). In other words:
+
+     - it's fine for use in state resolution, when we have already decided whether to
+       accept the event or not, and are now trying to decide whether it should make it
+       into the room state
+
+     - when we're doing the initial event auth, it is only suitable in combination with
+       a bunch of other tests.
+
+    Args:
+        room_version_obj: the version of the room
+        event: the event being checked.
+        auth_events: the room state to check the events against.
+
+    Raises:
+        AuthError if the checks fail
+    """
+    assert isinstance(auth_events, dict)
+
+    # We need to ensure that the auth events are actually for the same room, to
+    # stop people from using powers they've been granted in other rooms for
+    # example.
+    #
+    # Arguably we don't need to do this when we're just doing state res, as presumably
+    # the state res algorithm isn't silly enough to give us events from different rooms.
+    # Still, it's easier to do it anyway.
+    room_id = event.room_id
+    for auth_event in auth_events.values():
+        if auth_event.room_id != room_id:
+            raise AuthError(
+                403,
+                "During auth for event %s in room %s, found event %s in the state "
+                "which is in room %s"
+                % (event.event_id, room_id, auth_event.event_id, auth_event.room_id),
+            )
+
     # Implementation of https://matrix.org/docs/spec/rooms/v1#authorization-rules
     #
     # 1. If type is m.room.create:
diff --git a/synapse/state/v1.py b/synapse/state/v1.py
index 92336d7cc8be..017e6fd92d38 100644
--- a/synapse/state/v1.py
+++ b/synapse/state/v1.py
@@ -329,12 +329,10 @@ def _resolve_auth_events(
         auth_events[(prev_event.type, prev_event.state_key)] = prev_event
         try:
             # The signatures have already been checked at this point
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V1,
                 event,
                 auth_events,
-                do_sig_check=False,
-                do_size_check=False,
             )
             prev_event = event
         except AuthError:
@@ -349,12 +347,10 @@ def _resolve_normal_events(
     for event in _ordered_events(events):
         try:
             # The signatures have already been checked at this point
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V1,
                 event,
                 auth_events,
-                do_sig_check=False,
-                do_size_check=False,
             )
             return event
         except AuthError:
diff --git a/synapse/state/v2.py b/synapse/state/v2.py
index 7b1e8361def4..586b0e12febe 100644
--- a/synapse/state/v2.py
+++ b/synapse/state/v2.py
@@ -546,12 +546,10 @@ async def _iterative_auth_checks(
                     auth_events[key] = event_map[ev_id]
 
         try:
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 room_version,
                 event,
                 auth_events,
-                do_sig_check=False,
-                do_size_check=False,
             )
 
             resolved_state[(event.type, event.state_key)] = event_id
diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py
index 6ebd01bcbe78..e7a7d008832d 100644
--- a/tests/test_event_auth.py
+++ b/tests/test_event_auth.py
@@ -37,21 +37,19 @@ def test_random_users_cannot_send_state_before_first_pl(self):
         }
 
         # creator should be able to send state
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V1,
             _random_state_event(creator),
             auth_events,
-            do_sig_check=False,
         )
 
         # joiner should not be able to send state
         self.assertRaises(
             AuthError,
-            event_auth.check,
+            event_auth.check_auth_rules_for_event,
             RoomVersions.V1,
             _random_state_event(joiner),
             auth_events,
-            do_sig_check=False,
         )
 
     def test_state_default_level(self):
@@ -76,19 +74,17 @@ def test_state_default_level(self):
         # pleb should not be able to send state
         self.assertRaises(
             AuthError,
-            event_auth.check,
+            event_auth.check_auth_rules_for_event,
             RoomVersions.V1,
             _random_state_event(pleb),
             auth_events,
-            do_sig_check=False,
         ),
 
         # king should be able to send state
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V1,
             _random_state_event(king),
             auth_events,
-            do_sig_check=False,
         )
 
     def test_alias_event(self):
@@ -101,37 +97,33 @@ def test_alias_event(self):
         }
 
         # creator should be able to send aliases
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V1,
             _alias_event(creator),
             auth_events,
-            do_sig_check=False,
         )
 
         # Reject an event with no state key.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V1,
                 _alias_event(creator, state_key=""),
                 auth_events,
-                do_sig_check=False,
             )
 
         # If the domain of the sender does not match the state key, reject.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V1,
                 _alias_event(creator, state_key="test.com"),
                 auth_events,
-                do_sig_check=False,
             )
 
         # Note that the member does *not* need to be in the room.
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V1,
             _alias_event(other),
             auth_events,
-            do_sig_check=False,
         )
 
     def test_msc2432_alias_event(self):
@@ -144,34 +136,30 @@ def test_msc2432_alias_event(self):
         }
 
         # creator should be able to send aliases
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _alias_event(creator),
             auth_events,
-            do_sig_check=False,
         )
 
         # No particular checks are done on the state key.
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _alias_event(creator, state_key=""),
             auth_events,
-            do_sig_check=False,
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _alias_event(creator, state_key="test.com"),
             auth_events,
-            do_sig_check=False,
         )
 
         # Per standard auth rules, the member must be in the room.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _alias_event(other),
                 auth_events,
-                do_sig_check=False,
             )
 
     def test_msc2209(self):
@@ -191,20 +179,18 @@ def test_msc2209(self):
         }
 
         # pleb should be able to modify the notifications power level.
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V1,
             _power_levels_event(pleb, {"notifications": {"room": 100}}),
             auth_events,
-            do_sig_check=False,
         )
 
         # But an MSC2209 room rejects this change.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _power_levels_event(pleb, {"notifications": {"room": 100}}),
                 auth_events,
-                do_sig_check=False,
             )
 
     def test_join_rules_public(self):
@@ -221,59 +207,53 @@ def test_join_rules_public(self):
         }
 
         # Check join.
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
         # A user cannot be force-joined to a room.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _member_event(pleb, "join", sender=creator),
                 auth_events,
-                do_sig_check=False,
             )
 
         # Banned should be rejected.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban")
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user who left can re-join.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave")
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
         # A user can send a join if they're in the room.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "join")
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
         # A user can accept an invite.
         auth_events[("m.room.member", pleb)] = _member_event(
             pleb, "invite", sender=creator
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
     def test_join_rules_invite(self):
@@ -291,60 +271,54 @@ def test_join_rules_invite(self):
 
         # A join without an invite is rejected.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user cannot be force-joined to a room.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _member_event(pleb, "join", sender=creator),
                 auth_events,
-                do_sig_check=False,
             )
 
         # Banned should be rejected.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban")
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user who left cannot re-join.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave")
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user can send a join if they're in the room.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "join")
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
         # A user can accept an invite.
         auth_events[("m.room.member", pleb)] = _member_event(
             pleb, "invite", sender=creator
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V6,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
     def test_join_rules_msc3083_restricted(self):
@@ -369,11 +343,10 @@ def test_join_rules_msc3083_restricted(self):
 
         # Older room versions don't understand this join rule
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V6,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A properly formatted join event should work.
@@ -383,11 +356,10 @@ def test_join_rules_msc3083_restricted(self):
                 "join_authorised_via_users_server": "@creator:example.com"
             },
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V8,
             authorised_join_event,
             auth_events,
-            do_sig_check=False,
         )
 
         # A join issued by a specific user works (i.e. the power level checks
@@ -399,7 +371,7 @@ def test_join_rules_msc3083_restricted(self):
         pl_auth_events[("m.room.member", "@inviter:foo.test")] = _join_event(
             "@inviter:foo.test"
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V8,
             _join_event(
                 pleb,
@@ -408,16 +380,14 @@ def test_join_rules_msc3083_restricted(self):
                 },
             ),
             pl_auth_events,
-            do_sig_check=False,
         )
 
         # A join which is missing an authorised server is rejected.
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V8,
                 _join_event(pleb),
                 auth_events,
-                do_sig_check=False,
             )
 
         # An join authorised by a user who is not in the room is rejected.
@@ -426,7 +396,7 @@ def test_join_rules_msc3083_restricted(self):
             creator, {"invite": 100, "users": {"@other:example.com": 150}}
         )
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V8,
                 _join_event(
                     pleb,
@@ -435,13 +405,12 @@ def test_join_rules_msc3083_restricted(self):
                     },
                 ),
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user cannot be force-joined to a room. (This uses an event which
         # *would* be valid, but is sent be a different user.)
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V8,
                 _member_event(
                     pleb,
@@ -452,36 +421,32 @@ def test_join_rules_msc3083_restricted(self):
                     },
                 ),
                 auth_events,
-                do_sig_check=False,
             )
 
         # Banned should be rejected.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "ban")
         with self.assertRaises(AuthError):
-            event_auth.check(
+            event_auth.check_auth_rules_for_event(
                 RoomVersions.V8,
                 authorised_join_event,
                 auth_events,
-                do_sig_check=False,
             )
 
         # A user who left can re-join.
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "leave")
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V8,
             authorised_join_event,
             auth_events,
-            do_sig_check=False,
         )
 
         # A user can send a join if they're in the room. (This doesn't need to
         # be authorised since the user is already joined.)
         auth_events[("m.room.member", pleb)] = _member_event(pleb, "join")
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V8,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
         # A user can accept an invite. (This doesn't need to be authorised since
@@ -489,11 +454,10 @@ def test_join_rules_msc3083_restricted(self):
         auth_events[("m.room.member", pleb)] = _member_event(
             pleb, "invite", sender=creator
         )
-        event_auth.check(
+        event_auth.check_auth_rules_for_event(
             RoomVersions.V8,
             _join_event(pleb),
             auth_events,
-            do_sig_check=False,
         )
 
 

From 97638047915c82a43b1005d1e380f21195c19be5 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 13:38:52 +0100
Subject: [PATCH 2/8] `event_auth.check`: delete redundant `do_size_check`

`do_size_check` is now always true, so we may as well get rid of it.
---
 synapse/event_auth.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index b2b1da570098..66e198e25d10 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -46,7 +46,6 @@ def check(
     event: EventBase,
     auth_events: StateMap[EventBase],
     do_sig_check: bool = True,
-    do_size_check: bool = True,
 ) -> None:
     """Checks if this event is correctly authed.
 
@@ -56,7 +55,6 @@ def check(
         auth_events: the existing room state.
         do_sig_check: True if it should be verified that the sending server
             signed the event.
-        do_size_check: True if the size of the event fields should be verified.
 
     Raises:
         AuthError if the checks fail
@@ -64,8 +62,7 @@ def check(
     Returns:
          if the auth checks pass.
     """
-    if do_size_check:
-        _check_size_limits(event)
+    _check_size_limits(event)
 
     if not hasattr(event, "room_id"):
         raise AuthError(500, "Event has no room_id: %s" % event)

From b92299fbb7f63ddcd468a5a70633d73e84556560 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 14:02:09 +0100
Subject: [PATCH 3/8] factor out a separate `validate_event_for_room_version`

---
 synapse/event_auth.py | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index 66e198e25d10..5292e1356a3b 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -62,6 +62,38 @@ def check(
     Returns:
          if the auth checks pass.
     """
+    validate_event_for_room_version(room_version_obj, event, do_sig_check)
+    check_auth_rules_for_event(room_version_obj, event, auth_events)
+
+
+def validate_event_for_room_version(
+    room_version_obj: RoomVersion, event: EventBase, do_sig_check: bool = True
+) -> None:
+    """Ensure that the event complies with the limits, and has the right signatures
+
+    NB: does not *validate* the signatures - it assumes that any signatures present
+    have already been checked.
+
+    NB: it does not check that the event satisfies the auth rules (that is done in
+    check_auth_rules_for_event) - these tests are independent of the rest of the state
+    in the room.
+
+    NB: This is used to check events that have been received over federation. As such,
+    it can only enforce the checks specified in the relevant room version, to avoid
+    a split-brain situation where some servers accept such events, and others reject
+    them.
+
+    TODO: consider moving this into EventValidator
+
+    Args:
+        room_version_obj: the version of the room which contains this event
+        event: the event to be checked
+        do_sig_check: True if it should be verified that the sending server
+            signed the event.
+
+    Raises:
+        SynapseError if there is a problem with the event
+    """
     _check_size_limits(event)
 
     if not hasattr(event, "room_id"):
@@ -106,8 +138,6 @@ def check(
             if not event.signatures.get(authoriser_domain):
                 raise AuthError(403, "Event not signed by authorising server")
 
-    check_auth_rules_for_event(room_version_obj, event, auth_events)
-
 
 def check_auth_rules_for_event(
     room_version_obj: RoomVersion, event: EventBase, auth_events: StateMap[EventBase]

From 4a79f34208c05c338e6e58d6a9c56134b44a9526 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 14:10:11 +0100
Subject: [PATCH 4/8] Inline `event_auth.check`

---
 synapse/event_auth.py                | 25 -------------------------
 synapse/handlers/event_auth.py       |  9 ++++++---
 synapse/handlers/federation.py       |  7 ++++++-
 synapse/handlers/federation_event.py | 18 +++++++++++++-----
 4 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index 5292e1356a3b..665ced255075 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -41,31 +41,6 @@
 logger = logging.getLogger(__name__)
 
 
-def check(
-    room_version_obj: RoomVersion,
-    event: EventBase,
-    auth_events: StateMap[EventBase],
-    do_sig_check: bool = True,
-) -> None:
-    """Checks if this event is correctly authed.
-
-    Args:
-        room_version_obj: the version of the room
-        event: the event being checked.
-        auth_events: the existing room state.
-        do_sig_check: True if it should be verified that the sending server
-            signed the event.
-
-    Raises:
-        AuthError if the checks fail
-
-    Returns:
-         if the auth checks pass.
-    """
-    validate_event_for_room_version(room_version_obj, event, do_sig_check)
-    check_auth_rules_for_event(room_version_obj, event, auth_events)
-
-
 def validate_event_for_room_version(
     room_version_obj: RoomVersion, event: EventBase, do_sig_check: bool = True
 ) -> None:
diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
index cb81fa0986d5..9636c64ba0fb 100644
--- a/synapse/handlers/event_auth.py
+++ b/synapse/handlers/event_auth.py
@@ -23,6 +23,10 @@
 )
 from synapse.api.errors import AuthError, Codes, SynapseError
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
+from synapse.event_auth import (
+    check_auth_rules_for_event,
+    validate_event_for_room_version,
+)
 from synapse.events import EventBase
 from synapse.events.builder import EventBuilder
 from synapse.events.snapshot import EventContext
@@ -57,9 +61,8 @@ async def check_from_context(
         auth_events = {(e.type, e.state_key): e for e in auth_events_by_id.values()}
 
         room_version_obj = KNOWN_ROOM_VERSIONS[room_version]
-        event_auth.check(
-            room_version_obj, event, auth_events=auth_events, do_sig_check=do_sig_check
-        )
+        validate_event_for_room_version(room_version_obj, event, do_sig_check)
+        check_auth_rules_for_event(room_version_obj, event, auth_events)
 
     def compute_auth_events(
         self,
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 3b0b895b0777..654bfeb8cfc7 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -40,6 +40,10 @@
 )
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion, RoomVersions
 from synapse.crypto.event_signing import compute_event_signature
+from synapse.event_auth import (
+    check_auth_rules_for_event,
+    validate_event_for_room_version,
+)
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.events.validator import EventValidator
@@ -1168,7 +1172,8 @@ async def _persist_auth_tree(
                 auth_for_e[(EventTypes.Create, "")] = create_event
 
             try:
-                event_auth.check(room_version, e, auth_events=auth_for_e)
+                validate_event_for_room_version(room_version, e)
+                check_auth_rules_for_event(room_version, e, auth_for_e)
             except SynapseError as err:
                 # we may get SynapseErrors here as well as AuthErrors. For
                 # instance, there are a couple of (ancient) events in some
diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py
index 2c4644b4a32d..e587b5b3b351 100644
--- a/synapse/handlers/federation_event.py
+++ b/synapse/handlers/federation_event.py
@@ -29,7 +29,6 @@
 
 from prometheus_client import Counter
 
-from synapse import event_auth
 from synapse.api.constants import (
     EventContentFields,
     EventTypes,
@@ -47,7 +46,11 @@
     SynapseError,
 )
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
-from synapse.event_auth import auth_types_for_event
+from synapse.event_auth import (
+    auth_types_for_event,
+    check_auth_rules_for_event,
+    validate_event_for_room_version,
+)
 from synapse.events import EventBase
 from synapse.events.snapshot import EventContext
 from synapse.federation.federation_client import InvalidResponseError
@@ -1207,7 +1210,8 @@ def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]:
 
                 context = EventContext.for_outlier()
                 try:
-                    event_auth.check(room_version_obj, event, auth_events=auth)
+                    validate_event_for_room_version(room_version_obj, event)
+                    check_auth_rules_for_event(room_version_obj, event, auth)
                 except AuthError as e:
                     logger.warning("Rejecting %r because %s", event, e)
                     context.rejected = RejectedReason.AUTH_ERROR
@@ -1282,7 +1286,8 @@ async def _check_event_auth(
             auth_events_for_auth = calculated_auth_event_map
 
         try:
-            event_auth.check(room_version_obj, event, auth_events=auth_events_for_auth)
+            validate_event_for_room_version(room_version_obj, event)
+            check_auth_rules_for_event(room_version_obj, event, auth_events_for_auth)
         except AuthError as e:
             logger.warning("Failed auth resolution for %r because %s", event, e)
             context.rejected = RejectedReason.AUTH_ERROR
@@ -1394,7 +1399,10 @@ async def _check_for_soft_fail(
         }
 
         try:
-            event_auth.check(room_version_obj, event, auth_events=current_auth_events)
+            # TODO: skip the call to validate_event_for_room_version? we should already
+            #    have validated the event.
+            validate_event_for_room_version(room_version_obj, event)
+            check_auth_rules_for_event(room_version_obj, event, current_auth_events)
         except AuthError as e:
             logger.warning(
                 "Soft-failing %r (from %s) because %s",

From 733e474afe47f15daeab42c9d0d0cfdf0efa7584 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 15:15:08 +0100
Subject: [PATCH 5/8] Split call to `validate_event_for_room_version` out of
 `check_from_context`

... and rename to `check_auth_rules_from_context`, and make it take a
`RoomVersion` object.
---
 synapse/handlers/event_auth.py | 16 +++++-----------
 synapse/handlers/federation.py | 26 +++++++++++++++-----------
 synapse/handlers/message.py    |  6 ++++--
 synapse/handlers/room.py       |  6 ++++--
 4 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py
index 9636c64ba0fb..d089c56286a1 100644
--- a/synapse/handlers/event_auth.py
+++ b/synapse/handlers/event_auth.py
@@ -22,11 +22,8 @@
     RestrictedJoinRuleTypes,
 )
 from synapse.api.errors import AuthError, Codes, SynapseError
-from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
-from synapse.event_auth import (
-    check_auth_rules_for_event,
-    validate_event_for_room_version,
-)
+from synapse.api.room_versions import RoomVersion
+from synapse.event_auth import check_auth_rules_for_event
 from synapse.events import EventBase
 from synapse.events.builder import EventBuilder
 from synapse.events.snapshot import EventContext
@@ -49,19 +46,16 @@ def __init__(self, hs: "HomeServer"):
         self._store = hs.get_datastore()
         self._server_name = hs.hostname
 
-    async def check_from_context(
+    async def check_auth_rules_from_context(
         self,
-        room_version: str,
+        room_version_obj: RoomVersion,
         event: EventBase,
         context: EventContext,
-        do_sig_check: bool = True,
     ) -> None:
+        """Check an event passes the auth rules at its own auth events"""
         auth_event_ids = event.auth_event_ids()
         auth_events_by_id = await self._store.get_events(auth_event_ids)
         auth_events = {(e.type, e.state_key): e for e in auth_events_by_id.values()}
-
-        room_version_obj = KNOWN_ROOM_VERSIONS[room_version]
-        validate_event_for_room_version(room_version_obj, event, do_sig_check)
         check_auth_rules_for_event(room_version_obj, event, auth_events)
 
     def compute_auth_events(
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 654bfeb8cfc7..14c7ad03918c 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -746,10 +746,10 @@ async def on_make_join_request(
 
         # The remote hasn't signed it yet, obviously. We'll do the full checks
         # when we get the event back in `on_send_join_request`
-        await self._event_auth_handler.check_from_context(
-            room_version.identifier, event, context, do_sig_check=False
+        validate_event_for_room_version(room_version, event, do_sig_check=False)
+        await self._event_auth_handler.check_auth_rules_from_context(
+            room_version, event, context
         )
-
         return event
 
     async def on_invite_request(
@@ -920,8 +920,9 @@ async def on_make_leave_request(
         try:
             # The remote hasn't signed it yet, obviously. We'll do the full checks
             # when we get the event back in `on_send_leave_request`
-            await self._event_auth_handler.check_from_context(
-                room_version_obj.identifier, event, context, do_sig_check=False
+            validate_event_for_room_version(room_version_obj, event, do_sig_check=False)
+            await self._event_auth_handler.check_auth_rules_from_context(
+                room_version_obj, event, context
             )
         except AuthError as e:
             logger.warning("Failed to create new leave %r because %s", event, e)
@@ -982,8 +983,9 @@ async def on_make_knock_request(
         try:
             # The remote hasn't signed it yet, obviously. We'll do the full checks
             # when we get the event back in `on_send_knock_request`
-            await self._event_auth_handler.check_from_context(
-                room_version_obj.identifier, event, context, do_sig_check=False
+            validate_event_for_room_version(room_version_obj, event, do_sig_check=False)
+            await self._event_auth_handler.check_auth_rules_from_context(
+                room_version_obj, event, context
             )
         except AuthError as e:
             logger.warning("Failed to create new knock %r because %s", event, e)
@@ -1271,8 +1273,9 @@ async def exchange_third_party_invite(
             event.internal_metadata.send_on_behalf_of = self.hs.hostname
 
             try:
-                await self._event_auth_handler.check_from_context(
-                    room_version_obj.identifier, event, context
+                validate_event_for_room_version(room_version_obj, event)
+                await self._event_auth_handler.check_auth_rules_from_context(
+                    room_version_obj, event, context
                 )
             except AuthError as e:
                 logger.warning("Denying new third party invite %r because %s", event, e)
@@ -1322,8 +1325,9 @@ async def on_exchange_third_party_invite_request(
         )
 
         try:
-            await self._event_auth_handler.check_from_context(
-                room_version_obj.identifier, event, context
+            validate_event_for_room_version(room_version_obj, event)
+            await self._event_auth_handler.check_auth_rules_from_context(
+                room_version_obj, event, context
             )
         except AuthError as e:
             logger.warning("Denying third party invite %r because %s", event, e)
diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py
index 3b8cc50ec020..cdac53037cb3 100644
--- a/synapse/handlers/message.py
+++ b/synapse/handlers/message.py
@@ -44,6 +44,7 @@
 )
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions
 from synapse.api.urls import ConsentURIBuilder
+from synapse.event_auth import validate_event_for_room_version
 from synapse.events import EventBase
 from synapse.events.builder import EventBuilder
 from synapse.events.snapshot import EventContext
@@ -1098,8 +1099,9 @@ async def handle_new_client_event(
             assert event.content["membership"] == Membership.LEAVE
         else:
             try:
-                await self._event_auth_handler.check_from_context(
-                    room_version_obj.identifier, event, context
+                validate_event_for_room_version(room_version_obj, event)
+                await self._event_auth_handler.check_auth_rules_from_context(
+                    room_version_obj, event, context
                 )
             except AuthError as err:
                 logger.warning("Denying new event %r because %s", event, err)
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index bf8a85f563d8..873e08258ea0 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -52,6 +52,7 @@
 )
 from synapse.api.filtering import Filter
 from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
+from synapse.event_auth import validate_event_for_room_version
 from synapse.events import EventBase
 from synapse.events.utils import copy_power_levels_contents
 from synapse.rest.admin._base import assert_user_is_admin
@@ -238,8 +239,9 @@ async def _upgrade_room(
             },
         )
         old_room_version = await self.store.get_room_version(old_room_id)
-        await self._event_auth_handler.check_from_context(
-            old_room_version.identifier, tombstone_event, tombstone_context
+        validate_event_for_room_version(old_room_version, tombstone_event)
+        await self._event_auth_handler.check_auth_rules_from_context(
+            old_room_version, tombstone_event, tombstone_context
         )
 
         await self.clone_existing_room(

From 23112dd07557914997a858c6907cf1b3dfc6b969 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 15:21:14 +0100
Subject: [PATCH 6/8] Skip calls to validate_event_for_room_version for
 `make_*` functions

since we're not checking the signatures anyway, I think we can afford to skip
the whole thing.
---
 synapse/handlers/federation.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 14c7ad03918c..0a10a5c28aec 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -746,7 +746,6 @@ async def on_make_join_request(
 
         # The remote hasn't signed it yet, obviously. We'll do the full checks
         # when we get the event back in `on_send_join_request`
-        validate_event_for_room_version(room_version, event, do_sig_check=False)
         await self._event_auth_handler.check_auth_rules_from_context(
             room_version, event, context
         )
@@ -920,7 +919,6 @@ async def on_make_leave_request(
         try:
             # The remote hasn't signed it yet, obviously. We'll do the full checks
             # when we get the event back in `on_send_leave_request`
-            validate_event_for_room_version(room_version_obj, event, do_sig_check=False)
             await self._event_auth_handler.check_auth_rules_from_context(
                 room_version_obj, event, context
             )
@@ -983,7 +981,6 @@ async def on_make_knock_request(
         try:
             # The remote hasn't signed it yet, obviously. We'll do the full checks
             # when we get the event back in `on_send_knock_request`
-            validate_event_for_room_version(room_version_obj, event, do_sig_check=False)
             await self._event_auth_handler.check_auth_rules_from_context(
                 room_version_obj, event, context
             )

From 13f005db606f721ce5f22302ee3a5868425fd998 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 28 Sep 2021 15:27:38 +0100
Subject: [PATCH 7/8] remove redundant `do_sig_check` param on
 `validate_event_for_room_version`

This is now always true.
---
 synapse/event_auth.py | 74 +++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 38 deletions(-)

diff --git a/synapse/event_auth.py b/synapse/event_auth.py
index 665ced255075..eef354de6ea5 100644
--- a/synapse/event_auth.py
+++ b/synapse/event_auth.py
@@ -42,7 +42,7 @@
 
 
 def validate_event_for_room_version(
-    room_version_obj: RoomVersion, event: EventBase, do_sig_check: bool = True
+    room_version_obj: RoomVersion, event: EventBase
 ) -> None:
     """Ensure that the event complies with the limits, and has the right signatures
 
@@ -63,8 +63,6 @@ def validate_event_for_room_version(
     Args:
         room_version_obj: the version of the room which contains this event
         event: the event to be checked
-        do_sig_check: True if it should be verified that the sending server
-            signed the event.
 
     Raises:
         SynapseError if there is a problem with the event
@@ -74,44 +72,44 @@ def validate_event_for_room_version(
     if not hasattr(event, "room_id"):
         raise AuthError(500, "Event has no room_id: %s" % event)
 
-    if do_sig_check:
-        sender_domain = get_domain_from_id(event.sender)
+    # check that the event has the correct signatures
+    sender_domain = get_domain_from_id(event.sender)
 
-        is_invite_via_3pid = (
-            event.type == EventTypes.Member
-            and event.membership == Membership.INVITE
-            and "third_party_invite" in event.content
-        )
+    is_invite_via_3pid = (
+        event.type == EventTypes.Member
+        and event.membership == Membership.INVITE
+        and "third_party_invite" in event.content
+    )
 
-        # Check the sender's domain has signed the event
-        if not event.signatures.get(sender_domain):
-            # We allow invites via 3pid to have a sender from a different
-            # HS, as the sender must match the sender of the original
-            # 3pid invite. This is checked further down with the
-            # other dedicated membership checks.
-            if not is_invite_via_3pid:
-                raise AuthError(403, "Event not signed by sender's server")
-
-        if event.format_version in (EventFormatVersions.V1,):
-            # Only older room versions have event IDs to check.
-            event_id_domain = get_domain_from_id(event.event_id)
-
-            # Check the origin domain has signed the event
-            if not event.signatures.get(event_id_domain):
-                raise AuthError(403, "Event not signed by sending server")
-
-        is_invite_via_allow_rule = (
-            room_version_obj.msc3083_join_rules
-            and event.type == EventTypes.Member
-            and event.membership == Membership.JOIN
-            and "join_authorised_via_users_server" in event.content
+    # Check the sender's domain has signed the event
+    if not event.signatures.get(sender_domain):
+        # We allow invites via 3pid to have a sender from a different
+        # HS, as the sender must match the sender of the original
+        # 3pid invite. This is checked further down with the
+        # other dedicated membership checks.
+        if not is_invite_via_3pid:
+            raise AuthError(403, "Event not signed by sender's server")
+
+    if event.format_version in (EventFormatVersions.V1,):
+        # Only older room versions have event IDs to check.
+        event_id_domain = get_domain_from_id(event.event_id)
+
+        # Check the origin domain has signed the event
+        if not event.signatures.get(event_id_domain):
+            raise AuthError(403, "Event not signed by sending server")
+
+    is_invite_via_allow_rule = (
+        room_version_obj.msc3083_join_rules
+        and event.type == EventTypes.Member
+        and event.membership == Membership.JOIN
+        and "join_authorised_via_users_server" in event.content
+    )
+    if is_invite_via_allow_rule:
+        authoriser_domain = get_domain_from_id(
+            event.content["join_authorised_via_users_server"]
         )
-        if is_invite_via_allow_rule:
-            authoriser_domain = get_domain_from_id(
-                event.content["join_authorised_via_users_server"]
-            )
-            if not event.signatures.get(authoriser_domain):
-                raise AuthError(403, "Event not signed by authorising server")
+        if not event.signatures.get(authoriser_domain):
+            raise AuthError(403, "Event not signed by authorising server")
 
 
 def check_auth_rules_for_event(

From 0ee86f5865523a642053a41c856fb144c78a97cf Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Wed, 29 Sep 2021 12:52:21 +0100
Subject: [PATCH 8/8] changelog

---
 changelog.d/10940.misc | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/10940.misc

diff --git a/changelog.d/10940.misc b/changelog.d/10940.misc
new file mode 100644
index 000000000000..9a765435dbe4
--- /dev/null
+++ b/changelog.d/10940.misc
@@ -0,0 +1 @@
+Clean up some of the federation event authentication code for clarity.