client: Don't over-abstract locked/unlocked items

Fixes #416

Author: bilelmoussaoui

cli/src/main.rs

@@ -119,18 +119,16 @@ impl ItemOutput {
         }
     }
 
-    fn from_file_item(item: &oo7::file::Item, as_hex: bool) -> Self {
-        let unlocked = item.as_unlocked();
+    fn from_file_item(item: &oo7::file::UnlockedItem, as_hex: bool) -> Self {
         Self::new(
-            &unlocked.secret(),
-            unlocked.label(),
-            unlocked
-                .attributes()
+            &item.secret(),
+            item.label(),
+            item.attributes()
                 .iter()
                 .map(|(k, v)| (k.to_string(), v.to_string()))
                 .collect(),
-            unlocked.created(),
-            unlocked.modified(),
+            item.created(),
+            item.modified(),
             as_hex,
         )
     }
@@ -378,7 +376,7 @@ impl Commands {
                     let items = keyring.search_items(&attributes).await?;
                     if let Some(item) = items.first() {
                         if secret_only {
-                            Output::SecretOnly(vec![item.as_unlocked().secret().clone()], hex)
+                            Output::SecretOnly(vec![item.secret().clone()], hex)
                         } else {
                             Output::Items(vec![ItemOutput::from_file_item(item, hex)], json)
                         }
@@ -405,7 +403,7 @@ impl Commands {
                     if secret_only {
                         let secrets = items_to_print
                             .into_iter()
-                            .map(|item| item.as_unlocked().secret().clone())
+                            .map(|item| item.secret().clone())
                             .collect();
 
                         Output::SecretOnly(secrets, hex)
@@ -472,14 +470,15 @@ impl Commands {
             Commands::List { hex, json } => {
                 let items = match keyring {
                     Keyring::File(keyring) => {
-                        let items = keyring.items().await?;
+                        let items = keyring.all_items().await?;
                         let mut outputs = Vec::new();
                         for item in items {
-                            if let Ok(item) = item {
-                                outputs.push(ItemOutput::from_file_item(&item, hex));
-                            } else if !json {
-                                // Only print error message in text mode, skip in JSON mode
-                                println!("Item is not valid and cannot be decrypted");
+                            match item {
+                                Ok(item) => outputs.push(ItemOutput::from_file_item(&item, hex)),
+                                Err(_) if !json => {
+                                    println!("Item is not valid and cannot be decrypted");
+                                }
+                                Err(_) => {} // Skip invalid items in JSON mode
                             }
                         }
                         outputs

client/examples/file_tracing.rs

@@ -294,7 +294,7 @@ async fn test_search_performance() -> oo7::Result<()> {
     let start = Instant::now();
     let items = keyring.items().await?;
     let all_items_time = start.elapsed();
-    let valid_items = items.iter().filter(|r| r.is_ok()).count();
+    let valid_items = items.iter().count();
     info!(
         "Get all items: {:?} (found {} valid items)",
         all_items_time, valid_items

client/examples/schema.rs

@@ -52,15 +52,14 @@ async fn main() -> oo7::Result<()> {
     println!("Found {} item(s)", items.len());
 
     for item in &items {
-        let unlocked = item.as_unlocked();
-        println!("  Label: {}", unlocked.label());
-        println!("  Secret: {:?}", unlocked.secret());
+        println!("  Label: {}", item.label());
+        println!("  Secret: {:?}", item.secret());
     }
 
     println!("\n=== Typed attributes ===");
 
     if let Some(item) = items.first() {
-        let schema = item.as_unlocked().attributes_as::<PasswordSchema>()?;
+        let schema = item.attributes_as::<PasswordSchema>()?;
         println!("Username: {}", schema.username);
         println!("Server: {}", schema.server);
         println!("Port: {:?}", schema.port);

client/src/file/locked_keyring.rs

@@ -18,8 +18,8 @@ use tokio::{
     sync::{Mutex, RwLock},
 };
 
-use super::{Error, Item, LockedItem, UnlockedKeyring, api};
-use crate::{Secret, file::InvalidItemError};
+use super::{Error, LockedItem, UnlockedKeyring, api};
+use crate::Secret;
 
 /// A locked keyring that requires a secret to unlock.
 #[derive(Debug)]
@@ -56,16 +56,14 @@ impl LockedKeyring {
 
     /// Retrieve the list of available [`LockedItem`]s without decrypting them.
     #[cfg_attr(feature = "tracing", tracing::instrument(skip(self)))]
-    pub async fn items(&self) -> Result<Vec<Result<Item, InvalidItemError>>, Error> {
+    pub async fn items(&self) -> Result<Vec<LockedItem>, Error> {
         let keyring = self.keyring.read().await;
 
         Ok(keyring
             .items
             .iter()
-            .map(|encrypted_item| {
-                Ok(Item::Locked(LockedItem {
-                    inner: encrypted_item.clone(),
-                }))
+            .map(|encrypted_item| LockedItem {
+                inner: encrypted_item.clone(),
             })
             .collect())
     }

client/src/file/mod.rs

@@ -10,10 +10,7 @@
 //!     .await?;
 //!
 //! let items = keyring.search_items(&[("account", "alice")]).await?;
-//! assert_eq!(
-//!     items[0].as_unlocked().secret(),
-//!     oo7::Secret::blob("My Password")
-//! );
+//! assert_eq!(items[0].secret(), oo7::Secret::blob("My Password"));
 //!
 //! keyring.delete(&[("account", "alice")]).await?;
 //! #   Ok(())
@@ -46,6 +43,18 @@ pub enum Item {
     Unlocked(UnlockedItem),
 }
 
+impl From<UnlockedItem> for Item {
+    fn from(item: UnlockedItem) -> Self {
+        Self::Unlocked(item)
+    }
+}
+
+impl From<LockedItem> for Item {
+    fn from(item: LockedItem) -> Self {
+        Self::Locked(item)
+    }
+}
+
 impl Item {
     pub const fn is_locked(&self) -> bool {
         matches!(self, Self::Locked(_))
@@ -102,6 +111,18 @@ pub enum Keyring {
     Unlocked(UnlockedKeyring),
 }
 
+impl From<LockedKeyring> for Keyring {
+    fn from(keyring: LockedKeyring) -> Self {
+        Self::Locked(keyring)
+    }
+}
+
+impl From<UnlockedKeyring> for Keyring {
+    fn from(keyring: UnlockedKeyring) -> Self {
+        Self::Unlocked(keyring)
+    }
+}
+
 impl Keyring {
     /// Validate that a secret can decrypt the items in this keyring.
     #[cfg_attr(feature = "tracing", tracing::instrument(skip(self, secret)))]
@@ -136,10 +157,20 @@ impl Keyring {
             .and_then(|time| time.duration_since(std::time::SystemTime::UNIX_EPOCH).ok())
     }
 
-    pub async fn items(&self) -> Result<Vec<Result<Item, InvalidItemError>>, Error> {
+    pub async fn items(&self) -> Result<Vec<Item>, Error> {
         match self {
-            Self::Locked(keyring) => keyring.items().await,
-            Self::Unlocked(keyring) => keyring.items().await,
+            Self::Locked(keyring) => Ok(keyring
+                .items()
+                .await?
+                .into_iter()
+                .map(Item::Locked)
+                .collect()),
+            Self::Unlocked(keyring) => Ok(keyring
+                .items()
+                .await?
+                .into_iter()
+                .map(Item::Unlocked)
+                .collect()),
         }
     }
 

client/src/file/unlocked_keyring.rs

@@ -19,7 +19,7 @@ use tokio::{
 
 use crate::{
     AsAttributes, Key, Secret,
-    file::{Error, InvalidItemError, Item, LockedItem, LockedKeyring, UnlockedItem, api},
+    file::{Error, InvalidItemError, LockedItem, LockedKeyring, UnlockedItem, api},
 };
 
 /// Definition for batch item creation: (label, attributes, secret, replace)
@@ -243,45 +243,55 @@ impl UnlockedKeyring {
         self.keyring.read().await.items.len()
     }
 
-    /// Retrieve the list of available [`UnlockedItem`]s.
+    /// Retrieve all items including those that cannot be decrypted.
+    ///
+    /// Returns a [`Vec`] where each element is either an [`UnlockedItem`] or an
+    /// [`InvalidItemError`] for items that failed to decrypt.
     ///
-    /// If items cannot be decrypted, [`InvalidItemError`]s are returned for
-    /// them instead of [`UnlockedItem`]s.
+    /// Use this method when you need to know about or handle decryption
+    /// failures. For most use cases, [`items()`](Self::items) is more
+    /// convenient as it only returns successfully decrypted items.
     #[cfg_attr(feature = "tracing", tracing::instrument(skip(self)))]
-    pub async fn items(&self) -> Result<Vec<Result<Item, InvalidItemError>>, Error> {
+    pub async fn all_items(&self) -> Result<Vec<Result<UnlockedItem, InvalidItemError>>, Error> {
         let key = self.derive_key().await?;
         let keyring = self.keyring.read().await;
 
         #[cfg(feature = "tracing")]
-        let _span = tracing::debug_span!("decrypt", total_items = keyring.items.len());
+        let _span = tracing::debug_span!("decrypt_all", total_items = keyring.items.len());
 
         Ok(keyring
             .items
             .iter()
             .map(|e| {
-                (*e).clone()
-                    .decrypt(&key)
-                    .map_err(|err| {
-                        InvalidItemError::new(
-                            err,
-                            e.hashed_attributes.keys().map(|x| x.to_string()).collect(),
-                        )
-                    })
-                    .map(Item::Unlocked)
+                (*e).clone().decrypt(&key).map_err(|err| {
+                    InvalidItemError::new(
+                        err,
+                        e.hashed_attributes.keys().map(|x| x.to_string()).collect(),
+                    )
+                })
             })
             .collect())
     }
 
+    /// Retrieve the list of available [`UnlockedItem`]s.
+    ///
+    /// Items that cannot be decrypted are silently skipped. Use
+    /// [`all_items()`](Self::all_items) if you need access to decryption
+    /// errors.
+    #[cfg_attr(feature = "tracing", tracing::instrument(skip(self)))]
+    pub async fn items(&self) -> Result<Vec<UnlockedItem>, Error> {
+        Ok(self.all_items().await?.into_iter().flatten().collect())
+    }
+
     /// Search items matching the attributes.
     #[cfg_attr(feature = "tracing", tracing::instrument(skip(self, attributes)))]
-    pub async fn search_items(&self, attributes: &impl AsAttributes) -> Result<Vec<Item>, Error> {
+    pub async fn search_items(
+        &self,
+        attributes: &impl AsAttributes,
+    ) -> Result<Vec<UnlockedItem>, Error> {
         let key = self.derive_key().await?;
         let keyring = self.keyring.read().await;
-        let results = keyring
-            .search_items(attributes, &key)?
-            .into_iter()
-            .map(Item::Unlocked)
-            .collect::<Vec<Item>>();
+        let results = keyring.search_items(attributes, &key)?;
 
         #[cfg(feature = "tracing")]
         tracing::debug!("Found {} matching items", results.len());
@@ -291,13 +301,14 @@ impl UnlockedKeyring {
 
     /// Find the first item matching the attributes.
     #[cfg_attr(feature = "tracing", tracing::instrument(skip(self, attributes)))]
-    pub async fn lookup_item(&self, attributes: &impl AsAttributes) -> Result<Option<Item>, Error> {
+    pub async fn lookup_item(
+        &self,
+        attributes: &impl AsAttributes,
+    ) -> Result<Option<UnlockedItem>, Error> {
         let key = self.derive_key().await?;
         let keyring = self.keyring.read().await;
 
-        keyring
-            .lookup_item(attributes, &key)
-            .map(|maybe_item| maybe_item.map(Item::Unlocked))
+        keyring.lookup_item(attributes, &key)
     }
 
     /// Find the index in the list of items of the first item matching the
@@ -354,7 +365,7 @@ impl UnlockedKeyring {
         attributes: &impl AsAttributes,
         secret: impl Into<Secret>,
         replace: bool,
-    ) -> Result<Item, Error> {
+    ) -> Result<UnlockedItem, Error> {
         let item = {
             let key = self.derive_key().await?;
             let mut keyring = self.keyring.write().await;
@@ -375,7 +386,7 @@ impl UnlockedKeyring {
             Ok(_) => {
                 #[cfg(feature = "tracing")]
                 tracing::info!("Successfully created item");
-                Ok(Item::Unlocked(item))
+                Ok(item)
             }
         }
     }

client/src/keyring.rs

@@ -174,9 +174,7 @@ impl Keyring {
                         let items = backend.items().await.map_err(crate::Error::File)?;
                         items
                             .into_iter()
-                            // Ignore invalid items
-                            .flatten()
-                            .map(|i| Item::for_file(i, Arc::clone(keyring)))
+                            .map(|i| Item::for_file(i.into(), Arc::clone(keyring)))
                             .collect::<Vec<_>>()
                     }
                     Some(file::Keyring::Locked(_)) => {
@@ -239,7 +237,7 @@ impl Keyring {
                             .map_err(crate::Error::File)?;
                         items
                             .into_iter()
-                            .map(|i| Item::for_file(i, Arc::clone(keyring)))
+                            .map(|i| Item::for_file(i.into(), Arc::clone(keyring)))
                             .collect::<Vec<_>>()
                     }
                     Some(file::Keyring::Locked(_)) => {