Skip to content

Commit

Permalink
Fix bug where username from authentication cache could be ignored (#8345
Browse files Browse the repository at this point in the history 
)

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.
  • Loading branch information
@zanieb
zanieb authored Oct 18, 2024
1 parent e26eed1 commit 4b0a4da
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 35 deletions.
79 changes: 49 additions & 30 deletions crates/uv-auth/src/middleware.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -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:?}");
Expand All @@ -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
Expand Down Expand Up @@ -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;
8 changes: 4 additions & 4 deletions crates/uv-auth/src/middleware/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand All @@ -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();
Expand Down
100 changes: 99 additions & 1 deletion crates/uv/tests/it/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit 4b0a4da

Please sign in to comment.