From 384ccf2c86f9190e2867212514ac8202981f57b5 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Fri, 2 Dec 2022 18:00:01 +0100 Subject: [PATCH] Faster joins: don't stale when an user joins during a fast join Signed-off-by: Mathieu Velten --- changelog.d/14606.misc | 1 + synapse/handlers/event_auth.py | 8 ++--- synapse/handlers/federation.py | 6 ++-- synapse/handlers/federation_event.py | 5 +-- synapse/handlers/room_member.py | 52 +++++++++++++++++++++------- 5 files changed, 50 insertions(+), 22 deletions(-) create mode 100644 changelog.d/14606.misc diff --git a/changelog.d/14606.misc b/changelog.d/14606.misc new file mode 100644 index 000000000000..52f9a901e915 --- /dev/null +++ b/changelog.d/14606.misc @@ -0,0 +1 @@ +Faster joins: don't stale when another user joins during a fast join resync. diff --git a/synapse/handlers/event_auth.py b/synapse/handlers/event_auth.py index f91dbbecb79c..a258af6f4473 100644 --- a/synapse/handlers/event_auth.py +++ b/synapse/handlers/event_auth.py @@ -202,7 +202,7 @@ async def check_restricted_join_rules( state_ids: StateMap[str], room_version: RoomVersion, user_id: str, - prev_member_event: Optional[EventBase], + prev_membership: Optional[str], ) -> None: """ Check whether a user can join a room without an invite due to restricted join rules. @@ -214,15 +214,13 @@ async def check_restricted_join_rules( state_ids: The state of the room as it currently is. room_version: The room version of the room being joined. user_id: The user joining the room. - prev_member_event: The current membership event for this user. + prev_membership: The current membership state for this user. Raises: AuthError if the user cannot join the room. """ # If the member is invited or currently joined, then nothing to do. - if prev_member_event and ( - prev_member_event.membership in (Membership.JOIN, Membership.INVITE) - ): + if prev_membership in (Membership.JOIN, Membership.INVITE): return # This is not a room with a restricted join rule, so we don't need to do the diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index eca75f1108d1..0d2e1700b949 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -599,6 +599,8 @@ async def do_invite_join( except ValueError: pass + already_partially_joined = await self.store.is_partial_state_room(room_id) + ret = await self.federation_client.send_join( host_list, event, room_version_obj ) @@ -629,7 +631,7 @@ async def do_invite_join( state_events=state, ) - if ret.partial_state: + if ret.partial_state and not already_partially_joined: # Mark the room as having partial state. # The background process is responsible for unmarking this flag, # even if the join fails. @@ -676,7 +678,7 @@ async def do_invite_join( # state for the room. # If the join failed, the background process is responsible for # cleaning up — including unmarking the room as a partial state room. - if ret.partial_state: + if ret.partial_state and not already_partially_joined: # Kick off the process of asynchronously fetching the state for this # room. run_as_background_process( diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 31df7f55cc97..bb39cae01f27 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -438,16 +438,17 @@ async def check_join_restrictions( # Check if the user is already in the room or invited to the room. user_id = event.state_key prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None) - prev_member_event = None + prev_membership = None if prev_member_event_id: prev_member_event = await self._store.get_event(prev_member_event_id) + prev_membership = prev_member_event.membership # Check if the member should be allowed access via membership in a space. await self._event_auth_handler.check_restricted_join_rules( prev_state_ids, event.room_version, user_id, - prev_member_event, + prev_membership, ) @trace diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index d236cc09b526..e9061049c5c1 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -823,8 +823,14 @@ async def update_membership_locked( latest_event_ids = await self.store.get_prev_events_for_room(room_id) + # There are 2 reasons that mandate to have the full state: + # - to calculate `is_host_in_room`, however we now approximate that we consider the server + # in the room if we are still in the middle of a fast join. + # - to calculate if another user trying to join is allowed to. In this case we now + # make another `make/send_join` to a resident server to validate that. We could just + # accept it but it would mean that we would potentially leak the history to a banned user. state_before_join = await self.state_handler.compute_state_after_events( - room_id, latest_event_ids + room_id, latest_event_ids, await_full_state=False ) # TODO: Refactor into dictionary of explicitly allowed transitions @@ -881,7 +887,10 @@ async def update_membership_locked( if action == "kick": raise AuthError(403, "The target user is not in the room") - is_host_in_room = await self._is_host_in_room(state_before_join) + is_partial_state_room = await self.store.is_partial_state_room(room_id) + is_host_in_room = await self._is_host_in_room( + is_partial_state_room, state_before_join + ) if effective_membership_state == Membership.JOIN: if requester.is_guest: @@ -927,6 +936,7 @@ async def update_membership_locked( room_id, remote_room_hosts, content, + is_partial_state_room, is_host_in_room, state_before_join, ) @@ -1073,6 +1083,7 @@ async def _should_perform_remote_join( room_id: str, remote_room_hosts: List[str], content: JsonDict, + is_partial_state_room: bool, is_host_in_room: bool, state_before_join: StateMap[str], ) -> Tuple[bool, List[str]]: @@ -1093,6 +1104,8 @@ async def _should_perform_remote_join( remote_room_hosts: A list of remote room hosts. content: The content to use as the event body of the join. This may be modified. + is_partial_state_room: True if the server currently doesn't hold the fully + state of the room. is_host_in_room: True if the host is in the room. state_before_join: The state before the join event (i.e. the resolution of the states after its parent events). @@ -1109,6 +1122,20 @@ async def _should_perform_remote_join( if not is_host_in_room: return True, remote_room_hosts + prev_member_event_id = state_before_join.get((EventTypes.Member, user_id), None) + previous_membership = None + if prev_member_event_id: + prev_member_event = await self.store.get_event(prev_member_event_id) + previous_membership = prev_member_event.membership + + # If we are not fully join yet, and the target is not already in the room, + # let's do a remote join so another server with the full state can validate + # that the user has not been ban for example. + # We could just accept the join and wait for state-res to resolve that later on + # but we would then leak room history to this person until then, which is pretty bad. + if is_partial_state_room and previous_membership != Membership.JOIN: + return True, remote_room_hosts + # If the host is in the room, but not one of the authorised hosts # for restricted join rules, a remote join must be used. room_version = await self.store.get_room_version(room_id) @@ -1122,15 +1149,8 @@ async def _should_perform_remote_join( # If the user is invited to the room or already joined, the join # event can always be issued locally. - prev_member_event_id = state_before_join.get((EventTypes.Member, user_id), None) - prev_member_event = None - if prev_member_event_id: - prev_member_event = await self.store.get_event(prev_member_event_id) - if prev_member_event.membership in ( - Membership.JOIN, - Membership.INVITE, - ): - return False, [] + if previous_membership in (Membership.JOIN, Membership.INVITE): + return False, [] # If the local host has a user who can issue invites, then a local # join can be done. @@ -1154,7 +1174,7 @@ async def _should_perform_remote_join( # Ensure the member should be allowed access via membership in a room. await self.event_auth_handler.check_restricted_join_rules( - state_before_join, room_version, user_id, prev_member_event + state_before_join, room_version, user_id, previous_membership ) # If this is going to be a local join, additional information must @@ -1634,7 +1654,13 @@ async def _make_and_store_3pid_invite( ) return event, stream_id - async def _is_host_in_room(self, current_state_ids: StateMap[str]) -> bool: + async def _is_host_in_room( + self, is_partial_state_room: bool, current_state_ids: StateMap[str] + ) -> bool: + # When we only have a partial state, let's assume we are still in the room + if is_partial_state_room: + return True + # Have we just created the room, and is this about to be the very # first member event? create_event_id = current_state_ids.get(("m.room.create", ""))