chore(send queue): review

This commit is contained in:
Benjamin Bouvier
2024-11-06 10:54:18 +01:00
parent c04a73c28d
commit 9178e4ce33
5 changed files with 32 additions and 21 deletions

View File

@@ -61,6 +61,9 @@ pub trait EventCacheStore: AsyncTraitDeps {
/// keyed as a file before. The caller is responsible of ensuring that
/// the replacement makes sense, according to their use case.
///
/// This should not raise an error when the `from` parameter points to an
/// unknown media, and it should silently continue in this case.
///
/// # Arguments
///
/// * `from` - The previous `MediaRequest` of the file.

View File

@@ -86,7 +86,7 @@ pub enum QueuedRequestKind {
///
/// The bytes must be stored in the media cache, and are identified by the
/// cache key.
Upload {
MediaUpload {
/// Content type of the media to be uploaded.
///
/// Stored as a `String` because `Mime` which we'd really want to use

View File

@@ -82,7 +82,7 @@
//! - An initial media event is created and uses this temporary MXC ID, and
//! propagated as a local echo for an event.
//! - A [`QueuedRequest`] is pushed to upload the file's media
//! ([`QueuedRequestKind::Upload`]).
//! ([`QueuedRequestKind::MediaUpload`]).
//! - A [`DependentQueuedRequest`] is pushed to finish the upload
//! ([`DependentQueuedRequestKind::FinishUpload`]).
//!
@@ -106,7 +106,8 @@
//! When there is a thumbnail, things behave similarly, with some tweaks:
//!
//! - the thumbnail's content is also stored into the cache store immediately,
//! - the thumbnail is sent first as an [`QueuedRequestKind::Upload`] request,
//! - the thumbnail is sent first as an [`QueuedRequestKind::MediaUpload`]
//! request,
//! - the file upload is pushed as a dependent request of kind
//! [`DependentQueuedRequestKind::UploadFileWithThumbnail`] (this variant
//! keeps the file's key used to look it up in the cache store).
@@ -117,11 +118,11 @@
//!
//! - After the thumbnail has been uploaded, the dependent query will retrieve
//! the final MXC ID returned by the homeserver for the thumbnail, and store
//! it into the [`QueuedRequestKind::Upload`]'s `thumbnail_source` field,
//! it into the [`QueuedRequestKind::MediaUpload`]'s `thumbnail_source` field,
//! allowing to remember the thumbnail MXC ID when it's time to finish the
//! upload later.
//! - The dependent request is morphed into another
//! [`QueuedRequestKind::Upload`], for the file itself.
//! [`QueuedRequestKind::MediaUpload`], for the file itself.
//!
//! The rest of the process is then similar to that of uploading a file without
//! a thumbnail. The only difference is that there's a thumbnail source (MXC ID)
@@ -559,7 +560,7 @@ impl RoomSendQueue {
let txn_id = queued_request.transaction_id.clone();
trace!(txn_id = %txn_id, "received a request to send!");
let related_txn_id = as_variant!(&queued_request.kind, QueuedRequestKind::Upload { related_to, .. } => related_to.clone());
let related_txn_id = as_variant!(&queued_request.kind, QueuedRequestKind::MediaUpload { related_to, .. } => related_to.clone());
let Some(room) = room.get() else {
if is_dropping.load(Ordering::SeqCst) {
@@ -678,7 +679,7 @@ impl RoomSendQueue {
Ok(SentRequestKey::Event(res.event_id))
}
QueuedRequestKind::Upload {
QueuedRequestKind::MediaUpload {
content_type,
cache_key,
thumbnail_source,
@@ -1055,10 +1056,10 @@ impl QueueStorage {
.save_send_queue_request(
&self.room_id,
upload_thumbnail_txn.clone(),
QueuedRequestKind::Upload {
QueuedRequestKind::MediaUpload {
content_type: thumbnail_content_type.to_string(),
cache_key: thumbnail_media_request,
thumbnail_source: None,
thumbnail_source: None, // the thumbnail has no thumbnails :)
related_to: send_event_txn.clone(),
},
)
@@ -1085,7 +1086,7 @@ impl QueueStorage {
.save_send_queue_request(
&self.room_id,
upload_file_txn.clone(),
QueuedRequestKind::Upload {
QueuedRequestKind::MediaUpload {
content_type: content_type.to_string(),
cache_key: file_media_request,
thumbnail_source: None,
@@ -1169,7 +1170,7 @@ impl QueueStorage {
send_error: queued.error,
},
QueuedRequestKind::Upload { .. } => {
QueuedRequestKind::MediaUpload { .. } => {
// Don't return uploaded medias as their own things; the accompanying
// event represented as a dependent request should be sufficient.
return None;

View File

@@ -48,11 +48,17 @@ use crate::{
///
/// This uses a MXC ID that is only locally valid.
fn make_local_file_media_request(txn_id: &TransactionId) -> MediaRequest {
// .local is guaranteed to be on the local network. It would be a shame that
// `send-queue.local` resolves to an actual Synapse media server, we don't
// expect this to be likely though.
// This mustn't represent a potentially valid media server, otherwise it'd be
// possible for an attacker to return malicious content under some
// preconditions (e.g. the cache store has been cleared before the upload
// took place). To mitigate against this, we use the .localhost TLD,
// which is guaranteed to be on the local machine. As a result, the only attack
// possible would be coming from the user themselves, which we consider a
// non-threat.
MediaRequest {
source: MediaSource::Plain(OwnedMxcUri::from(format!("mxc://send-queue.local/{txn_id}"))),
source: MediaSource::Plain(OwnedMxcUri::from(format!(
"mxc://send-queue.localhost/{txn_id}"
))),
format: MediaFormat::File,
}
}
@@ -66,9 +72,9 @@ fn make_local_thumbnail_media_request(
height: UInt,
width: UInt,
) -> MediaRequest {
// See comment in [`Self::make_local_file_media_request`].
// See comment in [`make_local_file_media_request`].
let source =
MediaSource::Plain(OwnedMxcUri::from(format!("mxc://send-queue.local/{}", txn_id)));
MediaSource::Plain(OwnedMxcUri::from(format!("mxc://send-queue.localhost/{}", txn_id)));
let format = MediaFormat::Thumbnail(MediaThumbnailSettings {
size: MediaThumbnailSize { method: Method::Scale, width, height },
animated: false,
@@ -79,7 +85,7 @@ fn make_local_thumbnail_media_request(
/// Replace the source by the final ones in all the media types handled by
/// [`Room::make_attachment_type()`].
fn update_media_event_after_upload(echo: &mut RoomMessageEventContent, sent: SentMediaInfo) {
// Some variants look eerily similar below, but the `event` and `info` are all
// Some variants look really similar below, but the `event` and `info` are all
// different types…
match &mut echo.msgtype {
MessageType::Audio(event) => {
@@ -141,6 +147,7 @@ impl RoomSendQueue {
let Some(room) = self.inner.room.get() else {
return Err(RoomSendQueueError::RoomDisappeared);
};
if room.state() != RoomState::Joined {
return Err(RoomSendQueueError::RoomNotJoined);
}
@@ -367,7 +374,7 @@ impl QueueStorage {
trace!(related_to = %event_txn, "done uploading thumbnail, now queuing a request to send the media file itself");
let request = QueuedRequestKind::Upload {
let request = QueuedRequestKind::MediaUpload {
content_type,
cache_key,
// The thumbnail for the next upload is the file we just uploaded here.

View File

@@ -2117,7 +2117,7 @@ async fn test_media_uploads() {
// Check the data source: it should reference the send queue local storage.
let local_source = img_content.source;
assert_let!(MediaSource::Plain(mxc) = &local_source);
assert!(mxc.to_string().starts_with("mxc://send-queue.local/"), "{mxc}");
assert!(mxc.to_string().starts_with("mxc://send-queue.localhost/"), "{mxc}");
// The media is immediately available from the cache.
let file_media = client
@@ -2140,7 +2140,7 @@ async fn test_media_uploads() {
// Check the thumbnail source: it should reference the send queue local storage.
let local_thumbnail_source = info.thumbnail_source.unwrap();
assert_let!(MediaSource::Plain(mxc) = &local_thumbnail_source);
assert!(mxc.to_string().starts_with("mxc://send-queue.local/"), "{mxc}");
assert!(mxc.to_string().starts_with("mxc://send-queue.localhost/"), "{mxc}");
let thumbnail_media = client
.media()