From 92a35381b56ffa3c2a611287bb5011f574271478 Mon Sep 17 00:00:00 2001 From: Benjamin Lee Date: Sun, 15 Sep 2024 23:17:11 -0700 Subject: [PATCH 1/5] client-api: fix build when unstable-msc4168 feature is disabled (#1910) Without this feature, the v5 module does not exist. --- .../ruma-client-api/src/sync/sync_events/v4.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/crates/ruma-client-api/src/sync/sync_events/v4.rs b/crates/ruma-client-api/src/sync/sync_events/v4.rs index 03cea5ef..677f1221 100644 --- a/crates/ruma-client-api/src/sync/sync_events/v4.rs +++ b/crates/ruma-client-api/src/sync/sync_events/v4.rs @@ -22,7 +22,9 @@ use ruma_events::{ }; use serde::{de::Error as _, Deserialize, Serialize}; -use super::{v5, DeviceLists, UnreadNotificationsCount}; +#[cfg(feature = "unstable-msc4186")] +use super::v5; +use super::{DeviceLists, UnreadNotificationsCount}; const METADATA: Metadata = metadata! { method: POST, @@ -927,6 +929,7 @@ impl Typing { } } +#[cfg(feature = "unstable-msc4186")] impl From for Request { fn from(value: v5::Request) -> Self { Self { @@ -951,6 +954,7 @@ impl From for Request { } } +#[cfg(feature = "unstable-msc4186")] impl From for SyncRequestList { fn from(value: v5::request::List) -> Self { Self { @@ -973,12 +977,14 @@ impl From for SyncRequestList { } } +#[cfg(feature = "unstable-msc4186")] impl From for RoomDetailsConfig { fn from(value: v5::request::RoomDetails) -> Self { Self { required_state: value.required_state, timeline_limit: value.timeline_limit } } } +#[cfg(feature = "unstable-msc4186")] impl From for SyncRequestListFilters { fn from(value: v5::request::ListFilters) -> Self { Self { @@ -989,6 +995,7 @@ impl From for SyncRequestListFilters { } } +#[cfg(feature = "unstable-msc4186")] impl From for RoomSubscription { fn from(value: v5::request::RoomSubscription) -> Self { Self { @@ -999,6 +1006,7 @@ impl From for RoomSubscription { } } +#[cfg(feature = "unstable-msc4186")] impl From for ExtensionsConfig { fn from(value: v5::request::Extensions) -> Self { Self { @@ -1013,6 +1021,7 @@ impl From for ExtensionsConfig { } } +#[cfg(feature = "unstable-msc4186")] impl From for ToDeviceConfig { fn from(value: v5::request::ToDevice) -> Self { Self { @@ -1025,18 +1034,21 @@ impl From for ToDeviceConfig { } } +#[cfg(feature = "unstable-msc4186")] impl From for E2EEConfig { fn from(value: v5::request::E2EE) -> Self { Self { enabled: value.enabled } } } +#[cfg(feature = "unstable-msc4186")] impl From for AccountDataConfig { fn from(value: v5::request::AccountData) -> Self { Self { enabled: value.enabled, lists: value.lists, rooms: value.rooms } } } +#[cfg(feature = "unstable-msc4186")] impl From for ReceiptsConfig { fn from(value: v5::request::Receipts) -> Self { Self { @@ -1047,6 +1059,7 @@ impl From for ReceiptsConfig { } } +#[cfg(feature = "unstable-msc4186")] impl From for RoomReceiptConfig { fn from(value: v5::request::ReceiptsRoom) -> Self { match value { @@ -1056,6 +1069,7 @@ impl From for RoomReceiptConfig { } } +#[cfg(feature = "unstable-msc4186")] impl From for TypingConfig { fn from(value: v5::request::Typing) -> Self { Self { enabled: value.enabled, lists: value.lists, rooms: value.rooms } From 61f51503581a3284a8b7c9fd06688e82102bf844 Mon Sep 17 00:00:00 2001 From: Benjamin Lee Date: Tue, 17 Sep 2024 01:45:04 -0700 Subject: [PATCH 2/5] federation-api: allow multipart body without preceding CRLF on first boundary RFC 2046[1] is somewhat ambiguous on whether or not it's valid to omit the preceding CRLF for the first boundary. The prose on page 19 suggests that it is not: > The boundary delimiter MUST occur at the beginning of a line, i.e., > following a CRLF, and the initial CRLF is considered to be attached > to the boundary delimiter line rather than part of the preceding > part. The boundary may be followed by zero or more characters of > linear whitespace. It is then terminated by either another CRLF and > the header fields for the next part, or by two CRLFs, in which case > there are no header fields for the next part. If no Content-Type > field is present it is assumed to be "message/rfc822" in a > "multipart/digest" and "text/plain" otherwise. > > NOTE: The CRLF preceding the boundary delimiter line is conceptually > attached to the boundary so that it is possible to have a part that > does not end with a CRLF (line break). Body parts that must be > considered to end with line breaks, therefore, must have two CRLFs > preceding the boundary delimiter line, the first of which is part of > the preceding body part, and the second of which is part of the > encapsulation boundary. But the BNF on page 22 suggests that it is, as long as there is no preamble: > dash-boundary := "--" boundary > ; boundary taken from the value of > ; boundary parameter of the > ; Content-Type field. > > multipart-body := [preamble CRLF] > dash-boundary transport-padding CRLF > body-part *encapsulation > close-delimiter transport-padding > [CRLF epilogue] Dendrite currently generates multipart responses without a preceding CRLF for the first boundary[2], which were rejected by the previous ruma parsing logic. [1]: https://datatracker.ietf.org/doc/html/rfc2046 [2]: https://github.com/matrix-org/dendrite/issues/3414 --- .../src/authenticated_media.rs | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/crates/ruma-federation-api/src/authenticated_media.rs b/crates/ruma-federation-api/src/authenticated_media.rs index 135d3424..2063b97d 100644 --- a/crates/ruma-federation-api/src/authenticated_media.rs +++ b/crates/ruma-federation-api/src/authenticated_media.rs @@ -172,12 +172,20 @@ fn try_from_multipart_mixed_response>( let mut full_boundary = Vec::with_capacity(boundary.len() + 4); full_boundary.extend_from_slice(b"\r\n--"); full_boundary.extend_from_slice(boundary); + let full_boundary_no_crlf = full_boundary.strip_prefix(b"\r\n").unwrap(); let mut boundaries = memchr::memmem::find_iter(body, &full_boundary); - let metadata_start = boundaries.next().ok_or_else(|| { - MultipartMixedDeserializationError::MissingBodyParts { expected: 2, found: 0 } - })? + full_boundary.len(); + let metadata_start = if body.starts_with(full_boundary_no_crlf) { + // If there is no preamble before the first boundary, it may omit the + // preceding CRLF. + full_boundary_no_crlf.len() + } else { + boundaries.next().ok_or_else(|| MultipartMixedDeserializationError::MissingBodyParts { + expected: 2, + found: 0, + })? + full_boundary.len() + }; let metadata_end = boundaries.next().ok_or_else(|| { MultipartMixedDeserializationError::MissingBodyParts { expected: 2, found: 0 } })?; @@ -411,6 +419,15 @@ mod tests { .unwrap(); try_from_multipart_mixed_response(response).unwrap_err(); + + // Boundary without CRLF with preamble. + let body = "foo--abcdef\r\n\r\n{}\r\n--abcdef\r\n\r\nsome plain text\r\n--abcdef--"; + let response = http::Response::builder() + .header(http::header::CONTENT_TYPE, "multipart/mixed; boundary=abcdef") + .body(body) + .unwrap(); + + try_from_multipart_mixed_response(response).unwrap_err(); } #[test] @@ -477,6 +494,36 @@ mod tests { assert_eq!(file_content.content_type.unwrap(), "text/plain"); assert_eq!(file_content.content_disposition, None); + // No leading CRLF (and no preamble) + let body = "--abcdef\r\n\r\n{}\r\n--abcdef\r\n\r\nsome plain text\r\n--abcdef--"; + let response = http::Response::builder() + .header(http::header::CONTENT_TYPE, "multipart/mixed; boundary=abcdef") + .body(body) + .unwrap(); + + let (_metadata, content) = try_from_multipart_mixed_response(response).unwrap(); + + assert_matches!(content, FileOrLocation::File(file_content)); + assert_eq!(file_content.file, b"some plain text"); + assert_eq!(file_content.content_type, None); + assert_eq!(file_content.content_disposition, None); + + // Boundary text in preamble, but no leading CRLF, so it should be + // ignored. + let body = + "foo--abcdef\r\n--abcdef\r\n\r\n{}\r\n--abcdef\r\n\r\nsome plain text\r\n--abcdef--"; + let response = http::Response::builder() + .header(http::header::CONTENT_TYPE, "multipart/mixed; boundary=abcdef") + .body(body) + .unwrap(); + + let (_metadata, content) = try_from_multipart_mixed_response(response).unwrap(); + + assert_matches!(content, FileOrLocation::File(file_content)); + assert_eq!(file_content.file, b"some plain text"); + assert_eq!(file_content.content_type, None); + assert_eq!(file_content.content_disposition, None); + // No body part headers. let body = "\r\n--abcdef\r\n\r\n{}\r\n--abcdef\r\n\r\nsome plain text\r\n--abcdef--"; let response = http::Response::builder() From 1298c1d690ca539efc436aea4049cc34475433fb Mon Sep 17 00:00:00 2001 From: Timo Date: Thu, 12 Sep 2024 13:21:22 +0200 Subject: [PATCH 3/5] events: Fixing comments for `new` method. --- crates/ruma-events/src/call/member/member_state_key.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruma-events/src/call/member/member_state_key.rs b/crates/ruma-events/src/call/member/member_state_key.rs index b593d499..f50cb773 100644 --- a/crates/ruma-events/src/call/member/member_state_key.rs +++ b/crates/ruma-events/src/call/member/member_state_key.rs @@ -16,11 +16,11 @@ pub struct CallMemberStateKey { impl CallMemberStateKey { /// Constructs a new CallMemberStateKey there are three possible formats: - /// - "_{UserId}_{DeviceId}" example: "_@test:user.org_DEVICE". `device_id`: Some`, `underscore: + /// - `_{UserId}_{DeviceId}` example: `_@test:user.org_DEVICE`. `device_id: Some`, `underscore: /// true` - /// - "{UserId}_{DeviceId}" example: "@test:user.org_DEVICE". `device_id`: Some`, `underscore: + /// - `{UserId}_{DeviceId}` example: `@test:user.org_DEVICE`. `device_id: Some`, `underscore: /// false` - /// - "{UserId}" example example: "@test:user.org". `device_id`: None`, underscore is ignored: + /// - `{UserId}` example: `@test:user.org`. `device_id: None`, underscore is ignored: /// `underscore: false|true` /// /// Dependent on the parameters the correct CallMemberStateKey will be constructed. From 92110cabb55449b4db516750e2777934f3fd70da Mon Sep 17 00:00:00 2001 From: Timo Date: Mon, 16 Sep 2024 17:45:32 +0200 Subject: [PATCH 4/5] events: Use DeviceId instead of String. --- crates/ruma-events/src/call/member.rs | 20 +++++++++---------- .../src/call/member/member_data.rs | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/ruma-events/src/call/member.rs b/crates/ruma-events/src/call/member.rs index c193249e..a31b795a 100644 --- a/crates/ruma-events/src/call/member.rs +++ b/crates/ruma-events/src/call/member.rs @@ -11,7 +11,7 @@ mod member_state_key; pub use focus::*; pub use member_data::*; pub use member_state_key::*; -use ruma_common::MilliSecondsSinceUnixEpoch; +use ruma_common::{MilliSecondsSinceUnixEpoch, OwnedDeviceId}; use ruma_macros::{EventContent, StringEnum}; use serde::{Deserialize, Serialize}; @@ -56,7 +56,7 @@ impl CallMemberEventContent { /// Creates a new [`CallMemberEventContent`] with [`SessionMembershipData`]. pub fn new( application: Application, - device_id: String, + device_id: OwnedDeviceId, focus_active: ActiveFocus, foci_preferred: Vec, created_ts: Option, @@ -234,8 +234,8 @@ mod tests { use assert_matches2::assert_matches; use ruma_common::{ - device_id, user_id, MilliSecondsSinceUnixEpoch as TS, OwnedEventId, OwnedRoomId, - OwnedUserId, + device_id, owned_device_id, user_id, MilliSecondsSinceUnixEpoch as TS, OwnedEventId, + OwnedRoomId, OwnedUserId, }; use serde_json::{from_value as from_json_value, json, Value as JsonValue}; @@ -257,7 +257,7 @@ mod tests { call_id: "123456".to_owned(), scope: CallScope::Room, }), - device_id: "ABCDE".to_owned(), + device_id: owned_device_id!("ABCDE"), expires: Duration::from_secs(3600), foci_active: vec![Focus::Livekit(LivekitFocus { alias: "1".to_owned(), @@ -274,7 +274,7 @@ mod tests { call_id: "123456".to_owned(), scope: CallScope::Room, }), - "ABCDE".to_owned(), + owned_device_id!("ABCDE"), ActiveFocus::Livekit(ActiveLivekitFocus { focus_selection: FocusSelection::OldestMembership, }), @@ -354,7 +354,7 @@ mod tests { call_id: "123456".to_owned(), scope: CallScope::Room, }), - "THIS_DEVICE".to_owned(), + owned_device_id!("THIS_DEVICE"), ActiveFocus::Livekit(ActiveLivekitFocus { focus_selection: FocusSelection::OldestMembership, }), @@ -404,7 +404,7 @@ mod tests { call_id: "123456".to_owned(), scope: CallScope::Room, }), - device_id: "THIS_DEVICE".to_owned(), + device_id: owned_device_id!("THIS_DEVICE"), expires: Duration::from_secs(3600), foci_active: vec![Focus::Livekit(LivekitFocus { alias: "room1".to_owned(), @@ -418,7 +418,7 @@ mod tests { call_id: "".to_owned(), scope: CallScope::Room, }), - device_id: "OTHER_DEVICE".to_owned(), + device_id: owned_device_id!("OTHER_DEVICE"), expires: Duration::from_secs(3600), foci_active: vec![Focus::Livekit(LivekitFocus { alias: "room2".to_owned(), @@ -526,7 +526,7 @@ mod tests { call_id: "".to_owned(), scope: CallScope::Room, }), - device_id: "THIS_DEVICE".to_owned(), + device_id: owned_device_id!("THIS_DEVICE"), foci_preferred: [Focus::Livekit(LivekitFocus { alias: "room1".to_owned(), service_url: "https://livekit1.com".to_owned(), diff --git a/crates/ruma-events/src/call/member/member_data.rs b/crates/ruma-events/src/call/member/member_data.rs index 2939372d..6804c570 100644 --- a/crates/ruma-events/src/call/member/member_data.rs +++ b/crates/ruma-events/src/call/member/member_data.rs @@ -5,7 +5,7 @@ use std::time::Duration; use as_variant::as_variant; -use ruma_common::MilliSecondsSinceUnixEpoch; +use ruma_common::{DeviceId, MilliSecondsSinceUnixEpoch, OwnedDeviceId}; use ruma_macros::StringEnum; use serde::{Deserialize, Serialize}; use tracing::warn; @@ -41,7 +41,7 @@ impl<'a> MembershipData<'a> { } /// The device id of this membership. - pub fn device_id(&self) -> &String { + pub fn device_id(&self) -> &DeviceId { match self { MembershipData::Legacy(data) => &data.device_id, MembershipData::Session(data) => &data.device_id, @@ -121,7 +121,7 @@ pub struct LegacyMembershipData { /// The device id of this membership. /// /// The same user can join with their phone/computer. - pub device_id: String, + pub device_id: OwnedDeviceId, /// The duration in milliseconds relative to the time this membership joined /// during which the membership is valid. @@ -190,7 +190,7 @@ pub struct LegacyMembershipDataInit { /// The device id of this membership. /// /// The same user can join with their phone/computer. - pub device_id: String, + pub device_id: OwnedDeviceId, /// The duration in milliseconds relative to the time this membership joined /// during which the membership is valid. @@ -236,7 +236,7 @@ pub struct SessionMembershipData { /// The device id of this membership. /// /// The same user can join with their phone/computer. - pub device_id: String, + pub device_id: OwnedDeviceId, /// A list of the foci that this membership proposes to use. pub foci_preferred: Vec, From 1ae98db9c44f46a590f4c76baf5cef70ebb6970d Mon Sep 17 00:00:00 2001 From: Timo Date: Mon, 16 Sep 2024 17:49:35 +0200 Subject: [PATCH 5/5] changelog: Document `device_id` type change --- crates/ruma-events/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/ruma-events/CHANGELOG.md b/crates/ruma-events/CHANGELOG.md index f024b5b3..683bbc6d 100644 --- a/crates/ruma-events/CHANGELOG.md +++ b/crates/ruma-events/CHANGELOG.md @@ -18,6 +18,11 @@ Improvements: - Stabilize support for muting in VoIP calls, according to Matrix 1.11 - All the root `Any*EventContent` types now have a `EventContentFromType` implementations automatically derived by the `event_enum!` macro. +- `CallMemberEventContent` now supports two different formats: Session memberships and Legacy memberships. +The new format (Session) is required to reliably display the call member count (reliable call member events). +`CallMemberEventContent` is now an enum to model the two different formats. +- `CallMemberStateKey` (instead of `OwnedUserId`) is now used as the state key type for `CallMemberEventContent`. +This guarantees correct formatting of the event key. Breaking changes: