Use the `DecryptedOlmV1Event` when encrypting to-device events #4690

poljar · 2025-02-19T13:20:46Z

This requires a bit of a hack, but I think the stronger typing and having the fields defined in a single place make this worth it.

codecov · 2025-02-19T13:35:38Z

Codecov Report

Attention: Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.84%. Comparing base (9de6d28) to head (ffbf31c).
Report is 47 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/olm/session.rs	80.00%	3 Missing ⚠️
...rates/matrix-sdk-crypto/src/types/events/olm_v1.rs	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4690      +/-   ##
==========================================
+ Coverage   85.82%   85.84%   +0.01%     
==========================================
  Files         292      292              
  Lines       33761    33779      +18     
==========================================
+ Hits        28974    28996      +22     
+ Misses       4787     4783       -4

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

…-device events This ensures that new fields that are added to the `m.olm.v1.curve25519-aes-sha` content need to be added in a single place.

Hywan

Looks good to me, thanks!

Hywan · 2025-02-19T14:55:21Z

crates/matrix-sdk-crypto/src/olm/session.rs

+            // So we just leave EVENT_TYPE out. This works because the serialization is
+            // using `event_type()` and this type is contained to this function.
+            const EVENT_TYPE: &'static str = "";


So EVENT_TYPE is useless and never used in this context, is that alright? If yes, maybe it should be said explicitly in the doc?

It's used in all the other types that implement EventType. Only this one implementation is a special hack.

I assume Ivan means we should add this to the documentation of this implementation, to which, +1.

Suggested change

// So we just leave EVENT_TYPE out. This works because the serialization is

// using `event_type()` and this type is contained to this function.

const EVENT_TYPE: &'static str = "";

// We have to provide `EVENT_TYPE` to conform to the `EventType` trait, but

// don't actually use it, so we just leave it empty.

//

// This works because the serialization uses `event_type()` and this type is contained

// to this function.

const EVENT_TYPE: &'static str = "";

Yes, this. Thanks for clarifying my words.

richvdh

LGTM in general, a few nits

richvdh · 2025-02-20T09:42:32Z

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

    /// **Note**: This should never be implemented manually, this takes the
    /// event type from the constant.


This is no longer true? Or at least, the word "never" should be softened to "not usually" or something.

You could fix the comma splice while you're there.

richvdh · 2025-02-20T09:44:47Z

crates/matrix-sdk-crypto/src/identities/device.rs

-        let (event_type, content) = {
+        let content = {
            let export = if let Some(index) = message_index {
                session.export_at_index(index).await
            } else {
                session.export().await
            };
            let content: ForwardedRoomKeyContent = export.try_into()?;

-            (content.event_type(), content)
+            content
        };


Suggested change

let (event_type, content) = {

let content = {

let export = if let Some(index) = message_index {

session.export_at_index(index).await

} else {

session.export().await

};

let content: ForwardedRoomKeyContent = export.try_into()?;

(content.event_type(), content)

content

};

let content: ForwardedRoomKeyContent = {

let export = if let Some(index) = message_index {

session.export_at_index(index).await

} else {

session.export().await

};

export.try_into()?

};

richvdh · 2025-02-20T09:45:42Z

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

+        let event_type = content.event_type().to_owned();
+        let (used_session, content) = device.encrypt(&event_type, content).await?;
+
+        let event_type = content.event_type().to_owned();


this line looks redundant?

At first glance one would think so. But the first event_type is for content while the second event_type is for content after the device.encrypt() step.

Perhaps renaming the second content to encrypted_content might make this clearer.

oh right, yes. Some different names might help.

richvdh · 2025-02-20T09:46:32Z

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

@@ -162,7 +162,10 @@ impl AnyDecryptedOlmEvent {
 }

 /// An `m.olm.v1.curve25519-aes-sha2` decrypted to-device event.
-#[derive(Clone, Debug, Deserialize, Serialize)]
+///
+/// **Note**: This event will reserialize events lossy, unknown fields will be


Suggested change

/// **Note**: This event will reserialize events lossy, unknown fields will be

/// **Note**: This event will reserialize events lossily; unknown fields will be

richvdh · 2025-02-20T09:47:50Z

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

@@ -182,6 +185,52 @@ where
    pub content: C,
 }

+impl<C: EventType + Debug + Sized + Serialize> Serialize for DecryptedOlmV1Event<C> {


A comment as to why this is needed/what it does differently from derive(Serialize) might be helpful.

richvdh · 2025-02-20T09:52:16Z

crates/matrix-sdk-crypto/src/olm/session.rs

+            // This is a bit of a hack, usually we just define the `EVENT_TYPE` and use the
+            // default implementation of `event_type()`, we can't do this here
+            // because the event type isn't static.


multi-way comma splice!

Suggested change

// This is a bit of a hack, usually we just define the `EVENT_TYPE` and use the

// default implementation of `event_type()`, we can't do this here

// because the event type isn't static.

// This is a bit of a hack: usually we just define the `EVENT_TYPE` and use the

// default implementation of `event_type()`. We can't do this here

// because the event type isn't static.

richvdh · 2025-02-20T09:57:15Z

crates/matrix-sdk-crypto/src/olm/session.rs

+            // So we just leave EVENT_TYPE out. This works because the serialization is
+            // using `event_type()` and this type is contained to this function.
+            const EVENT_TYPE: &'static str = "";


I assume Ivan means we should add this to the documentation of this implementation, to which, +1.

Suggested change

// So we just leave EVENT_TYPE out. This works because the serialization is

// using `event_type()` and this type is contained to this function.

const EVENT_TYPE: &'static str = "";

// We have to provide `EVENT_TYPE` to conform to the `EventType` trait, but

// don't actually use it, so we just leave it empty.

//

// This works because the serialization uses `event_type()` and this type is contained

// to this function.

const EVENT_TYPE: &'static str = "";

poljar requested review from a team as code owners February 19, 2025 13:20

poljar requested review from Hywan and richvdh and removed request for a team February 19, 2025 13:20

poljar force-pushed the poljar/stronger-typing-olm-encryption branch from bfe7f6a to 4195afa Compare February 19, 2025 13:45

poljar added 3 commits February 19, 2025 15:34

refactor(crypto): Don't require event_type to return a static string

1ffb241

fix(crypto): Fix the serialization of DecryptedOlmV1Event

0d69135

refactor(crypto): Use the DecryptedOlmV1Event type when encrypting to…

ffbf31c

…-device events This ensures that new fields that are added to the `m.olm.v1.curve25519-aes-sha` content need to be added in a single place.

poljar force-pushed the poljar/stronger-typing-olm-encryption branch from 4195afa to ffbf31c Compare February 19, 2025 14:34

Hywan approved these changes Feb 19, 2025

View reviewed changes

richvdh approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the `DecryptedOlmV1Event` when encrypting to-device events #4690

Use the `DecryptedOlmV1Event` when encrypting to-device events #4690

poljar commented Feb 19, 2025

codecov bot commented Feb 19, 2025 •

edited

Loading

Hywan left a comment

Hywan Feb 19, 2025

poljar Feb 19, 2025

richvdh Feb 20, 2025

Hywan Feb 20, 2025

richvdh left a comment

richvdh Feb 20, 2025

richvdh Feb 20, 2025 •

edited

Loading

richvdh Feb 20, 2025

poljar Feb 20, 2025

richvdh Feb 20, 2025

richvdh Feb 20, 2025

richvdh Feb 20, 2025

richvdh Feb 20, 2025

richvdh Feb 20, 2025

		/// Note: This should never be implemented manually, this takes the
		/// event type from the constant.

	/// Note: This event will reserialize events lossy, unknown fields will be
	/// Note: This event will reserialize events lossily; unknown fields will be

Use the DecryptedOlmV1Event when encrypting to-device events #4690

Are you sure you want to change the base?

Use the DecryptedOlmV1Event when encrypting to-device events #4690

Conversation

poljar commented Feb 19, 2025

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

Hywan 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

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Feb 20, 2025 • edited Loading

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

Use the `DecryptedOlmV1Event` when encrypting to-device events #4690

Use the `DecryptedOlmV1Event` when encrypting to-device events #4690

codecov bot commented Feb 19, 2025 •

edited

Loading

richvdh Feb 20, 2025 •

edited

Loading