Timeline: Extend UtdCause with new reason codes #4126

richvdh · 2024-10-15T13:05:55Z

This PR is the final part of the epic of making unable-to-decrypt information available in the timeline. As of #4070, the UTD info is now available in the [Sync]TimelineEvent created by the network code: now, we just need to copy that information into the items that are added to the timeline itself.

To do this, we add a new variant to matrix_sdk_ui::event_handler::TimelineEventKind (not to be confused with matrix_sdk_common::deserialized_responses::TimelineEventKind which was recently added in #4082) to hold the relevant data, and then add a bunch of new reason codes to the UtdCause which is derived from the decryption failure.

Part of #4048.
~~Based on #4070.~~

codecov · 2024-10-15T13:37:22Z

Codecov Report

Attention: Patch coverage is 80.95238% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.83%. Comparing base (3887c10) to head (f578f26).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	66.66%	3 Missing ⚠️
...tes/matrix-sdk-ui/src/timeline/controller/state.rs	70.00%	3 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4126   +/-   ##
=======================================
  Coverage   84.83%   84.83%           
=======================================
  Files         269      269           
  Lines       28826    28842   +16     
=======================================
+ Hits        24455    24469   +14     
- Misses       4371     4373    +2

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

Give `matrix-sdk-ui::event_handler::TimelineEventKind` a new variant which specifically represents events that could not be decrypted.

Stash the reason for the decryption failure in `matrix-sdk-ui::event_handler::TimelineEventKind::UnableToDecrypt`. It's not yet used.

Rather than calling `UtdCause::determine` again when an event is successfully decrypted on retry, re-use the cause we already determined.

Before we do any more work here, give this variant a better name Breaking-Change: `matrix_sdk_crypto::type::events::UtdCause::Membership` has been renamed to `...::SentBeforeWeJoined`.

We'll need this for future changes

andybalaam

One suggestion, but good to go, thanks!

andybalaam · 2024-10-22T08:36:27Z

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs

@@ -180,6 +212,24 @@ mod tests {
        );
    }

+    #[test]
+    fn if_reason_is_not_missing_key_we_guess_unknown_even_if_membership_is_leave_we_guess_membership(


We have a convention (which I personally don't like, but do comply with) of test names starting with test_

For me, consistency within a file is more important than the convention for new tests

Then please prefix all the tests names with test_. This makes it easier to distinguish test functions from non-test functions in LSP searches.

done in 983c436

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs

bnjbvr · 2024-10-22T13:04:11Z

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

+                Some(AnyMessageLikeEventContent::RoomEncrypted(content)) => {
+                    // An event which is still encrypted.
+                    if let Some(unable_to_decrypt_info) = unable_to_decrypt_info {
+                        Self::UnableToDecrypt { content, unable_to_decrypt_info }
+                    } else {
+                        // If we get here, it means that some part of the code has created a
+                        // `SyncTimelineEvent` containing an `m.room.encrypted` event
+                        // without decrypting it. Possibly this means that encryption has not been
+                        // configured.
+                        // We treat it the same as any other message-like event.
+                        Self::Message {
+                            content: AnyMessageLikeEventContent::RoomEncrypted(content),
+                            relations: ev.relations(),
+                        }
+                    }
+                }


Why do we handle those two cases separately? The comment doesn't really explain it to me. I'm not quite sure about why we need a new TimelineEventKind variant just for this…

I'm not quite sure about why we need a new TimelineEventKind variant just for this…

Well, the objective here is to convey the unable_to_decrypt_info that is in the SyncTimelineEvent or TimelineEvent to TimelineEventHandler::handle_event. It seems wrong to just shove another field onto TimelineEventKind::Message, hence adding a new variant.

Why do we handle those two cases separately? The comment doesn't really explain it to me

If the lower layers have done their jobs correctly, then encrypted events will appear either as:

a [Sync]TimelineEvent with kind: Decrypted, in which case ev.original_content() here will be the decrypted event, and we hit the `Some(content) case on line 214

a [Sync]TimelineEvent with kind: UnableToDecrypt, in which case ev.original_content() here will be the encrypted event and unable_to_decrypt_info will be populated, and we hit the case at line 200.

However, the object model still allows for a [Sync]TimelineEvent with kind: PlainText whose content is actually a AnyMessageLikeEventContent::RoomEncrypted, and the second case (lines 203-211) is to deal with that situation. One way that I think it will happen is if the OlmMachine is not correctly set up for some reason (BaseClient::olm_machine is an Option which has to be loaded somewhere.) And that's what the comment is trying to say.

Hmm ok, thanks for explaining.

richvdh changed the title ~~Add unable_to_decrypt_info to EncryptedEvent~~ WIP: Add unable_to_decrypt_info to EncryptedEvent Oct 15, 2024

richvdh force-pushed the rav/utds_in_timeline_item_content branch from 563d51b to 4282f40 Compare October 15, 2024 13:22

richvdh force-pushed the rav/utds_in_timeline_item_content branch 9 times, most recently from 4118003 to 61b7d76 Compare October 17, 2024 21:34

richvdh changed the title ~~WIP: Add unable_to_decrypt_info to EncryptedEvent~~ Timeline: Extend UtdCause with new reason codes Oct 17, 2024

richvdh force-pushed the rav/utds_in_timeline_item_content branch from 4bd94a2 to efc6909 Compare October 21, 2024 11:53

richvdh added 5 commits October 21, 2024 18:01

refactor(ui): add TimelineEventKind::UnableToDecrypt

259784c

Give `matrix-sdk-ui::event_handler::TimelineEventKind` a new variant which specifically represents events that could not be decrypted.

refactor(ui): add UTD info to TimelineEventKind::UnableToDecrypt

6703978

Stash the reason for the decryption failure in `matrix-sdk-ui::event_handler::TimelineEventKind::UnableToDecrypt`. It's not yet used.

refactor(timeline): retry_event_decryption: re-use utd cause

d568627

Rather than calling `UtdCause::determine` again when an event is successfully decrypted on retry, re-use the cause we already determined.

feat(crypto): rename UtdCause::Membership

f170e07

Before we do any more work here, give this variant a better name Breaking-Change: `matrix_sdk_crypto::type::events::UtdCause::Membership` has been renamed to `...::SentBeforeWeJoined`.

refactor(crypto): pass utd info into UtdCause::determine

791a7a2

We'll need this for future changes

richvdh force-pushed the rav/utds_in_timeline_item_content branch from efc6909 to 3c2eda2 Compare October 21, 2024 17:03

richvdh marked this pull request as ready for review October 21, 2024 17:38

richvdh requested review from a team as code owners October 21, 2024 17:38

richvdh requested review from andybalaam and removed request for a team October 21, 2024 17:38

andybalaam approved these changes Oct 22, 2024

View reviewed changes

richvdh commented Oct 22, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs Outdated Show resolved Hide resolved

richvdh commented Oct 22, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs Outdated Show resolved Hide resolved

bnjbvr reviewed Oct 22, 2024

View reviewed changes

BillCarsonFr mentioned this pull request Oct 22, 2024

Feat: New expected UTD mode due to decryption TrustRequirement matrix-org/matrix-analytics-events#114

Merged

richvdh added 2 commits October 23, 2024 09:11

feat(crypto): Add more reason codes to UtdCause

bd7005e

test(crypto): rename UtdCause tests with test_ prefix

f578f26

richvdh force-pushed the rav/utds_in_timeline_item_content branch from 983c436 to f578f26 Compare October 23, 2024 08:11

richvdh merged commit 3291a42 into main Oct 23, 2024
40 checks passed

richvdh deleted the rav/utds_in_timeline_item_content branch October 23, 2024 08:44

This was referenced Oct 23, 2024

Trust & Decoration | Support new expected UTD causes element-hq/element-x-android#3729

Merged

New posthog codes for new decryption failure reasons element-hq/element-meta#2541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeline: Extend UtdCause with new reason codes #4126

Timeline: Extend UtdCause with new reason codes #4126

richvdh commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

andybalaam left a comment

andybalaam Oct 22, 2024

richvdh Oct 22, 2024

bnjbvr Oct 22, 2024

richvdh Oct 22, 2024

bnjbvr Oct 22, 2024

richvdh Oct 22, 2024

bnjbvr Oct 22, 2024

Timeline: Extend UtdCause with new reason codes #4126

Timeline: Extend UtdCause with new reason codes #4126

Conversation

richvdh commented Oct 15, 2024 • edited Loading

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

andybalaam left a comment

Choose a reason for hiding this comment

andybalaam Oct 22, 2024

Choose a reason for hiding this comment

richvdh Oct 22, 2024

Choose a reason for hiding this comment

bnjbvr Oct 22, 2024

Choose a reason for hiding this comment

richvdh Oct 22, 2024

Choose a reason for hiding this comment

bnjbvr Oct 22, 2024

Choose a reason for hiding this comment

richvdh Oct 22, 2024

Choose a reason for hiding this comment

bnjbvr Oct 22, 2024

Choose a reason for hiding this comment

richvdh commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading