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

Support MSC3916 by adding a federation /thumbnail endpoint and authenticated _matrix/client/v1/media/thumbnail endpoint #17388

Merged
merged 6 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions changelog.d/17388.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Support [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/blob/rav/authentication-for-media/proposals/3916-authentication-for-media.md)
by adding `_matrix/client/v1/media/thumbnail`, `_matrix/federation/v1/media/thumbnail` endpoints and stabilizing the
remaining `_matrix/client/v1/media` endpoints.
4 changes: 0 additions & 4 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -437,10 +437,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
"msc3823_account_suspension", False
)

self.msc3916_authenticated_media_enabled = experimental.get(
"msc3916_authenticated_media_enabled", False
)

# MSC4151: Report room API (Client-Server API)
self.msc4151_enabled: bool = experimental.get("msc4151_enabled", False)

Expand Down
6 changes: 5 additions & 1 deletion synapse/federation/transport/server/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
FEDERATION_SERVLET_CLASSES,
FederationAccountStatusServlet,
FederationMediaDownloadServlet,
FederationMediaThumbnailServlet,
FederationUnstableClientKeysClaimServlet,
)
from synapse.http.server import HttpServer, JsonResource
Expand Down Expand Up @@ -316,7 +317,10 @@ def register_servlets(
):
continue

if servletclass == FederationMediaDownloadServlet:
if (
servletclass == FederationMediaDownloadServlet
or servletclass == FederationMediaThumbnailServlet
):
if not hs.config.server.enable_media_repo:
continue

