Refactor create_new_client_event to use a new parameter, state_event_ids, which accurately describes the usage with MSC2716 instead of abusing auth_event_ids

changelog.d/12083.misc

@@ -0,0 +1 @@
+Refactor `create_new_client_event` to use a new parameter, `state_event_ids`, which accurately describes the usage with [MSC2716](https://github.com/matrix-org/matrix-doc/pull/2716) instead of abusing `auth_event_ids`.

scripts-dev/complement.sh

@@ -71,4 +71,4 @@ fi
 
 # Run the tests!
 echo "Images built; running complement"
-go test -v -tags synapse_blacklist,msc2403 -count=1 $EXTRA_COMPLEMENT_ARGS ./tests/...
+go test -v -tags synapse_blacklist,msc2403,msc2716 -count=1 $EXTRA_COMPLEMENT_ARGS ./tests/...

synapse/handlers/message.py

@@ -493,6 +493,7 @@ async def create_event(
         allow_no_prev_events: bool = False,
         prev_event_ids: Optional[List[str]] = None,
         auth_event_ids: Optional[List[str]] = None,
+        state_event_ids: Optional[List[str]] = None,
         require_consent: bool = True,
         outlier: bool = False,
         historical: bool = False,
@@ -527,6 +528,16 @@ async def create_event(
 
                 If non-None, prev_event_ids must also be provided.
 
+            state_event_ids:
+                The full state at a given event. This is used particularly by the MSC2716
+                /batch_send endpoint which shares the same state across the whole batch.
+                The state events will be stripped down to only what's necessary to auth
+                a given event and set as the auth_event_ids. For insertion events, we will
+                add all of these state events as the explicit state so the rest of the
+                historical batch can inherit the same state and state_group. This should
+                normally be left as None, which will cause the auth_event_ids to be
+                calculated based on the room state at the prev_events.
+
             require_consent: Whether to check if the requester has
                 consented to the privacy policy.
 
@@ -612,6 +623,7 @@ async def create_event(
             allow_no_prev_events=allow_no_prev_events,
             prev_event_ids=prev_event_ids,
             auth_event_ids=auth_event_ids,
+            state_event_ids=state_event_ids,
             depth=depth,
         )
 
@@ -772,6 +784,7 @@ async def create_and_send_nonmember_event(
         allow_no_prev_events: bool = False,
         prev_event_ids: Optional[List[str]] = None,
         auth_event_ids: Optional[List[str]] = None,
+        state_event_ids: Optional[List[str]] = None,
         ratelimit: bool = True,
         txn_id: Optional[str] = None,
         ignore_shadow_ban: bool = False,
@@ -801,6 +814,15 @@ async def create_and_send_nonmember_event(
                 based on the room state at the prev_events.
 
                 If non-None, prev_event_ids must also be provided.
+            state_event_ids:
+                The full state at a given event. This is used particularly by the MSC2716
+                /batch_send endpoint which shares the same state across the whole batch.
+                The state events will be stripped down to only what's necessary to auth
+                a given event and set as the auth_event_ids. For insertion events, we will
+                add all of these state events as the explicit state so the rest of the
+                historical batch can inherit the same state and state_group. This should
+                normally be left as None, which will cause the auth_event_ids to be
+                calculated based on the room state at the prev_events.
             ratelimit: Whether to rate limit this send.
             txn_id: The transaction ID.
             ignore_shadow_ban: True if shadow-banned users should be allowed to
@@ -856,8 +878,10 @@ async def create_and_send_nonmember_event(
                 requester,
                 event_dict,
                 txn_id=txn_id,
+                allow_no_prev_events=allow_no_prev_events,
                 prev_event_ids=prev_event_ids,
                 auth_event_ids=auth_event_ids,
+                state_event_ids=state_event_ids,
                 outlier=outlier,
                 historical=historical,
                 depth=depth,
@@ -893,6 +917,7 @@ async def create_new_client_event(
         allow_no_prev_events: bool = False,
         prev_event_ids: Optional[List[str]] = None,
         auth_event_ids: Optional[List[str]] = None,
+        state_event_ids: Optional[List[str]] = None,
         depth: Optional[int] = None,
     ) -> Tuple[EventBase, EventContext]:
         """Create a new event for a local client
@@ -915,38 +940,43 @@ async def create_new_client_event(
                 Should normally be left as None, which will cause them to be calculated
                 based on the room state at the prev_events.
 
+            state_event_ids:
+                The full state at a given event. This is used particularly by the MSC2716
+                /batch_send endpoint which shares the same state across the whole batch.
+                The state events will be stripped down to only what's necessary to auth
+                a given event and set as the auth_event_ids. For insertion events, we will
+                add all of these state events as the explicit state so the rest of the
+                historical batch can inherit the same state and state_group. This should
+                normally be left as None, which will cause the auth_event_ids to be
+                calculated based on the room state at the prev_events.
+
             depth: Override the depth used to order the event in the DAG.
                 Should normally be set to None, which will cause the depth to be calculated
                 based on the prev_events.
 
         Returns:
             Tuple of created event, context
         """
-        # Strip down the auth_event_ids to only what we need to auth the event.
+        # Strip down the state_event_ids to only what we need to auth the event.
         # For example, we don't need extra m.room.member that don't match event.sender
-        full_state_ids_at_event = None
-        if auth_event_ids is not None:
-            # If auth events are provided, prev events must be also.
+        if state_event_ids is not None:
+            # Do a quick check to make sure that prev_event_ids is present to
+            # make the type-checking around `builder.build` happy.
             # prev_event_ids could be an empty array though.
             assert prev_event_ids is not None
 
-            # Copy the full auth state before it stripped down
-            full_state_ids_at_event = auth_event_ids.copy()
-
             temp_event = await builder.build(
                 prev_event_ids=prev_event_ids,
-                auth_event_ids=auth_event_ids,
+                auth_event_ids=state_event_ids,
                 depth=depth,
             )
-            auth_events = await self.store.get_events_as_list(auth_event_ids)
+            state_events = await self.store.get_events_as_list(state_event_ids)
             # Create a StateMap[str]
-            auth_event_state_map = {
-                (e.type, e.state_key): e.event_id for e in auth_events
-            }
-            # Actually strip down and use the necessary auth events
+            state_map = {(e.type, e.state_key): e.event_id for e in state_events}
+            # Actually strip down and only use the necessary auth events
             auth_event_ids = self._event_auth_handler.compute_auth_events(
                 event=temp_event,
-                current_state_ids=auth_event_state_map,
+                current_state_ids=state_map,
                 for_verification=False,
             )
 
@@ -989,10 +1019,12 @@ async def create_new_client_event(
             context = EventContext.for_outlier()
         elif (
             event.type == EventTypes.MSC2716_INSERTION
-            and full_state_ids_at_event
+            and state_event_ids
             and builder.internal_metadata.is_historical()
         ):
-            old_state = await self.store.get_events_as_list(full_state_ids_at_event)
+            # Add explicit state to the insertion event so the rest of the batch
+            # can inherit the same state and state_group
+            old_state = await self.store.get_events_as_list(state_event_ids)
             context = await self.state.compute_event_context(event, old_state=old_state)
         else:
             context = await self.state.compute_event_context(event)

synapse/handlers/room_batch.py

@@ -121,12 +121,11 @@ async def create_requester_for_user_id_from_app_service(
 
         return create_requester(user_id, app_service=app_service)
 
-    async def get_most_recent_auth_event_ids_from_event_id_list(
+    async def get_most_recent_full_state_ids_from_event_id_list(
         self, event_ids: List[str]
     ) -> List[str]:
-        """Find the most recent auth event ids (derived from state events) that
-        allowed that message to be sent. We will use this as a base
-        to auth our historical messages against.
+        """Find the most recent event_id and grab the full state at that event.
+        We will use this as a base to auth our historical messages against.
 
         Args:
             event_ids: List of event ID's to look at
@@ -136,24 +135,23 @@ async def get_most_recent_auth_event_ids_from_event_id_list(
         """
 
         (
-            most_recent_prev_event_id,
+            most_recent_event_id,
             _,
         ) = await self.store.get_max_depth_of(event_ids)
         # mapping from (type, state_key) -> state_event_id
         prev_state_map = await self.state_store.get_state_ids_for_event(
-            most_recent_prev_event_id
+            most_recent_event_id
         )
         # List of state event ID's
-        prev_state_ids = list(prev_state_map.values())
-        auth_event_ids = prev_state_ids
+        full_state_ids = list(prev_state_map.values())
 
-        return auth_event_ids
+        return full_state_ids
 
     async def persist_state_events_at_start(
         self,
         state_events_at_start: List[JsonDict],
         room_id: str,
-        initial_auth_event_ids: List[str],
+        initial_state_ids_at_event: List[str],
         app_service_requester: Requester,
     ) -> List[str]:
         """Takes all `state_events_at_start` event dictionaries and creates/persists
@@ -164,10 +162,13 @@ async def persist_state_events_at_start(
         Args:
             state_events_at_start:
             room_id: Room where you want the events persisted in.
-            initial_auth_event_ids: These will be the auth_events for the first
-                state event created. Each event created afterwards will be
-                added to the list of auth events for the next state event
-                created.
+            initial_state_ids_at_event:
+                The base set of state of the historical batch. When persisting each of
+                the events in state_events_at_start, the state will be stripped down to
+                only what's necessary to auth the given event and set as the
+                auth_event_ids. After each state event from state_events_at_start is
+                persisted, it will be added to the ongoing list of state_event_ids for
+                the next state event to be persisted with.
             app_service_requester: The requester of an application service.
 
         Returns:
@@ -176,7 +177,7 @@ async def persist_state_events_at_start(
         assert app_service_requester.app_service
 
         state_event_ids_at_start = []
-        auth_event_ids = initial_auth_event_ids.copy()
+        state_event_ids = initial_state_ids_at_event.copy()
 
         # Make the state events float off on their own by specifying no
         # prev_events for the first one in the chain so we don't have a bunch of
@@ -189,9 +190,9 @@ async def persist_state_events_at_start(
             )
 
             logger.debug(
-                "RoomBatchSendEventRestServlet inserting state_event=%s, auth_event_ids=%s",
+                "RoomBatchSendEventRestServlet inserting state_event=%s, state_event_ids=%s",
                 state_event,
-                auth_event_ids,
+                state_event_ids,
             )
 
             event_dict = {
@@ -226,7 +227,7 @@ async def persist_state_events_at_start(
                     # Make sure to use a copy of this list because we modify it
                     # later in the loop here. Otherwise it will be the same
                     # reference and also update in the event when we append later.
-                    auth_event_ids=auth_event_ids.copy(),
+                    state_event_ids=state_event_ids.copy(),
                 )
             else:
                 # TODO: Add some complement tests that adds state that is not member joins
@@ -249,12 +250,12 @@ async def persist_state_events_at_start(
                     # Make sure to use a copy of this list because we modify it
                     # later in the loop here. Otherwise it will be the same
                     # reference and also update in the event when we append later.
-                    auth_event_ids=auth_event_ids.copy(),
+                    state_event_ids=state_event_ids.copy(),
                 )
                 event_id = event.event_id
 
             state_event_ids_at_start.append(event_id)
-            auth_event_ids.append(event_id)
+            state_event_ids.append(event_id)
             # Connect all the state in a floating chain
             prev_event_ids_for_state_chain = [event_id]
 
@@ -265,7 +266,7 @@ async def persist_historical_events(
         events_to_create: List[JsonDict],
         room_id: str,
         inherited_depth: int,
-        auth_event_ids: List[str],
+        state_event_ids: List[str],
         app_service_requester: Requester,
     ) -> List[str]:
         """Create and persists all events provided sequentially. Handles the
@@ -281,8 +282,13 @@ async def persist_historical_events(
             room_id: Room where you want the events persisted in.
             inherited_depth: The depth to create the events at (you will
                 probably by calling inherit_depth_from_prev_ids(...)).
-            auth_event_ids: Define which events allow you to create the given
-                event in the room.
+            state_event_ids:
+                The full state at a given event. We share the same state across the whole
+                historical batch. The state events will be stripped down to only what's
+                necessary to auth a given event and set as the auth_event_ids. For
+                insertion events, we will add all of these state events as the explicit
+                state so the rest of the historical batch can inherit the same state and
+                state_group.
             app_service_requester: The requester of an application service.
 
         Returns:
@@ -325,7 +331,7 @@ async def persist_historical_events(
                 # The rest should hang off each other in a chain.
                 allow_no_prev_events=index == 0,
                 prev_event_ids=event_dict.get("prev_events"),
-                auth_event_ids=auth_event_ids,
+                state_event_ids=state_event_ids,
                 historical=True,
                 depth=inherited_depth,
             )
@@ -343,10 +349,10 @@ async def persist_historical_events(
             )
 
             logger.debug(
-                "RoomBatchSendEventRestServlet inserting event=%s, prev_event_ids=%s, auth_event_ids=%s",
+                "RoomBatchSendEventRestServlet inserting event=%s, prev_event_ids=%s, state_event_ids=%s",
                 event,
                 prev_event_ids,
-                auth_event_ids,
+                state_event_ids,
             )
 
             events_to_persist.append((event, context))
@@ -376,7 +382,7 @@ async def handle_batch_of_events(
         room_id: str,
         batch_id_to_connect_to: str,
         inherited_depth: int,
-        auth_event_ids: List[str],
+        state_event_ids: List[str],
         app_service_requester: Requester,
     ) -> Tuple[List[str], str]:
         """
@@ -391,8 +397,13 @@ async def handle_batch_of_events(
                 want this batch to connect to.
             inherited_depth: The depth to create the events at (you will
                 probably by calling inherit_depth_from_prev_ids(...)).
-            auth_event_ids: Define which events allow you to create the given
-                event in the room.
+            state_event_ids:
+                The full state at a given event. We share the same state across the whole
+                historical batch. The state events will be stripped down to only what's
+                necessary to auth a given event and set as the auth_event_ids. For
+                insertion events, we will add all of these state events as the explicit
+                state so the rest of the historical batch can inherit the same state and
+                state_group.
             app_service_requester: The requester of an application service.
 
         Returns:
@@ -438,7 +449,7 @@ async def handle_batch_of_events(
             events_to_create=events_to_create,
             room_id=room_id,
             inherited_depth=inherited_depth,
-            auth_event_ids=auth_event_ids,
+            state_event_ids=state_event_ids,
             app_service_requester=app_service_requester,
         )