refactor(base): Don't drop whole m.room.create if predecessor is invalid

The `m.room.create` event contains at lot of important information for a
room, like its type (i.e. whether it is a space), its version and its
creator(s) (which are important in room version 12). So ignoring the
event completely might break a room.

Instead what we do here is simply ignore the `predecessor` field if it
is considered invalid, allowing us to access the other fields.

Signed-off-by: Kévin Commaille <[email protected]>

Author: zecakeh

crates/matrix-sdk-base/src/response_processors/state_events.rs

@@ -107,24 +107,16 @@ pub mod sync {
                 }
 
                 AnySyncStateEvent::RoomCreate(create) => {
-                    if super::is_create_event_valid(
+                    let edited_create = super::validate_create_event_predecessor(
                         context,
                         room_info.room_id(),
                         create,
                         state_store,
-                    ) {
-                        room_info.handle_state_event(event);
-                    } else {
-                        error!(
-                            room_id = ?room_info.room_id(),
-                            ?create,
-                            "`m.create.tombstone` event is invalid, it creates a loop"
-                        );
+                    );
 
-                        // Do not add the event to `room_info`.
-                        // Do not add the event to `context.state_changes.state`.
-                        continue;
-                    }
+                    room_info.handle_state_event(
+                        edited_create.map(Into::into).as_ref().unwrap_or(event),
+                    );
                 }
 
                 AnySyncStateEvent::RoomTombstone(tombstone) => {
@@ -317,31 +309,47 @@ where
         .unzip()
 }
 
-/// Check if `m.room.create` isn't creating a loop of rooms.
-pub fn is_create_event_valid(
+/// Check if the `predecessor` in `m.room.create` isn't creating a loop of
+/// rooms.
+///
+/// If it is, we return a clone of the event with the predecessor removed.
+pub fn validate_create_event_predecessor(
     context: &mut Context,
     room_id: &RoomId,
     event: &SyncStateEvent<RoomCreateEventContent>,
     state_store: &BaseStateStore,
-) -> bool {
+) -> Option<SyncStateEvent<RoomCreateEventContent>> {
     let mut already_seen = BTreeSet::new();
     already_seen.insert(room_id.to_owned());
 
-    let Some(mut predecessor_room_id) = event
-        .as_original()
-        .and_then(|event| Some(event.content.predecessor.as_ref()?.room_id.clone()))
+    // Redacted and non-redacted create events use the same content type.
+    let content = match event {
+        SyncStateEvent::Original(event) => &event.content,
+        SyncStateEvent::Redacted(event) => &event.content,
+    };
+
+    let Some(mut predecessor_room_id) =
+        content.predecessor.as_ref().map(|predecessor| predecessor.room_id.clone())
     else {
-        // `true` means no problem. No predecessor = no problem here.
-        return true;
+        // No predecessor = no problem here.
+        return None;
     };
 
     loop {
         // We must check immediately if the `predecessor_room_id` is in `already_seen`
         // in case of a room is created and marks itself as its predecessor in a single
         // sync.
-        if already_seen.contains(AsRef::<RoomId>::as_ref(&predecessor_room_id)) {
+        if already_seen.contains(&predecessor_room_id) {
             // Ahhh, there is a loop with `m.room.create` events!
-            return false;
+            // We remove the predecessor so that we don't process it later.
+            let mut event = event.clone();
+
+            match &mut event {
+                SyncStateEvent::Original(event) => event.content.predecessor.take(),
+                SyncStateEvent::Redacted(event) => event.content.predecessor.take(),
+            };
+
+            return Some(event);
         }
 
         already_seen.insert(predecessor_room_id.clone());
@@ -366,7 +374,7 @@ pub fn is_create_event_valid(
         predecessor_room_id = next_predecessor_room_id;
     }
 
-    true
+    None
 }
 
 /// Check if `m.room.tombstone` isn't creating a loop of rooms.
@@ -420,6 +428,7 @@ pub fn is_tombstone_event_valid(
 
 #[cfg(test)]
 mod tests {
+    use assert_matches2::assert_matches;
     use matrix_sdk_test::{
         DEFAULT_TEST_ROOM_ID, JoinedRoomBuilder, StateTestEvent, SyncResponseBuilder, async_test,
         event_factory::EventFactory,
@@ -920,11 +929,12 @@ mod tests {
             // The sync doesn't fail but…
             assert!(client.receive_sync_response(response).await.is_ok());
 
-            // … the state event has not been saved.
+            // … the predecessor has not been saved.
             let room_0 = client.get_room(room_id_0).unwrap();
 
             assert!(room_0.predecessor_room().is_none(), "room 0 must not have a predecessor");
             assert!(room_0.successor_room().is_none(), "room 0 must not have a successor");
+            assert_matches!(room_0.create_content(), Some(_), "room 0 must have a create content");
         }
     }
 
@@ -961,11 +971,12 @@ mod tests {
             // The sync doesn't fail but…
             assert!(client.receive_sync_response(response).await.is_ok());
 
-            // … the state event has not been saved.
+            // … the tombstone event and the predecessor have not been saved.
             let room_0 = client.get_room(room_id_0).unwrap();
 
             assert!(room_0.predecessor_room().is_none(), "room 0 must not have a predecessor");
             assert!(room_0.successor_room().is_none(), "room 0 must not have a successor");
+            assert_matches!(room_0.create_content(), Some(_), "room 0 must have a create content");
         }
     }
 
@@ -1070,6 +1081,7 @@ mod tests {
             // this state event is missing because it creates a loop
             assert!(room_2.predecessor_room().is_none(), "room 2 must not have a predecessor");
             assert!(room_2.successor_room().is_none(), "room 2 must not have a successor",);
+            assert_matches!(room_2.create_content(), Some(_), "room 2 must have a create content");
         }
     }