Skip to content

Commit ff18147

Browse files
jmartinesppoljar
authored andcommitted
fix(client): Add handle_verification_events field to BaseClient.
This is done to fix an issue with these events being received and processed twice when `NotificationProcessSetup` is `SingleProcess`, causing issues with user verification. This can be used to ignore verification requests in this sliding sync instance, preventing issues found where several sliding sync instances with the same client process events simultaneously and re-process the same verification request events during their initial syncs.
1 parent 074c0e5 commit ff18147

File tree

5 files changed

+143
-2
lines changed

5 files changed

+143
-2
lines changed

crates/matrix-sdk-base/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ All notable changes to this project will be documented in this file.
1515
- `EventCacheStoreMedia` has a new method `last_media_cleanup_time_inner`
1616
- There are new `'static` bounds in `MediaService` for the media cache stores
1717
- `event_cache::store::MemoryStore` implements `Clone`.
18+
- `BaseClient` now has a `handle_verification_events` field which is `true` by
19+
default and can be negated so the `NotificationClient` won't handle received
20+
verification events too, causing errors in the `VerificationMachine`.
1821

1922
## [0.10.0] - 2025-02-04
2023

crates/matrix-sdk-base/src/client.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ pub struct BaseClient {
124124
/// The trust requirement to use for decrypting events.
125125
#[cfg(feature = "e2e-encryption")]
126126
pub decryption_trust_requirement: TrustRequirement,
127+
128+
/// If the client should handle verification events received when syncing.
129+
#[cfg(feature = "e2e-encryption")]
130+
pub handle_verification_events: bool,
127131
}
128132

129133
#[cfg(not(tarpaulin_include))]
@@ -160,6 +164,8 @@ impl BaseClient {
160164
room_key_recipient_strategy: Default::default(),
161165
#[cfg(feature = "e2e-encryption")]
162166
decryption_trust_requirement: TrustRequirement::Untrusted,
167+
#[cfg(feature = "e2e-encryption")]
168+
handle_verification_events: true,
163169
}
164170
}
165171

