From e4c3e7bdf1de35b30d7ee4c64b4c13e56c8acb3b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 10 Jan 2025 12:17:48 -0600 Subject: [PATCH 01/45] Add membership to event representation when logging --- synapse/events/__init__.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 2e56b671f06..8e9d27138ca 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -42,7 +42,7 @@ from typing_extensions import Literal from unpaddedbase64 import encode_base64 -from synapse.api.constants import RelationTypes +from synapse.api.constants import EventTypes, RelationTypes from synapse.api.room_versions import EventFormatVersions, RoomVersion, RoomVersions from synapse.synapse_rust.events import EventInternalMetadata from synapse.types import JsonDict, StrCollection @@ -325,12 +325,17 @@ def __str__(self) -> str: def __repr__(self) -> str: rejection = f"REJECTED={self.rejected_reason}, " if self.rejected_reason else "" + conditional_membership_string = "" + if self.get("type") == EventTypes.Member: + conditional_membership_string = f"membership={self.membership}, " + return ( f"<{self.__class__.__name__} " f"{rejection}" f"event_id={self.event_id}, " f"type={self.get('type')}, " f"state_key={self.get('state_key')}, " + f"{conditional_membership_string}" f"outlier={self.internal_metadata.is_outlier()}" ">" ) From d31ff534954469a4b79011854e8ce9ba093cfd94 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 10 Jan 2025 15:15:45 -0600 Subject: [PATCH 02/45] Debug logging --- scripts-dev/complement.sh | 18 +++++++++--------- synapse/event_auth.py | 6 ++++-- synapse/federation/federation_server.py | 5 ++++- synapse/handlers/federation.py | 3 +++ synapse/handlers/federation_event.py | 13 ++++++++++++- synapse/handlers/message.py | 6 +++++- synapse/notifier.py | 1 + 7 files changed, 38 insertions(+), 14 deletions(-) diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 6be9177f110..8c4008b1306 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -220,15 +220,15 @@ extra_test_args=() test_packages=( ./tests/csapi - ./tests - ./tests/msc3874 - ./tests/msc3890 - ./tests/msc3391 - ./tests/msc3757 - ./tests/msc3930 - ./tests/msc3902 - ./tests/msc3967 - ./tests/msc4140 + # ./tests + # ./tests/msc3874 + # ./tests/msc3890 + # ./tests/msc3391 + # ./tests/msc3757 + # ./tests/msc3930 + # ./tests/msc3902 + # ./tests/msc3967 + # ./tests/msc4140 ) # Enable dirty runs, so tests will reuse the same container where possible. diff --git a/synapse/event_auth.py b/synapse/event_auth.py index c208b900c53..ea3b6a0e2e6 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -563,9 +563,10 @@ def _is_membership_change_allowed( invite_level = get_named_level(auth_events, "invite", 0) ban_level = get_named_level(auth_events, "ban", 50) - logger.debug( + logger.info( "_is_membership_change_allowed: %s", { + "caller_membership": caller.membership if caller else None, "caller_in_room": caller_in_room, "caller_invited": caller_invited, "caller_knocked": caller_knocked, @@ -677,7 +678,8 @@ def _is_membership_change_allowed( and join_rule == JoinRules.KNOCK_RESTRICTED ) ): - if not caller_in_room and not caller_invited: + # You can only join the room if you are invited or are already in the room. + if not (caller_in_room or caller_invited): raise AuthError(403, "You are not invited to this room.") else: # TODO (erikj): may_join list diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 1932fa82a4a..ec34e282d65 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1256,7 +1256,7 @@ async def _process_incoming_pdus_in_room_inner( # has started processing). while True: async with lock: - logger.info("handling received PDU in room %s: %s", room_id, event) + logger.info("📬 handling received PDU in room %s: %s", room_id, event) try: with nested_logging_context(event.event_id): # We're taking out a lock within a lock, which could @@ -1271,6 +1271,9 @@ async def _process_incoming_pdus_in_room_inner( await self._federation_event_handler.on_receive_pdu( origin, event ) + logger.info( + "✅ handled received PDU in room %s: %s", room_id, event + ) except FederationError as e: # XXX: Ideally we'd inform the remote we failed to process # the event, but we can't return an error in the transaction diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 17dd4af13ed..4ee6f19c8bd 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1051,6 +1051,8 @@ async def on_invite_request( Respond with the now signed event. """ + logger.info("🗳️ on_invite_request: handling event %s", event) + if event.state_key is None: raise SynapseError(400, "The invite event did not have a state key") @@ -1127,6 +1129,7 @@ async def on_invite_request( await self.store.remove_push_actions_from_staging(event.event_id) raise + logger.info("✅ on_invite_request: handled event %s", event) return event async def do_remotely_reject_invite( diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index c85deaed562..d0ef43a753c 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -382,6 +382,10 @@ async def on_send_membership_event( event.event_id, event.signatures, ) + # logger.info( + # "📮 on_send_membership_event: received event: %s", + # event, + # ) if get_domain_from_id(event.sender) != origin: logger.info( @@ -436,6 +440,10 @@ async def on_send_membership_event( await self._check_for_soft_fail(event, context=context, origin=origin) await self._run_push_actions_and_persist_event(event, context) + # logger.info( + # "✅ on_send_membership_event: handled event: %s", + # event, + # ) return event, context async def check_join_restrictions( @@ -2274,7 +2282,8 @@ async def persist_events_and_notify( # After persistence we always need to notify replication there may # be new data. - self._notifier.notify_replication() + # XXX: This already happens in `_notify_persisted_event` -> `on_new_room_events` -> `notify_new_room_events` -> `notify_replication` + # self._notifier.notify_replication() if self._ephemeral_messages_enabled: for event in events: @@ -2292,6 +2301,8 @@ async def persist_events_and_notify( str(len(events)), ) for event in events: + # TODO: Is this correct? + # if not event.internal_metadata.is_outlier(): await self._notify_persisted_event(event, max_stream_token) return max_stream_token.stream diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index df3010ecf68..99766b3f23d 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1415,6 +1415,9 @@ async def handle_new_client_event( PartialStateConflictError if attempting to persist a partial state event in a room that has been un-partial stated. """ + for event, _ in events_and_context: + logger.info("📮 handle_new_client_event: handling %s", event) + extra_users = extra_users or [] for event, context in events_and_context: @@ -1460,7 +1463,7 @@ async def handle_new_client_event( event, batched_auth_events ) except AuthError as err: - logger.warning("Denying new event %r because %s", event, err) + logger.warning("❌ Denying new event %r because %s", event, err) raise err # Ensure that we can round trip before trying to persist in db @@ -1492,6 +1495,7 @@ async def handle_new_client_event( gather_results(deferreds, consumeErrors=True) ).addErrback(unwrapFirstError) + logger.info("✅ handle_new_client_event: handled %s", event) return result async def _persist_events( diff --git a/synapse/notifier.py b/synapse/notifier.py index 88f531182a0..b129bf49528 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -308,6 +308,7 @@ async def on_new_room_events( notify_new_room_events with the results.""" event_entries = [] for event, pos in events_and_pos: + logger.info("📨 Notifying about new event %s", event, exc_info=True) entry = self.create_pending_room_event_entry( pos, extra_users, From 1d64beb21e5c8859521619f812a6bb5cbd4f1479 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 10 Jan 2025 17:07:17 -0600 Subject: [PATCH 03/45] A couple more debug logs --- synapse/handlers/federation.py | 2 ++ synapse/storage/databases/main/events.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 4ee6f19c8bd..3a22b2fe216 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -606,6 +606,8 @@ async def do_invite_join( content: The event content to use for the join event. """ + logger.info("🧲 do_invite_join for %s in %s", joinee, room_id) + # TODO: We should be able to call this on workers, but the upgrading of # room stuff after join currently doesn't work on workers. # TODO: Before we relax this condition, we need to allow re-syncing of diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index a23aaf50962..11a7de1cc12 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2836,6 +2836,8 @@ def _store_room_members_txn( for backfilled events because backfilled events in the past do not affect the current local state. """ + for event in events: + logger.info("🔦 _store_room_members_txn update room_memberships: %s", event) self.db_pool.simple_insert_many_txn( txn, @@ -2892,6 +2894,10 @@ def _store_room_members_txn( Membership.LEAVE, ) + logger.info( + "🔦 _store_room_members_txn update local_current_membership: %s", + event, + ) self.db_pool.simple_upsert_txn( txn, table="local_current_membership", From a32c1ba40f4954bd2c81ca0b261be6f7655ecd1d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 10 Jan 2025 17:57:29 -0600 Subject: [PATCH 04/45] Pipe store access to `check_state_dependent_auth_rules` --- synapse/event_auth.py | 10 +++- synapse/handlers/event_auth.py | 2 +- synapse/handlers/federation_event.py | 8 +-- synapse/state/__init__.py | 27 ++++++---- synapse/state/v1.py | 55 +++++++++++++++----- synapse/state/v2.py | 18 ++++--- tests/handlers/test_federation_event.py | 1 + tests/state/test_v2.py | 28 +++++----- tests/test_event_auth.py | 68 ++++++++++++++++++++++--- 9 files changed, 160 insertions(+), 57 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index ea3b6a0e2e6..c0784ab4250 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -85,7 +85,7 @@ class _EventSourceStore(Protocol): async def get_events( self, event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, get_prev_content: bool = False, allow_rejected: bool = False, ) -> Dict[str, "EventBase"]: ... @@ -277,6 +277,7 @@ async def check_state_independent_auth_rules( def check_state_dependent_auth_rules( + store: _EventSourceStore, event: "EventBase", auth_events: Iterable["EventBase"], ) -> None: @@ -295,12 +296,19 @@ def check_state_dependent_auth_rules( a bunch of other tests (including, but not limited to, check_state_independent_auth_rules). Args: + store: the datastore; used to fetch the auth events for validation event: the event being checked. auth_events: the room state to check the events against. Raises: AuthError if the checks fail """ + logger.info( + "🛂 Checking state-dependent auth rules for %s auth_events=%s", + event, + auth_events, + ) + # there are no state-dependent auth rules for create events. if event.type == EventTypes.Create: logger.debug("Allowing! %s", event) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index c4dbf22408b..337c51428f9 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -83,7 +83,7 @@ async def check_auth_rules_from_context( else: auth_events_by_id = await self._store.get_events(auth_event_ids) - check_state_dependent_auth_rules(event, auth_events_by_id.values()) + check_state_dependent_auth_rules(self._store, event, auth_events_by_id.values()) def compute_auth_events( self, diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index d0ef43a753c..86e51cbf650 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1749,7 +1749,7 @@ async def prep(event: EventBase) -> None: await check_state_independent_auth_rules( self._store, event, batched_auth_events=auth_map ) - check_state_dependent_auth_rules(event, auth) + check_state_dependent_auth_rules(self._store, event, auth) except AuthError as e: logger.warning("Rejecting %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -1849,7 +1849,7 @@ async def _check_event_auth( # otherwise it is rejected. try: await check_state_independent_auth_rules(self._store, event) - check_state_dependent_auth_rules(event, claimed_auth_events) + check_state_dependent_auth_rules(self._store, event, claimed_auth_events) except AuthError as e: logger.warning( "While checking auth of %r against auth_events: %s", event, e @@ -1930,7 +1930,7 @@ async def _check_event_auth( ) try: - check_state_dependent_auth_rules(event, calculated_auth_events) + check_state_dependent_auth_rules(self._store, event, calculated_auth_events) except AuthError as e: logger.warning( "While checking auth of %r against room state before the event: %s", @@ -2047,7 +2047,7 @@ async def _check_for_soft_fail( ) try: - check_state_dependent_auth_rules(event, current_auth_events) + check_state_dependent_auth_rules(self._store, event, current_auth_events) except AuthError as e: logger.warning( "Soft-failing %r (from %s) because %s", diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index 72b291889bb..e597f117edd 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -24,7 +24,6 @@ from typing import ( TYPE_CHECKING, Any, - Awaitable, Callable, DefaultDict, Dict, @@ -732,7 +731,7 @@ async def resolve_events_with_store( room_version_obj, state_sets, event_map, - state_res_store.get_events, + state_res_store, ) else: return await v2.resolve_events_with_store( @@ -886,9 +885,13 @@ class StateResolutionStore: store: "DataStore" - def get_events( - self, event_ids: StrCollection, allow_rejected: bool = False - ) -> Awaitable[Dict[str, EventBase]]: + async def get_events( + self, + event_ids: StrCollection, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + get_prev_content: bool = False, + allow_rejected: bool = False, + ) -> Dict[str, EventBase]: """Get events from the database Args: @@ -899,16 +902,18 @@ def get_events( An awaitable which resolves to a dict from event_id to event. """ - return self.store.get_events( + return await self.store.get_events( event_ids, - redact_behaviour=EventRedactBehaviour.as_is, - get_prev_content=False, + # TODO: Is `EventRedactBehaviour.as_is` important here? We're changing it to + # the default of `EventRedactBehaviour.redact` + redact_behaviour=redact_behaviour, + get_prev_content=get_prev_content, allow_rejected=allow_rejected, ) - def get_auth_chain_difference( + async def get_auth_chain_difference( self, room_id: str, state_sets: List[Set[str]] - ) -> Awaitable[Set[str]]: + ) -> Set[str]: """Given sets of state events figure out the auth chain difference (as per state res v2 algorithm). @@ -920,4 +925,4 @@ def get_auth_chain_difference( An awaitable that resolves to a set of event IDs. """ - return self.store.get_auth_chain_difference(room_id, state_sets) + return await self.store.get_auth_chain_difference(room_id, state_sets) diff --git a/synapse/state/v1.py b/synapse/state/v1.py index a2e9eb0a42a..9340eabfd67 100644 --- a/synapse/state/v1.py +++ b/synapse/state/v1.py @@ -21,8 +21,6 @@ import hashlib import logging from typing import ( - Awaitable, - Callable, Dict, Iterable, List, @@ -32,11 +30,14 @@ Tuple, ) +from typing_extensions import Protocol + from synapse import event_auth from synapse.api.constants import EventTypes from synapse.api.errors import AuthError from synapse.api.room_versions import RoomVersion from synapse.events import EventBase +from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.types import MutableStateMap, StateMap, StrCollection logger = logging.getLogger(__name__) @@ -45,12 +46,24 @@ POWER_KEY = (EventTypes.PowerLevels, "") +class StateResolutionStore(Protocol): + # This is usually synapse.state.StateResolutionStore, but it's replaced with a + # TestStateResolutionStore in tests. + async def get_events( + self, + event_ids: StrCollection, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + get_prev_content: bool = False, + allow_rejected: bool = False, + ) -> Dict[str, EventBase]: ... + + async def resolve_events_with_store( room_id: str, room_version: RoomVersion, state_sets: Sequence[StateMap[str]], event_map: Optional[Dict[str, EventBase]], - state_map_factory: Callable[[StrCollection], Awaitable[Dict[str, EventBase]]], + state_res_store: StateResolutionStore, ) -> StateMap[str]: """ Args: @@ -92,7 +105,7 @@ async def resolve_events_with_store( # A map from state event id to event. Only includes the state events which # are in conflict (and those in event_map). - state_map = await state_map_factory(needed_events) + state_map = await state_res_store.get_events(needed_events) if event_map is not None: state_map.update(event_map) @@ -124,7 +137,7 @@ async def resolve_events_with_store( "Asking for %d/%d auth events", len(new_needed_events), new_needed_event_count ) - state_map_new = await state_map_factory(new_needed_events) + state_map_new = await state_res_store.get_events(new_needed_events) for event in state_map_new.values(): if event.room_id != room_id: raise Exception( @@ -139,7 +152,12 @@ async def resolve_events_with_store( state_map.update(state_map_new) return _resolve_with_state( - room_version, unconflicted_state, conflicted_state, auth_events, state_map + room_version, + unconflicted_state, + conflicted_state, + auth_events, + state_map, + state_res_store, ) @@ -231,6 +249,7 @@ def _resolve_with_state( conflicted_state_ids: StateMap[Set[str]], auth_event_ids: StateMap[str], state_map: Dict[str, EventBase], + state_res_store: StateResolutionStore, ) -> MutableStateMap[str]: conflicted_state = {} for key, event_ids in conflicted_state_ids.items(): @@ -248,7 +267,7 @@ def _resolve_with_state( try: resolved_state = _resolve_state_events( - room_version, conflicted_state, auth_events + room_version, conflicted_state, auth_events, state_res_store ) except Exception: logger.exception("Failed to resolve state") @@ -265,6 +284,7 @@ def _resolve_state_events( room_version: RoomVersion, conflicted_state: StateMap[List[EventBase]], auth_events: MutableStateMap[EventBase], + state_res_store: StateResolutionStore, ) -> StateMap[EventBase]: """This is where we actually decide which of the conflicted state to use. @@ -280,7 +300,7 @@ def _resolve_state_events( events = conflicted_state[POWER_KEY] logger.debug("Resolving conflicted power levels %r", events) resolved_state[POWER_KEY] = _resolve_auth_events( - room_version, events, auth_events + room_version, events, auth_events, state_res_store ) auth_events.update(resolved_state) @@ -289,7 +309,7 @@ def _resolve_state_events( if key[0] == EventTypes.JoinRules: logger.debug("Resolving conflicted join rules %r", events) resolved_state[key] = _resolve_auth_events( - room_version, events, auth_events + room_version, events, auth_events, state_res_store ) auth_events.update(resolved_state) @@ -298,7 +318,7 @@ def _resolve_state_events( if key[0] == EventTypes.Member: logger.debug("Resolving conflicted member lists %r", events) resolved_state[key] = _resolve_auth_events( - room_version, events, auth_events + room_version, events, auth_events, state_res_store ) auth_events.update(resolved_state) @@ -306,13 +326,18 @@ def _resolve_state_events( for key, events in conflicted_state.items(): if key not in resolved_state: logger.debug("Resolving conflicted state %r:%r", key, events) - resolved_state[key] = _resolve_normal_events(events, auth_events) + resolved_state[key] = _resolve_normal_events( + events, auth_events, state_res_store + ) return resolved_state def _resolve_auth_events( - room_version: RoomVersion, events: List[EventBase], auth_events: StateMap[EventBase] + room_version: RoomVersion, + events: List[EventBase], + auth_events: StateMap[EventBase], + state_res_store: StateResolutionStore, ) -> EventBase: reverse = list(reversed(_ordered_events(events))) @@ -336,6 +361,7 @@ def _resolve_auth_events( try: # The signatures have already been checked at this point event_auth.check_state_dependent_auth_rules( + state_res_store, event, auth_events.values(), ) @@ -347,12 +373,15 @@ def _resolve_auth_events( def _resolve_normal_events( - events: List[EventBase], auth_events: StateMap[EventBase] + events: List[EventBase], + auth_events: StateMap[EventBase], + state_res_store: StateResolutionStore, ) -> EventBase: for event in _ordered_events(events): try: # The signatures have already been checked at this point event_auth.check_state_dependent_auth_rules( + state_res_store, event, auth_events.values(), ) diff --git a/synapse/state/v2.py b/synapse/state/v2.py index da926ad1464..b778f019be2 100644 --- a/synapse/state/v2.py +++ b/synapse/state/v2.py @@ -43,6 +43,7 @@ from synapse.api.errors import AuthError from synapse.api.room_versions import RoomVersion from synapse.events import EventBase +from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.types import MutableStateMap, StateMap, StrCollection logger = logging.getLogger(__name__) @@ -58,13 +59,17 @@ def sleep(self, duration_ms: float) -> Awaitable[None]: ... class StateResolutionStore(Protocol): # This is usually synapse.state.StateResolutionStore, but it's replaced with a # TestStateResolutionStore in tests. - def get_events( - self, event_ids: StrCollection, allow_rejected: bool = False - ) -> Awaitable[Dict[str, EventBase]]: ... - - def get_auth_chain_difference( + async def get_events( + self, + event_ids: StrCollection, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + get_prev_content: bool = False, + allow_rejected: bool = False, + ) -> Dict[str, EventBase]: ... + + async def get_auth_chain_difference( self, room_id: str, state_sets: List[Set[str]] - ) -> Awaitable[Set[str]]: ... + ) -> Set[str]: ... # We want to await to the reactor occasionally during state res when dealing @@ -595,6 +600,7 @@ async def _iterative_auth_checks( try: event_auth.check_state_dependent_auth_rules( + state_res_store, event, auth_events.values(), ) diff --git a/tests/handlers/test_federation_event.py b/tests/handlers/test_federation_event.py index 5db10fa74c2..234ced96f42 100644 --- a/tests/handlers/test_federation_event.py +++ b/tests/handlers/test_federation_event.py @@ -938,6 +938,7 @@ def test_process_pulled_event_with_rejected_missing_state(self) -> None: AuthError, ) check_state_dependent_auth_rules( + main_store, rejected_kick_event, [create_event, power_levels_event, other_member_event, bert_member_event], ) diff --git a/tests/state/test_v2.py b/tests/state/test_v2.py index 0b70f779d9d..ba76a16351b 100644 --- a/tests/state/test_v2.py +++ b/tests/state/test_v2.py @@ -20,7 +20,6 @@ import itertools from typing import ( - Collection, Dict, Iterable, List, @@ -44,7 +43,8 @@ lexicographical_topological_sort, resolve_events_with_store, ) -from synapse.types import EventID, StateMap +from synapse.storage.databases.main.events_worker import EventRedactBehaviour +from synapse.types import EventID, StateMap, StrCollection from tests import unittest @@ -878,9 +878,13 @@ def pairwise(iterable: Iterable[T]) -> Iterable[Tuple[T, T]]: class TestStateResolutionStore: event_map: Dict[str, EventBase] = attr.ib() - def get_events( - self, event_ids: Collection[str], allow_rejected: bool = False - ) -> "defer.Deferred[Dict[str, EventBase]]": + async def get_events( + self, + event_ids: StrCollection, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + get_prev_content: bool = False, + allow_rejected: bool = False, + ) -> Dict[str, EventBase]: """Get events from the database Args: @@ -891,9 +895,7 @@ def get_events( Dict from event_id to event. """ - return defer.succeed( - {eid: self.event_map[eid] for eid in event_ids if eid in self.event_map} - ) + return {eid: self.event_map[eid] for eid in event_ids if eid in self.event_map} def _get_auth_chain(self, event_ids: Iterable[str]) -> List[str]: """Gets the full auth chain for a set of events (including rejected @@ -929,10 +931,10 @@ def _get_auth_chain(self, event_ids: Iterable[str]) -> List[str]: return list(result) - def get_auth_chain_difference( - self, room_id: str, auth_sets: List[Set[str]] - ) -> "defer.Deferred[Set[str]]": - chains = [frozenset(self._get_auth_chain(a)) for a in auth_sets] + async def get_auth_chain_difference( + self, room_id: str, state_sets: List[Set[str]] + ) -> Set[str]: + chains = [frozenset(self._get_auth_chain(a)) for a in state_sets] common = set(chains[0]).intersection(*chains[1:]) - return defer.succeed(set(chains[0]).union(*chains[1:]) - common) + return set(chains[0]).union(*chains[1:]) - common diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index f12402f5f2c..f85cedd0fef 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -20,7 +20,7 @@ # import unittest -from typing import Any, Collection, Dict, Iterable, List, Optional +from typing import Any, Dict, Iterable, List, Optional from parameterized import parameterized @@ -30,7 +30,7 @@ from synapse.api.room_versions import EventFormatVersions, RoomVersion, RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.storage.databases.main.events_worker import EventRedactBehaviour -from synapse.types import JsonDict, get_domain_from_id +from synapse.types import JsonDict, StrCollection, get_domain_from_id from tests.test_utils import get_awaitable_result @@ -50,14 +50,16 @@ def add_events(self, events: Iterable[EventBase]) -> None: async def get_events( self, - event_ids: Collection[str], - redact_behaviour: EventRedactBehaviour, + event_ids: StrCollection, + redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, get_prev_content: bool = False, allow_rejected: bool = False, ) -> Dict[str, EventBase]: assert allow_rejected assert not get_prev_content - assert redact_behaviour == EventRedactBehaviour.as_is + # TODO: Is `EventRedactBehaviour.as_is` important here? We're changing it to + # the default of `EventRedactBehaviour.redact` + # assert redact_behaviour == EventRedactBehaviour.as_is results = {} for e in event_ids: if e in self._store: @@ -84,7 +86,7 @@ def test_rejected_auth_events(self) -> None: get_awaitable_result( event_auth.check_state_independent_auth_rules(event_store, event) ) - event_auth.check_state_dependent_auth_rules(event, auth_events) + event_auth.check_state_dependent_auth_rules(event_store, event, auth_events) # ... but a rejected join_rules event should cause it to be rejected rejected_join_rules = _join_rules_event( @@ -256,8 +258,11 @@ def test_random_users_cannot_send_state_before_first_pl(self) -> None: _join_event(RoomVersions.V1, joiner), ] + event_store = _StubEventSourceStore() + # creator should be able to send state event_auth.check_state_dependent_auth_rules( + event_store, _random_state_event(RoomVersions.V1, creator), auth_events, ) @@ -266,6 +271,7 @@ def test_random_users_cannot_send_state_before_first_pl(self) -> None: self.assertRaises( AuthError, event_auth.check_state_dependent_auth_rules, + event_store, _random_state_event(RoomVersions.V1, joiner), auth_events, ) @@ -291,11 +297,14 @@ def test_state_default_level(self) -> None: _join_event(RoomVersions.V1, king), ] + event_store = _StubEventSourceStore() + # pleb should not be able to send state ( self.assertRaises( AuthError, event_auth.check_state_dependent_auth_rules, + event_store, _random_state_event(RoomVersions.V1, pleb), auth_events, ), @@ -303,6 +312,7 @@ def test_state_default_level(self) -> None: # king should be able to send state event_auth.check_state_dependent_auth_rules( + event_store, _random_state_event(RoomVersions.V1, king), auth_events, ) @@ -315,9 +325,11 @@ def test_alias_event(self) -> None: _create_event(RoomVersions.V1, creator), _join_event(RoomVersions.V1, creator), ] + event_store = _StubEventSourceStore() # creator should be able to send aliases event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V1, creator), auth_events, ) @@ -325,6 +337,7 @@ def test_alias_event(self) -> None: # Reject an event with no state key. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V1, creator, state_key=""), auth_events, ) @@ -332,12 +345,14 @@ def test_alias_event(self) -> None: # If the domain of the sender does not match the state key, reject. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V1, creator, state_key="test.com"), auth_events, ) # Note that the member does *not* need to be in the room. event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V1, other), auth_events, ) @@ -350,19 +365,23 @@ def test_msc2432_alias_event(self) -> None: _create_event(RoomVersions.V6, creator), _join_event(RoomVersions.V6, creator), ] + event_store = _StubEventSourceStore() # creator should be able to send aliases event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V6, creator), auth_events, ) # No particular checks are done on the state key. event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V6, creator, state_key=""), auth_events, ) event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V6, creator, state_key="test.com"), auth_events, ) @@ -370,6 +389,7 @@ def test_msc2432_alias_event(self) -> None: # Per standard auth rules, the member must be in the room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _alias_event(RoomVersions.V6, other), auth_events, ) @@ -397,14 +417,20 @@ def test_notifications( room_version, pleb, {"notifications": {"room": 100}} ) + event_store = _StubEventSourceStore() + # on room V1, pleb should be able to modify the notifications power level. if allow_modification: - event_auth.check_state_dependent_auth_rules(pl_event, auth_events) + event_auth.check_state_dependent_auth_rules( + event_store, pl_event, auth_events + ) else: # But an MSC2209 room rejects this change. with self.assertRaises(AuthError): - event_auth.check_state_dependent_auth_rules(pl_event, auth_events) + event_auth.check_state_dependent_auth_rules( + event_store, pl_event, auth_events + ) def test_join_rules_public(self) -> None: """ @@ -420,9 +446,11 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, creator, "public" ), } + event_store = _StubEventSourceStore() # Check join. event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -430,6 +458,7 @@ def test_join_rules_public(self) -> None: # A user cannot be force-joined to a room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _member_event(RoomVersions.V6, pleb, "join", sender=creator), auth_events.values(), ) @@ -440,6 +469,7 @@ def test_join_rules_public(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -449,6 +479,7 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "leave" ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -458,6 +489,7 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "join" ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -467,6 +499,7 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -485,10 +518,12 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, creator, "invite" ), } + event_store = _StubEventSourceStore() # A join without an invite is rejected. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -496,6 +531,7 @@ def test_join_rules_invite(self) -> None: # A user cannot be force-joined to a room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _member_event(RoomVersions.V6, pleb, "join", sender=creator), auth_events.values(), ) @@ -506,6 +542,7 @@ def test_join_rules_invite(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -516,6 +553,7 @@ def test_join_rules_invite(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -525,6 +563,7 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, pleb, "join" ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -534,6 +573,7 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -553,9 +593,11 @@ def test_join_rules_restricted_old_room(self) -> None: RoomVersions.V6, creator, "restricted" ), } + event_store = _StubEventSourceStore() with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -583,6 +625,7 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, creator, "restricted" ), } + event_store = _StubEventSourceStore() # A properly formatted join event should work. authorised_join_event = _join_event( @@ -593,6 +636,7 @@ def test_join_rules_msc3083_restricted(self) -> None: }, ) event_auth.check_state_dependent_auth_rules( + event_store, authorised_join_event, auth_events.values(), ) @@ -609,6 +653,7 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, "@inviter:foo.test" ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event( RoomVersions.V8, pleb, @@ -622,6 +667,7 @@ def test_join_rules_msc3083_restricted(self) -> None: # A join which is missing an authorised server is rejected. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) @@ -635,6 +681,7 @@ def test_join_rules_msc3083_restricted(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _join_event( RoomVersions.V8, pleb, @@ -649,6 +696,7 @@ def test_join_rules_msc3083_restricted(self) -> None: # *would* be valid, but is sent be a different user.) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, _member_event( RoomVersions.V8, pleb, @@ -667,6 +715,7 @@ def test_join_rules_msc3083_restricted(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( + event_store, authorised_join_event, auth_events.values(), ) @@ -676,6 +725,7 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "leave" ) event_auth.check_state_dependent_auth_rules( + event_store, authorised_join_event, auth_events.values(), ) @@ -686,6 +736,7 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "join" ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) @@ -696,6 +747,7 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( + event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) From 9a351554173d7df623b0f74672b153beb3b9cc5b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jan 2025 18:06:14 -0600 Subject: [PATCH 05/45] Revert "Pipe store access to `check_state_dependent_auth_rules`" This reverts commit a32c1ba40f4954bd2c81ca0b261be6f7655ecd1d. --- synapse/event_auth.py | 10 +--- synapse/handlers/event_auth.py | 2 +- synapse/handlers/federation_event.py | 8 +-- synapse/state/__init__.py | 27 ++++------ synapse/state/v1.py | 55 +++++--------------- synapse/state/v2.py | 18 +++---- tests/handlers/test_federation_event.py | 1 - tests/state/test_v2.py | 28 +++++----- tests/test_event_auth.py | 68 +++---------------------- 9 files changed, 57 insertions(+), 160 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index c0784ab4250..ea3b6a0e2e6 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -85,7 +85,7 @@ class _EventSourceStore(Protocol): async def get_events( self, event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + redact_behaviour: EventRedactBehaviour, get_prev_content: bool = False, allow_rejected: bool = False, ) -> Dict[str, "EventBase"]: ... @@ -277,7 +277,6 @@ async def check_state_independent_auth_rules( def check_state_dependent_auth_rules( - store: _EventSourceStore, event: "EventBase", auth_events: Iterable["EventBase"], ) -> None: @@ -296,19 +295,12 @@ def check_state_dependent_auth_rules( a bunch of other tests (including, but not limited to, check_state_independent_auth_rules). Args: - store: the datastore; used to fetch the auth events for validation event: the event being checked. auth_events: the room state to check the events against. Raises: AuthError if the checks fail """ - logger.info( - "🛂 Checking state-dependent auth rules for %s auth_events=%s", - event, - auth_events, - ) - # there are no state-dependent auth rules for create events. if event.type == EventTypes.Create: logger.debug("Allowing! %s", event) diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index 337c51428f9..c4dbf22408b 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -83,7 +83,7 @@ async def check_auth_rules_from_context( else: auth_events_by_id = await self._store.get_events(auth_event_ids) - check_state_dependent_auth_rules(self._store, event, auth_events_by_id.values()) + check_state_dependent_auth_rules(event, auth_events_by_id.values()) def compute_auth_events( self, diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 86e51cbf650..d0ef43a753c 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1749,7 +1749,7 @@ async def prep(event: EventBase) -> None: await check_state_independent_auth_rules( self._store, event, batched_auth_events=auth_map ) - check_state_dependent_auth_rules(self._store, event, auth) + check_state_dependent_auth_rules(event, auth) except AuthError as e: logger.warning("Rejecting %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR @@ -1849,7 +1849,7 @@ async def _check_event_auth( # otherwise it is rejected. try: await check_state_independent_auth_rules(self._store, event) - check_state_dependent_auth_rules(self._store, event, claimed_auth_events) + check_state_dependent_auth_rules(event, claimed_auth_events) except AuthError as e: logger.warning( "While checking auth of %r against auth_events: %s", event, e @@ -1930,7 +1930,7 @@ async def _check_event_auth( ) try: - check_state_dependent_auth_rules(self._store, event, calculated_auth_events) + check_state_dependent_auth_rules(event, calculated_auth_events) except AuthError as e: logger.warning( "While checking auth of %r against room state before the event: %s", @@ -2047,7 +2047,7 @@ async def _check_for_soft_fail( ) try: - check_state_dependent_auth_rules(self._store, event, current_auth_events) + check_state_dependent_auth_rules(event, current_auth_events) except AuthError as e: logger.warning( "Soft-failing %r (from %s) because %s", diff --git a/synapse/state/__init__.py b/synapse/state/__init__.py index e597f117edd..72b291889bb 100644 --- a/synapse/state/__init__.py +++ b/synapse/state/__init__.py @@ -24,6 +24,7 @@ from typing import ( TYPE_CHECKING, Any, + Awaitable, Callable, DefaultDict, Dict, @@ -731,7 +732,7 @@ async def resolve_events_with_store( room_version_obj, state_sets, event_map, - state_res_store, + state_res_store.get_events, ) else: return await v2.resolve_events_with_store( @@ -885,13 +886,9 @@ class StateResolutionStore: store: "DataStore" - async def get_events( - self, - event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, - get_prev_content: bool = False, - allow_rejected: bool = False, - ) -> Dict[str, EventBase]: + def get_events( + self, event_ids: StrCollection, allow_rejected: bool = False + ) -> Awaitable[Dict[str, EventBase]]: """Get events from the database Args: @@ -902,18 +899,16 @@ async def get_events( An awaitable which resolves to a dict from event_id to event. """ - return await self.store.get_events( + return self.store.get_events( event_ids, - # TODO: Is `EventRedactBehaviour.as_is` important here? We're changing it to - # the default of `EventRedactBehaviour.redact` - redact_behaviour=redact_behaviour, - get_prev_content=get_prev_content, + redact_behaviour=EventRedactBehaviour.as_is, + get_prev_content=False, allow_rejected=allow_rejected, ) - async def get_auth_chain_difference( + def get_auth_chain_difference( self, room_id: str, state_sets: List[Set[str]] - ) -> Set[str]: + ) -> Awaitable[Set[str]]: """Given sets of state events figure out the auth chain difference (as per state res v2 algorithm). @@ -925,4 +920,4 @@ async def get_auth_chain_difference( An awaitable that resolves to a set of event IDs. """ - return await self.store.get_auth_chain_difference(room_id, state_sets) + return self.store.get_auth_chain_difference(room_id, state_sets) diff --git a/synapse/state/v1.py b/synapse/state/v1.py index 9340eabfd67..a2e9eb0a42a 100644 --- a/synapse/state/v1.py +++ b/synapse/state/v1.py @@ -21,6 +21,8 @@ import hashlib import logging from typing import ( + Awaitable, + Callable, Dict, Iterable, List, @@ -30,14 +32,11 @@ Tuple, ) -from typing_extensions import Protocol - from synapse import event_auth from synapse.api.constants import EventTypes from synapse.api.errors import AuthError from synapse.api.room_versions import RoomVersion from synapse.events import EventBase -from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.types import MutableStateMap, StateMap, StrCollection logger = logging.getLogger(__name__) @@ -46,24 +45,12 @@ POWER_KEY = (EventTypes.PowerLevels, "") -class StateResolutionStore(Protocol): - # This is usually synapse.state.StateResolutionStore, but it's replaced with a - # TestStateResolutionStore in tests. - async def get_events( - self, - event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, - get_prev_content: bool = False, - allow_rejected: bool = False, - ) -> Dict[str, EventBase]: ... - - async def resolve_events_with_store( room_id: str, room_version: RoomVersion, state_sets: Sequence[StateMap[str]], event_map: Optional[Dict[str, EventBase]], - state_res_store: StateResolutionStore, + state_map_factory: Callable[[StrCollection], Awaitable[Dict[str, EventBase]]], ) -> StateMap[str]: """ Args: @@ -105,7 +92,7 @@ async def resolve_events_with_store( # A map from state event id to event. Only includes the state events which # are in conflict (and those in event_map). - state_map = await state_res_store.get_events(needed_events) + state_map = await state_map_factory(needed_events) if event_map is not None: state_map.update(event_map) @@ -137,7 +124,7 @@ async def resolve_events_with_store( "Asking for %d/%d auth events", len(new_needed_events), new_needed_event_count ) - state_map_new = await state_res_store.get_events(new_needed_events) + state_map_new = await state_map_factory(new_needed_events) for event in state_map_new.values(): if event.room_id != room_id: raise Exception( @@ -152,12 +139,7 @@ async def resolve_events_with_store( state_map.update(state_map_new) return _resolve_with_state( - room_version, - unconflicted_state, - conflicted_state, - auth_events, - state_map, - state_res_store, + room_version, unconflicted_state, conflicted_state, auth_events, state_map ) @@ -249,7 +231,6 @@ def _resolve_with_state( conflicted_state_ids: StateMap[Set[str]], auth_event_ids: StateMap[str], state_map: Dict[str, EventBase], - state_res_store: StateResolutionStore, ) -> MutableStateMap[str]: conflicted_state = {} for key, event_ids in conflicted_state_ids.items(): @@ -267,7 +248,7 @@ def _resolve_with_state( try: resolved_state = _resolve_state_events( - room_version, conflicted_state, auth_events, state_res_store + room_version, conflicted_state, auth_events ) except Exception: logger.exception("Failed to resolve state") @@ -284,7 +265,6 @@ def _resolve_state_events( room_version: RoomVersion, conflicted_state: StateMap[List[EventBase]], auth_events: MutableStateMap[EventBase], - state_res_store: StateResolutionStore, ) -> StateMap[EventBase]: """This is where we actually decide which of the conflicted state to use. @@ -300,7 +280,7 @@ def _resolve_state_events( events = conflicted_state[POWER_KEY] logger.debug("Resolving conflicted power levels %r", events) resolved_state[POWER_KEY] = _resolve_auth_events( - room_version, events, auth_events, state_res_store + room_version, events, auth_events ) auth_events.update(resolved_state) @@ -309,7 +289,7 @@ def _resolve_state_events( if key[0] == EventTypes.JoinRules: logger.debug("Resolving conflicted join rules %r", events) resolved_state[key] = _resolve_auth_events( - room_version, events, auth_events, state_res_store + room_version, events, auth_events ) auth_events.update(resolved_state) @@ -318,7 +298,7 @@ def _resolve_state_events( if key[0] == EventTypes.Member: logger.debug("Resolving conflicted member lists %r", events) resolved_state[key] = _resolve_auth_events( - room_version, events, auth_events, state_res_store + room_version, events, auth_events ) auth_events.update(resolved_state) @@ -326,18 +306,13 @@ def _resolve_state_events( for key, events in conflicted_state.items(): if key not in resolved_state: logger.debug("Resolving conflicted state %r:%r", key, events) - resolved_state[key] = _resolve_normal_events( - events, auth_events, state_res_store - ) + resolved_state[key] = _resolve_normal_events(events, auth_events) return resolved_state def _resolve_auth_events( - room_version: RoomVersion, - events: List[EventBase], - auth_events: StateMap[EventBase], - state_res_store: StateResolutionStore, + room_version: RoomVersion, events: List[EventBase], auth_events: StateMap[EventBase] ) -> EventBase: reverse = list(reversed(_ordered_events(events))) @@ -361,7 +336,6 @@ def _resolve_auth_events( try: # The signatures have already been checked at this point event_auth.check_state_dependent_auth_rules( - state_res_store, event, auth_events.values(), ) @@ -373,15 +347,12 @@ def _resolve_auth_events( def _resolve_normal_events( - events: List[EventBase], - auth_events: StateMap[EventBase], - state_res_store: StateResolutionStore, + events: List[EventBase], auth_events: StateMap[EventBase] ) -> EventBase: for event in _ordered_events(events): try: # The signatures have already been checked at this point event_auth.check_state_dependent_auth_rules( - state_res_store, event, auth_events.values(), ) diff --git a/synapse/state/v2.py b/synapse/state/v2.py index b778f019be2..da926ad1464 100644 --- a/synapse/state/v2.py +++ b/synapse/state/v2.py @@ -43,7 +43,6 @@ from synapse.api.errors import AuthError from synapse.api.room_versions import RoomVersion from synapse.events import EventBase -from synapse.storage.databases.main.events_worker import EventRedactBehaviour from synapse.types import MutableStateMap, StateMap, StrCollection logger = logging.getLogger(__name__) @@ -59,17 +58,13 @@ def sleep(self, duration_ms: float) -> Awaitable[None]: ... class StateResolutionStore(Protocol): # This is usually synapse.state.StateResolutionStore, but it's replaced with a # TestStateResolutionStore in tests. - async def get_events( - self, - event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, - get_prev_content: bool = False, - allow_rejected: bool = False, - ) -> Dict[str, EventBase]: ... - - async def get_auth_chain_difference( + def get_events( + self, event_ids: StrCollection, allow_rejected: bool = False + ) -> Awaitable[Dict[str, EventBase]]: ... + + def get_auth_chain_difference( self, room_id: str, state_sets: List[Set[str]] - ) -> Set[str]: ... + ) -> Awaitable[Set[str]]: ... # We want to await to the reactor occasionally during state res when dealing @@ -600,7 +595,6 @@ async def _iterative_auth_checks( try: event_auth.check_state_dependent_auth_rules( - state_res_store, event, auth_events.values(), ) diff --git a/tests/handlers/test_federation_event.py b/tests/handlers/test_federation_event.py index 234ced96f42..5db10fa74c2 100644 --- a/tests/handlers/test_federation_event.py +++ b/tests/handlers/test_federation_event.py @@ -938,7 +938,6 @@ def test_process_pulled_event_with_rejected_missing_state(self) -> None: AuthError, ) check_state_dependent_auth_rules( - main_store, rejected_kick_event, [create_event, power_levels_event, other_member_event, bert_member_event], ) diff --git a/tests/state/test_v2.py b/tests/state/test_v2.py index ba76a16351b..0b70f779d9d 100644 --- a/tests/state/test_v2.py +++ b/tests/state/test_v2.py @@ -20,6 +20,7 @@ import itertools from typing import ( + Collection, Dict, Iterable, List, @@ -43,8 +44,7 @@ lexicographical_topological_sort, resolve_events_with_store, ) -from synapse.storage.databases.main.events_worker import EventRedactBehaviour -from synapse.types import EventID, StateMap, StrCollection +from synapse.types import EventID, StateMap from tests import unittest @@ -878,13 +878,9 @@ def pairwise(iterable: Iterable[T]) -> Iterable[Tuple[T, T]]: class TestStateResolutionStore: event_map: Dict[str, EventBase] = attr.ib() - async def get_events( - self, - event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, - get_prev_content: bool = False, - allow_rejected: bool = False, - ) -> Dict[str, EventBase]: + def get_events( + self, event_ids: Collection[str], allow_rejected: bool = False + ) -> "defer.Deferred[Dict[str, EventBase]]": """Get events from the database Args: @@ -895,7 +891,9 @@ async def get_events( Dict from event_id to event. """ - return {eid: self.event_map[eid] for eid in event_ids if eid in self.event_map} + return defer.succeed( + {eid: self.event_map[eid] for eid in event_ids if eid in self.event_map} + ) def _get_auth_chain(self, event_ids: Iterable[str]) -> List[str]: """Gets the full auth chain for a set of events (including rejected @@ -931,10 +929,10 @@ def _get_auth_chain(self, event_ids: Iterable[str]) -> List[str]: return list(result) - async def get_auth_chain_difference( - self, room_id: str, state_sets: List[Set[str]] - ) -> Set[str]: - chains = [frozenset(self._get_auth_chain(a)) for a in state_sets] + def get_auth_chain_difference( + self, room_id: str, auth_sets: List[Set[str]] + ) -> "defer.Deferred[Set[str]]": + chains = [frozenset(self._get_auth_chain(a)) for a in auth_sets] common = set(chains[0]).intersection(*chains[1:]) - return set(chains[0]).union(*chains[1:]) - common + return defer.succeed(set(chains[0]).union(*chains[1:]) - common) diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index f85cedd0fef..f12402f5f2c 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -20,7 +20,7 @@ # import unittest -from typing import Any, Dict, Iterable, List, Optional +from typing import Any, Collection, Dict, Iterable, List, Optional from parameterized import parameterized @@ -30,7 +30,7 @@ from synapse.api.room_versions import EventFormatVersions, RoomVersion, RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.storage.databases.main.events_worker import EventRedactBehaviour -from synapse.types import JsonDict, StrCollection, get_domain_from_id +from synapse.types import JsonDict, get_domain_from_id from tests.test_utils import get_awaitable_result @@ -50,16 +50,14 @@ def add_events(self, events: Iterable[EventBase]) -> None: async def get_events( self, - event_ids: StrCollection, - redact_behaviour: EventRedactBehaviour = EventRedactBehaviour.redact, + event_ids: Collection[str], + redact_behaviour: EventRedactBehaviour, get_prev_content: bool = False, allow_rejected: bool = False, ) -> Dict[str, EventBase]: assert allow_rejected assert not get_prev_content - # TODO: Is `EventRedactBehaviour.as_is` important here? We're changing it to - # the default of `EventRedactBehaviour.redact` - # assert redact_behaviour == EventRedactBehaviour.as_is + assert redact_behaviour == EventRedactBehaviour.as_is results = {} for e in event_ids: if e in self._store: @@ -86,7 +84,7 @@ def test_rejected_auth_events(self) -> None: get_awaitable_result( event_auth.check_state_independent_auth_rules(event_store, event) ) - event_auth.check_state_dependent_auth_rules(event_store, event, auth_events) + event_auth.check_state_dependent_auth_rules(event, auth_events) # ... but a rejected join_rules event should cause it to be rejected rejected_join_rules = _join_rules_event( @@ -258,11 +256,8 @@ def test_random_users_cannot_send_state_before_first_pl(self) -> None: _join_event(RoomVersions.V1, joiner), ] - event_store = _StubEventSourceStore() - # creator should be able to send state event_auth.check_state_dependent_auth_rules( - event_store, _random_state_event(RoomVersions.V1, creator), auth_events, ) @@ -271,7 +266,6 @@ def test_random_users_cannot_send_state_before_first_pl(self) -> None: self.assertRaises( AuthError, event_auth.check_state_dependent_auth_rules, - event_store, _random_state_event(RoomVersions.V1, joiner), auth_events, ) @@ -297,14 +291,11 @@ def test_state_default_level(self) -> None: _join_event(RoomVersions.V1, king), ] - event_store = _StubEventSourceStore() - # pleb should not be able to send state ( self.assertRaises( AuthError, event_auth.check_state_dependent_auth_rules, - event_store, _random_state_event(RoomVersions.V1, pleb), auth_events, ), @@ -312,7 +303,6 @@ def test_state_default_level(self) -> None: # king should be able to send state event_auth.check_state_dependent_auth_rules( - event_store, _random_state_event(RoomVersions.V1, king), auth_events, ) @@ -325,11 +315,9 @@ def test_alias_event(self) -> None: _create_event(RoomVersions.V1, creator), _join_event(RoomVersions.V1, creator), ] - event_store = _StubEventSourceStore() # creator should be able to send aliases event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V1, creator), auth_events, ) @@ -337,7 +325,6 @@ def test_alias_event(self) -> None: # Reject an event with no state key. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V1, creator, state_key=""), auth_events, ) @@ -345,14 +332,12 @@ def test_alias_event(self) -> None: # If the domain of the sender does not match the state key, reject. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V1, creator, state_key="test.com"), auth_events, ) # Note that the member does *not* need to be in the room. event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V1, other), auth_events, ) @@ -365,23 +350,19 @@ def test_msc2432_alias_event(self) -> None: _create_event(RoomVersions.V6, creator), _join_event(RoomVersions.V6, creator), ] - event_store = _StubEventSourceStore() # creator should be able to send aliases event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V6, creator), auth_events, ) # No particular checks are done on the state key. event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V6, creator, state_key=""), auth_events, ) event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V6, creator, state_key="test.com"), auth_events, ) @@ -389,7 +370,6 @@ def test_msc2432_alias_event(self) -> None: # Per standard auth rules, the member must be in the room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _alias_event(RoomVersions.V6, other), auth_events, ) @@ -417,20 +397,14 @@ def test_notifications( room_version, pleb, {"notifications": {"room": 100}} ) - event_store = _StubEventSourceStore() - # on room V1, pleb should be able to modify the notifications power level. if allow_modification: - event_auth.check_state_dependent_auth_rules( - event_store, pl_event, auth_events - ) + event_auth.check_state_dependent_auth_rules(pl_event, auth_events) else: # But an MSC2209 room rejects this change. with self.assertRaises(AuthError): - event_auth.check_state_dependent_auth_rules( - event_store, pl_event, auth_events - ) + event_auth.check_state_dependent_auth_rules(pl_event, auth_events) def test_join_rules_public(self) -> None: """ @@ -446,11 +420,9 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, creator, "public" ), } - event_store = _StubEventSourceStore() # Check join. event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -458,7 +430,6 @@ def test_join_rules_public(self) -> None: # A user cannot be force-joined to a room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _member_event(RoomVersions.V6, pleb, "join", sender=creator), auth_events.values(), ) @@ -469,7 +440,6 @@ def test_join_rules_public(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -479,7 +449,6 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "leave" ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -489,7 +458,6 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "join" ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -499,7 +467,6 @@ def test_join_rules_public(self) -> None: RoomVersions.V6, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -518,12 +485,10 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, creator, "invite" ), } - event_store = _StubEventSourceStore() # A join without an invite is rejected. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -531,7 +496,6 @@ def test_join_rules_invite(self) -> None: # A user cannot be force-joined to a room. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _member_event(RoomVersions.V6, pleb, "join", sender=creator), auth_events.values(), ) @@ -542,7 +506,6 @@ def test_join_rules_invite(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -553,7 +516,6 @@ def test_join_rules_invite(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -563,7 +525,6 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, pleb, "join" ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -573,7 +534,6 @@ def test_join_rules_invite(self) -> None: RoomVersions.V6, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -593,11 +553,9 @@ def test_join_rules_restricted_old_room(self) -> None: RoomVersions.V6, creator, "restricted" ), } - event_store = _StubEventSourceStore() with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V6, pleb), auth_events.values(), ) @@ -625,7 +583,6 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, creator, "restricted" ), } - event_store = _StubEventSourceStore() # A properly formatted join event should work. authorised_join_event = _join_event( @@ -636,7 +593,6 @@ def test_join_rules_msc3083_restricted(self) -> None: }, ) event_auth.check_state_dependent_auth_rules( - event_store, authorised_join_event, auth_events.values(), ) @@ -653,7 +609,6 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, "@inviter:foo.test" ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event( RoomVersions.V8, pleb, @@ -667,7 +622,6 @@ def test_join_rules_msc3083_restricted(self) -> None: # A join which is missing an authorised server is rejected. with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) @@ -681,7 +635,6 @@ def test_join_rules_msc3083_restricted(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _join_event( RoomVersions.V8, pleb, @@ -696,7 +649,6 @@ def test_join_rules_msc3083_restricted(self) -> None: # *would* be valid, but is sent be a different user.) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, _member_event( RoomVersions.V8, pleb, @@ -715,7 +667,6 @@ def test_join_rules_msc3083_restricted(self) -> None: ) with self.assertRaises(AuthError): event_auth.check_state_dependent_auth_rules( - event_store, authorised_join_event, auth_events.values(), ) @@ -725,7 +676,6 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "leave" ) event_auth.check_state_dependent_auth_rules( - event_store, authorised_join_event, auth_events.values(), ) @@ -736,7 +686,6 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "join" ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) @@ -747,7 +696,6 @@ def test_join_rules_msc3083_restricted(self) -> None: RoomVersions.V8, pleb, "invite", sender=creator ) event_auth.check_state_dependent_auth_rules( - event_store, _join_event(RoomVersions.V8, pleb), auth_events.values(), ) From 825b249b550d66cb41f7ccfe6c27cccabbcf4ee5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jan 2025 18:06:30 -0600 Subject: [PATCH 06/45] Add out-of-band membership to auth events when creating new events --- synapse/events/builder.py | 55 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/synapse/events/builder.py b/synapse/events/builder.py index 10ef01131b4..b660b4104a9 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -24,7 +24,7 @@ import attr from signedjson.types import SigningKey -from synapse.api.constants import MAX_DEPTH +from synapse.api.constants import MAX_DEPTH, EventTypes, Membership from synapse.api.room_versions import ( KNOWN_EVENT_FORMAT_VERSIONS, EventFormatVersions, @@ -109,6 +109,19 @@ def state_key(self) -> str: def is_state(self) -> bool: return self._state_key is not None + def is_mine_id(self, string: str) -> bool: + """Determines whether a user ID or room alias originates from this homeserver. + + Returns: + `True` if the hostname part of the user ID or room alias matches this + homeserver. + `False` otherwise, or if the user ID or room alias is malformed. + """ + localpart_hostname = string.split(":", 1) + if len(localpart_hostname) < 2: + return False + return localpart_hostname[1] == self._hostname + async def build( self, prev_event_ids: List[str], @@ -142,6 +155,46 @@ async def build( self, state_ids ) + # Check for out-of-band membership that may that may have been exposed on + # `/sync` but the events have not been de-outliered yet so they won't be + # part of the room state yet. + # + # This helps in situations where a remote homeserver invites a local user to + # a room that we're already participating in; and we've persisted the invite + # as an out-of-band membership (outlier), but it hasn't been pushed to us as + # part of a `/send` transaction yet and de-outliered. This also helps for + # any of the other out-of-band membership transitions. + # + # If the state already includes a non-`leave` membership event, then we can + # assume the membership event has been de-outliered and we don't need to + # check for it. + state_membership_event = state_ids.get((EventTypes.Member, "")) + state_already_includes_non_leave_membership_event = ( + state_membership_event is not None + # TODO: Check for non-leave membership + and False in (Membership.LIST - {Membership.LEAVE}) + ) + if ( + not state_already_includes_non_leave_membership_event + and self.type == EventTypes.Member + and self.is_mine_id(self.state_key) + ): + ( + membership, + member_event_id, + ) = await self._store.get_local_current_membership_for_user_in_room( + user_id=self.state_key, + room_id=self.room_id, + ) + if member_event_id is not None: + # Check to make sure this membership is an outlier indicating that it is indeed + # an out-of-band membership. This is a sanity check to ensure that TODO + # await self._store.get_events(event_ids=[member_event_id]) + logger.debug( + f"Adding out-of-band membership event ({member_event_id}) to auth events of {self.type} in {self.room_id} sent by {self.sender}" + ) + auth_event_ids.append(member_event_id) + format_version = self.room_version.event_format # The types of auth/prev events changes between event versions. prev_events: Union[StrCollection, List[Tuple[str, Dict[str, str]]]] From 0698dcfa3d4854260937b2dbc46787a8b587b10e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jan 2025 18:11:38 -0600 Subject: [PATCH 07/45] Add changelog --- changelog.d/18075.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18075.bugfix diff --git a/changelog.d/18075.bugfix b/changelog.d/18075.bugfix new file mode 100644 index 00000000000..95b486bed11 --- /dev/null +++ b/changelog.d/18075.bugfix @@ -0,0 +1 @@ +Fix join being denied after being invited over federation. Also fixes other out-of-band membership transitions. From 8c6b294c747f23e8b9c88a9a5143d33bf76c7bdb Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 14:21:43 -0600 Subject: [PATCH 08/45] Clean up fix --- synapse/events/builder.py | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/synapse/events/builder.py b/synapse/events/builder.py index b660b4104a9..cb4741b4e0b 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -24,7 +24,7 @@ import attr from signedjson.types import SigningKey -from synapse.api.constants import MAX_DEPTH, EventTypes, Membership +from synapse.api.constants import MAX_DEPTH, EventTypes from synapse.api.room_versions import ( KNOWN_EVENT_FORMAT_VERSIONS, EventFormatVersions, @@ -165,34 +165,25 @@ async def build( # part of a `/send` transaction yet and de-outliered. This also helps for # any of the other out-of-band membership transitions. # - # If the state already includes a non-`leave` membership event, then we can - # assume the membership event has been de-outliered and we don't need to - # check for it. - state_membership_event = state_ids.get((EventTypes.Member, "")) - state_already_includes_non_leave_membership_event = ( - state_membership_event is not None - # TODO: Check for non-leave membership - and False in (Membership.LIST - {Membership.LEAVE}) - ) - if ( - not state_already_includes_non_leave_membership_event - and self.type == EventTypes.Member - and self.is_mine_id(self.state_key) - ): + # As an optimization, we could check if the room state already includes a + # non-`leave` membership event, then we can assume the membership event has + # been de-outliered and we don't need to check for an out-of-band + # membership. But we don't have the necessary information from a + # `StateMap[str]` and we'll just have to take the hit of this extra lookup + # for any membership event for now. + if self.type == EventTypes.Member and self.is_mine_id(self.state_key): ( - membership, + _membership, member_event_id, ) = await self._store.get_local_current_membership_for_user_in_room( user_id=self.state_key, room_id=self.room_id, ) if member_event_id is not None: - # Check to make sure this membership is an outlier indicating that it is indeed - # an out-of-band membership. This is a sanity check to ensure that TODO - # await self._store.get_events(event_ids=[member_event_id]) - logger.debug( - f"Adding out-of-band membership event ({member_event_id}) to auth events of {self.type} in {self.room_id} sent by {self.sender}" - ) + # There is no need to check if the membership is actually an + # out-of-band membership (`outlier`) as we would end up with the + # same result either way (adding the member event to the + # `auth_event_ids`). auth_event_ids.append(member_event_id) format_version = self.room_version.event_format From 7e60ec0ba2594e0acff7d9f1d519d4c25d940105 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 14:24:00 -0600 Subject: [PATCH 09/45] It's fine to notify about out-of-band membership This is just normal for how someone finds out about an invite over federation See https://github.com/element-hq/synapse/pull/18075#discussion_r1911764490 --- synapse/handlers/federation_event.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index d0ef43a753c..6c0d13d648f 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -2301,8 +2301,6 @@ async def persist_events_and_notify( str(len(events)), ) for event in events: - # TODO: Is this correct? - # if not event.internal_metadata.is_outlier(): await self._notify_persisted_event(event, max_stream_token) return max_stream_token.stream From ad8ed0ca0e6d5a27a42a0229443a8c62ff37966e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 14:24:59 -0600 Subject: [PATCH 10/45] Remove duplicated code --- synapse/handlers/federation_event.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 6c0d13d648f..db090ba8f71 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -2280,11 +2280,6 @@ async def persist_events_and_notify( event_and_contexts, backfilled=backfilled ) - # After persistence we always need to notify replication there may - # be new data. - # XXX: This already happens in `_notify_persisted_event` -> `on_new_room_events` -> `notify_new_room_events` -> `notify_replication` - # self._notifier.notify_replication() - if self._ephemeral_messages_enabled: for event in events: # If there's an expiry timestamp on the event, schedule its expiry. From d0639e4a68e68a65d752045188a98f2cb05608e5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 14:44:46 -0600 Subject: [PATCH 11/45] Restore `notifier.notify_replication()` See https://github.com/element-hq/synapse/pull/18075#discussion_r1915552743 --- synapse/handlers/federation_event.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index db090ba8f71..818e91a6bbc 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -2280,6 +2280,10 @@ async def persist_events_and_notify( event_and_contexts, backfilled=backfilled ) + # After persistence, we always need to notify replication there may be new + # data (backfilled or not) because TODO. + self._notifier.notify_replication() + if self._ephemeral_messages_enabled: for event in events: # If there's an expiry timestamp on the event, schedule its expiry. From 7855892e1cf6583d17e95b47fda71b36881d7c56 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 16:08:00 -0600 Subject: [PATCH 12/45] Log all handled events --- synapse/handlers/message.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 99766b3f23d..3ddef80f411 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1495,7 +1495,8 @@ async def handle_new_client_event( gather_results(deferreds, consumeErrors=True) ).addErrback(unwrapFirstError) - logger.info("✅ handle_new_client_event: handled %s", event) + for event, _ in events_and_context: + logger.info("✅ handle_new_client_event: handled %s", event) return result async def _persist_events( From b3bda0ee90fc608b9f16bee566e220fc54aefbf6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 16:08:06 -0600 Subject: [PATCH 13/45] Clean up some pre-existing tests --- tests/test_federation.py | 169 ++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 83 deletions(-) diff --git a/tests/test_federation.py b/tests/test_federation.py index 94b0fa98565..09bd0977e2d 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -19,96 +19,93 @@ # # -from typing import Collection, List, Optional, Union +import logging +from typing import Optional, Union from unittest.mock import AsyncMock, Mock from twisted.test.proto_helpers import MemoryReactor +import synapse.rest.admin +from synapse.api.constants import EventTypes, Membership from synapse.api.errors import FederationError -from synapse.api.room_versions import RoomVersion, RoomVersions -from synapse.events import EventBase, make_event_from_dict -from synapse.events.snapshot import EventContext +from synapse.api.room_versions import RoomVersions +from synapse.events import make_event_from_dict from synapse.federation.federation_base import event_from_pdu_json from synapse.handlers.device import DeviceListUpdater from synapse.http.types import QueryParams from synapse.logging.context import LoggingContext +from synapse.rest.client import login, room from synapse.server import HomeServer -from synapse.types import JsonDict, UserID, create_requester +from synapse.types import JsonDict from synapse.util import Clock from synapse.util.retryutils import NotRetryingDestination from tests import unittest +logger = logging.getLogger(__name__) + + +class MessageAcceptTests(unittest.FederatingHomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] -class MessageAcceptTests(unittest.HomeserverTestCase): def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.http_client = Mock() return self.setup_test_homeserver(federation_http_client=self.http_client) def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - user_id = UserID("us", "test") - our_user = create_requester(user_id) - room_creator = self.hs.get_room_creation_handler() - self.room_id = self.get_success( - room_creator.create_room( - our_user, room_creator._presets_dict["public_chat"], ratelimit=False - ) - )[0] - self.store = self.hs.get_datastores().main - - # Figure out what the most recent event is - most_recent = next( - iter( - self.get_success( - self.hs.get_datastores().main.get_latest_event_ids_in_room( - self.room_id - ) - ) - ) + self.storage_controllers = hs.get_storage_controllers() + self.federation_event_handler = self.hs.get_federation_event_handler() + + # Create a local room + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + self.room_id = self.helper.create_room_as( + user1_id, tok=user1_tok, is_public=True ) - join_event = make_event_from_dict( - { - "room_id": self.room_id, - "sender": "@baduser:test.serv", - "state_key": "@baduser:test.serv", - "event_id": "$join:test.serv", - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.member", - "origin": "test.servx", - "content": {"membership": "join"}, - "auth_events": [], - "prev_state": [(most_recent, {})], - "prev_events": [(most_recent, {})], - } + state_map = self.get_success( + self.storage_controllers.state.get_current_state(self.room_id) ) - self.handler = self.hs.get_federation_handler() - federation_event_handler = self.hs.get_federation_event_handler() - - async def _check_event_auth( - origin: Optional[str], event: EventBase, context: EventContext - ) -> None: - pass - - federation_event_handler._check_event_auth = _check_event_auth # type: ignore[method-assign] - self.client = self.hs.get_federation_client() - - async def _check_sigs_and_hash_for_pulled_events_and_fetch( - dest: str, pdus: Collection[EventBase], room_version: RoomVersion - ) -> List[EventBase]: - return list(pdus) + # Figure out what the forward extremeties in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) + ) - self.client._check_sigs_and_hash_for_pulled_events_and_fetch = ( # type: ignore[method-assign] - _check_sigs_and_hash_for_pulled_events_and_fetch # type: ignore[assignment] + # Join a remote user to the room that will attempt to send bad events + self.remote_bad_user_id = f"@baduser:{self.OTHER_SERVER_NAME}" + self.remote_bad_user_join_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": self.room_id, + "sender": self.remote_bad_user_id, + "state_key": self.remote_bad_user_id, + "depth": 1000, + "origin_server_ts": 1, + "type": EventTypes.Member, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + state_map[(EventTypes.Create, "")].event_id, + state_map[(EventTypes.JoinRules, "")].event_id, + ], + "prev_events": list(forward_extremity_event_ids), + } + ), + room_version=RoomVersions.V10, ) # Send the join, it should return None (which is not an error) self.assertEqual( self.get_success( - federation_event_handler.on_receive_pdu("test.serv", join_event) + self.federation_event_handler.on_receive_pdu( + self.OTHER_SERVER_NAME, self.remote_bad_user_join_event + ) ), None, ) @@ -116,7 +113,7 @@ async def _check_sigs_and_hash_for_pulled_events_and_fetch( # Make sure we actually joined the room self.assertEqual( self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)), - {"$join:test.serv"}, + {self.remote_bad_user_join_event.event_id}, ) def test_cant_hide_direct_ancestors(self) -> None: @@ -141,33 +138,35 @@ async def post_json( self.http_client.post_json = post_json - # Figure out what the most recent event is - most_recent = next( - iter( - self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)) - ) + # Figure out what the forward extremeties in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) ) - # Now lie about an event + # Now lie about an event's prev_events lying_event = make_event_from_dict( - { - "room_id": self.room_id, - "sender": "@baduser:test.serv", - "event_id": "one:test.serv", - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.message", - "origin": "test.serv", - "content": {"body": "hewwo?"}, - "auth_events": [], - "prev_events": [("two:test.serv", {}), (most_recent, {})], - } + self.add_hashes_and_signatures_from_other_server( + { + "room_id": self.room_id, + "sender": self.remote_bad_user_id, + "depth": 1000, + "origin_server_ts": 1, + "type": "m.room.message", + "content": {"body": "hewwo?"}, + "auth_events": [], + "prev_events": ["$missing_prev_event"] + + list(forward_extremity_event_ids), + } + ), + room_version=RoomVersions.V10, ) - federation_event_handler = self.hs.get_federation_event_handler() with LoggingContext("test-context"): failure = self.get_failure( - federation_event_handler.on_receive_pdu("test.serv", lying_event), + self.federation_event_handler.on_receive_pdu( + self.OTHER_SERVER_NAME, lying_event + ), FederationError, ) @@ -182,7 +181,12 @@ async def post_json( # Make sure the invalid event isn't there extrem = self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)) - self.assertEqual(extrem, {"$join:test.serv"}) + self.assertEqual(extrem, {self.remote_bad_user_join_event.event_id}) + + +class DeviceListResyncTestCase(unittest.HomeserverTestCase): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = self.hs.get_datastores().main def test_retry_device_list_resync(self) -> None: """Tests that device lists are marked as stale if they couldn't be synced, and @@ -211,8 +215,7 @@ def query_user_devices( # Register a mock on the store so that the incoming update doesn't fail because # we don't share a room with the user. - store = self.hs.get_datastores().main - store.get_rooms_for_user = AsyncMock(return_value=["!someroom:test"]) + self.store.get_rooms_for_user = AsyncMock(return_value=["!someroom:test"]) # Manually inject a fake device list update. We need this update to include at # least one prev_id so that the user's device list will need to be retried. @@ -238,7 +241,7 @@ def query_user_devices( # Check that the resync attempt failed and caused the user's device list to be # marked as stale. need_resync = self.get_success( - store.get_user_ids_requiring_device_list_resync() + self.store.get_user_ids_requiring_device_list_resync() ) self.assertIn(remote_user_id, need_resync) From 0d01cda0014d35af66734c79c705146e3fce67cc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 18:36:22 -0600 Subject: [PATCH 14/45] Try adding out of band federation trial tests Doesn't work very well because we need a lot of interaction with another homeserver --- tests/test_federation.py | 183 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 178 insertions(+), 5 deletions(-) diff --git a/tests/test_federation.py b/tests/test_federation.py index 09bd0977e2d..7e47f9d232b 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -20,34 +20,44 @@ # import logging -from typing import Optional, Union +from http import HTTPStatus +from typing import Optional, Tuple, Union from unittest.mock import AsyncMock, Mock from twisted.test.proto_helpers import MemoryReactor -import synapse.rest.admin -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.api.errors import FederationError from synapse.api.room_versions import RoomVersions from synapse.events import make_event_from_dict +from synapse.events.utils import strip_event from synapse.federation.federation_base import event_from_pdu_json from synapse.handlers.device import DeviceListUpdater from synapse.http.types import QueryParams from synapse.logging.context import LoggingContext -from synapse.rest.client import login, room +from synapse.rest import admin +from synapse.rest.client import login, room, sync from synapse.server import HomeServer from synapse.types import JsonDict from synapse.util import Clock from synapse.util.retryutils import NotRetryingDestination +from synapse.http.matrixfederationclient import ( + MatrixFederationRequest, +) from tests import unittest +from tests.utils import test_timeout logger = logging.getLogger(__name__) class MessageAcceptTests(unittest.FederatingHomeserverTestCase): + """ + Tests to make sure that we don't accept flawed events from federation (incoming). + """ + servlets = [ - synapse.rest.admin.register_servlets, + admin.register_servlets, login.register_servlets, room.register_servlets, ] @@ -57,6 +67,8 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: return self.setup_test_homeserver(federation_http_client=self.http_client) def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + super().prepare(reactor, clock, hs) + self.store = self.hs.get_datastores().main self.storage_controllers = hs.get_storage_controllers() self.federation_event_handler = self.hs.get_federation_event_handler() @@ -184,6 +196,167 @@ async def post_json( self.assertEqual(extrem, {self.remote_bad_user_join_event.event_id}) +class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): + """ + Tests to make sure that we can join remote rooms over federation. + """ + + servlets = [ + admin.register_servlets, + room.register_servlets, + login.register_servlets, + sync.register_servlets, + ] + + sync_endpoint = "/_matrix/client/unstable/org.matrix.simplified_msc3575/sync" + + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: + self.federation_http_client = Mock() + return self.setup_test_homeserver( + federation_http_client=self.federation_http_client + ) + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + super().prepare(reactor, clock, hs) + + self.store = self.hs.get_datastores().main + + def do_sync( + self, sync_body: JsonDict, *, since: Optional[str] = None, tok: str + ) -> Tuple[JsonDict, str]: + """Do a sliding sync request with given body. + + Asserts the request was successful. + + Attributes: + sync_body: The full request body to use + since: Optional since token + tok: Access token to use + + Returns: + A tuple of the response body and the `pos` field. + """ + + sync_path = self.sync_endpoint + if since: + sync_path += f"?pos={since}" + + channel = self.make_request( + method="POST", + path=sync_path, + content=sync_body, + access_token=tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + return channel.json_body, channel.json_body["pos"] + + def test_asdf(self) -> None: + # Create a local room + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + # Create a remote room + room_creator_user_id = f"@user:{self.OTHER_SERVER_NAME}" + room_id = f"!foo:{self.OTHER_SERVER_NAME}" + room_version = RoomVersions.V10 + + room_create_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": room_id, + "sender": room_creator_user_id, + "depth": 1, + "origin_server_ts": 1, + "type": EventTypes.Create, + "state_key": "", + "content": { + # The `ROOM_CREATOR` field could be removed if we used a room + # version > 10 (in favor of relying on `sender`) + EventContentFields.ROOM_CREATOR: room_creator_user_id, + EventContentFields.ROOM_VERSION: room_version.identifier, + }, + "auth_events": [], + "prev_events": [], + } + ), + room_version=RoomVersions.V10, + ) + + creator_membership_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": room_id, + "sender": room_creator_user_id, + "depth": 2, + "origin_server_ts": 2, + "type": EventTypes.Member, + "state_key": room_creator_user_id, + "content": {"membership": Membership.JOIN}, + "auth_events": [room_create_event.event_id], + "prev_events": [room_create_event.event_id], + } + ), + room_version=RoomVersions.V10, + ) + + # From the remote homeserver, invite user1 on the local homserver + user1_invite_membership_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": room_id, + "sender": room_creator_user_id, + "depth": 3, + "origin_server_ts": 3, + "type": EventTypes.Member, + "state_key": user1_id, + "content": {"membership": Membership.INVITE}, + "auth_events": [ + room_create_event.event_id, + creator_membership_event.event_id, + ], + "prev_events": [creator_membership_event.event_id], + } + ), + room_version=RoomVersions.V10, + ) + channel = self.make_signed_federation_request( + "PUT", + f"/_matrix/federation/v2/invite/{room_id}/{user1_invite_membership_event.event_id}", + content={ + "event": user1_invite_membership_event.get_dict(), + "invite_room_state": [ + strip_event(room_create_event), + strip_event(creator_membership_event), + ], + "room_version": room_version.identifier, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + } + + # Sync until the local user1 can see the invite + with test_timeout(5): + while True: + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + if room_id in response_body["rooms"].keys(): + break + + # User1 joins the room + self.helper.join(room_id, user1_id, tok=user1_tok) + + class DeviceListResyncTestCase(unittest.HomeserverTestCase): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = self.hs.get_datastores().main From 83237f717b3ff0e9c258659741cc392a8a94c74c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 14 Jan 2025 19:21:32 -0600 Subject: [PATCH 15/45] User1 can join remote room --- tests/test_federation.py | 122 +++++++++++++++++++++++++++++++++++---- 1 file changed, 112 insertions(+), 10 deletions(-) diff --git a/tests/test_federation.py b/tests/test_federation.py index 7e47f9d232b..d91053fd35f 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -20,8 +20,9 @@ # import logging +import urllib.parse from http import HTTPStatus -from typing import Optional, Tuple, Union +from typing import Callable, Optional, Tuple, TypeVar, Union from unittest.mock import AsyncMock, Mock from twisted.test.proto_helpers import MemoryReactor @@ -32,7 +33,11 @@ from synapse.events import make_event_from_dict from synapse.events.utils import strip_event from synapse.federation.federation_base import event_from_pdu_json +from synapse.federation.transport.client import SendJoinResponse from synapse.handlers.device import DeviceListUpdater +from synapse.http.matrixfederationclient import ( + ByteParser, +) from synapse.http.types import QueryParams from synapse.logging.context import LoggingContext from synapse.rest import admin @@ -41,9 +46,6 @@ from synapse.types import JsonDict from synapse.util import Clock from synapse.util.retryutils import NotRetryingDestination -from synapse.http.matrixfederationclient import ( - MatrixFederationRequest, -) from tests import unittest from tests.utils import test_timeout @@ -211,7 +213,9 @@ class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): sync_endpoint = "/_matrix/client/unstable/org.matrix.simplified_msc3575/sync" def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: - self.federation_http_client = Mock() + self.federation_http_client = Mock( + # spec=MatrixFederationHttpClient + ) return self.setup_test_homeserver( federation_http_client=self.federation_http_client ) @@ -255,8 +259,8 @@ def test_asdf(self) -> None: # Create a local room user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - user2_id = self.register_user("user2", "pass") - user2_tok = self.login(user2_id, "pass") + # user2_id = self.register_user("user2", "pass") + # user2_tok = self.login(user2_id, "pass") # Create a remote room room_creator_user_id = f"@user:{self.OTHER_SERVER_NAME}" @@ -282,7 +286,7 @@ def test_asdf(self) -> None: "prev_events": [], } ), - room_version=RoomVersions.V10, + room_version=room_version, ) creator_membership_event = make_event_from_dict( @@ -299,7 +303,7 @@ def test_asdf(self) -> None: "prev_events": [room_create_event.event_id], } ), - room_version=RoomVersions.V10, + room_version=room_version, ) # From the remote homeserver, invite user1 on the local homserver @@ -320,7 +324,7 @@ def test_asdf(self) -> None: "prev_events": [creator_membership_event.event_id], } ), - room_version=RoomVersions.V10, + room_version=room_version, ) channel = self.make_signed_federation_request( "PUT", @@ -353,6 +357,104 @@ def test_asdf(self) -> None: if room_id in response_body["rooms"].keys(): break + user1_join_membership_event_template = make_event_from_dict( + { + "room_id": room_id, + "sender": user1_id, + "depth": 4, + "origin_server_ts": 4, + "type": EventTypes.Member, + "state_key": user1_id, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + room_create_event.event_id, + user1_invite_membership_event.event_id, + ], + "prev_events": [user1_invite_membership_event.event_id], + }, + room_version=room_version, + ) + + T = TypeVar("T") + + async def get_json( + destination: str, + path: str, + args: Optional[QueryParams] = None, + retry_on_dns_fail: bool = True, + timeout: Optional[int] = None, + ignore_backoff: bool = False, + try_trailing_slash_on_400: bool = False, + parser: Optional[ByteParser[T]] = None, + ) -> Union[JsonDict, T]: + logger.info("asdf get_json %s %s", destination, path) + + if ( + path + == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(room_id)}/{urllib.parse.quote_plus(user1_id)}" + ): + return { + "event": user1_join_membership_event_template.get_pdu_json(), + "room_version": room_version.identifier, + } + + return {} + + self.federation_http_client.get_json.side_effect = get_json + + async def put_json( + destination: str, + path: str, + args: Optional[QueryParams] = None, + data: Optional[JsonDict] = None, + json_data_callback: Optional[Callable[[], JsonDict]] = None, + long_retries: bool = False, + timeout: Optional[int] = None, + ignore_backoff: bool = False, + backoff_on_404: bool = False, + try_trailing_slash_on_400: bool = False, + parser: Optional[ByteParser[T]] = None, + backoff_on_all_error_codes: bool = False, + ) -> Union[JsonDict, T]: + logger.info("asdf put_json %s %s parser=%s", destination, path, parser) + + if ( + path.startswith( + f"/_matrix/federation/v2/send_join/{urllib.parse.quote_plus(room_id)}/" + ) + and data is not None + and data.get("type") == EventTypes.Member + and data.get("state_key") == user1_id + # We're assuming this is a `ByteParser[SendJoinResponse]` + and parser is not None + ): + user1_join_membership_event_signed = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server(data), + room_version=room_version, + ) + + return SendJoinResponse( + auth_events=[ + room_create_event, + user1_invite_membership_event, + ], + state=[ + room_create_event, + creator_membership_event, + user1_invite_membership_event, + ], + event_dict=user1_join_membership_event_signed.get_pdu_json(), + event=user1_join_membership_event_signed, + members_omitted=False, + servers_in_room=[ + self.OTHER_SERVER_NAME, + ], + ) + + return {} + + self.federation_http_client.put_json.side_effect = put_json + # User1 joins the room self.helper.join(room_id, user1_id, tok=user1_tok) From 1779d76cf47a0eb28a0ed9d6fb4b2329edaf96cb Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 13:11:45 -0600 Subject: [PATCH 16/45] Move `DeviceListResyncTestCase` to their own file --- tests/federation/test_federation_devices.py | 161 ++++++++++++++++++++ tests/test_federation.py | 131 +--------------- 2 files changed, 162 insertions(+), 130 deletions(-) create mode 100644 tests/federation/test_federation_devices.py diff --git a/tests/federation/test_federation_devices.py b/tests/federation/test_federation_devices.py new file mode 100644 index 00000000000..ba27e694798 --- /dev/null +++ b/tests/federation/test_federation_devices.py @@ -0,0 +1,161 @@ +# +# This file is licensed under the Affero General Public License (AGPL) version 3. +# +# Copyright (C) 2024 New Vector, Ltd +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# See the GNU Affero General Public License for more details: +# . +# +# Originally licensed under the Apache License, Version 2.0: +# . +# +# [This file includes modifications made by New Vector Limited] +# +# + +import logging +from unittest.mock import AsyncMock, Mock + +from twisted.test.proto_helpers import MemoryReactor + +from synapse.handlers.device import DeviceListUpdater +from synapse.server import HomeServer +from synapse.types import JsonDict +from synapse.util import Clock +from synapse.util.retryutils import NotRetryingDestination + +from tests import unittest + +logger = logging.getLogger(__name__) + + +class DeviceListResyncTestCase(unittest.HomeserverTestCase): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = self.hs.get_datastores().main + + def test_retry_device_list_resync(self) -> None: + """Tests that device lists are marked as stale if they couldn't be synced, and + that stale device lists are retried periodically. + """ + remote_user_id = "@john:test_remote" + remote_origin = "test_remote" + + # Track the number of attempts to resync the user's device list. + self.resync_attempts = 0 + + # When this function is called, increment the number of resync attempts (only if + # we're querying devices for the right user ID), then raise a + # NotRetryingDestination error to fail the resync gracefully. + def query_user_devices( + destination: str, user_id: str, timeout: int = 30000 + ) -> JsonDict: + if user_id == remote_user_id: + self.resync_attempts += 1 + + raise NotRetryingDestination(0, 0, destination) + + # Register the mock on the federation client. + federation_client = self.hs.get_federation_client() + federation_client.query_user_devices = Mock(side_effect=query_user_devices) # type: ignore[method-assign] + + # Register a mock on the store so that the incoming update doesn't fail because + # we don't share a room with the user. + self.store.get_rooms_for_user = AsyncMock(return_value=["!someroom:test"]) + + # Manually inject a fake device list update. We need this update to include at + # least one prev_id so that the user's device list will need to be retried. + device_list_updater = self.hs.get_device_handler().device_list_updater + assert isinstance(device_list_updater, DeviceListUpdater) + self.get_success( + device_list_updater.incoming_device_list_update( + origin=remote_origin, + edu_content={ + "deleted": False, + "device_display_name": "Mobile", + "device_id": "QBUAZIFURK", + "prev_id": [5], + "stream_id": 6, + "user_id": remote_user_id, + }, + ) + ) + + # Check that there was one resync attempt. + self.assertEqual(self.resync_attempts, 1) + + # Check that the resync attempt failed and caused the user's device list to be + # marked as stale. + need_resync = self.get_success( + self.store.get_user_ids_requiring_device_list_resync() + ) + self.assertIn(remote_user_id, need_resync) + + # Check that waiting for 30 seconds caused Synapse to retry resyncing the device + # list. + self.reactor.advance(30) + self.assertEqual(self.resync_attempts, 2) + + def test_cross_signing_keys_retry(self) -> None: + """Tests that resyncing a device list correctly processes cross-signing keys from + the remote server. + """ + remote_user_id = "@john:test_remote" + remote_master_key = "85T7JXPFBAySB/jwby4S3lBPTqY3+Zg53nYuGmu1ggY" + remote_self_signing_key = "QeIiFEjluPBtI7WQdG365QKZcFs9kqmHir6RBD0//nQ" + + # Register mock device list retrieval on the federation client. + federation_client = self.hs.get_federation_client() + federation_client.query_user_devices = AsyncMock( # type: ignore[method-assign] + return_value={ + "user_id": remote_user_id, + "stream_id": 1, + "devices": [], + "master_key": { + "user_id": remote_user_id, + "usage": ["master"], + "keys": {"ed25519:" + remote_master_key: remote_master_key}, + }, + "self_signing_key": { + "user_id": remote_user_id, + "usage": ["self_signing"], + "keys": { + "ed25519:" + remote_self_signing_key: remote_self_signing_key + }, + }, + } + ) + + # Resync the device list. + device_handler = self.hs.get_device_handler() + self.get_success( + device_handler.device_list_updater.multi_user_device_resync( + [remote_user_id] + ), + ) + + # Retrieve the cross-signing keys for this user. + keys = self.get_success( + self.store.get_e2e_cross_signing_keys_bulk(user_ids=[remote_user_id]), + ) + self.assertIn(remote_user_id, keys) + key = keys[remote_user_id] + assert key is not None + + # Check that the master key is the one returned by the mock. + master_key = key["master"] + self.assertEqual(len(master_key["keys"]), 1) + self.assertTrue("ed25519:" + remote_master_key in master_key["keys"].keys()) + self.assertTrue(remote_master_key in master_key["keys"].values()) + + # Check that the self-signing key is the one returned by the mock. + self_signing_key = key["self_signing"] + self.assertEqual(len(self_signing_key["keys"]), 1) + self.assertTrue( + "ed25519:" + remote_self_signing_key in self_signing_key["keys"].keys(), + ) + self.assertTrue(remote_self_signing_key in self_signing_key["keys"].values()) diff --git a/tests/test_federation.py b/tests/test_federation.py index d91053fd35f..d6bb87f8de7 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -23,7 +23,7 @@ import urllib.parse from http import HTTPStatus from typing import Callable, Optional, Tuple, TypeVar, Union -from unittest.mock import AsyncMock, Mock +from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor @@ -34,7 +34,6 @@ from synapse.events.utils import strip_event from synapse.federation.federation_base import event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse -from synapse.handlers.device import DeviceListUpdater from synapse.http.matrixfederationclient import ( ByteParser, ) @@ -45,7 +44,6 @@ from synapse.server import HomeServer from synapse.types import JsonDict from synapse.util import Clock -from synapse.util.retryutils import NotRetryingDestination from tests import unittest from tests.utils import test_timeout @@ -459,133 +457,6 @@ async def put_json( self.helper.join(room_id, user1_id, tok=user1_tok) -class DeviceListResyncTestCase(unittest.HomeserverTestCase): - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - self.store = self.hs.get_datastores().main - - def test_retry_device_list_resync(self) -> None: - """Tests that device lists are marked as stale if they couldn't be synced, and - that stale device lists are retried periodically. - """ - remote_user_id = "@john:test_remote" - remote_origin = "test_remote" - - # Track the number of attempts to resync the user's device list. - self.resync_attempts = 0 - - # When this function is called, increment the number of resync attempts (only if - # we're querying devices for the right user ID), then raise a - # NotRetryingDestination error to fail the resync gracefully. - def query_user_devices( - destination: str, user_id: str, timeout: int = 30000 - ) -> JsonDict: - if user_id == remote_user_id: - self.resync_attempts += 1 - - raise NotRetryingDestination(0, 0, destination) - - # Register the mock on the federation client. - federation_client = self.hs.get_federation_client() - federation_client.query_user_devices = Mock(side_effect=query_user_devices) # type: ignore[method-assign] - - # Register a mock on the store so that the incoming update doesn't fail because - # we don't share a room with the user. - self.store.get_rooms_for_user = AsyncMock(return_value=["!someroom:test"]) - - # Manually inject a fake device list update. We need this update to include at - # least one prev_id so that the user's device list will need to be retried. - device_list_updater = self.hs.get_device_handler().device_list_updater - assert isinstance(device_list_updater, DeviceListUpdater) - self.get_success( - device_list_updater.incoming_device_list_update( - origin=remote_origin, - edu_content={ - "deleted": False, - "device_display_name": "Mobile", - "device_id": "QBUAZIFURK", - "prev_id": [5], - "stream_id": 6, - "user_id": remote_user_id, - }, - ) - ) - - # Check that there was one resync attempt. - self.assertEqual(self.resync_attempts, 1) - - # Check that the resync attempt failed and caused the user's device list to be - # marked as stale. - need_resync = self.get_success( - self.store.get_user_ids_requiring_device_list_resync() - ) - self.assertIn(remote_user_id, need_resync) - - # Check that waiting for 30 seconds caused Synapse to retry resyncing the device - # list. - self.reactor.advance(30) - self.assertEqual(self.resync_attempts, 2) - - def test_cross_signing_keys_retry(self) -> None: - """Tests that resyncing a device list correctly processes cross-signing keys from - the remote server. - """ - remote_user_id = "@john:test_remote" - remote_master_key = "85T7JXPFBAySB/jwby4S3lBPTqY3+Zg53nYuGmu1ggY" - remote_self_signing_key = "QeIiFEjluPBtI7WQdG365QKZcFs9kqmHir6RBD0//nQ" - - # Register mock device list retrieval on the federation client. - federation_client = self.hs.get_federation_client() - federation_client.query_user_devices = AsyncMock( # type: ignore[method-assign] - return_value={ - "user_id": remote_user_id, - "stream_id": 1, - "devices": [], - "master_key": { - "user_id": remote_user_id, - "usage": ["master"], - "keys": {"ed25519:" + remote_master_key: remote_master_key}, - }, - "self_signing_key": { - "user_id": remote_user_id, - "usage": ["self_signing"], - "keys": { - "ed25519:" + remote_self_signing_key: remote_self_signing_key - }, - }, - } - ) - - # Resync the device list. - device_handler = self.hs.get_device_handler() - self.get_success( - device_handler.device_list_updater.multi_user_device_resync( - [remote_user_id] - ), - ) - - # Retrieve the cross-signing keys for this user. - keys = self.get_success( - self.store.get_e2e_cross_signing_keys_bulk(user_ids=[remote_user_id]), - ) - self.assertIn(remote_user_id, keys) - key = keys[remote_user_id] - assert key is not None - - # Check that the master key is the one returned by the mock. - master_key = key["master"] - self.assertEqual(len(master_key["keys"]), 1) - self.assertTrue("ed25519:" + remote_master_key in master_key["keys"].keys()) - self.assertTrue(remote_master_key in master_key["keys"].values()) - - # Check that the self-signing key is the one returned by the mock. - self_signing_key = key["self_signing"] - self.assertEqual(len(self_signing_key["keys"]), 1) - self.assertTrue( - "ed25519:" + remote_self_signing_key in self_signing_key["keys"].keys(), - ) - self.assertTrue(remote_self_signing_key in self_signing_key["keys"].values()) - - class StripUnsignedFromEventsTestCase(unittest.TestCase): def test_strip_unauthorized_unsigned_values(self) -> None: event1 = { From 4f0cf80f75f38e42aee19fec522df294f0cf40cc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 13:19:39 -0600 Subject: [PATCH 17/45] Move `StripUnsignedFromEventsTestCase` to a more specific file --- tests/federation/test_federation_server.py | 96 +++++++++++++++++++--- tests/test_federation.py | 71 ---------------- 2 files changed, 86 insertions(+), 81 deletions(-) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 88261450b1f..c0f568a02bd 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -25,9 +25,10 @@ from twisted.test.proto_helpers import MemoryReactor -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.events import EventBase, make_event_from_dict +from synapse.federation.federation_base import event_from_pdu_json from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer @@ -85,6 +86,18 @@ async def failing_handler(_origin: str, _content: JsonDict) -> None: self.assertEqual(500, channel.code, channel.result) +def _create_acl_event(content: JsonDict) -> EventBase: + return make_event_from_dict( + { + "room_id": "!a:b", + "event_id": "$a:b", + "type": "m.room.server_acls", + "sender": "@a:b", + "content": content, + } + ) + + class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]}) @@ -355,13 +368,76 @@ def test_send_join_contributes_to_room_join_rate_limit_and_is_limited(self) -> N # is probably sufficient to reassure that the bucket is updated. -def _create_acl_event(content: JsonDict) -> EventBase: - return make_event_from_dict( - { - "room_id": "!a:b", - "event_id": "$a:b", - "type": "m.room.server_acls", - "sender": "@a:b", - "content": content, +class StripUnsignedFromEventsTestCase(unittest.TestCase): + """ + Test to make sure that we handle the raw JSON events from federation carefully and + strip anything that shouldn't be there. + """ + + def test_strip_unauthorized_unsigned_values(self) -> None: + event1 = { + "sender": "@baduser:test.serv", + "state_key": "@baduser:test.serv", + "event_id": "$event1:test.serv", + "depth": 1000, + "origin_server_ts": 1, + "type": "m.room.member", + "origin": "test.servx", + "content": {"membership": "join"}, + "auth_events": [], + "unsigned": {"malicious garbage": "hackz", "more warez": "more hackz"}, } - ) + filtered_event = event_from_pdu_json(event1, RoomVersions.V1) + # Make sure unauthorized fields are stripped from unsigned + self.assertNotIn("more warez", filtered_event.unsigned) + + def test_strip_event_maintains_allowed_fields(self) -> None: + event2 = { + "sender": "@baduser:test.serv", + "state_key": "@baduser:test.serv", + "event_id": "$event2:test.serv", + "depth": 1000, + "origin_server_ts": 1, + "type": "m.room.member", + "origin": "test.servx", + "auth_events": [], + "content": {"membership": "join"}, + "unsigned": { + "malicious garbage": "hackz", + "more warez": "more hackz", + "age": 14, + "invite_room_state": [], + }, + } + + filtered_event2 = event_from_pdu_json(event2, RoomVersions.V1) + self.assertIn("age", filtered_event2.unsigned) + self.assertEqual(14, filtered_event2.unsigned["age"]) + self.assertNotIn("more warez", filtered_event2.unsigned) + # Invite_room_state is allowed in events of type m.room.member + self.assertIn("invite_room_state", filtered_event2.unsigned) + self.assertEqual([], filtered_event2.unsigned["invite_room_state"]) + + def test_strip_event_removes_fields_based_on_event_type(self) -> None: + event3 = { + "sender": "@baduser:test.serv", + "state_key": "@baduser:test.serv", + "event_id": "$event3:test.serv", + "depth": 1000, + "origin_server_ts": 1, + "type": "m.room.power_levels", + "origin": "test.servx", + "content": {}, + "auth_events": [], + "unsigned": { + "malicious garbage": "hackz", + "more warez": "more hackz", + "age": 14, + "invite_room_state": [], + }, + } + filtered_event3 = event_from_pdu_json(event3, RoomVersions.V1) + self.assertIn("age", filtered_event3.unsigned) + # Invite_room_state field is only permitted in event type m.room.member + self.assertNotIn("invite_room_state", filtered_event3.unsigned) + self.assertNotIn("more warez", filtered_event3.unsigned) diff --git a/tests/test_federation.py b/tests/test_federation.py index d6bb87f8de7..1f0b7b4bbbe 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -32,7 +32,6 @@ from synapse.api.room_versions import RoomVersions from synapse.events import make_event_from_dict from synapse.events.utils import strip_event -from synapse.federation.federation_base import event_from_pdu_json from synapse.federation.transport.client import SendJoinResponse from synapse.http.matrixfederationclient import ( ByteParser, @@ -455,73 +454,3 @@ async def put_json( # User1 joins the room self.helper.join(room_id, user1_id, tok=user1_tok) - - -class StripUnsignedFromEventsTestCase(unittest.TestCase): - def test_strip_unauthorized_unsigned_values(self) -> None: - event1 = { - "sender": "@baduser:test.serv", - "state_key": "@baduser:test.serv", - "event_id": "$event1:test.serv", - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.member", - "origin": "test.servx", - "content": {"membership": "join"}, - "auth_events": [], - "unsigned": {"malicious garbage": "hackz", "more warez": "more hackz"}, - } - filtered_event = event_from_pdu_json(event1, RoomVersions.V1) - # Make sure unauthorized fields are stripped from unsigned - self.assertNotIn("more warez", filtered_event.unsigned) - - def test_strip_event_maintains_allowed_fields(self) -> None: - event2 = { - "sender": "@baduser:test.serv", - "state_key": "@baduser:test.serv", - "event_id": "$event2:test.serv", - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.member", - "origin": "test.servx", - "auth_events": [], - "content": {"membership": "join"}, - "unsigned": { - "malicious garbage": "hackz", - "more warez": "more hackz", - "age": 14, - "invite_room_state": [], - }, - } - - filtered_event2 = event_from_pdu_json(event2, RoomVersions.V1) - self.assertIn("age", filtered_event2.unsigned) - self.assertEqual(14, filtered_event2.unsigned["age"]) - self.assertNotIn("more warez", filtered_event2.unsigned) - # Invite_room_state is allowed in events of type m.room.member - self.assertIn("invite_room_state", filtered_event2.unsigned) - self.assertEqual([], filtered_event2.unsigned["invite_room_state"]) - - def test_strip_event_removes_fields_based_on_event_type(self) -> None: - event3 = { - "sender": "@baduser:test.serv", - "state_key": "@baduser:test.serv", - "event_id": "$event3:test.serv", - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.power_levels", - "origin": "test.servx", - "content": {}, - "auth_events": [], - "unsigned": { - "malicious garbage": "hackz", - "more warez": "more hackz", - "age": 14, - "invite_room_state": [], - }, - } - filtered_event3 = event_from_pdu_json(event3, RoomVersions.V1) - self.assertIn("age", filtered_event3.unsigned) - # Invite_room_state field is only permitted in event type m.room.member - self.assertNotIn("invite_room_state", filtered_event3.unsigned) - self.assertNotIn("more warez", filtered_event3.unsigned) From 741f25649176d3256d8e731814e05ec89a4fcb1f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 13:20:47 -0600 Subject: [PATCH 18/45] Fix extremities typo --- tests/handlers/test_federation_event.py | 2 +- tests/test_federation.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_federation_event.py b/tests/handlers/test_federation_event.py index 5db10fa74c2..61b0efb87e2 100644 --- a/tests/handlers/test_federation_event.py +++ b/tests/handlers/test_federation_event.py @@ -375,7 +375,7 @@ def test_process_pulled_event_clears_backfill_attempts_after_being_successfully_ In this test, we pretend we are processing a "pulled" event via backfill. The pulled event succesfully processes and the backward - extremeties are updated along with clearing out any failed pull attempts + extremities are updated along with clearing out any failed pull attempts for those old extremities. We check that we correctly cleared failed pull attempts of the diff --git a/tests/test_federation.py b/tests/test_federation.py index 1f0b7b4bbbe..b7e7b15dcbe 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -83,7 +83,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.storage_controllers.state.get_current_state(self.room_id) ) - # Figure out what the forward extremeties in the room are (the most recent + # Figure out what the forward extremities in the room are (the most recent # events that aren't tied into the DAG) forward_extremity_event_ids = self.get_success( self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) @@ -149,7 +149,7 @@ async def post_json( self.http_client.post_json = post_json - # Figure out what the forward extremeties in the room are (the most recent + # Figure out what the forward extremities in the room are (the most recent # events that aren't tied into the DAG) forward_extremity_event_ids = self.get_success( self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) From a736ead8925d875c71adc4f9bbc387624353b2d8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 15:11:32 -0600 Subject: [PATCH 19/45] More robust test timeout --- ...test_federation_out_of_band_membership.py} | 203 +++++------------- tests/federation/test_federation_server.py | 151 +++++++++++++ tests/utils.py | 20 +- 3 files changed, 223 insertions(+), 151 deletions(-) rename tests/{test_federation.py => federation/test_federation_out_of_band_membership.py} (65%) diff --git a/tests/test_federation.py b/tests/federation/test_federation_out_of_band_membership.py similarity index 65% rename from tests/test_federation.py rename to tests/federation/test_federation_out_of_band_membership.py index b7e7b15dcbe..11f033905d8 100644 --- a/tests/test_federation.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -19,6 +19,7 @@ # # +import time import logging import urllib.parse from http import HTTPStatus @@ -28,20 +29,21 @@ from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventContentFields, EventTypes, Membership -from synapse.api.errors import FederationError from synapse.api.room_versions import RoomVersions -from synapse.events import make_event_from_dict +from synapse.events import make_event_from_dict, EventBase from synapse.events.utils import strip_event from synapse.federation.transport.client import SendJoinResponse from synapse.http.matrixfederationclient import ( ByteParser, ) from synapse.http.types import QueryParams -from synapse.logging.context import LoggingContext from synapse.rest import admin from synapse.rest.client import login, room, sync from synapse.server import HomeServer -from synapse.types import JsonDict +from synapse.types import JsonDict, MutableStateMap, StateMap +from synapse.types.handlers.sliding_sync import ( + StateValues, +) from synapse.util import Clock from tests import unittest @@ -50,154 +52,28 @@ logger = logging.getLogger(__name__) -class MessageAcceptTests(unittest.FederatingHomeserverTestCase): - """ - Tests to make sure that we don't accept flawed events from federation (incoming). - """ - - servlets = [ - admin.register_servlets, - login.register_servlets, - room.register_servlets, - ] - - def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: - self.http_client = Mock() - return self.setup_test_homeserver(federation_http_client=self.http_client) - - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: - super().prepare(reactor, clock, hs) - - self.store = self.hs.get_datastores().main - self.storage_controllers = hs.get_storage_controllers() - self.federation_event_handler = self.hs.get_federation_event_handler() - - # Create a local room - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") - self.room_id = self.helper.create_room_as( - user1_id, tok=user1_tok, is_public=True - ) - - state_map = self.get_success( - self.storage_controllers.state.get_current_state(self.room_id) - ) - - # Figure out what the forward extremities in the room are (the most recent - # events that aren't tied into the DAG) - forward_extremity_event_ids = self.get_success( - self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) - ) - - # Join a remote user to the room that will attempt to send bad events - self.remote_bad_user_id = f"@baduser:{self.OTHER_SERVER_NAME}" - self.remote_bad_user_join_event = make_event_from_dict( - self.add_hashes_and_signatures_from_other_server( - { - "room_id": self.room_id, - "sender": self.remote_bad_user_id, - "state_key": self.remote_bad_user_id, - "depth": 1000, - "origin_server_ts": 1, - "type": EventTypes.Member, - "content": {"membership": Membership.JOIN}, - "auth_events": [ - state_map[(EventTypes.Create, "")].event_id, - state_map[(EventTypes.JoinRules, "")].event_id, - ], - "prev_events": list(forward_extremity_event_ids), - } - ), - room_version=RoomVersions.V10, - ) - - # Send the join, it should return None (which is not an error) - self.assertEqual( - self.get_success( - self.federation_event_handler.on_receive_pdu( - self.OTHER_SERVER_NAME, self.remote_bad_user_join_event - ) - ), - None, - ) - - # Make sure we actually joined the room - self.assertEqual( - self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)), - {self.remote_bad_user_join_event.event_id}, - ) - - def test_cant_hide_direct_ancestors(self) -> None: - """ - If you send a message, you must be able to provide the direct - prev_events that said event references. - """ +def required_state_json_to_state_map(required_state: JsonDict) -> StateMap[EventBase]: + state_map: MutableStateMap[EventBase] = {} - async def post_json( - destination: str, - path: str, - data: Optional[JsonDict] = None, - long_retries: bool = False, - timeout: Optional[int] = None, - ignore_backoff: bool = False, - args: Optional[QueryParams] = None, - ) -> Union[JsonDict, list]: - # If it asks us for new missing events, give them NOTHING - if path.startswith("/_matrix/federation/v1/get_missing_events/"): - return {"events": []} - return {} - - self.http_client.post_json = post_json - - # Figure out what the forward extremities in the room are (the most recent - # events that aren't tied into the DAG) - forward_extremity_event_ids = self.get_success( - self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) + for state_event_dict in required_state: + state_map[(state_event_dict["type"], state_event_dict["state_key"])] = ( + make_event_from_dict(state_event_dict) ) - # Now lie about an event's prev_events - lying_event = make_event_from_dict( - self.add_hashes_and_signatures_from_other_server( - { - "room_id": self.room_id, - "sender": self.remote_bad_user_id, - "depth": 1000, - "origin_server_ts": 1, - "type": "m.room.message", - "content": {"body": "hewwo?"}, - "auth_events": [], - "prev_events": ["$missing_prev_event"] - + list(forward_extremity_event_ids), - } - ), - room_version=RoomVersions.V10, - ) - - with LoggingContext("test-context"): - failure = self.get_failure( - self.federation_event_handler.on_receive_pdu( - self.OTHER_SERVER_NAME, lying_event - ), - FederationError, - ) - - # on_receive_pdu should throw an error - self.assertEqual( - failure.value.args[0], - ( - "ERROR 403: Your server isn't divulging details about prev_events " - "referenced in this event." - ), - ) - - # Make sure the invalid event isn't there - extrem = self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)) - self.assertEqual(extrem, {self.remote_bad_user_join_event.event_id}) + return state_map class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): """ - Tests to make sure that we can join remote rooms over federation. + Tests to make sure that interactions with out-of-band membership (outliers) works as + expected. + + - invites received over federation, before we join the room + - *rejections* for said invites + - knock events for rooms that we would like to join but have not yet joined. + + See the "Out-of-band membership events" section in + `docs/development/room-dag-concepts.md` for more information. """ servlets = [ @@ -341,19 +217,29 @@ def test_asdf(self) -> None: "lists": { "foo-list": { "ranges": [[0, 1]], - "required_state": [], + "required_state": [(EventTypes.Member, StateValues.WILDCARD)], "timeline_limit": 0, } } } # Sync until the local user1 can see the invite - with test_timeout(5): + with test_timeout( + 3, + "Unable to find user1's invite event in the room", + ): while True: response_body, _ = self.do_sync(sync_body, tok=user1_tok) - if room_id in response_body["rooms"].keys(): + if ( + room_id in response_body["rooms"].keys() + # If they have `invite_state` for the room, they are invited + and len(response_body["rooms"][room_id].get("invite_state", [])) > 0 + ): break + # Prevent tight-looping to allow the `test_timeout` to work + time.sleep(0.1) + user1_join_membership_event_template = make_event_from_dict( { "room_id": room_id, @@ -454,3 +340,24 @@ async def put_json( # User1 joins the room self.helper.join(room_id, user1_id, tok=user1_tok) + + # Sync until the local user1 can see that they are now joined to the room + with test_timeout( + 3, + "Unable to find user1's join event in the room", + ): + while True: + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + logger.info("response_body %s", response_body) + if room_id in response_body["rooms"].keys(): + required_state_map = required_state_json_to_state_map( + response_body["rooms"][room_id]["required_state"] + ) + if ( + required_state_map.get((EventTypes.Member, user1_id)) + is not None + ): + break + + # Prevent tight-looping to allow the `test_timeout` to work + time.sleep(0.1) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index c0f568a02bd..42dc8447349 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -20,15 +20,21 @@ # import logging from http import HTTPStatus +from typing import Optional, Union +from unittest.mock import Mock from parameterized import parameterized from twisted.test.proto_helpers import MemoryReactor +from synapse.api.constants import EventTypes, Membership +from synapse.api.errors import FederationError from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.events import EventBase, make_event_from_dict from synapse.federation.federation_base import event_from_pdu_json +from synapse.http.types import QueryParams +from synapse.logging.context import LoggingContext from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer @@ -98,6 +104,151 @@ def _create_acl_event(content: JsonDict) -> EventBase: ) +class MessageAcceptTests(unittest.FederatingHomeserverTestCase): + """ + Tests to make sure that we don't accept flawed events from federation (incoming). + """ + + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: + self.http_client = Mock() + return self.setup_test_homeserver(federation_http_client=self.http_client) + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + super().prepare(reactor, clock, hs) + + self.store = self.hs.get_datastores().main + self.storage_controllers = hs.get_storage_controllers() + self.federation_event_handler = self.hs.get_federation_event_handler() + + # Create a local room + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + self.room_id = self.helper.create_room_as( + user1_id, tok=user1_tok, is_public=True + ) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(self.room_id) + ) + + # Figure out what the forward extremities in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) + ) + + # Join a remote user to the room that will attempt to send bad events + self.remote_bad_user_id = f"@baduser:{self.OTHER_SERVER_NAME}" + self.remote_bad_user_join_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": self.room_id, + "sender": self.remote_bad_user_id, + "state_key": self.remote_bad_user_id, + "depth": 1000, + "origin_server_ts": 1, + "type": EventTypes.Member, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + state_map[(EventTypes.Create, "")].event_id, + state_map[(EventTypes.JoinRules, "")].event_id, + ], + "prev_events": list(forward_extremity_event_ids), + } + ), + room_version=RoomVersions.V10, + ) + + # Send the join, it should return None (which is not an error) + self.assertEqual( + self.get_success( + self.federation_event_handler.on_receive_pdu( + self.OTHER_SERVER_NAME, self.remote_bad_user_join_event + ) + ), + None, + ) + + # Make sure we actually joined the room + self.assertEqual( + self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)), + {self.remote_bad_user_join_event.event_id}, + ) + + def test_cant_hide_direct_ancestors(self) -> None: + """ + If you send a message, you must be able to provide the direct + prev_events that said event references. + """ + + async def post_json( + destination: str, + path: str, + data: Optional[JsonDict] = None, + long_retries: bool = False, + timeout: Optional[int] = None, + ignore_backoff: bool = False, + args: Optional[QueryParams] = None, + ) -> Union[JsonDict, list]: + # If it asks us for new missing events, give them NOTHING + if path.startswith("/_matrix/federation/v1/get_missing_events/"): + return {"events": []} + return {} + + self.http_client.post_json = post_json + + # Figure out what the forward extremities in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(self.room_id) + ) + + # Now lie about an event's prev_events + lying_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": self.room_id, + "sender": self.remote_bad_user_id, + "depth": 1000, + "origin_server_ts": 1, + "type": "m.room.message", + "content": {"body": "hewwo?"}, + "auth_events": [], + "prev_events": ["$missing_prev_event"] + + list(forward_extremity_event_ids), + } + ), + room_version=RoomVersions.V10, + ) + + with LoggingContext("test-context"): + failure = self.get_failure( + self.federation_event_handler.on_receive_pdu( + self.OTHER_SERVER_NAME, lying_event + ), + FederationError, + ) + + # on_receive_pdu should throw an error + self.assertEqual( + failure.value.args[0], + ( + "ERROR 403: Your server isn't divulging details about prev_events " + "referenced in this event." + ), + ) + + # Make sure the invalid event isn't there + extrem = self.get_success(self.store.get_latest_event_ids_in_room(self.room_id)) + self.assertEqual(extrem, {self.remote_bad_user_join_event.event_id}) + + class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]}) diff --git a/tests/utils.py b/tests/utils.py index 9fd26ef348e..5ad7883400b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -38,6 +38,7 @@ import attr from typing_extensions import Literal, ParamSpec +import threading from synapse.api.constants import EventTypes from synapse.api.room_versions import RoomVersions @@ -399,11 +400,24 @@ class TestTimeout(Exception): class test_timeout: + """ + FIXME: This implementation is not robust against other code tight-looping and + preventing the signals propagating and timing out the test. You may need to add + `time.sleep(0.1)` to your code in order to allow this timeout to work correctly. + + ```py + with test_timeout(3): + while True: + my_checking_func() + time.sleep(0.1) + ``` + """ + def __init__(self, seconds: int, error_message: Optional[str] = None) -> None: - if error_message is None: - error_message = "test timed out after {}s.".format(seconds) + self.error_message = f"Test timed out after {seconds}s" + if error_message is not None: + self.error_message += f": {error_message}" self.seconds = seconds - self.error_message = error_message def handle_timeout(self, signum: int, frame: Optional[FrameType]) -> None: raise TestTimeout(self.error_message) From 70cbff81503e201d31521bc91a752b134f7df6a7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 15:31:53 -0600 Subject: [PATCH 20/45] Clean up test --- .../test_federation_out_of_band_membership.py | 74 +++++++++++++------ tests/utils.py | 1 - 2 files changed, 52 insertions(+), 23 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 11f033905d8..8c67d7dfacc 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -19,18 +19,18 @@ # # -import time import logging +import time import urllib.parse from http import HTTPStatus -from typing import Callable, Optional, Tuple, TypeVar, Union +from typing import Any, Callable, Optional, Tuple, TypeVar, Union from unittest.mock import Mock from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.api.room_versions import RoomVersions -from synapse.events import make_event_from_dict, EventBase +from synapse.events import EventBase, make_event_from_dict from synapse.events.utils import strip_event from synapse.federation.transport.client import SendJoinResponse from synapse.http.matrixfederationclient import ( @@ -52,13 +52,31 @@ logger = logging.getLogger(__name__) -def required_state_json_to_state_map(required_state: JsonDict) -> StateMap[EventBase]: +def required_state_json_to_state_map(required_state: Any) -> StateMap[EventBase]: state_map: MutableStateMap[EventBase] = {} - for state_event_dict in required_state: - state_map[(state_event_dict["type"], state_event_dict["state_key"])] = ( - make_event_from_dict(state_event_dict) - ) + # Scrutinize JSON values to ensure it's in the expected format + if isinstance(required_state, list): + for state_event_dict in required_state: + if isinstance(state_event_dict, dict): + event_type = state_event_dict["type"] + event_state_key = state_event_dict["state_key"] + + if isinstance(event_type, str) and isinstance(event_state_key, str): + state_map[(event_type, event_state_key)] = make_event_from_dict( + state_event_dict + ) + else: + # Yell because we're in a test and this is unexpected + raise AssertionError( + "Each event in `required_state` should have a `type` and `state_key` as strings" + ) + else: + # Yell because we're in a test and this is unexpected + raise AssertionError("`required_state` should be a list of event dicts") + else: + # Yell because we're in a test and this is unexpected + raise AssertionError("`required_state` should be a list of event dicts") return state_map @@ -128,16 +146,23 @@ def do_sync( return channel.json_body, channel.json_body["pos"] - def test_asdf(self) -> None: - # Create a local room + def test_can_join_from_out_of_band_invite(self) -> None: + """ + Test to make sure that we can join a room that we were invited to over federation. + + 1. The remote user invites our local user to a room on their remote server (which + creates an out-of-band invite membership for user1 on our local server). + 2. The local user notices the invite from `/sync`. + 3. The local user joins the room. + 4. The local user can see that they are now joined to the room from `/sync`. + """ + # Create a local user user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") - # user2_id = self.register_user("user2", "pass") - # user2_tok = self.login(user2_id, "pass") # Create a remote room - room_creator_user_id = f"@user:{self.OTHER_SERVER_NAME}" - room_id = f"!foo:{self.OTHER_SERVER_NAME}" + room_creator_user_id = f"@remote-user:{self.OTHER_SERVER_NAME}" + room_id = f"!remote-room:{self.OTHER_SERVER_NAME}" room_version = RoomVersions.V10 room_create_event = make_event_from_dict( @@ -260,6 +285,12 @@ def test_asdf(self) -> None: T = TypeVar("T") + # Mock the remote server responding to our HTTP requests + # + # We're going to mock the following endpoints so that user1 can join the room: + # - GET /_matrix/federation/v1/make_join/{room_id}/{user_id} + # - PUT /_matrix/federation/v2/send_join/{room_id}/{user_id} + # async def get_json( destination: str, path: str, @@ -270,8 +301,6 @@ async def get_json( try_trailing_slash_on_400: bool = False, parser: Optional[ByteParser[T]] = None, ) -> Union[JsonDict, T]: - logger.info("asdf get_json %s %s", destination, path) - if ( path == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(room_id)}/{urllib.parse.quote_plus(user1_id)}" @@ -281,7 +310,9 @@ async def get_json( "room_version": room_version.identifier, } - return {} + raise NotImplementedError( + f"We have not mocked a response for `get_json(...)` for the following endpoint yet: {destination}{path}" + ) self.federation_http_client.get_json.side_effect = get_json @@ -298,9 +329,7 @@ async def put_json( try_trailing_slash_on_400: bool = False, parser: Optional[ByteParser[T]] = None, backoff_on_all_error_codes: bool = False, - ) -> Union[JsonDict, T]: - logger.info("asdf put_json %s %s parser=%s", destination, path, parser) - + ) -> Union[JsonDict, T, SendJoinResponse]: if ( path.startswith( f"/_matrix/federation/v2/send_join/{urllib.parse.quote_plus(room_id)}/" @@ -334,7 +363,9 @@ async def put_json( ], ) - return {} + raise NotImplementedError( + f"We have not mocked a response for `put_json(...)` for the following endpoint yet: {destination}{path}" + ) self.federation_http_client.put_json.side_effect = put_json @@ -348,7 +379,6 @@ async def put_json( ): while True: response_body, _ = self.do_sync(sync_body, tok=user1_tok) - logger.info("response_body %s", response_body) if room_id in response_body["rooms"].keys(): required_state_map = required_state_json_to_state_map( response_body["rooms"][room_id]["required_state"] diff --git a/tests/utils.py b/tests/utils.py index 5ad7883400b..af43c8015cf 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -38,7 +38,6 @@ import attr from typing_extensions import Literal, ParamSpec -import threading from synapse.api.constants import EventTypes from synapse.api.room_versions import RoomVersions From 9716478082eff4a1221df5c7073accdf2c755429 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 15:37:50 -0600 Subject: [PATCH 21/45] More cleanup --- .../test_federation_out_of_band_membership.py | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 8c67d7dfacc..c8674262fd6 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -162,13 +162,13 @@ def test_can_join_from_out_of_band_invite(self) -> None: # Create a remote room room_creator_user_id = f"@remote-user:{self.OTHER_SERVER_NAME}" - room_id = f"!remote-room:{self.OTHER_SERVER_NAME}" + remote_room_id = f"!remote-room:{self.OTHER_SERVER_NAME}" room_version = RoomVersions.V10 room_create_event = make_event_from_dict( self.add_hashes_and_signatures_from_other_server( { - "room_id": room_id, + "room_id": remote_room_id, "sender": room_creator_user_id, "depth": 1, "origin_server_ts": 1, @@ -190,7 +190,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: creator_membership_event = make_event_from_dict( self.add_hashes_and_signatures_from_other_server( { - "room_id": room_id, + "room_id": remote_room_id, "sender": room_creator_user_id, "depth": 2, "origin_server_ts": 2, @@ -208,7 +208,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: user1_invite_membership_event = make_event_from_dict( self.add_hashes_and_signatures_from_other_server( { - "room_id": room_id, + "room_id": remote_room_id, "sender": room_creator_user_id, "depth": 3, "origin_server_ts": 3, @@ -226,7 +226,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: ) channel = self.make_signed_federation_request( "PUT", - f"/_matrix/federation/v2/invite/{room_id}/{user1_invite_membership_event.event_id}", + f"/_matrix/federation/v2/invite/{remote_room_id}/{user1_invite_membership_event.event_id}", content={ "event": user1_invite_membership_event.get_dict(), "invite_room_state": [ @@ -256,9 +256,12 @@ def test_can_join_from_out_of_band_invite(self) -> None: while True: response_body, _ = self.do_sync(sync_body, tok=user1_tok) if ( - room_id in response_body["rooms"].keys() + remote_room_id in response_body["rooms"].keys() # If they have `invite_state` for the room, they are invited - and len(response_body["rooms"][room_id].get("invite_state", [])) > 0 + and len( + response_body["rooms"][remote_room_id].get("invite_state", []) + ) + > 0 ): break @@ -267,7 +270,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: user1_join_membership_event_template = make_event_from_dict( { - "room_id": room_id, + "room_id": remote_room_id, "sender": user1_id, "depth": 4, "origin_server_ts": 4, @@ -285,9 +288,9 @@ def test_can_join_from_out_of_band_invite(self) -> None: T = TypeVar("T") - # Mock the remote server responding to our HTTP requests + # Mock the remote homeserver responding to our HTTP requests # - # We're going to mock the following endpoints so that user1 can join the room: + # We're going to mock the following endpoints so that user1 can join the remote room: # - GET /_matrix/federation/v1/make_join/{room_id}/{user_id} # - PUT /_matrix/federation/v2/send_join/{room_id}/{user_id} # @@ -303,7 +306,7 @@ async def get_json( ) -> Union[JsonDict, T]: if ( path - == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(room_id)}/{urllib.parse.quote_plus(user1_id)}" + == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(remote_room_id)}/{urllib.parse.quote_plus(user1_id)}" ): return { "event": user1_join_membership_event_template.get_pdu_json(), @@ -332,7 +335,7 @@ async def put_json( ) -> Union[JsonDict, T, SendJoinResponse]: if ( path.startswith( - f"/_matrix/federation/v2/send_join/{urllib.parse.quote_plus(room_id)}/" + f"/_matrix/federation/v2/send_join/{urllib.parse.quote_plus(remote_room_id)}/" ) and data is not None and data.get("type") == EventTypes.Member @@ -345,6 +348,8 @@ async def put_json( room_version=room_version, ) + # Since they passed in a `parser`, we need to return the type that + # they're expecting instead of just a `JsonDict` return SendJoinResponse( auth_events=[ room_create_event, @@ -370,7 +375,7 @@ async def put_json( self.federation_http_client.put_json.side_effect = put_json # User1 joins the room - self.helper.join(room_id, user1_id, tok=user1_tok) + self.helper.join(remote_room_id, user1_id, tok=user1_tok) # Sync until the local user1 can see that they are now joined to the room with test_timeout( @@ -379,9 +384,9 @@ async def put_json( ): while True: response_body, _ = self.do_sync(sync_body, tok=user1_tok) - if room_id in response_body["rooms"].keys(): + if remote_room_id in response_body["rooms"].keys(): required_state_map = required_state_json_to_state_map( - response_body["rooms"][room_id]["required_state"] + response_body["rooms"][remote_room_id]["required_state"] ) if ( required_state_map.get((EventTypes.Member, user1_id)) From 05c875cdf79a9c44caf11ea43a747571cf6c8674 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 15:43:33 -0600 Subject: [PATCH 22/45] Better explanations --- tests/federation/test_federation_out_of_band_membership.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index c8674262fd6..2a80c7c7588 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -314,7 +314,8 @@ async def get_json( } raise NotImplementedError( - f"We have not mocked a response for `get_json(...)` for the following endpoint yet: {destination}{path}" + "We have not mocked a response for `get_json(...)` for the following endpoint yet: " + + f"{destination}{path}" ) self.federation_http_client.get_json.side_effect = get_json @@ -343,6 +344,7 @@ async def put_json( # We're assuming this is a `ByteParser[SendJoinResponse]` and parser is not None ): + # As the remote server, we need to sign the event before sending it back user1_join_membership_event_signed = make_event_from_dict( self.add_hashes_and_signatures_from_other_server(data), room_version=room_version, @@ -369,7 +371,8 @@ async def put_json( ) raise NotImplementedError( - f"We have not mocked a response for `put_json(...)` for the following endpoint yet: {destination}{path}" + "We have not mocked a response for `put_json(...)` for the following endpoint yet: " + + f"{destination}{path} with the following body data: {data}" ) self.federation_http_client.put_json.side_effect = put_json From 0a757b683ce8381247d89a1ada36d72b773a0263 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 15:48:42 -0600 Subject: [PATCH 23/45] Reset the mocks when we don't need them anymore --- tests/federation/test_federation_out_of_band_membership.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 2a80c7c7588..745e1921ae8 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -380,6 +380,10 @@ async def put_json( # User1 joins the room self.helper.join(remote_room_id, user1_id, tok=user1_tok) + # Reset the mocks now that user1 has joined the room + self.federation_http_client.get_json.side_effect = None + self.federation_http_client.put_json.side_effect = None + # Sync until the local user1 can see that they are now joined to the room with test_timeout( 3, From d9bf5a961e4cbdf9928eda3333cd48dd26a150cd Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 16:07:18 -0600 Subject: [PATCH 24/45] Add test that reproduces the problem --- .../test_federation_out_of_band_membership.py | 158 ++++++++++++++++-- 1 file changed, 143 insertions(+), 15 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 745e1921ae8..b7ee1c90a61 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -26,10 +26,12 @@ from typing import Any, Callable, Optional, Tuple, TypeVar, Union from unittest.mock import Mock +import attr + from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventContentFields, EventTypes, Membership -from synapse.api.room_versions import RoomVersions +from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.events.utils import strip_event from synapse.federation.transport.client import SendJoinResponse @@ -81,6 +83,16 @@ def required_state_json_to_state_map(required_state: Any) -> StateMap[EventBase] return state_map +@attr.s(slots=True, auto_attribs=True) +class RemoteRoomJoinResult: + remote_room_id: str + room_version: RoomVersion + remote_room_creator_user_id: str + local_user1_id: str + local_user1_tok: str + state_map: StateMap[EventBase] + + class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): """ Tests to make sure that interactions with out-of-band membership (outliers) works as @@ -115,6 +127,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: super().prepare(reactor, clock, hs) self.store = self.hs.get_datastores().main + self.storage_controllers = hs.get_storage_controllers() def do_sync( self, sync_body: JsonDict, *, since: Optional[str] = None, tok: str @@ -146,9 +159,9 @@ def do_sync( return channel.json_body, channel.json_body["pos"] - def test_can_join_from_out_of_band_invite(self) -> None: + def _invite_local_user_to_remote_room_and_join(self) -> RemoteRoomJoinResult: """ - Test to make sure that we can join a room that we were invited to over federation. + Helper to reproduce this scenario: 1. The remote user invites our local user to a room on their remote server (which creates an out-of-band invite membership for user1 on our local server). @@ -156,9 +169,10 @@ def test_can_join_from_out_of_band_invite(self) -> None: 3. The local user joins the room. 4. The local user can see that they are now joined to the room from `/sync`. """ + # Create a local user - user1_id = self.register_user("user1", "pass") - user1_tok = self.login(user1_id, "pass") + local_user1_id = self.register_user("user1", "pass") + local_user1_tok = self.login(local_user1_id, "pass") # Create a remote room room_creator_user_id = f"@remote-user:{self.OTHER_SERVER_NAME}" @@ -213,7 +227,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: "depth": 3, "origin_server_ts": 3, "type": EventTypes.Member, - "state_key": user1_id, + "state_key": local_user1_id, "content": {"membership": Membership.INVITE}, "auth_events": [ room_create_event.event_id, @@ -231,7 +245,6 @@ def test_can_join_from_out_of_band_invite(self) -> None: "event": user1_invite_membership_event.get_dict(), "invite_room_state": [ strip_event(room_create_event), - strip_event(creator_membership_event), ], "room_version": room_version.identifier, }, @@ -254,7 +267,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: "Unable to find user1's invite event in the room", ): while True: - response_body, _ = self.do_sync(sync_body, tok=user1_tok) + response_body, _ = self.do_sync(sync_body, tok=local_user1_tok) if ( remote_room_id in response_body["rooms"].keys() # If they have `invite_state` for the room, they are invited @@ -271,11 +284,11 @@ def test_can_join_from_out_of_band_invite(self) -> None: user1_join_membership_event_template = make_event_from_dict( { "room_id": remote_room_id, - "sender": user1_id, + "sender": local_user1_id, "depth": 4, "origin_server_ts": 4, "type": EventTypes.Member, - "state_key": user1_id, + "state_key": local_user1_id, "content": {"membership": Membership.JOIN}, "auth_events": [ room_create_event.event_id, @@ -306,7 +319,7 @@ async def get_json( ) -> Union[JsonDict, T]: if ( path - == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(remote_room_id)}/{urllib.parse.quote_plus(user1_id)}" + == f"/_matrix/federation/v1/make_join/{urllib.parse.quote_plus(remote_room_id)}/{urllib.parse.quote_plus(local_user1_id)}" ): return { "event": user1_join_membership_event_template.get_pdu_json(), @@ -340,7 +353,7 @@ async def put_json( ) and data is not None and data.get("type") == EventTypes.Member - and data.get("state_key") == user1_id + and data.get("state_key") == local_user1_id # We're assuming this is a `ByteParser[SendJoinResponse]` and parser is not None ): @@ -378,7 +391,7 @@ async def put_json( self.federation_http_client.put_json.side_effect = put_json # User1 joins the room - self.helper.join(remote_room_id, user1_id, tok=user1_tok) + self.helper.join(remote_room_id, local_user1_id, tok=local_user1_tok) # Reset the mocks now that user1 has joined the room self.federation_http_client.get_json.side_effect = None @@ -390,16 +403,131 @@ async def put_json( "Unable to find user1's join event in the room", ): while True: - response_body, _ = self.do_sync(sync_body, tok=user1_tok) + response_body, _ = self.do_sync(sync_body, tok=local_user1_tok) if remote_room_id in response_body["rooms"].keys(): required_state_map = required_state_json_to_state_map( response_body["rooms"][remote_room_id]["required_state"] ) if ( - required_state_map.get((EventTypes.Member, user1_id)) + required_state_map.get((EventTypes.Member, local_user1_id)) is not None ): break # Prevent tight-looping to allow the `test_timeout` to work time.sleep(0.1) + + return RemoteRoomJoinResult( + remote_room_id=remote_room_id, + room_version=room_version, + remote_room_creator_user_id=room_creator_user_id, + local_user1_id=local_user1_id, + local_user1_tok=local_user1_tok, + state_map=self.get_success( + self.storage_controllers.state.get_current_state(remote_room_id) + ), + ) + + def test_can_join_from_out_of_band_invite(self) -> None: + """ + Test to make sure that we can join a room that we were invited to over + federation; even if our server has never participated in the room before. + """ + self._invite_local_user_to_remote_room_and_join() + + def test_can_join_from_out_of_band_invite_after_we_are_already_participating_in_the_room( + self, + ) -> None: + """ + Test to make sure that we can join a room that we were invited to over + federation; even after we are already participating in the room. + """ + remote_room_join_result = self._invite_local_user_to_remote_room_and_join() + remote_room_id = remote_room_join_result.remote_room_id + room_version = remote_room_join_result.room_version + logger.info("asdf remote_room_join_result: %s", remote_room_join_result) + + # Create another local user + local_user2_id = self.register_user("user2", "pass") + local_user2_tok = self.login(local_user2_id, "pass") + + # From the remote homeserver, invite user2 on the local homserver + user2_invite_membership_event = make_event_from_dict( + self.add_hashes_and_signatures_from_other_server( + { + "room_id": remote_room_id, + "sender": remote_room_join_result.remote_room_creator_user_id, + "depth": 5, + "origin_server_ts": 5, + "type": EventTypes.Member, + "state_key": local_user2_id, + "content": {"membership": Membership.INVITE}, + "auth_events": [ + remote_room_join_result.state_map[ + (EventTypes.Create, "") + ].event_id, + remote_room_join_result.state_map[ + ( + EventTypes.Member, + remote_room_join_result.remote_room_creator_user_id, + ) + ].event_id, + ], + "prev_events": [ + remote_room_join_result.state_map[ + (EventTypes.Member, remote_room_join_result.local_user1_id) + ].event_id + ], + } + ), + room_version=room_version, + ) + channel = self.make_signed_federation_request( + "PUT", + f"/_matrix/federation/v2/invite/{remote_room_id}/{user2_invite_membership_event.event_id}", + content={ + "event": user2_invite_membership_event.get_dict(), + "invite_room_state": [ + strip_event( + remote_room_join_result.state_map[(EventTypes.Create, "")] + ), + ], + "room_version": room_version.identifier, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [(EventTypes.Member, StateValues.WILDCARD)], + "timeline_limit": 0, + } + } + } + + # Sync until the local user2 can see the invite + with test_timeout( + 3, + "Unable to find user2's invite event in the room", + ): + while True: + response_body, _ = self.do_sync(sync_body, tok=local_user2_tok) + if ( + remote_room_id in response_body["rooms"].keys() + # If they have `invite_state` for the room, they are invited + and len( + response_body["rooms"][remote_room_id].get("invite_state", []) + ) + > 0 + ): + break + + # Prevent tight-looping to allow the `test_timeout` to work + time.sleep(0.1) + + # User2 joins the room + self.helper.join( + remote_room_join_result.remote_room_id, local_user2_id, tok=local_user2_tok + ) From f88557575153d41705a2040066a6637d28d2c09e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 16:16:42 -0600 Subject: [PATCH 25/45] Add tests for accepting and rejecting the invite --- .../test_federation_out_of_band_membership.py | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index b7ee1c90a61..7cde8ed7f9f 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -23,6 +23,7 @@ import time import urllib.parse from http import HTTPStatus +from parameterized import parameterized from typing import Any, Callable, Optional, Tuple, TypeVar, Union from unittest.mock import Mock @@ -435,12 +436,16 @@ def test_can_join_from_out_of_band_invite(self) -> None: """ self._invite_local_user_to_remote_room_and_join() - def test_can_join_from_out_of_band_invite_after_we_are_already_participating_in_the_room( - self, + @parameterized.expand( + [("accept invite", Membership.JOIN), ("reject invite", Membership.LEAVE)] + ) + def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the_room( + self, _test_description, membership_action ) -> None: """ - Test to make sure that we can join a room that we were invited to over - federation; even after we are already participating in the room. + Test to make sure that we can do either a) join the room (accept the invite) or + b) reject the invite after being invited to over federation; even if we are + already participating in the room """ remote_room_join_result = self._invite_local_user_to_remote_room_and_join() remote_room_id = remote_room_join_result.remote_room_id @@ -527,7 +532,21 @@ def test_can_join_from_out_of_band_invite_after_we_are_already_participating_in_ # Prevent tight-looping to allow the `test_timeout` to work time.sleep(0.1) - # User2 joins the room - self.helper.join( - remote_room_join_result.remote_room_id, local_user2_id, tok=local_user2_tok - ) + if membership_action == Membership.JOIN: + # User2 joins the room + self.helper.join( + remote_room_join_result.remote_room_id, + local_user2_id, + tok=local_user2_tok, + ) + elif membership_action == Membership.LEAVE: + # User2 rejects the invite + self.helper.leave( + remote_room_join_result.remote_room_id, + local_user2_id, + tok=local_user2_tok, + ) + else: + raise NotImplementedError( + "This test does not support this membership action yet" + ) From 87cfe87f0bb8b40db2a4c0a9a3dbbf312aca821f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 16:47:48 -0600 Subject: [PATCH 26/45] Try to explain regression test --- scripts-dev/complement.sh | 2 +- .../test_federation_out_of_band_membership.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 8c4008b1306..3643dd063ad 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -220,7 +220,7 @@ extra_test_args=() test_packages=( ./tests/csapi - # ./tests + ./tests # ./tests/msc3874 # ./tests/msc3890 # ./tests/msc3391 diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 7cde8ed7f9f..ed78182b6b0 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -445,12 +445,18 @@ def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the """ Test to make sure that we can do either a) join the room (accept the invite) or b) reject the invite after being invited to over federation; even if we are - already participating in the room + already participating in the room. + + This is a regression test to make sure we stress the scenario where even though + we are already participating in the room, local users can still react to invites + regardless of whether the remote server has told us about the invite event (via + a federation `/send` transaction) and we have de-outliered the invite event. + Previously, we would mistakenly throw and error saying the user wasn't in the + room when they tried to join or reject the invite. """ remote_room_join_result = self._invite_local_user_to_remote_room_and_join() remote_room_id = remote_room_join_result.remote_room_id room_version = remote_room_join_result.room_version - logger.info("asdf remote_room_join_result: %s", remote_room_join_result) # Create another local user local_user2_id = self.register_user("user2", "pass") From 33ac6c93a839db9570eacfa4f5e0f57aa4cd5569 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 18:19:00 -0600 Subject: [PATCH 27/45] Harden up the tests --- .../test_federation_out_of_band_membership.py | 104 +++++++++++++++++- 1 file changed, 99 insertions(+), 5 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index ed78182b6b0..9785792c1a3 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -23,11 +23,11 @@ import time import urllib.parse from http import HTTPStatus -from parameterized import parameterized -from typing import Any, Callable, Optional, Tuple, TypeVar, Union +from typing import Any, Callable, Optional, Set, Tuple, TypeVar, Union from unittest.mock import Mock import attr +from parameterized import parameterized from twisted.test.proto_helpers import MemoryReactor @@ -35,6 +35,9 @@ from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.events import EventBase, make_event_from_dict from synapse.events.utils import strip_event +from synapse.federation.federation_base import ( + event_from_pdu_json, +) from synapse.federation.transport.client import SendJoinResponse from synapse.http.matrixfederationclient import ( ByteParser, @@ -116,6 +119,14 @@ class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): sync_endpoint = "/_matrix/client/unstable/org.matrix.simplified_msc3575/sync" + def default_config(self) -> JsonDict: + conf = super().default_config() + # Federation sending is disabled by default in the test environment + # so we need to enable it like this. + conf["federation_sender_instances"] = ["master"] + + return conf + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.federation_http_client = Mock( # spec=MatrixFederationHttpClient @@ -384,6 +395,13 @@ async def put_json( ], ) + if path.startswith("/_matrix/federation/v1/send/") and data is not None: + # Just acknowledge everything hs1 is trying to send hs2 + return { + event_from_pdu_json(pdu, room_version).event_id: {} + for pdu in data.get("pdus", []) + } + raise NotImplementedError( "We have not mocked a response for `put_json(...)` for the following endpoint yet: " + f"{destination}{path} with the following body data: {data}" @@ -440,7 +458,7 @@ def test_can_join_from_out_of_band_invite(self) -> None: [("accept invite", Membership.JOIN), ("reject invite", Membership.LEAVE)] ) def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the_room( - self, _test_description, membership_action + self, _test_description: str, membership_action: str ) -> None: """ Test to make sure that we can do either a) join the room (accept the invite) or @@ -462,6 +480,44 @@ def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the local_user2_id = self.register_user("user2", "pass") local_user2_tok = self.login(local_user2_id, "pass") + T = TypeVar("T") + + # PDU's that hs1 sent to hs2 + collected_pdus_from_hs1_federation_send: Set[str] = set() + + async def put_json( + destination: str, + path: str, + args: Optional[QueryParams] = None, + data: Optional[JsonDict] = None, + json_data_callback: Optional[Callable[[], JsonDict]] = None, + long_retries: bool = False, + timeout: Optional[int] = None, + ignore_backoff: bool = False, + backoff_on_404: bool = False, + try_trailing_slash_on_400: bool = False, + parser: Optional[ByteParser[T]] = None, + backoff_on_all_error_codes: bool = False, + ) -> Union[JsonDict, T]: + if path.startswith("/_matrix/federation/v1/send/") and data is not None: + logger.info("asdf data: %s", data) + for pdu in data.get("pdus", []): + event = event_from_pdu_json(pdu, room_version) + collected_pdus_from_hs1_federation_send.add(event.event_id) + + # Just acknowledge everything hs1 is trying to send hs2 + return { + event_from_pdu_json(pdu, room_version).event_id: {} + for pdu in data.get("pdus", []) + } + + raise NotImplementedError( + "We have not mocked a response for `put_json(...)` for the following endpoint yet: " + + f"{destination}{path} with the following body data: {data}" + ) + + self.federation_http_client.put_json.side_effect = put_json + # From the remote homeserver, invite user2 on the local homserver user2_invite_membership_event = make_event_from_dict( self.add_hashes_and_signatures_from_other_server( @@ -540,19 +596,57 @@ def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the if membership_action == Membership.JOIN: # User2 joins the room - self.helper.join( + join_event = self.helper.join( remote_room_join_result.remote_room_id, local_user2_id, tok=local_user2_tok, ) + expected_pdu_event_id = join_event["event_id"] elif membership_action == Membership.LEAVE: # User2 rejects the invite - self.helper.leave( + leave_event = self.helper.leave( remote_room_join_result.remote_room_id, local_user2_id, tok=local_user2_tok, ) + expected_pdu_event_id = leave_event["event_id"] else: raise NotImplementedError( "This test does not support this membership action yet" ) + + # Sync until the local user2 can see their new membership in the room + with test_timeout( + 3, + "Unable to find user2's new membership event in the room", + ): + while True: + response_body, _ = self.do_sync(sync_body, tok=local_user2_tok) + if membership_action == Membership.JOIN: + if remote_room_id in response_body["rooms"].keys(): + required_state_map = required_state_json_to_state_map( + response_body["rooms"][remote_room_id]["required_state"] + ) + if ( + required_state_map.get((EventTypes.Member, local_user2_id)) + is not None + ): + break + elif membership_action == Membership.LEAVE: + if remote_room_id not in response_body["rooms"].keys(): + break + else: + raise NotImplementedError( + "This test does not support this membership action yet" + ) + + # Prevent tight-looping to allow the `test_timeout` to work + time.sleep(0.1) + + # Make sure that we let hs2 know about the new membership event + self.assertIncludes( + collected_pdus_from_hs1_federation_send, + {expected_pdu_event_id}, + exact=True, + message="Expected to find the event ID of the user2 membership to be sent from hs1 over federation to hs2", + ) From 0d871b6799b1ce830612914a56470099ea37f0ca Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 15 Jan 2025 18:22:36 -0600 Subject: [PATCH 28/45] More hardening --- .../test_federation_out_of_band_membership.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 9785792c1a3..9b0d46480cb 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -345,6 +345,9 @@ async def get_json( self.federation_http_client.get_json.side_effect = get_json + # PDU's that hs1 sent to hs2 + collected_pdus_from_hs1_federation_send: Set[str] = set() + async def put_json( destination: str, path: str, @@ -396,6 +399,10 @@ async def put_json( ) if path.startswith("/_matrix/federation/v1/send/") and data is not None: + for pdu in data.get("pdus", []): + event = event_from_pdu_json(pdu, room_version) + collected_pdus_from_hs1_federation_send.add(event.event_id) + # Just acknowledge everything hs1 is trying to send hs2 return { event_from_pdu_json(pdu, room_version).event_id: {} @@ -436,6 +443,15 @@ async def put_json( # Prevent tight-looping to allow the `test_timeout` to work time.sleep(0.1) + # Nothing needs to be sent from hs1 to hs2 since we already let the other + # homeserver know by doing the `/make_join` and `/send_join` dance. + self.assertIncludes( + collected_pdus_from_hs1_federation_send, + set(), + exact=True, + message="Didn't expect any events to be sent from hs1 over federation to hs2", + ) + return RemoteRoomJoinResult( remote_room_id=remote_room_id, room_version=room_version, @@ -500,7 +516,6 @@ async def put_json( backoff_on_all_error_codes: bool = False, ) -> Union[JsonDict, T]: if path.startswith("/_matrix/federation/v1/send/") and data is not None: - logger.info("asdf data: %s", data) for pdu in data.get("pdus", []): event = event_from_pdu_json(pdu, room_version) collected_pdus_from_hs1_federation_send.add(event.event_id) From 2fbc2fa122a0554aa5e177b9a64017d5f7013ba8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 16:22:17 -0600 Subject: [PATCH 29/45] Allow events to auth correctly (computed state matches our auth events) --- synapse/events/builder.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/synapse/events/builder.py b/synapse/events/builder.py index cb4741b4e0b..828e3dbc617 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -179,12 +179,21 @@ async def build( user_id=self.state_key, room_id=self.room_id, ) - if member_event_id is not None: - # There is no need to check if the membership is actually an - # out-of-band membership (`outlier`) as we would end up with the - # same result either way (adding the member event to the - # `auth_event_ids`). + # There is no need to check if the membership is actually an + # out-of-band membership (`outlier`) as we would end up with the + # same result either way (adding the member event to the + # `auth_event_ids`). + if ( + member_event_id is not None + # We only need to be careful about duplicating the event in the + # `auth_event_ids` list (duplicate `type`/`state_key` is part of the + # authorization rules) + and member_event_id not in auth_event_ids + ): auth_event_ids.append(member_event_id) + # Also make sure to point to the previous membership event that will + # allow this one to happen so the computed state works out. + prev_event_ids.append(member_event_id) format_version = self.room_version.event_format # The types of auth/prev events changes between event versions. From bbeb68a6026bcabbb8df418a207355d2369a8ea6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 16:23:38 -0600 Subject: [PATCH 30/45] Debug logs to figure out why rejected --- synapse/app/_base.py | 1 - synapse/app/homeserver.py | 2 + synapse/config/workers.py | 8 +++- synapse/federation/federation_server.py | 5 ++- synapse/handlers/sync.py | 36 +++++++++++++++++ .../storage/databases/main/events_worker.py | 39 +++++++++++++++++++ synapse/storage/databases/main/stream.py | 32 +++++++++++++++ 7 files changed, 120 insertions(+), 3 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 4cc260d5519..96ee1ba7ca6 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -133,7 +133,6 @@ def start_worker_reactor( config: config object run_command: callable that actually runs the reactor """ - logger = logging.getLogger(config.worker.worker_app) start_reactor( diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 2a824e8457f..e5ffd53b950 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -368,6 +368,8 @@ def setup(config_options: List[str]) -> SynapseHomeServer: synapse.config.logger.setup_logging(hs, config, use_worker_options=False) logger.info("Setting up server") + logger.info("asdf config=%s", config) + logger.info("asdf send_federation=%s", config.worker.send_federation) try: hs.setup() diff --git a/synapse/config/workers.py b/synapse/config/workers.py index ab896be3077..8c0acc10744 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -219,6 +219,7 @@ class WorkerConfig(Config): section = "worker" def read_config(self, config: JsonDict, **kwargs: Any) -> None: + logger.info("asdf WorkerConfig.read_config: %s", config) self.worker_app = config.get("worker_app") # Canonicalise worker_app so that master always has None @@ -274,6 +275,12 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "federation_sender_instances", ) self.send_federation = self.instance_name in federation_sender_instances + logger.info( + "asdf self.send_federation: %s %s %s", + self.send_federation, + self.instance_name, + federation_sender_instances, + ) self.federation_shard_config = ShardedWorkerHandlingConfig( federation_sender_instances ) @@ -574,7 +581,6 @@ def _worker_names_performing_this_duty( Returns: A list of worker instance names handling the given duty. """ - legacy_option = config.get(legacy_option_name, True) worker_instances = config.get(modern_instance_list_name) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index ec34e282d65..a8acf550bd9 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1272,7 +1272,10 @@ async def _process_incoming_pdus_in_room_inner( origin, event ) logger.info( - "✅ handled received PDU in room %s: %s", room_id, event + "✅ handled received PDU in room %s: %s stream_ordering=%s", + room_id, + event, + event.internal_metadata.stream_ordering, ) except FederationError as e: # XXX: Ideally we'd inform the remote we failed to process diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 350c3fa09a1..24778fd3f16 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -583,6 +583,12 @@ async def _wait_for_sync_for_user( async def current_sync_callback( before_token: StreamToken, after_token: StreamToken ) -> Union[SyncResult, E2eeSyncResult]: + logger.info( + "asdf notifier saw new event before_token=%s after_token=%s, syncing with since_token=%s", + before_token.room_key.stream, + after_token.room_key.stream, + since_token.room_key.stream if since_token else None, + ) return await self.current_sync_for_user( sync_config, sync_version, since_token ) @@ -2386,6 +2392,12 @@ async def _generate_sync_entry_for_rooms( since_token = sync_result_builder.since_token user_id = sync_result_builder.sync_config.user.to_string() + logger.info( + "asdf _generate_sync_entry_for_rooms: since_token=%s now_token=%s", + since_token.room_key.stream if since_token else None, + sync_result_builder.now_token.room_key.stream, + ) + blocks_all_rooms = ( sync_result_builder.sync_config.filter_collection.blocks_all_rooms() ) @@ -2453,6 +2465,11 @@ async def _generate_sync_entry_for_rooms( log_kv({"rooms_changed": len(room_changes.room_entries)}) + logger.info( + "asdf _generate_sync_entry_for_rooms rooms_changed: rooms_changed=%s", + room_changes.room_entries, + ) + room_entries = room_changes.room_entries invited = room_changes.invited knocked = room_changes.knocked @@ -2538,6 +2555,12 @@ async def _get_room_changes_for_incremental_sync( assert since_token + logger.info( + "asdf _get_room_changes_for_incremental_sync since_token=%s, now_token=%s", + since_token.room_key.stream if since_token else None, + now_token.room_key.stream, + ) + mem_change_events_by_room_id: Dict[str, List[EventBase]] = {} for event in membership_change_events: mem_change_events_by_room_id.setdefault(event.room_id, []).append(event) @@ -2710,6 +2733,7 @@ async def _get_room_changes_for_incremental_sync( limit=timeline_limit + 1, direction=Direction.BACKWARDS, ) + logger.info("asdf room_to_events results: %s", room_to_events) # We loop through all room ids, even if there are no new events, in case # there are non room events that we need to notify about. @@ -2886,6 +2910,18 @@ async def _generate_room_entry( ) events = room_builder.events + logger.info( + "asdf _generate_room_entry since_token=%s upto_token=%s end_token=%s always_include=%s full_state=%s events=%s", + room_builder.since_token.room_key.stream + if room_builder.since_token + else None, + room_builder.upto_token.room_key.stream, + room_builder.end_token.room_key.stream, + always_include, + full_state, + events, + ) + # We want to shortcut out as early as possible. if not (always_include or account_data or ephemeral or full_state): if events == [] and tags is None: diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index 222df8757ac..1f43612872d 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -798,6 +798,12 @@ async def get_unredacted_events_from_cache_or_db( missing_events_ids = {e for e in event_ids if e not in event_entry_map} + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (1): event_entry_map=%s missing_events_ids=%s", + event_entry_map, + missing_events_ids, + ) + # We now look up if we're already fetching some of the events in the DB, # if so we wait for those lookups to finish instead of pulling the same # events out of the DB multiple times. @@ -861,6 +867,11 @@ async def get_missing_events_from_cache_or_db() -> ( db_missing_events = await self._get_events_from_db( missing_events_ids - missing_events.keys(), ) + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (1): tried to fetch %s but found db_missing_events=%s", + missing_events_ids - missing_events.keys(), + db_missing_events, + ) missing_events.update(db_missing_events) except Exception as e: with PreserveLoggingContext(): @@ -884,6 +895,11 @@ async def get_missing_events_from_cache_or_db() -> ( ) event_entry_map.update(missing_events) + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (2): event_entry_map=%s", + event_entry_map, + ) + if already_fetching_deferreds: # Wait for the other event requests to finish and add their results # to ours. @@ -904,13 +920,36 @@ async def get_missing_events_from_cache_or_db() -> ( if event_id in already_fetching_ids ) + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (3): event_entry_map=%s", + event_entry_map, + ) + + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (4): event_entry_map=%s", + event_entry_map, + ) + if not allow_rejected: + for event_id, entry in event_entry_map.items(): + if entry.event.rejected_reason: + logger.info( + "asdf get_unredacted_events_from_cache_or_db saw rejected_reason for event_id=%s rejected_reason=%s", + event_id, + entry.event.rejected_reason, + ) + event_entry_map = { event_id: entry for event_id, entry in event_entry_map.items() if not entry.event.rejected_reason } + logger.info( + "asdf get_unredacted_events_from_cache_or_db at (5): event_entry_map=%s", + event_entry_map, + ) + return event_entry_map def invalidate_get_event_cache_after_txn( diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index b4258a44362..dde11ec8a54 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -700,6 +700,8 @@ async def get_room_events_stream_for_rooms( When Direction.FORWARDS: from_key < x <= to_key, (ascending order) When Direction.BACKWARDS: from_key >= x > to_key, (descending order) """ + before_room_ids_asdf = room_ids + if direction == Direction.FORWARDS: room_ids = self._events_stream_cache.get_entities_changed( room_ids, from_key.stream @@ -712,6 +714,16 @@ async def get_room_events_stream_for_rooms( else: assert_never(direction) + logger.info( + "asdf get_room_events_stream_for_rooms from_key=%s, to_key=%s limit=%s direction=%s before_room_ids=%s after_room_ids=%s", + from_key.stream, + to_key.stream if to_key else None, + limit, + direction, + before_room_ids_asdf, + room_ids, + ) + if not room_ids: return {} @@ -868,6 +880,14 @@ async def paginate_room_events_by_stream_ordering( else: assert_never(direction) + logger.info( + "asdf paginate_room_events_by_stream_ordering has_changed=%s from_key=%s, to_key=%s direction=%s", + has_changed, + from_key.stream, + to_key.stream if to_key else None, + direction, + ) + if not has_changed: # Token selection matches what we do below if there are no rows return [], to_key if to_key else from_key, False @@ -902,6 +922,12 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: fetched_rows = txn.fetchall() limited = len(fetched_rows) >= 2 * limit + logger.info( + "asdf paginate_room_events_by_stream_ordering fetched_rows=%s sql=%s", + fetched_rows, + sql, + ) + rows = [ _EventDictReturn(event_id, None, stream_ordering) for event_id, instance_name, stream_ordering in fetched_rows @@ -917,6 +943,10 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: ) ] + logger.info( + "asdf paginate_room_events_by_stream_ordering filtered_rows=%s", rows + ) + if len(rows) > limit: limited = True @@ -931,6 +961,8 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: [r.event_id for r in rows], get_prev_content=True ) + logger.info("asdf paginate_room_events_by_stream_ordering ret=%s", ret) + if rows: next_key = generate_next_token( direction=direction, From 11d997068d9b1c8fe08b39781fdff65849bbc632 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 16:23:51 -0600 Subject: [PATCH 31/45] Revert "Debug logs to figure out why rejected" This reverts commit bbeb68a6026bcabbb8df418a207355d2369a8ea6. --- synapse/app/_base.py | 1 + synapse/app/homeserver.py | 2 - synapse/config/workers.py | 8 +--- synapse/federation/federation_server.py | 5 +-- synapse/handlers/sync.py | 36 ----------------- .../storage/databases/main/events_worker.py | 39 ------------------- synapse/storage/databases/main/stream.py | 32 --------------- 7 files changed, 3 insertions(+), 120 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 96ee1ba7ca6..4cc260d5519 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -133,6 +133,7 @@ def start_worker_reactor( config: config object run_command: callable that actually runs the reactor """ + logger = logging.getLogger(config.worker.worker_app) start_reactor( diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index e5ffd53b950..2a824e8457f 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -368,8 +368,6 @@ def setup(config_options: List[str]) -> SynapseHomeServer: synapse.config.logger.setup_logging(hs, config, use_worker_options=False) logger.info("Setting up server") - logger.info("asdf config=%s", config) - logger.info("asdf send_federation=%s", config.worker.send_federation) try: hs.setup() diff --git a/synapse/config/workers.py b/synapse/config/workers.py index 8c0acc10744..ab896be3077 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -219,7 +219,6 @@ class WorkerConfig(Config): section = "worker" def read_config(self, config: JsonDict, **kwargs: Any) -> None: - logger.info("asdf WorkerConfig.read_config: %s", config) self.worker_app = config.get("worker_app") # Canonicalise worker_app so that master always has None @@ -275,12 +274,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "federation_sender_instances", ) self.send_federation = self.instance_name in federation_sender_instances - logger.info( - "asdf self.send_federation: %s %s %s", - self.send_federation, - self.instance_name, - federation_sender_instances, - ) self.federation_shard_config = ShardedWorkerHandlingConfig( federation_sender_instances ) @@ -581,6 +574,7 @@ def _worker_names_performing_this_duty( Returns: A list of worker instance names handling the given duty. """ + legacy_option = config.get(legacy_option_name, True) worker_instances = config.get(modern_instance_list_name) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index a8acf550bd9..ec34e282d65 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1272,10 +1272,7 @@ async def _process_incoming_pdus_in_room_inner( origin, event ) logger.info( - "✅ handled received PDU in room %s: %s stream_ordering=%s", - room_id, - event, - event.internal_metadata.stream_ordering, + "✅ handled received PDU in room %s: %s", room_id, event ) except FederationError as e: # XXX: Ideally we'd inform the remote we failed to process diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 24778fd3f16..350c3fa09a1 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -583,12 +583,6 @@ async def _wait_for_sync_for_user( async def current_sync_callback( before_token: StreamToken, after_token: StreamToken ) -> Union[SyncResult, E2eeSyncResult]: - logger.info( - "asdf notifier saw new event before_token=%s after_token=%s, syncing with since_token=%s", - before_token.room_key.stream, - after_token.room_key.stream, - since_token.room_key.stream if since_token else None, - ) return await self.current_sync_for_user( sync_config, sync_version, since_token ) @@ -2392,12 +2386,6 @@ async def _generate_sync_entry_for_rooms( since_token = sync_result_builder.since_token user_id = sync_result_builder.sync_config.user.to_string() - logger.info( - "asdf _generate_sync_entry_for_rooms: since_token=%s now_token=%s", - since_token.room_key.stream if since_token else None, - sync_result_builder.now_token.room_key.stream, - ) - blocks_all_rooms = ( sync_result_builder.sync_config.filter_collection.blocks_all_rooms() ) @@ -2465,11 +2453,6 @@ async def _generate_sync_entry_for_rooms( log_kv({"rooms_changed": len(room_changes.room_entries)}) - logger.info( - "asdf _generate_sync_entry_for_rooms rooms_changed: rooms_changed=%s", - room_changes.room_entries, - ) - room_entries = room_changes.room_entries invited = room_changes.invited knocked = room_changes.knocked @@ -2555,12 +2538,6 @@ async def _get_room_changes_for_incremental_sync( assert since_token - logger.info( - "asdf _get_room_changes_for_incremental_sync since_token=%s, now_token=%s", - since_token.room_key.stream if since_token else None, - now_token.room_key.stream, - ) - mem_change_events_by_room_id: Dict[str, List[EventBase]] = {} for event in membership_change_events: mem_change_events_by_room_id.setdefault(event.room_id, []).append(event) @@ -2733,7 +2710,6 @@ async def _get_room_changes_for_incremental_sync( limit=timeline_limit + 1, direction=Direction.BACKWARDS, ) - logger.info("asdf room_to_events results: %s", room_to_events) # We loop through all room ids, even if there are no new events, in case # there are non room events that we need to notify about. @@ -2910,18 +2886,6 @@ async def _generate_room_entry( ) events = room_builder.events - logger.info( - "asdf _generate_room_entry since_token=%s upto_token=%s end_token=%s always_include=%s full_state=%s events=%s", - room_builder.since_token.room_key.stream - if room_builder.since_token - else None, - room_builder.upto_token.room_key.stream, - room_builder.end_token.room_key.stream, - always_include, - full_state, - events, - ) - # We want to shortcut out as early as possible. if not (always_include or account_data or ephemeral or full_state): if events == [] and tags is None: diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index 1f43612872d..222df8757ac 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -798,12 +798,6 @@ async def get_unredacted_events_from_cache_or_db( missing_events_ids = {e for e in event_ids if e not in event_entry_map} - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (1): event_entry_map=%s missing_events_ids=%s", - event_entry_map, - missing_events_ids, - ) - # We now look up if we're already fetching some of the events in the DB, # if so we wait for those lookups to finish instead of pulling the same # events out of the DB multiple times. @@ -867,11 +861,6 @@ async def get_missing_events_from_cache_or_db() -> ( db_missing_events = await self._get_events_from_db( missing_events_ids - missing_events.keys(), ) - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (1): tried to fetch %s but found db_missing_events=%s", - missing_events_ids - missing_events.keys(), - db_missing_events, - ) missing_events.update(db_missing_events) except Exception as e: with PreserveLoggingContext(): @@ -895,11 +884,6 @@ async def get_missing_events_from_cache_or_db() -> ( ) event_entry_map.update(missing_events) - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (2): event_entry_map=%s", - event_entry_map, - ) - if already_fetching_deferreds: # Wait for the other event requests to finish and add their results # to ours. @@ -920,36 +904,13 @@ async def get_missing_events_from_cache_or_db() -> ( if event_id in already_fetching_ids ) - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (3): event_entry_map=%s", - event_entry_map, - ) - - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (4): event_entry_map=%s", - event_entry_map, - ) - if not allow_rejected: - for event_id, entry in event_entry_map.items(): - if entry.event.rejected_reason: - logger.info( - "asdf get_unredacted_events_from_cache_or_db saw rejected_reason for event_id=%s rejected_reason=%s", - event_id, - entry.event.rejected_reason, - ) - event_entry_map = { event_id: entry for event_id, entry in event_entry_map.items() if not entry.event.rejected_reason } - logger.info( - "asdf get_unredacted_events_from_cache_or_db at (5): event_entry_map=%s", - event_entry_map, - ) - return event_entry_map def invalidate_get_event_cache_after_txn( diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index dde11ec8a54..b4258a44362 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -700,8 +700,6 @@ async def get_room_events_stream_for_rooms( When Direction.FORWARDS: from_key < x <= to_key, (ascending order) When Direction.BACKWARDS: from_key >= x > to_key, (descending order) """ - before_room_ids_asdf = room_ids - if direction == Direction.FORWARDS: room_ids = self._events_stream_cache.get_entities_changed( room_ids, from_key.stream @@ -714,16 +712,6 @@ async def get_room_events_stream_for_rooms( else: assert_never(direction) - logger.info( - "asdf get_room_events_stream_for_rooms from_key=%s, to_key=%s limit=%s direction=%s before_room_ids=%s after_room_ids=%s", - from_key.stream, - to_key.stream if to_key else None, - limit, - direction, - before_room_ids_asdf, - room_ids, - ) - if not room_ids: return {} @@ -880,14 +868,6 @@ async def paginate_room_events_by_stream_ordering( else: assert_never(direction) - logger.info( - "asdf paginate_room_events_by_stream_ordering has_changed=%s from_key=%s, to_key=%s direction=%s", - has_changed, - from_key.stream, - to_key.stream if to_key else None, - direction, - ) - if not has_changed: # Token selection matches what we do below if there are no rows return [], to_key if to_key else from_key, False @@ -922,12 +902,6 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: fetched_rows = txn.fetchall() limited = len(fetched_rows) >= 2 * limit - logger.info( - "asdf paginate_room_events_by_stream_ordering fetched_rows=%s sql=%s", - fetched_rows, - sql, - ) - rows = [ _EventDictReturn(event_id, None, stream_ordering) for event_id, instance_name, stream_ordering in fetched_rows @@ -943,10 +917,6 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: ) ] - logger.info( - "asdf paginate_room_events_by_stream_ordering filtered_rows=%s", rows - ) - if len(rows) > limit: limited = True @@ -961,8 +931,6 @@ def f(txn: LoggingTransaction) -> Tuple[List[_EventDictReturn], bool]: [r.event_id for r in rows], get_prev_content=True ) - logger.info("asdf paginate_room_events_by_stream_ordering ret=%s", ret) - if rows: next_key = generate_next_token( direction=direction, From 872f717a6ab0a8e9ab598e1c1c853af88c39a11c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 18:23:22 -0600 Subject: [PATCH 32/45] Remove debug logs --- scripts-dev/complement.sh | 16 ++++++++-------- synapse/event_auth.py | 2 +- synapse/federation/federation_server.py | 4 ---- synapse/handlers/federation.py | 5 ----- synapse/handlers/federation_event.py | 8 -------- synapse/handlers/message.py | 6 +----- synapse/notifier.py | 1 - synapse/storage/databases/main/events.py | 6 ------ 8 files changed, 10 insertions(+), 38 deletions(-) diff --git a/scripts-dev/complement.sh b/scripts-dev/complement.sh index 3643dd063ad..6be9177f110 100755 --- a/scripts-dev/complement.sh +++ b/scripts-dev/complement.sh @@ -221,14 +221,14 @@ extra_test_args=() test_packages=( ./tests/csapi ./tests - # ./tests/msc3874 - # ./tests/msc3890 - # ./tests/msc3391 - # ./tests/msc3757 - # ./tests/msc3930 - # ./tests/msc3902 - # ./tests/msc3967 - # ./tests/msc4140 + ./tests/msc3874 + ./tests/msc3890 + ./tests/msc3391 + ./tests/msc3757 + ./tests/msc3930 + ./tests/msc3902 + ./tests/msc3967 + ./tests/msc4140 ) # Enable dirty runs, so tests will reuse the same container where possible. diff --git a/synapse/event_auth.py b/synapse/event_auth.py index ea3b6a0e2e6..3fe344ac937 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -563,7 +563,7 @@ def _is_membership_change_allowed( invite_level = get_named_level(auth_events, "invite", 0) ban_level = get_named_level(auth_events, "ban", 50) - logger.info( + logger.debug( "_is_membership_change_allowed: %s", { "caller_membership": caller.membership if caller else None, diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index ec34e282d65..92e7a6ee837 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1256,7 +1256,6 @@ async def _process_incoming_pdus_in_room_inner( # has started processing). while True: async with lock: - logger.info("📬 handling received PDU in room %s: %s", room_id, event) try: with nested_logging_context(event.event_id): # We're taking out a lock within a lock, which could @@ -1271,9 +1270,6 @@ async def _process_incoming_pdus_in_room_inner( await self._federation_event_handler.on_receive_pdu( origin, event ) - logger.info( - "✅ handled received PDU in room %s: %s", room_id, event - ) except FederationError as e: # XXX: Ideally we'd inform the remote we failed to process # the event, but we can't return an error in the transaction diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 3a22b2fe216..17dd4af13ed 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -606,8 +606,6 @@ async def do_invite_join( content: The event content to use for the join event. """ - logger.info("🧲 do_invite_join for %s in %s", joinee, room_id) - # TODO: We should be able to call this on workers, but the upgrading of # room stuff after join currently doesn't work on workers. # TODO: Before we relax this condition, we need to allow re-syncing of @@ -1053,8 +1051,6 @@ async def on_invite_request( Respond with the now signed event. """ - logger.info("🗳️ on_invite_request: handling event %s", event) - if event.state_key is None: raise SynapseError(400, "The invite event did not have a state key") @@ -1131,7 +1127,6 @@ async def on_invite_request( await self.store.remove_push_actions_from_staging(event.event_id) raise - logger.info("✅ on_invite_request: handled event %s", event) return event async def do_remotely_reject_invite( diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 818e91a6bbc..c86fd46aca6 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -382,10 +382,6 @@ async def on_send_membership_event( event.event_id, event.signatures, ) - # logger.info( - # "📮 on_send_membership_event: received event: %s", - # event, - # ) if get_domain_from_id(event.sender) != origin: logger.info( @@ -440,10 +436,6 @@ async def on_send_membership_event( await self._check_for_soft_fail(event, context=context, origin=origin) await self._run_push_actions_and_persist_event(event, context) - # logger.info( - # "✅ on_send_membership_event: handled event: %s", - # event, - # ) return event, context async def check_join_restrictions( diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 3ddef80f411..d0da4801833 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1415,8 +1415,6 @@ async def handle_new_client_event( PartialStateConflictError if attempting to persist a partial state event in a room that has been un-partial stated. """ - for event, _ in events_and_context: - logger.info("📮 handle_new_client_event: handling %s", event) extra_users = extra_users or [] @@ -1463,7 +1461,7 @@ async def handle_new_client_event( event, batched_auth_events ) except AuthError as err: - logger.warning("❌ Denying new event %r because %s", event, err) + logger.warning("Denying new event %r because %s", event, err) raise err # Ensure that we can round trip before trying to persist in db @@ -1495,8 +1493,6 @@ async def handle_new_client_event( gather_results(deferreds, consumeErrors=True) ).addErrback(unwrapFirstError) - for event, _ in events_and_context: - logger.info("✅ handle_new_client_event: handled %s", event) return result async def _persist_events( diff --git a/synapse/notifier.py b/synapse/notifier.py index b129bf49528..88f531182a0 100644 --- a/synapse/notifier.py +++ b/synapse/notifier.py @@ -308,7 +308,6 @@ async def on_new_room_events( notify_new_room_events with the results.""" event_entries = [] for event, pos in events_and_pos: - logger.info("📨 Notifying about new event %s", event, exc_info=True) entry = self.create_pending_room_event_entry( pos, extra_users, diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 11a7de1cc12..a23aaf50962 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2836,8 +2836,6 @@ def _store_room_members_txn( for backfilled events because backfilled events in the past do not affect the current local state. """ - for event in events: - logger.info("🔦 _store_room_members_txn update room_memberships: %s", event) self.db_pool.simple_insert_many_txn( txn, @@ -2894,10 +2892,6 @@ def _store_room_members_txn( Membership.LEAVE, ) - logger.info( - "🔦 _store_room_members_txn update local_current_membership: %s", - event, - ) self.db_pool.simple_upsert_txn( txn, table="local_current_membership", From d85d84c49b7e6216708ba960cb081873dc12b27f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 18:28:09 -0600 Subject: [PATCH 33/45] Clean up whitespace --- synapse/handlers/message.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index d0da4801833..df3010ecf68 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1415,7 +1415,6 @@ async def handle_new_client_event( PartialStateConflictError if attempting to persist a partial state event in a room that has been un-partial stated. """ - extra_users = extra_users or [] for event, context in events_and_context: From 14dc54bc25b1975a4a4b13718c6a54b8b7662bb7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 18:37:02 -0600 Subject: [PATCH 34/45] Unable to reproduce to with knocks But seems plausible --- tests/federation/test_federation_out_of_band_membership.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 9b0d46480cb..7d1bea87589 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -104,7 +104,6 @@ class OutOfBandMembershipTests(unittest.FederatingHomeserverTestCase): - invites received over federation, before we join the room - *rejections* for said invites - - knock events for rooms that we would like to join but have not yet joined. See the "Out-of-band membership events" section in `docs/development/room-dag-concepts.md` for more information. From 139db200afbb3860eb2bc5b7257fc987dc2475b3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 18:42:06 -0600 Subject: [PATCH 35/45] Bump db calls --- tests/rest/client/test_rooms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 4cf1a3dc519..833bd6fff8c 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -742,7 +742,7 @@ def test_post_room_no_keys(self) -> None: self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.assertTrue("room_id" in channel.json_body) assert channel.resource_usage is not None - self.assertEqual(33, channel.resource_usage.db_txn_count) + self.assertEqual(34, channel.resource_usage.db_txn_count) def test_post_room_initial_state(self) -> None: # POST with initial_state config key, expect new room id @@ -755,7 +755,7 @@ def test_post_room_initial_state(self) -> None: self.assertEqual(HTTPStatus.OK, channel.code, channel.result) self.assertTrue("room_id" in channel.json_body) assert channel.resource_usage is not None - self.assertEqual(35, channel.resource_usage.db_txn_count) + self.assertEqual(36, channel.resource_usage.db_txn_count) def test_post_room_visibility_key(self) -> None: # POST with visibility config key, expect new room id From 78bee3de3a084faf9f9a6a99f807d22e5bf272c3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 19:09:02 -0600 Subject: [PATCH 36/45] Fix presence tests --- tests/handlers/test_presence.py | 81 +++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 598d6c13cdb..544e73d45ba 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -23,14 +23,19 @@ from unittest.mock import Mock, call from parameterized import parameterized -from signedjson.key import generate_signing_key +from signedjson.key import ( + encode_verify_key_base64, + generate_signing_key, + get_verify_key, +) from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventTypes, Membership, PresenceState from synapse.api.presence import UserDevicePresenceState, UserPresenceState from synapse.api.room_versions import KNOWN_ROOM_VERSIONS -from synapse.events.builder import EventBuilder +from synapse.crypto.event_signing import add_hashes_and_signatures +from synapse.events import make_event_from_dict from synapse.federation.sender import FederationSender from synapse.handlers.presence import ( BUSY_ONLINE_TIMEOUT, @@ -48,6 +53,7 @@ from synapse.rest.client import room from synapse.server import HomeServer from synapse.storage.database import LoggingDatabaseConnection +from synapse.storage.keys import FetchKeyResult from synapse.types import JsonDict, UserID, get_domain_from_id from synapse.util import Clock @@ -1825,6 +1831,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: # self.event_builder_for_2.hostname = "test2" self.store = hs.get_datastores().main + self.storage_controllers = hs.get_storage_controllers() self.state = hs.get_state_handler() self._event_auth_handler = hs.get_event_auth_handler() @@ -1942,27 +1949,63 @@ def _add_new_user(self, room_id: str, user_id: str) -> None: room_version = self.get_success(self.store.get_room_version_id(room_id)) - builder = EventBuilder( - state=self.state, - event_auth_handler=self._event_auth_handler, - store=self.store, - clock=self.clock, - hostname=hostname, - signing_key=self.random_signing_key, - room_version=KNOWN_ROOM_VERSIONS[room_version], - room_id=room_id, - type=EventTypes.Member, - sender=user_id, - state_key=user_id, - content={"membership": Membership.JOIN}, + # poke the other server's signing key into the key store, so that we don't + # make requests for it + other_server_signature_key = generate_signing_key("test") + verify_key = get_verify_key(other_server_signature_key) + verify_key_id = "%s:%s" % (verify_key.alg, verify_key.version) + + self.get_success( + self.hs.get_datastores().main.store_server_keys_response( + hostname, + from_server=hostname, + ts_added_ms=self.clock.time_msec(), + verify_keys={ + verify_key_id: FetchKeyResult( + verify_key=verify_key, + valid_until_ts=self.clock.time_msec() + 10000, + ), + }, + response_json={ + "verify_keys": { + verify_key_id: {"key": encode_verify_key_base64(verify_key)} + } + }, + ) ) - prev_event_ids = self.get_success( - self.store.get_latest_event_ids_in_room(room_id) + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) ) - event = self.get_success( - builder.build(prev_event_ids=list(prev_event_ids), auth_event_ids=None) + # Figure out what the forward extremities in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(room_id) + ) + event_dict = { + "room_id": room_id, + "sender": user_id, + "state_key": user_id, + "depth": 1000, + "origin_server_ts": 1, + "type": EventTypes.Member, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + state_map[(EventTypes.Create, "")].event_id, + state_map[(EventTypes.JoinRules, "")].event_id, + ], + "prev_events": list(forward_extremity_event_ids), + } + add_hashes_and_signatures( + room_version=KNOWN_ROOM_VERSIONS[room_version], + event_dict=event_dict, + signature_name=hostname, + signing_key=other_server_signature_key, + ) + event = make_event_from_dict( + event_dict, + room_version=KNOWN_ROOM_VERSIONS[room_version], ) self.get_success(self.federation_event_handler.on_receive_pdu(hostname, event)) From 074483ddd891c3fdb606ad95213fc96cacc007e0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 19:15:37 -0600 Subject: [PATCH 37/45] Fix sync join/ban race test failing We're now better at rejecting this --- tests/handlers/test_sync.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 9dd0e98971b..0cbc9a8a21d 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -19,6 +19,7 @@ # from typing import Collection, ContextManager, List, Optional from unittest.mock import AsyncMock, Mock, patch +from http import HTTPStatus from parameterized import parameterized, parameterized_class @@ -347,7 +348,15 @@ def test_ban_wins_race_with_join(self) -> None: # the prev_events used when creating the join event, such that the ban does not # precede the join. with self._patch_get_latest_events([last_room_creation_event_id]): - self.helper.join(room_id, eve, tok=eve_token) + self.helper.join( + room_id, + eve, + tok=eve_token, + # Previously, this join would succeed but now we expect it to fail at + # this point. The rest of the test is for the case when this used to + # succeed. + expect_code=HTTPStatus.FORBIDDEN, + ) # Eve makes a second, incremental sync. eve_incremental_sync_after_join: SyncResult = self.get_success( From 545f22ddf342fbc6f5e1dffc8975fcdf48fec98c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 19:20:13 -0600 Subject: [PATCH 38/45] Fix lints --- tests/handlers/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 0cbc9a8a21d..6b202dfbd53 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -17,9 +17,9 @@ # [This file includes modifications made by New Vector Limited] # # +from http import HTTPStatus from typing import Collection, ContextManager, List, Optional from unittest.mock import AsyncMock, Mock, patch -from http import HTTPStatus from parameterized import parameterized, parameterized_class From 0b3110032071e547b2ddda016469858e909cd150 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 17 Jan 2025 20:07:11 -0600 Subject: [PATCH 39/45] Fix federation sender shard tests --- tests/handlers/test_presence.py | 91 +++++++++------ .../test_federation_sender_shard.py | 110 +++++++++++++++--- 2 files changed, 146 insertions(+), 55 deletions(-) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 544e73d45ba..aea2e17480c 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -33,9 +33,11 @@ from synapse.api.constants import EventTypes, Membership, PresenceState from synapse.api.presence import UserDevicePresenceState, UserPresenceState -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS +from synapse.api.room_versions import ( + RoomVersion, +) from synapse.crypto.event_signing import add_hashes_and_signatures -from synapse.events import make_event_from_dict +from synapse.events import EventBase, make_event_from_dict from synapse.federation.sender import FederationSender from synapse.handlers.presence import ( BUSY_ONLINE_TIMEOUT, @@ -1947,7 +1949,51 @@ def _add_new_user(self, room_id: str, user_id: str) -> None: hostname = get_domain_from_id(user_id) - room_version = self.get_success(self.store.get_room_version_id(room_id)) + room_version = self.get_success(self.store.get_room_version(room_id)) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + + # Figure out what the forward extremities in the room are (the most recent + # events that aren't tied into the DAG) + forward_extremity_event_ids = self.get_success( + self.hs.get_datastores().main.get_latest_event_ids_in_room(room_id) + ) + + event = self.create_fake_event_from_remote_server( + remote_server_name=hostname, + event_dict={ + "room_id": room_id, + "sender": user_id, + "type": EventTypes.Member, + "state_key": user_id, + "depth": 1000, + "origin_server_ts": 1, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + state_map[(EventTypes.Create, "")].event_id, + state_map[(EventTypes.JoinRules, "")].event_id, + ], + "prev_events": list(forward_extremity_event_ids), + }, + room_version=room_version, + ) + + self.get_success(self.federation_event_handler.on_receive_pdu(hostname, event)) + + # Check that it was successfully persisted. + self.get_success(self.store.get_event(event.event_id)) + self.get_success(self.store.get_event(event.event_id)) + + def create_fake_event_from_remote_server( + self, remote_server_name: str, event_dict: JsonDict, room_version: RoomVersion + ) -> EventBase: + """ + This is similar to what `FederatingHomeserverTestCase` is doing but we don't + need all of the extra baggage and we want to be able to create an event from + many remote servers. + """ # poke the other server's signing key into the key store, so that we don't # make requests for it @@ -1957,8 +2003,8 @@ def _add_new_user(self, room_id: str, user_id: str) -> None: self.get_success( self.hs.get_datastores().main.store_server_keys_response( - hostname, - from_server=hostname, + remote_server_name, + from_server=remote_server_name, ts_added_ms=self.clock.time_msec(), verify_keys={ verify_key_id: FetchKeyResult( @@ -1974,42 +2020,15 @@ def _add_new_user(self, room_id: str, user_id: str) -> None: ) ) - state_map = self.get_success( - self.storage_controllers.state.get_current_state(room_id) - ) - - # Figure out what the forward extremities in the room are (the most recent - # events that aren't tied into the DAG) - forward_extremity_event_ids = self.get_success( - self.hs.get_datastores().main.get_latest_event_ids_in_room(room_id) - ) - event_dict = { - "room_id": room_id, - "sender": user_id, - "state_key": user_id, - "depth": 1000, - "origin_server_ts": 1, - "type": EventTypes.Member, - "content": {"membership": Membership.JOIN}, - "auth_events": [ - state_map[(EventTypes.Create, "")].event_id, - state_map[(EventTypes.JoinRules, "")].event_id, - ], - "prev_events": list(forward_extremity_event_ids), - } add_hashes_and_signatures( - room_version=KNOWN_ROOM_VERSIONS[room_version], + room_version=room_version, event_dict=event_dict, - signature_name=hostname, + signature_name=remote_server_name, signing_key=other_server_signature_key, ) event = make_event_from_dict( event_dict, - room_version=KNOWN_ROOM_VERSIONS[room_version], + room_version=room_version, ) - self.get_success(self.federation_event_handler.on_receive_pdu(hostname, event)) - - # Check that it was successfully persisted. - self.get_success(self.store.get_event(event.event_id)) - self.get_success(self.store.get_event(event.event_id)) + return event diff --git a/tests/replication/test_federation_sender_shard.py b/tests/replication/test_federation_sender_shard.py index 4429d0f4e29..58a7a9dc721 100644 --- a/tests/replication/test_federation_sender_shard.py +++ b/tests/replication/test_federation_sender_shard.py @@ -22,14 +22,26 @@ from unittest.mock import AsyncMock, Mock from netaddr import IPSet +from signedjson.key import ( + encode_verify_key_base64, + generate_signing_key, + get_verify_key, +) + +from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventTypes, Membership -from synapse.events.builder import EventBuilderFactory +from synapse.api.room_versions import RoomVersion +from synapse.crypto.event_signing import add_hashes_and_signatures +from synapse.events import EventBase, make_event_from_dict from synapse.handlers.typing import TypingWriterHandler from synapse.http.federation.matrix_federation_agent import MatrixFederationAgent from synapse.rest.admin import register_servlets_for_client_rest_resource from synapse.rest.client import login, room -from synapse.types import UserID, create_requester +from synapse.server import HomeServer +from synapse.storage.keys import FetchKeyResult +from synapse.types import JsonDict, UserID, create_requester +from synapse.util import Clock from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.server import get_clock @@ -63,6 +75,9 @@ def setUp(self) -> None: ip_blocklist=IPSet(), ) + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.storage_controllers = hs.get_storage_controllers() + def test_send_event_single_sender(self) -> None: """Test that using a single federation sender worker correctly sends a new event. @@ -243,35 +258,92 @@ def test_send_typing_sharded(self) -> None: self.assertTrue(sent_on_1) self.assertTrue(sent_on_2) + def create_fake_event_from_remote_server( + self, remote_server_name: str, event_dict: JsonDict, room_version: RoomVersion + ) -> EventBase: + """ + This is similar to what `FederatingHomeserverTestCase` is doing but we don't + need all of the extra baggage and we want to be able to create an event from + many remote servers. + """ + + # poke the other server's signing key into the key store, so that we don't + # make requests for it + other_server_signature_key = generate_signing_key("test") + verify_key = get_verify_key(other_server_signature_key) + verify_key_id = "%s:%s" % (verify_key.alg, verify_key.version) + + self.get_success( + self.hs.get_datastores().main.store_server_keys_response( + remote_server_name, + from_server=remote_server_name, + ts_added_ms=self.clock.time_msec(), + verify_keys={ + verify_key_id: FetchKeyResult( + verify_key=verify_key, + valid_until_ts=self.clock.time_msec() + 10000, + ), + }, + response_json={ + "verify_keys": { + verify_key_id: {"key": encode_verify_key_base64(verify_key)} + } + }, + ) + ) + + add_hashes_and_signatures( + room_version=room_version, + event_dict=event_dict, + signature_name=remote_server_name, + signing_key=other_server_signature_key, + ) + event = make_event_from_dict( + event_dict, + room_version=room_version, + ) + + return event + def create_room_with_remote_server( self, user: str, token: str, remote_server: str = "other_server" ) -> str: - room = self.helper.create_room_as(user, tok=token) + room_id = self.helper.create_room_as(user, tok=token) store = self.hs.get_datastores().main federation = self.hs.get_federation_event_handler() - prev_event_ids = self.get_success(store.get_latest_event_ids_in_room(room)) - room_version = self.get_success(store.get_room_version(room)) + room_version = self.get_success(store.get_room_version(room_id)) - factory = EventBuilderFactory(self.hs) - factory.hostname = remote_server + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id) + ) + + # Figure out what the forward extremities in the room are (the most recent + # events that aren't tied into the DAG) + prev_event_ids = self.get_success(store.get_latest_event_ids_in_room(room_id)) user_id = UserID("user", remote_server).to_string() - event_dict = { - "type": EventTypes.Member, - "state_key": user_id, - "content": {"membership": Membership.JOIN}, - "sender": user_id, - "room_id": room, - } - - builder = factory.for_room_version(room_version, event_dict) - join_event = self.get_success( - builder.build(prev_event_ids=list(prev_event_ids), auth_event_ids=None) + join_event = self.create_fake_event_from_remote_server( + remote_server_name=remote_server, + event_dict={ + "room_id": room_id, + "sender": user_id, + "type": EventTypes.Member, + "state_key": user_id, + "depth": 1000, + "origin_server_ts": 1, + "content": {"membership": Membership.JOIN}, + "auth_events": [ + state_map[(EventTypes.Create, "")].event_id, + state_map[(EventTypes.JoinRules, "")].event_id, + ], + "prev_events": list(prev_event_ids), + }, + room_version=room_version, ) self.get_success(federation.on_send_membership_event(remote_server, join_event)) self.replicate() - return room + return room_id From d80984e394cf6c682c5a30a3a58686a407e09d51 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Jan 2025 15:28:06 -0600 Subject: [PATCH 40/45] Better `user_id` parameter for `is_mine_id` See https://github.com/element-hq/synapse/pull/18075#discussion_r1928905966 --- synapse/events/builder.py | 4 ++-- synapse/server.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/events/builder.py b/synapse/events/builder.py index 828e3dbc617..328e5ad3524 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -109,7 +109,7 @@ def state_key(self) -> str: def is_state(self) -> bool: return self._state_key is not None - def is_mine_id(self, string: str) -> bool: + def is_mine_id(self, user_id: str) -> bool: """Determines whether a user ID or room alias originates from this homeserver. Returns: @@ -117,7 +117,7 @@ def is_mine_id(self, string: str) -> bool: homeserver. `False` otherwise, or if the user ID or room alias is malformed. """ - localpart_hostname = string.split(":", 1) + localpart_hostname = user_id.split(":", 1) if len(localpart_hostname) < 2: return False return localpart_hostname[1] == self._hostname diff --git a/synapse/server.py b/synapse/server.py index 462e15cc2ff..bd2faa61b94 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -391,7 +391,7 @@ def get_reactor(self) -> ISynapseReactor: def is_mine(self, domain_specific_string: DomainSpecificString) -> bool: return domain_specific_string.domain == self.hostname - def is_mine_id(self, string: str) -> bool: + def is_mine_id(self, user_id: str) -> bool: """Determines whether a user ID or room alias originates from this homeserver. Returns: @@ -399,7 +399,7 @@ def is_mine_id(self, string: str) -> bool: homeserver. `False` otherwise, or if the user ID or room alias is malformed. """ - localpart_hostname = string.split(":", 1) + localpart_hostname = user_id.split(":", 1) if len(localpart_hostname) < 2: return False return localpart_hostname[1] == self.hostname From ab098d9f5c5b3b921a85ab53dd517195b0afaef2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Jan 2025 15:30:25 -0600 Subject: [PATCH 41/45] Fix some typos See https://github.com/element-hq/synapse/pull/18075#discussion_r1928969236 https://github.com/element-hq/synapse/pull/18075#discussion_r1929180266 --- synapse/events/builder.py | 6 +++--- tests/federation/test_federation_out_of_band_membership.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/events/builder.py b/synapse/events/builder.py index 328e5ad3524..76df083d691 100644 --- a/synapse/events/builder.py +++ b/synapse/events/builder.py @@ -155,9 +155,9 @@ async def build( self, state_ids ) - # Check for out-of-band membership that may that may have been exposed on - # `/sync` but the events have not been de-outliered yet so they won't be - # part of the room state yet. + # Check for out-of-band membership that may have been exposed on `/sync` but + # the events have not been de-outliered yet so they won't be part of the + # room state yet. # # This helps in situations where a remote homeserver invites a local user to # a room that we're already participating in; and we've persisted the invite diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 7d1bea87589..1e0454d259e 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -484,7 +484,7 @@ def test_can_x_from_out_of_band_invite_after_we_are_already_participating_in_the we are already participating in the room, local users can still react to invites regardless of whether the remote server has told us about the invite event (via a federation `/send` transaction) and we have de-outliered the invite event. - Previously, we would mistakenly throw and error saying the user wasn't in the + Previously, we would mistakenly throw an error saying the user wasn't in the room when they tried to join or reject the invite. """ remote_room_join_result = self._invite_local_user_to_remote_room_and_join() From 2d5d246ccb9a24f688a64ed13f87cb9715173544 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Jan 2025 15:33:57 -0600 Subject: [PATCH 42/45] Assert instead of nest See https://github.com/element-hq/synapse/pull/18075#discussion_r1929004774 --- .../test_federation_out_of_band_membership.py | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 1e0454d259e..66c5a04ce7e 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -64,22 +64,25 @@ def required_state_json_to_state_map(required_state: Any) -> StateMap[EventBase] # Scrutinize JSON values to ensure it's in the expected format if isinstance(required_state, list): for state_event_dict in required_state: - if isinstance(state_event_dict, dict): - event_type = state_event_dict["type"] - event_state_key = state_event_dict["state_key"] - - if isinstance(event_type, str) and isinstance(event_state_key, str): - state_map[(event_type, event_state_key)] = make_event_from_dict( - state_event_dict - ) - else: - # Yell because we're in a test and this is unexpected - raise AssertionError( - "Each event in `required_state` should have a `type` and `state_key` as strings" - ) - else: - # Yell because we're in a test and this is unexpected - raise AssertionError("`required_state` should be a list of event dicts") + # Yell because we're in a test and this is unexpected + assert isinstance( + state_event_dict, dict + ), "`required_state` should be a list of event dicts" + + event_type = state_event_dict["type"] + event_state_key = state_event_dict["state_key"] + + # Yell because we're in a test and this is unexpected + assert isinstance( + event_type, str + ), "Each event in `required_state` should have a string `type`" + assert isinstance( + event_state_key, str + ), "Each event in `required_state` should have a string `state_key`" + + state_map[(event_type, event_state_key)] = make_event_from_dict( + state_event_dict + ) else: # Yell because we're in a test and this is unexpected raise AssertionError("`required_state` should be a list of event dicts") From dc547d621932a58718119d435959b87298b88947 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Jan 2025 15:34:54 -0600 Subject: [PATCH 43/45] Restore log See https://github.com/element-hq/synapse/pull/18075#discussion_r1928974961 --- synapse/federation/federation_server.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 92e7a6ee837..1932fa82a4a 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1256,6 +1256,7 @@ async def _process_incoming_pdus_in_room_inner( # has started processing). while True: async with lock: + logger.info("handling received PDU in room %s: %s", room_id, event) try: with nested_logging_context(event.event_id): # We're taking out a lock within a lock, which could From e9ee86a877b25edfcd55b73521beed82f8d1183d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 24 Jan 2025 15:42:41 -0600 Subject: [PATCH 44/45] Explain why not using `spec=MatrixFederationHttpClient` See https://github.com/element-hq/synapse/pull/18075#discussion_r1929143851 --- tests/federation/test_federation_out_of_band_membership.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/federation/test_federation_out_of_band_membership.py b/tests/federation/test_federation_out_of_band_membership.py index 66c5a04ce7e..a4a266cf06d 100644 --- a/tests/federation/test_federation_out_of_band_membership.py +++ b/tests/federation/test_federation_out_of_band_membership.py @@ -131,7 +131,9 @@ def default_config(self) -> JsonDict: def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: self.federation_http_client = Mock( - # spec=MatrixFederationHttpClient + # The problem with using `spec=MatrixFederationHttpClient` here is that it + # requires everything to be mocked which is a lot of work that I don't want + # to do when the code only uses a few methods (`get_json` and `put_json`). ) return self.setup_test_homeserver( federation_http_client=self.federation_http_client From 42df9e61661c568c34605e776aaff0ce39922b31 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 27 Jan 2025 10:48:50 -0600 Subject: [PATCH 45/45] Expand comment but still no concrete answer See https://github.com/element-hq/synapse/pull/18075#discussion_r1920895848 --- synapse/handlers/federation_event.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index c86fd46aca6..1b535ea2cb7 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -2272,8 +2272,9 @@ async def persist_events_and_notify( event_and_contexts, backfilled=backfilled ) - # After persistence, we always need to notify replication there may be new - # data (backfilled or not) because TODO. + # After persistence, we never notify clients (wake up `/sync` streams) about + # backfilled events but it's important to let all the workers know about any + # new event (backfilled or not) because TODO self._notifier.notify_replication() if self._ephemeral_messages_enabled: