From 0ecdc2f43c8ced3474fcd6abbdc248aee5df25f9 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 18 Sep 2023 16:01:53 +0200 Subject: [PATCH] fix(e2ee): query keys for untracked users even if we didn't explicitly sync members ourselves --- crates/matrix-sdk/src/room/mod.rs | 15 +++- .../src/tests/e2ee.rs | 76 +++++++++++++++---- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/crates/matrix-sdk/src/room/mod.rs b/crates/matrix-sdk/src/room/mod.rs index 86a2ab9e4..9aafd5855 100644 --- a/crates/matrix-sdk/src/room/mod.rs +++ b/crates/matrix-sdk/src/room/mod.rs @@ -1449,7 +1449,10 @@ impl Room { let (req_id, request) = olm.query_keys_for_users(members_with_unknown_devices.map(|owned| owned.borrow())); - self.client.keys_query(&req_id, request.device_keys).await?; + if !request.device_keys.is_empty() { + self.client.keys_query(&req_id, request.device_keys).await?; + } + Ok(()) } @@ -1549,11 +1552,15 @@ impl Room { if !self.are_members_synced() { self.sync_members().await?; - - // Query keys in case we don't have them for newly synced members. - self.query_keys_for_untracked_users().await?; } + // Query keys in case we don't have them for newly synced members. + // + // Note we do it all the time, because we might have sync'd members before + // sending a message (so didn't enter the above branch), but + // could have not query their keys ever. + self.query_keys_for_untracked_users().await?; + self.preshare_room_key().await?; let olm = self.client.olm_machine().await; diff --git a/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs b/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs index a74286ae3..484c00ee4 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs @@ -33,7 +33,6 @@ async fn test_encryption_missing_member_keys() -> Result<()> { bob.sync_once().await?; bob.get_room(alice_room.room_id()).unwrap().join().await?; - bob.sync_once().await?; // TODO poljar says this shouldn't be required, but it is in practice? warn!("bob has joined"); @@ -50,31 +49,82 @@ async fn test_encryption_missing_member_keys() -> Result<()> { // Bob sends message WITHOUT syncing. warn!("bob sends message..."); let bob_room = bob.get_room(alice_room.room_id()).unwrap(); - bob_room.send(RoomMessageEventContent::text_plain("Hello world!"), None).await?; + let message = "Hello world!"; + let bob_message_content = Arc::new(Mutex::new(message)); + bob_room.send(RoomMessageEventContent::text_plain(message), None).await?; warn!("bob is done sending the message"); - // All the other uses get to decrypt the message. - for user in [alice, carl] { - warn!("{} is looking for decrypted message", user.user_id().unwrap()); + // Alice was in the room when Bob sent the message, so they'll see it. + let alice_found_event = Arc::new(Mutex::new(false)); + { + warn!("alice is looking for decrypted message"); - let found_event = Arc::new(Mutex::new(false)); - - let found_event_handler = found_event.clone(); - user.add_event_handler(move |event: SyncRoomMessageEvent| async move { + let found_event_handler = alice_found_event.clone(); + let bob_message_content = bob_message_content.clone(); + alice.add_event_handler(move |event: SyncRoomMessageEvent| async move { warn!("Found a message \\o/ {event:?}"); let MessageType::Text(text_content) = &event.as_original().unwrap().content.msgtype else { return; }; - if text_content.body == "Hello world!" { + if text_content.body == *bob_message_content.lock().unwrap() { *found_event_handler.lock().unwrap() = true; } }); - user.sync_once().await?; + alice.sync_once().await?; - let found = *found_event.lock().unwrap(); - assert!(found, "event has not been found for {}", user.user_id().unwrap()); + let found = *alice_found_event.lock().unwrap(); + assert!(found, "event has not been found for alice"); + } + + // Bob wasn't aware of Carl's presence (no sync() since Carl joined), so Carl + // won't see it first. + let carl_found_event = Arc::new(Mutex::new(false)); + { + warn!("carl is looking for decrypted message"); + + let found_event_handler = carl_found_event.clone(); + let bob_message_content = bob_message_content.clone(); + carl.add_event_handler(move |event: SyncRoomMessageEvent| async move { + warn!("Found a message \\o/ {event:?}"); + let MessageType::Text(text_content) = &event.as_original().unwrap().content.msgtype + else { + return; + }; + if text_content.body == *bob_message_content.lock().unwrap() { + *found_event_handler.lock().unwrap() = true; + } + }); + + carl.sync_once().await?; + + let found = *carl_found_event.lock().unwrap(); + assert!(!found, "event has been unexpectedly found for carl"); + } + + // Now Bob syncs, thus notices the presence of Carl. + bob.sync_once().await?; + + warn!("bob sends another message..."); + let bob_room = bob.get_room(alice_room.room_id()).unwrap(); + let message = "Wassup"; + *bob_message_content.lock().unwrap() = message; + bob_room.send(RoomMessageEventContent::text_plain(message), None).await?; + warn!("bob is done sending another message"); + + { + *alice_found_event.lock().unwrap() = false; + alice.sync_once().await?; + let found = *alice_found_event.lock().unwrap(); + assert!(found, "second message has not been found for alice"); + } + + { + *carl_found_event.lock().unwrap() = false; + carl.sync_once().await?; + let found = *carl_found_event.lock().unwrap(); + assert!(found, "second message has not been found for carl"); } Ok(())