Stop returning unsigned.invite_room_state in PUT /_matrix/federation/v2/invite/{roomId}/{eventId} responses

[anoadragon453]

We were adding an 'invite_room_state' field in the 'unsigned' dict of invite events in response to a
PUT /_matrix/federation/v2/invite/{roomId}/{eventId} request.

Internally, Synapse expects this to field to exist (as it did in the v1 version of this API), so we plonked it on before reusing the old code.

Unfortunately, we then forgot to remove it before sending the event out again.

Confirmed the problem and the fix using PyCharm's debugger. I thought about writing a unit/complement test but it's a bit specific...

More information about stripped state events.

I assume this is backwards-compatible, and that nobody is relying on it. It appears in an example in the S2S spec (likely by accident), but not the actual schema.

[DMRobertson]

Related: #10800?

[squahtx]

Will we expose unsigned.invite_room_state in other endpoints? (and does that matter?)

[anoadragon453]

The field is purposely stripped when serialising an event for clients:

synapse/synapse/events/utils.py

Lines 371 to 377 in 06df5d4

	# invite_room_state and knock_room_state are a list of stripped room state events
	# that are meant to provide metadata about a room to an invitee/knocker. They are
	# intended to only be included in specific circumstances, such as down sync, and
	# should not be included in any other case.
	if not config.include_stripped_room_state:
	d["unsigned"].pop("invite_room_state", None)
	d["unsigned"].pop("knock_room_state", None)

which covers the areas where clients could get access to this field. As for federation, it may be included during a backfill... perhaps it's something we should additionally add to filter_events_for_server:

synapse/synapse/visibility.py

Lines 534 to 623 in 535f8c8

	async def filter_events_for_server(
	storage: StorageControllers,
	server_name: str,
	events: List[EventBase],
	redact: bool = True,
	check_history_visibility_only: bool = False,
	) -> List[EventBase]:
	"""Filter a list of events based on whether given server is allowed to
	see them.
	Args:
	storage
	server_name
	events
	redact: Whether to return a redacted version of the event, or
	to filter them out entirely.
	check_history_visibility_only: Whether to only check the
	history visibility, rather than things like if the sender has been
	erased. This is used e.g. during pagination to decide whether to
	backfill or not.
	Returns
	The filtered events.
	"""
	def is_sender_erased(event: EventBase, erased_senders: Dict[str, bool]) -> bool:
	if erased_senders and erased_senders[event.sender]:
	logger.info("Sender of %s has been erased, redacting", event.event_id)
	return True
	return False
	def check_event_is_visible(
	visibility: str, memberships: StateMap[EventBase]
	) -> bool:
	if visibility not in (HistoryVisibility.INVITED, HistoryVisibility.JOINED):
	return True
	# We now loop through all membership events looking for
	# membership states for the requesting server to determine
	# if the server is either in the room or has been invited
	# into the room.
	for ev in memberships.values():
	assert get_domain_from_id(ev.state_key) == server_name
	memtype = ev.membership
	if memtype == Membership.JOIN:
	return True
	elif memtype == Membership.INVITE:
	if visibility == HistoryVisibility.INVITED:
	return True
	# server has no users in the room: redact
	return False
	if not check_history_visibility_only:
	erased_senders = await storage.main.are_users_erased(e.sender for e in events)
	else:
	# We don't want to check whether users are erased, which is equivalent
	# to no users having been erased.
	erased_senders = {}
	# Let's check to see if all the events have a history visibility
	# of "shared" or "world_readable". If that's the case then we don't
	# need to check membership (as we know the server is in the room).
	event_to_history_vis = await _event_to_history_vis(storage, events)
	# for any with restricted vis, we also need the memberships
	event_to_memberships = await _event_to_memberships(
	storage,
	[
	e
	for e in events
	if event_to_history_vis[e.event_id]
	not in (HistoryVisibility.SHARED, HistoryVisibility.WORLD_READABLE)
	],
	server_name,
	)
	to_return = []
	for e in events:
	erased = is_sender_erased(e, erased_senders)
	visible = check_event_is_visible(
	event_to_history_vis[e.event_id], event_to_memberships.get(e.event_id, {})
	)
	if visible and not erased:
	to_return.append(e)
	elif redact:
	to_return.append(prune_event(e))
	return to_return

[anoadragon453]

Actually, perhaps it's best if we just stop storing these fields altogether. I've filed #14160 to discuss it.

[DMRobertson]

Looks like tests fell foul of #14079. I'll re-run. (Edit: err, merge in develop).

[DMRobertson]

@anoadragon453 looks like this is good to go now. (I assume you still want to land this?)

[anoadragon453]

Yep, thank you!