diff --git a/Cargo.lock b/Cargo.lock index 07396ff49..5107ca912 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5095,6 +5095,7 @@ dependencies = [ "toml", "tracing", "url", + "uv-fs", "uv-keyring", "uv-once-map", "uv-redacted", diff --git a/crates/uv-auth/Cargo.toml b/crates/uv-auth/Cargo.toml index cf28700f2..690b3cea0 100644 --- a/crates/uv-auth/Cargo.toml +++ b/crates/uv-auth/Cargo.toml @@ -10,6 +10,7 @@ doctest = false workspace = true [dependencies] +uv-fs = { workspace = true } uv-keyring = { workspace = true, features = ["apple-native", "secret-service", "windows-native"] } uv-once-map = { workspace = true } uv-redacted = { workspace = true } diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 3a1b288aa..8c877bcda 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -71,8 +71,8 @@ impl Default for TextStoreMode { }) .ok()?; - match TextCredentialStore::from_file(&path) { - Ok(store) => { + match TextCredentialStore::read(&path) { + Ok((store, _lock)) => { debug!("Loaded credential file {}", path.display()); Some(store) } diff --git a/crates/uv-auth/src/store.rs b/crates/uv-auth/src/store.rs index 7a981586c..ef58f9418 100644 --- a/crates/uv-auth/src/store.rs +++ b/crates/uv-auth/src/store.rs @@ -6,6 +6,7 @@ use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use thiserror::Error; use url::Url; +use uv_fs::{LockedFile, with_added_extension}; use uv_redacted::DisplaySafeUrl; use uv_state::{StateBucket, StateStore}; @@ -199,8 +200,17 @@ impl TextCredentialStore { Ok(dir.join("credentials.toml")) } + /// Acquire a lock on the credentials file at the given path. + pub fn lock(path: &Path) -> Result { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + let lock = with_added_extension(path, ".lock"); + Ok(LockedFile::acquire_blocking(lock, "credentials store")?) + } + /// Read credentials from a file. - pub fn from_file>(path: P) -> Result { + fn from_file>(path: P) -> Result { let content = fs::read_to_string(path)?; let credentials: TomlCredentials = toml::from_str(&content)?; @@ -213,8 +223,26 @@ impl TextCredentialStore { Ok(Self { credentials }) } + /// Read credentials from a file. + /// + /// Returns [`TextCredentialStore`] and a [`LockedFile`] to hold if mutating the store. + /// + /// If the store will not be written to following the read, the lock can be dropped. + pub fn read>(path: P) -> Result<(Self, LockedFile), TomlCredentialError> { + let lock = Self::lock(path.as_ref())?; + let store = Self::from_file(path)?; + Ok((store, lock)) + } + /// Persist credentials to a file. - pub fn write>(self, path: P) -> Result<(), TomlCredentialError> { + /// + /// Requires a [`LockedFile`] from [`TextCredentialStore::lock`] or + /// [`TextCredentialStore::read`] to ensure exclusive access. + pub fn write>( + self, + path: P, + _lock: LockedFile, + ) -> Result<(), TomlCredentialError> { let credentials = self .credentials .into_iter() @@ -390,7 +418,12 @@ password = "pass2" // Test saving let temp_output = NamedTempFile::new().unwrap(); - store.write(temp_output.path()).unwrap(); + store + .write( + temp_output.path(), + TextCredentialStore::lock(temp_file.path()).unwrap(), + ) + .unwrap(); let content = fs::read_to_string(temp_output.path()).unwrap(); assert!(content.contains("example.com")); diff --git a/crates/uv-fs/src/lib.rs b/crates/uv-fs/src/lib.rs index 17f52dcf5..e8cfc4d8a 100644 --- a/crates/uv-fs/src/lib.rs +++ b/crates/uv-fs/src/lib.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::fmt::Display; use std::io; use std::path::{Path, PathBuf}; @@ -12,6 +13,24 @@ pub mod cachedir; mod path; pub mod which; +/// Append an extension to a [`PathBuf`]. +/// +/// Unlike [`Path::with_extension`], this function does not replace an existing extension. +/// +/// If there is no file name, the path is returned unchanged. +/// +/// This mimics the behavior of the unstable [`Path::with_added_extension`] method. +pub fn with_added_extension<'a>(path: &'a Path, extension: &str) -> Cow<'a, Path> { + let Some(name) = path.file_name() else { + // If there is no file name, we cannot add an extension. + return Cow::Borrowed(path); + }; + let mut name = name.to_os_string(); + name.push("."); + name.push(extension.trim_start_matches('.')); + Cow::Owned(path.with_file_name(name)) +} + /// Attempt to check if the two paths refer to the same file. /// /// Returns `Some(true)` if the files are missing, but would be the same if they existed. @@ -812,3 +831,45 @@ pub fn copy_dir_all(src: impl AsRef, dst: impl AsRef) -> std::io::Re } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + #[test] + fn test_with_added_extension() { + // Test with simple package name (no dots) + let path = PathBuf::from("python"); + let result = with_added_extension(&path, "exe"); + assert_eq!(result, PathBuf::from("python.exe")); + + // Test with package name containing single dot + let path = PathBuf::from("awslabs.cdk-mcp-server"); + let result = with_added_extension(&path, "exe"); + assert_eq!(result, PathBuf::from("awslabs.cdk-mcp-server.exe")); + + // Test with package name containing multiple dots + let path = PathBuf::from("org.example.tool"); + let result = with_added_extension(&path, "exe"); + assert_eq!(result, PathBuf::from("org.example.tool.exe")); + + // Test with different extensions + let path = PathBuf::from("script"); + let result = with_added_extension(&path, "ps1"); + assert_eq!(result, PathBuf::from("script.ps1")); + + // Test with path that has directory components + let path = PathBuf::from("some/path/to/awslabs.cdk-mcp-server"); + let result = with_added_extension(&path, "exe"); + assert_eq!( + result, + PathBuf::from("some/path/to/awslabs.cdk-mcp-server.exe") + ); + + // Test with empty path (edge case) + let path = PathBuf::new(); + let result = with_added_extension(&path, "exe"); + assert_eq!(result, path); // Should return unchanged + } +} diff --git a/crates/uv-shell/src/runnable.rs b/crates/uv-shell/src/runnable.rs index f36fd8a12..a36e99ae4 100644 --- a/crates/uv-shell/src/runnable.rs +++ b/crates/uv-shell/src/runnable.rs @@ -2,27 +2,10 @@ use std::env::consts::EXE_EXTENSION; use std::ffi::OsStr; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::Command; -/// Append an extension to a [`PathBuf`]. -/// -/// Unlike [`Path::with_extension`], this function does not replace an existing extension. -/// -/// If there is no file name, the path is returned unchanged. -/// -/// This mimics the behavior of the unstable [`Path::with_added_extension`] method. -fn add_extension_to_path(mut path: PathBuf, extension: &str) -> PathBuf { - let Some(name) = path.file_name() else { - // If there is no file name, we cannot add an extension. - return path; - }; - let mut name = name.to_os_string(); - name.push("."); - name.push(extension.trim_start_matches('.')); - path.set_file_name(name); - path -} +use uv_fs::with_added_extension; #[derive(Debug)] pub enum WindowsRunnable { @@ -109,7 +92,7 @@ impl WindowsRunnable { .map(|script_type| { ( script_type, - add_extension_to_path(script_path.clone(), script_type.to_extension()), + with_added_extension(&script_path, script_type.to_extension()), ) }) .find(|(_, script_path)| script_path.is_file()) @@ -120,50 +103,16 @@ impl WindowsRunnable { #[cfg(test)] mod tests { - use super::*; - use std::path::PathBuf; + #[cfg(target_os = "windows")] + use super::WindowsRunnable; #[cfg(target_os = "windows")] use fs_err as fs; #[cfg(target_os = "windows")] + use std::ffi::OsStr; + #[cfg(target_os = "windows")] use std::io; - #[test] - fn test_add_extension_to_path() { - // Test with simple package name (no dots) - let path = PathBuf::from("python"); - let result = add_extension_to_path(path, "exe"); - assert_eq!(result, PathBuf::from("python.exe")); - - // Test with package name containing single dot - let path = PathBuf::from("awslabs.cdk-mcp-server"); - let result = add_extension_to_path(path, "exe"); - assert_eq!(result, PathBuf::from("awslabs.cdk-mcp-server.exe")); - - // Test with package name containing multiple dots - let path = PathBuf::from("org.example.tool"); - let result = add_extension_to_path(path, "exe"); - assert_eq!(result, PathBuf::from("org.example.tool.exe")); - - // Test with different extensions - let path = PathBuf::from("script"); - let result = add_extension_to_path(path, "ps1"); - assert_eq!(result, PathBuf::from("script.ps1")); - - // Test with path that has directory components - let path = PathBuf::from("some/path/to/awslabs.cdk-mcp-server"); - let result = add_extension_to_path(path, "exe"); - assert_eq!( - result, - PathBuf::from("some/path/to/awslabs.cdk-mcp-server.exe") - ); - - // Test with empty path (edge case) - let path = PathBuf::new(); - let result = add_extension_to_path(path.clone(), "exe"); - assert_eq!(result, path); // Should return unchanged - } - /// Helper function to create a temporary directory with test files #[cfg(target_os = "windows")] fn create_test_environment() -> io::Result { diff --git a/crates/uv/src/commands/auth/login.rs b/crates/uv/src/commands/auth/login.rs index 43e6664cd..0c740e5d6 100644 --- a/crates/uv/src/commands/auth/login.rs +++ b/crates/uv/src/commands/auth/login.rs @@ -115,9 +115,9 @@ pub(crate) async fn login( AuthBackend::Keyring(provider) => { provider.store(&url, &credentials).await?; } - AuthBackend::TextStore(mut text_store) => { - text_store.insert(service.clone(), credentials); - text_store.write(TextCredentialStore::default_file()?)?; + AuthBackend::TextStore(mut store, _lock) => { + store.insert(service.clone(), credentials); + store.write(TextCredentialStore::default_file()?, _lock)?; } } diff --git a/crates/uv/src/commands/auth/logout.rs b/crates/uv/src/commands/auth/logout.rs index 9776bb257..e6c2ac7a6 100644 --- a/crates/uv/src/commands/auth/logout.rs +++ b/crates/uv/src/commands/auth/logout.rs @@ -61,12 +61,12 @@ pub(crate) async fn logout( .await .with_context(|| format!("Unable to remove credentials for {display_url}"))?; } - AuthBackend::TextStore(mut text_store) => { - if text_store.remove(&service).is_none() { + AuthBackend::TextStore(mut store, _lock) => { + if store.remove(&service).is_none() { bail!("No matching entry found for {display_url}"); } - text_store - .write(TextCredentialStore::default_file()?) + store + .write(TextCredentialStore::default_file()?, _lock) .with_context(|| "Failed to persist changes to credentials after removal")?; } } diff --git a/crates/uv/src/commands/auth/mod.rs b/crates/uv/src/commands/auth/mod.rs index fefdc87c4..a781ef8b9 100644 --- a/crates/uv/src/commands/auth/mod.rs +++ b/crates/uv/src/commands/auth/mod.rs @@ -1,5 +1,6 @@ use uv_auth::{KeyringProvider, TextCredentialStore, TomlCredentialError}; use uv_configuration::KeyringProviderType; +use uv_fs::LockedFile; use uv_preview::Preview; pub(crate) mod dir; @@ -10,7 +11,7 @@ pub(crate) mod token; /// The storage backend to use in `uv auth` commands. enum AuthBackend { Keyring(KeyringProvider), - TextStore(TextCredentialStore), + TextStore(TextCredentialStore, LockedFile), } impl AuthBackend { @@ -27,10 +28,14 @@ impl AuthBackend { } // Otherwise, we'll use the plain text credential store - match TextCredentialStore::from_file(TextCredentialStore::default_file()?) { - Ok(store) => Ok(Self::TextStore(store)), + let path = TextCredentialStore::default_file()?; + match TextCredentialStore::read(&path) { + Ok((store, lock)) => Ok(Self::TextStore(store, lock)), Err(TomlCredentialError::Io(err)) if err.kind() == std::io::ErrorKind::NotFound => { - Ok(Self::TextStore(TextCredentialStore::default())) + Ok(Self::TextStore( + TextCredentialStore::default(), + TextCredentialStore::lock(&path)?, + )) } Err(err) => Err(err), } diff --git a/crates/uv/src/commands/auth/token.rs b/crates/uv/src/commands/auth/token.rs index 92e0a9215..c47a8eea8 100644 --- a/crates/uv/src/commands/auth/token.rs +++ b/crates/uv/src/commands/auth/token.rs @@ -47,7 +47,7 @@ pub(crate) async fn token( .fetch(url, Some(&username)) .await .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?, - AuthBackend::TextStore(text_store) => text_store + AuthBackend::TextStore(store, _lock) => store .get_credentials(url) .cloned() .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?,