-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC2753: Peeking via sync (take 2) #2753
Open
ara4n
wants to merge
21
commits into
old_master
Choose a base branch
from
matthew/msc2753
base: old_master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
52d8d77
MSC2753: Peeking via sync (take 2)
ara4n 2f88343
fix MSC refs
ara4n 4aba8c8
fix MSC refs again
ara4n 752d569
fix md
ara4n 74cc1c3
s/peeking/peek/ and s/joined/join/
ara4n c4c4b35
spell out that joins should automatically cancel peeks, and that clie…
ara4n 4fba927
incorporate review
ara4n a070935
spell out that we should flash unpeeks in leave to let the client kno…
ara4n acec300
format and edit
richvdh a255140
add information about responses
richvdh 558f66f
remove duplicated info
richvdh 4dea820
add unstable prefix
richvdh 8b08c09
more updates
richvdh b2cb3f2
clarify interaction with e2ee
ara4n a80364d
minor updates
richvdh 90eba26
initialsync does not clear peeks
richvdh 3b629ba
lots of updates
richvdh 405c05d
ore updates
richvdh 12740e5
unpeeked rooms go in the leave section of /sync
richvdh c757849
Drop `_room` suffix
richvdh 50537aa
for /peek, put room id in the uri
richvdh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
# MSC2753: Peeking via Sync (Take 2) | ||
|
||
## Problem | ||
|
||
Peeking into rooms without joining them currently relies on the deprecated v1 /initialSync and /events APIs. | ||
|
||
This poses the following issues: | ||
|
||
* Servers and clients must implement two separate sets of event-syncing logic, doubling complexity. | ||
* Peeking a room involves setting a stream of long-lived /events requests | ||
going. Having multiple streams is racey, competes for resources with the | ||
/sync stream, and doesn't scale given each room requires a new /events | ||
stream. | ||
* v1 APIs are deprecated and not implemented on new servers. | ||
|
||
This MSC likely obsoletes [MSC1776](https://github.com/matrix-org/matrix-doc/pull/1776). | ||
|
||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Proposal | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
We add an CS API called `/peek/{roomIdOrAlias}`, very similar to `/join/{roomIdOrAlias}`. | ||
|
||
Calling `/peek`: | ||
* Resolves the given room alias to a room ID, if needed. | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Adds the room (if permissions allow) to a new section of the /sync response | ||
called `peek` - but only for the device which called `/peek`. | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* The `peek` section is identical to `join`, but shows the live activity of | ||
rooms for which that device is peeking. | ||
|
||
The API returns 404 on an unrecognised room ID or alias, or 403 if the room | ||
does not allow peeking. Rooms allow peeking if they have `history_visibility` | ||
of `world_readable`. N.B. `join_rules` do not affect peekability - it's | ||
possible to have an invite-only room which joe public can still peek into, if | ||
`history_visibility` has been set to `world_readable`. | ||
|
||
If the `history_visibility` of a room changes to not be `world_readable`, any | ||
peeks on the room are cancelled. | ||
|
||
In order to clear up stale peeks if a client restarts without cleaning up | ||
nicely, the act of calling `/sync` without a `since` token must cancel any peeks | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
created by that device. | ||
|
||
Similar to `/join`, `/peek` lets you specify `server_name` querystring | ||
parameters to specify which server(s) to try to peek into the room via (when | ||
coupled with [MSC2444](https://github.com/matrix-org/matrix-doc/pull/2444)). | ||
|
||
If a user subsequently `/join`s the room they're peeking, we atomically move | ||
the room to the `join` block of the `/sync` response, allowing the client to | ||
build on the state and history it has already received without re-sending it | ||
down `/sync`. | ||
|
||
To stop peeking, the user calls `/unpeek` on the room, similar to `/leave` or | ||
`/forget`. This returns 200 on success, 404 on unrecognised ID, or 400 if the | ||
room was not being peeked in the first place. Having stopped peeking, the | ||
empty room block appears in the `leave` block of the next sync response to tell | ||
the client that the user is no longer peeking. | ||
|
||
The new `/peek` and `/unpeek` endpoints require authentication and can be | ||
ratelimited. Their responses are analogous to their `/join` and `/leave` | ||
counterparts (eg: `room_id` in the response and empty object when stopping). | ||
|
||
The act of joining a peeked room automatically cancels any ongoing `/peeks` by | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
that user. | ||
|
||
Clients should check for any irrelevant peeked rooms on launch (left over from | ||
previous instances of the app) and explicitly `/unpeek` them to conserve | ||
resources. | ||
|
||
|
||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Potential issues | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
It could be seen as controversial to add another new block to the `/sync` | ||
response. We could use the existing `join` block, but: | ||
|
||
* it's a misnomer (given the user hasn't joined the rooms) | ||
* `join` refers to rooms which the *user* is in, rather than that they are | ||
peeking into using a given *device* | ||
* we risk breaking clients who aren't aware of the new style of peeking. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. surely not given that it would still be per-device and those clients wouldn't be calling the new |
||
* there's already a precedent for per-device blocks in the sync response (for | ||
to-device messages) | ||
|
||
It could be seen as controversial to make peeking a per-device rather than | ||
per-user feature. When thinking through use cases for peeking, however: | ||
|
||
1. Peeking a chatroom before joining it is the common case, and is definitely | ||
per-device - you would not expect peeked rooms to randomly pop up on other | ||
devices, or consume their bandwidth. | ||
2. [MSC1769](https://github.com/matrix-org/matrix-doc/pull/1769) (Profiles as | ||
rooms) is also per device: if a given client wants to look at the Member | ||
Info for a given user in a room, it shouldn't pollute the others with that | ||
data. | ||
3. [MSC1772](https://github.com/matrix-org/matrix-doc/pull/1772) (Groups as | ||
rooms) uses room joins to indicate your own membership, and peeks to query | ||
the group membership of other users. Similarly to profiles, it's not clear | ||
that this should be per-user rather than per-device (and worse case, it's a | ||
matter of effectively opting in rather than trying to filter out peeks you | ||
don't care about). | ||
|
||
## Alternatives | ||
|
||
[MSC1776](https://github.com/matrix-org/matrix-doc/pull/1776) defined an | ||
alternative approach, where you could use filters to add peeked rooms into a | ||
given `/sync` response as needed. This however had some issues: | ||
|
||
* You can't specify what servers to peek remote rooms via. | ||
* You can't identify rooms via alias, only ID | ||
* It feels hacky to bodge peeked rooms into the `join` block of a given | ||
`/sync` response | ||
* Fiddling around with custom filters feels clunky relative to just calling a | ||
`/peek` endpoint similar to `/join`. | ||
|
||
While experimenting with implementing MSC1776, I came up with this as an | ||
alternative that empirically feels much simpler and tidier. | ||
|
||
## Security considerations | ||
|
||
Servers should ratelimit calls to `/peek` to stop someone DoSing the server. | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Servers may stop maintaining a `/peek` if its device has not `/sync`ed | ||
recently, and thus reclaim resources. At the next `/sync` the server would | ||
need to restore the `peek` and provide a gappy update. This is most relevant | ||
in the context of peeking into a remote room via | ||
[MSC2444](https://github.com/matrix-org/matrix-doc/pull/2444) however. | ||
|
||
## Unstable prefix | ||
|
||
TBD |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Something this should also handle is supporting /event and /context into peeked rooms.
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.
Shouldn't that be accepted/handled automatically as long as a peek is active?