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

Clarify the meaning of "public rooms" in the room directory #2104

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Mar 19, 2025

Relates to: #633

Pull Request Checklist

Preview: https://pr2104--matrix-spec-previews.netlify.app

Comment on lines -33 to -36
The room's join rule. When not present, the room is assumed to
be `public`. Note that rooms with `invite` join rules are not
expected here, but rooms with `knock` rules are given their
near-public nature.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came in through fa6cc8a but I cannot find any references to it in matrix-org/matrix-spec-proposals#2403. I think it is trying to say that knockable rooms can be made visible in the directory? If so, I think this should rather be moved to where the meaning of visibility is described because here it makes it seem as if servers should do this automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that it's more a clarification about which join rules clients can expect to find here.

I agree that this should be next to the meaning of visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added a phrase about knockable rooms in the warning box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anoadragon453 says:

@Johennes Synapse considers rooms with knock and knock_restricted join rules part of its /publicRooms response. This stems from MSC2403, which specifically added rooms with the knock join rule to /publicRooms, so that users could find rooms to knock on.

Does this change the definition of a "public room" to one that has the join_rule public, knock or knock_restricted?

Copy link
Contributor Author

@Johennes Johennes Mar 20, 2025

Choose a reason for hiding this comment

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

Thanks for the context @anoadragon453! 🙏

So when responding to /publicRooms, Synapse first retrieves all rooms with public visibility but then applies an additional filter to only include these cases:

join_rules = '{JoinRules.PUBLIC}'
OR join_rules = '{JoinRules.KNOCK}'
OR join_rules = '{JoinRules.KNOCK_RESTRICTED}'
OR history_visibility = 'world_readable'

I'm struggling a bit with understanding how the room directory is meant to work. Other than the slightly unclear statement "rooms with knock rules are given their near-public nature", the spec doesn't mention any restrictions on what rooms can be included in /publicRooms. Therefore, my assumption was that rooms with a public visibility would show up regardless of their join rules or history visibility.

In fact, MSC2403 seems to only define join_rule as a new field in the response of /publicRooms. It doesn't appear to specify any filtering and instead says:

The spec does not prevent us from adding rooms with 'knock' join_rules to the public rooms directory.

That matches my assumption but not what Synapse actually does.

😵‍💫

Copy link
Member

Choose a reason for hiding this comment

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

Correct, the room is only considered if visibility is public when modifying the public rooms list with PUT /_matrix/client/v3/directory/list/room/{roomId}.

To speak to Synapse's implementation, the join_rule restriction within the query was only added during a performance improvement change. I could see this requirement being useful, in that if a room is set to private, it would immediately no longer show in the public rooms list (as it is not publicly-joinable). While the spec may not require homeservers to manage their lists in that way, Synapse can choose to do so.

Regardless, it's clear that Synapse's filtering of its public rooms list here should not influence the spec's definition of a public room. So I think we can ignore this and consider a join_rule of public the only variant that constitutes a room as public.

Copy link
Contributor Author

@Johennes Johennes Mar 21, 2025

Choose a reason for hiding this comment

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

To speak to Synapse's implementation, the join_rule restriction within the query was only added during a performance improvement change. I could see this requirement being useful, in that if a room is set to private, it would immediately no longer show in the public rooms list (as it is not publicly-joinable). While the spec may not require homeservers to manage their lists in that way, Synapse can choose to do so.

Oh, interesting. This sounds sensible. Maybe we could add this additional filtering as a MAY hint into the spec?

So I think we can ignore this and consider a join_rule of public the only variant that constitutes a room as public.

You mean visibility of public?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, interesting. This sounds sensible. Maybe we could add this additional filtering as a MAY hint into the spec?

Sounds reasonable to me.

You mean visibility of public?

Hah, yes sorry. join_rules aren't mandated affect the filtering. Terminology!

Comment on lines -77 to -79

Note that this endpoint receives and returns the same format that is seen
in the Client-Server API's `POST /publicRooms` endpoint.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This felt slightly confusing because the CS endpoint has a server parameter that is missing here. I think the identity of the request and response bodies is clear enough without this note.

Comment on lines -156 to -157
required:
- chunk
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this is the only difference to public_rooms_response.yaml. I cannot imagine that chunk was supposed to be required on only one of the four endpoints so I have made this reuse the shared schema.

@Johennes Johennes marked this pull request as ready for review March 19, 2025 14:00
@Johennes Johennes requested a review from a team as a code owner March 19, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants