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 wording of users to include in the user directory search #1386

Open
HarHarLinks opened this issue Dec 22, 2022 · 6 comments
Open

Clarify wording of users to include in the user directory search #1386

HarHarLinks opened this issue Dec 22, 2022 · 6 comments
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@HarHarLinks
Copy link
Contributor

HarHarLinks commented Dec 22, 2022

Link to problem area:
https://spec.matrix.org/v1.5/client-server-api/#post_matrixclientv3user_directorysearch

Issue

the homeserver MUST at a minimum consider the users the requesting user shares a room with and those who reside in public rooms (known to the homeserver). The search MUST consider local users to the homeserver, and SHOULD query remote users as part of the search.

Assuming the intersection of these 2 sentences is the correct meaning, I get:
image

This appears to meet what the synapse docs say.

The alternative interpretation would be the conjunction of both sentences, i.e. must show all (local and remote) users in shared and public rooms AND all other local users and should further include remote users. This would appear to match enabling the synapse user_directory.search_all_users configuration option.

related
#1022

@HarHarLinks HarHarLinks added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label Dec 22, 2022
@richvdh
Copy link
Member

richvdh commented Dec 22, 2022

For the record, this text was added in matrix-org/matrix-spec-proposals#1569.

@richvdh
Copy link
Member

richvdh commented Jan 3, 2023

I find your question rather hard to parse, and I'm not sure that either interpretation is completely correct (your venn diagram does not appear to consider users who neither share a room with the calling user nor are members of a public room).

It's worth noting that, per matrix-org/matrix-spec-proposals#1569, this is deliberately left somewhat vague to allow local administrators or HS developers to make their own decisions here.

Still, as far as I can tell, there are six sets of users:

  • Local users who share a room with the caller. MUST be returned.
  • Local users who do not share a room with the caller, but are in a public room. MUST be returned.
  • Local users who do not share a room with the caller, and are not in any (visible) public rooms. Unspecified. (Synapse's behaviour is governed by search_all_users.)
  • Remote users who share a room with the caller. SHOULD be returned. I think? Synapse does so, in any case.
  • Remote users who do not share a room with the caller, but are in a public room where the homeserver is otherwise active (ie, there are other users on the same HS as the caller who are members of that room). SHOULD be returned, I think? Again, Synapse does so.
  • Remote users who do not share a room with the caller, and are not in any (visible) public rooms. Unspecified, but returning such users would be very difficult since we know nothing about them.

@turt2live as the original author of the sentence "The search MUST consider local users to the homeserver, and SHOULD query remote users as part of the search," can you clarify the intention here? Why are remote users only a SHOULD while local users are a MUST? What does it mean to "query" remote users? Note that Synapse never performs any federation queries as part of user-directory search.

@turt2live
Copy link
Member

2018 was a bit too long ago, unfortunately. I believe the intention was to try and run the feature through an MSC-like process before MSCs were a thing, meaning it's closer to idealism than reality (as somewhat implied by Synapse not querying profiles over federation).

@richvdh
Copy link
Member

richvdh commented Jan 3, 2023

I feel like we should probably:

  • remove that sentence which distinguishes between local and remote users
  • relax the "MUST at a minimum consider the users... " sentence to a recommendation

@HarHarLinks
Copy link
Contributor Author

I'm sorry, it is a hard question to phrase, at least to me. :)


this is deliberately left somewhat vague to allow local administrators or HS developers to make their own decisions here.

Are you referring to the following?

The text is intentionally left vague for whether or not the API is local users only to give the homeserver some flexibility in this area.

I don't feel this is particularly vague: spec pretty clearly states MUST and SHOULD, without leaving this point open imo.


your venn diagram does not appear to consider users who neither share a room with the calling user nor are members of a public room

Those would be part of the very left (local) very right (remote) rounded rectangles respectively, where they don't intersect with neither public nor shared room circles.


Remote users who do not share a room with the caller, and are not in any (visible) public rooms. Unspecified, but returning such users would be very difficult since we know nothing about them.

It is relatively clear to me, although you might even go so far as to spell it out, that for all of this, we are regarding at most the users the querying user's hs knows about, i.e. any remote users it finds can only be those who share a room with at least any one local ("same hs") user.


The alternative interpretation in bulletpoints, which however I think is incorrect and perhaps is due to a language barrier:

  • all local users MUST, regardless of shared/public room membership
  • all users sharing a room with me MUST, regardless whether they're local/remote
  • all user in public rooms MUST, regardless whether they're local/remote
  • any leftover remote users SHOULD

Basically I would be happy if you'd replace the 2 sentences quoted in the OP were replaced by your bullet point list.

@Johennes
Copy link
Contributor

Johennes commented Aug 7, 2024

For cross-reference: matrix-org/matrix-spec-proposals#4170 is trying to apply the same room membership requirement to the profile APIs.

  • Remote users who share a room with the caller. SHOULD be returned. I think? Synapse does so, in any case.
  • Remote users who do not share a room with the caller, but are in a public room where the homeserver is otherwise active (ie, there are other users on the same HS as the caller who are members of that room). SHOULD be returned, I think? Again, Synapse does so.

I wonder if these shouldn't be a MUST, too. Room membership should already provide the information that is returned by the user directory API. So it's a bit odd that you may not be able to find somebody you already share a room with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

4 participants