fix room version comparisons

Fixes a set of bugs introduced by 00b77144c1,
where we replaced explicit `RoomVersionId` matches with `version < V11`
comparisons. The `Ord` impl on `RoomVersionId` does not work like that,
and is in fact a lexicographic string comparison[1]. The most visible
effect of these bugs is that incoming redaction events would sometimes
be ignored.

Instead of reverting to the explicit matches, which were quite verbose,
I implemented a `RoomVersion` struct that has flags for each property
that we care about. This is similar to the approach used by ruma[2] and
synapse[3].

[1]: 7cfa3be0c6/crates/ruma-common/src/identifiers/room_version_id.rs (L136)
[2]: 7cfa3be0c6/crates/ruma-state-res/src/room_version.rs
[3]: c856ae4724/synapse/api/room_versions.py

src/api/client_server/membership.rs

@@ -1032,8 +1032,9 @@ async fn join_room_by_id_helper(
 
         info!("Running send_join auth check");
         let authenticated = state_res::event_auth::auth_check(
-            &state_res::RoomVersion::new(&room_version_id)
+            &state_res::RoomVersion::new(&room_version_id).map_err(|_| {
-                .expect("room version is supported"),
+                Error::UnsupportedRoomVersion(room_version_id.clone())
+            })?,
             &parsed_join_pdu,
             // TODO: third party invite
             None::<PduEvent>,

src/api/client_server/room.rs

@@ -24,14 +24,14 @@ use ruma::{
     },
     int,
     serde::JsonObject,
-    CanonicalJsonObject, OwnedRoomAliasId, RoomAliasId, RoomId, RoomVersionId,
+    CanonicalJsonObject, OwnedRoomAliasId, RoomAliasId, RoomId,
 };
 use serde_json::{json, value::to_raw_value};
 use tracing::{info, warn};
 
 use crate::{
-    api::client_server::invite_helper, service::pdu::PduBuilder, services, Ar,
+    api::client_server::invite_helper, service::pdu::PduBuilder, services,
-    Error, Ra, Result,
+    utils::room_version::RoomVersion, Ar, Error, Ra, Result,
 };
 
 /// # `POST /_matrix/client/r0/createRoom`
@@ -114,7 +114,7 @@ pub(crate) async fn create_room_route(
         }
     }
 
