From 629421214fa4ab77b08e415e1b68dceb84e01e85 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Mon, 17 Feb 2025 11:48:35 +0200 Subject: [PATCH] change(crypto): have the RoomIdentityProvider return processing changes when identities transition to IdentityState::Verified too (#4670) While implementing user verification on Element X I realized that the `IdentityStatusChanges` stream does not notify us when an user becomes `Verified`, which is a shame as it's perfect for powering app updates after verifying users (i.e. all the decorations we show throughout the app: room header, room details, room member list, room member profile etc.) Had a look at the existing code and, while that seems completely intentional, there is no reason why we can't expand its purview. --- crates/matrix-sdk-crypto/CHANGELOG.md | 4 ++ .../src/identities/room_identity_state.rs | 64 +++++++------------ .../src/room/identity_status_changes.rs | 10 ++- 3 files changed, 36 insertions(+), 42 deletions(-) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 79de982d2..b9c9b34d8 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -21,6 +21,10 @@ All notable changes to this project will be documented in this file. - Room keys are not shared with unsigned dehydrated devices. ([#4551](https://github.com/matrix-org/matrix-rust-sdk/pull/4551)) + +- Have the `RoomIdentityProvider` return processing changes when identities transition + to `IdentityState::Verified` too. + ([#4670](https://github.com/matrix-org/matrix-rust-sdk/pull/4670)) ## [0.9.0] - 2024-12-18 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 1d7c65aa4..c567cf21b 100644 --- a/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs +++ b/crates/matrix-sdk-crypto/src/identities/room_identity_state.rs @@ -146,6 +146,14 @@ impl RoomIdentityState { if let Some(user_identity @ UserIdentity::Other(_)) = self.room.user_identity(user_id).await { + // Don't notify on membership changes of verified or pinned identities + if matches!( + self.room.state_of(&user_identity), + IdentityState::Verified | IdentityState::Pinned + ) { + return vec![]; + } + match event.content.membership { MembershipState::Join | MembershipState::Invite => { // They are joining the room - check whether we need to display a @@ -192,50 +200,20 @@ impl RoomIdentityState { } /// Updates our internal state for this user to the supplied `new_state`. If - /// the change of state is significant (it requires something to change - /// in the UI, like a warning being added or removed), returns the - /// change information we will surface to the UI. + /// the state changed it returns the change information we will surface to + /// the UI. 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); - match (old_state, &new_state) { - // good -> bad - report so we can add a message - (Pinned, PinViolation) | - (Pinned, VerificationViolation) | - (Verified, PinViolation) | - (Verified, VerificationViolation) | - - // bad -> good - report so we can remove a message - (PinViolation, Pinned) | - (PinViolation, Verified) | - (VerificationViolation, Pinned) | - (VerificationViolation, Verified) | - - // Changed the type of bad - report so can change the message - (PinViolation, VerificationViolation) | - (VerificationViolation, PinViolation) => Some(self.set_state(user_id, new_state)), - - // good -> good - don't report - no message needed in either case - (Pinned, Verified) | - (Verified, Pinned) => { - // The state has changed, so we update it - self.set_state(user_id, new_state); - // but there is no need to report a change to the UI - None - } - - // State didn't change - don't report - nothing changed - (Pinned, Pinned) | - (Verified, Verified) | - (PinViolation, PinViolation) | - (VerificationViolation, VerificationViolation) => None, + if old_state == new_state { + return None; } + + Some(self.set_state(user_id, new_state)) } fn set_state(&mut self, user_id: &UserId, new_state: IdentityState) -> IdentityStatusChange { @@ -397,7 +375,7 @@ mod tests { } #[async_test] - async fn test_verifying_a_pinned_identity_in_the_room_does_nothing() { + async fn test_verifying_a_pinned_identity_in_the_room_notifies() { // Given someone in the room is pinned let user_id = user_id!("@u:s.co"); let mut room = FakeRoom::new(); @@ -409,8 +387,14 @@ mod tests { 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![]); + // Then we emit an update + assert_eq!( + update, + vec![IdentityStatusChange { + user_id: user_id.to_owned(), + changed_to: IdentityState::Verified + }] + ); } #[async_test] @@ -722,7 +706,7 @@ mod tests { #[async_test] async fn test_a_verified_identity_leaving_the_room_does_nothing() { - // Given a pinned user is in the room + // Given a verified 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); diff --git a/crates/matrix-sdk/src/room/identity_status_changes.rs b/crates/matrix-sdk/src/room/identity_status_changes.rs index c7386249b..e37ccd2ad 100644 --- a/crates/matrix-sdk/src/room/identity_status_changes.rs +++ b/crates/matrix-sdk/src/room/identity_status_changes.rs @@ -276,7 +276,7 @@ mod tests { } #[async_test] - async fn test_when_user_becomes_verified_we_dont_report_it() { + async fn test_when_user_becomes_verified_we_report_it() { // Given a room containing us and Bob let t = TestSetup::new_room_with_other_bob().await; @@ -287,10 +287,16 @@ mod tests { // When Bob becomes verified t.verify_bob().await; + // Then we are notified about Bob being verified + let change = assert_next_with_timeout!(stream); + assert_eq!(change[0].user_id, t.bob_user_id()); + assert_eq!(change[0].changed_to, IdentityState::Verified); + assert_eq!(change.len(), 1); + // (And then unpinned, so we have something to come through the stream) t.unpin_bob().await; - // Then we are only notified about the unpinning part + // Then we are notified about the unpinning part let change = assert_next_with_timeout!(stream); assert_eq!(change[0].user_id, t.bob_user_id()); assert_eq!(change[0].changed_to, IdentityState::VerificationViolation);