From 85e1615b200e851a90e21369086a4dde3b8730cf Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:39:34 +0100 Subject: [PATCH 1/6] Rename a variable in `_update_auth_events_and_context_for_auth` ... because `e` is a terrible name. --- synapse/handlers/federation_event.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index ccbfce0219ee..a5ced42d318e 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1558,39 +1558,41 @@ async def _update_auth_events_and_context_for_auth( event.room_id, [e.event_id for e in remote_auth_chain] ) - for e in remote_auth_chain: - if e.event_id in seen_remotes: + for auth_event in remote_auth_chain: + if auth_event.event_id in seen_remotes: continue - if e.event_id == event.event_id: + if auth_event.event_id == event.event_id: continue try: - auth_ids = e.auth_event_ids() + auth_ids = auth_event.auth_event_ids() auth = { (e.type, e.state_key): e for e in remote_auth_chain if e.event_id in auth_ids or e.type == EventTypes.Create } - e.internal_metadata.outlier = True + auth_event.internal_metadata.outlier = True logger.debug( "_check_event_auth %s missing_auth: %s", event.event_id, - e.event_id, + auth_event.event_id, ) missing_auth_event_context = ( - await self._state_handler.compute_event_context(e) + await self._state_handler.compute_event_context(auth_event) ) await self._auth_and_persist_event( origin, - e, + auth_event, missing_auth_event_context, claimed_auth_event_map=auth, ) - if e.event_id in event_auth_events: - auth_events[(e.type, e.state_key)] = e + if auth_event.event_id in event_auth_events: + auth_events[ + (auth_event.type, auth_event.state_key) + ] = auth_event except AuthError: pass From 169c9207d11973212e346ada3bc73a847c360834 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:41:26 +0100 Subject: [PATCH 2/6] Inline a call to `_auth_and_persist_event` --- synapse/handlers/federation_event.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index a5ced42d318e..466aa259df68 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1582,12 +1582,16 @@ async def _update_auth_events_and_context_for_auth( missing_auth_event_context = ( await self._state_handler.compute_event_context(auth_event) ) - await self._auth_and_persist_event( + + missing_auth_event_context = await self._check_event_auth( origin, auth_event, missing_auth_event_context, claimed_auth_event_map=auth, ) + await self._run_push_actions_and_persist_event( + auth_event, missing_auth_event_context, backfilled=False + ) if auth_event.event_id in event_auth_events: auth_events[ From cf6a9f23d5dd3eced7e6451f700dfc01d442f442 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:44:43 +0100 Subject: [PATCH 3/6] Inline `_run_push_actions_and_persist_event` since we know this is an outlier, we can skip handling the push actions and skip straight to `persist_events_and_notify`. --- synapse/handlers/federation_event.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 466aa259df68..f9efcd6b262c 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1589,8 +1589,8 @@ async def _update_auth_events_and_context_for_auth( missing_auth_event_context, claimed_auth_event_map=auth, ) - await self._run_push_actions_and_persist_event( - auth_event, missing_auth_event_context, backfilled=False + await self.persist_events_and_notify( + event.room_id, [(auth_event, missing_auth_event_context)] ) if auth_event.event_id in event_auth_events: From b95c08f15d4e42d43bbd5972f691ac99e9483e84 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:49:39 +0100 Subject: [PATCH 4/6] Inline the other call to `_auth_and_persist_event` --- synapse/handlers/federation_event.py | 55 ++++------------------------ tests/test_federation.py | 15 ++++++-- 2 files changed, 20 insertions(+), 50 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index f9efcd6b262c..799aaf2e1a12 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -909,12 +909,18 @@ async def _process_received_pdu( context = await self._state_handler.compute_event_context( event, old_state=state ) - await self._auth_and_persist_event( - origin, event, context, state=state, backfilled=backfilled + context = await self._check_event_auth( + origin, + event, + context, + state=state, + backfilled=backfilled, ) except AuthError as e: raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) + await self._run_push_actions_and_persist_event(event, context, backfilled) + if backfilled: return @@ -1239,51 +1245,6 @@ async def prep(ev_info: _NewEventInfo): ], ) - async def _auth_and_persist_event( - self, - origin: str, - event: EventBase, - context: EventContext, - state: Optional[Iterable[EventBase]] = None, - claimed_auth_event_map: Optional[StateMap[EventBase]] = None, - backfilled: bool = False, - ) -> None: - """ - Process an event by performing auth checks and then persisting to the database. - - Args: - origin: The host the event originates from. - event: The event itself. - context: - The event context. - - state: - The state events used to check the event for soft-fail. If this is - not provided the current state events will be used. - - claimed_auth_event_map: - A map of (type, state_key) => event for the event's claimed auth_events. - Possibly incomplete, and possibly including events that are not yet - persisted, or authed, or in the right room. - - Only populated when populating outliers. - - backfilled: True if the event was backfilled. - """ - # claimed_auth_event_map should be given iff the event is an outlier - assert bool(claimed_auth_event_map) == event.internal_metadata.outlier - - context = await self._check_event_auth( - origin, - event, - context, - state=state, - claimed_auth_event_map=claimed_auth_event_map, - backfilled=backfilled, - ) - - await self._run_push_actions_and_persist_event(event, context, backfilled) - async def _check_event_auth( self, origin: str, diff --git a/tests/test_federation.py b/tests/test_federation.py index 61c9d7c2ef96..c51e018da1a1 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -76,9 +76,18 @@ def setUp(self): self.handler = self.homeserver.get_federation_handler() federation_event_handler = self.homeserver.get_federation_event_handler() - federation_event_handler._check_event_auth = lambda origin, event, context, state, claimed_auth_event_map, backfilled: succeed( - context - ) + + async def _check_event_auth( + origin, + event, + context, + state=None, + claimed_auth_event_map=None, + backfilled=False, + ): + return context + + federation_event_handler._check_event_auth = _check_event_auth self.client = self.homeserver.get_federation_client() self.client._check_sigs_and_hash_and_fetch = lambda dest, pdus, **k: succeed( pdus From c204971e66d348d37026b82b3bca59227ed1dede Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:56:27 +0100 Subject: [PATCH 5/6] Add an assertion to `_run_push_actions_and_persist_event` --- synapse/handlers/federation_event.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 799aaf2e1a12..9ec90ac8c101 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1700,10 +1700,13 @@ async def _run_push_actions_and_persist_event( context: The event context. backfilled: True if the event was backfilled. """ + # this method should not be called on outliers (those code paths call + # persist_events_and_notify directly.) + assert not event.internal_metadata.outlier + try: if ( - not event.internal_metadata.is_outlier() - and not backfilled + not backfilled and not context.rejected and (await self._store.get_min_depth(event.room_id)) <= event.depth ): From 232979224bded185ad036cd732a458ede17fc86a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 8 Sep 2021 13:57:18 +0100 Subject: [PATCH 6/6] changelog --- changelog.d/10781.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10781.misc diff --git a/changelog.d/10781.misc b/changelog.d/10781.misc new file mode 100644 index 000000000000..9a765435dbe4 --- /dev/null +++ b/changelog.d/10781.misc @@ -0,0 +1 @@ +Clean up some of the federation event authentication code for clarity.