From 4262529d0915f29db81dea4b164716d031d3002f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 18 Oct 2024 15:39:48 +0100 Subject: [PATCH] feat(crypto) Support Verified and VerificationViolation updates in IdentityStatusChanges streams --- .../src/identities/room_identity_state.rs | 213 ++++++++++++++-- .../src/room/identity_status_changes.rs | 237 ++++++++++++++++-- 2 files changed, 410 insertions(+), 40 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs b/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs index 1b963be34fe..8b6aec3939d 100644 --- a/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs +++ b/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs @@ -121,7 +121,6 @@ impl RoomIdentityState { for user_identity in identity_updates.new.values().chain(identity_updates.changed.values()) { - // Ignore updates to our own identity let user_id = user_identity.user_id(); if self.room.is_member(user_id).await { let update = self.update_user_state(user_id, user_identity); @@ -144,20 +143,26 @@ impl RoomIdentityState { // Ignore invalid user IDs let user_id: Result<&UserId, _> = event.state_key.as_str().try_into(); if let Ok(user_id) = user_id { - // Ignore non-existent users - if let Some(user_identity) = self.room.user_identity(user_id).await { + // Ignore non-existent users, and changes to our own identity + if let Some(user_identity @ UserIdentity::Other(_)) = + self.room.user_identity(user_id).await + { match event.content.membership { MembershipState::Join | MembershipState::Invite => { + // They are joining the room - check whether we need to display a + // warning to the user if let Some(update) = self.update_user_state(user_id, &user_identity) { return vec![update]; } } MembershipState::Leave | MembershipState::Ban => { - let leaving_state = self.room.state_of(&user_identity); - if leaving_state == IdentityState::PinViolation { - // If a user with bad state leaves the room, set them to Pinned, - // which effectively removes them - return vec![self.set_state(user_id, IdentityState::Pinned)]; + // They are leaving the room - treat that as if they are becoming + // Pinned + + if let Some(update) = + self.update_user_state_to(user_id, IdentityState::Pinned) + { + return vec![update]; } } MembershipState::Knock => { @@ -179,21 +184,58 @@ impl RoomIdentityState { user_identity: &UserIdentity, ) -> Option { if let UserIdentity::Other(_) = &user_identity { - // If the user's state has changed - let new_state = self.room.state_of(user_identity); - let old_state = self.known_states.get(user_id); - if new_state != old_state { - Some(self.set_state(user_identity.user_id(), new_state)) - } else { - // Nothing changed - None - } + self.update_user_state_to(user_id, self.room.state_of(user_identity)) } else { // Ignore updates to our own identity None } } + fn update_user_state_to( + &mut self, + user_id: &UserId, + new_state: IdentityState, + ) -> Option { + use IdentityState::*; + + let old_state = self.known_states.get(user_id); + let change = self.set_state(user_id, new_state.clone()); + + let send_update = match (old_state, new_state) { + // good -> bad - report so we can add a message + (Pinned, PinViolation) => true, + (Pinned, VerificationViolation) => true, + (Verified, PinViolation) => true, + (Verified, VerificationViolation) => true, + + // bad -> good - report so we can remove a message + (PinViolation, Pinned) => true, + (PinViolation, Verified) => true, + (VerificationViolation, Pinned) => true, + (VerificationViolation, Verified) => true, + + // Changed the type of bad - report so can change the message + (PinViolation, VerificationViolation) => true, + (VerificationViolation, PinViolation) => true, + + // good -> good - don't report - no message needed in either case + (Pinned, Verified) => false, + (Verified, Pinned) => false, + + // State didn't change - don't report - nothing changed + (Pinned, Pinned) => false, + (Verified, Verified) => false, + (PinViolation, PinViolation) => false, + (VerificationViolation, VerificationViolation) => false, + }; + + if send_update { + Some(change) + } else { + None + } + } + fn set_state(&mut self, user_id: &UserId, new_state: IdentityState) -> IdentityStatusChange { // Remember the new state of the user self.known_states.set(user_id, &new_state); @@ -204,8 +246,16 @@ impl RoomIdentityState { } /// A change in the status of the identity of a member of the room. Returned by -/// [`RoomIdentityState::process_change`] to indicate that something changed in -/// this room and we should either show or hide a warning. +/// [`RoomIdentityState::process_change`] to indicate that something significant +/// changed in this room and we should either show or hide a warning. +/// +/// Examples of "significant" changes: +/// - pinned->unpinned +/// - verification violation->verified +/// +/// Examples of "insignificant" changes: +/// - pinned->verified +/// - verified->pinned #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct IdentityStatusChange { /// The user ID of the user whose identity status changed @@ -344,6 +394,23 @@ mod tests { ); } + #[async_test] + async fn test_verifying_a_pinned_identity_in_the_room_does_nothing() { + // Given someone in the room is pinned + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.member(other_user_identity(user_id).await, IdentityState::Pinned); + let mut state = RoomIdentityState::new(room.clone()).await; + + // When their identity changes to verified + let updates = + identity_change(&mut room, user_id, IdentityState::Verified, false, false).await; + let update = state.process_change(updates).await; + + // Then we emit no update + assert_eq!(update, vec![]); + } + #[async_test] async fn test_pinning_an_unpinned_identity_in_the_room_notifies() { // Given someone in the room is unpinned @@ -367,6 +434,30 @@ mod tests { ); } + #[async_test] + async fn test_unpinned_identity_becoming_verification_violating_in_the_room_notifies() { + // Given someone in the room is unpinned + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.member(other_user_identity(user_id).await, IdentityState::PinViolation); + let mut state = RoomIdentityState::new(room.clone()).await; + + // When their identity changes to verification violation + let updates = + identity_change(&mut room, user_id, IdentityState::VerificationViolation, false, false) + .await; + let update = state.process_change(updates).await; + + // Then we emit an update saying they became verification violating + assert_eq!( + update, + vec![IdentityStatusChange { + user_id: user_id.to_owned(), + changed_to: IdentityState::VerificationViolation + }] + ); + } + #[async_test] async fn test_unpinning_an_identity_not_in_the_room_does_nothing() { // Given an empty room @@ -448,6 +539,22 @@ mod tests { assert_eq!(update, []); } + #[async_test] + async fn test_a_verified_identity_joining_the_room_does_nothing() { + // Given an empty room and we know of a user who is verified + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.non_member(other_user_identity(user_id).await, IdentityState::Verified); + let mut state = RoomIdentityState::new(room).await; + + // When the verified user joins the room + let updates = room_change(user_id, MembershipState::Join); + let update = state.process_change(updates).await; + + // Then we emit no update because they are verified + assert_eq!(update, []); + } + #[async_test] async fn test_an_unpinned_identity_joining_the_room_notifies() { // Given an empty room and we know of a user who is unpinned @@ -508,6 +615,28 @@ mod tests { ); } + #[async_test] + async fn test_a_verification_violating_identity_invited_to_the_room_notifies() { + // Given an empty room and we know of a user who is unpinned + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.non_member(other_user_identity(user_id).await, IdentityState::VerificationViolation); + let mut state = RoomIdentityState::new(room).await; + + // When the user is invited to the room + let updates = room_change(user_id, MembershipState::Invite); + let update = state.process_change(updates).await; + + // Then we emit an update saying they became verification violation + assert_eq!( + update, + vec![IdentityStatusChange { + user_id: user_id.to_owned(), + changed_to: IdentityState::VerificationViolation + }] + ); + } + #[async_test] async fn test_own_identity_becoming_unpinned_is_ignored() { // Given I am pinned @@ -589,6 +718,22 @@ mod tests { assert_eq!(update, []); } + #[async_test] + async fn test_a_verified_identity_leaving_the_room_does_nothing() { + // Given a pinned user is in the room + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.member(other_user_identity(user_id).await, IdentityState::Verified); + let mut state = RoomIdentityState::new(room).await; + + // When the user leaves the room + let updates = room_change(user_id, MembershipState::Leave); + let update = state.process_change(updates).await; + + // Then we emit no update because they are verified + assert_eq!(update, []); + } + #[async_test] async fn test_an_unpinned_identity_leaving_the_room_notifies() { // Given an unpinned user is in the room @@ -601,7 +746,29 @@ mod tests { let updates = room_change(user_id, MembershipState::Leave); let update = state.process_change(updates).await; - // Then we emit an update saying they became unpinned + // Then we emit an update saying they became pinned + assert_eq!( + update, + vec![IdentityStatusChange { + user_id: user_id.to_owned(), + changed_to: IdentityState::Pinned + }] + ); + } + + #[async_test] + async fn test_a_verification_violating_identity_leaving_the_room_notifies() { + // Given an unpinned user is in the room + let user_id = user_id!("@u:s.co"); + let mut room = FakeRoom::new(); + room.member(other_user_identity(user_id).await, IdentityState::VerificationViolation); + let mut state = RoomIdentityState::new(room).await; + + // When the user leaves the room + let updates = room_change(user_id, MembershipState::Leave); + let update = state.process_change(updates).await; + + // Then we emit an update saying they became pinned assert_eq!( update, vec![IdentityStatusChange { @@ -780,17 +947,21 @@ mod tests { } #[async_test] - async fn test_current_state_contains_all_unpinned_users() { + async fn test_current_state_contains_all_nonpinned_users() { // Given some people are unpinned let user1 = user_id!("@u1:s.co"); let user2 = user_id!("@u2:s.co"); let user3 = user_id!("@u3:s.co"); let user4 = user_id!("@u4:s.co"); + let user5 = user_id!("@u5:s.co"); + let user6 = user_id!("@u6:s.co"); let mut room = FakeRoom::new(); room.member(other_user_identity(user1).await, IdentityState::Pinned); room.member(other_user_identity(user2).await, IdentityState::PinViolation); room.member(other_user_identity(user3).await, IdentityState::Pinned); room.member(other_user_identity(user4).await, IdentityState::PinViolation); + room.member(other_user_identity(user5).await, IdentityState::Verified); + room.member(other_user_identity(user6).await, IdentityState::VerificationViolation); let mut state = RoomIdentityState::new(room).await.current_state(); state.sort_by_key(|change| change.user_id.to_owned()); assert_eq!( diff --git a/crates/matrix-sdk/src/room/identity_status_changes.rs b/crates/matrix-sdk/src/room/identity_status_changes.rs index 5d85456a4fb..0816862d9ef 100644 --- a/crates/matrix-sdk/src/room/identity_status_changes.rs +++ b/crates/matrix-sdk/src/room/identity_status_changes.rs @@ -56,12 +56,18 @@ pub struct IdentityStatusChanges { } impl IdentityStatusChanges { - /// Create a new stream of changes to the identity status of members of a - /// room. + /// Create a new stream of significant changes to the identity status of + /// members of a room. /// /// The "status" of an identity changes when our level of trust in it /// changes. /// + /// A "significant" change means a warning should either be added or removed + /// (e.g. the user changed from pinned to unpinned (show a warning) or + /// from verification violation to pinned (remove a warning). An + /// insignificant change would be from pinned to verified - no warning + /// is needed in this case. + /// /// For example, if an identity is "pinned" i.e. not manually verified, but /// known, and it becomes a "unpinned" i.e. unknown, because the /// encryption keys are different and the user has not acknowledged @@ -196,7 +202,7 @@ mod tests { use futures_core::Stream; use futures_util::FutureExt; use matrix_sdk_base::crypto::{IdentityState, IdentityStatusChange}; - use matrix_sdk_test::async_test; + use matrix_sdk_test::{async_test, test_json::keys_query_sets::IdentityChangeDataSet}; use test_setup::TestSetup; use tokio_stream::{StreamExt, Timeout}; @@ -221,6 +227,27 @@ mod tests { assert_eq!(change.len(), 1); } + #[async_test] + async fn test_when_user_becomes_verification_violation_we_report_it() { + // Given a room containing us and Bob + let t = TestSetup::new_room_with_other_member().await; + + // And Bob's identity is verified + t.verify().await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + + // When Bob's identity changes + t.unpin().await; + + // Then we were notified about a verification violation + let change = next_change(&mut pin!(changes)).await; + assert_eq!(change[0].user_id, t.user_id()); + assert_eq!(change[0].changed_to, IdentityState::VerificationViolation); + assert_eq!(change.len(), 1); + } + #[async_test] async fn test_when_user_becomes_pinned_we_report_it() { // Given a room containing us and Bob @@ -249,6 +276,90 @@ mod tests { assert_eq!(change2.len(), 1); } + #[async_test] + async fn test_when_user_becomes_verified_we_dont_report_it() { + // Given a room containing us and Bob + let t = TestSetup::new_room_with_other_member().await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + let mut changes = pin!(changes); + + // When Bob becomes verified + t.verify().await; + + // (And then unpinned, so we have something to come through the stream) + t.unpin().await; + + // Then we are only notified about the unpinning part + let change2 = next_change(&mut changes).await; + assert_eq!(change2[0].user_id, t.user_id()); + assert_eq!(change2[0].changed_to, IdentityState::VerificationViolation); + assert_eq!(change2.len(), 1); + } + + #[async_test] + async fn test_when_an_unpinned_user_becomes_verified_we_report_it() { + // Given a room containing us and Bob + let t = TestSetup::new_room_with_other_member().await; + + // And Bob's identity is unpinned + t.unpin_with(IdentityChangeDataSet::key_query_with_identity_a()).await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + let mut changes = pin!(changes); + + // When Bob becomes verified + t.verify().await; + + // Then we were notified about the initial state of the room + let change1 = next_change(&mut changes).await; + assert_eq!(change1[0].user_id, t.user_id()); + assert_eq!(change1[0].changed_to, IdentityState::PinViolation); + assert_eq!(change1.len(), 1); + + // And the change when Bob became verified + let change2 = next_change(&mut changes).await; + assert_eq!(change2[0].user_id, t.user_id()); + assert_eq!(change2[0].changed_to, IdentityState::Verified); + assert_eq!(change2.len(), 1); + } + + #[async_test] + async fn test_when_user_in_verification_violation_becomes_verified_we_report_it() { + // Given a room containing us and Bob + let t = TestSetup::new_room_with_other_member().await; + + // And Bob's is in verification violation + t.verify_with( + IdentityChangeDataSet::key_query_with_identity_b(), + IdentityChangeDataSet::msk_b(), + IdentityChangeDataSet::ssk_b(), + ) + .await; + t.unpin().await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + let mut changes = pin!(changes); + + // When Bob becomes verified + t.verify().await; + + // Then we were notified about the initial state of the room + let change1 = next_change(&mut changes).await; + assert_eq!(change1[0].user_id, t.user_id()); + assert_eq!(change1[0].changed_to, IdentityState::VerificationViolation); + assert_eq!(change1.len(), 1); + + // And the change when Bob became verified + let change2 = next_change(&mut changes).await; + assert_eq!(change2[0].user_id, t.user_id()); + assert_eq!(change2[0].changed_to, IdentityState::Verified); + assert_eq!(change2.len(), 1); + } + #[async_test] async fn test_when_an_unpinned_user_joins_we_report_it() { // Given a room containing just us @@ -270,6 +381,53 @@ mod tests { assert_eq!(change.len(), 1); } + #[async_test] + async fn test_when_an_verification_violating_user_joins_we_report_it() { + // Given a room containing just us + let mut t = TestSetup::new_just_me_room().await; + + // And Bob's identity is in verification violation + t.verify().await; + t.unpin().await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + + // When Bob joins the room + t.join().await; + + // Then we were notified about it + let change = next_change(&mut pin!(changes)).await; + assert_eq!(change[0].user_id, t.user_id()); + assert_eq!(change[0].changed_to, IdentityState::VerificationViolation); + assert_eq!(change.len(), 1); + } + + #[async_test] + async fn test_when_a_verified_user_joins_we_dont_report_it() { + // Given a room containing just us + let mut t = TestSetup::new_just_me_room().await; + + // And Bob's identity is verified + t.verify().await; + + // And we are listening for identity changes + let changes = t.subscribe_to_identity_status_changes().await; + + // When Bob joins the room + t.join().await; + + // (Then becomes unpinned so we have something to report) + t.unpin().await; + + //// Then we were only notified about the unpin + let mut changes = pin!(changes); + let change = next_change(&mut changes).await; + assert_eq!(change[0].user_id, t.user_id()); + assert_eq!(change[0].changed_to, IdentityState::VerificationViolation); + assert_eq!(change.len(), 1); + } + #[async_test] async fn test_when_a_pinned_user_joins_we_do_not_report() { // Given a room containing just us @@ -422,8 +580,8 @@ mod tests { changes .next() .await - .expect("There should be an identity update") - .expect("Should not time out") + .expect("Should new reach end of changes stream") + .expect("Should not time out waiting for a change") } mod test_setup { @@ -442,8 +600,9 @@ mod tests { StateTestEvent, SyncResponseBuilder, DEFAULT_TEST_ROOM_ID, }; use ruma::{ - api::client::keys::get_keys, events::room::member::MembershipState, owned_user_id, - OwnedUserId, TransactionId, UserId, + api::client::keys::{get_keys, get_keys::v3::Response as KeyQueryResponse}, + events::room::member::MembershipState, + owned_user_id, OwnedUserId, TransactionId, UserId, }; use serde_json::json; use tokio_stream::{StreamExt as _, Timeout}; @@ -510,13 +669,41 @@ mod tests { } pub(super) async fn unpin(&self) { - // Change/set their identity - this will unpin if they already had one. - // If this was the first time we'd done this, they are now pinned. - self.change_identity(IdentityChangeDataSet::key_query_with_identity_a()).await; + self.unpin_with(IdentityChangeDataSet::key_query_with_identity_b()).await; + } - if self.is_pinned().await { - // Change their identity. Now they are definitely unpinned - self.change_identity(IdentityChangeDataSet::key_query_with_identity_b()).await; + pub(super) async fn unpin_with(&self, requested: KeyQueryResponse) { + fn master_key_json(key_query_response: &KeyQueryResponse) -> String { + serde_json::to_string( + key_query_response + .master_keys + .first_key_value() + .expect("Master key should have a value") + .1, + ) + .expect("Should be able to serialise master key") + } + + let a = IdentityChangeDataSet::key_query_with_identity_a(); + let b = IdentityChangeDataSet::key_query_with_identity_b(); + let requested_master_key = master_key_json(&requested); + let a_master_key = master_key_json(&a); + + // Change/set their identity pin it, then change it again - this will definitely + // unpin, even if the first identity we supply is their very first, making them + // initially pinned. + if requested_master_key == a_master_key { + self.change_identity(b).await; + if !self.is_pinned().await { + self.pin().await; + } + self.change_identity(a).await; + } else { + self.change_identity(a).await; + if !self.is_pinned().await { + self.pin().await; + } + self.change_identity(b).await; } // Sanity: they are unpinned @@ -524,10 +711,22 @@ mod tests { } pub(super) async fn verify(&self) { - // If they don't have an identity yet, set one up - if self.user_identity().await.is_none() { - self.change_identity(IdentityChangeDataSet::key_query_with_identity_a()).await; - } + self.verify_with( + IdentityChangeDataSet::key_query_with_identity_a(), + IdentityChangeDataSet::msk_a(), + IdentityChangeDataSet::ssk_a(), + ) + .await; + } + + pub(super) async fn verify_with( + &self, + key_query: KeyQueryResponse, + msk: serde_json::Value, + ssk: serde_json::Value, + ) { + // Make sure the requested identity is set + self.change_identity(key_query).await; let my_user_id = self.client.user_id().expect("I should have a user id"); let my_identity = self @@ -554,8 +753,8 @@ mod tests { my_identity, my_user_id, self.user_id(), - IdentityChangeDataSet::msk_a(), - IdentityChangeDataSet::ssk_a(), + msk, + ssk, ); // Receive the response into our client