MSC3266: Room summary API

[deepbluev7]

Rendered.

Somewhat related to MSC2946 this provides an API to get a summary for a specific room either from the local server or over federation.

Useful for a few cases, where you need to show a summary for a room like matrix.to, traveler bots, showing spaces, lightweight clients, etc.

Open design questions looking for feedback: see this comment chain: #3266 (comment) resolved as of 2021/10/06

Implementations:

• matrix.to: matrix-org/matrix.to#219
• Synapse: matrix-org/synapse#10394 (Only CS API) and matrix-org/synapse#11507 (for the federation bits). The allowed_room_ids part is implemented here: deepbluev7/synapse@37f4253.
• Element Android: Better room preview, use room Summary API if available element-hq/element-android#3986
• Element Web (for member counts on space invites): For space invite previews, use room summary API to get the right member count matrix-react-sdk#6982 and Add method to fetch the MSC3266 Room Summary of a Room matrix-js-sdk#1988
• Nheko (to have a prettier join room preview): Nheko-Reborn/nheko@9d8d6b4
• matrix-rust-sdk: room preview: use the room summary MSC3266 endpoint matrix-rust-sdk#3339
• shields.io: https://shields.io/badges/matrix, Support [Matrix] summary endpoint badges/shields#10782
• Matrix Room Search: https://gitlab.com/etke.cc/mrs/api/-/commit/bc4a4a649cfbf98d850ea0248c78afc5d552e1be

Signed-off-by: Nicolas Werner [email protected]

---

SCT stuff:

FCP tickyboxes

Checklist: #3266 (comment)

[aine-etke]

@deepbluev7 could you update the implementations list with MRS, please?

• issue: https://gitlab.com/etke.cc/mrs/api/-/issues/27
• commit: https://gitlab.com/etke.cc/mrs/api/-/commit/bc4a4a649cfbf98d850ea0248c78afc5d552e1be

[turt2live]

The SCT is aiming to ensure MSCs actually pass the checklist before FCP is proposed. This comment is that checklist tracking.

• Are appropriate implementation(s) specified in the MSC’s PR description?
• Are all MSCs that this MSC depends on already accepted?
• For each new endpoint that is introduced:
  • Have authentication requirements been specified?
  • Have rate-limiting requirements been specified?
  • Have guest access requirements been specified?
  • Are error responses specified?
    • Does each error case have a specified errcode (e.g. M_FORBIDDEN) and HTTP status code?
      • If a new errcode is introduced, is it clear that it is new?
• Will the MSC require a new room version, and if so, has that been made clear?
  • Is the reason for a new room version clearly stated? For example, modifying the set of redacted fields changes how event IDs are calculated, thus requiring a new room version.
• Are backwards-compatibility concerns appropriately addressed?
• Are the endpoint conventions honoured?
  • Do HTTP endpoints use_underscores_like_this?
  • Will the endpoint return unbounded data? If so, has pagination been considered?
  • If the endpoint utilises pagination, is it consistent with the appendices?
• An introduction exists and clearly outlines the problem being solved. Ideally, the first paragraph should be understandable by a non-technical audience.
• All outstanding threads are resolved
  • All feedback is incorporated into the proposal text itself, either as a fix or noted as an alternative
• While the exact sections do not need to be present, the details implied by the proposal template are covered. Namely:
  • Introduction
  • Proposal text
  • Potential issues
  • Alternatives
  • Dependencies
• Stable identifiers are used throughout the proposal, except for the unstable prefix section
  • Unstable prefixes consider the awkward accepted-but-not-merged state
  • Chosen unstable prefixes do not pollute any global namespace (use “org.matrix.mscXXXX”, not “org.matrix”).
• Changes have applicable Sign Off from all authors/editors/contributors
• There is a dedicated "Security Considerations" section which detail any possible attacks/vulnerabilities this proposal may introduce, even if this is "None.". See RFC3552 for things to think about, but in particular pay attention to the OWASP Top Ten.

[deepbluev7]

Okay, I updated the MSC with most of the experiences I had with it from the last 2 years. Also shields.io now uses it, so does that remove your concerns regarding the unauthenticated usage, @turt2live? It apparently is a lot better than their previous approach of registering a new guest user per request.

[turt2live]

I haven't perfectly verified the implementation matches, especially on the server side, but this has been sitting for far too long anyways. Let's ship it.

[turt2live]

@mscbot fcp merge

[mscbot]

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

• @clokep
• @dbkr
• @uhoreg
• @turt2live
• @ara4n
• @anoadragon453
• @richvdh
• @tulir
• @erikjohnston
• @KitsuneRal

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

