diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index c2651c835..f1f6b9d93 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -131,25 +131,7 @@ impl Middleware for AuthMiddleware { // In the middleware, existing credentials are already moved from the URL // to the headers so for display purposes we restore some information - let url = if tracing::enabled!(tracing::Level::DEBUG) { - let mut url = request.url().clone(); - if let Some(username) = credentials - .as_ref() - .and_then(|credentials| credentials.username()) - { - let _ = url.set_username(username); - }; - if credentials - .as_ref() - .and_then(|credentials| credentials.password()) - .is_some() - { - let _ = url.set_password(Some("****")); - }; - url.to_string() - } else { - request.url().to_string() - }; + let url = tracing_url(&request, credentials.as_ref()); trace!("Handling request for {url}"); if let Some(credentials) = credentials { @@ -198,13 +180,20 @@ impl Middleware for AuthMiddleware { // We have no credentials trace!("Request for {url} is unauthenticated, checking cache"); - // Check the cache for a URL match + // Check the cache for a URL match first, this can save us from making a failing request let credentials = self.cache().get_url(request.url(), &Username::none()); if let Some(credentials) = credentials.as_ref() { request = credentials.authenticate(request); + + // If it's fully authenticated, finish the request if credentials.password().is_some() { + trace!("Request for {url} is fully authenticated"); return self.complete_request(None, request, extensions, next).await; } + + // If we just found a username, we'll make the request then look for password elsewhere + // if it fails + trace!("Found username for {url} in cache, attempting request"); } let attempt_has_username = credentials .as_ref() @@ -216,8 +205,12 @@ impl Middleware for AuthMiddleware { trace!("Checking for credentials for {url}"); (request, None) } else { - // Otherwise, attempt an anonymous request - trace!("Attempting unauthenticated request for {url}"); + let url = tracing_url(&request, credentials.as_deref()); + if credentials.is_none() { + trace!("Attempting unauthenticated request for {url}"); + } else { + trace!("Attempting partially authenticated request for {url}"); + } // // Clone the request so we can retry it on authentication failure @@ -247,13 +240,17 @@ impl Middleware for AuthMiddleware { (retry_request, Some(response)) }; - // Check in the cache first - let credentials = self.cache().get_realm( - Realm::from(retry_request.url()), - credentials - .map(|credentials| credentials.to_username()) - .unwrap_or(Username::none()), - ); + // Check if there are credentials in the realm-level cache + let credentials = self + .cache() + .get_realm( + Realm::from(retry_request.url()), + credentials + .as_ref() + .map(|credentials| credentials.to_username()) + .unwrap_or(Username::none()), + ) + .or(credentials); if let Some(credentials) = credentials.as_ref() { if credentials.password().is_some() { trace!("Retrying request for {url} with credentials from cache {credentials:?}"); @@ -265,7 +262,7 @@ impl Middleware for AuthMiddleware { } // Then, fetch from external services. - // Here we use the username from the cache if present. + // Here, we use the username from the cache if present. if let Some(credentials) = self .fetch_credentials(credentials.as_deref(), retry_request.url()) .await @@ -406,5 +403,27 @@ impl AuthMiddleware { } } +fn tracing_url(request: &Request, credentials: Option<&Credentials>) -> String { + if tracing::enabled!(tracing::Level::DEBUG) { + let mut url = request.url().clone(); + if let Some(username) = credentials + .as_ref() + .and_then(|credentials| credentials.username()) + { + let _ = url.set_username(username); + }; + if credentials + .as_ref() + .and_then(|credentials| credentials.password()) + .is_some() + { + let _ = url.set_password(Some("****")); + }; + url.to_string() + } else { + request.url().to_string() + } +} + #[cfg(test)] mod tests; diff --git a/crates/uv-auth/src/middleware/tests.rs b/crates/uv-auth/src/middleware/tests.rs index f4ad7af25..89b0f83b6 100644 --- a/crates/uv-auth/src/middleware/tests.rs +++ b/crates/uv-auth/src/middleware/tests.rs @@ -505,8 +505,8 @@ async fn test_credentials_in_keyring_seed() -> Result<(), Error> { let base_url = Url::parse(&server.uri())?; let cache = CredentialsCache::new(); - // Seed _just_ the username. This cache entry should be ignored and we should - // still find a password via the keyring. + // Seed _just_ the username. We should pull the username from the cache if not present on the + // URL. cache.insert( &base_url, Arc::new(Credentials::new(Some(username.to_string()), None)), @@ -530,8 +530,8 @@ async fn test_credentials_in_keyring_seed() -> Result<(), Error> { assert_eq!( client.get(server.uri()).send().await?.status(), - 401, - "Credentials are not pulled from the keyring without a username" + 200, + "The username is pulled from the cache, and the password from the keyring" ); let mut url = base_url.clone(); diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs index 6aa872d76..cc1b0c35d 100644 --- a/crates/uv/tests/it/lock.rs +++ b/crates/uv/tests/it/lock.rs @@ -8,7 +8,7 @@ use url::Url; use crate::common::{ self, build_vendor_links_url, decode_token, download_to_disk, packse_index_url, uv_snapshot, - TestContext, + venv_bin_path, TestContext, }; use uv_fs::Simplified; use uv_static::EnvVars; @@ -14689,6 +14689,104 @@ fn lock_change_requires_python() -> Result<()> { Ok(()) } +/// Pass credentials for a named index via environment variables. +#[test] +fn lock_keyring_credentials() -> Result<()> { + let keyring_context = TestContext::new("3.12"); + + // Install our keyring plugin + keyring_context + .pip_install() + .arg( + keyring_context + .workspace_root + .join("scripts") + .join("packages") + .join("keyring_test_plugin"), + ) + .assert() + .success(); + + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "foo" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["iniconfig"] + + [build-system] + requires = ["setuptools>=42"] + build-backend = "setuptools.build_meta" + + [tool.uv] + keyring-provider = "subprocess" + + [[tool.uv.index]] + name = "proxy" + url = "https://pypi-proxy.fly.dev/basic-auth/simple" + default = true + "#, + )?; + + // Provide credentials via environment variables. + uv_snapshot!(context.filters(), context.lock() + .env(EnvVars::index_username("PROXY"), "public") + .env(EnvVars::KEYRING_TEST_CREDENTIALS, r#"{"pypi-proxy.fly.dev": {"public": "heron"}}"#) + .env(EnvVars::PATH, venv_bin_path(&keyring_context.venv)), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Request for public@https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/ + Request for public@pypi-proxy.fly.dev + Resolved 2 packages in [TIME] + "###); + + let lock = fs_err::read_to_string(context.temp_dir.join("uv.lock")).unwrap(); + + // The lockfile shout omit the credentials. + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + lock, @r###" + version = 1 + requires-python = ">=3.12" + + [options] + exclude-newer = "2024-03-25T00:00:00Z" + + [[package]] + name = "foo" + version = "0.1.0" + source = { editable = "." } + dependencies = [ + { name = "iniconfig" }, + ] + + [package.metadata] + requires-dist = [{ name = "iniconfig" }] + + [[package]] + name = "iniconfig" + version = "2.0.0" + source = { registry = "https://pypi-proxy.fly.dev/basic-auth/simple" } + sdist = { url = "https://pypi-proxy.fly.dev/basic-auth/files/packages/d7/4b/cbd8e699e64a6f16ca3a8220661b5f83792b3017d0f79807cb8708d33913/iniconfig-2.0.0.tar.gz", hash = "sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3", size = 4646 } + wheels = [ + { url = "https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", hash = "sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374", size = 5892 }, + ] + "### + ); + }); + + Ok(()) +} + #[test] fn lock_multiple_sources() -> Result<()> { let context = TestContext::new("3.12");