Merge pull request #3442 from matrix-org/rav/no_device_update_on_unchanged

crypto: emit no `identities_stream` items on no-op changes

Author: Hywan

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -4,7 +4,11 @@ Security fixes:
 
 - Don't log the private part of the backup key, introduced in [#71136e4](https://github.com/matrix-org/matrix-rust-sdk/commit/71136e44c03c79f80d6d1a2446673bc4d53a2067).
 
-Changed:
+Changes:
+
+- Avoid emitting entries from `identities_stream_raw` and `devices_stream` when
+  we receive a `/keys/query` response which shows that no devices changed.
+  ([#3442](https://github.com/matrix-org/matrix-rust-sdk/pull/3442))
 
 - Fallback keys are rotated in a time-based manner, instead of waiting for the
   server to tell us that a fallback key got used.

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

@@ -848,7 +848,12 @@ impl ReadOnlyDevice {
     }
 
     /// Update a device with a new device keys struct.
-    pub(crate) fn update_device(&mut self, device_keys: &DeviceKeys) -> Result<(), SignatureError> {
+    ///
+    /// Returns `true` if any changes were made to the data.
+    pub(crate) fn update_device(
+        &mut self,
+        device_keys: &DeviceKeys,
+    ) -> Result<bool, SignatureError> {
         self.verify_device_keys(device_keys)?;
 
         if self.user_id() != device_keys.user_id || self.device_id() != device_keys.device_id {
@@ -858,10 +863,13 @@ impl ReadOnlyDevice {
                 self.ed25519_key().map(Box::new),
                 device_keys.ed25519_key().map(Box::new),
             ))
-        } else {
+        } else if self.inner.as_ref() != device_keys {
             self.inner = device_keys.clone().into();
 
-            Ok(())
+            Ok(true)
+        } else {
+            // no changes needed
+            Ok(false)
         }
     }
 
@@ -1066,9 +1074,11 @@ pub(crate) mod tests {
 
         let mut device_keys = device_keys();
         device_keys.unsigned.device_display_name = Some(display_name.clone());
-        device.update_device(&device_keys).unwrap();
-
+        assert!(device.update_device(&device_keys).unwrap());
         assert_eq!(&display_name, device.display_name().as_ref().unwrap());
+
+        // A second call to `update_device` with the same data should return `false`.
+        assert!(!device.update_device(&device_keys).unwrap());
     }
 
     #[test]

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

@@ -227,17 +227,18 @@ impl IdentityManager {
             store.get_readonly_device(&device_keys.user_id, &device_keys.device_id).await?;
 
         if let Some(mut device) = old_device {
-            if let Err(e) = device.update_device(&device_keys) {
-                warn!(
-                    user_id = ?device.user_id(),
-                    device_id = ?device.device_id(),
-                    error = ?e,
-                    "Rejecting device update",
-                );
-
-                Ok(DeviceChange::None)
-            } else {
-                Ok(DeviceChange::Updated(device))
+            match device.update_device(&device_keys) {
+                Err(e) => {
+                    warn!(
+                        user_id = ?device.user_id(),
+                        device_id = ?device.device_id(),
+                        error = ?e,
+                        "Rejecting device update",
+                    );
+                    Ok(DeviceChange::None)
+                }
+                Ok(true) => Ok(DeviceChange::Updated(device)),
+                Ok(false) => Ok(DeviceChange::None),
             }
         } else {
             match ReadOnlyDevice::try_from(&device_keys) {
@@ -1668,6 +1669,67 @@ pub(crate) mod tests {
         assert_eq!(devices.devices().next().unwrap().device_id(), "LVWOVGOXME");
     }
 
+    #[async_test]
+    async fn test_invalid_key_response() {
+        let my_user_id = user_id();
+        let my_device_id = device_id();
+        let manager = manager_test_helper(my_user_id, my_device_id).await;
+
+        // First of all, populate the store with good data
+        let (reqid, _) = manager.build_key_query_for_users(vec![user_id()]);
+        let (device_changes, identity_changes) =
+            manager.receive_keys_query_response(&reqid, &own_key_query()).await.unwrap();
+        assert_eq!(device_changes.new.len(), 1);
+        let test_device_id = device_changes.new.first().unwrap().device_id().to_owned();
+        use crate::store::Changes;
+        let changes =
+            Changes { devices: device_changes, identities: identity_changes, ..Changes::default() };
+        manager.store.save_changes(changes).await.unwrap();
+
+        // Now provide an invalid update
+        let (reqid, _) = manager.build_key_query_for_users(vec![my_user_id]);
+        let response_data = matrix_sdk_test::response_from_file(&json!({
+            "device_keys": {
+                my_user_id: {
+                    test_device_id.as_str(): {
+                        "algorithms": [
+                            "m.olm.v1.curve25519-aes-sha2",
+                        ],
+                        "device_id": test_device_id.as_str(),
+                        "keys": {
+                            format!("curve25519:{}", test_device_id): "wnip2tbJBJxrFayC88NNJpm61TeSNgYcqBH4T9yEDhU",
+                            format!("ed25519:{}", test_device_id): "lQ+eshkhgKoo+qp9Qgnj3OX5PBoWMU5M9zbuEevwYqE"
+                        },
+                        "signatures": {
+                            my_user_id: {
+                                // Not a valid signature.
+                                format!("ed25519:{}", test_device_id): "imadethisup",
+                            }
+                        },
+                        "user_id": my_user_id,
+                    }
+                }
+            }
+        }));
+        let response =
+            ruma::api::client::keys::get_keys::v3::Response::try_from_http_response(response_data)
+                .expect("Can't parse the `/keys/upload` response");
+
+        let (device_changes, identity_changes) =
+            manager.receive_keys_query_response(&reqid, &response).await.unwrap();
+
+        // The result should be empty
+        assert_eq!(device_changes.new.len(), 0);
+        assert_eq!(device_changes.changed.len(), 0);
+        assert_eq!(device_changes.deleted.len(), 0);
+        assert_eq!(identity_changes.new.len(), 0);
+
+        // And the device should not have been updated.
+        let device =
+            manager.store.get_user_devices(my_user_id).await.unwrap().get(&test_device_id).unwrap();
+        assert_eq!(device.algorithms().len(), 2);
+    }
+
     #[async_test]
     async fn test_devices_stream() {
         let manager = manager_test_helper(user_id(), device_id()).await;
@@ -1723,18 +1785,15 @@ pub(crate) mod tests {
             manager.as_ref().unwrap().build_key_query_for_users(vec![user_id()]);
 
         // A second `/keys/query` response with the same result shouldn't fire a change
-        // notification: the identity should be unchanged.
+        // notification: the identity and device should be unchanged.
         manager
             .as_ref()
             .unwrap()
             .receive_keys_query_response(&new_request_id, &own_key_query())
             .await
             .unwrap();
 
-        let (identity_update_2, _) = assert_ready!(stream);
-        assert_eq!(identity_update_2.new.len(), 0);
-        assert_eq!(identity_update_2.changed.len(), 0);
-        assert_eq!(identity_update_2.unchanged.len(), 1);
+        assert_pending!(stream);
 
         // dropping the manager (and hence dropping the store) should close the stream
         manager.take();

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

@@ -39,7 +39,7 @@ use super::{EventEncryptionAlgorithm, Signatures};
 /// identity keys.
 ///
 /// [device_keys_spec]: https://spec.matrix.org/v1.10/client-server-api/#_matrixclientv3keysupload_devicekeys
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
 #[serde(try_from = "DeviceKeyHelper", into = "DeviceKeyHelper")]
 pub struct DeviceKeys {
     /// The ID of the user the device belongs to.
@@ -130,7 +130,7 @@ impl DeviceKeys {
 }
 
 /// Additional data added to device key information by intermediate servers.
-#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)]
 pub struct UnsignedDeviceInfo {
     /// The display name which the user set on the device.
     #[serde(skip_serializing_if = "Option::is_none")]