-    let room_version = match body.room_version.clone() {
+    let room_version_id = match body.room_version.clone() {
         Some(room_version) => {
             if services()
                 .globals
@@ -131,6 +131,7 @@ pub(crate) async fn create_room_route(
         }
         None => services().globals.default_room_version(),
     };
+    let room_version = RoomVersion::try_from(&room_version_id)?;
 
     let content = match &body.creation_content {
         Some(content) => {
@@ -138,8 +139,7 @@ pub(crate) async fn create_room_route(
                 .deserialize_as::<CanonicalJsonObject>()
                 .expect("Invalid creation content");
 
-            match &room_version {
+            if room_version.create_event_creator_prop {
-                room_version if *room_version < RoomVersionId::V11 => {
                 content.insert(
                     "creator".into(),
                     json!(&sender_user).try_into().map_err(|_| {
@@ -150,18 +150,9 @@ pub(crate) async fn create_room_route(
                     })?,
                 );
             }
-                // V11 removed the "creator" key
-                RoomVersionId::V11 => {}
-                _ => {
-                    return Err(Error::BadServerResponse(
-                        "Unsupported room version.",
-                    ))
-                }
-            }
-
             content.insert(
                 "room_version".into(),
-                json!(room_version.as_str()).try_into().map_err(|_| {
+                json!(room_version_id.as_str()).try_into().map_err(|_| {
                     Error::BadRequest(
                         ErrorKind::BadJson,
                         "Invalid creation content",
@@ -171,16 +162,10 @@ pub(crate) async fn create_room_route(
             content
         }
         None => {
-            let content = match &room_version {
+            let content = if room_version.create_event_creator_prop {
-                room_version if *room_version < RoomVersionId::V11 => {
                 RoomCreateEventContent::new_v1(sender_user.to_owned())
-                }
+            } else {
-                RoomVersionId::V11 => RoomCreateEventContent::new_v11(),
+                RoomCreateEventContent::new_v11()
-                _ => {
-                    return Err(Error::BadServerResponse(
-                        "Unsupported room version.",
-                    ))
-                }
             };
             let mut content = serde_json::from_str::<CanonicalJsonObject>(
                 to_raw_value(&content)
@@ -195,7 +180,7 @@ pub(crate) async fn create_room_route(
             .unwrap();
             content.insert(
                 "room_version".into(),
-                json!(room_version.as_str()).try_into().map_err(|_| {
+                json!(room_version_id.as_str()).try_into().map_err(|_| {
                     Error::BadRequest(
                         ErrorKind::BadJson,
                         "Invalid creation content",
@@ -598,6 +583,7 @@ pub(crate) async fn upgrade_room_route(
             "This server does not support that room version.",
         ));
     }
+    let new_version = RoomVersion::try_from(&body.new_version)?;
 
     // Create a replacement room
     let replacement_room = RoomId::new(services().globals.server_name());
@@ -661,8 +647,7 @@ pub(crate) async fn upgrade_room_route(
 
     // Send a m.room.create event containing a predecessor field and the
     // applicable room_version
-    match &body.new_version {
+    if new_version.create_event_creator_prop {
-        room_version if *room_version < RoomVersionId::V11 => {
         create_event_content.insert(
             "creator".into(),
             json!(&sender_user).try_into().map_err(|_| {
@@ -672,13 +657,9 @@ pub(crate) async fn upgrade_room_route(
                 )
             })?,
         );
-        }
+    } else {
-        RoomVersionId::V11 => {
-            // "creator" key no longer exists in V11 rooms
         create_event_content.remove("creator");
     }
-        _ => return Err(Error::BadServerResponse("Unsupported room version.")),
-    }
     create_event_content.insert(
         "room_version".into(),
         json!(&body.new_version).try_into().map_err(|_| {

src/service/admin.rs

@@ -34,7 +34,7 @@ use super::pdu::PduBuilder;
 use crate::{
     api::client_server::{leave_all_rooms, AUTO_GEN_PASSWORD_LENGTH},
     services,
-    utils::{self, dbg_truncate_str},
+    utils::{self, dbg_truncate_str, room_version::RoomVersion},
     Error, PduEvent, Result,
 };
 
@@ -1301,23 +1301,18 @@ impl Service {
 
         services().users.create(&services().globals.admin_bot_user_id, None)?;
 
-        let room_version = services().globals.default_room_version();
+        let room_version_id = services().globals.default_room_version();
-        let mut content = match &room_version {
+        let room_version = RoomVersion::try_from(&room_version_id)?;
-            room_version if *room_version < RoomVersionId::V11 => {
+        let mut content = if room_version.create_event_creator_prop {
             RoomCreateEventContent::new_v1(
                 services().globals.admin_bot_user_id.clone(),
             )
-            }
+        } else {
-            RoomVersionId::V11 => RoomCreateEventContent::new_v11(),
+            RoomCreateEventContent::new_v11()
-            _ => {
-                return Err(Error::BadServerResponse(
-                    "Unsupported room version.",
-                ))
-            }
         };
         content.federate = true;
         content.predecessor = None;
-        content.room_version = room_version;
+        content.room_version = room_version_id;
 
         // 1. The room create event
         services()

src/service/rooms/event_handler.rs

@@ -27,7 +27,7 @@ use ruma::{
         StateEventType, TimelineEventType,
     },
     int,
-    state_res::{self, RoomVersion, StateMap},
+    state_res::{self, StateMap},
     uint, CanonicalJsonObject, CanonicalJsonValue, EventId,
     MilliSecondsSinceUnixEpoch, OwnedServerName, OwnedServerSigningKeyId,
     RoomId, RoomVersionId, ServerName,
@@ -44,7 +44,7 @@ use super::{
 use crate::{
     service::{globals::SigningKeys, pdu},
     services,
-    utils::debug_slice_truncated,
+    utils::{debug_slice_truncated, room_version::RoomVersion},
     Error, PduEvent, Result,
 };
 
@@ -339,8 +339,10 @@ impl Service {
                 )?;
 
             let room_version_id = &create_event_content.room_version;
-            let room_version = RoomVersion::new(room_version_id)
+            let ruma_room_version =
-                .expect("room version is supported");
+                state_res::RoomVersion::new(room_version_id).map_err(|_| {
+                    Error::UnsupportedRoomVersion(room_version_id.clone())
+                })?;
 
             // TODO: For RoomVersion6 we must check that Raw<..> is canonical,
             // do we anywhere?
@@ -527,7 +529,7 @@ impl Service {
             }
 
             if !state_res::event_auth::auth_check(
-                &room_version,
+                &ruma_room_version,
                 &incoming_pdu,
                 // TODO: third party invite
                 None::<PduEvent>,
@@ -601,8 +603,11 @@ impl Service {
             )?;
 
         let room_version_id = &create_event_content.room_version;
-        let room_version = RoomVersion::new(room_version_id)
+        let room_version = RoomVersion::try_from(room_version_id)?;
-            .expect("room version is supported");
+        let ruma_room_version = state_res::RoomVersion::new(room_version_id)
+            .map_err(|_| {
+                Error::UnsupportedRoomVersion(room_version_id.clone())
+            })?;
 
         // 10. Fetch missing state and auth chain events by calling /state_ids
         //     at backwards extremities doing all the checks in this list
@@ -885,7 +890,7 @@ impl Service {
         // 11. Check the auth of the event passes based on the state of the
         //     event
         let check_result = state_res::event_auth::auth_check(
-            &room_version,
+            &ruma_room_version,
             &incoming_pdu,
             // TODO: third party invite
             None::<PduEvent>,
@@ -930,7 +935,7 @@ impl Service {
         )?;
 
         let soft_fail = !state_res::event_auth::auth_check(
-            &room_version,
+            &ruma_room_version,
             &incoming_pdu,
             None::<PduEvent>,
             |k, s| auth_events.get(&(k.clone(), s.to_owned())),
@@ -939,24 +944,10 @@ impl Service {
             Error::BadRequest(ErrorKind::InvalidParam, "Auth check failed.")
         })? || incoming_pdu.kind
             == TimelineEventType::RoomRedaction
-            && match room_version_id {
+            && if room_version.redaction_event_redacts_in_content {
-                room_version if *room_version < RoomVersionId::V11 => {
+                let content =
-                    if let Some(redact_id) = &incoming_pdu.redacts {
+                    serde_json::from_str::<RoomRedactionEventContent>(
-                        !services().rooms.state_accessor.user_can_redact(
+                        incoming_pdu.content.get(),
-                            redact_id,
-                            &incoming_pdu.sender,
-                            &incoming_pdu.room_id,
-                            true,
-                        )?
-                    } else {
-                        false
-                    }
-                }
-                RoomVersionId::V11 => {
-                    let content = serde_json::from_str::<
-                        RoomRedactionEventContent,
-                    >(
-                        incoming_pdu.content.get()
                     )
                     .map_err(|_| {
                         Error::bad_database("Invalid content in redaction pdu.")
@@ -972,12 +963,15 @@ impl Service {
                 } else {
                     false
                 }
-                }
+            } else if let Some(redact_id) = &incoming_pdu.redacts {
-                _ => {
+                !services().rooms.state_accessor.user_can_redact(
-                    return Err(Error::BadServerResponse(
+                    redact_id,
-                        "Unsupported room version.",
+                    &incoming_pdu.sender,
-                    ))
+                    &incoming_pdu.room_id,
-                }
+                    true,
+                )?
+            } else {
+                false
             };
 
         // 13. Use state resolution to find new room state

src/service/rooms/timeline.rs

@@ -21,10 +21,9 @@ use ruma::{
         GlobalAccountDataEventType, StateEventType, TimelineEventType,
     },
     push::{Action, Ruleset, Tweak},
-    state_res::{self, Event, RoomVersion},
+    state_res::{self, Event},
     uint, user_id, CanonicalJsonObject, CanonicalJsonValue, EventId,
-    OwnedEventId, OwnedRoomId, OwnedServerName, RoomId, RoomVersionId,
+    OwnedEventId, OwnedRoomId, OwnedServerName, RoomId, ServerName, UserId,
-    ServerName, UserId,
 };
 use serde::Deserialize;
 use serde_json::value::{to_raw_value, RawValue as RawJsonValue};
@@ -40,7 +39,7 @@ use crate::{
         pdu::{EventHash, PduBuilder},
     },
     services,
-    utils::{self, on_demand_hashmap::KeyToken},
+    utils::{self, on_demand_hashmap::KeyToken, room_version::RoomVersion},
     Error, PduEvent, Result,
 };
 
@@ -410,28 +409,13 @@ impl Service {
             TimelineEventType::RoomRedaction => {
                 let room_version_id =
                     services().rooms.state.get_room_version(&pdu.room_id)?;
-                match &room_version_id {
+                let room_version = RoomVersion::try_from(&room_version_id)?;
-                    room_version if *room_version < RoomVersionId::V11 => {
+                if room_version.redaction_event_redacts_in_content {
-                        if let Some(redact_id) = &pdu.redacts {
+                    let content = serde_json::from_str::<
-                            if services().rooms.state_accessor.user_can_redact(
+                        RoomRedactionEventContent,
-                                redact_id,
+                    >(pdu.content.get())
-                                &pdu.sender,
-                                &pdu.room_id,
-                                false,
-                            )? {
-                                self.redact_pdu(redact_id, pdu, shortroomid)?;
-                            }
-                        }
-                    }
-                    RoomVersionId::V11 => {
-                        let content =
-                            serde_json::from_str::<RoomRedactionEventContent>(
-                                pdu.content.get(),
-                            )
                     .map_err(|_| {
-                                Error::bad_database(
+                        Error::bad_database("Invalid content in redaction pdu.")
-                                    "Invalid content in redaction pdu.",
-                                )
                     })?;
                     if let Some(redact_id) = &content.redacts {
                         if services().rooms.state_accessor.user_can_redact(
@@ -443,11 +427,14 @@ impl Service {
                             self.redact_pdu(redact_id, pdu, shortroomid)?;
                         }
                     }
-                    }
+                } else if let Some(redact_id) = &pdu.redacts {
-                    _ => {
+                    if services().rooms.state_accessor.user_can_redact(
-                        return Err(Error::BadServerResponse(
+                        redact_id,
-                            "Unsupported room version.",
+                        &pdu.sender,
-                        ));
+                        &pdu.room_id,
+                        false,
+                    )? {
+                        self.redact_pdu(redact_id, pdu, shortroomid)?;
                     }
                 };
             }
@@ -737,8 +724,10 @@ impl Service {
                 }
             })?;
 
-        let room_version = RoomVersion::new(&room_version_id)
+        let ruma_room_version = state_res::RoomVersion::new(&room_version_id)
-            .expect("room version is supported");
+            .map_err(|_| {
+            Error::UnsupportedRoomVersion(room_version_id.clone())
+        })?;
 
         let auth_events = services().rooms.state.get_auth_events(
             room_id,
@@ -810,7 +799,7 @@ impl Service {
         };
 
         let auth_check = state_res::auth_check(
-            &room_version,
+            &ruma_room_version,
             &pdu,
             // TODO: third_party_invite
             None::<PduEvent>,
@@ -1008,35 +997,14 @@ impl Service {
         // If redaction event is not authorized, do not append it to the
         // timeline
         if pdu.kind == TimelineEventType::RoomRedaction {
-            match services().rooms.state.get_room_version(&pdu.room_id)? {
+            let room_version_id =
-                RoomVersionId::V1
+                services().rooms.state.get_room_version(&pdu.room_id)?;
-                | RoomVersionId::V2
+            let room_version = RoomVersion::try_from(&room_version_id)?;
-                | RoomVersionId::V3
+            if room_version.redaction_event_redacts_in_content {
-                | RoomVersionId::V4
+                let content =
-                | RoomVersionId::V5
+                    serde_json::from_str::<RoomRedactionEventContent>(
-                | RoomVersionId::V6
+                        pdu.content.get(),
-                | RoomVersionId::V7
+                    )
-                | RoomVersionId::V8
-                | RoomVersionId::V9
-                | RoomVersionId::V10 => {
-                    if let Some(redact_id) = &pdu.redacts {
-                        if !services().rooms.state_accessor.user_can_redact(
-                            redact_id,
-                            &pdu.sender,
-                            &pdu.room_id,
-                            false,
-                        )? {
-                            return Err(Error::BadRequest(
-                                ErrorKind::forbidden(),
-                                "User cannot redact this event.",
-                            ));
-                        }
-                    };
-                }
-                RoomVersionId::V11 => {
-                    let content = serde_json::from_str::<
-                        RoomRedactionEventContent,
-                    >(pdu.content.get())
                     .map_err(|_| {
                         Error::bad_database("Invalid content in redaction pdu.")
                     })?;
@@ -1054,11 +1022,16 @@ impl Service {
                         ));
                     }
                 }
-                }
+            } else if let Some(redact_id) = &pdu.redacts {
-                _ => {
+                if !services().rooms.state_accessor.user_can_redact(
+                    redact_id,
+                    &pdu.sender,
+                    &pdu.room_id,
+                    false,
+                )? {
                     return Err(Error::BadRequest(
-                        ErrorKind::UnsupportedRoomVersion,
+                        ErrorKind::forbidden(),
-                        "Unsupported room version",
+                        "User cannot redact this event.",
                     ));
                 }
             }

src/utils.rs

@@ -1,5 +1,6 @@
 pub(crate) mod error;
 pub(crate) mod on_demand_hashmap;
+pub(crate) mod room_version;
 
 use std::{
     borrow::Cow,

src/utils/error.rs

@@ -80,6 +80,8 @@ pub(crate) enum Error {
     AdminCommand(&'static str),
     #[error("from {0}: {1}")]
     Redaction(OwnedServerName, ruma::canonical_json::RedactionError),
+    #[error("unsupported room version {0}")]
+    UnsupportedRoomVersion(ruma::RoomVersionId),
     #[error("{0} in {1}")]
     InconsistentRoomState(&'static str, ruma::OwnedRoomId),
 }
@@ -146,6 +148,10 @@ impl Error {
                     _ => StatusCode::BAD_REQUEST,
                 },
             ),
+            Self::UnsupportedRoomVersion(_) => (
+                ErrorKind::UnsupportedRoomVersion,
+                StatusCode::INTERNAL_SERVER_ERROR,
+            ),
             Self::Conflict(_) => (Unknown, StatusCode::CONFLICT),
             _ => (Unknown, StatusCode::INTERNAL_SERVER_ERROR),
         };

src/utils/room_version.rs

@@ -0,0 +1,87 @@
+use ruma::RoomVersionId;
+
+use crate::Error;
+
+/// Properties that we care about in grapevine that differ between supported
+/// room versions.
+///
+/// This is similar to [`ruma::state_res::RoomVersion`], except that it has
+/// properties that are relevant to us instead of only properties relevant to
+/// ruma state resolution.
+///
+/// When branching for different room versions, prefer constructing a
+/// `RoomVersion` and branching on it's properties over branching based on the
+/// [`RoomVersionId`] directly. If there is a relevant property that is not
+/// already included in `RoomVersion`, add it. In particular, comparisons like
+/// `room_version_id < RoomVersionId::V11` do not work, because room versions
+/// do not have a defined order. Ruma implements `PartialOrd` on `RoomVersionId`
+/// as a lexicographic string comparison, which is almost certainly not what you
+/// want.
+pub(crate) struct RoomVersion {
+    /// Whether `m.room.create` state events have a `creator` property.
+    ///
+    /// The `creator` property is always equivalent to the event `sender`, and
+    /// was [removed][spec] in v11.
+    ///
+    /// [spec]: https://spec.matrix.org/v1.11/rooms/v11/#remove-the-creator-property-of-mroomcreate-events
+    pub(crate) create_event_creator_prop: bool,
+
+    /// Whether the `redacts` property of `m.room.redaction` events is a
+    /// property in the event `content` or a top-level property of the event.
+    ///
+    /// This property was [moved][spec] from top-level to `content` in v11.
+    ///
+    /// [spec]: https://spec.matrix.org/v1.11/rooms/v11/#moving-the-redacts-property-of-mroomredaction-events-to-a-content-property
+    pub(crate) redaction_event_redacts_in_content: bool,
+}
+
+// Allowed so that we can use struct-update syntax for incremental changes
+// between versions even when all fields change.
+#[allow(clippy::needless_update)]
+mod known_versions {
+    use super::RoomVersion;
+
+    pub(super) const V6: RoomVersion = RoomVersion {
+        create_event_creator_prop: true,
+        redaction_event_redacts_in_content: false,
+    };
+
+    pub(super) const V7: RoomVersion = RoomVersion {
+        ..V6
+    };
+
+    pub(super) const V8: RoomVersion = RoomVersion {
+        ..V7
+    };
+
+    pub(super) const V9: RoomVersion = RoomVersion {
+        ..V8
+    };
+
+    pub(super) const V10: RoomVersion = RoomVersion {
+        ..V9
+    };
+
+    pub(super) const V11: RoomVersion = RoomVersion {
+        create_event_creator_prop: false,
+        redaction_event_redacts_in_content: true,
+        ..V10
+    };
+}
+
+/// May return an error for unsupported room versions.
+impl TryFrom<&RoomVersionId> for RoomVersion {
+    type Error = Error;
+
+    fn try_from(version: &RoomVersionId) -> Result<RoomVersion, Error> {
+        match version {
+            RoomVersionId::V6 => Ok(known_versions::V6),
+            RoomVersionId::V7 => Ok(known_versions::V7),
+            RoomVersionId::V8 => Ok(known_versions::V8),
+            RoomVersionId::V9 => Ok(known_versions::V9),
+            RoomVersionId::V10 => Ok(known_versions::V10),
+            RoomVersionId::V11 => Ok(known_versions::V11),
+            _ => Err(Error::UnsupportedRoomVersion(version.clone())),
+        }
+    }
+}