Add dehydrated flag to device_keys of dehydrated devices #3164

uhoreg · 2024-02-23T21:17:39Z

No description provided.

poljar

I know this is still a draft, but since the PR is small I took a look. Left some comments.

crates/matrix-sdk-crypto/src/dehydrated_devices.rs

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

codecov-commenter · 2024-02-28T22:06:50Z

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.65%. Comparing base (876d323) to head (cfedc24).
Report is 27 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-crypto/src/identities/device.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3164      +/-   ##
==========================================
- Coverage   83.67%   83.65%   -0.02%     
==========================================
  Files         236      236              
  Lines       24398    24415      +17     
==========================================
+ Hits        20414    20425      +11     
- Misses       3984     3990       +6

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

uhoreg · 2024-02-29T19:22:08Z

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

@@ -352,6 +358,9 @@ pub struct PickledAccount {
    pub pickle: AccountPickle,
    /// Was the account shared.
    pub shared: bool,
+    /// Whether this is for a dehydrated device
+    #[serde(default)]
+    pub dehydrated: bool,


adding this flag here will break things that create PickledAccount, such as https://github.com/matrix-org/matrix-rust-sdk-crypto-wasm/blob/main/src/libolm_migration.rs#L136 I'm not sure if there's a good way to deal with this.

They'll need to update the bindings just like you did with the matrix-sdk-crypto-ffi bindings.

OK, I just wasn't sure if there was a better way to do it. I assume this means that the version will need to be bumped since this is a breaking change. Is there a way to flag this so that when the next release gets made, the version will be bumped correctly?

We're still in the 0.X.Y range of releases, so the correct version will get bumped by design. Adding this as a breaking change to the changelog might make sense to notify people about it.

poljar

Left some small nits, looks generally good.

poljar · 2024-03-12T13:48:18Z

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

@@ -352,6 +358,9 @@ pub struct PickledAccount {
    pub pickle: AccountPickle,
    /// Was the account shared.
    pub shared: bool,
+    /// Whether this is for a dehydrated device
+    #[serde(default)]
+    pub dehydrated: bool,


They'll need to update the bindings just like you did with the matrix-sdk-crypto-ffi bindings.

crates/matrix-sdk-crypto/src/dehydrated_devices.rs

poljar · 2024-03-12T13:51:30Z

crates/matrix-sdk-crypto/src/dehydrated_devices.rs

@@ -477,6 +477,10 @@ mod tests {
            !request.fallback_keys.is_empty(),
            "The dehydrated device creation request should contain some fallback keys"
        );
+
+        let device_keys: crate::types::DeviceKeys =


Doesn't request.device_keys.deserialize_as() work here? Also please import DeviceKeys if you need them.

changed to use deserialize_as (I didn't know about the function). mod tests already imports ruma::encryption::DeviceKeys, so I can't import crate::types::DeviceKeys at the module-level. Do I just use crate::types::DeviceKeys at the top of this function? It looks like that's possible, but I've never seen that done, so I don't know if that's a "normal" thing to do.

It's possible, you can also rename things if you import things using use Foo as Bar.

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

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

Co-authored-by: Damir Jelić <poljar@termina.org.uk> Signed-off-by: Hubert Chathi <hubert@uhoreg.ca>

poljar

Looks good. Could you get rid of the merge conflict so CI kicks in. After that we can merge.

uhoreg · 2024-03-16T19:03:34Z

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

+    pub fn new_dehydrated(user_id: &UserId) -> Self {
+        let account = InnerAccount::new();
+        let device_id: OwnedDeviceId =
+            base64_encode(account.identity_keys().curve25519.as_bytes()).into();


I meant to ask earlier, but on the MSC, I suggested using URL-safe unpadded Base64 instead of plain Base64, since the device ID gets put in path parameters. If you don't have any objections to that, I may as well change this code to make this device ID URL-safe before we merge.

We were pondering this when we did the iOS implementation and decided against it. The reason being that it would be confusing to have two different representations for a Curve25510 key. One being in the path and one being uploaded as the device keys. Both encodings would end up in the logs as well and confuse potential bug hunters.

poljar · 2024-03-18T08:45:42Z

crates/matrix-sdk-crypto/CHANGELOG.md

 Additions:

 - When Olm message decryption fails, report the error code(s) from the failure.
  ([#3212](https://github.com/matrix-org/matrix-rust-sdk/pull/3212))

+>>>>>>> main


I don't think that this belongs here 😅

poljar

Ah, I just noticed one final problem with this. Looks good otherwise.

poljar · 2024-03-18T12:55:23Z

crates/matrix-sdk-crypto/src/types/device_keys.rs

+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub dehydrated: Option<bool>,


Oh wait, this will create an edge case in the signature verification, because we fold the null case for the dehydrated property as well as a missing dehydrated property into the None case.

You'll have to use this instead: https://docs.rs/js_option/0.1.1/js_option/enum.JsOption.html

Sorry for missing this.

poljar

Looks good, just ensure to make CI happy with the formatting.

uhoreg · 2024-03-18T15:59:22Z

Code coverage failed due to hitting a limit with codecov.io. But the previous commit was OK, and the last commit was just a formatting change, so should be OK.

I don't have permissions in this repo, so someone will need to hit the merge button for me.

add dehydrated flag to device_keys of dehydrated devices

df59389

uhoreg mentioned this pull request Feb 23, 2024

Add bindings for dehydrated devices matrix-org/matrix-rust-sdk-crypto-wasm#104

Merged

poljar requested changes Feb 26, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/dehydrated_devices.rs Outdated Show resolved Hide resolved

crates/matrix-sdk-crypto/src/olm/account.rs Outdated Show resolved Hide resolved

uhoreg added 2 commits February 28, 2024 16:34

mark account as dehydrated when creating

911beb7

fix serialization

e93b16c

uhoreg marked this pull request as ready for review February 28, 2024 21:49

uhoreg requested a review from a team as a code owner February 28, 2024 21:49

uhoreg requested review from Hywan and removed request for a team February 28, 2024 21:49

add missing flag in ffi

e065017

Hywan removed their request for review February 29, 2024 10:54

uhoreg commented Feb 29, 2024

View reviewed changes

uhoreg requested a review from poljar March 6, 2024 23:34

poljar requested changes Mar 12, 2024

View reviewed changes

uhoreg and others added 3 commits March 13, 2024 14:23

Apply suggestions from code review

dd81a50

Co-authored-by: Damir Jelić <poljar@termina.org.uk> Signed-off-by: Hubert Chathi <hubert@uhoreg.ca>

apply changes from code review

eca4aa4

add changelog

8019bbf

uhoreg requested a review from poljar March 13, 2024 22:24

uhoreg mentioned this pull request Mar 14, 2024

WebR full support for dehydrated device v2 element-hq/element-meta#2278

Closed

poljar approved these changes Mar 15, 2024

View reviewed changes

Merge branch 'main' into dehydrated_flag

cd245b5

uhoreg commented Mar 16, 2024

View reviewed changes

poljar requested changes Mar 18, 2024

View reviewed changes

remove leftover from merge conflict

ae39e4f

poljar requested changes Mar 18, 2024

View reviewed changes

use JsOption for dehydrated flag, and import DeviceKeys type

c9cca55

poljar approved these changes Mar 18, 2024

View reviewed changes

fix formatting

cfedc24

poljar enabled auto-merge (squash) March 18, 2024 16:00

poljar merged commit 1e35188 into matrix-org:main Mar 19, 2024
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dehydrated flag to device_keys of dehydrated devices #3164

Add dehydrated flag to device_keys of dehydrated devices #3164

uhoreg commented Feb 23, 2024

poljar left a comment

codecov-commenter commented Feb 28, 2024 •

edited by codecov bot

Loading

uhoreg Feb 29, 2024

poljar Mar 12, 2024

uhoreg Mar 12, 2024

poljar Mar 12, 2024

poljar left a comment

poljar Mar 12, 2024

poljar Mar 12, 2024

uhoreg Mar 13, 2024

poljar Mar 15, 2024

poljar left a comment

uhoreg Mar 16, 2024

poljar Mar 18, 2024

poljar Mar 18, 2024

poljar left a comment

poljar Mar 18, 2024

poljar left a comment

uhoreg commented Mar 18, 2024

		#[serde(default, skip_serializing_if = "Option::is_none")]
		pub dehydrated: Option<bool>,

Add dehydrated flag to device_keys of dehydrated devices #3164

Add dehydrated flag to device_keys of dehydrated devices #3164

Conversation

uhoreg commented Feb 23, 2024

poljar left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 28, 2024 • edited by codecov bot Loading

Codecov Report

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

poljar 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

poljar 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

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

uhoreg commented Mar 18, 2024

codecov-commenter commented Feb 28, 2024 •

edited by codecov bot

Loading