[richvdh]

What exactly does publicly accessible mean here? join_rule: public, presumably?

(obligatory mention of matrix-org/matrix-spec#633)

[deepbluev7]

It means if the content can be accessed by anyone. This can be either join_rule of public, history visibility of world_readable and also rooms you can knock on: matrix-org/matrix-spec#1184

This might change in the future, but as you can currently preview such rooms via the /hierarchy API, this MSC doesn't change it. Whatever the conclusion ends up being for the hierarchy API long term should also apply to this MSC.

An argument could be made, that world_readable should be required for unauthenticated users, but that doesn't match what is currently implemented. Another alternative could be to have a separate setting for if the room info is publicly readable instead of only having a toggle for messages and the room info. But in general this room info is already readable without joining the room, so it isn't clear why authentication would be required.

The wording of this MSC was changed multiple times already during the discussion on the issue I linked and in the end I reverted it to "publicly accessible" as I really don't want to split the discussion. I would prefer it someone wrote an MSC to clarify matrix-org/matrix-spec#1184, matrix-org/matrix-spec#633, etc, instead of me having to do that on the side on this MSC. I was also told to have the MSC match the current implementation and not do additional changes to it.

[richvdh]

To be explicit, I am concerned about this sentence:

For unauthenticated requests a response should only be returned if the room is publicly accessible.

I'm not sure it's true that this wording has changed multiple times, and I don't see anywhere it has been discussed. But anyway, it seems like you have a clear idea of what is meant; I'm just asking that we make it explicit.

Suggested change

	returned if the room is publicly accessible.
	returned if the room is publicly accessible; specifically, that means either:
	* the room has `join_rule: public` or `join_rule: knock`, or
	* the room has `history_visibility: world_readable`.

It probably also needs more paragraph breaks, for the bullet list to make sense.

[richvdh]

I spent a while looking for this in C-S /hierarchy:

Suggested change

	| allowed_room_ids | Room ids allows in restricted joins. | Copied from `hierarchy`. Necessary to distinguish if a room can be joined or only knocked at. |
	| allowed_room_ids | Room ids allows in restricted joins. | Copied from [`GET /_matrix/federation/v1/hierarchy/{roomId}`](https://spec.matrix.org/v1.13/server-server-api/#get_matrixfederationv1hierarchyroomid). Necessary to distinguish if the room can be joined or only knocked at. |

[richvdh]

Does synapse actually implement this for im.nheko.summary right now? The PR linked in the MSC description (matrix-org/synapse#10394) doesn't include it. If it does, could you link to the implementation?

[deepbluev7]

As mentioned in the PR description, this is currently not implemented in Synapse. For summaries fetched over federation this can be implemented by removing this line of code: https://github.com/matrix-org/synapse/pull/11507/files#diff-e459d7f3392554e6e1672632312969188d31a405fd3bb4ad141307618af3cabdR847

For summarizing locally this should be possible as well, but you need to change the plumbing that is currently responsible for the "for_federation" boolean flag.

So in general the change to implement that extra field is trivial, but currently not implemented.

[richvdh]

So in general the change to implement that extra field is trivial, but currently not implemented.

Hrm. I have to say, the whole "this is certain to be trivial to implement" argument makes me nervous. We've all been bitten by things which we think are going to be trivial but then turn out not to be.

I'd feel much more comfortable about this if either, we removed this from the MSC, or we made the implementation match the MSC. If it's really trivial, the latter shouldn't be a blocker?

[deepbluev7]

Here is an implementation for the missing parts:

diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py
index 720459f1e7..5bec1235e6 100644
--- a/synapse/handlers/room_summary.py
+++ b/synapse/handlers/room_summary.py
@@ -308,7 +308,7 @@ class RoomSummaryHandler:
             # inaccessible to the requesting user.
             if room_entry:
                 # Add the room (including the stripped m.space.child events).
-                rooms_result.append(room_entry.as_json(for_client=True))
+                rooms_result.append(room_entry.as_json())

                 # If this room is not at the max-depth, check if there are any
                 # children to process.
@@ -445,7 +445,7 @@ class RoomSummaryHandler:
         if not await self._is_local_room_accessible(room_id, requester, origin):
             return None

-        room_entry = await self._build_room_entry(room_id, for_federation=bool(origin))
+        room_entry = await self._build_room_entry(room_id)

         # If the room is not a space return just the room information.
         if room_entry.get("room_type") != RoomTypes.SPACE or not include_children:
@@ -701,14 +701,12 @@ class RoomSummaryHandler:
         # pending invite, etc.
         return await self._is_local_room_accessible(room_id, requester)

-    async def _build_room_entry(self, room_id: str, for_federation: bool) -> JsonDict:
+    async def _build_room_entry(self, room_id: str) -> JsonDict:
         """
         Generate en entry summarising a single room.

         Args:
             room_id: The room ID to summarize.
-            for_federation: True if this is a summary requested over federation
-                (which includes additional fields).

         Returns:
             The JSON dictionary for the room.
@@ -739,9 +737,6 @@ class RoomSummaryHandler:
             entry["im.nheko.summary.version"] = stats.version
             entry["im.nheko.summary.encryption"] = stats.encryption

-        # Federation requests need to provide additional information so the
-        # requested server is able to filter the response appropriately.
-        if for_federation:
             current_state_ids = (
                 await self._storage_controllers.state.get_current_state_ids(room_id)
             )
@@ -866,7 +861,6 @@ class RoomSummaryHandler:
                 raise NotFoundError("Room not found or is not accessible")

             room = dict(room_entry.room)
-            room.pop("allowed_room_ids", None)

             # If there was a requester, add their membership.
             # We keep the membership in the local membership table unless the
@@ -909,25 +903,16 @@ class _RoomEntry:
     # This may not include all children.
     children_state_events: Sequence[JsonDict] = ()

-    def as_json(self, for_client: bool = False) -> JsonDict:
+    def as_json(self) -> JsonDict:
         """
         Returns a JSON dictionary suitable for the room hierarchy endpoint.

         It returns the room summary including the stripped m.space.child events
         as a sub-key.

-        Args:
-            for_client: If true, any server-server only fields are stripped from
-                the result.
-
         """
         result = dict(self.room)

-        # Before returning to the client, remove the allowed_room_ids key, if it
-        # exists.
-        if for_client:
-            result.pop("allowed_room_ids", False)
-
         result["children_state"] = self.children_state_events
         return result

You can also find it here: deepbluev7/synapse@37f4253

The feature flags aren't quite correct, but I would say that doesn't matter, as it does the right thing when enabled :)

[clokep]

Why can't this be done today using the /hierarchy endpoint?

[deepbluev7]

That is explained in the paragraph after this list. Some of these things can be achieved using the /hierarchy API. But this specific case for example works well if you want to show the whole space. It doesn't work if you want to show a single room in a space. The hierarchy API requires you to load possibly the whole hierarchy to show a specific room in the space. This falls under "heavier than necessary".

[clokep]

But if you're showing the space don't you already have the info to show the room in the room list? I just be misunderstanding something.

[clokep]

This MSC doesn't use stripped state -- it uses a bunch of information derived from the room state.

[deepbluev7]

That sounds like a technicality to me? This MSC uses information, that is available event for rooms where only the events in the stripped state you linked to is available. Yes, this information is also available in the room state. But the stripped state of a room is also derived from the room state. The big difference between normal room state and the stripped state is:

• only the sender, type, state_key and content properties of an event
• it isn't verified via the normal event signatures
• it usually only includes a few specific events: create, name, avatar, topic, join_rules, canonical_alias and encryption

This MSC specifically derives from this sentence of the spec:

Stripped state typically appears in invites, knocks, and in other places where a user could join the room under the conditions available (such as a restricted room).

The sentence you commented on points out that relationship: These previews also work for unjoined rooms, where the full room state might be unavailable.

So could you clarify what your comment is supposed to mean? Am I supposed to change something in the text? Is it a question? Or is it just about the technicality, that this MSC doesn't directly return stripped state events, even though it uses the definition of stripped state events to define what information is available and what rooms should be previewable, without anything for me to act on? I'm a bit at a loss here, how I am supposed to resolve or reply to comments like these.

[clokep]

I found this sentence to imply that the stripped state was returned, which it is not. (Maybe it is a technicality, but this is a technical document -- details are important.) perhaps saying the information is derived from the stripped state would be enough. (I'm not even really sure the stripped aspect of it is needed here?)

[clokep]

Suggested change

	the client is using lazy loading. This MSC provides similar information as
	calling `/sync`, but it uses the stripped state, which is needed to allow this
	to work for unjoined rooms and it excludes `m.heroes` as well as membership
	the client is using lazy loading. This MSC provides similar information as
	calling `/sync`, but to allow it to work for unjoined rooms it only uses information
	from the stripped state. Additionally, it excludes `m.heroes` as well as membership

Maybe that's clearer?

[ara4n]

I've given this a green tick in order not to block its progress, given the MSC as a whole is definitely useful and very proven at this point. @deepbluev7 can you please deal with the remaining open PR comments though (e.g. spelling out what 'public' means; etc)?