From e45deb622c7d09aae1c951a0a1d4745d5edde17f Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Sat, 21 Feb 2026 17:29:52 +0100 Subject: [PATCH 1/3] server: Add missing description --- server/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/server/Cargo.toml b/server/Cargo.toml index a6d85442..db505399 100644 --- a/server/Cargo.toml +++ b/server/Cargo.toml @@ -1,6 +1,7 @@ [package] name = "oo7-daemon" authors = ["Bilal Elmoussaoui", "Dhanuka Warusadura"] +description = "Server side implementation of the freedesktop Secret Service" edition.workspace = true exclude.workspace = true homepage.workspace = true From 450c1083f15699bbd1b852df5734b2f53b9f8116 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Fri, 6 Mar 2026 17:08:55 +0100 Subject: [PATCH 2/3] client: Don't over-abstract locked/unlocked items Fixes #416 --- cli/src/main.rs | 31 ++++--- client/examples/file_tracing.rs | 2 +- client/examples/schema.rs | 7 +- client/src/file/locked_keyring.rs | 12 ++- client/src/file/mod.rs | 45 ++++++++-- client/src/file/unlocked_keyring.rs | 65 +++++++++------ client/src/keyring.rs | 6 +- client/tests/file_unlocked_keyring.rs | 115 ++++++++++---------------- client/tests/schema.rs | 8 +- server/src/collection/mod.rs | 4 +- server/src/error.rs | 3 - 11 files changed, 150 insertions(+), 148 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 3dc79ae4..82679d4d 100644 --- a/cli/src/main.rs +++ b/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 diff --git a/client/examples/file_tracing.rs b/client/examples/file_tracing.rs index 70a1609e..6cf25200 100644 --- a/client/examples/file_tracing.rs +++ b/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 diff --git a/client/examples/schema.rs b/client/examples/schema.rs index 402125ca..5b7c301a 100644 --- a/client/examples/schema.rs +++ b/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::()?; + let schema = item.attributes_as::()?; println!("Username: {}", schema.username); println!("Server: {}", schema.server); println!("Port: {:?}", schema.port); diff --git a/client/src/file/locked_keyring.rs b/client/src/file/locked_keyring.rs index 36c80447..aa522632 100644 --- a/client/src/file/locked_keyring.rs +++ b/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>, Error> { + pub async fn items(&self) -> Result, 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()) } diff --git a/client/src/file/mod.rs b/client/src/file/mod.rs index 770ce231..2157828a 100644 --- a/client/src/file/mod.rs +++ b/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 for Item { + fn from(item: UnlockedItem) -> Self { + Self::Unlocked(item) + } +} + +impl From 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 for Keyring { + fn from(keyring: LockedKeyring) -> Self { + Self::Locked(keyring) + } +} + +impl From 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>, Error> { + pub async fn items(&self) -> Result, 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()), } } diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index ed1be1db..65376586 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/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>, Error> { + pub async fn all_items(&self) -> Result>, 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, 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, Error> { + pub async fn search_items( + &self, + attributes: &impl AsAttributes, + ) -> Result, 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::>(); + 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, Error> { + pub async fn lookup_item( + &self, + attributes: &impl AsAttributes, + ) -> Result, 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, replace: bool, - ) -> Result { + ) -> Result { 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) } } } diff --git a/client/src/keyring.rs b/client/src/keyring.rs index f760af27..81fabb19 100644 --- a/client/src/keyring.rs +++ b/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::>() } 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::>() } Some(file::Keyring::Locked(_)) => { diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 74f9a206..e9d90053 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -96,12 +96,11 @@ async fn concurrent_writes() -> Result<(), Error> { async fn check_items(keyring: &UnlockedKeyring) -> Result<(), Error> { assert_eq!(keyring.n_items().await, 1); - let items: Result, _> = keyring.items().await?.into_iter().collect(); - let items = items.expect("unable to retrieve items"); + let items = keyring.items().await?; assert_eq!(items.len(), 1); - assert_eq!(items[0].as_unlocked().label(), "foo"); - assert_eq!(items[0].as_unlocked().secret(), Secret::blob("foo")); - let attributes = items[0].as_unlocked().attributes(); + assert_eq!(items[0].label(), "foo"); + assert_eq!(items[0].secret(), Secret::blob("foo")); + let attributes = items[0].attributes(); assert_eq!(attributes.len(), 2); assert_eq!( attributes @@ -348,7 +347,6 @@ async fn change_secret() -> Result<(), Error> { let item_before = keyring .create_item("test", attributes, "password", false) .await?; - let item_before = item_before.as_unlocked(); let secret = Secret::blob("new_secret"); keyring.change_secret(secret).await?; @@ -356,7 +354,6 @@ async fn change_secret() -> Result<(), Error> { let secret = Secret::blob("new_secret"); let keyring = UnlockedKeyring::load(&keyring_path, secret).await?; let item_now = keyring.lookup_item(attributes).await?.unwrap(); - let item_now = item_now.as_unlocked(); assert_eq!(item_before.label(), item_now.label()); assert_eq!(item_before.secret(), item_now.secret()); @@ -395,17 +392,11 @@ async fn content_type() -> Result<(), Error> { let items = keyring.search_items(&[("type", "text")]).await?; assert_eq!(items.len(), 1); - assert_eq!( - items[0].as_unlocked().secret().content_type(), - oo7::ContentType::Text - ); + assert_eq!(items[0].secret().content_type(), oo7::ContentType::Text); let items = keyring.search_items(&[("type", "password")]).await?; assert_eq!(items.len(), 1); - assert_eq!( - items[0].as_unlocked().secret().content_type(), - oo7::ContentType::Blob - ); + assert_eq!(items[0].secret().content_type(), oo7::ContentType::Blob); Ok(()) } @@ -490,7 +481,7 @@ async fn comprehensive_search_patterns() -> Result<(), Error> { ]) .await?; assert_eq!(exact.len(), 1); - assert_eq!(exact[0].as_unlocked().label(), "Email Password"); + assert_eq!(exact[0].label(), "Email Password"); // Test partial match - by app let email_items = keyring.search_items(&[("app", "email")]).await?; @@ -526,8 +517,8 @@ async fn item_replacement_behavior() -> Result<(), Error> { // Verify initial state let items = keyring.search_items(attrs).await?; assert_eq!(items.len(), 1); - assert_eq!(items[0].as_unlocked().label(), "Original"); - assert_eq!(items[0].as_unlocked().secret(), Secret::text("secret1")); + assert_eq!(items[0].label(), "Original"); + assert_eq!(items[0].secret(), Secret::text("secret1")); // With replace=false, allows duplicates (discovered behavior) keyring @@ -539,7 +530,7 @@ async fn item_replacement_behavior() -> Result<(), Error> { assert_eq!(items.len(), 2); // Verify both items exist with different content - let labels: Vec<_> = items.iter().map(|i| i.as_unlocked().label()).collect(); + let labels: Vec<_> = items.iter().map(|i| i.label()).collect(); assert!(labels.contains(&"Original")); assert!(labels.contains(&"Duplicate")); @@ -552,8 +543,8 @@ async fn item_replacement_behavior() -> Result<(), Error> { // After replace=true, should only have the new item let items = keyring.search_items(attrs).await?; assert_eq!(items.len(), 1); - assert_eq!(items[0].as_unlocked().label(), "Replacement"); - assert_eq!(items[0].as_unlocked().secret(), Secret::text("secret3")); + assert_eq!(items[0].label(), "Replacement"); + assert_eq!(items[0].secret(), Secret::text("secret3")); // Test replace=true on empty attributes (should just add) let unique_attrs = &[("app", "unique"), ("user", "bob")]; @@ -563,7 +554,7 @@ async fn item_replacement_behavior() -> Result<(), Error> { let unique_items = keyring.search_items(unique_attrs).await?; assert_eq!(unique_items.len(), 1); - assert_eq!(unique_items[0].as_unlocked().label(), "Unique Item"); + assert_eq!(unique_items[0].label(), "Unique Item"); // Test replace=true again on the unique item - should replace it keyring @@ -572,11 +563,8 @@ async fn item_replacement_behavior() -> Result<(), Error> { let unique_items = keyring.search_items(unique_attrs).await?; assert_eq!(unique_items.len(), 1); - assert_eq!(unique_items[0].as_unlocked().label(), "Updated Unique"); - assert_eq!( - unique_items[0].as_unlocked().secret(), - Secret::text("updated_secret") - ); + assert_eq!(unique_items[0].label(), "Updated Unique"); + assert_eq!(unique_items[0].secret(), Secret::text("updated_secret")); Ok(()) } @@ -657,33 +645,27 @@ async fn secret_types_handling() -> Result<(), Error> { // Verify all secrets can be retrieved correctly let text_items = keyring.search_items(&[("type", "text")]).await?; assert_eq!(text_items.len(), 1); + assert_eq!(text_items[0].secret(), Secret::text("Hello, World!")); assert_eq!( - text_items[0].as_unlocked().secret(), - Secret::text("Hello, World!") - ); - assert_eq!( - text_items[0].as_unlocked().secret().content_type(), + text_items[0].secret().content_type(), oo7::ContentType::Text ); let binary_items = keyring.search_items(&[("type", "binary")]).await?; assert_eq!(binary_items.len(), 1); + assert_eq!(&*binary_items[0].secret(), &[0x00, 0x01, 0x02, 0xFF]); assert_eq!( - &*binary_items[0].as_unlocked().secret(), - &[0x00, 0x01, 0x02, 0xFF] - ); - assert_eq!( - binary_items[0].as_unlocked().secret().content_type(), + binary_items[0].secret().content_type(), oo7::ContentType::Blob ); let large_items = keyring.search_items(&[("type", "large")]).await?; assert_eq!(large_items.len(), 1); - assert_eq!(&*large_items[0].as_unlocked().secret(), &large_data); + assert_eq!(&*large_items[0].secret(), &large_data); let empty_items = keyring.search_items(&[("type", "empty")]).await?; assert_eq!(empty_items.len(), 1); - assert_eq!(empty_items[0].as_unlocked().secret(), Secret::text("")); + assert_eq!(empty_items[0].secret(), Secret::text("")); Ok(()) } @@ -715,17 +697,13 @@ async fn item_lifecycle_operations() -> Result<(), Error> { // Test retrieving all items let items = keyring.items().await?; - let valid_items: Vec<_> = items.into_iter().map(|r| r.unwrap()).collect(); - assert_eq!(valid_items.len(), 2); + assert_eq!(items.len(), 2); // Test searching by user let alice_items = keyring.search_items(&[("user", "alice")]).await?; assert_eq!(alice_items.len(), 1); - assert_eq!(alice_items[0].as_unlocked().label(), "Test Item 1"); - assert_eq!( - alice_items[0].as_unlocked().secret(), - Secret::text("secret1") - ); + assert_eq!(alice_items[0].label(), "Test Item 1"); + assert_eq!(alice_items[0].secret(), Secret::text("secret1")); // Test searching by app (should find both) let app_items = keyring.search_items(&[("app", "myapp")]).await?; @@ -734,9 +712,8 @@ async fn item_lifecycle_operations() -> Result<(), Error> { // Test deleting items keyring.delete(&[("user", "alice")]).await?; let remaining_items = keyring.items().await?; - let valid_remaining: Vec<_> = remaining_items.into_iter().map(|r| r.unwrap()).collect(); - assert_eq!(valid_remaining.len(), 1); - assert_eq!(valid_remaining[0].as_unlocked().label(), "Test Item 2"); + assert_eq!(remaining_items.len(), 1); + assert_eq!(remaining_items[0].label(), "Test Item 2"); Ok(()) } @@ -759,7 +736,7 @@ async fn item_attribute_operations() -> Result<(), Error> { let items = keyring.search_items(&[("app", "testapp")]).await?; assert_eq!(items.len(), 1); - let item = &items[0].as_unlocked(); + let item = &items[0]; // Test reading attributes let attrs = item.attributes(); @@ -775,24 +752,20 @@ async fn item_attribute_operations() -> Result<(), Error> { .unwrap(); let items = keyring.items().await?; - let mut item_to_replace = items.into_iter().next().unwrap().unwrap(); - - if let Item::Unlocked(ref mut unlocked) = item_to_replace { - unlocked.set_attributes(&[ - ("app", "testapp"), - ("version", "2.0"), // updated - ("env", "production"), // updated - ("new_attr", "new_value"), // added - ]); - } + let mut item_to_replace = items.into_iter().next().unwrap(); - keyring - .replace_item_index(index, item_to_replace.as_unlocked()) - .await?; + item_to_replace.set_attributes(&[ + ("app", "testapp"), + ("version", "2.0"), // updated + ("env", "production"), // updated + ("new_attr", "new_value"), // added + ]); + + keyring.replace_item_index(index, &item_to_replace).await?; let updated_items = keyring.search_items(&[("app", "testapp")]).await?; assert_eq!(updated_items.len(), 1); - let updated_attrs = updated_items[0].as_unlocked().attributes(); + let updated_attrs = updated_items[0].attributes(); assert_eq!(updated_attrs.get("version").unwrap().to_string(), "2.0"); assert_eq!(updated_attrs.get("env").unwrap().to_string(), "production"); assert_eq!( @@ -932,7 +905,7 @@ async fn invalid_item_error_on_decrypt_failure() -> Result<(), Error> { let wrong_secret = Secret::from("wrong-password-long-enough".as_bytes()); let keyring = unsafe { UnlockedKeyring::load_unchecked(&keyring_path, wrong_secret).await? }; - let items_result = keyring.items().await?; + let items_result = keyring.all_items().await?; assert_eq!(items_result.len(), 2); assert!(matches!( @@ -960,10 +933,8 @@ async fn replace_item_index_invalid() -> Result<(), Error> { // Try to replace at invalid index let items = keyring.items().await?; - let existing_item = items.into_iter().next().unwrap().unwrap(); - let result = keyring - .replace_item_index(100, existing_item.as_unlocked()) - .await; + let existing_item = items.into_iter().next().unwrap(); + let result = keyring.replace_item_index(100, &existing_item).await; assert!(matches!(result, Err(Error::InvalidItemIndex(100)))); @@ -988,8 +959,7 @@ async fn set_attributes() -> Result<(), Error> { let mut items = keyring.items().await?; assert_eq!(items.len(), 1); - let mut item = items.remove(0).unwrap(); - let item = item.as_mut_unlocked(); + let mut item = items.remove(0); assert_eq!(item.label(), "my item"); assert_eq!(item.secret(), Secret::text("my_secret")); let attrs = item.attributes(); @@ -1005,8 +975,7 @@ async fn set_attributes() -> Result<(), Error> { // Now retrieve the item again from the keyring to verify the changes persisted let mut items = keyring.items().await?; assert_eq!(items.len(), 1); - let item = items.remove(0).unwrap(); - let item = item.as_unlocked(); + let item = items.remove(0); assert_eq!(item.label(), "my item"); assert_eq!(item.secret(), Secret::text("my_secret")); let attrs = item.attributes(); diff --git a/client/tests/schema.rs b/client/tests/schema.rs index 832a87cc..9dc0ddf8 100644 --- a/client/tests/schema.rs +++ b/client/tests/schema.rs @@ -41,7 +41,7 @@ async fn schema_content_type_and_attributes() { .await .unwrap(); - let text_items = keyring + let mut text_items = keyring .search_items(&TestSchema { username: "alice".to_string(), ..Default::default() @@ -50,7 +50,7 @@ async fn schema_content_type_and_attributes() { .unwrap(); assert_eq!(text_items.len(), 1); - let text_item = text_items[0].as_unlocked(); + let text_item = &text_items[0]; assert_eq!( text_item.attributes().get("xdg:content-type").unwrap(), @@ -62,7 +62,7 @@ async fn schema_content_type_and_attributes() { assert_eq!(schema.username, "alice"); assert_eq!(schema.port, Some(8080)); - let mut unlocked_item = text_items[0].as_unlocked().clone(); + let unlocked_item = &mut text_items[0]; unlocked_item.set_attributes(&TestSchema { username: "alice".to_string(), port: Some(9090), @@ -87,7 +87,7 @@ async fn schema_content_type_and_attributes() { .unwrap(); assert_eq!(blob_items.len(), 1); - let blob_item = blob_items[0].as_unlocked(); + let blob_item = &blob_items[0]; assert_eq!( blob_item.attributes().get("xdg:content-type").unwrap(), diff --git a/server/src/collection/mod.rs b/server/src/collection/mod.rs index f71e1190..b6dc0aa5 100644 --- a/server/src/collection/mod.rs +++ b/server/src/collection/mod.rs @@ -259,7 +259,7 @@ impl Collection { let item_path = OwnedObjectPath::try_from(format!("{}/{n_items}", self.path)).unwrap(); let item = item::Item::new( - item, + item.into(), self.service.clone(), self.path.clone(), item_path.clone(), @@ -553,7 +553,7 @@ impl Collection { for keyring_item in keyring_items { let item_path = OwnedObjectPath::try_from(format!("{}/{n_items}", self.path)).unwrap(); let item = item::Item::new( - keyring_item.map_err(Error::InvalidItem)?, + keyring_item, self.service.clone(), self.path.clone(), item_path.clone(), diff --git a/server/src/error.rs b/server/src/error.rs index 5840429c..d935b420 100644 --- a/server/src/error.rs +++ b/server/src/error.rs @@ -12,8 +12,6 @@ pub enum Error { IO(std::io::Error), // Empty password error EmptyPassword, - // Invalid item error - InvalidItem(oo7::file::InvalidItemError), // Capability error Capability(rustix::io::Errno), } @@ -51,7 +49,6 @@ impl fmt::Display for Error { Self::Zbus(err) => write!(f, "Zbus error {err}"), Self::IO(err) => write!(f, "IO error {err}"), Self::EmptyPassword => write!(f, "Login password can't be empty"), - Self::InvalidItem(err) => write!(f, "Item cannot be decrypted {err}"), Self::Capability(err) => write!(f, "Capability error {err}"), } } From fc8875225bd9dec8fedb5317a65d14745ea983c8 Mon Sep 17 00:00:00 2001 From: Bilal Elmoussaoui Date: Sat, 7 Mar 2026 17:22:40 +0100 Subject: [PATCH 3/3] cli: Avoid crashing if items are locked By not querying any sensitive data if the items are locked except the label which is queryable. --- cli/src/main.rs | 141 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 100 insertions(+), 41 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 82679d4d..63a7e8e1 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -62,84 +62,130 @@ impl Termination for Error { #[derive(Serialize)] struct ItemOutput { label: String, - secret: String, - created_at: String, - modified_at: String, + /// Those are None if the item is locked + #[serde(skip_serializing_if = "Option::is_none")] + secret: Option, + #[serde(skip_serializing_if = "Option::is_none")] + created_at: Option, + #[serde(skip_serializing_if = "Option::is_none")] + modified_at: Option, #[serde(skip_serializing_if = "Option::is_none")] schema: Option, #[serde(skip_serializing_if = "Option::is_none")] content_type: Option, - attributes: HashMap, + #[serde(skip_serializing_if = "Option::is_none")] + attributes: Option>, + is_locked: bool, } impl ItemOutput { fn new( - secret: &oo7::Secret, + secret: Option<&oo7::Secret>, label: &str, - mut attributes: HashMap, - created: Duration, - modified: Duration, + mut attributes: Option>, + created: Option, + modified: Option, + is_locked: bool, as_hex: bool, ) -> Self { - let bytes = secret.as_bytes(); let local_offset = UtcOffset::current_local_offset().unwrap_or(UtcOffset::UTC); - let created = OffsetDateTime::from_unix_timestamp(created.as_secs() as i64) - .unwrap() - .to_offset(local_offset); - let modified = OffsetDateTime::from_unix_timestamp(modified.as_secs() as i64) - .unwrap() - .to_offset(local_offset); + let created = created.map(|created| { + OffsetDateTime::from_unix_timestamp(created.as_secs() as i64) + .unwrap() + .to_offset(local_offset) + }); + let modified = modified.map(|modified| { + OffsetDateTime::from_unix_timestamp(modified.as_secs() as i64) + .unwrap() + .to_offset(local_offset) + }); let format = time::format_description::parse_borrowed::<2>( "[year]-[month]-[day] [hour]:[minute]:[second]", ) .unwrap(); - let secret_str = if as_hex { - hex::encode(bytes) - } else { - match std::str::from_utf8(bytes) { - Ok(s) => s.to_string(), - Err(_) => hex::encode(bytes), + let secret_str = secret.map(|s| { + let bytes = s.as_bytes(); + if as_hex { + hex::encode(bytes) + } else { + match std::str::from_utf8(bytes) { + Ok(s) => s.to_string(), + Err(_) => hex::encode(bytes), + } } - }; + }); - let schema = attributes.remove(oo7::XDG_SCHEMA_ATTRIBUTE); - let content_type = attributes.remove(oo7::CONTENT_TYPE_ATTRIBUTE); + let schema = attributes + .as_mut() + .and_then(|attrs| attrs.remove(oo7::XDG_SCHEMA_ATTRIBUTE)); + let content_type = attributes + .as_mut() + .and_then(|attrs| attrs.remove(oo7::CONTENT_TYPE_ATTRIBUTE)); Self { label: label.to_string(), secret: secret_str, - created_at: created.format(&format).unwrap(), - modified_at: modified.format(&format).unwrap(), + created_at: created.map(|created| created.format(&format).unwrap()), + modified_at: modified.map(|modified| modified.format(&format).unwrap()), schema, content_type, attributes, + is_locked, } } fn from_file_item(item: &oo7::file::UnlockedItem, as_hex: bool) -> Self { Self::new( - &item.secret(), + Some(&item.secret()), item.label(), - item.attributes() - .iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(), - item.created(), - item.modified(), + Some( + item.attributes() + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + ), + Some(item.created()), + Some(item.modified()), + false, as_hex, ) } async fn from_dbus_item(item: &oo7::dbus::Item, as_hex: bool) -> Result { + use oo7::dbus::ServiceError; + + let is_locked = item.is_locked().await?; + let secret = match item.secret().await { + Ok(secret) => Ok(Some(secret)), + Err(oo7::dbus::Error::Service(ServiceError::IsLocked(_))) => Ok(None), + Err(e) => Err(e), + }?; + let attributes = match item.attributes().await { + Ok(attributes) => Ok(Some(attributes)), + Err(oo7::dbus::Error::Service(ServiceError::IsLocked(_))) => Ok(None), + Err(e) => Err(e), + }?; + let created = match item.created().await { + Ok(created) => Ok(Some(created)), + Err(oo7::dbus::Error::Service(ServiceError::IsLocked(_))) => Ok(None), + Err(e) => Err(e), + }?; + let modified = match item.modified().await { + Ok(modified) => Ok(Some(modified)), + Err(oo7::dbus::Error::Service(ServiceError::IsLocked(_))) => Ok(None), + Err(e) => Err(e), + }?; + Ok(Self::new( - &item.secret().await?, + secret.as_ref(), &item.label().await?, - item.attributes().await?, - item.created().await?, - item.modified().await?, + attributes, + created, + modified, + is_locked, as_hex, )) } @@ -148,16 +194,29 @@ impl ItemOutput { impl fmt::Display for ItemOutput { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { writeln!(f, "[{}]", self.label)?; - writeln!(f, "secret = {}", self.secret)?; - writeln!(f, "created = {}", self.created_at)?; - writeln!(f, "modified = {}", self.modified_at)?; + if let Some(ref secret) = self.secret { + writeln!(f, "secret = {}", secret)?; + } + if let Some(ref created_at) = self.created_at { + writeln!(f, "created = {}", created_at)?; + } + if let Some(ref modified_at) = self.modified_at { + writeln!(f, "modified = {}", modified_at)?; + } if let Some(schema) = &self.schema { writeln!(f, "schema = {schema}")?; } if let Some(content_type) = &self.content_type { writeln!(f, "content_type = {content_type}")?; } - writeln!(f, "attributes = {:?}", self.attributes)?; + if let Some(attributes) = &self.attributes { + writeln!(f, "attributes = {:?}", attributes)?; + } + if self.is_locked { + writeln!(f, "locked = true")?; + } else { + writeln!(f, "locked = false")?; + } Ok(()) } }