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

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

Fixes element-hq/element-meta#2697
Fixes https://github.com/element-hq/crypto-internal/issues/398

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 80.59701% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.36%. Comparing base (3fbf159) to head (3b1e18e).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...es/matrix-sdk-common/src/deserialized_responses.rs	0.00%	6 Missing ⚠️
...i/src/timeline/controller/decryption_retry_task.rs	89.13%	5 Missing ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4644      +/-   ##
==========================================
+ Coverage   86.34%   86.36%   +0.01%     
==========================================
  Files         291      291              
  Lines       34233    34283      +50     
==========================================
+ Hits        29559    29607      +48     
- Misses       4674     4676       +2

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

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

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

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).

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

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

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

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

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

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

…tion

…ryption

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

…ecure_device_if_it_is_secure

It's unusual to have the method on the parent type when the field type could also hold the method. In fact, this was the only bool getter inspecting the timeline's content, so let's move the method next to as its siblings, for consistency, and let's spell it out fully for clarity.

bnjbvr

approving, i'll address my own comments as a separate commit and will squash-merge it altogether.

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

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

bnjbvr · 2025-03-11T06:37:13Z

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

+/// Create a replacement TimelineItem for the supplied one, with new
+/// [`EncryptionInfo`] from the supplied `room_data_provider`. Returns None if
+/// the supplied item is not a remote event, or if it doesn't have a session ID.
+async fn replacement_for<P: RoomDataProvider>(


In this case, let's name it make_replacement_for.

…non-utds

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 4 times, most recently 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

bnjbvr reviewed Feb 17, 2025

View reviewed changes

crates/matrix-sdk-ui/src/timeline/controller/mod.rs Outdated Show resolved Hide resolved

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

crates/matrix-sdk-ui/src/timeline/controller/mod.rs Outdated Show resolved Hide resolved

andybalaam marked this pull request as draft February 25, 2025 15:15

andybalaam mentioned this pull request Feb 25, 2025

EX: Messages sent after an identity reset can sometimes be flagged as sent from insecure device element-hq/element-meta#2697

Closed

richvdh removed their request for review February 26, 2025 08:45

andybalaam and others added 19 commits March 7, 2025 10:20

Return reference from session_id

5eb3944

Replace .into() with more explicit ::from()

449f234

Use subscribe_events to ignore date divider

cef82ad

Separate sanity checks into blocks

e4e6e83

Add a newline

2735180

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

Mock get_encryption_info by storing a hashmap

da8a993

Remove unneeded pub(crate)

7acac9b

Remove unneeded Just

52846f7

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

Replace if with filter

8de03ad

Merge replacement_item into replacement_for

f501afe

Prefix tests with test_

585ceba

Rename retry_fetch_encryption_info

6212cf3

Expand comment about why we are calling event_indices_to_retry_decryp…

c3066c9

…tion

Specify that we are returning indices from event_indices_to_retry_dec…

e52add1

…ryption

Collapse two and_thens

8e0af15

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Signed-off-by: Andy Balaam <mail@artificialworlds.net>

Replace items as we create them in retry_fetch_encryption_info

650fe4b

Replace summarize_* methods with direct assertions

5b3cc43

Provide timeline_messages function to simplify assertions

4ea1127

Move helpers inside test_new_users_first_messages_dont_warn_about_ins…

6570556

…ecure_device_if_it_is_secure

andybalaam requested a review from bnjbvr March 7, 2025 12:53

andybalaam added 2 commits March 7, 2025 13:13

feat(timeline): Provide is_utd on EventTimelineItem

dd55342

Make use of new is_utd method on EventTimelineItem

34013b3

andybalaam mentioned this pull request Mar 7, 2025

feat(timeline): Provide is_utd on EventTimelineItem #4769

Merged

bnjbvr approved these changes Mar 11, 2025

View reviewed changes

bnjbvr added 2 commits March 11, 2025 07:38

Merge branch 'bnjbvr/is-unable-to-decrypt' into andybalaam/redecrypt-…

513d1e8

…non-utds

review

3b1e18e

bnjbvr enabled auto-merge (squash) March 11, 2025 06:47

bnjbvr merged commit c8536e9 into main Mar 11, 2025
42 checks passed

bnjbvr deleted the andybalaam/redecrypt-non-utds branch March 11, 2025 07:01

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

andybalaam commented Feb 17, 2025

bnjbvr left a comment

bnjbvr left a comment

bnjbvr Mar 11, 2025

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

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

andybalaam commented Feb 17, 2025

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr Mar 11, 2025

Choose a reason for hiding this comment

andybalaam commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading