diff --git a/client/src/keyring.rs b/client/src/keyring.rs index 5676c900c..f459d471a 100644 --- a/client/src/keyring.rs +++ b/client/src/keyring.rs @@ -19,66 +19,102 @@ use crate::{AsAttributes, Result, Secret, dbus, file}; #[derive(Debug)] pub enum Keyring { #[doc(hidden)] - File(Arc>>), + File(Arc>>, Secret), #[doc(hidden)] DBus(dbus::Collection), } impl Keyring { /// Create a new instance of the Keyring. + /// + /// Auto-detects whether the application is sandboxed and uses the + /// appropriate backend (file backend for sandboxed apps, D-Bus service + /// for host apps). Falls back to D-Bus if the secret portal is not + /// available. pub async fn new() -> Result { if ashpd::is_sandboxed() { - #[cfg(feature = "tracing")] - tracing::debug!("Application is sandboxed, using the file backend"); - - let secret = Secret::sandboxed().await?; - match file::UnlockedKeyring::load( - crate::file::api::Keyring::default_path()?, - secret.clone(), - ) - .await - { - Ok(file) => { - return Ok(Self::File(Arc::new(RwLock::new(Some( - file::Keyring::Unlocked(file), - ))))); - } - // Do nothing in this case, we are supposed to fallback to the host keyring - Err(super::file::Error::Portal(ashpd::Error::PortalNotFound(_))) => { + match Self::sandboxed().await { + Ok(keyring) => Ok(keyring), + // Fallback to host keyring if portal is not available + Err(crate::Error::File(file::Error::Portal(ashpd::Error::PortalNotFound(_)))) => { #[cfg(feature = "tracing")] tracing::debug!( "org.freedesktop.portal.Secrets is not available, falling back to the Secret Service backend" ); + Self::host().await } - Err(e) => { - return Err(crate::Error::File(e)); - } - }; + Err(e) => Err(e), + } } else { - #[cfg(feature = "tracing")] - tracing::debug!( - "Application is not sandboxed, falling back to the Secret Service backend" - ); + Self::host().await } + } + + /// Use the file backend with secret portal (for sandboxed apps). + pub async fn sandboxed() -> Result { + #[cfg(feature = "tracing")] + tracing::debug!("Using file backend (sandboxed mode)"); + + let secret = Secret::sandboxed().await?; + let path = crate::file::api::Keyring::default_path()?; + Self::sandboxed_with_path(&path, secret).await + } + + /// Use the file backend with a custom path. + /// + /// # Arguments + /// * `path` - Path to the keyring file + /// * `secret` - Secret to unlock the keyring (use `Secret::sandboxed()` or + /// `Secret::random()` for tests) + pub async fn sandboxed_with_path( + path: impl AsRef, + secret: Secret, + ) -> Result { + #[cfg(feature = "tracing")] + tracing::debug!("Using file backend with custom path"); + + let file = file::UnlockedKeyring::load(path, secret.clone()).await?; + Ok(Self::File( + Arc::new(RwLock::new(Some(file::Keyring::Unlocked(file)))), + secret, + )) + } + + /// Use the D-Bus Secret Service. + pub async fn host() -> Result { + #[cfg(feature = "tracing")] + tracing::debug!("Using D-Bus Secret Service (host mode)"); + let service = dbus::Service::new().await?; let collection = service.default_collection().await?; Ok(Self::DBus(collection)) } + /// Use the D-Bus Secret Service with a custom connection. + pub async fn host_with_connection(connection: zbus::Connection) -> Result { + #[cfg(feature = "tracing")] + tracing::debug!("Using D-Bus Secret Service with custom connection (test mode)"); + + let service = dbus::Service::new_with_connection(&connection).await?; + let collection = service.default_collection().await?; + Ok(Self::DBus(collection)) + } + /// Unlock the used collection. pub async fn unlock(&self) -> Result<()> { match self { Self::DBus(backend) => backend.unlock(None).await?, - Self::File(keyring) => { + Self::File(keyring, secret) => { let mut kg = keyring.write().await; let kg_value = kg.take(); if let Some(file::Keyring::Locked(locked)) = kg_value { #[cfg(feature = "tracing")] tracing::debug!("Unlocking file backend keyring"); - // Retrieve secret from portal - let secret = Secret::sandboxed().await?; - let unlocked = locked.unlock(secret).await.map_err(crate::Error::File)?; + let unlocked = locked + .unlock(secret.clone()) + .await + .map_err(crate::Error::File)?; *kg = Some(file::Keyring::Unlocked(unlocked)); } else { *kg = kg_value; @@ -92,7 +128,7 @@ impl Keyring { pub async fn lock(&self) -> Result<()> { match self { Self::DBus(backend) => backend.lock(None).await?, - Self::File(keyring) => { + Self::File(keyring, _) => { let mut kg = keyring.write().await; let kg_value = kg.take(); if let Some(file::Keyring::Unlocked(unlocked)) = kg_value { @@ -113,7 +149,7 @@ impl Keyring { pub async fn is_locked(&self) -> Result { match self { Self::DBus(collection) => collection.is_locked().await.map_err(From::from), - Self::File(keyring) => { + Self::File(keyring, _) => { let keyring_guard = keyring.read().await; Ok(keyring_guard .as_ref() @@ -132,7 +168,7 @@ impl Keyring { item.delete(None).await?; } } - Self::File(keyring) => { + Self::File(keyring, _) => { let kg = keyring.read().await; match kg.as_ref() { Some(file::Keyring::Unlocked(backend)) => { @@ -158,7 +194,7 @@ impl Keyring { let items = backend.items().await?; items.into_iter().map(Item::for_dbus).collect::>() } - Self::File(keyring) => { + Self::File(keyring, _) => { let kg = keyring.read().await; match kg.as_ref() { Some(file::Keyring::Unlocked(backend)) => { @@ -192,7 +228,7 @@ impl Keyring { .create_item(label, attributes, secret, replace, None) .await?; } - Self::File(keyring) => { + Self::File(keyring, _) => { let kg = keyring.read().await; match kg.as_ref() { Some(file::Keyring::Unlocked(backend)) => { @@ -218,7 +254,7 @@ impl Keyring { let items = backend.search_items(attributes).await?; items.into_iter().map(Item::for_dbus).collect::>() } - Self::File(keyring) => { + Self::File(keyring, _) => { let kg = keyring.read().await; match kg.as_ref() { Some(file::Keyring::Unlocked(backend)) => { diff --git a/client/tests/keyring.rs b/client/tests/keyring.rs index 4fdd9d404..98d92202d 100644 --- a/client/tests/keyring.rs +++ b/client/tests/keyring.rs @@ -1,37 +1,26 @@ -use std::sync::Arc; - -#[cfg(feature = "async-std")] -use async_lock::RwLock; -use oo7::{Keyring, Secret, dbus, file}; +use oo7::{Keyring, Secret, file}; use tempfile::tempdir; -#[cfg(feature = "tokio")] -use tokio::sync::RwLock; async fn all_backends( - temp_dir: tempfile::TempDir, + temp_dir: &tempfile::TempDir, ) -> (oo7_server::tests::TestServiceSetup, Vec) { let mut backends = Vec::new(); let keyring_path = temp_dir.path().join("test.keyring"); let secret = Secret::from([1, 2].into_iter().cycle().take(64).collect::>()); - let unlocked = file::UnlockedKeyring::load(&keyring_path, secret) + let unlocked = Keyring::sandboxed_with_path(keyring_path, secret) .await .unwrap(); - let keyring = Keyring::File(Arc::new(RwLock::new(Some(file::Keyring::Unlocked( - unlocked, - ))))); - - backends.push(keyring); + backends.push(unlocked); let setup = oo7_server::tests::TestServiceSetup::plain_session(true) .await .unwrap(); - let service = dbus::Service::plain_with_connection(&setup.client_conn) + + let service = Keyring::host_with_connection(setup.client_conn.clone()) .await .unwrap(); - if let Ok(collection) = service.default_collection().await { - backends.push(Keyring::DBus(collection)); - } + backends.push(service); (setup, backends) } @@ -40,7 +29,7 @@ async fn all_backends( #[cfg(feature = "tokio")] async fn create_and_retrieve_items() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -94,7 +83,7 @@ async fn create_and_retrieve_items() { #[cfg(feature = "tokio")] async fn delete_items() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -141,7 +130,7 @@ async fn delete_items() { #[cfg(feature = "tokio")] async fn item_update_label() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -184,7 +173,7 @@ async fn item_update_label() { #[cfg(feature = "tokio")] async fn item_update_attributes() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -241,7 +230,7 @@ async fn item_update_attributes() { #[cfg(feature = "tokio")] async fn item_update_secret() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -278,7 +267,7 @@ async fn item_update_secret() { #[cfg(feature = "tokio")] async fn item_delete() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -327,7 +316,7 @@ async fn item_delete() { #[cfg(feature = "tokio")] async fn item_replace() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -362,7 +351,7 @@ async fn item_replace() { #[cfg(feature = "tokio")] async fn item_timestamps() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -398,7 +387,7 @@ async fn item_timestamps() { #[cfg(feature = "tokio")] async fn item_is_locked() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; for (idx, keyring) in backends.iter().enumerate() { println!("Running test on backend {}", idx); @@ -426,139 +415,130 @@ async fn item_is_locked() { } } -// File-backend specific tests, as the DBus one require prompting #[tokio::test] #[cfg(feature = "tokio")] -async fn file_keyring_lock_unlock() { +async fn keyring_lock_unlock() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; - let keyring = &backends[0]; + let (_setup, backends) = all_backends(&temp_dir).await; - assert!(!keyring.is_locked().await.unwrap()); + for keyring in backends.iter() { + assert!(!keyring.is_locked().await.unwrap()); - keyring.lock().await.unwrap(); - assert!(keyring.is_locked().await.unwrap()); + keyring.lock().await.unwrap(); + assert!(keyring.is_locked().await.unwrap()); - // Test edge case: locking an already locked keyring - keyring.lock().await.unwrap(); - assert!(keyring.is_locked().await.unwrap()); - - let result = keyring - .create_item("test", &[("app", "test")], "secret", false) - .await; - assert!(matches!(result, Err(oo7::Error::File(file::Error::Locked)))); + // Test edge case: locking an already locked keyring + keyring.lock().await.unwrap(); + assert!(keyring.is_locked().await.unwrap()); - if let Keyring::File(kg) = &keyring { - let mut kg_guard = kg.write().await; - if let Some(file::Keyring::Locked(locked)) = kg_guard.take() { - let secret = Secret::from([1, 2].into_iter().cycle().take(64).collect::>()); + keyring.unlock().await.unwrap(); + assert!(!keyring.is_locked().await.unwrap()); - let unlocked = unsafe { locked.unlock_unchecked(secret).await.unwrap() }; - *kg_guard = Some(file::Keyring::Unlocked(unlocked)); - } + // Test edge case: unlocking an already unlocked keyring + keyring.unlock().await.unwrap(); + assert!(!keyring.is_locked().await.unwrap()); } - - assert!(!keyring.is_locked().await.unwrap()); } #[tokio::test] #[cfg(feature = "tokio")] -async fn file_item_lock_unlock() { +async fn item_lock_unlock() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; - let keyring = &backends[0]; + let (_setup, backends) = all_backends(&temp_dir).await; - keyring - .create_item("Test Item", &[("app", "test")], "secret", false) - .await - .unwrap(); + for (idx, keyring) in backends.iter().enumerate() { + println!("Testing item lock/unlock on backend {}", idx); - let items = keyring.items().await.unwrap(); - let item = &items[0]; + keyring + .create_item( + "Test Item", + &[("test-name", "item_lock_unlock")], + "secret", + false, + ) + .await + .unwrap(); - assert!(!item.is_locked().await.unwrap()); - assert_eq!(item.secret().await.unwrap(), Secret::text("secret")); + let items = keyring + .search_items(&[("test-name", "item_lock_unlock")]) + .await + .unwrap(); + let item = &items[0]; - // Test edge case: unlocking an already unlocked item - item.unlock().await.unwrap(); - assert!(!item.is_locked().await.unwrap()); + assert!(!item.is_locked().await.unwrap()); + assert_eq!(item.secret().await.unwrap(), Secret::text("secret")); - item.lock().await.unwrap(); - assert!(item.is_locked().await.unwrap()); + // Test edge case: unlocking an already unlocked item + item.unlock().await.unwrap(); + assert!(!item.is_locked().await.unwrap()); - // Test edge case: locking an already locked item - item.lock().await.unwrap(); - assert!(item.is_locked().await.unwrap()); + item.lock().await.unwrap(); + assert!(item.is_locked().await.unwrap()); - let result = item.secret().await; - assert!(matches!(result, Err(oo7::Error::File(file::Error::Locked)))); + // Test edge case: locking an already locked item + item.lock().await.unwrap(); + assert!(item.is_locked().await.unwrap()); - // Unlock the item - item.unlock().await.unwrap(); - assert!(!item.is_locked().await.unwrap()); - assert_eq!(item.secret().await.unwrap(), Secret::text("secret")); + // Unlock the item + item.unlock().await.unwrap(); + assert!(!item.is_locked().await.unwrap()); + assert_eq!(item.secret().await.unwrap(), Secret::text("secret")); + + // Cleanup + keyring + .delete(&[("test-name", "item_lock_unlock")]) + .await + .unwrap(); + } } #[tokio::test] #[cfg(feature = "tokio")] -async fn file_locked_item_operations_fail() { +async fn locked_item_operations_fail() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; - let keyring = &backends[0]; + let (_setup, backends) = all_backends(&temp_dir).await; - keyring - .create_item("Test", &[("app", "test")], "secret", false) - .await - .unwrap(); + for (idx, keyring) in backends.iter().enumerate() { + println!("Testing locked item operations on backend {}", idx); - let items = keyring.items().await.unwrap(); - let item = &items[0]; + keyring + .create_item("Test", &[("test-name", "locked_item_ops")], "secret", false) + .await + .unwrap(); - item.lock().await.unwrap(); + let items = keyring + .search_items(&[("test-name", "locked_item_ops")]) + .await + .unwrap(); + let item = &items[0]; - assert!(matches!( - item.label().await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.attributes().await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.secret().await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.set_label("new").await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.set_attributes(&[("app", "test")]).await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.set_secret("new").await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.delete().await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.created().await, - Err(oo7::Error::File(file::Error::Locked)) - )); - assert!(matches!( - item.modified().await, - Err(oo7::Error::File(file::Error::Locked)) - )); + item.lock().await.unwrap(); + + // All operations should fail on locked items + assert!(item.label().await.is_err()); + assert!(item.attributes().await.is_err()); + assert!(item.secret().await.is_err()); + assert!(item.set_label("new").await.is_err()); + assert!(item.set_attributes(&[("app", "test")]).await.is_err()); + assert!(item.set_secret("new").await.is_err()); + // Note: delete() prompts for unlock on D-Bus backend, skip testing + assert!(item.created().await.is_err()); + assert!(item.modified().await.is_err()); + + // Cleanup: unlock and delete + item.unlock().await.unwrap(); + keyring + .delete(&[("test-name", "locked_item_ops")]) + .await + .unwrap(); + } } #[tokio::test] #[cfg(feature = "tokio")] async fn file_locked_keyring_operations_fail() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; + let (_setup, backends) = all_backends(&temp_dir).await; let keyring = &backends[0]; keyring @@ -610,21 +590,25 @@ async fn file_locked_keyring_operations_fail() { #[tokio::test] #[cfg(feature = "tokio")] -async fn file_item_lock_with_locked_keyring_fails() { +async fn item_lock_with_locked_keyring_fails() { let temp_dir = tempdir().unwrap(); - let (_setup, backends) = all_backends(temp_dir).await; - let keyring = &backends[0]; + let (_setup, backends) = all_backends(&temp_dir).await; - keyring - .create_item("Test", &[("app", "test")], "secret", false) - .await - .unwrap(); + for keyring in backends.iter() { + keyring + .create_item("Test", &[("app", "test")], "secret", false) + .await + .unwrap(); - let items = keyring.items().await.unwrap(); - let item = &items[0]; + let items = keyring.items().await.unwrap(); + let item = &items[0]; - keyring.lock().await.unwrap(); + keyring.lock().await.unwrap(); + + let result = item.lock().await; + assert!(result.is_err()); - let result = item.lock().await; - assert!(matches!(result, Err(oo7::Error::File(file::Error::Locked)))); + keyring.unlock().await.unwrap(); + keyring.delete(&[("app", "test")]).await.unwrap(); + } } diff --git a/coverage.sh b/coverage.sh index 3dc731837..50e5a000a 100755 --- a/coverage.sh +++ b/coverage.sh @@ -13,7 +13,7 @@ for crypto in "${CRYPTO_FEATURES[@]}"; do cargo tarpaulin \ --package oo7 \ --no-default-features \ - --features "tracing,tokio,${crypto}" \ + --features "tracing,tokio,schema,${crypto}" \ --ignore-panics \ --out Lcov \ --output-dir coverage-raw diff --git a/server/src/pam_listener/mod.rs b/server/src/pam_listener/mod.rs index 59529e9e9..ba68dc6a6 100644 --- a/server/src/pam_listener/mod.rs +++ b/server/src/pam_listener/mod.rs @@ -34,6 +34,12 @@ struct PamMessage { new_secret: Vec, } +#[derive(Serialize, Deserialize, Type)] +struct PamResponse { + success: bool, + error_message: String, +} + impl PamMessage { fn from_bytes(bytes: &[u8]) -> Result { let ctxt = Context::new_dbus(zvariant::LE, 0); @@ -42,6 +48,40 @@ impl PamMessage { } } +impl PamResponse { + fn success() -> Self { + Self { + success: true, + error_message: String::new(), + } + } + + fn error(message: String) -> Self { + Self { + success: false, + error_message: message, + } + } + + #[cfg(test)] + fn from_bytes(bytes: &[u8]) -> Result { + let ctxt = Context::new_dbus(zvariant::LE, 0); + let data = Data::new(bytes, ctxt); + data.deserialize().map(|(msg, _)| msg) + } + + fn to_bytes(&self) -> Result, zvariant::Error> { + let ctxt = Context::new_dbus(zvariant::LE, 0); + let encoded = zvariant::to_bytes(ctxt, self)?; + let message_bytes = encoded.to_vec(); + + // Prepend length prefix (4 bytes, little-endian) + let mut result = (message_bytes.len() as u32).to_le_bytes().to_vec(); + result.extend_from_slice(&message_bytes); + Ok(result) + } +} + /// PAM listener that receives authentication secrets from the PAM module #[derive(Clone)] pub struct PamListener { @@ -149,7 +189,7 @@ impl PamListener { let message = PamMessage::from_bytes(&message_bytes) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string()))?; - match message.operation { + let response = match message.operation { PamOperation::Unlock => { tracing::info!("Received unlock request for user: {}", message.username); tracing::debug!( @@ -169,6 +209,7 @@ impl PamListener { "Successfully unlocked collections for user: {}", message.username ); + PamResponse::success() } Err(e) => { tracing::warn!( @@ -176,6 +217,7 @@ impl PamListener { message.username, e ); + PamResponse::error(e.to_string()) } } } @@ -208,6 +250,7 @@ impl PamListener { .write() .await .insert(message.username.clone(), new_secret); + PamResponse::success() } Err(e) => { tracing::error!( @@ -215,9 +258,17 @@ impl PamListener { message.username, e ); + PamResponse::error(e.to_string()) } } } + }; + + // Send response back to client + use tokio::io::AsyncWriteExt; + if let Ok(response_bytes) = response.to_bytes() { + let _ = stream.write_all(&response_bytes).await; + let _ = stream.flush().await; } Ok(()) diff --git a/server/src/pam_listener/tests.rs b/server/src/pam_listener/tests.rs index 8bcdc4c0c..b8f77b3a3 100644 --- a/server/src/pam_listener/tests.rs +++ b/server/src/pam_listener/tests.rs @@ -30,11 +30,27 @@ async fn send_pam_message( socket_path: &std::path::Path, message_bytes: &[u8], ) -> Result<(), Box> { - use tokio::io::AsyncWriteExt; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; let mut stream = tokio::net::UnixStream::connect(socket_path).await?; stream.write_all(message_bytes).await?; stream.flush().await?; + + // Read response + let mut length_bytes = [0u8; 4]; + stream.read_exact(&mut length_bytes).await?; + let response_length = u32::from_le_bytes(length_bytes) as usize; + + let mut response_bytes = vec![0u8; response_length]; + stream.read_exact(&mut response_bytes).await?; + + let response = PamResponse::from_bytes(&response_bytes)?; + + // Check if response indicates success or error + if !response.success { + return Err(format!("PAM operation failed: {}", response.error_message).into()); + } + Ok(()) } @@ -79,15 +95,7 @@ async fn pam_migrates_v0_keyrings() -> Result<(), Box> { let message = create_pam_message(PamOperation::Unlock, "testuser", &[], v0_secret.as_bytes()); send_pam_message(&socket_path, &message).await?; - for i in 0..10 { - tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; - let pending = setup.server.pending_migrations.lock().await.len(); - eprintln!("Attempt {}: pending_migrations = {}", i + 1, pending); - if pending == 0 { - break; - } - } - + // Response received means operation completed let pending = setup.server.pending_migrations.lock().await; assert_eq!( pending.len(), @@ -125,6 +133,10 @@ async fn pam_migrates_v0_keyrings() -> Result<(), Box> { async fn pam_unlocks_locked_collections() -> Result<(), Box> { let temp_dir = tempfile::tempdir()?; + // Create the v1 directory structure + let v1_dir = temp_dir.path().join("keyrings/v1"); + tokio::fs::create_dir_all(&v1_dir).await?; + // Create a v1 keyring with a known password let secret = Secret::from("my-secure-password"); let keyring = UnlockedKeyring::open_at(temp_dir.path(), "work", secret.clone()).await?; @@ -173,8 +185,7 @@ async fn pam_unlocks_locked_collections() -> Result<(), Box Result<(), Box> { ); send_pam_message(&socket_path, &message).await?; - tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; - let collections = setup.server.collections.lock().await; let mut work_collection = None; for collection in collections.values() { @@ -316,5 +325,18 @@ async fn message_serialization() -> Result<(), Box> { assert_eq!(decoded.old_secret, b"old-pass"); assert_eq!(decoded.new_secret, b"new-pass"); + // Test PamResponse serialization + let success_response = PamResponse::success(); + let encoded = success_response.to_bytes()?; + let decoded = PamResponse::from_bytes(&encoded[4..])?; // Skip length prefix + assert!(decoded.success); + assert!(decoded.error_message.is_empty()); + + let error_response = PamResponse::error("Something went wrong".to_string()); + let encoded = error_response.to_bytes()?; + let decoded = PamResponse::from_bytes(&encoded[4..])?; // Skip length prefix + assert!(!decoded.success); + assert_eq!(decoded.error_message, "Something went wrong"); + Ok(()) } diff --git a/server/src/service/mod.rs b/server/src/service/mod.rs index 65098e4bf..63d411e21 100644 --- a/server/src/service/mod.rs +++ b/server/src/service/mod.rs @@ -1125,24 +1125,31 @@ impl Service { } break; } else if let Some(item) = collection.item_from_path(object).await { - if locked == item.is_locked().await { + // If collection is locked, can't perform any item lock/unlock operations + if collection_locked { + // Unlocking an item when collection is locked requires unlocking collection + if !locked { + with_prompt.push(object.clone()); + } else { + // Can't lock an item when collection is locked + return Err(ServiceError::IsLocked(format!( + "Cannot lock item {} when collection is locked", + object + ))); + } + } else if locked == item.is_locked().await { tracing::debug!( "Item: {} is already {}.", object, if locked { "locked" } else { "unlocked" } ); without_prompt.push(object.clone()); - // If the collection is unlocked, we can lock/unlock the - // item directly - } else if !collection_locked { + } else { + // Collection is unlocked, we can lock/unlock the item directly let keyring = collection.keyring.read().await; item.set_locked(locked, keyring.as_ref().unwrap().as_unlocked()) .await?; without_prompt.push(object.clone()); - } else { - // Collection is locked, unlocking the item requires unlocking the - // collection - with_prompt.push(object.clone()); } break; }