From 9aacbf26502b32e709b48ee694e01b64c291e4cc Mon Sep 17 00:00:00 2001 From: timedout Date: Sun, 12 Jul 2026 03:51:28 +0100 Subject: [PATCH] feat: Start on PDU builders ran out of creative juices so this doesn't work but i'll find a way later --- ROADMAP.md | 34 +++++ src/api/client/membership/leave.rs | 15 +- src/core/matrix/pdu.rs | 197 ++++++++++++++++++++++++++- src/core/matrix/pdu/hashes.rs | 3 +- src/core/matrix/pdu/metadata.rs | 7 +- src/core/matrix/pdu/v2.rs | 8 ++ src/core/matrix/pdu/v3.rs | 6 + src/service/rooms/timeline/create.rs | 6 +- 8 files changed, 261 insertions(+), 15 deletions(-) create mode 100644 ROADMAP.md diff --git a/ROADMAP.md b/ROADMAP.md new file mode 100644 index 000000000..c6f45889b --- /dev/null +++ b/ROADMAP.md @@ -0,0 +1,34 @@ +# Roadmap for PDU versioning + +Currently, PDUs are one monolithic `PduEvent` struct which applies a one-size-fits-all approach to PDUs, however as we +support a wide range of room versions, this only just about currently works, and will cause more and more problems as +time progresses. This branch will split the `PduEvent` struct into multiple versioned structs that represent the proper +format for appropriate room versions, and will drop the `Event` trait in favour of Ruma's `ruma::state_res::Event` +trait. + +This will require a lot of work, and since I forget everything the moment I look out the window, here be a list (waow): + +- [x] Split `PduEvent` into multiple versioned structs, implementing `ruma::state_res::Event` for each of them. +- [ ] Refactor out all references to `PduEvent`. +- [ ] Refactor out all references to `conduwuit_core::event::Event` trait in favour of `ruma::state_res::Event`. +- [ ] Centralise PDU struct creation in a way that allows us to greater control the deserialization process. + - Developers can't be trusted to read documentation so we can't store the PDU metadata (things like event ID and + rejection status) in the PDU struct itself, as this may end up being leaked to consumers. Instead, PDU metadata + should be stored seprately from the PDU struct itself, but needs to be strongly associated with it (i.e. fetched + at the same time, always returned together, etc). This may take a form similar to + `StoredPdu { pdu: T, metadata: PduMetadata }`. +- [ ] Stop storing generated data in the `unsigned` field, but instead calculate it on demand(?). + - Storing generated data in the `unsigned` field results in: + - History visibility cannot be enforced on `prev_content` or `redacted_because`. + - `age` is manually calculated but only sometimes. + - `membership` is not provided at all. + - `prev_content` can leak redacted state event content ([#1103]). + - `prev_content` cannot easily be populated for backfilled events, leaving them contextless. + - It is easy to accidentally leak `transaction_id`. + +Once `PduEvent` is gone, we can then do the following lovely things: + +- Drop our local state resolution logic and event authorisation logic and instead use Ruma's, which naturally receives + more love and attention. Our current `Event` trait and `PduEvent` implementations are incompatible with what Ruma + needs, and do not store the appropriate information required. +- There's something else but I forgot just trust me diff --git a/src/api/client/membership/leave.rs b/src/api/client/membership/leave.rs index afb9e0e97..d0f95f6f2 100644 --- a/src/api/client/membership/leave.rs +++ b/src/api/client/membership/leave.rs @@ -1,9 +1,11 @@ use std::collections::HashSet; +use assign::assign; use axum::extract::State; use conduwuit::{ Err, Pdu, Result, debug_info, debug_warn, err, matrix::{event::gen_event_id, pdu::PartialPdu}, + pdu::{AnyPDU, metadata::PduMetadata, new_pdu}, utils::{self, FutureBoolExt, future::ReadyEqExt}, warn, }; @@ -216,7 +218,7 @@ pub async fn remote_leave_room( room_id: &RoomId, reason: Option, mut servers: HashSet, -) -> Result { +) -> Result { let mut make_leave_response_and_server = Err!(BadServerResponse("No remote server available to assist in leaving {room_id}.")); @@ -406,9 +408,14 @@ pub async fn remote_leave_room( .outlier .add_pdu_outlier(&event_id, &leave_event); - let leave_pdu = Pdu::from_id_val(&event_id, leave_event).map_err(|e| { - err!(BadServerResponse("Invalid leave PDU received during federated leave: {e:?}")) - })?; + let leave_pdu = new_pdu( + room_version_rules.into(), + PduMetadata::new(event_id), + serde_json::to_value(leave_event)?, + )?; + // let leave_pdu = Pdu::from_id_val(&event_id, leave_event).map_err(|e| { + // err!(BadServerResponse("Invalid leave PDU received during federated leave: + // {e:?}")) })?; Ok(leave_pdu) } diff --git a/src/core/matrix/pdu.rs b/src/core/matrix/pdu.rs index 2ebebb54a..390100540 100644 --- a/src/core/matrix/pdu.rs +++ b/src/core/matrix/pdu.rs @@ -13,11 +13,13 @@ pub mod v3; use std::cmp::Ordering; +use assign::assign; use ruma::{ CanonicalJsonObject, CanonicalJsonValue, EventId, MilliSecondsSinceUnixEpoch, OwnedEventId, - OwnedRoomId, OwnedServerName, OwnedUserId, RoomId, UInt, UserId, events::TimelineEventType, + OwnedRoomId, OwnedServerName, OwnedUserId, RoomId, ServerSignatures, UInt, UserId, event_id, + events::TimelineEventType, room_version_rules::RoomVersionRules, state_res::Event, }; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; use serde_json::value::RawValue as RawJsonValue; pub use self::{ @@ -27,10 +29,193 @@ pub use self::{ partial::PartialPdu, raw_id::*, }; -use super::{Event, StateKey}; -use crate::Result; +use super::StateKey; +use crate::{ + Err, Result, + pdu::{hashes::Hashes, metadata::PduMetadata}, +}; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum EventFormatVersion { + /// V1 represents the PDU format for rooms v1 and v2. + V1, + /// V2 represents the PDU format for rooms v3-v11. + V2, + /// V3 represents the PDU format for rooms v12+. + V3, +} + +impl From for EventFormatVersion { + fn from(rules: RoomVersionRules) -> Self { + // TODO: this is hacky because ruma doesn't actually define event versions in an + // enum, but does define consts with the appropriate rules. + if rules.event_format.require_event_id { + Self::V1 + } else if rules.event_format.require_room_create_room_id { + Self::V2 + } else { + Self::V3 + } + } +} + +/// Trait that allows conversion from a generic PDU to a concrete PDU type. +pub trait ConcretePDU: ruma::state_res::Event { + /// The concrete type for this PDU (`v2::PDU`, `v3::PDU`, etc) + type Concrete; + + /// Get a reference to the concrete PDU type. + fn to_concrete(&self) -> &Self::Concrete; + + /// Get the value of the concrete PDU type, consuming. + fn into_concrete(self) -> Self::Concrete; +} + +pub fn new_pdu( + format: EventFormatVersion, + metadata: PduMetadata, + json: serde_json::Value, +) -> Result +where + E: ConcretePDU, + JSON: DeserializeOwned, +{ + match format { + | EventFormatVersion::V2 => + Ok(assign!(v2::PDU::try_from(json)?, {internal_metadata: metadata})), + | EventFormatVersion::V3 => + Ok(assign!(v3::PDU::try_from(json)?, {internal_metadata: metadata})), + | _ => Err!("Unsupported event format: {format:?}"), + } +} + +/// A builder type that specifies every field present on any PDU type, allowing +/// it to be used to create any PDU version with the appropriate function. +/// +/// See `create_pdu`. +#[derive(Clone, Debug, Default)] +pub struct CommonPDUBuilder { + pub auth_events: Option>, + pub content: Option>, + pub depth: Option, + pub hashes: Option, + pub origin_server_ts: Option, + pub prev_events: Option>, + pub redacts: Option, + pub room_id: Option, + pub sender: Option, + pub signatures: Option, + pub state_key: Option, + pub event_type: Option, + pub unsigned: Option>, + pub internal_metadata: Option, +} + +/// Creates a new PDU based on the given version and builder. +/// +/// ## Example +/// +/// ```rust +/// use conduwuit_core::pdu::{CommonPDUBuilder, create_pdu} +/// +/// let room_version_rules = RoomVersionRules::V10; // This usually comes dynamically. +/// let builder = CommonPDUBuilder { +/// auth_events: Some(vec![event_id!("$auth_event_id").to_owned()]), +/// content: Some(Box::new(serde_json::json!({"key": "value"}))), +/// depth: Some(1), +/// origin_server_ts: Some(MilliSecondsSinceUnixEpoch(123456789)), +/// prev_events: Some(vec![event_id!("$prev_event_id").to_owned()]), +/// room_id: Some(room_id!("!room_id").to_owned()), +/// sender: Some(user_id!("@sender:example.com").to_owned()), +/// event_type: Some(TimelineEventType::RoomMessage), +/// ..Default::default() +/// }; +/// let pdu = create_pdu( +/// room_version_rules.into(), +/// builder, +/// ).expect("PDU creation must succeed"); +/// ``` +pub fn create_pdu(version: EventFormatVersion, builder: CommonPDUBuilder) -> Result { + match version { + | EventFormatVersion::V2 => Ok(create_v2_pdu(builder)), + | EventFormatVersion::V3 => Ok(create_v3_pdu(builder)), + | _ => Err!("Unsupported event format: {version:?}"), + } +} + +fn create_v2_pdu(builder: CommonPDUBuilder) -> v2::PDU { + v2::PDU { + auth_events: builder + .auth_events + .expect("auth_events must be provided for a V2 PDU"), + content: builder + .content + .expect("content must be provided for a V2 PDU"), + depth: builder.depth.expect("depth must be provided for a V2 PDU"), + hashes: builder.hashes.unwrap_or_default(), /* We allow default here since we might not + * have hashed yet */ + origin_server_ts: builder + .origin_server_ts + .expect("origin_server_ts must be provided for a V2 PDU"), + prev_events: builder + .prev_events + .expect("prev_events must be provided for a V2 PDU"), + redacts: builder.redacts, + room_id: builder + .room_id + .expect("room_id must be provided for a V2 PDU"), + sender: builder + .sender + .expect("sender must be provided for a V2 PDU"), + signatures: builder.signatures.unwrap_or_default(), /* We allow default here since we + * might not have signed yet */ + state_key: builder.state_key, + event_type: builder + .event_type + .expect("event_type must be provided for a V2 PDU"), + unsigned: builder.unsigned, + internal_metadata: builder + .internal_metadata + .unwrap_or_else(|| PduMetadata::new(event_id!("$unknown").to_owned())), + } +} + +fn create_v3_pdu(builder: CommonPDUBuilder) -> v3::PDU { + v3::PDU { + auth_events: builder + .auth_events + .expect("auth_events must be provided for a V3 PDU"), + content: builder + .content + .expect("content must be provided for a V3 PDU"), + depth: builder.depth.expect("depth must be provided for a V3 PDU"), + hashes: builder.hashes.unwrap_or_default(), /* We allow default here since we might not + * have hashed yet */ + origin_server_ts: builder + .origin_server_ts + .expect("origin_server_ts must be provided for a V3 PDU"), + prev_events: builder + .prev_events + .expect("prev_events must be provided for a V3 PDU"), + room_id: builder.room_id, + sender: builder + .sender + .expect("sender must be provided for a V3 PDU"), + signatures: builder.signatures.unwrap_or_default(), /* We allow default here since we + * might not have signed yet */ + state_key: builder.state_key, + event_type: builder + .event_type + .expect("event_type must be provided for a V3 PDU"), + unsigned: builder.unsigned, + internal_metadata: builder + .internal_metadata + .unwrap_or_else(|| PduMetadata::new(event_id!("$unknown").to_owned())), + } +} /// Persistent Data Unit (Event) +#[deprecated(note = "Use `v2::PDU` or `v3::PDU` or `ruma::state_res::Event` instead")] #[derive(Clone, Deserialize, Serialize, Debug)] pub struct Pdu { pub event_id: OwnedEventId, @@ -89,7 +274,7 @@ impl Pdu { } } -impl Event for Pdu { +impl crate::Event for Pdu { #[inline] fn auth_events(&self) -> impl DoubleEndedIterator + Clone + Send + '_ { self.auth_events.iter().map(AsRef::as_ref) @@ -157,7 +342,7 @@ impl Event for Pdu { fn into_pdu(self) -> Pdu { self } } -impl Event for &Pdu { +impl crate::Event for &Pdu { #[inline] fn auth_events(&self) -> impl DoubleEndedIterator + Clone + Send + '_ { self.auth_events.iter().map(AsRef::as_ref) diff --git a/src/core/matrix/pdu/hashes.rs b/src/core/matrix/pdu/hashes.rs index aa523ff90..8b6901e74 100644 --- a/src/core/matrix/pdu/hashes.rs +++ b/src/core/matrix/pdu/hashes.rs @@ -3,9 +3,10 @@ use std::collections::HashMap; use serde::{Deserialize, Serialize}; /// The content hash map for a PDU. -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize, Default)] pub struct Hashes { /// The SHA256 content hash. + #[serde(default, skip_serializing_if = "String::is_empty")] pub sha256: String, /// Any other hashes present, if any. diff --git a/src/core/matrix/pdu/metadata.rs b/src/core/matrix/pdu/metadata.rs index 29da2dc2c..dfadb0926 100644 --- a/src/core/matrix/pdu/metadata.rs +++ b/src/core/matrix/pdu/metadata.rs @@ -1,4 +1,4 @@ -use ruma::OwnedEventId; +use ruma::{EventId, OwnedEventId}; use serde::{Deserialize, Serialize}; #[derive(Clone, Debug, Deserialize, Serialize)] @@ -9,3 +9,8 @@ pub struct PduMetadata { /// Indicates whether the event has been rejected under auth rules. pub rejected: bool, } + +impl PduMetadata { + /// Creates a new PduMetadata instance with the minimum required fields. + pub fn new(event_id: OwnedEventId) -> Self { Self { event_id, rejected: false } } +} diff --git a/src/core/matrix/pdu/v2.rs b/src/core/matrix/pdu/v2.rs index 5af84db59..633b82937 100644 --- a/src/core/matrix/pdu/v2.rs +++ b/src/core/matrix/pdu/v2.rs @@ -67,6 +67,14 @@ impl PDU { pub fn internal_metadata(&self) -> &PduMetadata { &self.internal_metadata } } +impl crate::pdu::ConcretePDU for PDU { + type Concrete = PDU; + + fn to_concrete(&self) -> &Self::Concrete { &self } + + fn into_concrete(self) -> Self::Concrete { self } +} + impl ruma::state_res::Event for PDU { type Id = OwnedEventId; diff --git a/src/core/matrix/pdu/v3.rs b/src/core/matrix/pdu/v3.rs index 667866298..e41a7e5af 100644 --- a/src/core/matrix/pdu/v3.rs +++ b/src/core/matrix/pdu/v3.rs @@ -65,6 +65,12 @@ impl PDU { pub fn internal_metadata(&self) -> &PduMetadata { &self.internal_metadata } } +impl crate::pdu::ConcretePDU for PDU { + fn to_concrete(&self) -> &PDU { &self } + + fn into_concrete(self) -> PDU { self } +} + impl ruma::state_res::Event for PDU { type Id = OwnedEventId; diff --git a/src/service/rooms/timeline/create.rs b/src/service/rooms/timeline/create.rs index 2f05ca8cc..737e468f5 100644 --- a/src/service/rooms/timeline/create.rs +++ b/src/service/rooms/timeline/create.rs @@ -1,6 +1,6 @@ use std::{cmp, collections::HashMap}; -use conduwuit::{smallstr::SmallString, trace}; +use conduwuit::{pdu::AnyPDU, smallstr::SmallString, trace}; use conduwuit_core::{ Err, Error, Result, err, matrix::{ @@ -84,7 +84,7 @@ impl super::Service { sender: &UserId, room_id: Option<&RoomId>, _mutex_lock: &RoomMutexGuard, - ) -> Result<(PduEvent, RoomVersionRules)> { + ) -> Result { let PartialPdu { event_type, content, @@ -282,7 +282,7 @@ impl super::Service { sender: &UserId, room_id: Option<&RoomId>, mutex_lock: &RoomMutexGuard, - ) -> Result<(PduEvent, CanonicalJsonObject)> { + ) -> Result { if !self.services.globals.user_is_local(sender) { return Err!(Request(Forbidden("Sender must be a local user"))); }