crypto: expose new method `OlmMachine::try_decrypt_room_event` #4116

richvdh · 2024-10-10T16:52:15Z

... which returns an enum which encodes decryption failure information, rather than returning MegolmError

To make this work, we also extend the existing UnableToDecryptInfo to include a reason code for the failure.

Part of #4048

codecov · 2024-10-10T17:24:00Z

Codecov Report

Attention: Patch coverage is 62.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.57%. Comparing base (a4bda1a) to head (f003f6b).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/machine/mod.rs	60.86%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4116      +/-   ##
==========================================
- Coverage   84.69%   84.57%   -0.13%     
==========================================
  Files         269      269              
  Lines       28816    28825       +9     
==========================================
- Hits        24405    24378      -27     
- Misses       4411     4447      +36

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

andybalaam

Looks good to me.

andybalaam · 2024-10-16T08:31:49Z

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

+    /// that session for some reason (e.g. incorrect MAC).
+    ///
+    /// This represents all `vodozemac::megolm::DecryptionError`s, with the
+    /// exception of `UnknownMessageIndex`, which is represented as


I found this difficult to parse because of the special meaning of "exception" in my head. Would "with the exception of" -> "except" be better?

bnjbvr

LGTM, thanks for breaking it into simple changes :)

bnjbvr · 2024-10-16T08:49:07Z

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

+fn unknown_utd_reason() -> UnableToDecryptReason {
+    UnableToDecryptReason::Unknown
+}


For what it's worth, you can instead derive(Default) to UnableToDecryptReason, and then mark one variant as the default:

#[default] #[doc(hidden)] Unknown,

Yeah, I wanted to explicitly avoid implementing Default for UnableToDecryptReason, because then people might start using it outside the deserialization case.

bnjbvr · 2024-10-16T08:52:01Z

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

+    MegolmDecryptionFailure,
+
+    /// The event could not be deserialized after decryption.
+    PayloadDeserializationFailure,


double nit: since those are errors, i'd say "failure" in the variant's name is redundant, and would lightly encourage to remove it.

I don't really agree. If I compare with something like MissingMegolmSession: a missing megolm session is a sensible reason for being unable to decrypt an event. So is a "megolm decryption failure". But just MegolmDecryption on its own doesn't really make sense to me.

I'm prepared to believe there could be better names for these things (UnableToDecryptMegolmMessage? VodozemacDecryptionError?), but I don't think MegolmDecryption would be a good one.

bnjbvr · 2024-10-16T08:54:06Z

crates/matrix-sdk-crypto/src/machine/mod.rs

@@ -2540,6 +2536,38 @@ pub struct EncryptionSyncChanges<'a> {
    pub next_batch_token: Option<String>,
 }

+fn megolm_error_to_utd_info(


Can you write a tiny doc comment for this, please? (It wasn't clear at a first glance at the call-site why this could return an error...)

bnjbvr · 2024-10-16T08:55:00Z

crates/matrix-sdk-crypto/src/machine/mod.rs

-                    Some(UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo {
-                        session_id,
-                    }))
+                Err(e) => {


nit: can you name this variable err, if not error, please? One-letter variables are bleh.

bnjbvr · 2024-10-16T08:56:17Z

crates/matrix-sdk-crypto/src/machine/mod.rs

+    ) -> Result<RoomEventDecryptionResult, CryptoStoreError> {
+        match self.decrypt_room_event_inner(raw_event, room_id, true, decryption_settings).await {
+            Ok(decrypted) => Ok(RoomEventDecryptionResult::Decrypted(decrypted)),
+            Err(e) => Ok(RoomEventDecryptionResult::UnableToDecrypt(megolm_error_to_utd_info(


nit: e => err too here please

bnjbvr · 2024-10-16T08:57:23Z

crates/matrix-sdk-crypto/src/machine/tests/mod.rs

+    let decrypt_result =
+        bob.try_decrypt_room_event(&room_event, room_id, &decryption_settings).await.unwrap();
+    assert_let!(RoomEventDecryptionResult::UnableToDecrypt(utd_info) = decrypt_result);
+    assert!(utd_info.session_id.is_some());


could we assert the reason too?

Add a field to store the reason that the decryption failed

richvdh force-pushed the rav/try_decrypt_room_event branch from 7420e03 to 33f5dd9 Compare October 11, 2024 11:43

richvdh mentioned this pull request Oct 15, 2024

Support for UTD error codes in [Sync]TimelineEvent #4070

Merged

timeline: tests for deserializing SyncTimelineEvent with unsigned events

bb1d678

richvdh force-pushed the rav/try_decrypt_room_event branch from 33f5dd9 to 61f3cec Compare October 15, 2024 13:23

richvdh marked this pull request as ready for review October 15, 2024 16:57

richvdh requested review from a team as code owners October 15, 2024 16:57

richvdh requested review from bnjbvr and andybalaam and removed request for a team October 15, 2024 16:57

andybalaam approved these changes Oct 16, 2024

View reviewed changes

bnjbvr approved these changes Oct 16, 2024

View reviewed changes

richvdh added 3 commits October 16, 2024 16:29

crypto: add UnableToDecryptReason to UnableToDecryptInfo

bd7eff0

Add a field to store the reason that the decryption failed

crypto: new method OlmMachine::try_decrypt_room_event

fa89102

crypto: update changelog

f003f6b

richvdh force-pushed the rav/try_decrypt_room_event branch from 390f187 to f003f6b Compare October 16, 2024 15:30

richvdh enabled auto-merge (rebase) October 16, 2024 15:30

richvdh merged commit 87f89ec into main Oct 16, 2024
39 checks passed

richvdh deleted the rav/try_decrypt_room_event branch October 16, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: expose new method `OlmMachine::try_decrypt_room_event` #4116

crypto: expose new method `OlmMachine::try_decrypt_room_event` #4116

richvdh commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

andybalaam left a comment

andybalaam Oct 16, 2024

bnjbvr left a comment

bnjbvr Oct 16, 2024

richvdh Oct 16, 2024

bnjbvr Oct 16, 2024

richvdh Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

bnjbvr Oct 16, 2024

crypto: expose new method OlmMachine::try_decrypt_room_event #4116

crypto: expose new method OlmMachine::try_decrypt_room_event #4116

Conversation

richvdh commented Oct 10, 2024 • edited Loading

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

andybalaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

crypto: expose new method `OlmMachine::try_decrypt_room_event` #4116

crypto: expose new method `OlmMachine::try_decrypt_room_event` #4116

richvdh commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading