diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index 5cf1fd4e622..b891e6234e0 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/crates/matrix-sdk-base/src/rooms/normal.rs @@ -271,7 +271,9 @@ impl Room { /// /// *Note*: The member list might have been modified in the meantime and /// the targets might not even be in the room anymore. This setting should - /// only be considered as guidance. + /// only be considered as guidance. We leave members in this list to allow + /// us to re-find a DM with a user even if they have left, since we may + /// want to re-invite them. pub fn direct_targets(&self) -> HashSet { self.inner.read().unwrap().base_info.dm_targets.clone() } diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 183d7b30f99..f0dce153d72 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -387,20 +387,23 @@ fn process_room_properties(room_data: &v4::SlidingSyncRoom, room_info: &mut Room #[cfg(test)] mod test { + use std::collections::{BTreeMap, HashSet}; + use matrix_sdk_test::async_test; use ruma::{ device_id, event_id, events::{ + direct::DirectEventContent, room::{ avatar::RoomAvatarEventContent, canonical_alias::RoomCanonicalAliasEventContent, member::{MembershipState, RoomMemberEventContent}, }, - AnySyncStateEvent, StateEventContent, + AnySyncStateEvent, GlobalAccountDataEventContent, StateEventContent, }, mxc_uri, room_alias_id, room_id, serde::Raw, - uint, user_id, MxcUri, RoomAliasId, RoomId, UserId, + uint, user_id, MxcUri, OwnedRoomId, OwnedUserId, RoomAliasId, RoomId, UserId, }; use serde_json::json; @@ -510,6 +513,55 @@ mod test { assert_eq!(client.get_room(room_id).unwrap().state(), RoomState::Invited); } + #[async_test] + async fn other_person_leaving_a_dm_is_reflected_in_their_membership_and_direct_targets() { + let room_id = room_id!("!r:e.uk"); + let user_a_id = user_id!("@a:e.uk"); + let user_b_id = user_id!("@b:e.uk"); + + // Given we have a DM with B, who is joined + let client = logged_in_client().await; + create_dm(&client, room_id, user_a_id, user_b_id, MembershipState::Join).await; + + // (Sanity: B is a direct target, and is in Join state) + assert!(direct_targets(&client, room_id).contains(user_b_id)); + assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Join); + + // When B leaves + update_room_membership(&client, room_id, user_b_id, MembershipState::Leave).await; + + // Then B is still a direct target, and is in Leave state (B is a direct target + // because we want to return to our old DM in the UI even if the other + // user left, so we can reinvite them. See https://github.com/matrix-org/matrix-rust-sdk/issues/2017) + assert!(direct_targets(&client, room_id).contains(user_b_id)); + assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Leave); + } + + #[async_test] + async fn other_person_refusing_invite_to_a_dm_is_reflected_in_their_membership_and_direct_targets( + ) { + let room_id = room_id!("!r:e.uk"); + let user_a_id = user_id!("@a:e.uk"); + let user_b_id = user_id!("@b:e.uk"); + + // Given I have invited B to a DM + let client = logged_in_client().await; + create_dm(&client, room_id, user_a_id, user_b_id, MembershipState::Invite).await; + + // (Sanity: B is a direct target, and is in Invite state) + assert!(direct_targets(&client, room_id).contains(user_b_id)); + assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Invite); + + // When B declines the invitation (i.e. leaves) + update_room_membership(&client, room_id, user_b_id, MembershipState::Leave).await; + + // Then B is still a direct target, and is in Leave state (B is a direct target + // because we want to return to our old DM in the UI even if the other + // user left, so we can reinvite them. See https://github.com/matrix-org/matrix-rust-sdk/issues/2017) + assert!(direct_targets(&client, room_id).contains(user_b_id)); + assert_eq!(membership(&client, room_id, user_b_id).await, MembershipState::Leave); + } + #[async_test] async fn avatar_is_found_when_processing_sliding_sync_response() { // Given a logged-in client @@ -594,6 +646,65 @@ mod test { assert_eq!(client_room.canonical_alias(), Some(room_alias_id.to_owned())); } + async fn membership( + client: &BaseClient, + room_id: &RoomId, + user_id: &UserId, + ) -> MembershipState { + let room = client.get_room(room_id).expect("Room not found!"); + let member = room.get_member(user_id).await.unwrap().expect("B not in room"); + member.membership().clone() + } + + fn direct_targets(client: &BaseClient, room_id: &RoomId) -> HashSet { + let room = client.get_room(room_id).expect("Room not found!"); + room.direct_targets() + } + + /// Create a DM with the other user, setting our membership to Join and + /// theirs to other_state + async fn create_dm( + client: &BaseClient, + room_id: &RoomId, + my_id: &UserId, + their_id: &UserId, + other_state: MembershipState, + ) { + let mut room = v4::SlidingSyncRoom::new(); + set_room_joined(&mut room, my_id); + room.required_state.push(make_membership_event(their_id, other_state)); + let mut response = response_with_room(room_id, room).await; + set_direct_with(&mut response, their_id.to_owned(), vec![room_id.to_owned()]); + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + } + + /// Set this user's membership within this room to new_state + async fn update_room_membership( + client: &BaseClient, + room_id: &RoomId, + user_id: &UserId, + new_state: MembershipState, + ) { + let mut room = v4::SlidingSyncRoom::new(); + room.required_state.push(make_membership_event(user_id, new_state)); + let response = response_with_room(room_id, room).await; + client.process_sliding_sync(&response).await.expect("Failed to process sync"); + } + + fn set_direct_with( + response: &mut v4::Response, + user_id: OwnedUserId, + room_ids: Vec, + ) { + let mut direct_content = BTreeMap::new(); + direct_content.insert(user_id, room_ids); + response + .extensions + .account_data + .global + .push(make_global_account_data_event(DirectEventContent(direct_content))); + } + async fn logged_in_client() -> BaseClient { let client = BaseClient::new(); client @@ -671,6 +782,15 @@ mod test { make_state_event(user_id, user_id.as_str(), RoomMemberEventContent::new(state), None) } + fn make_global_account_data_event(content: C) -> Raw { + Raw::new(&json!({ + "type": content.event_type(), + "content": content, + })) + .expect("Failed to create account data event") + .cast() + } + fn make_state_event( sender: &UserId, state_key: &str, diff --git a/crates/matrix-sdk/src/encryption/mod.rs b/crates/matrix-sdk/src/encryption/mod.rs index 9c090834bcb..7a1eb441c35 100644 --- a/crates/matrix-sdk/src/encryption/mod.rs +++ b/crates/matrix-sdk/src/encryption/mod.rs @@ -836,10 +836,14 @@ impl Encryption { #[cfg(all(test, not(target_arch = "wasm32")))] mod tests { - use matrix_sdk_test::{async_test, test_json, EventBuilder, JoinedRoomBuilder, StateTestEvent}; + use matrix_sdk_test::{ + async_test, test_json, EventBuilder, GlobalAccountDataTestEvent, JoinedRoomBuilder, + StateTestEvent, + }; use ruma::{ event_id, events::{reaction::ReactionEventContent, relation::Annotation}, + user_id, }; use serde_json::json; use wiremock::{ @@ -897,4 +901,74 @@ mod tests { .await .expect("Sending the reaction should not fail"); } + + #[async_test] + async fn get_dm_room_returns_the_room_we_have_with_this_user() { + let server = MockServer::start().await; + let client = logged_in_client(Some(server.uri())).await; + // This is the user ID that is inside MemberAdditional. + // Note the confusing username, so we can share + // GlobalAccountDataTestEvent::Direct with the invited test. + let user_id = user_id!("@invited:localhost"); + + // When we receive a sync response saying "invited" is invited to a DM + let response = EventBuilder::default() + .add_joined_room( + JoinedRoomBuilder::default().add_state_event(StateTestEvent::MemberAdditional), + ) + .add_global_account_data_event(GlobalAccountDataTestEvent::Direct) + .build_sync_response(); + client.base_client().receive_sync_response(response).await.unwrap(); + + // Then get_dm_room finds this room + let found_room = client.get_dm_room(user_id).expect("DM not found!"); + assert!(found_room.get_member_no_sync(user_id).await.unwrap().is_some()); + } + + #[async_test] + async fn get_dm_room_still_finds_room_where_participant_is_only_invited() { + let server = MockServer::start().await; + let client = logged_in_client(Some(server.uri())).await; + // This is the user ID that is inside MemberInvite + let user_id = user_id!("@invited:localhost"); + + // When we receive a sync response saying "invited" is invited to a DM + let response = EventBuilder::default() + .add_joined_room( + JoinedRoomBuilder::default().add_state_event(StateTestEvent::MemberInvite), + ) + .add_global_account_data_event(GlobalAccountDataTestEvent::Direct) + .build_sync_response(); + client.base_client().receive_sync_response(response).await.unwrap(); + + // Then get_dm_room finds this room + let found_room = client.get_dm_room(user_id).expect("DM not found!"); + assert!(found_room.get_member_no_sync(user_id).await.unwrap().is_some()); + } + + #[async_test] + async fn get_dm_room_still_finds_left_room() { + // See the discussion in https://github.com/matrix-org/matrix-rust-sdk/issues/2017 + // and the high-level issue at https://github.com/vector-im/element-x-ios/issues/1077 + + let server = MockServer::start().await; + let client = logged_in_client(Some(server.uri())).await; + // This is the user ID that is inside MemberAdditional. + // Note the confusing username, so we can share + // GlobalAccountDataTestEvent::Direct with the invited test. + let user_id = user_id!("@invited:localhost"); + + // When we receive a sync response saying "invited" is invited to a DM + let response = EventBuilder::default() + .add_joined_room( + JoinedRoomBuilder::default().add_state_event(StateTestEvent::MemberLeave), + ) + .add_global_account_data_event(GlobalAccountDataTestEvent::Direct) + .build_sync_response(); + client.base_client().receive_sync_response(response).await.unwrap(); + + // Then get_dm_room finds this room + let found_room = client.get_dm_room(user_id).expect("DM not found!"); + assert!(found_room.get_member_no_sync(user_id).await.unwrap().is_some()); + } } diff --git a/testing/matrix-sdk-test/src/event_builder/mod.rs b/testing/matrix-sdk-test/src/event_builder/mod.rs index 4af354fe2f2..3922da750ba 100644 --- a/testing/matrix-sdk-test/src/event_builder/mod.rs +++ b/testing/matrix-sdk-test/src/event_builder/mod.rs @@ -153,6 +153,7 @@ impl EventBuilder { event: GlobalAccountDataTestEvent, ) -> &mut Self { let val = match event { + GlobalAccountDataTestEvent::Direct => test_json::DIRECT.to_owned(), GlobalAccountDataTestEvent::PushRules => test_json::PUSH_RULES.to_owned(), GlobalAccountDataTestEvent::Tags => test_json::TAG.to_owned(), GlobalAccountDataTestEvent::Custom(json) => json, diff --git a/testing/matrix-sdk-test/src/event_builder/test_event.rs b/testing/matrix-sdk-test/src/event_builder/test_event.rs index eccc6db4358..79499b46218 100644 --- a/testing/matrix-sdk-test/src/event_builder/test_event.rs +++ b/testing/matrix-sdk-test/src/event_builder/test_event.rs @@ -83,8 +83,10 @@ pub enum StateTestEvent { HistoryVisibility, JoinRules, Member, + MemberAdditional, MemberBan, MemberInvite, + MemberLeave, MemberNameChange, PowerLevels, RedactedInvalid, @@ -106,8 +108,10 @@ impl StateTestEvent { Self::HistoryVisibility => test_json::sync_events::HISTORY_VISIBILITY.to_owned(), Self::JoinRules => test_json::sync_events::JOIN_RULES.to_owned(), Self::Member => test_json::sync_events::MEMBER.to_owned(), + Self::MemberAdditional => test_json::sync_events::MEMBER_ADDITIONAL.to_owned(), Self::MemberBan => test_json::sync_events::MEMBER_BAN.to_owned(), Self::MemberInvite => test_json::sync_events::MEMBER_INVITE.to_owned(), + Self::MemberLeave => test_json::sync_events::MEMBER_LEAVE.to_owned(), Self::MemberNameChange => test_json::sync_events::MEMBER_NAME_CHANGE.to_owned(), Self::PowerLevels => test_json::sync_events::POWER_LEVELS.to_owned(), Self::RedactedInvalid => test_json::sync_events::REDACTED_INVALID.to_owned(), @@ -217,6 +221,7 @@ impl PresenceTestEvent { /// Test events that can be added to the global account data. pub enum GlobalAccountDataTestEvent { + Direct, PushRules, Tags, Custom(JsonValue), @@ -226,6 +231,7 @@ impl GlobalAccountDataTestEvent { /// Get the JSON representation of this test event. pub fn into_json_value(self) -> JsonValue { match self { + Self::Direct => test_json::sync_events::DIRECT.to_owned(), Self::PushRules => test_json::sync_events::PUSH_RULES.to_owned(), Self::Tags => test_json::sync_events::TAG.to_owned(), Self::Custom(json) => json, diff --git a/testing/matrix-sdk-test/src/test_json/mod.rs b/testing/matrix-sdk-test/src/test_json/mod.rs index 5af09dca513..d8cfb5763fc 100644 --- a/testing/matrix-sdk-test/src/test_json/mod.rs +++ b/testing/matrix-sdk-test/src/test_json/mod.rs @@ -27,10 +27,10 @@ pub use sync::{ MORE_SYNC, MORE_SYNC_2, SYNC, VOIP_SYNC, }; pub use sync_events::{ - ALIAS, ALIASES, ENCRYPTION, MEMBER, MEMBER_BAN, MEMBER_INVITE, MEMBER_NAME_CHANGE, - MEMBER_STRIPPED, MESSAGE_EDIT, MESSAGE_TEXT, NAME, NAME_STRIPPED, POWER_LEVELS, PRESENCE, - PUSH_RULES, REACTION, READ_RECEIPT, READ_RECEIPT_OTHER, REDACTED, REDACTED_INVALID, - REDACTED_STATE, REDACTION, TAG, TOPIC, TOPIC_REDACTION, TYPING, + ALIAS, ALIASES, DIRECT, ENCRYPTION, MEMBER, MEMBER_ADDITIONAL, MEMBER_BAN, MEMBER_INVITE, + MEMBER_LEAVE, MEMBER_NAME_CHANGE, MEMBER_STRIPPED, MESSAGE_EDIT, MESSAGE_TEXT, NAME, + NAME_STRIPPED, POWER_LEVELS, PRESENCE, PUSH_RULES, REACTION, READ_RECEIPT, READ_RECEIPT_OTHER, + REDACTED, REDACTED_INVALID, REDACTED_STATE, REDACTION, TAG, TOPIC, TOPIC_REDACTION, TYPING, }; /// An empty response. diff --git a/testing/matrix-sdk-test/src/test_json/sync_events.rs b/testing/matrix-sdk-test/src/test_json/sync_events.rs index ba63c5282f4..1e95cb098b2 100644 --- a/testing/matrix-sdk-test/src/test_json/sync_events.rs +++ b/testing/matrix-sdk-test/src/test_json/sync_events.rs @@ -55,6 +55,22 @@ pub static CREATE: Lazy = Lazy::new(|| { }) }); +pub static DIRECT: Lazy = Lazy::new(|| { + json!({ + "content": { + "@invited:localhost": ["!SVkFJHzfwvuaIEawgC:localhost"], + }, + "event_id": "$757957878228ekrDs:localhost", + "origin_server_ts": 17195787, + "sender": "@example:localhost", + "state_key": "", + "type": "m.direct", + "unsigned": { + "age": 139298 + } + }) +}); + pub static FULLY_READ: Lazy = Lazy::new(|| { json!({ "content": { @@ -146,6 +162,40 @@ pub static MEMBER: Lazy = Lazy::new(|| { }) }); +// Make @invited:localhost a member (note the confusing name) +pub static MEMBER_ADDITIONAL: Lazy = Lazy::new(|| { + json!({ + "content": { + "membership": "join", + }, + "event_id": "$747273582443PhrSn:localhost", + "origin_server_ts": 1472735824, + "sender": "@example:localhost", + "state_key": "@invited:localhost", + "type": "m.room.member", + "unsigned": { + "age": 1234 + } + }) +}); + +// Make @invited:localhost leave the room (note the confusing name) +pub static MEMBER_LEAVE: Lazy = Lazy::new(|| { + json!({ + "content": { + "membership": "leave", + }, + "event_id": "$747273582443PhrS9:localhost", + "origin_server_ts": 1472735820, + "sender": "@example:localhost", + "state_key": "@invited:localhost", + "type": "m.room.member", + "unsigned": { + "age": 1234 + } + }) +}); + pub static MEMBER_BAN: Lazy = Lazy::new(|| { json!({ "content": {