Repository: bilelmoussaoui/oo7 (language: rust)

Issue:
`oo7::Item::set_attributes()` with the file backend creates a new item
The reason to call that method should be to update the attributes of an existing item.

However, for the file backend, that method calls `Keyring::create_item()` with `replace: true`, that only replaces items with the same attributes, so if the attributes changed (which is usually the case) a new item is created.

Maybe the implementation should be changed to use `Keyring::replace_item_index()` instead?


Discussion / hints:

Fix patch:
diff --git a/client/src/keyring.rs b/client/src/keyring.rs
index d1270bc1d..0ee3c4ae8 100644
--- a/client/src/keyring.rs
+++ b/client/src/keyring.rs
@@ -238,11 +238,20 @@ impl Item {
     pub async fn set_attributes(&self, attributes: &impl AsAttributes) -> Result<()> {
         match self {
             Self::File(item, backend) => {
+                let index = backend
+                    .lookup_item_index(item.read().await.attributes())
+                    .await;
+
                 item.write().await.set_attributes(attributes);
                 let item_guard = item.read().await;
-                backend
-                    .create_item(item_guard.label(), attributes, &*item_guard.secret(), true)
-                    .await?;
+
+                if let Some(index) = index {
+                    backend.replace_item_index(index, &item_guard).await?;
+                } else {
+                    backend
+                        .create_item(item_guard.label(), attributes, &*item_guard.secret(), true)
+                        .await?;
+                }
             }
             Self::DBus(item) => item.set_attributes(attributes).await?,
         };
@@ -342,3 +351,55 @@ impl Item {
         }
     }
 }
+
+#[cfg(test)]
+#[cfg(feature = "tokio")]
+mod tests {
+    use tempfile::tempdir;
+    use tokio::fs;
+
+    use super::*;
+
+    #[tokio::test]
+    async fn portal_set_attributes() -> Result<()> {
+        let data_dir = tempdir().unwrap();
+        let dir = data_dir.path().join("keyrings");
+        fs::create_dir_all(&dir).await.unwrap();
+        let path = dir.join("default.keyring");
+
+        let password = b"test";
+        let secret = portal::Secret::from(password.to_vec());
+        let keyring = Keyring::File(portal::Keyring::load(&path, secret).await?.into());
+
+        let items = keyring.items().await?;
+        assert_eq!(items.len(), 0);
+
+        keyring
+            .create_item("my item", &vec![("key", "value")], "my_secret", false)
+            .await?;
+
+        let mut items = keyring.items().await?;
+        assert_eq!(items.len(), 1);
+        let item = items.remove(0);
+        assert_eq!(item.label().await?, "my item");
+        assert_eq!(*item.secret().await?, b"my_secret");
+        let attrs = item.attributes().await?;
+        assert_eq!(attrs.len(), 1);
+        assert_eq!(attrs.get("key").unwrap(), "value");
+
+        item.set_attributes(&vec![("key", "changed_value"), ("new_key", "new_value")])
+            .await?;
+
+        let mut items = keyring.items().await?;
+        assert_eq!(items.len(), 1);
+        let item = items.remove(0);
+        assert_eq!(item.label().await?, "my item");
+        assert_eq!(*item.secret().await?, b"my_secret");
+        let attrs = item.attributes().await?;
+        assert_eq!(attrs.len(), 2);
+        assert_eq!(attrs.get("key").unwrap(), "changed_value");
+        assert_eq!(attrs.get("new_key").unwrap(), "new_value");
+
+        Ok(())
+    }
+}
diff --git a/client/src/portal/api/mod.rs b/client/src/portal/api/mod.rs
index 487fd2494..ce159b98d 100644
--- a/client/src/portal/api/mod.rs
+++ b/client/src/portal/api/mod.rs
@@ -198,6 +198,14 @@ impl Keyring {
             .transpose()
     }
 
+    pub fn lookup_item_index(&self, attributes: &impl AsAttributes, key: &Key) -> Option<usize> {
+        let hashed_search = attributes.hash(key);
+
+        self.items
+            .iter()
+            .position(|e| hashed_search.iter().all(|(k, v)| e.has_attribute(k, v)))
+    }
+
     pub fn remove_items(&mut self, attributes: &impl AsAttributes, key: &Key) -> Result<(), Error> {
         let hashed_search = attributes.hash(key);
 
diff --git a/client/src/portal/mod.rs b/client/src/portal/mod.rs
index fd96825cd..461eeff4f 100644
--- a/client/src/portal/mod.rs
+++ b/client/src/portal/mod.rs
@@ -253,6 +253,14 @@ impl Keyring {
         keyring.lookup_item(attributes, &key)
     }
 
+    /// Find the index in the list of items of the first item matching the
+    /// attributes.
+    pub async fn lookup_item_index(&self, attributes: &impl AsAttributes) -> Option<usize> {
+        let key = self.derive_key().await;
+        let keyring = self.keyring.read().await;
+        keyring.lookup_item_index(attributes, &key)
+    }
+
     /// Delete an item.
     pub async fn delete(&self, attributes: &impl AsAttributes) -> Result<(), Error> {
         {


Test patch: