mirror of
https://github.com/astral-sh/uv.git
synced 2025-08-04 10:58:28 +00:00
Fix bug where username from authentication cache could be ignored (#8345)
Basically, if username-only authentication came from the _cache_ instead of being present on the _request URL_ to start, we'd end up ignoring it during password lookups which breaks keyring. Includes some cosmetic changes to the logging and commentary in the middleware, because I was confused when reading the code and logs.
This commit is contained in:
Zanie Blue
GitHub
parent
e26eed10e4
commit
4b0a4dadb7
3 changed files with 152 additions and 35 deletions
|
|
@ -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}");
|
||||
}
|
||||
|
||||
// <https://github.com/TrueLayer/reqwest-middleware/blob/abdf1844c37092d323683c2396b7eefda1418d3c/reqwest-retry/src/middleware.rs#L141-L149>
|
||||
// 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;
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue