From 2e410e5d94c91de5c0dc0fed347327e662cddc3e Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 5 Aug 2024 13:51:30 +0200 Subject: [PATCH] CodeReview: rename verification_latch to previously verified --- .../src/identities/manager.rs | 60 +++++++++---------- .../matrix-sdk-crypto/src/identities/user.rs | 41 +++++++------ .../src/store/crypto_store_wrapper.rs | 10 ++-- .../src/test_json/keys_query_sets.rs | 4 +- 4 files changed, 59 insertions(+), 56 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index 088ff4771..44c67f94d 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -533,7 +533,7 @@ impl IdentityManager { own_user_identity.is_identity_signed(&identity).is_ok() }); if is_verified { - identity.mark_as_verified_once(); + identity.mark_as_previously_verified(); } Ok(identity.into()) @@ -2065,8 +2065,8 @@ pub(crate) mod tests { // Set up a machine do initial own key query and import cross-signing secret to // make the current session verified. - async fn common_verification_latch_machine_setup() -> OlmMachine { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; + async fn common_verified_identity_changes_machine_setup() -> OlmMachine { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; let machine = OlmMachine::new(DataSet::own_id(), device_id!("LOCAL")).await; @@ -2086,9 +2086,9 @@ pub(crate) mod tests { } #[async_test] async fn test_manager_verified_latch_setup_on_new_identities() { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; - let machine = common_verification_latch_machine_setup().await; + let machine = common_verified_identity_changes_machine_setup().await; // ###### // First test: Assert that the latch is properly set on new identities @@ -2105,7 +2105,7 @@ pub(crate) mod tests { let bob_identity = machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap().other().unwrap(); // The verified latch should be true - assert!(bob_identity.is_verified_latch_set()); + assert!(bob_identity.was_previously_verified()); // And bob is verified assert!(bob_identity.is_verified()); @@ -2121,7 +2121,7 @@ pub(crate) mod tests { // Bob is not verified anymore assert!(!bob_identity.is_verified()); // The verified latch should still be true - assert!(bob_identity.is_verified_latch_set()); + assert!(bob_identity.was_previously_verified()); // Bob device_2 is self-signed even if there is this verification latch // violation let bob_device = machine @@ -2136,15 +2136,15 @@ pub(crate) mod tests { bob_identity.pin_current_master_key().await.unwrap(); assert!(!bob_identity.has_pin_violation()); let has_latch_violation = - bob_identity.is_verified_latch_set() && !bob_identity.is_verified(); + bob_identity.was_previously_verified() && !bob_identity.is_verified(); assert!(has_latch_violation); } #[async_test] - async fn test_manager_verified_latch_setup_on_updated_identities() { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; + async fn test_manager_verified_identity_changes_setup_on_updated_identities() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; - let machine = common_verification_latch_machine_setup().await; + let machine = common_verified_identity_changes_machine_setup().await; // ###### // Get the Carol identity for the first time @@ -2163,10 +2163,10 @@ pub(crate) mod tests { // The identity is not verified assert!(!carol_identity.is_verified()); // The verified latch is off - assert!(!carol_identity.is_verified_latch_set()); + assert!(!carol_identity.was_previously_verified()); // Carol is verified, likely from another session. Ensure the latch is updated - // when the key query response is processes + // when the key query response is processed let keys_query = DataSet::carol_keys_query_response_signed(); let txn_id = TransactionId::new(); machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); @@ -2180,7 +2180,7 @@ pub(crate) mod tests { .unwrap(); assert!(carol_identity.is_verified()); // This should have updated the latch - assert!(carol_identity.is_verified_latch_set()); + assert!(carol_identity.was_previously_verified()); // It is the same identity, it's just signed now so no pin violation assert!(!carol_identity.has_pin_violation()); } @@ -2188,8 +2188,8 @@ pub(crate) mod tests { // Set up a machine do initial own key query. // The cross signing secrets are not yet uploaded. // Then query keys for carol and bob (both signed by own identity) - async fn common_verification_latch_own_trust_change_machine_setup() -> OlmMachine { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; + async fn common_verified_identity_changes_own_trust_change_machine_setup() -> OlmMachine { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; // Start on a non-verified session let machine = OlmMachine::new(DataSet::own_id(), device_id!("LOCAL")).await; @@ -2222,9 +2222,9 @@ pub(crate) mod tests { } #[async_test] - async fn test_manager_verified_latch_setup_on_own_identity_trust_change() { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; - let machine = common_verification_latch_own_trust_change_machine_setup().await; + async fn test_manager_verified_identity_changes_setup_on_own_identity_trust_change() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + let machine = common_verified_identity_changes_own_trust_change_machine_setup().await; let own_identity = machine.get_identity(DataSet::own_id(), None).await.unwrap().unwrap().own().unwrap(); @@ -2233,7 +2233,7 @@ pub(crate) mod tests { machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap().other().unwrap(); // Carol is verified by our identity but our own identity is not yet trusted assert!(own_identity.is_identity_signed(&bob_identity).is_ok()); - assert!(!bob_identity.is_verified_latch_set()); + assert!(!bob_identity.was_previously_verified()); let carol_identity = machine .get_identity(DataSet::carol_id(), None) @@ -2244,7 +2244,7 @@ pub(crate) mod tests { .unwrap(); // Carol is verified by our identity but our own identity is not yet trusted assert!(own_identity.is_identity_signed(&carol_identity).is_ok()); - assert!(!carol_identity.is_verified_latch_set()); + assert!(!carol_identity.was_previously_verified()); // Marking our own identity as trusted should update the existing identities let _ = own_identity.verify().await; @@ -2262,19 +2262,19 @@ pub(crate) mod tests { .unwrap(); assert!(carol_identity.is_verified()); // The latch should be set now - assert!(carol_identity.is_verified_latch_set()); + assert!(carol_identity.was_previously_verified()); let bob_identity = machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap().other().unwrap(); assert!(bob_identity.is_verified()); // The latch should be set now - assert!(bob_identity.is_verified_latch_set()); + assert!(bob_identity.was_previously_verified()); } #[async_test] - async fn test_manager_verified_latch_setup_on_import_secrets() { - use test_json::keys_query_sets::VerificationLatchTestData as DataSet; - let machine = common_verification_latch_own_trust_change_machine_setup().await; + async fn test_manager_verified_identity_change_setup_on_import_secrets() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + let machine = common_verified_identity_changes_own_trust_change_machine_setup().await; let own_identity = machine.get_identity(DataSet::own_id(), None).await.unwrap().unwrap().own().unwrap(); @@ -2283,7 +2283,7 @@ pub(crate) mod tests { machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap().other().unwrap(); // Carol is verified by our identity but our own identity is not yet trusted assert!(own_identity.is_identity_signed(&bob_identity).is_ok()); - assert!(!bob_identity.is_verified_latch_set()); + assert!(!bob_identity.was_previously_verified()); let carol_identity = machine .get_identity(DataSet::carol_id(), None) @@ -2294,7 +2294,7 @@ pub(crate) mod tests { .unwrap(); // Carol is verified by our identity but our own identity is not yet trusted assert!(own_identity.is_identity_signed(&carol_identity).is_ok()); - assert!(!carol_identity.is_verified_latch_set()); + assert!(!carol_identity.was_previously_verified()); // Marking our own identity as trusted should update the existing identities machine @@ -2319,12 +2319,12 @@ pub(crate) mod tests { .unwrap(); assert!(carol_identity.is_verified()); // The latch should be set now - assert!(carol_identity.is_verified_latch_set()); + assert!(carol_identity.was_previously_verified()); let bob_identity = machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap().other().unwrap(); assert!(bob_identity.is_verified()); // The latch should be set now - assert!(bob_identity.is_verified_latch_set()); + assert!(bob_identity.was_previously_verified()); } } diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 120087152..126a57e2f 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -427,9 +427,10 @@ impl UserIdentityData { /// identity changes (for example, in case of key material or device loss). /// /// As soon as the cryptographic identity is verified (i.e. signed by our own -/// trusted identity), a latch is set to remember it (`verified_latch`). Future -/// interactions will expect this user to stay verified, in case of violation -/// the user should be notified with a blocking warning. +/// trusted identity), a flag is set to remember it (`previously_verified`). +/// Future interactions will expect this user to stay verified, in case of +/// violation the user should be notified with a blocking warning when sending a +/// message. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(try_from = "OtherUserIdentityDataSerializer", into = "OtherUserIdentityDataSerializer")] pub struct OtherUserIdentityData { @@ -437,10 +438,10 @@ pub struct OtherUserIdentityData { pub(crate) master_key: Arc, self_signing_key: Arc, pinned_master_key: Arc>, - /// This tracks whether we've ever verified this user with any identity at + /// This tracks whether we've verified this user with any identity at /// any point in time on this device. To use it in the future to detect /// cases where the user has become unverified for any reason. - verified_latch: Arc, + previously_verified: Arc, } /// Intermediate struct to help serialize OtherUserIdentityData and support @@ -476,7 +477,7 @@ struct OtherUserIdentityDataSerializerV2 { master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, pinned_master_key: MasterPubkey, - verified_latch: bool, + previously_verified: bool, } impl TryFrom for OtherUserIdentityData { @@ -494,7 +495,7 @@ impl TryFrom for OtherUserIdentityData { self_signing_key: Arc::new(v0.self_signing_key), // We migrate by pinning the current master key pinned_master_key: Arc::new(RwLock::new(v0.master_key)), - verified_latch: Arc::new(false.into()), + previously_verified: Arc::new(false.into()), }) } Some(v) if v == "1" => { @@ -506,7 +507,7 @@ impl TryFrom for OtherUserIdentityData { pinned_master_key: Arc::new(RwLock::new(v1.pinned_master_key)), // Put it to false. There will be a migration to mark all users as dirty, so we // will receive an update for the identity that will correctly set up the value. - verified_latch: Arc::new(false.into()), + previously_verified: Arc::new(false.into()), }) } Some(v) if v == "2" => { @@ -516,7 +517,7 @@ impl TryFrom for OtherUserIdentityData { master_key: Arc::new(v2.master_key.clone()), self_signing_key: Arc::new(v2.self_signing_key), pinned_master_key: Arc::new(RwLock::new(v2.pinned_master_key)), - verified_latch: Arc::new(v2.verified_latch.into()), + previously_verified: Arc::new(v2.previously_verified.into()), }) } _ => Err(serde::de::Error::custom(format!("Unsupported Version {:?}", value.version))), @@ -531,7 +532,7 @@ impl From for OtherUserIdentityDataSerializer { master_key: value.master_key().to_owned(), self_signing_key: value.self_signing_key().to_owned(), pinned_master_key: value.pinned_master_key.read().unwrap().clone(), - verified_latch: value.verified_latch.load(Ordering::SeqCst), + previously_verified: value.previously_verified.load(Ordering::SeqCst), }; OtherUserIdentityDataSerializer { version: Some("2".to_owned()), @@ -583,7 +584,7 @@ impl OtherUserIdentityData { master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), pinned_master_key: RwLock::new(master_key).into(), - verified_latch: Arc::new(false.into()), + previously_verified: Arc::new(false.into()), }) } @@ -598,7 +599,7 @@ impl OtherUserIdentityData { master_key: Arc::new(master_key.clone()), self_signing_key, pinned_master_key: Arc::new(RwLock::new(master_key.clone())), - verified_latch: Arc::new(false.into()), + previously_verified: Arc::new(false.into()), } } @@ -624,17 +625,17 @@ impl OtherUserIdentityData { } /// Remember that this identity used to be verified at some point. - pub(crate) fn mark_as_verified_once(&self) { - self.verified_latch.store(true, Ordering::SeqCst) + pub(crate) fn mark_as_previously_verified(&self) { + self.previously_verified.store(true, Ordering::SeqCst) } - /// True if we ever verified this identity (with any own identity, at any + /// True if we verified this identity (with any own identity, at any /// point). /// /// To pass this latch back to false, one must call /// [`OtherUserIdentityData::withdraw_verification()`]. - pub fn is_verified_latch_set(&self) -> bool { - self.verified_latch.load(Ordering::SeqCst) + pub fn was_previously_verified(&self) -> bool { + self.previously_verified.load(Ordering::SeqCst) } /// Remove the requirement for this identity to be verified. @@ -643,7 +644,7 @@ impl OtherUserIdentityData { /// reported to the user. In order to remove this notice users have to /// verify again or to withdraw the verification requirement. pub fn withdraw_verification(&self) { - self.verified_latch.store(false, Ordering::SeqCst) + self.previously_verified.store(false, Ordering::SeqCst) } /// Returns true if the identity has changed since we last pinned it. @@ -700,7 +701,9 @@ impl OtherUserIdentityData { master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), pinned_master_key: RwLock::new(pinned_master_key).into(), - verified_latch: Arc::new((self.is_verified_latch_set() || updated_is_verified).into()), + previously_verified: Arc::new( + (self.was_previously_verified() || updated_is_verified).into(), + ), }; let changed = new != *self; diff --git a/crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs b/crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs index b3f715eaa..1f4997dc2 100644 --- a/crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs +++ b/crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs @@ -151,12 +151,12 @@ impl CryptoStoreWrapper { // identities. if let Some(own_identity_after) = maybe_own_identity.as_ref() { // Only do this if our identity is passing from not verified to verified, - // the verified_latch can only go this way. + // the previously_verified can only change in that case. if !own_identity_was_verified_before_change && own_identity_after.is_verified() { debug!("Own identity is now verified, check all known identities for verification status changes"); // We need to review all the other identities to see if they are verified now // and mark them as such - self.check_all_identities_and_update_verified_latch_if_needed( + self.check_all_identities_and_update_was_previousy_verified_flag_if_needed( own_identity_after, ) .await?; @@ -169,7 +169,7 @@ impl CryptoStoreWrapper { Ok(()) } - async fn check_all_identities_and_update_verified_latch_if_needed( + async fn check_all_identities_and_update_was_previousy_verified_flag_if_needed( &self, own_identity_after: &OwnUserIdentityData, ) -> Result<(), CryptoStoreError> { @@ -183,11 +183,11 @@ impl CryptoStoreWrapper { .as_ref() .and_then(|i| i.other()) { - if !other_identity.is_verified_latch_set() + if !other_identity.was_previously_verified() && own_identity_after.is_identity_signed(other_identity).is_ok() { trace!(?tracked_user.user_id, "Marking set verified_latch to true."); - other_identity.mark_as_verified_once(); + other_identity.mark_as_previously_verified(); updated_identities.push(other_identity.clone().into()); } } diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index d481c09ea..cfa290427 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -668,10 +668,10 @@ impl IdentityChangeDataSet { } } -pub struct VerificationLatchTestData {} +pub struct PreviouslyVerifiedTestData {} #[allow(dead_code)] -impl VerificationLatchTestData { +impl PreviouslyVerifiedTestData { pub const MASTER_KEY_PRIVATE_EXPORT: &'static str = "bSa0nVTocZArMzL7OLmeFUIVF4ycp64rrkVMgqOYg6Y"; pub const SELF_SIGNING_KEY_PRIVATE_EXPORT: &'static str =