Feature/improvements/error handling and testing

cmd/passless/src/authenticator.rs

@@ -58,11 +58,15 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
             self.security_config.user_verification_authentication
         };
 
-        // If backend handles verification (e.g., GPG) and not registration, skip notification
-        if self.storage.lock().unwrap().disable_user_verification()
-            && !is_registration
-            && !should_verify
-        {
+        let storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock during user verification request");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
+
+        if storage.disable_user_verification() && !is_registration && !should_verify {
             debug!("User verification handled by backend (e.g., GPG): {}", info);
             return Ok(UpResult::Accepted);
         }
@@ -113,7 +117,15 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
     fn write_credential(&self, credential: &CredentialRef) -> Result<()> {
         info!("Storing credential for RP: {}", credential.rp_id);
         debug!("Credential ID: {}", bytes_to_hex(credential.id));
-        let mut storage = self.storage.lock().unwrap();
+
+        let mut storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while writing credential");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
+
         storage.write(*credential)?;
         info!(
             "Credential persisted successfully for RP: {}",
@@ -124,7 +136,14 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
 
     fn read_credential(&self, cred_id: &[u8]) -> Result<Option<Credential>> {
         debug!("Reading credential: id={}", bytes_to_hex(cred_id));
-        let mut storage = self.storage.lock().unwrap();
+
+        let mut storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while reading credential");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
 
         match storage.read(cred_id) {
             Ok(cred) => {
@@ -140,15 +159,30 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
 
     fn delete_credential(&self, cred_id: &[u8]) -> Result<()> {
         info!("Removing credential ID: {}", bytes_to_hex(cred_id));
-        let mut storage = self.storage.lock().unwrap();
+
+        let mut storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while deleting credential");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
+
         storage.delete(cred_id)?;
         debug!("Credential removed");
         Ok(())
     }
 
     fn list_credentials(&self, rp_id: &str, _user_id: Option<&[u8]>) -> Result<Vec<Credential>> {
         info!("Listing credentials for RP: {}", rp_id);
-        let mut storage = self.storage.lock().unwrap();
+
+        let mut storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while listing credentials");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
 
         let filter = CredentialFilter::ByRp(rp_id.to_string());
 
@@ -185,7 +219,14 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
 
     fn enumerate_rps(&self) -> Result<Vec<(String, Option<String>, usize)>> {
         debug!("Enumerating relying parties");
-        let mut storage = self.storage.lock().unwrap();
+
+        let mut storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while enumerating RPs");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
 
         // Get all credentials
         let filter = CredentialFilter::None;
@@ -221,7 +262,15 @@ impl<S: CredentialStorage> AuthenticatorCallbacks for PasslessCallbacks<S> {
 
     fn credential_count(&self) -> Result<usize> {
         debug!("Counting total credentials");
-        let storage = self.storage.lock().unwrap();
+
+        let storage = match self.storage.lock() {
+            Ok(s) => s,
+            Err(_) => {
+                error!("Failed to acquire storage lock while counting credentials");
+                return Err(soft_fido2::Error::Other);
+            }
+        };
+
         let count = storage.count_credentials();
         debug!("Total credentials: {}", count);
         Ok(count)
@@ -339,8 +388,13 @@ mod tests {
     #[test]
     fn test_service_creation() {
         let temp_dir = std::env::temp_dir().join("test_passless");
-        std::fs::create_dir_all(&temp_dir).unwrap();
-        let storage = LocalStorageAdapter::new(temp_dir.clone()).unwrap();
+        if let Err(e) = std::fs::create_dir_all(&temp_dir) {
+            panic!("Failed to create temp directory: {}", e);
+        }
+        let storage = match LocalStorageAdapter::new(temp_dir.clone()) {
+            Ok(s) => s,
+            Err(e) => panic!("Failed to create local storage: {}", e),
+        };
 
         let security_config = SecurityConfig {
             check_mlock: false,
@@ -352,7 +406,7 @@ mod tests {
         };
 
         let service = AuthenticatorService::new(storage, security_config);
-        assert!(service.is_ok());
+        assert!(service.is_ok(), "Service creation should succeed");
 
         // Cleanup
         let _ = std::fs::remove_dir_all(temp_dir);

cmd/passless/src/commands/client.rs

@@ -218,7 +218,9 @@ fn open_device_by_selector(list: &TransportList, selector: &str) -> Result<Trans
             )))
         }
         1 => {
-            let (_device_info, transport) = matches.into_iter().next().unwrap();
+            let (_device_info, transport) = matches.into_iter().next().ok_or_else(|| {
+                passless_core::Error::Other("No device found after filtering".to_string())
+            })?;
             Ok(transport)
         }
         _ => {

cmd/passless/src/commands/custom.rs

@@ -152,11 +152,18 @@ mod tests {
         use passless_core::config::SecurityConfig;
 
         let temp_dir = std::env::temp_dir().join("test_passless_custom");
-        std::fs::create_dir_all(&temp_dir).unwrap();
-        let storage = LocalStorageAdapter::new(temp_dir.clone()).unwrap();
+        if let Err(e) = std::fs::create_dir_all(&temp_dir) {
+            panic!("Failed to create temp directory: {}", e);
+        }
+        let storage = match LocalStorageAdapter::new(temp_dir.clone()) {
+            Ok(s) => s,
+            Err(e) => panic!("Failed to create local storage: {}", e),
+        };
+
+        let service = AuthenticatorService::new(storage, SecurityConfig::default());
+        assert!(service.is_ok(), "Service creation should succeed");
 
-        let mut service = AuthenticatorService::new(storage, SecurityConfig::default()).unwrap();
-        register_yubikey_credential_mgmt(&mut service);
+        register_yubikey_credential_mgmt(&mut service.unwrap());
 
         // Test that custom command was registered
         // (Further testing would require CTAP protocol simulation)

cmd/passless/src/notification.rs

@@ -98,14 +98,16 @@ pub fn show_verification_notification(
     // Wait for user action
     handle.wait_for_action(|action| {
         debug!("User action received: {}", action);
-        let mut result = action_result_clone.lock().unwrap();
+        let mut result = action_result_clone
+            .lock()
+            .expect("Failed to lock action result");
         *result = Some(action.to_string());
     });
 
     // Process the action taken
     let action = action_result
         .lock()
-        .unwrap()
+        .expect("Failed to lock action result")
         .clone()
         .unwrap_or_else(|| "__closed".to_string());
 
@@ -174,14 +176,16 @@ pub fn show_yes_no_notification(title: &str, question: &str) -> Result<YesNoResu
     // Wait for user action
     handle.wait_for_action(|action| {
         debug!("User action received: {}", action);
-        let mut result = action_result_clone.lock().unwrap();
+        let mut result = action_result_clone
+            .lock()
+            .expect("Failed to lock action result");
         *result = Some(action.to_string());
     });
 
     // Process the action taken
     let action = action_result
         .lock()
-        .unwrap()
+        .expect("Failed to lock action result")
         .clone()
         .unwrap_or_else(|| "__closed".to_string());
 

cmd/passless/src/storage/credential.rs

@@ -460,8 +460,11 @@ mod tests {
         let _bytes = our_cred.to_bytes().expect("Serialization should succeed");
 
         // Test migration: read soft-fido2 bytes using auto deserializer
-        let deserialized = Credential::from_bytes(&soft_cred.to_bytes().unwrap())
-            .expect("Should migrate from soft-fido2 format");
+        let soft_bytes = soft_cred
+            .to_bytes()
+            .expect("Should be able to serialize soft-fido2 credential");
+        let deserialized =
+            Credential::from_bytes(&soft_bytes).expect("Should migrate from soft-fido2 format");
 
         // Verify all fields match
         assert_eq!(deserialized.id.as_ref(), &[1, 2, 3, 4]);
@@ -496,10 +499,13 @@ mod tests {
         };
 
         // Serialize in soft-fido2's native format
-        let soft_bytes = soft_cred.to_bytes().unwrap();
+        let soft_bytes = soft_cred
+            .to_bytes()
+            .expect("Should be able to serialize soft-fido2 credential");
 
         // Read using auto deserializer (should use soft-fido2 bridge for migration)
-        let our_cred = Credential::from_bytes(&soft_bytes).unwrap();
+        let our_cred =
+            Credential::from_bytes(&soft_bytes).expect("Should migrate from soft-fido2 format");
 
         assert_eq!(our_cred.id.as_ref(), &[1, 2, 3, 4]);
         assert_eq!(our_cred.rp.id, "example.com");
@@ -533,7 +539,9 @@ mod tests {
 
         // Serialize with our format
         let our_cred = Credential::from_soft_fido2(&soft_cred);
-        let our_bytes = our_cred.to_bytes().unwrap();
+        let our_bytes = our_cred
+            .to_bytes()
+            .expect("Should be able to serialize our credential format");
 
         // Deserialize and verify
         let deserialized =

cmd/passless/src/storage/index.rs

@@ -1,4 +1,18 @@
 //! Shared indexing and caching for credential storage backends
+//!
+//! This module provides:
+//! - Efficient credential indexing from file system paths
+//! - Time-limited credential caching for performance
+//! - Memory-safe zeroization of cached credentials
+//!
+//! # Architecture
+//!
+//! The index system uses a three-level lookup structure:
+//! 1. **By ID**: Direct lookup by credential ID (O(1))
+//! 2. **By RP ID**: Filter credentials by relying party (O(1))
+//! 3. **By RP Hash**: Filter by credential ID hash (O(1))
+//!
+//! This allows fast iteration without loading all credentials.
 
 use crate::util::bytes_to_hex;
 
@@ -14,8 +28,20 @@ pub const CREDENTIAL_CACHE_TTL: Duration = Duration::from_secs(30);
 pub const MAX_CACHE_SIZE: usize = 10;
 
 /// Represents the structured path of a credential file
+///
 /// Path structure: {base_dir}/{rp_id}/{cred_id_hex}.{extension}
-/// This allows extracting RP ID and cred ID without loading the file
+///
+/// This structure allows efficient metadata extraction without loading the file:
+/// - RP ID is in the directory name (parent of file)
+/// - Credential ID is decoded from the filename
+/// - Extension determines storage type (bin, gpg, tpm)
+///
+/// # Example
+///
+/// For path `{store_dir}/example.com/a1b2c3d4.gpg`:
+/// - RP ID: "example.com"
+/// - Credential ID: [0xa1, 0xb2, 0xc3, 0xd4]
+/// - Extension: "gpg"
 #[derive(Debug, Clone)]
 pub struct CredentialPathInfo {
     /// Relying Party ID (extracted from directory name)
@@ -192,7 +218,41 @@ pub fn get_credential_path(
 }
 
 /// Build indexes from directory structure - no credential loading needed!
-/// Extracts RP ID and cred ID directly from the file path structure
+///
+/// This function efficiently extracts credential metadata from the file system
+/// without loading or decrypting any credentials. It builds indexes that allow
+/// fast lookups by credential ID, RP ID, and RP ID hash.
+///
+/// # Performance
+///
+/// O(n) complexity where n = total credentials (single directory traversal)
+///
+/// Uses lazy evaluation with path parsing rather than loading file contents.
+///
+/// # Arguments
+///
+/// * `storage_dir` - The root directory containing credential files
+/// * `extension` - Expected file extension (e.g., "bin", "gpg", "tpm")
+///
+/// # Returns
+///
+/// A `CredentialIndexes` structure containing:
+/// - `id`: Map of credential ID to path information
+/// - `rp`: Map of RP ID to list of credential IDs
+/// - `rp_hash`: Map of RP ID hash to list of credential IDs
+///
+/// # Error Handling
+///
+/// Returns default indexes if directory is inaccessible or empty.
+/// Errors are logged at debug level and don't affect the calling code.
+///
+/// # Examples
+///
+/// ```
+/// let indexes = load_credential_paths(&Path::new("/path/to/store"), "gpg")?;
+/// assert_eq!(indexes.id.len(), 10); // 10 credentials indexed
+/// assert!(indexes.rp.contains_key("example.com"));
+/// ```
 pub fn load_credential_paths(
     storage_dir: &Path,
     extension: &str,

cmd/passless/src/storage/local/mod.rs

@@ -15,7 +15,7 @@ use std::fs::{self, File};
 use std::io::{Read, Write};
 use std::path::{Path, PathBuf};
 
-use log::{debug, info};
+use log::{debug, info, warn};
 use zeroize::Zeroizing;
 
 /// Local file system storage adapter
@@ -36,6 +36,13 @@ impl LocalStorageAdapter {
         info!("Using local file system backend");
         info!("Storage path: {}", storage_dir.display());
 
+        // Validate storage directory path
+        if storage_dir.is_absolute() {
+            warn!("Storage path is absolute: {}", storage_dir.display());
+        } else {
+            debug!("Storage path is relative: {}", storage_dir.display());
+        }
+
         // Ensure the storage directory is initialized
         // This will prompt the user via notifications if not initialized
         self::init::ensure_initialized(&storage_dir).map_err(|_| soft_fido2::Error::Other)?;