Locally-rejected invites don't get passed on to the client over SSS

[richvdh]

Description

[in fairness I have not validated if this is a server-side or client-side problem]

Background: When a user rejects an invite, we generate a "leave" event to update their membership state. For an invite received from a remote server, we normally ask the remote server to generate the leave event for us. However, there is a fallback case for when the remote server is unreachable: we will generate an "outlier" event locally, which is then only sent to the invited user.

It appears that such locally-generated leave events are not passed down SSS.

In other words: this is a regression of matrix-org/synapse#2181, but applies only to SSS.

Steps to reproduce

• Receive an invite from a user on a remote server
• The remote server goes offline
• On (say) Element-Web, reject the invite
• On EX, observe that you still have a pending invite

Homeserver

sw1v.org

Synapse Version

v1.115.0

Installation Method

pip (from PyPI)

Database

postgres. no. yes. yes.

Workers

Single process

Platform

Configuration

Relevant log output

-

Anything else that would be useful to know?

[Andoriax]

I have something like this going on with EX, but it cleared up after rejecting Invites in Element then logging out on EX and back in on EX to find the Invites list updated and cleared up.

[HarHarLinks]

I was having this issue for a long time now that an (abusive) invite is sitting in my EXA inbox, which is not there in any of my many other incl non-Element clients and while in principle I know enough workarounds to get rid of it, my goal is to help uncover and keep track of flaws impacting "Matrix 2.0" users with this.

I've had the time to investigate a bit and the invite is definitely coming down synapse's SSS endpoint. Sadly it's almost impossible to check the stable sync for it manually, but I tested that a client doing a stable initial sync does not receive it.

I'm using the mjolnir Antispam module off the shelf from the matrix-docker-ansible-deploy playbook with #matrix-org-coc-bl:matrix.org and CME.

My suspicion is currently that I'm hitting this bug or it's equivalent with the module API.

[devonh]

I have been digging into this and this is what I have found:

• the matrix-rust-sdk is expecting a SSS response with the leave event in the rooms: "required_state" section
• Synapse doesn't appear to provide the leave event in the appropriate section when rejecting a remote invite
  • this is true whether or not the remote server is up (ie. whether or not we resort to the local fallback mechanism mentioned)
