From aadccdee645d40610d443c2bf5098ad19bdabe63 Mon Sep 17 00:00:00 2001 From: Devin Ragotzy Date: Thu, 27 Aug 2020 09:08:52 -0400 Subject: [PATCH] Fix DM room creator rejoining Check only the previous event is a RoomCreate event not that one exists --- src/event_auth.rs | 69 ++++++++++++++++++++------------------------- src/lib.rs | 10 ++++--- tests/event_auth.rs | 17 ++++++++--- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/src/event_auth.rs b/src/event_auth.rs index e0fa6d0c..0f5f2cb3 100644 --- a/src/event_auth.rs +++ b/src/event_auth.rs @@ -80,7 +80,7 @@ pub fn auth_types_for_event( pub fn auth_check( room_version: &RoomVersionId, event: &StateEvent, - redacted_event: Option<&StateEvent>, + prev_event: Option<&StateEvent>, auth_events: StateMap, do_sig_check: bool, ) -> Result { @@ -197,9 +197,8 @@ pub fn auth_check( // return Ok(false); // and be non-empty state_key (point to a user_id) // } - // TODO what? "sender's domain doesn't matches" // If sender's domain doesn't matches state_key, reject - if event.state_key() != Some(event.sender().to_string()) { + if event.state_key().as_deref() != Some(event.sender().server_name().as_str()) { tracing::warn!("state_key does not match sender"); return Ok(false); } @@ -224,7 +223,7 @@ pub fn auth_check( return Ok(false); } - if !is_membership_change_allowed(event.to_requester(), &auth_events)? { + if !is_membership_change_allowed(event.to_requester(), prev_event, &auth_events)? { return Ok(false); } @@ -275,9 +274,7 @@ pub fn auth_check( } if event.kind() == EventType::RoomRedaction { - if let RedactAllowed::No = - check_redaction(room_version, event, redacted_event, &auth_events)? - { + if let RedactAllowed::No = check_redaction(room_version, event, &auth_events)? { return Ok(false); } } @@ -303,6 +300,7 @@ pub fn can_federate(auth_events: &StateMap) -> bool { /// Does the user who sent this member event have required power levels to do so. pub fn is_membership_change_allowed( user: Requester<'_>, + prev_event: Option<&StateEvent>, auth_events: &StateMap, ) -> Result { let content = @@ -312,7 +310,7 @@ pub fn is_membership_change_allowed( // If the only previous event is an m.room.create and the state_key is the creator, allow if user.prev_event_ids.len() == 1 && membership == MembershipState::Join { - if let Some(create) = auth_events.get(&(EventType::RoomCreate, Some("".into()))) { + if let Some(create) = prev_event { if let Ok(create_ev) = create.deserialize_content::() { if user.state_key == Some(create_ev.creator.to_string()) @@ -371,6 +369,7 @@ pub fn is_membership_change_allowed( .unwrap(), ); + // we already check if the join event was the room creator if membership == MembershipState::Invite && content.third_party_invite.is_some() { // TODO this is unimpled if !verify_third_party_invite(&user, auth_events) { @@ -383,9 +382,30 @@ pub fn is_membership_change_allowed( } tracing::info!("invite succeded"); return Ok(true); - } + } else if membership == MembershipState::Join { + // If the sender does not match state_key, reject. + if user.sender != &target_user_id { + tracing::warn!("cannot force another user to join"); + return Ok(false); // cannot force another user to join + } else if target_banned { + tracing::warn!("cannot join when banned"); + return Ok(false); // cannot joined when banned + } else if join_rule == JoinRule::Invite { + if !caller_in_room && !caller_invited { + tracing::warn!("user has not been invited to this room"); + return Ok(false); // you are not invited to this room + } + } else if join_rule == JoinRule::Public { + tracing::info!("join rule public") + // pass + } else { + tracing::warn!("the join rule is Private or yet to be spec'ed by Matrix"); + // synapse has 2 TODO's may_join list and private rooms - if membership == MembershipState::Invite { + // the join_rule is Private or Knock which means it is not yet spec'ed + return Ok(false); + } + } else if membership == MembershipState::Invite { // if senders current membership is not join reject if !caller_in_room { tracing::warn!("invite sender not in room they are inviting user to"); @@ -407,30 +427,6 @@ pub fn is_membership_change_allowed( return Ok(false); } } - // we already check if the join event was the room creator - } else if membership == MembershipState::Join { - // If the sender does not match state_key, reject. - if user.sender != &target_user_id { - tracing::warn!("cannot force another user to join"); - return Ok(false); // cannot force another user to join - } else if target_banned { - tracing::warn!("cannot join when banned"); - return Ok(false); // cannot joined when banned - } else if join_rule == JoinRule::Public { - tracing::info!("join rule public") - // pass - } else if join_rule == JoinRule::Invite { - if !caller_in_room && !caller_invited { - tracing::warn!("user has not been invited to this room"); - return Ok(false); // you are not invited to this room - } - } else { - tracing::warn!("the join rule is Private or yet to be spec'ed by Matrix"); - // synapse has 2 TODO's may_join list and private rooms - - // the join_rule is Private or Knock which means it is not yet spec'ed - return Ok(false); - } } else if membership == MembershipState::Leave { if user.sender == &target_user_id && !(caller_in_room || caller_invited) {} // if senders current membership is not join reject @@ -670,7 +666,6 @@ fn get_deserialize_levels( pub fn check_redaction( room_version: &RoomVersionId, redaction_event: &StateEvent, - redacted_event: Option<&StateEvent>, auth_events: &StateMap, ) -> Result { let user_level = get_user_power_level(redaction_event.sender(), auth_events); @@ -689,10 +684,6 @@ pub fn check_redaction( tracing::info!("redaction event allowed via room version 1 rules"); return Ok(RedactAllowed::OwnEvent); } - // redactions to events where the sender's domains match, allow - } else if redacted_event.map(|ev| ev.sender()) == Some(redaction_event.sender()) { - tracing::info!("redaction allowed via own redaction"); - return Ok(RedactAllowed::OwnEvent); } Ok(RedactAllowed::No) diff --git a/src/lib.rs b/src/lib.rs index a7b09469..42c9d197 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -553,14 +553,16 @@ impl StateResolution { tracing::debug!("event to check {:?}", event.event_id().to_string()); - let redacted_event = event - .redacts() - .and_then(|id| StateResolution::get_or_load_event(room_id, id, event_map, store)); + let most_recent_prev_event = event + .prev_event_ids() + .iter() + .filter_map(|id| StateResolution::get_or_load_event(room_id, id, event_map, store)) + .next_back(); if event_auth::auth_check( room_version, &event, - redacted_event.as_ref(), + most_recent_prev_event.as_ref(), auth_events, false, )? { diff --git a/tests/event_auth.rs b/tests/event_auth.rs index f9d0c752..7ffa58e7 100644 --- a/tests/event_auth.rs +++ b/tests/event_auth.rs @@ -9,12 +9,13 @@ use ruma::{ }, EventType, }, - identifiers::{EventId, RoomId, RoomVersionId, UserId}, + identifiers::{EventId, RoomId, UserId}, }; use serde_json::{json, Value as JsonValue}; +#[rustfmt::skip] // this deletes the comments for some reason yay! use state_res::{ event_auth::{ - auth_check, auth_types_for_event, can_federate, check_power_levels, check_redaction, + // auth_check, auth_types_for_event, can_federate, check_power_levels, check_redaction, is_membership_change_allowed, }, Requester, StateEvent, StateMap, StateStore, @@ -243,6 +244,10 @@ fn INITIAL_EVENTS() -> BTreeMap { fn test_ban_pass() { let events = INITIAL_EVENTS(); + let prev = events + .values() + .find(|ev| ev.event_id().as_str().contains("IMC")); + let auth_events = events .values() .map(|ev| ((ev.kind(), ev.state_key()), ev.clone())) @@ -256,13 +261,17 @@ fn test_ban_pass() { sender: &alice(), }; - assert!(is_membership_change_allowed(requester, &auth_events).unwrap()) + assert!(is_membership_change_allowed(requester, prev, &auth_events).unwrap()) } #[test] fn test_ban_fail() { let events = INITIAL_EVENTS(); + let prev = events + .values() + .find(|ev| ev.event_id().as_str().contains("IMC")); + let auth_events = events .values() .map(|ev| ((ev.kind(), ev.state_key()), ev.clone())) @@ -276,5 +285,5 @@ fn test_ban_fail() { sender: &charlie(), }; - assert!(!is_membership_change_allowed(requester, &auth_events).unwrap()) + assert!(!is_membership_change_allowed(requester, prev, &auth_events).unwrap()) }