-
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
MSC1902: Split the media repo into s2s and c2s parts #1902
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f915ff4
Proposal to split the media repo into s2s and c2s parts
turt2live 557c266
Move client media access to the client namespace
turt2live f34d176
Merge branch 'master' into travis/msc/rudimentary-media-auth
turt2live ee31e81
Update for modern standards
turt2live 1c54f6a
Add unstable prefix
turt2live 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,65 @@ | ||||||||||||||||||||||
# Splitting the media repo into a client-side and server-side component | ||||||||||||||||||||||
|
||||||||||||||||||||||
Currently Matrix relies on media being pulled from servers using the same set | ||||||||||||||||||||||
of endpoints which clients use. This has so far meant that media repository | ||||||||||||||||||||||
implementations cannot reliably enforce auth on given resources, and implementations | ||||||||||||||||||||||
are left confused/concerned about the lack of split in the endpoints. | ||||||||||||||||||||||
|
||||||||||||||||||||||
## Proposal | ||||||||||||||||||||||
|
||||||||||||||||||||||
Instead of using `/_matrix/media` for both servers and clients, servers must now use | ||||||||||||||||||||||
`/_matrix/federation/v1/remote_media` and clients must use `/_matrix/client/r0/media`. | ||||||||||||||||||||||
|
||||||||||||||||||||||
To support backwards compatibility and logical usage of the media repo, only downloads | ||||||||||||||||||||||
and thumbnails will be available over the new federation prefix. The remaining endpoints | ||||||||||||||||||||||
will only be available through the client-server API. | ||||||||||||||||||||||
|
||||||||||||||||||||||
The mapping of each endpoint would look like: | ||||||||||||||||||||||
|
||||||||||||||||||||||
| Old | Client-Server | Federation | | ||||||||||||||||||||||
|-----|---------------|------------| | ||||||||||||||||||||||
| `/_matrix/media/:version/download/:origin/:mediaId` | `/_matrix/client/r0/media/download/:origin/:mediaId` | `/_matrix/federation/v1/remote_media/download/:origin/:mediaId` | | ||||||||||||||||||||||
| `/_matrix/media/:version/thumbnail/:origin/:mediaId` | `/_matrix/client/r0/media/thumbnail/:origin/:mediaId` | `/_matrix/federation/v1/remote_media/thumbnail/:origin/:mediaId` | | ||||||||||||||||||||||
| `/_matrix/media/:version/upload` | `/_matrix/client/r0/media/upload` | N/A | | ||||||||||||||||||||||
| `/_matrix/media/:version/preview_url` | `/_matrix/client/r0/media/preview_url` | N/A | | ||||||||||||||||||||||
| `/_matrix/media/:version/config` | `/_matrix/client/r0/media/config` | N/A | | ||||||||||||||||||||||
Comment on lines
+21
to
+25
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. Since this has been last changed a while ago;
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
No schema changes are proposed to the endpoints, aside from the obvious pathing differences. | ||||||||||||||||||||||
|
||||||||||||||||||||||
### Federation endpoints | ||||||||||||||||||||||
|
||||||||||||||||||||||
Because media is not (currently) actively federated, the endpoint is named "remote_media" | ||||||||||||||||||||||
to imply it is not a resource the target server is expected to have. | ||||||||||||||||||||||
|
||||||||||||||||||||||
The federation endpoints will require standard [request authentication](https://matrix.org/docs/spec/server_server/r0.1.4#request-authentication). | ||||||||||||||||||||||
|
||||||||||||||||||||||
### Client-server endpoints | ||||||||||||||||||||||
|
||||||||||||||||||||||
With the exception of `/download` and `/thumbnail`, all media endpoints require authentication. | ||||||||||||||||||||||
We could require access tokens on media, however [MSC701](https://github.com/matrix-org/matrix-doc/issues/701) | ||||||||||||||||||||||
and similar are better positioned to handle the problem. | ||||||||||||||||||||||
|
||||||||||||||||||||||
## Changes required by known implementations | ||||||||||||||||||||||
|
||||||||||||||||||||||
Clients and servers will have to adopt the new endpoints entirely. It is expected that | ||||||||||||||||||||||
existing implementations will continue to use the legacy routes for a short time while | ||||||||||||||||||||||
these changes gain popularity in the ecosystem. | ||||||||||||||||||||||
|
||||||||||||||||||||||
In a prior version of this proposal it was suggested to use `/_matrix/media/v1` for | ||||||||||||||||||||||
server-server communication with the authentication requirements described here, however | ||||||||||||||||||||||
due to some clients still using `/_matrix/media/v1` (despite the endpoint changing to be | ||||||||||||||||||||||
`/_matrix/media/r0` some versions ago) it is not feasible to support backwards compatibility | ||||||||||||||||||||||
with the authenticated endpoints. | ||||||||||||||||||||||
|
||||||||||||||||||||||
## References | ||||||||||||||||||||||
|
||||||||||||||||||||||
* [MSC701](https://github.com/matrix-org/matrix-doc/issues/701) - Access control and | ||||||||||||||||||||||
GDPR for the media repo. | ||||||||||||||||||||||
* [matrix-doc#1341](https://github.com/matrix-org/matrix-doc/issues/1341) - Concerns | ||||||||||||||||||||||
with servers using the client-server endpoints to download media over federation. | ||||||||||||||||||||||
|
||||||||||||||||||||||
## Unstable prefix | ||||||||||||||||||||||
|
||||||||||||||||||||||
While this MSC is not in a released version of the spec, use `unstable/org.matrix.msc1902` in place | ||||||||||||||||||||||
of the version. For example, `/_matrix/client/r0/media/config` becomes | ||||||||||||||||||||||
`/_matrix/client/unstable/org.matrix.msc1902/media/config`. |
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.
I don't really understand how this supports backwards compatibility.
Is the idea that clients and requesting-servers should use the new endpoints and fall back to the old ones on 400? Or do we just have to wait for everyone to support the new endpoints before we can switch to using them?
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.
TLDR: We should fix this MSC to use fallback semantics.
This paragraph certainly mixes two different ideas in it:
/media/:version
endpoints are deprecated in favour of the new ones, but function as they do today (supporting both federation and client requests). The new ones would validate via authentication, where appropriate, that the correct thing is being called.Now, is this the correct way to handle backwards compatibility in not-2018 Matrix? I don't think so. We probably just want to deprecate the existing endpoints (so we can remove them later) and rely on fallback semantics for servers who care. Spec-wise, we don't really need to define how the fallback works once the ecosystem has sufficient adoption, but the MSC should definitely cover it.
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.
It is frustrating that we still don't have a defined behaviour for unrecognised endpoints (MSC3743), making fallback 50 times harder than it should be.