• When calculating new forward extremities after receiving a remote invite or generating a leave for a remote room, there is no forward extremity returned from the calculation:

  synapse/synapse/storage/controllers/persist_events.py

  Line 674 in 1efb826

  new_latest_event_ids = await self._calculate_new_extremities(

  • This is because the invite & the leave are outliers:

    synapse/synapse/storage/controllers/persist_events.py

    Lines 781 to 789 in 1efb826

    	# we're only interested in new events which aren't outliers and which aren't
    	# being rejected.
    	new_events = [
    	event
    	for event, ctx in event_contexts
    	if not event.internal_metadata.is_outlier()
    	and not ctx.rejected
    	and not event.internal_metadata.is_soft_failed()
    	]

  • This leads to not updating the current_state_delta_stream:

    synapse/synapse/storage/controllers/persist_events.py

    Line 646 in 1efb826

    await self.persist_events_store._persist_events_and_state_updates(

    synapse/synapse/storage/databases/main/events.py

    Lines 340 to 348 in 1efb826

    	sliding_sync_table_changes = None
    	if state_delta_for_room is not None:
    	sliding_sync_table_changes = (
    	await self._calculate_sliding_sync_table_changes(
    	room_id, events_and_contexts, state_delta_for_room
    	)
    	)
    	await self.db_pool.runInteraction(

    synapse/synapse/storage/databases/main/events.py

    Lines 1010 to 1023 in 1efb826

    	# We call this last as it assumes we've inserted the events into
    	# room_memberships, where applicable.
    	# NB: This function invalidates all state related caches
    	if state_delta_for_room:
    	# If the state delta exists, the sliding sync table changes should also exist
    	assert sliding_sync_table_changes is not None
    	self._update_current_state_txn(
    	txn,
    	room_id,
    	state_delta_for_room,
    	min_stream_order,
    	sliding_sync_table_changes,
    	)

  • which leads to not sending the leave down SSS:

    synapse/synapse/handlers/sliding_sync/room_lists.py

    Lines 294 to 311 in 1efb826

    	(
    	newly_joined_room_ids,
    	newly_left_room_map,
    	) = await self._get_newly_joined_and_left_rooms(
    	user_id, from_token=from_token, to_token=to_token
    	)
    	dm_room_ids = await self._get_dm_rooms_for_user(user_id)
    	# Add back `newly_left` rooms (rooms left in the from -> to token range).
    	#
    	# We do this because `get_sliding_sync_rooms_for_user(...)` doesn't include
    	# rooms that the user left themselves as it's more efficient to add them back
    	# here than to fetch all rooms and then filter out the old left rooms. The user
    	# only leaves a room once in a blue moon so this barely needs to run.
    	#
    	missing_newly_left_rooms = (
    	newly_left_room_map.keys() - room_membership_for_user_map.keys()
    	)

    synapse/synapse/handlers/sliding_sync/room_lists.py

    Lines 1118 to 1122 in 1efb826

    	# 1) Fetch membership changes that fall in the range from `from_token` up to `to_token`
    	current_state_delta_membership_changes_in_from_to_range = []
    	if from_token:
    	current_state_delta_membership_changes_in_from_to_range = (
    	await self.store.get_current_state_delta_membership_changes_for_user(

I have this reproducible behaviour as a complement-crypto test. The test passes if I swtich bob/client2 to also be on hs1 so the invite & leave are no longer for a remote room:

// A and B are on different servers.
// A invites B to a room.
// A's server goes offline.
// B rejects the invite.
// B should see a `leave` for the room come down /sync.
func TestRemoteInviteRejectionSlidingSync(t *testing.T) {
	Instance().ForEachClientType(t, func(t *testing.T, clientType api.ClientType) {
		tc := Instance().CreateTestContext(t, api.ClientType{
			Lang: clientType.Lang,
			HS:   "hs1",
		}, api.ClientType{
			Lang: clientType.Lang,
			HS:   "hs2",
		})

		// NOTE: sleep a while to let Synapse background updates catch up, to ensure we use the
		// new sliding sync tables, not the fallback
		time.Sleep(time.Minute)

		tc.WithClientSyncing(t, &cc.ClientCreationRequest{
			User: tc.Bob,
		}, func(bob api.TestClient) {
			// Ensure plenty of time for initial sync to have happened
			time.Sleep(time.Second * 3)

			// Alice creates a new room and invites Bob
			roomID := tc.CreateNewEncryptedRoom(t, tc.Alice, cc.EncRoomOptions.Invite([]string{tc.Bob.UserID}))

			// Ensure plenty of time for bob to receive invite
			time.Sleep(time.Second * 3)

			// And Alice's HS becomes unreachable
			//tc.Deployment.PauseServer(t, "hs1")

			// And when Bob rejects the invite
			tc.Bob.MustLeaveRoom(t, roomID)

			// Then Bob should receive the leave down /sync
			bob.WaitUntilEventInRoom(
				t,
				roomID,
				api.CheckEventHasMembership(bob.UserID(), "leave"),
			).Waitf(t, 5*time.Second, "bob did not see bob's leave")
		})
	})
}

I have a very hacky version of Synapse going where it does respond with the leave event in SSS required_state, but the test still fails. I think there may be another issue where the rust-sdk is eating the leave event.

[devonh]

I no longer think there is an issue in the rust-sdk with the leave event. I have written a unit test for the SDK and it handles the leave just fine. I think what I was seeing was an issue with the way the complement test waits for events vs what my test expected. It waits for timeline events, but in this case the event wasn't being added to the timeline. Just the required_state.

[devonh]

I tested this out further with the lastest version of Element X Android.
When rejecting the remote invite in Element Web, it takes a while before Element X Android also eventually removes the invite.
With my hacky Synapse fix, as soon as Element Web finishes rejecting the invite (which can take a while with the remote server being down), Element X Android also instantly removes the invite. So the behaviour is much better when we pass the leave event down the required_state field in the SSS response.

[devonh]

And here is the Synapse hack which "fixes" the issue: #18278

[erikjohnston]

This boils down to a combination of two things when sliding sync calculates the set of rooms the user is interested in:

1. get_sliding_sync_rooms_for_user explicitly does not return rooms the user has left. This is for performance, as large accounts often have a huge number of left rooms.
2. _get_newly_joined_and_left_rooms, which adds back in rooms that the user has newly left, uses the current_state_delta_stream table to pick up changes in membership. However, the current state only is filled out for rooms the server is in, and so doesn't pick up out-of-band leaves (including locally rejected invites).

[erikjohnston]

A WIP is here: https://github.com/element-hq/synapse/compare/erikj/ss_reject_invites

A few things left to do (beyond cleaning it up), see # TODO in the code:

1. Correctly filter out membership "changes" caused by display name changes (which causes JOIN -> JOIN)
2. Add an index
3. In get_room_sync_data we need to pull out the membership change

[devonh]

I added a commit to that branch that I think accomplishes 1 & 3 from the list above & fixes the linter errors.
It also expands the unit test to validate the response actually contains the leave as expected.

Remaining work should be to add the index.

[devonh]

I've tested out these changes using element web (using develop.element.io to enable experimental sliding sync)

Element version: 421a79aaa56f-js-f322f32a077f
Crypto version: Rust SDK 0.11.0 (f2e32d4), Vodozemac 0.9.0

I have:

• tester_a - old sync
• tester_a - sliding sync
• tester_b

With the develop branch of Synapse, if tester_b invites tester_a to a room, and tester_a - old sync rejects it, the invite is stuck on tester_a sliding sync (present, and cannot reject due to error: Failed to reject invite
MatrixError: [500] Internal server error (http://localhost:8080/_matrix/client/v3/rooms/!jetvclGxXQAszbvsLg%3Alocalhost%3A8481/leave))

With the erikj/ss_reject_invites branch, if tester_b invites tester_a to a room, and tester_a - old sync rejects it, the invite is immediately cleared on tester_a - sliding sync.