fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields #4644

andybalaam · 2025-02-07T14:16:21Z

I'm sorry it's a big change. I've tried to break it into decent commits, and I did a couple of preparatory PRs to make it less painful, but it's still a bit to get your head around.

The basic idea is that when a session is updated and we call retry_event_decryption, we don't only look at UTDs any more - now we also look at decrypted events, and re-request their EncryptionInfo, in case it has improved.

codecov · 2025-02-07T14:32:38Z

Codecov Report

Attention: Patch coverage is 75.71429% with 17 lines in your changes missing coverage. Please review.

Project coverage is 85.81%. Comparing base (9de6d28) to head (a9331cf).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...es/matrix-sdk-common/src/deserialized_responses.rs	0.00%	6 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs	0.00%	4 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	92.85%	2 Missing ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	60.00%	2 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/traits.rs	0.00%	2 Missing ⚠️
...tes/matrix-sdk-ui/src/timeline/controller/state.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4644      +/-   ##
==========================================
- Coverage   85.82%   85.81%   -0.01%     
==========================================
  Files         292      292              
  Lines       33761    33822      +61     
==========================================
+ Hits        28974    29024      +50     
- Misses       4787     4798      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…eItem

…imelineItem ... meaning we attempt to redecrypt them even if they are not UTDs. The redecryption doesn't actually work - that will be fixed in the next commit.

…etry decryption In later commits, we will make this cleverer, so it can calculate which non-UTD events need updating too.

Future commits will add code to try non-UTDs too, in another function.

…-UTDs Fixes element-hq/element-meta#2697

andybalaam · 2025-02-14T12:16:15Z

I swapped the Rust team reviewer to @bnjbvr since I know he has questions about including session ID in the timeline item.

richvdh

seems generally good to me. A few comments.

crates/matrix-sdk-ui/src/timeline/traits.rs

crates/matrix-sdk-ui/src/timeline/controller/state_transaction.rs

crates/matrix-sdk-ui/src/timeline/event_handler.rs

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

crates/matrix-sdk-ui/src/timeline/event_item/remote.rs

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

richvdh · 2025-02-14T14:25:42Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

@@ -225,6 +228,45 @@ impl TimelineState {
        txn.commit();
    }

