From 78fa3125e220eb0dd581fd301823d4a7ddb80510 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 12 Nov 2024 17:54:32 +0100 Subject: [PATCH 1/4] fix(utd_hook): Fix regression causing retry to report false late decrypt --- .../src/timeline/controller/mod.rs | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 8f09d4f309..c2d0a9e5db 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -1064,16 +1064,22 @@ impl TimelineController

{ match decryptor.decrypt_event_impl(original_json).await { Ok(event) => { - trace!( - "Successfully decrypted event that previously failed to decrypt" - ); - - // Notify observers that we managed to eventually decrypt an event. - if let Some(hook) = unable_to_decrypt_hook { - hook.on_late_decrypt(&remote_event.event_id, *utd_cause).await; + if let matrix_sdk::deserialized_responses::TimelineEventKind::UnableToDecrypt { utd_info, ..} = event.kind { + info!("Failed to decrypt event after receiving room key: {:?}", utd_info.reason); + None + } else { + trace!( + kind = ?event.kind, + "Successfully decrypted event that previously failed to decrypt" + ); + + // Notify observers that we managed to eventually decrypt an event. + if let Some(hook) = unable_to_decrypt_hook { + hook.on_late_decrypt(&remote_event.event_id, *utd_cause).await; + } + + Some(event) } - - Some(event) } Err(e) => { info!("Failed to decrypt event after receiving room key: {e}"); From 6fc6c2c7d54e08fd4f86b2379444ce3ff26595c1 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 15 Nov 2024 17:20:03 +0100 Subject: [PATCH 2/4] fix(utd_hook): Add regression test --- .../src/timeline/tests/encryption.rs | 90 ++++++++++++++++--- crates/matrix-sdk-ui/src/timeline/traits.rs | 14 ++- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs b/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs index d7e0f812d6..f2f6110ba4 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs @@ -18,6 +18,7 @@ use std::{ io::Cursor, iter, sync::{Arc, Mutex}, + time::Duration, }; use as_variant::as_variant; @@ -43,6 +44,7 @@ use ruma::{ }; use serde_json::{json, value::to_raw_value}; use stream_assert::assert_next_matches; +use tokio::time::sleep; use super::TestTimeline; use crate::{ @@ -50,6 +52,17 @@ use crate::{ unable_to_decrypt_hook::{UnableToDecryptHook, UnableToDecryptInfo, UtdHookManager}, }; +#[derive(Debug, Default)] +struct DummyUtdHook { + utds: Mutex>, +} + +impl UnableToDecryptHook for DummyUtdHook { + fn on_utd(&self, info: UnableToDecryptInfo) { + self.utds.lock().unwrap().push(info); + } +} + #[async_test] async fn test_retry_message_decryption() { const SESSION_ID: &str = "gM8i47Xhu0q52xLfgUXzanCMpLinoyVyH7R58cBuVBU"; @@ -67,17 +80,6 @@ async fn test_retry_message_decryption() { HztoSJUr/2Y\n\ -----END MEGOLM SESSION DATA-----"; - #[derive(Debug, Default)] - struct DummyUtdHook { - utds: Mutex>, - } - - impl UnableToDecryptHook for DummyUtdHook { - fn on_utd(&self, info: UnableToDecryptInfo) { - self.utds.lock().unwrap().push(info); - } - } - let hook = Arc::new(DummyUtdHook::default()); let client = test_client_builder(None).build().await.unwrap(); let utd_hook = Arc::new(UtdHookManager::new(hook.clone(), client)); @@ -170,6 +172,72 @@ async fn test_retry_message_decryption() { } } +// There has been a regression when the `retry_event_decryption` function +// changed from failing with an Error to instead return a new type of timeline +// event in UTD. The regression caused the timeline to consider any +// re-decryption attempt as successful. +#[async_test] +async fn test_false_positive_late_decryption_regression() { + const SESSION_ID: &str = "gM8i47Xhu0q52xLfgUXzanCMpLinoyVyH7R58cBuVBU"; + + let hook = Arc::new(DummyUtdHook::default()); + let client = test_client_builder(None).build().await.unwrap(); + let utd_hook = + Arc::new(UtdHookManager::new(hook.clone(), client).with_max_delay(Duration::from_secs(1))); + + let timeline = TestTimeline::with_unable_to_decrypt_hook(utd_hook.clone()); + + let f = &timeline.factory; + timeline + .handle_live_event( + f.event(RoomEncryptedEventContent::new( + EncryptedEventScheme::MegolmV1AesSha2( + MegolmV1AesSha2ContentInit { + ciphertext: "\ + AwgAEtABPRMavuZMDJrPo6pGQP4qVmpcuapuXtzKXJyi3YpEsjSWdzuRKIgJzD4P\ + cSqJM1A8kzxecTQNJsC5q22+KSFEPxPnI4ltpm7GFowSoPSW9+bFdnlfUzEP1jPq\ + YevHAsMJp2fRKkzQQbPordrUk1gNqEpGl4BYFeRqKl9GPdKFwy45huvQCLNNueql\ + CFZVoYMuhxrfyMiJJAVNTofkr2um2mKjDTlajHtr39pTG8k0eOjSXkLOSdZvNOMz\ + hGhSaFNeERSA2G2YbeknOvU7MvjiO0AKuxaAe1CaVhAI14FCgzrJ8g0y5nly+n7x\ + QzL2G2Dn8EoXM5Iqj8W99iokQoVsSrUEnaQ1WnSIfewvDDt4LCaD/w7PGETMCQ" + .to_owned(), + sender_key: "DeHIg4gwhClxzFYcmNntPNF9YtsdZbmMy8+3kzCMXHA".to_owned(), + device_id: "NLAZCWIOCO".into(), + session_id: SESSION_ID.into(), + } + .into(), + ), + None, + )) + .sender(&BOB) + .into_utd_sync_timeline_event(), + ) + .await; + + let own_user_id = user_id!("@example:morheus.localhost"); + let olm_machine = OlmMachine::new(own_user_id, "SomeDeviceId".into()).await; + + timeline + .controller + .retry_event_decryption_test( + room_id!("!DovneieKSTkdHKpIXy:morpheus.localhost"), + olm_machine, + Some(iter::once(SESSION_ID.to_owned()).collect()), + ) + .await; + assert_eq!(timeline.controller.items().await.len(), 2); + + // Wait past the max delay for utd late decryption detection + sleep(Duration::from_secs(2)).await; + + { + let utds = hook.utds.lock().unwrap(); + assert_eq!(utds.len(), 1); + // Without the regression fix this would be Some(time to decrypt) + assert!(utds[0].time_to_decrypt.is_none()); + } +} + #[async_test] async fn test_retry_edit_decryption() { const SESSION1_KEY: &[u8] = b"\ diff --git a/crates/matrix-sdk-ui/src/timeline/traits.rs b/crates/matrix-sdk-ui/src/timeline/traits.rs index 158488eb06..24015c4677 100644 --- a/crates/matrix-sdk-ui/src/timeline/traits.rs +++ b/crates/matrix-sdk-ui/src/timeline/traits.rs @@ -18,7 +18,7 @@ use eyeball::Subscriber; use futures_util::FutureExt as _; use indexmap::IndexMap; #[cfg(test)] -use matrix_sdk::crypto::{DecryptionSettings, TrustRequirement}; +use matrix_sdk::crypto::{DecryptionSettings, RoomEventDecryptionResult, TrustRequirement}; use matrix_sdk::{ deserialized_responses::TimelineEvent, event_cache::paginator::PaginableRoom, BoxFuture, Result, Room, @@ -302,8 +302,14 @@ impl Decryptor for (matrix_sdk_base::crypto::OlmMachine, ruma::OwnedRoomId) { let (olm_machine, room_id) = self; let decryption_settings = DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted }; - let event = - olm_machine.decrypt_room_event(raw.cast_ref(), room_id, &decryption_settings).await?; - Ok(event.into()) + match olm_machine + .try_decrypt_room_event(raw.cast_ref(), room_id, &decryption_settings) + .await? + { + RoomEventDecryptionResult::Decrypted(decrypted) => Ok(decrypted.into()), + RoomEventDecryptionResult::UnableToDecrypt(utd_info) => { + Ok(TimelineEvent::new_utd_event(raw.clone(), utd_info)) + } + } } } From aef2efec15680e6f55622155a96b774323aca7f4 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 18 Nov 2024 17:46:44 +0100 Subject: [PATCH 3/4] review: Better comment --- crates/matrix-sdk-ui/src/timeline/tests/encryption.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs b/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs index f2f6110ba4..131cffae58 100644 --- a/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs +++ b/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs @@ -233,7 +233,8 @@ async fn test_false_positive_late_decryption_regression() { { let utds = hook.utds.lock().unwrap(); assert_eq!(utds.len(), 1); - // Without the regression fix this would be Some(time to decrypt) + // This is the main thing we're testing: if this wasn't identified as a definite + // UTD, this would be `Some(..)`. assert!(utds[0].time_to_decrypt.is_none()); } } From 24ef0849d05fe4542b04288c6160c7e0d9f847be Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 18 Nov 2024 17:47:22 +0100 Subject: [PATCH 4/4] logs: Remove spammy log for succesful decryption --- crates/matrix-sdk-ui/src/timeline/controller/mod.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index c2d0a9e5db..0eaaefbcf7 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -1068,11 +1068,6 @@ impl TimelineController

{ info!("Failed to decrypt event after receiving room key: {:?}", utd_info.reason); None } else { - trace!( - kind = ?event.kind, - "Successfully decrypted event that previously failed to decrypt" - ); - // Notify observers that we managed to eventually decrypt an event. if let Some(hook) = unable_to_decrypt_hook { hook.on_late_decrypt(&remote_event.event_id, *utd_cause).await;