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.

Sign up for GitHub

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

Jump to bottom

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
91bc8f9
add a federation thumbnail endpoint
H-Shay Jul 1, 2024
7c215c5
add _matrix/client/v1/media/thumbnail endpoint
H-Shay Jul 1, 2024
dcff123
stablize remaining _matrix/client/v1/media endpoints
H-Shay Jul 1, 2024
7fed5d2
tests
H-Shay Jul 1, 2024
b1bffa5
newsfragment
H-Shay Jul 1, 2024
63c129c
requested changes
H-Shay Jul 8, 2024
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
11 changes: 9 additions & 2 deletions synapse/media/media_repository.py
View file
Open in desktop
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
View file
Open in desktop
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 not for_federation:
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
await respond_with_responder(
request, responder, info.type, info.length
)
return
else:
await respond_with_multipart_responder(
self.hs.get_clock(), request, responder, media_info
)
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
27 changes: 20 additions & 7 deletions synapse/rest/client/media.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ async def on_GET(self, request: SynapseRequest) -> None:
respond_with_json(request, 200, self.limits_dict, send_cors=True)


class UnstableThumbnailResource(RestServlet):
class ThumbnailResource(RestServlet):
PATTERNS = [
re.compile(
"/_matrix/client/unstable/org.matrix.msc3916/media/thumbnail/(?P<server_name>[^/]*)/(?P<media_id>[^/]*)$"
"/_matrix/client/v1/media/thumbnail/(?P<server_name>[^/]*)/(?P<media_id>[^/]*)$"
)
]

Expand Down Expand Up @@ -159,11 +159,25 @@ async def on_GET(
if self._is_mine_server_name(server_name):
if self.dynamic_thumbnails:
await self.thumbnailer.select_or_generate_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms
request,
media_id,
width,
height,
method,
m_type,
max_timeout_ms,
False,
)
else:
await self.thumbnailer.respond_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms
request,
media_id,
width,
height,
method,
m_type,
max_timeout_ms,
False,
)
self.media_repo.mark_recently_accessed(None, media_id)
else:
Expand Down Expand Up @@ -191,6 +205,7 @@ async def on_GET(
m_type,
max_timeout_ms,
ip_address,
True,
)
self.media_repo.mark_recently_accessed(server_name, media_id)

Expand Down Expand Up @@ -264,7 +279,5 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None:
http_server
)
UnstableMediaConfigResource(hs).register(http_server)
UnstableThumbnailResource(hs, media_repo, media_repo.media_storage).register(
http_server
)
ThumbnailResource(hs, media_repo, media_repo.media_storage).register(http_server)
DownloadResource(hs, media_repo).register(http_server)
19 changes: 17 additions & 2 deletions synapse/rest/media/thumbnail_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,25 @@ async def on_GET(
if self._is_mine_server_name(server_name):
if self.dynamic_thumbnails:
await self.thumbnail_provider.select_or_generate_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms
request,
media_id,
width,
height,
method,
m_type,
max_timeout_ms,
False,
)
else:
await self.thumbnail_provider.respond_local_thumbnail(
request, media_id, width, height, method, m_type, max_timeout_ms
request,
media_id,
width,
height,
method,
m_type,
max_timeout_ms,
False,
)
self.media_repo.mark_recently_accessed(None, media_id)
else:
Expand Down Expand Up @@ -120,5 +134,6 @@ async def on_GET(
m_type,
max_timeout_ms,
ip_address,
False,
)
self.media_repo.mark_recently_accessed(server_name, media_id)