Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix join being denied after being invited over federation #18075

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jan 10, 2025

Fix join being denied after being invited over federation. I initially thought this might be a Synapse consistency issue (see issues labeled with Z-Read-After-Write) but it seems to be an event auth logic problem. Workers probably do increase the number of possible race condition scenarios that make this visible though (replication and cache invalidation lag).

Note

The fix hasn't been implemented yet (just an investigation)

Probably fixes matrix-org/synapse#15012 (#15012)

Problems:

  1. We don't consider out-of-band membership (outliers) in our event_auth logic even though we expose them in /sync.
  2. Perhaps we should consider authing events in the persistence queue as events already in the queue could allow subsequent events to be allowed (events come through many channels: federation transaction, remote invite, remote join, local send). But this doesn't save us in the case where the event is more delayed over federation.

What happened before?

I wrote a Complement test that stresses this exact scenario and reproduces the problem: matrix-org/complement#754

COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestSynapseConsistency

We have hs1 and hs2 running in monolith mode (no workers):

  1. @charlie1:hs2 is invited and joins the room:
    1. hs1 invites @charlie1:hs2 to a room which we receive on hs2 as PUT /_matrix/federation/v1/invite/{roomId}/{eventId} (on_invite_request(...)) and the invite membership is persisted as an outlier. The room_membership and local_current_membership database tables are also updated which means they are visible down /sync at this point.
    2. @charlie1:hs2 decides to join because it saw the invite down /sync. Because hs2 is not yet in the room, this happens as a remote join make_join/send_join which comes back with all of the auth events needed to auth successfully and now @charlie1:hs2 is successfully joined to the room.
  2. @charlie2:hs2 is invited and and tries to join the room:
    1. hs1 invites @charlie2:hs2 to the room which we receive on hs2 as PUT /_matrix/federation/v1/invite/{roomId}/{eventId} (on_invite_request(...)) and the invite membership is persisted as an outlier. The room_membership and local_current_membership database tables are also updated which means they are visible down /sync at this point.
    2. Because hs2 is already participating in the room, we also see the invite come over federation in a transaction and we start processing it (not done yet, see below)
    3. @charlie2:hs2 decides to join because it saw the invite down /sync. Because hs2, is already in the room, this happens as a local join but we deny the event because our event_auth logic thinks that we have no membership in the room ❌ (expected to be able to join because we saw the invite down `/sync)
    4. We finally finish processing the @charlie2:hs2 invite event from and de-outlier it.
      • If this finished before we tried to join we would have been fine but this is the race condition that makes this situation visible.

Logs:

🗳️ on_invite_request: handling event <FrozenEventV3 event_id=$PRPCvdXdcqyjdUKP_NxGF2CcukmwOaoK0ZR1WiVOZVk, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=invite, outlier=False>
🔦 _store_room_members_txn update room_memberships: <FrozenEventV3 event_id=$lxOjwdOr3GV3RDDEMQusTNiRyVuT94Z3SIEzp4uKIbc, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=invite, outlier=True>
🔦 _store_room_members_txn update local_current_membership: <FrozenEventV3 event_id=$lxOjwdOr3GV3RDDEMQusTNiRyVuT94Z3SIEzp4uKIbc, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=invite, outlier=True>
📨 Notifying about new event <FrozenEventV3 event_id=$PRPCvdXdcqyjdUKP_NxGF2CcukmwOaoK0ZR1WiVOZVk, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=invite, outlier=True>
✅ on_invite_request: handled event <FrozenEventV3 event_id=$PRPCvdXdcqyjdUKP_NxGF2CcukmwOaoK0ZR1WiVOZVk, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=invite, outlier=True>
🧲 do_invite_join for @user-2-charlie1:hs2 in !sfZVBdLUezpPWetrol:hs1
🔦 _store_room_members_txn update room_memberships: <FrozenEventV3 event_id=$bwv8LxFnqfpsw_rhR7OrTjtz09gaJ23MqstKOcs7ygA, type=m.room.member, state_key=@user-1-alice:hs1, membership=join, outlier=True>
🔦 _store_room_members_txn update room_memberships: <FrozenEventV3 event_id=$4lEMP-YH4vPhSr7Be0w-mZ69O_C5Q0krLc-SgCJPrys, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=join, outlier=False>
📨 Notifying about new event <FrozenEventV3 event_id=$oju1ts3G3pz5O62IesrxX5is4LxAwU3WPr4xvid5ijI, type=m.room.member, state_key=@user-2-charlie1:hs2, membership=join, outlier=False>

...

🗳️ on_invite_request: handling event <FrozenEventV3 event_id=$O_54j7O--6xMsegY5EVZ9SA-mI4_iHJOIoRwYyeWIPY, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=False>
🔦 _store_room_members_txn update room_memberships: <FrozenEventV3 event_id=$hmWIgssP6TKb826XYpczp-dUgWGkhotu_ru-P3z48ww, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=True>
🔦 _store_room_members_txn update local_current_membership: <FrozenEventV3 event_id=$hmWIgssP6TKb826XYpczp-dUgWGkhotu_ru-P3z48ww, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=True>
📨 Notifying about new event <FrozenEventV3 event_id=$O_54j7O--6xMsegY5EVZ9SA-mI4_iHJOIoRwYyeWIPY, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=True>
✅ on_invite_request: handled event <FrozenEventV3 event_id=$O_54j7O--6xMsegY5EVZ9SA-mI4_iHJOIoRwYyeWIPY, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=True>
📬 handling received PDU in room !sfZVBdLUezpPWetrol:hs1: <FrozenEventV3 event_id=$O_54j7O--6xMsegY5EVZ9SA-mI4_iHJOIoRwYyeWIPY, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=False>
📮 handle_new_client_event: handling <FrozenEventV3 event_id=$WNVDTQrxy5tCdPQHMyHyIn7tE4NWqKsZ8Bn8R4WbBSA, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=join, outlier=False>
❌ Denying new event <FrozenEventV3 event_id=$WNVDTQrxy5tCdPQHMyHyIn7tE4NWqKsZ8Bn8R4WbBSA, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=join, outlier=False> because 403: You are not invited to this room.
synapse.http.server - 130 - INFO - POST-16 - <SynapseRequest at 0x7f460c91fbf0 method='POST' uri='/_matrix/client/v3/join/%21sfZVBdLUezpPWetrol:hs1?server_name=hs1' clientproto='HTTP/1.0' site='8080'> SynapseError: 403 - You are not invited to this room.
📨 Notifying about new event <FrozenEventV3 event_id=$O_54j7O--6xMsegY5EVZ9SA-mI4_iHJOIoRwYyeWIPY, type=m.room.member, state_key=@user-3-charlie2:hs2, membership=invite, outlier=False>

Dev notes

handle_new_client_event
_persist_events
persist_and_notify_client_events
	persist_events
	notifier.on_new_room_events(
_process_incoming_pdus_in_room_inner
on_receive_pdu
_process_received_pdu
_run_push_actions_and_persist_event
persist_events_and_notify
	_send_events
		ReplicationFederationSendEventsRestServlet._handle_request
		persist_events_and_notify
	persist_events
		enqueue
		_persist_event_batch
		_persist_events_and_state_updates
		_persist_events_txn
		_update_metadata_tables_txn
		_store_room_members_txn
	_notify_persisted_event
		_notifier.on_new_room_events(
on_invite_request
persist_events_and_notify
update_membership_locked
	_remote_join
		do_invite_join
		process_remote_join
		persist_events_and_notify
	_local_membership_update
_notifier.on_new_room_events(
notify_new_room_events
	_notify_pending_new_room_events
		on_new_event
	notify_replication

handle_new_client_event
check_auth_rules_from_context
check_state_dependent_auth_rules
_is_membership_change_allowed

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

MadLittleMods added a commit to MadLittleMods/fork-complement that referenced this pull request Jan 10, 2025
MadLittleMods added a commit to MadLittleMods/fork-complement that referenced this pull request Jan 10, 2025
@MadLittleMods MadLittleMods changed the title Fix Synapse consistency issue with join being denied after being invited over federation Fix join being denied after being invited over federation Jan 10, 2025
Comment on lines +2304 to 2306
# TODO: Is this correct?
# if not event.internal_metadata.is_outlier():
await self._notify_persisted_event(event, max_stream_token)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to notify about out-of-band membership ⏩

Comment on lines 2283 to +2286
# 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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Remove this

Comment on lines +681 to +682
# You can only join the room if you are invited or are already in the room.
if not (caller_in_room or caller_invited):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is equivalent to before but I found this easier to grok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same via De Morgan's theorem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Federation automatic joins raise 403 on callee homeserver
2 participants