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

MSC2666: Get rooms in common with another user #2666

Open
wants to merge 29 commits into
base: old_master
Choose a base branch
from
Open
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c61790e
MSC 2664: Get rooms in common with another user
Half-Shot Jul 5, 2020
4264f32
correct MSC number
Half-Shot Jul 5, 2020
008951f
Try to clarify Proposal, update response format
Half-Shot Jul 5, 2020
29f02ed
Update MSC number in prefix
Half-Shot Jul 5, 2020
2b75da8
Wording tidyup
Half-Shot Jul 5, 2020
630af1c
more tidyup
Half-Shot Jul 5, 2020
d885bcf
Clarify empty responses
Half-Shot Jul 5, 2020
5254076
uhoreg fixes my spelling
Half-Shot Jul 5, 2020
3f2faef
add same user case
Half-Shot Jul 6, 2020
db99583
Update to use /uk.half-shot.msc2666/
Half-Shot Jul 10, 2020
10a2df2
Fixes
Half-Shot Jul 20, 2020
d3b17e6
Merge branch 'hs/shared-rooms' of github.com:matrix-org/matrix-doc in…
Half-Shot Jul 20, 2020
4ac7ce8
remove referneces to user_id
Half-Shot Jul 20, 2020
a4f5bae
another reference to the auth user id
Half-Shot Jul 20, 2020
c453704
consistent newlines
Half-Shot Jul 20, 2020
cd173d5
Add errcode
Half-Shot Aug 18, 2020
1a389f9
Update proposals/2666-get-rooms-in-common.md
Half-Shot Aug 18, 2020
fbbb2d9
typo fix and shared_rooms -> mutual_rooms (#3631)
ShadowJonathan Jan 9, 2022
591d3e5
Apply suggestions from code review
Half-Shot Apr 12, 2022
a1de65f
@MSC2666: Add "may return 400" qualifier (#3770)
ShadowJonathan Apr 13, 2022
d59d051
Reject r0, embrace v1
Half-Shot Apr 15, 2022
6a4e523
Remove M_UNKNOWN response (#3822)
ShadowJonathan May 31, 2022
b946cc3
Update an old link to the spec; fix endpoint text
anoadragon453 Aug 2, 2022
ea49670
provide a link to lazy-loading of room members spec
anoadragon453 Aug 2, 2022
6f4f01b
Apply review feedback (#3913)
ShadowJonathan Jan 31, 2023
60ae94f
Add invalid batch_token error code (#4017)
ShadowJonathan May 18, 2023
7829c3b
Update proposals/2666-get-rooms-in-common.md
anoadragon453 May 19, 2023
92aef5b
restrict the allowed characters for the next_batch_token
anoadragon453 Jun 5, 2023
d58d0a1
apply review feedback (#4035)
ShadowJonathan Jul 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions proposals/2666-get-rooms-in-common.md
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
# MSC 2666: Get rooms in common with another user
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

It is useful to be able to fetch rooms you have in common with another user. Popular messaging
services such as Telegram offer users the ability to show "groups in common", which allows users to
determine what they have in common before participating in conversation.

There are a variety of applications for this information. Some users may want to block invites from
users they do not share a room with at the client level, and need a way to poll the homeserver for
this information. Another use case would be trying to determine how a user came across your MXID, as
invites on their own do not present much context. With this endpoint, a client could tell you what
rooms you have in common before you accept an invite.

While this information can be determined if the user has full access to member state for all rooms,
modern clients often implement
[lazy-loading of room members](https://spec.matrix.org/v1.3/client-server-api/#lazy-loading-room-members),
so they often only have a subset of state for the rooms the user is in. Therefore, the homeserver
should have a means to provide this information.

This proposal aims to implement a simple mechanism to fetch rooms you have in common with another
user.

## Proposal

Homeservers will implement a new endpoint `/_matrix/client/v1/user/mutual_rooms`.

This endpoint will take a query parameter of `user_id` which will contain the MXID of the user
matched against.
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

If user_id can't be specified multiple times, it might belong in the HTTP path instead. Also, can it be specified multiple times?

Copy link
Member

Choose a reason for hiding this comment

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

ah, it's listed later in the MSC. Is there a technical reason preventing multiple users from being searched? It feels like an awkward thing to make a future MSC for when we're already here.

Copy link
Contributor

@ShadowJonathan ShadowJonathan Jun 6, 2023

Choose a reason for hiding this comment

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

The discussion context is here: #2666 (comment)

The convention of putting user IDs in path parameters was originally intended to allow AS' to insert puppeted Matrix User ID. But that ended up being scrapped for the user_id query parameter. As a result, ideally we'd remove about half of the instances of user IDs in path parameters; thus it may not be the best precedence to adhere to.

Copy link
Contributor

@ShadowJonathan ShadowJonathan Jun 6, 2023

Choose a reason for hiding this comment

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

There is no technical reason preventing multiple users for being searched, but I don't see a convincing client usecase, while I agreed to keep the path open for one if the time came, to make it easily extendible.


In the past, this MSC had path-element handling, but I got convinced that that was a leftover pattern of an older way of handling things, and doing things via query was a newer way, so I applied that.

Copy link
Member

Choose a reason for hiding this comment

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

I think if we're not putting the user id in the path, then we shouldn't use the /_matrix/client/v1/user hierarchy, since most of the endpoints under that hierarchy do have a user_id as the next path component.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also realising that if we want to be able to get rooms in common with more users, there could just be another version of this endpoint.

Copy link
Member

@KitsuneRal KitsuneRal Jun 9, 2023

Choose a reason for hiding this comment

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

With the current MSC text (see "Forward-compatibility considerations"), this seems to be resolved?

Copy link
Contributor

Choose a reason for hiding this comment

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

I want some clear direction on this. The current version (that I merge in this branch via #4035) changes some labels around enough that i'd have to give it another unstable_features tag.

I haven't yet, as i want to sweep the result of this in that tag, before i propose it FCP again.


Should I change this back to a path component, or should I keep this as a query element endpoint? I can do either.


This endpoint can be rate limited.
Copy link
Member

Choose a reason for hiding this comment

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

boilerplate:

Suggested change
This endpoint can be rate limited.
This endpoint can be rate limited and requires authentication.

Copy link
Contributor

Choose a reason for hiding this comment

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

/res #4035


The response format will be an array containing all rooms where both the authenticated user and
`user_id` have a membership of type `join`. If the `user_id` does not exist, or does not share any
rooms with the authenticated user, an empty array should be returned.
Copy link
Member

Choose a reason for hiding this comment

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

is the behaviour on invalid user IDs left as an implementation decision? (I think that's fine if so, but best to say so explicitly)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC that would fall under some 400 Bad Request semantics, implementation-specific, yeah.


```http
GET /_matrix/client/v1/user/mutual_rooms/?user_id=%40bob%3Aexample.com
Copy link
Member

Choose a reason for hiding this comment

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

this example includes a trailing /, which is inconsistent with the definition above.

In general Matrix does not allow extra /s in endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's an error from a previous change that moved path-element parameters to query parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

/res #4035

```

```json
{
"joined": [
"!OGEhHVWSdvArJzumhm:matrix.org",
turt2live marked this conversation as resolved.
Show resolved Hide resolved
"!HYlSnuBHTxUPgyZPKC:half-shot.uk",
"!DueayyFpVTeVOQiYjR:example.com"
]
}
```

anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
The server may decide that the response to this endpoint is too large, and thus an optional key
`"next_batch_token"` can be inserted, which the client has to pass to `batch_token` in the query
clokep marked this conversation as resolved.
Show resolved Hide resolved
parameters together with the original `user_id` to fetch the next batch of responses. This will
continue until the server does no longer insert `"next_batch_token"`.
Copy link
Member

Choose a reason for hiding this comment

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

Somewhere we need the words "this batch_token is only valid for use with this endpoint", and the semantics for expiration (does the homeserver need to remember the batch token forever?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an expiration time of 10 minutes could be reasonable here.

Copy link
Member

Choose a reason for hiding this comment

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

In which case we should specify an error case for clients supplying an unknown / expired batch token, so that they can retry.

We can't use M_UNKNOWN_TOKEN - as this error would also be returned if the user used an unknown access/refresh token to access this endpoint.

We may need to define a new errcode - perhaps M_UNKNOWN_PAGINATION_TOKEN?

Copy link
Contributor

Choose a reason for hiding this comment

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

#4035

I have added the 10 minute timer, and added text that it will handle expired tokens the same as invalid tokens, so that implementations may forget those tokens entirely. Do you think this is a good method?

Then the clients' default response to "invalid token" is just to start over, without any token.

The only problem i see is that clients may loop on a broken implementation, where it is not passing the right token back to the server will cause it to get stuck in its mechanism where it continually retries.

Copy link
Member

Choose a reason for hiding this comment

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

I would just leave the validity period up to the implementation, but recommend at least 10 minutes. Clients should be prepared to handle tokens that do have a ridiculously short window though.

I would also recommend clients limit the maximum number of retries before giving up, precisely to prevent infinite loops as you describe.


```json5
{
"joined": [
// ...
],
"next_batch_token": "<a string up to 32 characters abiding by the regex /[a-zA-Z0-9]+/>"
turt2live marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Per this comment

Suggested change
"next_batch_token": "<a string up to 32 characters abiding by the regex /[a-zA-Z0-9]+/>"
"next_batch_token": "<a string up to 32 characters abiding by the regex /[a-zA-Z0-9_-]+/>"

Copy link
Member

Choose a reason for hiding this comment

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

can we just use https://github.com/matrix-org/matrix-spec-proposals/blob/rav/proposals/id_grammar/proposals/1597-id-grammar.md#opaque-ids please?

Ideally by name: "a batch_token is an opaque identifier, currently capable of containing the characters [0-9a-zA-Z._~-], must be at most 255 characters, and non-empty"

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I was looking for something like that, will add it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

/res #4035

}
```

The response error for when the given `batch_token` is invalid will be a response with HTTP code 400,
with `M_INVALID_PARAM` as `errcode`.

The response error for trying to get shared rooms with yourself will be an HTTP code 422, with
`M_INVALID_PARAM` as the `errcode`.
Copy link
Member

Choose a reason for hiding this comment

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

Not a strong opinion here, but 422 is pretty obscure (and we don't use it anywhere else in matrix, afaik).

The idea in Matrix is that we shouldn't rely too heavily on HTTP response codes, to make it easier (one day) to move to alternative transports. So if we need to distinguish this from other error cases, we'd do that via a custom errcode rather than HTTP response code.

Copy link
Member

@KitsuneRal KitsuneRal Jun 9, 2023

Choose a reason for hiding this comment

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

Ftr, CoAP doesn't have code 4.22 or anything semantically similar to HTTP's 422, and there don't seem to be subsequent RFCs addressing that (while 429, e.g., has got its CoAP equivalent in a later RFC).

Copy link
Member

Choose a reason for hiding this comment

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

Doing this sounds like programmer error vs. something that would actually need to be explicitly handled in practice. So this could just be a 400 / M_UNKNOWN - "you cannot request rooms in common with yourself".

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed the error to 400 + M_UNKNOWN

/res #4035


## Potential issues

Homeserver performance and storage may be impacted by this endpoint. While a homeserver already
stores membership information for each of its users, the information may not be stored in a way that
is readily accessible. Homeservers that have implemented
[POST /user_directory/search](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3user_directorysearch)
may have started some of this work, if they are limiting users to searching for users for which they
share rooms. While this is not a given by any means, it may mean that implementations of this API
and /search may be complimentary.

## Alternatives
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

A client which holds full and current state can already see all membership for all rooms, and thus
determine which of those rooms contains a "join" membership for the given user_id. Clients which "lazy-load"
however do not have this information, as they will have only synced a subset of the full membership for
all rooms. While a client *could* pull all membership for all rooms at the point of needing this information,
it's computationally expensive for both the homeserver and the client, as well as a bandwidth waste for constrained
clients.

## Forward-compatibility considerations

There possibly comes a time where it's desirable to query mutual rooms for more-than-one other user,
where multiple people (including the self-user) are matched against for which rooms all of them
share.

Because of that, the endpoint accepts a query parameter, however, it will only accept *one* query
parameter for the time being. In the future this restriction can be lifted to accept multiple query
parameters under `user_id`

## Security considerations

The information provided in this endpoint is already accessible to the client if it has a copy of all
state that the user can see. This endpoint only makes it possible to get this information without having
to request all state ahead of time.

## Unstable prefix

The implementation MUST use `/_matrix/client/unstable/uk.half-shot.msc2666/user/mutual_rooms`.

The /versions endpoint MUST include a new key in `unstable_features` with the name
`uk.half-shot.msc2666.query_mutual_rooms`.

Previous iterations of this MSC has used the following `unstable_features` key(s):
- `uk.half-shot.msc2666.mutual_rooms`
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
- `uk.half-shot.msc2666`

If the value is false or the key is not present, clients MUST assume the feature is not available.

Once the MSC has been merged, and the homeserver has advertised support for the Matrix version that
this endpoint is included in, clients should use `/_matrix/client/v1/user/mutual_rooms` and will no
longer need to check for the `unstable_features` flag.
Comment on lines +127 to +129
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Once the MSC has been merged, and the homeserver has advertised support for the Matrix version that
this endpoint is included in, clients should use `/_matrix/client/v1/user/mutual_rooms` and will no
longer need to check for the `unstable_features` flag.
Once this MSC is considered stable, implementations can use the stable endpoint instead.

It may also be worth reading #4024 to match the language there, as this seems like the sort of feature a client would want to use between FCP completing and spec release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think with this original language I remember/wrote this down to "consider stable" to be "when it is included in a spec version"

Together with richvdh's comment here, i think that for this MSC it is unnecessary to introduce a .stable unstable_features flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

/res