Minor fix on join member auth rules

Previous code was not checking/enforcing rule 4.3.1
(https://spec.matrix.org/v1.2/rooms/v9/), which states that a room
member event (with a join membership) must be accepted only if it contains
a previous event which `state_key` is the room creator.

On top of that, it simplifies the public interface for `auth_rules`,
removing the need of (externally) compute `prev_event`, which, as a side
effect, should reduce the amount of times the previous event needs to be
fetched: It will only load it if the authorized event is a `m.room_member`
with a `Join` state.

Finally, I've splitted the join conditions so it's (hopefully) more
readable and apply auth rules in the same order as they
appear in the spec.

crates/ruma-state-res/CHANGELOG.md

@@ -1,5 +1,11 @@
 # [unreleased]
 
+Breaking changes:
+
+* `auth_check` does not require `prev_event` parameter. It was only required on
+  some specific cases. Previous event is now calculated on demand only when
+  it's required. 
+
 # 0.6.0
 
 Breaking changes:

crates/ruma-state-res/src/event_auth.rs

@@ -108,7 +108,6 @@ pub fn auth_types_for_event(
 pub fn auth_check<E: Event>(
     room_version: &RoomVersion,
     incoming_event: impl Event,
-    prev_event: Option<impl Event>,
     current_third_party_invite: Option<impl Event>,
     fetch_state: impl Fn(&EventType, &str) -> Option<E>,
 ) -> Result<bool> {
@@ -192,14 +191,15 @@ pub fn auth_check<E: Event>(
     }
     */
 
-    let room_create_event = fetch_state(&EventType::RoomCreate, "");
-
     // 3. If event does not have m.room.create in auth_events reject
-    if room_create_event.is_none() {
-        warn!("no m.room.create event in auth chain");
+    let room_create_event = match fetch_state(&EventType::RoomCreate, "") {
+        Some(event) => event,
+        None => {
+            warn!("no m.room.create event in auth chain");
 
-        return Ok(false);
-    }
+            return Ok(false);
+        }
+    };
 
     // [synapse] checks for federation here
 
@@ -256,13 +256,13 @@ pub fn auth_check<E: Event>(
             fetch_state(&EventType::RoomMember, target_user.as_str()).as_ref(),
             sender,
             sender_member_event.as_ref(),
-            incoming_event.content(),
-            prev_event,
+            &incoming_event,
             current_third_party_invite,
             power_levels_event.as_ref(),
             fetch_state(&EventType::RoomJoinRules, "").as_ref(),
             join_authed_user.as_deref(),
             join_authed_user_membership,
+            room_create_event,
         )? {
             return Ok(false);
         }
@@ -304,8 +304,8 @@ pub fn auth_check<E: Event>(
         }
     } else {
         // If no power level event found the creator gets 100 everyone else gets 0
-        room_create_event
-            .and_then(|create| from_json_str::<RoomCreateEventContent>(create.content().get()).ok())
+        from_json_str::<RoomCreateEventContent>(room_create_event.content().get())
+            .ok()
             .and_then(|create| (create.creator == *sender).then(|| int!(100)))
             .unwrap_or_default()
     };
@@ -387,7 +387,6 @@ pub fn auth_check<E: Event>(
 /// Does the user who sent this member event have required power levels to do so.
 ///
 /// * `user` - Information about the membership event and user making the request.
-/// * `prev_event` - The event that occurred immediately before the `user` event or None.
 /// * `auth_events` - The set of auth events that relate to a membership event.
 ///
 /// This is generated by calling `auth_types_for_event` with the membership event and the current
@@ -399,18 +398,19 @@ fn valid_membership_change(
     target_user_membership_event: Option<impl Event>,
     sender: &UserId,
     sender_membership_event: Option<impl Event>,
-    content: &RawJsonValue,
-    prev_event: Option<impl Event>,
+    current_event: impl Event,
     current_third_party_invite: Option<impl Event>,
     power_levels_event: Option<impl Event>,
     join_rules_event: Option<impl Event>,
     authed_user_id: Option<&UserId>,
     auth_user_membership: Option<MembershipState>,
+    create_room: impl Event,
 ) -> Result<bool> {
     #[derive(Deserialize)]
     struct GetThirdPartyInvite {
         third_party_invite: Option<Raw<ThirdPartyInvite>>,
     }
+    let content = current_event.content();
 
     let target_membership = from_json_str::<GetMembership>(content.get())?.membership;
     let third_party_invite =
@@ -446,12 +446,6 @@ fn valid_membership_change(
         join_rules = from_json_str::<RoomJoinRulesEventContent>(jr.content().get())?.join_rule;
     }
 
-    if let Some(prev) = prev_event {
-        if *prev.event_type() == EventType::RoomCreate && prev.prev_events().next().is_none() {
-            return Ok(true);
-        }
-    }
-
     let power_levels_event_id = power_levels_event.as_ref().map(|e| e.event_id());
     let sender_membership_event_id = sender_membership_event.as_ref().map(|e| e.event_id());
     let target_user_membership_event_id =
@@ -496,6 +490,23 @@ fn valid_membership_change(
 
     Ok(match target_membership {
         MembershipState::Join => {
+            let mut prev_events = current_event.prev_events();
+
+            let prev_event_is_create_event = prev_events
+                .next()
+                .map(|event_id| event_id.borrow() == create_room.event_id().borrow())
+                .unwrap_or(false);
+            let no_more_prev_events = prev_events.next().is_none();
+
+            if prev_event_is_create_event && no_more_prev_events {
+                let create_content =
+                    from_json_str::<RoomCreateEventContent>(create_room.content().get())?;
+
+                if create_content.creator == sender && create_content.creator == target_user {
+                    return Ok(true);
+                }
+            }
+
             if sender != target_user {
                 warn!("Can't make other user join");
                 false
@@ -503,27 +514,33 @@ fn valid_membership_change(
                 warn!(?target_user_membership_event_id, "Banned user can't join");
                 false
             } else {
-                let allow = join_rules == JoinRule::Invite
+                let invite_allowed = join_rules == JoinRule::Invite
                     && (target_user_current_membership == MembershipState::Join
-                        || target_user_current_membership == MembershipState::Invite)
-                    || join_rules == JoinRule::Public
-                    || room_version.restricted_join_rules
-                        // 0. room version of 8 ^ and join rule of restricted
-                        && restricted
-                        // 1. The user's previous membership was invite or join
-                        && allow_based_on_membership
-                        // 2. The join event has a valid signature from a homeserver whose
-                        // users have the power to issue invites or the field was `None`.
-                        && restricted_join_rules_auth;
+                        || target_user_current_membership == MembershipState::Invite);
 
-                if !allow {
-                    warn!(
-                        join_rules_event_id = ?join_rules_event.as_ref().map(|e| e.event_id()),
-                        ?target_user_membership_event_id,
-                        "Can't join if join rules is not public and user is not invited / joined",
-                    );
+                if invite_allowed {
+                    return Ok(true);
                 }
-                allow
+
+                if room_version.restricted_join_rules
+                    && restricted
+                    && allow_based_on_membership
+                    && restricted_join_rules_auth
+                {
+                    return Ok(true);
+                }
+
+                if join_rules == JoinRule::Public {
+                    return Ok(true);
+                }
+
+                warn!(
+                    join_rules_event_id = ?join_rules_event.as_ref().map(|e| e.event_id()),
+                    ?target_user_membership_event_id,
+                    "Can't join if join rules is not public and user is not invited / joined",
+                );
+
+                false
             }
         }
         MembershipState::Invite => {
@@ -943,8 +960,8 @@ mod tests {
     use crate::{
         event_auth::valid_membership_change,
         test_utils::{
-            alice, bob, charlie, ella, event_id, member_content_ban, room_id, to_pdu_event,
-            StateEvent, INITIAL_EVENTS,
+            alice, bob, charlie, ella, event_id, member_content_ban, member_content_join, room_id,
+            to_pdu_event, StateEvent, INITIAL_EVENTS, INITIAL_EVENTS_CREATE_ROOM,
         },
         Event, RoomVersion, StateMap,
     };
@@ -955,9 +972,6 @@ mod tests {
             tracing::subscriber::set_default(tracing_subscriber::fmt().with_test_writer().finish());
         let events = INITIAL_EVENTS();
 
-        let prev_event =
-            events.values().find(|ev| ev.event_id.as_str().contains("IMC")).map(Arc::clone);
-
         let auth_events = events
             .values()
             .map(|ev| {
@@ -985,13 +999,101 @@ mod tests {
             fetch_state(EventType::RoomMember, target_user.to_string()),
             &sender,
             fetch_state(EventType::RoomMember, sender.to_string()),
-            requester.content(),
-            prev_event,
+            &requester,
             None::<StateEvent>,
             fetch_state(EventType::RoomPowerLevels, "".to_owned()),
             fetch_state(EventType::RoomJoinRules, "".to_owned()),
             None,
             None,
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
+        )
+        .unwrap());
+    }
+
+    #[test]
+    fn test_join_non_creator() {
+        let _ =
+            tracing::subscriber::set_default(tracing_subscriber::fmt().with_test_writer().finish());
+        let events = INITIAL_EVENTS_CREATE_ROOM();
+
+        let auth_events = events
+            .values()
+            .map(|ev| {
+                ((ev.event_type().to_owned(), ev.state_key().unwrap().to_owned()), Arc::clone(ev))
+            })
+            .collect::<StateMap<_>>();
+
+        let requester = to_pdu_event(
+            "HELLO",
+            charlie(),
+            EventType::RoomMember,
+            Some(charlie().as_str()),
+            member_content_join(),
+            &["CREATE"],
+            &["CREATE"],
+        );
+
+        let fetch_state = |ty, key| auth_events.get(&(ty, key)).cloned();
+        let target_user = charlie();
+        let sender = charlie();
+
+        assert!(!valid_membership_change(
+            &RoomVersion::V6,
+            &target_user,
+            fetch_state(EventType::RoomMember, target_user.to_string()),
+            &sender,
+            fetch_state(EventType::RoomMember, sender.to_string()),
+            &requester,
+            None::<StateEvent>,
+            fetch_state(EventType::RoomPowerLevels, "".to_owned()),
+            fetch_state(EventType::RoomJoinRules, "".to_owned()),
+            None,
+            None,
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
+        )
+        .unwrap());
+    }
+
+    #[test]
+    fn test_join_creator() {
+        let _ =
+            tracing::subscriber::set_default(tracing_subscriber::fmt().with_test_writer().finish());
+        let events = INITIAL_EVENTS_CREATE_ROOM();
+
+        let auth_events = events
+            .values()
+            .map(|ev| {
+                ((ev.event_type().to_owned(), ev.state_key().unwrap().to_owned()), Arc::clone(ev))
+            })
+            .collect::<StateMap<_>>();
+
+        let requester = to_pdu_event(
+            "HELLO",
+            alice(),
+            EventType::RoomMember,
+            Some(alice().as_str()),
+            member_content_join(),
+            &["CREATE"],
+            &["CREATE"],
+        );
+
+        let fetch_state = |ty, key| auth_events.get(&(ty, key)).cloned();
+        let target_user = alice();
+        let sender = alice();
+
+        assert!(valid_membership_change(
+            &RoomVersion::V6,
+            &target_user,
+            fetch_state(EventType::RoomMember, target_user.to_string()),
+            &sender,
+            fetch_state(EventType::RoomMember, sender.to_string()),
+            &requester,
+            None::<StateEvent>,
+            fetch_state(EventType::RoomPowerLevels, "".to_owned()),
+            fetch_state(EventType::RoomJoinRules, "".to_owned()),
+            None,
+            None,
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
         )
         .unwrap());
     }
@@ -1002,9 +1104,6 @@ mod tests {
             tracing::subscriber::set_default(tracing_subscriber::fmt().with_test_writer().finish());
         let events = INITIAL_EVENTS();
 
-        let prev_event =
-            events.values().find(|ev| ev.event_id.as_str().contains("IMC")).map(Arc::clone);
-
         let auth_events = events
             .values()
             .map(|ev| {
@@ -1032,13 +1131,13 @@ mod tests {
             fetch_state(EventType::RoomMember, target_user.to_string()),
             &sender,
             fetch_state(EventType::RoomMember, sender.to_string()),
-            requester.content(),
-            prev_event,
+            &requester,
             None::<StateEvent>,
             fetch_state(EventType::RoomPowerLevels, "".to_owned()),
             fetch_state(EventType::RoomJoinRules, "".to_owned()),
             None,
             None,
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
         )
         .unwrap());
     }
@@ -1078,9 +1177,6 @@ mod tests {
             ),
         );
 
-        let prev_event =
-            events.values().find(|ev| ev.event_id.as_str().contains("IMC")).map(Arc::clone);
-
         let auth_events = events
             .values()
             .map(|ev| {
@@ -1108,13 +1204,13 @@ mod tests {
             fetch_state(EventType::RoomMember, target_user.to_string()),
             &sender,
             fetch_state(EventType::RoomMember, sender.to_string()),
-            requester.content(),
-            prev_event.clone(),
+            &requester,
             None::<StateEvent>,
             fetch_state(EventType::RoomPowerLevels, "".to_owned()),
             fetch_state(EventType::RoomJoinRules, "".to_owned()),
             Some(&alice()),
             Some(MembershipState::Join),
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
         )
         .unwrap());
 
@@ -1124,13 +1220,13 @@ mod tests {
             fetch_state(EventType::RoomMember, target_user.to_string()),
             &sender,
             fetch_state(EventType::RoomMember, sender.to_string()),
-            requester.content(),
-            prev_event,
+            &requester,
             None::<StateEvent>,
             fetch_state(EventType::RoomPowerLevels, "".to_owned()),
             fetch_state(EventType::RoomJoinRules, "".to_owned()),
             Some(&ella()),
             Some(MembershipState::Join),
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
         )
         .unwrap());
     }
@@ -1150,9 +1246,6 @@ mod tests {
             &["IPOWER"],
         );
 
-        let prev_event =
-            events.values().find(|ev| ev.event_id.as_str().contains("IMC")).map(Arc::clone);
-
         let auth_events = events
             .values()
             .map(|ev| {
@@ -1180,13 +1273,13 @@ mod tests {
             fetch_state(EventType::RoomMember, target_user.to_string()),
             &sender,
             fetch_state(EventType::RoomMember, sender.to_string()),
-            requester.content(),
-            prev_event,
+            &requester,
             None::<StateEvent>,
             fetch_state(EventType::RoomPowerLevels, "".to_owned()),
             fetch_state(EventType::RoomJoinRules, "".to_owned()),
             None,
             None,
+            fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(),
         )
         .unwrap());
     }

crates/ruma-state-res/src/lib.rs

@@ -456,22 +456,15 @@ fn iterative_auth_check<E: Event + Clone>(
 
         debug!("event to check {:?}", event.event_id());
 
-        let most_recent_prev_event =
-            event.prev_events().filter_map(|id| fetch_event(id.borrow())).next_back();
-
         // The key for this is (eventType + a state_key of the signed token not sender) so
         // search for it
         let current_third_party = auth_events.iter().find_map(|(_, pdu)| {
             (*pdu.event_type() == EventType::RoomThirdPartyInvite).then(|| pdu)
         });
 
-        if auth_check(
-            room_version,
-            &event,
-            most_recent_prev_event.as_ref(),
-            current_third_party,
-            |ty, key| auth_events.get(&(ty.clone(), key.to_owned())),
-        )? {
+        if auth_check(room_version, &event, current_third_party, |ty, key| {
+            auth_events.get(&(ty.clone(), key.to_owned()))
+        })? {
             // add event to resolved state map
             resolved_state
                 .insert((event.event_type().to_owned(), state_key.to_owned()), event_id.clone());

crates/ruma-state-res/src/test_utils.rs

@@ -536,6 +536,23 @@ pub fn INITIAL_EVENTS() -> HashMap<Box<EventId>, Arc<StateEvent>> {
     .collect()
 }
 
+// all graphs start with these input events
+#[allow(non_snake_case)]
+pub fn INITIAL_EVENTS_CREATE_ROOM() -> HashMap<Box<EventId>, Arc<StateEvent>> {
+    vec![to_pdu_event::<&EventId>(
+        "CREATE",
+        alice(),
+        EventType::RoomCreate,
+        Some(""),
+        to_raw_json_value(&json!({ "creator": alice() })).unwrap(),
+        &[],
+        &[],
+    )]
+    .into_iter()
+    .map(|ev| (ev.event_id().to_owned(), ev))
+    .collect()
+}
+
 #[allow(non_snake_case)]
 pub fn INITIAL_EDGES() -> Vec<Box<EventId>> {
     vec!["START", "IMC", "IMB", "IJR", "IPOWER", "IMA", "CREATE"]