Expand Down
4 changes: 4 additions & 0 deletions synapse/federation/transport/server/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ async def new_func(
if (
func.__self__.__class__.__name__ # type: ignore
== "FederationMediaDownloadServlet"
or func.__self__.__class__.__name__ # type: ignore
== "FederationMediaThumbnailServlet"
):
response = await func(
origin, content, request, *args, **kwargs
Expand All @@ -375,6 +377,8 @@ async def new_func(
if (
func.__self__.__class__.__name__ # type: ignore
== "FederationMediaDownloadServlet"
or func.__self__.__class__.__name__ # type: ignore
== "FederationMediaThumbnailServlet"
):
response = await func(
origin, content, request, *args, **kwargs
Expand Down
56 changes: 56 additions & 0 deletions synapse/federation/transport/server/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,13 @@
parse_boolean_from_args,
parse_integer,
parse_integer_from_args,
parse_string,
parse_string_from_args,
parse_strings_from_args,
)
from synapse.http.site import SynapseRequest
from synapse.media._base import DEFAULT_MAX_TIMEOUT_MS, MAXIMUM_ALLOWED_MAX_TIMEOUT_MS
from synapse.media.thumbnailer import ThumbnailProvider
from synapse.types import JsonDict
from synapse.util import SYNAPSE_VERSION
from synapse.util.ratelimitutils import FederationRateLimiter
Expand Down Expand Up @@ -826,6 +828,59 @@ async def on_GET(
)


class FederationMediaThumbnailServlet(BaseFederationServerServlet):
"""
Implementation of new federation media `/thumbnail` endpoint outlined in MSC3916. Returns
a multipart/mixed response consisting of a JSON object and the requested media
item. This endpoint only returns local media.
"""

PATH = "/media/thumbnail/(?P<media_id>[^/]*)"
RATELIMIT = True

def __init__(
self,
hs: "HomeServer",
ratelimiter: FederationRateLimiter,
authenticator: Authenticator,
server_name: str,
):
super().__init__(hs, authenticator, ratelimiter, server_name)
self.media_repo = self.hs.get_media_repository()
self.dynamic_thumbnails = hs.config.media.dynamic_thumbnails
self.thumbnail_provider = ThumbnailProvider(
hs, self.media_repo, self.media_repo.media_storage
)

async def on_GET(
self,
origin: Optional[str],
content: Literal[None],
request: SynapseRequest,
media_id: str,
) -> None:

width = parse_integer(request, "width", required=True)
height = parse_integer(request, "height", required=True)
method = parse_string(request, "method", "scale")
# TODO Parse the Accept header to get an prioritised list of thumbnail types.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is temporary (or at least looks it, given the TODO), but I am a little bit sceptical here at always using PNGs.

I would think that this will balloon the size of photographs for example. It's technically 'safe' but it wouldn't surprise me if the PNG thumbnail of a JPG photograph is bigger than the photograph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do a follow-up PR to address this - I don't want to tackle it in this PR because we're trying to get this in to the RC on tuesday.

m_type = "image/png"
max_timeout_ms = parse_integer(
request, "timeout_ms", default=DEFAULT_MAX_TIMEOUT_MS
)
max_timeout_ms = min(max_timeout_ms, MAXIMUM_ALLOWED_MAX_TIMEOUT_MS)

if self.dynamic_thumbnails:
await self.thumbnail_provider.select_or_generate_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms, True
)
else:
await self.thumbnail_provider.respond_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms, True
)
self.media_repo.mark_recently_accessed(None, media_id)


FEDERATION_SERVLET_CLASSES: Tuple[Type[BaseFederationServlet], ...] = (
FederationSendServlet,
FederationEventServlet,
Expand Down Expand Up @@ -858,4 +913,5 @@ async def on_GET(
FederationMakeKnockServlet,
FederationAccountStatusServlet,
FederationMediaDownloadServlet,
FederationMediaThumbnailServlet,
)
11 changes: 9 additions & 2 deletions synapse/media/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,12 @@ async def get_remote_media(
respond_404(request)

async def get_remote_media_info(
self, server_name: str, media_id: str, max_timeout_ms: int, ip_address: str
self,
server_name: str,
media_id: str,
max_timeout_ms: int,
ip_address: str,
use_federation: bool,
) -> RemoteMedia:
"""Gets the media info associated with the remote file, downloading
if necessary.
Expand All @@ -553,6 +558,8 @@ async def get_remote_media_info(
max_timeout_ms: the maximum number of milliseconds to wait for the
media to be uploaded.
ip_address: IP address of the requester
use_federation: if a download is necessary, whether to request the remote file
over the federation `/download` endpoint

Returns:
The media info of the file
Expand All @@ -573,7 +580,7 @@ async def get_remote_media_info(
max_timeout_ms,
self.download_ratelimiter,
ip_address,
False,
use_federation,
)

# Ensure we actually use the responder so that it releases resources
Expand Down
82 changes: 61 additions & 21 deletions synapse/media/thumbnailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@
ThumbnailInfo,
respond_404,
respond_with_file,
respond_with_multipart_responder,
respond_with_responder,
)
from synapse.media.media_storage import MediaStorage
from synapse.media.media_storage import FileResponder, MediaStorage
from synapse.storage.databases.main.media_repository import LocalMedia

if TYPE_CHECKING:
from synapse.media.media_repository import MediaRepository
Expand Down Expand Up @@ -271,6 +273,7 @@ async def respond_local_thumbnail(
method: str,
m_type: str,
max_timeout_ms: int,
for_federation: bool,
) -> None:
media_info = await self.media_repo.get_local_media_info(
request, media_id, max_timeout_ms
Expand All @@ -290,6 +293,8 @@ async def respond_local_thumbnail(
media_id,
url_cache=bool(media_info.url_cache),
server_name=None,
for_federation=for_federation,
media_info=media_info,
)

async def select_or_generate_local_thumbnail(
Expand All @@ -301,6 +306,7 @@ async def select_or_generate_local_thumbnail(
desired_method: str,
desired_type: str,
max_timeout_ms: int,
for_federation: bool,
) -> None:
media_info = await self.media_repo.get_local_media_info(
request, media_id, max_timeout_ms
Expand All @@ -326,10 +332,16 @@ async def select_or_generate_local_thumbnail(

responder = await self.media_storage.fetch_media(file_info)
if responder:
await respond_with_responder(
request, responder, info.type, info.length
)
return
if for_federation:
await respond_with_multipart_responder(
self.hs.get_clock(), request, responder, media_info
)
return
else:
await respond_with_responder(
request, responder, info.type, info.length
)
return

logger.debug("We don't have a thumbnail of that size. Generating")

Expand All @@ -344,7 +356,15 @@ async def select_or_generate_local_thumbnail(
)

if file_path:
await respond_with_file(request, desired_type, file_path)
if for_federation:
await respond_with_multipart_responder(
self.hs.get_clock(),
request,
FileResponder(open(file_path, "rb")),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to have any handling for if open() fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like in the other places it's used there is no special handling - I can't think of what it would be other than an internal server error?

media_info,
)
else:
await respond_with_file(request, desired_type, file_path)
else:
logger.warning("Failed to generate thumbnail")
raise SynapseError(400, "Failed to generate thumbnail.")
Expand All @@ -360,9 +380,10 @@ async def select_or_generate_remote_thumbnail(
desired_type: str,
max_timeout_ms: int,
ip_address: str,
use_federation: bool,
) -> None:
media_info = await self.media_repo.get_remote_media_info(
server_name, media_id, max_timeout_ms, ip_address
server_name, media_id, max_timeout_ms, ip_address, use_federation
)
if not media_info:
respond_404(request)
Expand Down Expand Up @@ -424,12 +445,13 @@ async def respond_remote_thumbnail(
m_type: str,
max_timeout_ms: int,
ip_address: str,
use_federation: bool,
) -> None:
# TODO: Don't download the whole remote file
# We should proxy the thumbnail from the remote server instead of
# downloading the remote file and generating our own thumbnails.
media_info = await self.media_repo.get_remote_media_info(
server_name, media_id, max_timeout_ms, ip_address
server_name, media_id, max_timeout_ms, ip_address, use_federation
)
if not media_info:
return
Expand All @@ -448,6 +470,7 @@ async def respond_remote_thumbnail(
media_info.filesystem_id,
url_cache=False,
server_name=server_name,
for_federation=False,
)

async def _select_and_respond_with_thumbnail(
Expand All @@ -461,7 +484,9 @@ async def _select_and_respond_with_thumbnail(
media_id: str,
file_id: str,
url_cache: bool,
for_federation: bool,
server_name: Optional[str] = None,
media_info: Optional[LocalMedia] = None,
) -> None:
"""
Respond to a request with an appropriate thumbnail from the previously generated thumbnails.
Expand All @@ -476,6 +501,8 @@ async def _select_and_respond_with_thumbnail(
file_id: The ID of the media that a thumbnail is being requested for.
url_cache: True if this is from a URL cache.
server_name: The server name, if this is a remote thumbnail.
for_federation: whether the request is from the federation /thumbnail request
media_info: metadata about the media being requested.
"""
logger.debug(
"_select_and_respond_with_thumbnail: media_id=%s desired=%sx%s (%s) thumbnail_infos=%s",
Expand Down Expand Up @@ -511,13 +538,20 @@ async def _select_and_respond_with_thumbnail(

responder = await self.media_storage.fetch_media(file_info)
if responder:
await respond_with_responder(
request,
responder,
file_info.thumbnail.type,
file_info.thumbnail.length,
)
return
if for_federation:
assert media_info is not None
await respond_with_multipart_responder(
self.hs.get_clock(), request, responder, media_info
)
return
else:
await respond_with_responder(
request,
responder,
file_info.thumbnail.type,
file_info.thumbnail.length,
)
return

# If we can't find the thumbnail we regenerate it. This can happen
# if e.g. we've deleted the thumbnails but still have the original
Expand Down Expand Up @@ -558,12 +592,18 @@ async def _select_and_respond_with_thumbnail(
)

responder = await self.media_storage.fetch_media(file_info)
await respond_with_responder(
request,
responder,
file_info.thumbnail.type,
file_info.thumbnail.length,
)
if for_federation:
assert media_info is not None
await respond_with_multipart_responder(
self.hs.get_clock(), request, responder, media_info
)
else:
await respond_with_responder(
request,
responder,
file_info.thumbnail.type,
file_info.thumbnail.length,
)
else:
# This might be because:
# 1. We can't create thumbnails for the given media (corrupted or
Expand Down
Loading
Loading