-
Notifications
You must be signed in to change notification settings - Fork 396
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
MSC4258: Federated User Directory #4258
base: main
Are you sure you want to change the base?
Conversation
224fe5c
to
f4ee6bc
Compare
Co-authored-by: Maghen Calinghee <[email protected]> Co-authored-by: Olivier Delcroix <[email protected]> Co-authored-by: Yoan Pintas <[email protected]> Co-authored-by: Nicolas Buquet <[email protected]>
We are planning to implement the MSC in the coming months, however we would happily take early feedback in the meantime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation requirements:
- Client
- Server
```json | ||
{ | ||
"limit": 10, | ||
"search_term": "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there guidance on how the search term is used? Is it the same as the current API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current spec is telling to search the Matrix ID and the display name and for now this proposal doesn't change that.
Should we change that to also search all profile fields instead ? I am not sure, opinions welcome here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the user id search term contains (part of) a domain name, we can probably stop asking random servers? Such as search term "@Steve:matri" probably does not need to query the etke.cc server for users? I am just a little worried about the performance impact of these searches. (although most search will probably NOT contain domain parts. Not sure really.
We first propose a new federation endpoint similar to the [current client API](https://spec.matrix.org/v1.12/client-server-api#post_matrixclientv3user_directorysearch). | ||
It would be authenticated and rate limited. | ||
|
||
#### `POST /_matrix/federation/v3/user_directory/search` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the valid error conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging on to this, the profile federation API has a 403 to let server admins deny profile look-up. This might be good to have on the user directory API as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see several cases:
- an empty list should be returned if all matching users has visibility settings that would hide them.
- federated user directory is fully disabled on the server. 404 could be used here.
- federated user directory is restricted to a set of allowed servers for example. We should probably use 403 then.
|
||
All profile fields (cf [MSC4133](https://github.com/matrix-org/matrix-spec-proposals/pull/4133)) should be returned here. | ||
|
||
When an user calls the client user search API, the server should send a federated user search request to all known servers. It would then receive the results and return them to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds really really expensive for the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could benefit from #4259 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed MSC #4259 could/should be mentioned as a possibility to build upon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds really really expensive for the server.
We should probably relax the requirement to include all known servers, so that implementations can optimize that if needed. to all known servers or a subset of it
for example ?
I am not sure the impact will be that heavy however : typing in a room with almost all servers like Matrix HQ
will similarly broadcast events to all of those.
This could benefit from #4259 🙂
Indeed MSC #4259 could/should be mentioned as a possibility to build upon?
Could you elaborate what you both have in mind ? for now it can help a bit to receive profile updates if we have a local search cache, but I don't really see something else.
- `restricted`: visible to any user sharing a room with | ||
- `remote` (or federated or public ?): visible to users on local and remote homeservers | ||
|
||
If no value is provided (or it is null), the user hasn't set a preference and the server should follow the current expected behavior (visible if sharing a room in common or in public room). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"visible if sharing a room in common or in public room" is actually only the minimum requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal would change that, and user defined visibility would prevail, as expected by the users :)
- introduce `search_scope` in the client API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it is such a hard problem (mainly performance and privacy). There is a reason why there is no federated email directory....
A few minor comments inline.
@@ -0,0 +1,162 @@ | |||
# MSC4258: Federated User Directory | |||
|
|||
Currently user search can only be done locally, which would at best get a list of all users known to the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I know what is meant, but the grammar of the second half of that sentence seems weird or at least difficult to parse.
|
||
The federation search endpoint should be rate limited. | ||
|
||
We recommend to not answer for `search_term` with less than 3 characters like "a" or "at". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so @A:matrix.org could never be found?
```json | ||
{ | ||
"limit": 10, | ||
"search_term": "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the user id search term contains (part of) a domain name, we can probably stop asking random servers? Such as search term "@Steve:matri" probably does not need to query the etke.cc server for users? I am just a little worried about the performance impact of these searches. (although most search will probably NOT contain domain parts. Not sure really.
@@ -0,0 +1,162 @@ | |||
# MSC4258: Federated User Directory | |||
|
|||
Currently user search can only be done locally, which would at best get a list of all users known to the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently user search can only be done locally, which would at best get a list of all users known to the server. | |
Currently user search can only be done locally, which lists - at maximum - all users known to the user's server. |
Rendered
This proposal has been thought and written by me and people listed below, all employed by the French state for the Tchap project.
@mcalinghee @odelcroi @yostyle @NicolasBuquet