@@ -169,6 +175,7 @@ impl BaseClient {
169175
pub async fn clone_with_in_memory_state_store(
170176
&self,
171177
cross_process_store_locks_holder_name: &str,
178+
handle_verification_events: bool,
172179
) -> Result<Self> {
173180
let config = StoreConfig::new(cross_process_store_locks_holder_name.to_owned())
174181
.state_store(MemoryStore::new());
@@ -189,6 +196,7 @@ impl BaseClient {
189196
room_info_notable_update_sender: self.room_info_notable_update_sender.clone(),
190197
room_key_recipient_strategy: self.room_key_recipient_strategy.clone(),
191198
decryption_trust_requirement: self.decryption_trust_requirement,
199+
handle_verification_events,
192200
};
193201

194202
if let Some(session_meta) = self.session_meta().cloned() {
@@ -207,6 +215,7 @@ impl BaseClient {
207215
pub async fn clone_with_in_memory_state_store(
208216
&self,
209217
cross_process_store_locks_holder: &str,
218+
_handle_verification_events: bool,
210219
) -> Result<Self> {
211220
let config = StoreConfig::new(cross_process_store_locks_holder.to_owned())
212221
.state_store(MemoryStore::new());
@@ -339,6 +348,10 @@ impl BaseClient {
339348
event: &AnySyncMessageLikeEvent,
340349
room_id: &RoomId,
341350
) -> Result<()> {
351+
if !self.handle_verification_events {
352+
return Ok(());
353+
}
354+
342355
if let Some(olm) = self.olm_machine().await.as_ref() {
343356
olm.receive_verification_event(&event.clone().into_full_event(room_id.to_owned()))
344357
.await?;

crates/matrix-sdk/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ simpler methods:
2929
[BCP 195](https://datatracker.ietf.org/doc/bcp195/).
3030
([#4647](https://github.com/matrix-org/matrix-rust-sdk/pull/4647))
3131
- Add `Room::report_room` api. ([#4713](https://github.com/matrix-org/matrix-rust-sdk/pull/4713))
32+
- `Client::notification_client` will create a copy of the existing `Client`, but now it'll make sure
33+
it doesn't handle any verification events to avoid an issue with these events being received and
34+
processed twice if `NotificationProcessSetup` was `SingleSetup`.
3235

3336
### Bug Fixes
3437

crates/matrix-sdk/src/client/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2422,7 +2422,7 @@ impl Client {
24222422
self.inner.http_client.clone(),
24232423
self.inner
24242424
.base_client
2425-
.clone_with_in_memory_state_store(&cross_process_store_locks_holder_name)
2425+
.clone_with_in_memory_state_store(&cross_process_store_locks_holder_name, false)
24262426
.await?,
24272427
self.inner.caches.server_capabilities.read().await.clone(),
24282428
self.inner.respect_login_well_known,

testing/matrix-sdk-integration-testing/src/tests/e2ee.rs

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
use std::sync::{Arc, Mutex};
1+
use std::{
2+
sync::{Arc, Mutex},
3+
time::Duration,
4+
};
25

36
use anyhow::Result;
47
use assert_matches::assert_matches;
@@ -26,14 +29,133 @@ use matrix_sdk::{
2629
secret_storage::secret::SecretEventContent,
2730
GlobalAccountDataEventType, OriginalSyncMessageLikeEvent,
2831
},
32+
OwnedEventId,
2933
},
34+
timeout::timeout,
3035
Client,
3136
};
37+
use matrix_sdk_ui::{
38+
notification_client::{NotificationClient, NotificationProcessSetup},
39+
sync_service::SyncService,
40+
};
3241
use similar_asserts::assert_eq;
3342
use tracing::{debug, warn};
3443

3544
use crate::helpers::{SyncTokenAwareClient, TestClientBuilder};
3645

46+
// This test reproduces a bug seen on clients that use the same `Client`
47+
// instance for both the usual sliding sync loop and for getting the event for a
48+
// notification (i.e. Element X Android). The verification events will be
49+
// processed twice, meaning incorrect verification states will be found and the
50+
// process will fail, especially with user verification.
51+
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
52+
async fn test_mutual_sas_verification_with_notification_client_ignores_verification_events(
53+
) -> Result<()> {
54+
let encryption_settings =
55+
EncryptionSettings { auto_enable_cross_signing: true, ..Default::default() };
56+
let alice = TestClientBuilder::new("alice")
57+
.use_sqlite()
58+
.encryption_settings(encryption_settings)
59+
.build()
60+
.await?;
61+
62+
let alice_sync_service =
63+
Arc::new(SyncService::builder(alice.clone()).build().await.expect("Wat"));
64+
65+
let bob = TestClientBuilder::new("bob")
66+
.use_sqlite()
67+
.encryption_settings(encryption_settings)
68+
.build()
69+
.await?;
70+
71+
let bob_sync_service = Arc::new(SyncService::builder(bob.clone()).build().await.expect("Wat"));
72+
let bob_id = bob.user_id().expect("Bob should be logged in by now");
73+
74+
alice.encryption().wait_for_e2ee_initialization_tasks().await;
75+
bob.encryption().wait_for_e2ee_initialization_tasks().await;
76+
77+
alice_sync_service.start().await;
78+
bob_sync_service.start().await;
79+
80+
warn!("alice's device: {}", alice.device_id().unwrap());
81+
warn!("bob's device: {}", bob.device_id().unwrap());
82+
83+
// Set up the test: Alice creates the DM room, and invites Bob, who joins
84+
let invite = vec![bob_id.to_owned()];
85+
let request = assign!(CreateRoomRequest::new(), {
86+
invite,
87+
is_direct: true,
88+
});
89+
90+
let alice_room = alice.create_room(request).await?;
91+
alice_room.enable_encryption().await?;
92+
let room_id = alice_room.room_id();
93+
94+
warn!("alice has created and enabled encryption in the room");
95+
96+
timeout(
97+
async {
98+
loop {
99+
if let Some(room) = bob.get_room(room_id) {
100+
room.join().await.expect("We should be able to join a room");
101+
return;
102+
}
103+
}
104+
},
105+
Duration::from_secs(1),
106+
)
107+
.await
108+
.expect("Bob should have joined the room");
109+
110+
bob_sync_service.stop().await;
111+
112+
warn!("alice and bob are both aware of each other in the e2ee room");
113+
114+
alice_room
115+
.send(RoomMessageEventContent::text_plain("Hello Bob"))
116+
.await
117+
.expect("We should be able to send a message to the room");
118+
119+
let alice_bob_identity = alice
120+
.encryption()
121+
.get_user_identity(bob_id)
122+
.await
123+
.expect("We should be able to fetch an identity from the store")
124+
.expect("Bob's identity should be known by now");
125+
126+
warn!("alice has found bob's identity");
127+
128+
let alice_verification_request = alice_bob_identity.request_verification().await?;
129+
130+
// The notification client must use the `SingleProcess` setup
131+
let notification_client = NotificationClient::new(
132+
bob.clone(),
133+
NotificationProcessSetup::SingleProcess { sync_service: bob_sync_service.clone() },
134+
)
135+
.await
136+
.expect("couldn't create notification client");
137+
138+
let event_id = OwnedEventId::try_from(alice_verification_request.flow_id())
139+
.expect("We should be able to get the event id from the verification flow id");
140+
141+
// Simulate getting the event for a notification
142+
let _ = notification_client.get_notification_with_sliding_sync(room_id, &event_id).await;
143+
144+
let verification = bob
145+
.encryption()
146+
.get_verification_request(
147+
alice_verification_request.own_user_id(),
148+
alice_verification_request.flow_id(),
149+
)
150+
.await;
151+
152+
// If the ignore_verification_events parameter is true in NotificationClient,
153+
// no verification request should have been received
154+
assert!(verification.is_none());
155+
156+
Ok(())
157+
}
158+
37159
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
38160
async fn test_mutual_sas_verification() -> Result<()> {
39161
let encryption_settings =

0 commit comments

Comments
 (0)