Merge pull request #3442 from matrix-org/rav/no_device_update_on_unch…

…anged

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

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")]