State event updates are not forwarded to the widget from invitation room

[maheichyk]

State events that are shared on room invite (added into synapse room_prejoin_state->additional_event_types) are forwarded fine to the widget by Element.

But if the event is updated (resend with new content) and user is re-invited, then a new state event is received by Element but is not forwarded to the widget.

This PR suggests a fix and provides e2e test for the issue.

---

Here's what your changelog entry will look like:

🐛 Bug Fixes

• State event updates are not forwarded to the widget from invitation room (#9802). Contributed by @maheichyk.

[maheichyk]

Signed-off-by: Mikhail Aheichyk [email protected]

[turt2live]

I'm having trouble understanding the problem this PR is fixing: is it possible to get a bug report filed so we can get that information?

[maheichyk]

Problem is that there are cases when widget doesn’t receive events from the room where user has been invited.

This PR contains events.spec.ts that describes the case when custom state event is resent from the invited room. It is special case because it uses custom event that is configured in synapse to be shared on invite (by default only few events are shared). Event sending and user inviting works fine: widget receives the event, sending a new event and re-inviting user again - “fails” because widget doesn’t get a new event sent.

Another case was found by Dominik (@dhenneke) before and is described in separate issue. Current PR is updated to fix this issue (no e2e test though).

Both cases are related to the same part of StopGapWidget code:

matrix-react-sdk/src/stores/widgets/StopGapWidget.ts

Lines 493 to 527 in 7a36ba0

	const upToEventId = this.readUpToMap[ev.getRoomId()];
	if (upToEventId) {
	// Small optimization for exact match (prevent search)
	if (upToEventId === ev.getId()) {
	return;
	}
	let isBeforeMark = true;
	// Timelines are most recent last, so reverse the order and limit ourselves to 100 events
	// to avoid overusing the CPU.
	const timeline = this.client.getRoom(ev.getRoomId()).getLiveTimeline();
	const events = arrayFastClone(timeline.getEvents()).reverse().slice(0, 100);
	for (const timelineEvent of events) {
	if (timelineEvent.getId() === upToEventId) {
	break;
	} else if (timelineEvent.getId() === ev.getId()) {
	isBeforeMark = false;
	break;
	}
	}
	if (isBeforeMark) {
	// Ignore the event: it is before our interest.
	return;
	}
	}
	this.readUpToMap[ev.getRoomId()] = ev.getId();
	const raw = ev.getEffectiveEvent();
	this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId).catch((e) => {
	logger.error("Error sending event to widget: ", e);
	});

Statements 495-519 are executed in both cases. In the case 1 event is skipped because timeline (Line 504) of the room is empty. In the case 2 event is skipped because timeline contains only one event (that is a marker itself) and condition on Line 508 evaluates to true. In both cases these skipped events don’t have ids because of been in a stripped state and they are not in the timeline so condition on Line 510 that could allow them to follow is always false.

Statements 495-519 are executed if assignment of not undefined event_id happens on Line 522. It happens in both cases with m.room.member event that has event_id defined (it goes in contrast with all other events that are in stripped state and don't have event_id). In the case 1 the m.room.member with invite has an event_id. In the case 2 there is m.room.member with leave that has an event_id. After assignment happens the subsequent events from the room are not sent due to logic in statements 495-519.

This PR suggests to disable statements 495-519 logic for the invite and leave memberships and to keep it for join membership to fix the issue that events are skipped when timeline is empty and coming events are in stripped state.

[maheichyk]

events.spec.ts is changed a bit to use m.room.topic (is shared by synapse by default) instead of custom event to describe the problem.

[robintown]

I think this is fine - security implications were what we were concerned about, but since the only thing this code path seems to be responsible for is preventing duplicate or out of order events, I don't see that applying.

The code looks good!

[andybalaam]

Force-merging.

Failures:

• coverage: there is adequate system testing, and unit testing in this area has little value since the state is hard to replicate.
• cypress failure is a flaky test.

[andybalaam]

Thank you!