+    /// Try to fetch [`EncryptionInfo`] for the events with the supplied
+    /// indices, and update them where we succeed.
+    pub(super) async fn retry_event_encryption_info<P: RoomDataProvider>(


I have to say that having this here but retry_event_decryption_by_index in the other place feels rather asymmetric. Is there a reason they need to be different?

In my mind, TimelineState::retry_event_encryption_info is a partner to TimelineState::retry_event_decryption, not to TimelineController::retry_event_decryption_by_index. retry_event_decryption_by_index is needed because we need to construct a complex retry_one closure to pass to retry_event_decryption whereas retry_event_encryption_info can do its work directly.

bnjbvr · 2025-02-17T13:08:16Z

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

+        if let Some(event) = item.as_event() {
+            if let Some(remote_event) = event.as_remote() {
+                if let Some(session_id) = &remote_event.session_id {
+                    if should_retry(session_id) {


uh, that's a lot of indent levels 😅

You could use .and_then on all the Option to chain them, as well as early continue when you're seeing something you're not expecting:

let Some(thing) = item.as_event() .and_then(|event| event.as_remote()) .map(/* or and_then, not sure what session_id is */|remote_event| *remote_event.session_id) else { continue; };

After the changes I've already made, this is a little flatter, and I don't think there is a nice way to make it better?

On similar lines, I had a look at TimelineState::retry_event_encryption_info which has a similar problem, but I couldn't see a way to make that better because it has an async call all the way over to the right. Suggestions welcome.

andybalaam · 2025-02-17T15:40:14Z

This is ready for re-review. I think I would recommend looking at the full diff now, since the review changes reverted some of what I did in earlier commits. I will rebase into nice commits after review.

bnjbvr

A few stylistic change requests below. On the one hand, thanks for not storing any extra data in the TimelineItemContent itself, on the other… you know my opinion about adding more code to the timeline re-decryption. This all should be moved somewhere else, and tested in isolation there (could be in the crypto crate, the main SDK crate, inside or outside the event cache,… any place other than the timeline would make more sense).

bnjbvr · 2025-02-19T09:45:08Z

crates/matrix-sdk-ui/src/timeline/controller/state_transaction.rs

+    /// Replace the timeline item at the supplied index with the supplied item.
+    pub(super) fn replace(
+        &mut self,
+        timeline_item_index: usize,
+        timeline_item: Arc<TimelineItem>,
+    ) -> Arc<TimelineItem> {
+        self.items.replace(timeline_item_index, timeline_item)
+    }


Is this function useful? I would have assumed that there are many other places in the code that already replaces items from a transaction, and that they didn't need this function to do this… (Admittedly, the blackboxing of the TimelineStateTransaction is a bit compromised already at this point.)

bnjbvr · 2025-02-19T09:49:55Z

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

+            if let Some(event) = item.as_event() {
+                if let TimelineItemContent::UnableToDecrypt(encrypted) = event.content() {
+                    if let Some(session_id) = encrypted.session_id() {
+                        if should_retry(session_id) {
+                            return Some(Either::Left(idx));
+                        }
+                    }
+                } else if let Some(remote_event) = event.as_remote() {
+                    if let Some(encryption_info) = &remote_event.encryption_info {
+                        if should_retry(&encryption_info.session_id) {
+                            return Some(Either::Right(idx));
+                        }
+                    }
+                }
+            }
+            None


style nit: this could be much less indented if you shortcut the Nones:

Suggested change

if let Some(event) = item.as_event() {

if let TimelineItemContent::UnableToDecrypt(encrypted) = event.content() {

if let Some(session_id) = encrypted.session_id() {

if should_retry(session_id) {

return Some(Either::Left(idx));

}

}

} else if let Some(remote_event) = event.as_remote() {

if let Some(encryption_info) = &remote_event.encryption_info {

if should_retry(&encryption_info.session_id) {

return Some(Either::Right(idx));

}

}

}

}

None

let event = item.as_event()?;

if let TimelineItemContent::UnableToDecrypt(encrypted) = event.content() {

let session_id = encrypted.session_id()?;

if should_retry(session_id) {

return Some(Either::Left(idx));

}

} else {

let remote_event = event.as_remote()?;

let encryption_info = remote_event.encryption_info.as_ref()?;

if should_retry(&encryption_info.session_id) {

return Some(Either::Right(idx));

}

}

None

bnjbvr · 2025-02-19T09:50:38Z

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

        if retry_indices.is_empty() {
            return;
        }

        debug!("Retrying decryption");

+        let mut state = state.clone().write_owned().await;


I think it's better to have the caller pass the owned lock here; that shows that the lock will be taken, at the call sites.

bnjbvr · 2025-02-19T09:51:57Z

crates/matrix-sdk-common/src/deserialized_responses.rs

+    #[serde(default)]
+    pub session_id: String,


Should it be an Option instead of a string that defaults to "" when the data was reloaded from the store, and we didn't have it? Seems dangerous to have this floating knowledge that "" means "didn't have this information in the store"

bnjbvr · 2025-02-19T09:52:35Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+            if let Some(item) = item {
+                if let Some(event) = item.as_event() {
+                    let sender = event.sender.clone();
+                    if let Some(remote) = event.as_remote() {
+                        if let Some(encryption_info) = &remote.encryption_info {


too many indent levels. Please use either a closure that returns an option, or short-circuiting with let Some(thing) = ... else { continue; };

bnjbvr · 2025-02-19T09:53:06Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+        let mut txn = self.transaction();
+        for (idx, item) in new_info {
+            txn.replace(idx, item);
+        }
+        txn.commit();


You don't need a transaction if no operations between the start of the transaction and the end aren't fallible.

bnjbvr · 2025-02-19T09:54:44Z

crates/matrix-sdk-ui/src/timeline/controller/state.rs

+
+                            let new_item = item.with_kind(TimelineItemKind::Event(new_event));
+
+                            new_info.push((idx, new_item));


Can you replace the item here, instead of storing it temporarily? Or can't because iterating on the items themselves?

andybalaam · 2025-02-20T13:48:14Z

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

+
+        // Retry fetching encryption info for events that are already decrypted
+        let room_data_provider = self.room_data_provider.clone();
+        matrix_sdk::executor::spawn(async move {


From @bnjbvr

anyhow, one change that should happen now, and unnoted in this review because D4mir noticed it afterwards, was that this is spawning a task that's detached; it should be attached to the timeline, one way or another:

either as a one-off, making sure there's only one at any time
or a task that listens to a channel of commands, asking for retrying decryption of a set of tasks, and running those requests linearly

andybalaam changed the title ~~refactor(crypto): Store the session ID of a UTD in RemoteEventTimelineItem~~ fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields Feb 7, 2025

andybalaam force-pushed the andybalaam/redecrypt-non-utds branch from 9a19fcf to 9cbfbfb Compare February 7, 2025 14:17

andybalaam force-pushed the andybalaam/redecrypt-non-utds branch 3 times, most recently from 741aa40 to d83e187 Compare February 14, 2025 11:45

andybalaam added 7 commits February 14, 2025 11:59

tool(ui): Allow getting encryption info via a Room instance

aa64150

tool(ui): Provide a replace method for TimelineStateTransaction

8d0fd62

refactor(crypto): Store the session ID of a UTD in RemoteEventTimelin…

45ddbf1

…eItem

refactor(crypto): Store session ID in EncryptionInfo and provide to T…

5cc10ba

…imelineItem ... meaning we attempt to redecrypt them even if they are not UTDs. The redecryption doesn't actually work - that will be fixed in the next commit.

refactor(timeline): Extract a function to calculate which events to r…

dca6881

…etry decryption In later commits, we will make this cleverer, so it can calculate which non-UTD events need updating too.

refactor(timeline): Extract a function for retrying decryption

ff1dde9

Future commits will add code to try non-UTDs too, in another function.

fix(crypto): When a session updates, re-fetch encryption info for non…

414f03d

…-UTDs Fixes element-hq/element-meta#2697

andybalaam force-pushed the andybalaam/redecrypt-non-utds branch from d83e187 to 414f03d Compare February 14, 2025 11:59

andybalaam marked this pull request as ready for review February 14, 2025 12:11

andybalaam requested review from a team as code owners February 14, 2025 12:11

andybalaam requested review from stefanceriu, richvdh and bnjbvr and removed request for a team and stefanceriu February 14, 2025 12:11

richvdh reviewed Feb 14, 2025

View reviewed changes

andybalaam marked this pull request as draft February 17, 2025 12:26

bnjbvr removed their request for review February 17, 2025 13:05

Avoid storing session_id in TimelineItem

d6d812b

bnjbvr reviewed Feb 17, 2025

View reviewed changes

Update doc comments

c700762

andybalaam added 3 commits February 17, 2025 13:27

Collapse an if else into a single block

0d2b71d

Move the clone of state into retry_event_decryption_by_index

076f076

Use a partition_map instead of building temporary Vecs

a9331cf

andybalaam requested review from bnjbvr and richvdh February 17, 2025 15:16

andybalaam marked this pull request as ready for review February 17, 2025 15:16

bnjbvr requested changes Feb 19, 2025

View reviewed changes

andybalaam commented Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields #4644

fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields #4644

andybalaam commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

andybalaam commented Feb 14, 2025

richvdh left a comment

richvdh Feb 14, 2025

andybalaam Feb 17, 2025

bnjbvr Feb 17, 2025

andybalaam Feb 17, 2025

andybalaam commented Feb 17, 2025

bnjbvr left a comment

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

bnjbvr Feb 19, 2025

andybalaam Feb 20, 2025 •

edited

Loading


		let new_item = item.with_kind(TimelineItemKind::Event(new_event));

		new_info.push((idx, new_item));

fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields #4644

Are you sure you want to change the base?

fix(crypto): Redecrypt non-UTD messages to remove no-longer-relevant warning shields #4644

Conversation

andybalaam commented Feb 7, 2025 • edited Loading

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

andybalaam commented Feb 14, 2025

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybalaam commented Feb 17, 2025

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybalaam Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

andybalaam commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

andybalaam Feb 20, 2025 •

edited

Loading