From fca0f6a22b114ca274a046379b290836bda332ff Mon Sep 17 00:00:00 2001 From: gnieto Date: Tue, 8 Mar 2022 08:27:47 +0100 Subject: [PATCH] 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 | 6 + crates/ruma-state-res/src/event_auth.rs | 217 +++++++++++++++++------- crates/ruma-state-res/src/lib.rs | 13 +- crates/ruma-state-res/src/test_utils.rs | 17 ++ 4 files changed, 181 insertions(+), 72 deletions(-) diff --git a/crates/ruma-state-res/CHANGELOG.md b/crates/ruma-state-res/CHANGELOG.md index ff011a7d..46e95305 100644 --- a/crates/ruma-state-res/CHANGELOG.md +++ b/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: diff --git a/crates/ruma-state-res/src/event_auth.rs b/crates/ruma-state-res/src/event_auth.rs index 413636a9..339d28e2 100644 --- a/crates/ruma-state-res/src/event_auth.rs +++ b/crates/ruma-state-res/src/event_auth.rs @@ -108,7 +108,6 @@ pub fn auth_types_for_event( pub fn auth_check( room_version: &RoomVersion, incoming_event: impl Event, - prev_event: Option, current_third_party_invite: Option, fetch_state: impl Fn(&EventType, &str) -> Option, ) -> Result { @@ -192,14 +191,15 @@ pub fn auth_check( } */ - 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( 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( } } else { // If no power level event found the creator gets 100 everyone else gets 0 - room_create_event - .and_then(|create| from_json_str::(create.content().get()).ok()) + from_json_str::(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( /// 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, sender: &UserId, sender_membership_event: Option, - content: &RawJsonValue, - prev_event: Option, + current_event: impl Event, current_third_party_invite: Option, power_levels_event: Option, join_rules_event: Option, authed_user_id: Option<&UserId>, auth_user_membership: Option, + create_room: impl Event, ) -> Result { #[derive(Deserialize)] struct GetThirdPartyInvite { third_party_invite: Option>, } + let content = current_event.content(); let target_membership = from_json_str::(content.get())?.membership; let third_party_invite = @@ -446,12 +446,6 @@ fn valid_membership_change( join_rules = from_json_str::(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::(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::, 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::>(); + + 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::, + 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::>(); + + 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::, + 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::, 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::, 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::, 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::, fetch_state(EventType::RoomPowerLevels, "".to_owned()), fetch_state(EventType::RoomJoinRules, "".to_owned()), None, None, + fetch_state(EventType::RoomCreate, "".to_owned()).unwrap(), ) .unwrap()); } diff --git a/crates/ruma-state-res/src/lib.rs b/crates/ruma-state-res/src/lib.rs index b8634296..caad6a27 100644 --- a/crates/ruma-state-res/src/lib.rs +++ b/crates/ruma-state-res/src/lib.rs @@ -456,22 +456,15 @@ fn iterative_auth_check( 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()); diff --git a/crates/ruma-state-res/src/test_utils.rs b/crates/ruma-state-res/src/test_utils.rs index fa4258cc..10402462 100644 --- a/crates/ruma-state-res/src/test_utils.rs +++ b/crates/ruma-state-res/src/test_utils.rs @@ -536,6 +536,23 @@ pub fn INITIAL_EVENTS() -> HashMap, Arc> { .collect() } +// all graphs start with these input events +#[allow(non_snake_case)] +pub fn INITIAL_EVENTS_CREATE_ROOM() -> HashMap, Arc> { + 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> { vec!["START", "IMC", "IMB", "IJR", "IPOWER", "IMA", "CREATE"]