media_filepath is not passed to storage proviers

[majkrzak]

Description

synapse/synapse/media/media_storage.py

Lines 206 to 279 in ab277b3

	async def store_into_file(
	self, file_info: FileInfo
	) -> AsyncIterator[tuple[BinaryIO, str]]:
	"""Async Context manager used to get a file like object to write into, as
	described by file_info.
	Actually yields a 2-tuple (file, media_filepath,), where file is a file
	like object that can be written to and media_filepath is the absolute path
	of the file on disk.
	media_filepath can be used to read the contents from after upload, e.g. to
	generate thumbnails.
	Args:
	file_info: Info about the file to store
	Example:
	async with media_storage.store_into_file(info) as (f, media_filepath,):
	# .. write into f ...
	"""
	path = self._file_info_to_path(file_info)
	is_temp_file = False
	if self.local_provider:
	media_filepath = os.path.join(self.local_media_directory, path) # type: ignore[arg-type]
	os.makedirs(os.path.dirname(media_filepath), exist_ok=True)
	with start_active_span("writing to main media repo"):
	with open(media_filepath, "wb") as f:
	yield f, media_filepath
	else:
	# No local provider, write to temp file
	is_temp_file = True
	with tempfile.NamedTemporaryFile(delete=False) as f:
	media_filepath = f.name
	yield cast(BinaryIO, f), media_filepath
	# Spam check and store to other providers (runs for both local and temp file cases)
	try:
	with start_active_span("spam checking and writing to storage providers"):
	spam_check = (
	await self._spam_checker_module_callbacks.check_media_file_for_spam(
	ReadableFileWrapper(self.clock, media_filepath), file_info
	)
	)
	if spam_check != self._spam_checker_module_callbacks.NOT_SPAM:
	logger.info("Blocking media due to spam checker")
	# Note that we'll delete the stored media, due to the
	# try/except below. The media also won't be stored in
	# the DB.
	# We currently ignore any additional field returned by
	# the spam-check API.
	raise SpamMediaException(errcode=spam_check[0])
	for provider in self.storage_providers:
	with start_active_span(str(provider)):
	await provider.store_file(path, file_info)
	# If using a temp file, delete it after uploading to storage providers
	if is_temp_file:
	try:
	os.remove(media_filepath)
	except Exception:
	pass
	except Exception as e:
	try:
	os.remove(media_filepath)
	except Exception:
	pass
	raise e from None

According to this implementation, the media_filepath is not passed to the storage providers as described in:

synapse/synapse/media/storage_provider.py

Lines 47 to 54 in ab277b3

	async def store_file(self, path: str, file_info: FileInfo) -> None:
	"""Store the file described by file_info. The actual contents can be
	retrieved by reading the file in file_info.upload_path.
	Args:
	path: Relative path of file in local cache
	file_info: The metadata of the file.
	"""

This causes an issue when local storage is disabled, for example with matrix-org/synapse-s3-storage-provider#162

Steps to reproduce

• list the steps
• that reproduce the bug
• using hyphens as bullet points

Homeserver

NOT RELEVANT

Synapse Version

ab277b3

Installation Method

Other (please mention below)

Database

NOT RELEVANT

Workers

Single process

Platform

NOT RELEVANT

Configuration

NOT RELEVANT

Relevant log output

NOT RELEVANT

Anything else that would be useful to know?

I love usefull forms...

[majkrzak]

Solution may be either to expand the fileinfo with actual upload_path as stated in the function docsting, or change function singature and pass additional argument. Both solutions are relatively simple.

[anoadragon453]

Related: #19204