From 451398612a4baebf2fc89381a062d21958a4240e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 9 Jun 2023 15:43:57 +0100 Subject: [PATCH 1/5] In Sliding Sync, find out which rooms we have left from state events Look through m.room.member events when processing Sliding Sync responses, finding those that refer to our user's membership, so that we can recognise which rooms we have left. --- crates/matrix-sdk-base/src/rooms/normal.rs | 21 +++- crates/matrix-sdk-base/src/sliding_sync.rs | 138 ++++++++++++++++++--- 2 files changed, 137 insertions(+), 22 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index e0de85980..1eff6b122 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -26,8 +26,8 @@ use ruma::{ room::{ create::RoomCreateEventContent, encryption::RoomEncryptionEventContent, guest_access::GuestAccess, history_visibility::HistoryVisibility, join_rules::JoinRule, - name::RoomNameEventContent, redaction::OriginalSyncRoomRedactionEvent, - tombstone::RoomTombstoneEventContent, + member::MembershipState, name::RoomNameEventContent, + redaction::OriginalSyncRoomRedactionEvent, tombstone::RoomTombstoneEventContent, }, tag::Tags, AnyRoomAccountDataEvent, AnyStrippedStateEvent, AnySyncStateEvent, @@ -83,6 +83,19 @@ pub enum RoomState { Invited, } +impl From<&MembershipState> for RoomState { + fn from(membership_state: &MembershipState) -> Self { + match membership_state { + MembershipState::Ban => Self::Left, // TODO: is this right? + MembershipState::Invite => Self::Invited, + MembershipState::Join => Self::Joined, + MembershipState::Knock => Self::Left, // TODO: is this right? + MembershipState::Leave => Self::Left, + _ => panic!("Unexpected MembershipState: {}", membership_state), + } + } +} + impl Room { pub(crate) fn new( own_user_id: &UserId, @@ -637,6 +650,10 @@ impl RoomInfo { self.room_state = RoomState::Invited; } + pub(crate) fn set_state(&mut self, room_state: RoomState) { + self.room_state = room_state; + } + /// Mark this Room as having all the members synced. pub fn mark_members_synced(&mut self) { self.members_synced = true; diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 59f3d485c..5d11b2976 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -20,9 +20,11 @@ use ruma::{ v3::{self, InvitedRoom, RoomSummary}, v4::{self, AccountData}, }, + events::AnySyncStateEvent, + serde::Raw, RoomId, }; -use tracing::{debug, info, instrument}; +use tracing::{debug, info, instrument, warn}; use super::BaseClient; #[cfg(feature = "e2e-encryption")] @@ -269,7 +271,8 @@ impl BaseClient { /// Look through the sliding sync data for this room, find/create it in the /// store, and process any invite information. /// If any invite_state exists, we take it to mean that we are invited to - /// this room https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters + /// this room, unless that state contains membership events that specify + /// otherwise. https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#room-list-parameters async fn process_sliding_sync_room_membership( &self, room_data: &v4::SlidingSyncRoom, @@ -285,13 +288,13 @@ impl BaseClient { // might not contain an m.room.member event, or they might set the // membership to something other than invite. This would be very // weird behaviour by the server, because invite_state is supposed - // to contain an m.room.member. Later, we will call - // handle_invited_state, which will reflect any information found in - // the real events inside invite_state, but we default to considering this room - // invited simply because invite_state exists. This is needed in the normal - // case, because the sliding sync server tries to send minimal - // state, meaning that we normally actually just receive {"type": - // "m.room.member"} with no content at all. + // to contain an m.room.member. We will call handle_invited_state, which will + // reflect any information found in the real events inside + // invite_state, but we default to considering this room invited + // simply because invite_state exists. This is needed in the normal + // case, because the sliding sync server tries to send minimal state, + // meaning that we normally actually just receive {"type": "m.room.member"} with + // no content at all. room_info.mark_as_invited(); self.handle_invited_state(invite_state.as_slice(), &mut room_info, changes); @@ -305,15 +308,55 @@ impl BaseClient { let room = store.get_or_create_room(room_id, RoomState::Joined).await; let mut room_info = room.clone_info(); - // We default to considering this room joined if it's not an invite, but we - // expect to find a m.room.member event in the required_state, so - // this should get fixed to the real correct value when we call - // self.handle_state below. + // We default to considering this room joined if it's not an invite. If it's + // actually left (and we remembered to request membership events in + // our sync request), then we can find this out from the events in + // required_state by calling handle_own_room_membership. room_info.mark_as_joined(); + // We don't need to do this in a v2 sync, because the membership of a room can + // be figured out by whether the room is in the "join", "leave" etc. + // property. In sliding sync we only have invite_state and + // required_state, so we must process required_state looking for + // relevant membership events. + self.handle_own_room_membership(&room_data.required_state, &mut room_info).await; + (room, room_info, None) } } + + /// Find any m.room.member events that refer to the current user, and update the state in + /// room_info to reflect the "membership" property. + pub(crate) async fn handle_own_room_membership( + &self, + required_state: &[Raw], + room_info: &mut RoomInfo, + ) { + // Note: we deserialise these same events inside handle_state(). We could refactor to do + // this only once. + + for raw_event in required_state { + let event = match raw_event.deserialize() { + Ok(ev) => ev, + Err(e) => { + warn!("Couldn't deserialize state event: {e}"); + continue; + } + }; + + if let AnySyncStateEvent::RoomMember(member) = &event { + // If this event updates the current user's membership, record that in the + // room_info. + if let Some(meta) = self.session_meta() { + if member.sender() == meta.user_id + && member.state_key() == meta.user_id.as_str() + { + room_info.set_state(member.membership().into()); + } + } + } + } + } } fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut RoomInfo) { @@ -345,9 +388,11 @@ mod test { device_id, event_id, events::{ room::{ - avatar::RoomAvatarEventContent, canonical_alias::RoomCanonicalAliasEventContent, + avatar::RoomAvatarEventContent, + canonical_alias::RoomCanonicalAliasEventContent, + member::{MembershipState, RoomMemberEventContent}, }, - StateEventContent, + AnySyncStateEvent, StateEventContent, }, mxc_uri, room_alias_id, room_id, serde::Raw, @@ -412,10 +457,11 @@ mod test { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@w:e.uk"); // When I send sliding sync response containing a room with a name let mut room = v4::SlidingSyncRoom::new(); - set_room_invited(&mut room); + set_room_invited(&mut room, user_id); room.name = Some("little room".to_owned()); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -425,6 +471,41 @@ mod test { assert_eq!(client_room.name(), Some("little room".to_owned())); } + #[async_test] + async fn can_be_reinvited_to_a_left_room() { + // See https://github.com/matrix-org/matrix-rust-sdk/issues/1834 + + // Given a logged-in client + let client = logged_in_client().await; + let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); + + // When I join... + let mut room = v4::SlidingSyncRoom::new(); + set_room_joined(&mut room, user_id); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + // (sanity: state is join) + assert_eq!(client.get_room(room_id).unwrap().state(), RoomState::Joined); + + // And then leave... + let mut room = v4::SlidingSyncRoom::new(); + set_room_left(&mut room, user_id); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + // (sanity: state is invite) + assert_eq!(client.get_room(room_id).unwrap().state(), RoomState::Left); + + // And then get invited back + let mut room = v4::SlidingSyncRoom::new(); + set_room_invited(&mut room, user_id); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + + // Then the room is in the invite state + assert_eq!(client.get_room(room_id).unwrap().state(), RoomState::Invited); + } + #[async_test] async fn avatar_is_found_when_processing_sliding_sync_response() { // Given a logged-in client @@ -450,10 +531,11 @@ mod test { // Given a logged-in client let client = logged_in_client().await; let room_id = room_id!("!r:e.uk"); + let user_id = user_id!("@u:e.uk"); // When I send sliding sync response containing an invited room let mut room = v4::SlidingSyncRoom::new(); - set_room_invited(&mut room); + set_room_invited(&mut room, user_id); let response = response_with_room(room_id, room).await; let sync_resp = client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -477,7 +559,7 @@ mod test { // When I send sliding sync response containing an invited room with an avatar let mut room = room_with_avatar(mxc_uri!("mxc://e.uk/med1"), user_id); - set_room_invited(&mut room); + set_room_invited(&mut room, user_id); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -499,7 +581,7 @@ mod test { // When I send sliding sync response containing an invited room with an avatar let mut room = room_with_canonical_alias(room_alias_id, user_id); - set_room_invited(&mut room); + set_room_invited(&mut room, user_id); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); @@ -556,7 +638,7 @@ mod test { room } - fn set_room_invited(room: &mut v4::SlidingSyncRoom) { + fn set_room_invited(room: &mut v4::SlidingSyncRoom, user_id: &UserId) { // MSC3575 shows an almost-empty event to indicate that we are invited to a // room. Just the type is supplied. @@ -567,6 +649,22 @@ mod test { .cast(); room.invite_state = Some(vec![evt]); + + // We expect that there will also be an invite event in the required_state, + // assuming you've asked for this type of event. + room.required_state.push(make_membership_event(user_id, MembershipState::Invite)); + } + + fn set_room_joined(room: &mut v4::SlidingSyncRoom, user_id: &UserId) { + room.required_state.push(make_membership_event(user_id, MembershipState::Join)); + } + + fn set_room_left(room: &mut v4::SlidingSyncRoom, user_id: &UserId) { + room.required_state.push(make_membership_event(user_id, MembershipState::Leave)); + } + + fn make_membership_event(user_id: &UserId, state: MembershipState) -> Raw { + make_state_event(user_id, user_id.as_str(), RoomMemberEventContent::new(state), None) } fn make_state_event( From f36a5b8cd71fc2185d55f098b4b62307431db506 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 13 Jun 2023 12:34:19 +0100 Subject: [PATCH 2/5] Deserialise events early so we don't have to do it multiple times We pass the already-deserialised event in to handle_state and process_sliding_sync_room_membership, to avoid deserialising inside both of them. --- crates/matrix-sdk-base/src/client.rs | 43 +++++++++++++------ crates/matrix-sdk-base/src/sliding_sync.rs | 50 ++++++++++++---------- 2 files changed, 58 insertions(+), 35 deletions(-) diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index c08c28e7f..e59e8cbc0 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -17,7 +17,7 @@ use std::ops::Deref; use std::{ collections::{BTreeMap, BTreeSet}, - fmt, + fmt, iter, sync::Arc, }; @@ -478,10 +478,16 @@ impl BaseClient { changes.stripped_state.insert(room_info.room_id().to_owned(), state_events); } + /// Process the events provided during a sync. + /// + /// events must be exactly the same list of events that are in raw_events, + /// but deserialised. We demand them here to avoid deserialising + /// multiple times. #[instrument(skip_all, fields(room_id = ?room_info.room_id))] pub(crate) async fn handle_state( &self, - events: &[Raw], + raw_events: &[Raw], + events: &[AnySyncStateEvent], room_info: &mut RoomInfo, changes: &mut StateChanges, ambiguity_cache: &mut AmbiguityCache, @@ -490,16 +496,8 @@ impl BaseClient { let mut user_ids = BTreeSet::new(); let mut profiles = BTreeMap::new(); - for raw_event in events { - let event = match raw_event.deserialize() { - Ok(ev) => ev, - Err(e) => { - warn!("Couldn't deserialize state event: {e}"); - continue; - } - }; - - room_info.handle_state_event(&event); + for (raw_event, event) in iter::zip(raw_events, events) { + room_info.handle_state_event(event); if let AnySyncStateEvent::RoomMember(member) = &event { ambiguity_cache.handle_event(changes, &room_info.room_id, member).await?; @@ -714,9 +712,12 @@ impl BaseClient { room_info.set_prev_batch(new_info.timeline.prev_batch.as_deref()); room_info.mark_state_fully_synced(); + let deserialized_events = Self::deserialize_events(&new_info.state.events); + let mut user_ids = self .handle_state( &new_info.state.events, + &deserialized_events, &mut room_info, &mut changes, &mut ambiguity_cache, @@ -800,9 +801,12 @@ impl BaseClient { room_info.mark_as_left(); room_info.mark_state_partially_synced(); + let deserialized_events = Self::deserialize_events(&new_info.state.events); + let mut user_ids = self .handle_state( &new_info.state.events, + &deserialized_events, &mut room_info, &mut changes, &mut ambiguity_cache, @@ -1242,6 +1246,21 @@ impl BaseClient { pub fn subscribe_to_ignore_user_list_changes(&self) -> Subscriber<()> { self.ignore_user_list_changes_tx.subscribe() } + + pub(crate) fn deserialize_events( + raw_events: &[Raw], + ) -> Vec { + raw_events + .iter() + .filter_map(|raw_event| match raw_event.deserialize() { + Ok(ev) => Some(ev), + Err(e) => { + warn!("Couldn't deserialize state event: {e}"); + None + } + }) + .collect() + } } impl Default for BaseClient { diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 5d11b2976..5e6674a1a 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -21,10 +21,9 @@ use ruma::{ v4::{self, AccountData}, }, events::AnySyncStateEvent, - serde::Raw, RoomId, }; -use tracing::{debug, info, instrument, warn}; +use tracing::{debug, info, instrument}; use super::BaseClient; #[cfg(feature = "e2e-encryption")] @@ -193,15 +192,30 @@ impl BaseClient { ambiguity_cache: &mut AmbiguityCache, account_data: &AccountData, ) -> Result<(RoomInfo, Option, Option)> { + let required_state = Self::deserialize_events(&room_data.required_state); + // Find or create the room in the store - let (room, mut room_info, invited_room) = - self.process_sliding_sync_room_membership(room_data, store, room_id, changes).await; + let (room, mut room_info, invited_room) = self + .process_sliding_sync_room_membership( + room_data, + &required_state, + store, + room_id, + changes, + ) + .await; room_info.mark_state_partially_synced(); - let mut user_ids = if !room_data.required_state.is_empty() { - self.handle_state(&room_data.required_state, &mut room_info, changes, ambiguity_cache) - .await? + let mut user_ids = if !required_state.is_empty() { + self.handle_state( + &room_data.required_state, + &required_state, + &mut room_info, + changes, + ambiguity_cache, + ) + .await? } else { Default::default() }; @@ -276,6 +290,7 @@ impl BaseClient { async fn process_sliding_sync_room_membership( &self, room_data: &v4::SlidingSyncRoom, + required_state: &[AnySyncStateEvent], store: &Store, room_id: &RoomId, changes: &mut StateChanges, @@ -319,31 +334,20 @@ impl BaseClient { // property. In sliding sync we only have invite_state and // required_state, so we must process required_state looking for // relevant membership events. - self.handle_own_room_membership(&room_data.required_state, &mut room_info).await; + self.handle_own_room_membership(required_state, &mut room_info).await; (room, room_info, None) } } - /// Find any m.room.member events that refer to the current user, and update the state in - /// room_info to reflect the "membership" property. + /// Find any m.room.member events that refer to the current user, and update + /// the state in room_info to reflect the "membership" property. pub(crate) async fn handle_own_room_membership( &self, - required_state: &[Raw], + required_state: &[AnySyncStateEvent], room_info: &mut RoomInfo, ) { - // Note: we deserialise these same events inside handle_state(). We could refactor to do - // this only once. - - for raw_event in required_state { - let event = match raw_event.deserialize() { - Ok(ev) => ev, - Err(e) => { - warn!("Couldn't deserialize state event: {e}"); - continue; - } - }; - + for event in required_state { if let AnySyncStateEvent::RoomMember(member) = &event { // If this event updates the current user's membership, record that in the // room_info. From 240a7f7aaebb52e411d9f8e5bbb165fcc7c8de33 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 13 Jun 2023 13:43:46 +0100 Subject: [PATCH 3/5] Make set_state public to avoid unused warning --- crates/matrix-sdk-base/src/rooms/normal.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 1eff6b122..badb7399a 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -650,7 +650,8 @@ impl RoomInfo { self.room_state = RoomState::Invited; } - pub(crate) fn set_state(&mut self, room_state: RoomState) { + /// Set the membership RoomState of this Room + pub fn set_state(&mut self, room_state: RoomState) { self.room_state = room_state; } From 22fd44df104e5aef39444e5d669b1bd43cbf290b Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 14 Jun 2023 15:31:25 +0100 Subject: [PATCH 4/5] Fix comments from review feedback --- crates/matrix-sdk-base/src/rooms/normal.rs | 6 ++++-- crates/matrix-sdk-base/src/sliding_sync.rs | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index badb7399a..986c474e5 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -85,11 +85,13 @@ pub enum RoomState { impl From<&MembershipState> for RoomState { fn from(membership_state: &MembershipState) -> Self { + // We consider Ban, Knock and Leave to be Left, because they all mean we are not + // in the room. match membership_state { - MembershipState::Ban => Self::Left, // TODO: is this right? + MembershipState::Ban => Self::Left, MembershipState::Invite => Self::Invited, MembershipState::Join => Self::Joined, - MembershipState::Knock => Self::Left, // TODO: is this right? + MembershipState::Knock => Self::Left, MembershipState::Leave => Self::Left, _ => panic!("Unexpected MembershipState: {}", membership_state), } diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 5e6674a1a..b35d18bb3 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -497,7 +497,7 @@ mod test { set_room_left(&mut room, user_id); let response = response_with_room(room_id, room).await; client.process_sliding_sync(&response).await.expect("Failed to process sync"); - // (sanity: state is invite) + // (sanity: state is left) assert_eq!(client.get_room(room_id).unwrap().state(), RoomState::Left); // And then get invited back From 7ca3686aea0c9675f393d16b4f3e8d048a90c556 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 15 Jun 2023 09:37:27 +0100 Subject: [PATCH 5/5] Fix formatting after merge --- crates/matrix-sdk-base/src/rooms/normal.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 34e4ba3ee..3109d5852 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -25,10 +25,15 @@ use ruma::{ ignored_user_list::IgnoredUserListEventContent, receipt::{Receipt, ReceiptThread, ReceiptType}, room::{ - create::RoomCreateEventContent, encryption::RoomEncryptionEventContent, - guest_access::GuestAccess, history_visibility::HistoryVisibility, join_rules::JoinRule, - member::MembershipState, member::RoomMemberEventContent, name::RoomNameEventContent, - redaction::OriginalSyncRoomRedactionEvent, tombstone::RoomTombstoneEventContent, + create::RoomCreateEventContent, + encryption::RoomEncryptionEventContent, + guest_access::GuestAccess, + history_visibility::HistoryVisibility, + join_rules::JoinRule, + member::{MembershipState, RoomMemberEventContent}, + name::RoomNameEventContent, + redaction::OriginalSyncRoomRedactionEvent, + tombstone::RoomTombstoneEventContent, }, tag::Tags, AnyRoomAccountDataEvent, AnyStrippedStateEvent, AnySyncStateEvent,