Skip to content

Distinguish between authentication failures due to missing vs invalid credentials #12667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jtfmumm wants to merge 2 commits into from
Closed

Distinguish between authentication failures due to missing vs invalid credentials #12667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4368739
Improve authentication failure error messages
jtfmumm Apr 4, 2025
e1d235d
..
jtfmumm Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 45 additions & 48 deletions crates/uv-auth/src/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,11 +250,15 @@ impl Middleware for AuthMiddleware {

// If we don't fail with authorization related codes or
// authentication policy is Never, return the response.
if !matches!(
response.status(),
StatusCode::FORBIDDEN | StatusCode::NOT_FOUND | StatusCode::UNAUTHORIZED
) || matches!(auth_policy, AuthPolicy::Never)
if matches!(auth_policy, AuthPolicy::Never)
|| !matches!(
response.status(),
StatusCode::FORBIDDEN | StatusCode::NOT_FOUND | StatusCode::UNAUTHORIZED
)
{
if credentials.is_none() && response.status() == StatusCode::UNAUTHORIZED {
return Err(missing_credentials_error(&url));
}
return Ok(response);
}

Expand Down Expand Up @@ -323,12 +327,11 @@ impl Middleware for AuthMiddleware {
}

if let Some(response) = response {
Ok(response)
} else {
Err(Error::Middleware(format_err!(
"Missing credentials for {url}"
)))
if !(response.status() == StatusCode::UNAUTHORIZED && credentials.is_none()) {
return Ok(response);
}
}
Err(missing_credentials_error(&url))
}
}

Expand Down Expand Up @@ -527,6 +530,10 @@ fn tracing_url(request: &Request, credentials: Option<&Credentials>) -> String {
}
}

fn missing_credentials_error(url: &str) -> reqwest_middleware::Error {
Error::Middleware(format_err!("Missing credentials for {url}"))
}

#[cfg(test)]
mod tests {
use std::io::Write;
Expand Down Expand Up @@ -575,22 +582,22 @@ mod tests {
.with(AuthMiddleware::new().with_cache(CredentialsCache::new()))
.build();

assert_eq!(
assert!(
client
.get(format!("{}/foo", server.uri()))
.send()
.await?
.status(),
401
.await
.is_err(),
"Requests should require credentials"
);
Comment on lines 578 to 592
Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this one change? There's no policy set here.

Copy link
Contributor Author

@jtfmumm jtfmumm Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's a better way to do it, but the existing approach had been to throw an error for the missing credentials case. There's no policy set here, but the reason for the 401 is because of missing credentials.

Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can throw an error there though, doesn't downstream logic rely on a successful request? (i.e., in #12667 (comment))

Perhaps I misunderstood the intent of this PR?

Copy link
Contributor Author

@jtfmumm jtfmumm Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only tests that broke were cases where we'd want the message to indicate that missing credentials was the cause of failing to find the package. But if we are depending on getting the 401 out of this response downstream, then this approach won't work. In that case, I can try using reqwest extensions to add more context about the 401.

Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I understand the goal now, you want to make the case where no credentials were present distinct from the case where the credentials are wrong. Sorry it took me a while to get there. I think the snapshot changes should be like..

hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to a lack of valid authentication credentials (401 Unauthorized).

to

hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to missing credentials (401 Unauthorized)

and

hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to invalid credentials (401 Unauthorized)

I think changing that error path entirely feels too risky and is hopefully unnecessary.

Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fwiw, I think this would have been clearer if the title was something like "Distinguish between authentication failures due to missing vs invalid credentials")


assert_eq!(
assert!(
client
.get(format!("{}/bar", server.uri()))
.send()
.await?
.status(),
401
.await
.is_err(),
"Requests should require credentials"
);

Ok(())
Expand Down Expand Up @@ -865,9 +872,8 @@ mod tests {
)
.build();

assert_eq!(
client.get(server.uri()).send().await?.status(),
401,
assert!(
client.get(server.uri()).send().await.is_err(),
"Credentials should not be pulled from the netrc file due to host mismatch"
);

Expand Down Expand Up @@ -949,9 +955,8 @@ mod tests {
)
.build();

assert_eq!(
client.get(server.uri()).send().await?.status(),
401,
assert!(
client.get(server.uri()).send().await.is_err(),
"Credentials are not pulled from the keyring without a username"
);

Expand Down Expand Up @@ -1237,14 +1242,12 @@ mod tests {
.build();

// Both servers do not work without a username
assert_eq!(
client.get(server_1.uri()).send().await?.status(),
401,
assert!(
client.get(server_1.uri()).send().await.is_err(),
"Requests should require a username"
);
assert_eq!(
client.get(server_2.uri()).send().await?.status(),
401,
assert!(
client.get(server_2.uri()).send().await.is_err(),
"Requests should require a username"
);

Expand All @@ -1255,9 +1258,8 @@ mod tests {
200,
"Requests with a username should succeed"
);
assert_eq!(
client.get(server_2.uri()).send().await?.status(),
401,
assert!(
client.get(server_2.uri()).send().await.is_err(),
"Credentials should not be re-used for the second server"
);

Expand Down Expand Up @@ -1487,14 +1489,12 @@ mod tests {
.build();

// Both servers do not work without a username
assert_eq!(
client.get(base_url_1.clone()).send().await?.status(),
401,
assert!(
client.get(base_url_1.clone()).send().await.is_err(),
"Requests should require a username"
);
assert_eq!(
client.get(base_url_2.clone()).send().await?.status(),
401,
assert!(
client.get(base_url_2.clone()).send().await.is_err(),
"Requests should require a username"
);

Expand Down Expand Up @@ -1602,14 +1602,12 @@ mod tests {
.build();

// Both servers do not work without a username
assert_eq!(
client.get(base_url_1.clone()).send().await?.status(),
401,
assert!(
client.get(base_url_1.clone()).send().await.is_err(),
"Requests should require a username"
);
assert_eq!(
client.get(base_url_2.clone()).send().await?.status(),
401,
assert!(
client.get(base_url_2.clone()).send().await.is_err(),
"Requests should require a username"
);

Expand Down Expand Up @@ -1795,13 +1793,12 @@ mod tests {
url.set_username(username).unwrap();
url.set_password(Some(password)).unwrap();

assert_eq!(
assert!(
client
.get(format!("{}/foo", server.uri()))
.send()
.await?
.status(),
401,
.await
.is_err(),
"Requests should not be completed if credentials are required"
);

Expand Down
11 changes: 4 additions & 7 deletions crates/uv/tests/it/edit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10358,7 +10358,7 @@ fn add_auth_policy_never_with_url_credentials() -> Result<()> {

----- stderr -----
error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/files/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl.metadata`
Caused by: HTTP status client error (401 Unauthorized) for url (https://pypi-proxy.fly.dev/basic-auth/files/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl.metadata)
Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/files/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl.metadata
"
);

Expand Down Expand Up @@ -10391,15 +10391,12 @@ fn add_auth_policy_never_with_env_var_credentials() -> Result<()> {
.env("UV_INDEX_MY_INDEX_USERNAME", "public")
.env("UV_INDEX_MY_INDEX_PASSWORD", "heron"), @r"
success: false
exit_code: 1
exit_code: 2
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because anyio was not found in the package registry and your project depends on anyio, we can conclude that your project's requirements are unsatisfiable.

hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to a lack of valid authentication credentials (401 Unauthorized).
help: If you want to add the package regardless of the failed resolution, provide the `--frozen` flag to skip locking and syncing.
error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/simple/anyio/`
Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/simple/anyio/
"
);

Expand Down
41 changes: 19 additions & 22 deletions crates/uv/tests/it/lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7517,16 +7517,15 @@ fn lock_index_workspace_member() -> Result<()> {
)?;

// Locking without the necessary credentials should fail.
uv_snapshot!(context.filters(), context.lock(), @r###"
uv_snapshot!(context.filters(), context.lock(), @r"
success: false
exit_code: 1
exit_code: 2
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because iniconfig was not found in the package registry and child depends on iniconfig>=2, we can conclude that child's requirements are unsatisfiable.
And because your workspace requires child, we can conclude that your workspace's requirements are unsatisfiable.
"###);
error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/`
Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/
");
Comment on lines -7520 to +7528
Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of an aside, but... why weren't we showing the hint from https://github.com/astral-sh/uv/pull/12667/files#diff-82edd36151736f44055f699a34c8b19a63ffc4cf3c86bf5fb34d69f8ac88a957L8471 in this case?

Copy link
Contributor Author

@jtfmumm jtfmumm Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I'll look into it


uv_snapshot!(context.filters(), context.lock()
.env("UV_INDEX_MY_INDEX_USERNAME", "public")
Expand Down Expand Up @@ -7870,30 +7869,30 @@ fn lock_redact_https() -> Result<()> {

// Installing from the lockfile should fail without credentials. Omit the root, so that we fail
// when installing `iniconfig`, rather than when building `foo`.
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--index-url").arg("https://pypi-proxy.fly.dev/basic-auth/simple").arg("--no-install-project"), @r###"
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--index-url").arg("https://pypi-proxy.fly.dev/basic-auth/simple").arg("--no-install-project"), @r"
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× Failed to download `iniconfig==2.0.0`
├─▶ Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl`
╰─▶ HTTP status client error (401 Unauthorized) for url (https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl)
╰─▶ Missing credentials for https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl
help: `iniconfig` (v2.0.0) was included because `foo` (v0.1.0) depends on `iniconfig`
"###);
");

// Installing from the lockfile should fail without an index.
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--no-install-project"), @r###"
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--no-install-project"), @r"
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× Failed to download `iniconfig==2.0.0`
├─▶ Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl`
╰─▶ HTTP status client error (401 Unauthorized) for url (https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl)
╰─▶ Missing credentials for https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl
help: `iniconfig` (v2.0.0) was included because `foo` (v0.1.0) depends on `iniconfig`
"###);
");

// Installing from the lockfile should succeed when credentials are included on the command-line.
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--index-url").arg("https://public:heron@pypi-proxy.fly.dev/basic-auth/simple"), @r###"
Expand Down Expand Up @@ -7921,17 +7920,17 @@ fn lock_redact_https() -> Result<()> {
"###);

// Installing without credentials will fail without a cache.
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--reinstall").arg("--no-cache").arg("--no-install-project"), @r###"
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--reinstall").arg("--no-cache").arg("--no-install-project"), @r"
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× Failed to download `iniconfig==2.0.0`
├─▶ Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl`
╰─▶ HTTP status client error (401 Unauthorized) for url (https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl)
╰─▶ Missing credentials for https://pypi-proxy.fly.dev/basic-auth/files/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl
help: `iniconfig` (v2.0.0) was included because `foo` (v0.1.0) depends on `iniconfig`
"###);
");

// Installing with credentials from with `UV_INDEX_URL` should succeed.
uv_snapshot!(context.filters(), context.sync().arg("--frozen").arg("--reinstall").arg("--no-cache").env(EnvVars::UV_INDEX_URL, "https://public:heron@pypi-proxy.fly.dev/basic-auth/simple"), @r###"
Expand Down Expand Up @@ -8459,17 +8458,15 @@ fn lock_env_credentials() -> Result<()> {
)?;

// Without credentials, the resolution should fail.
uv_snapshot!(context.filters(), context.lock(), @r###"
uv_snapshot!(context.filters(), context.lock(), @r"
success: false
exit_code: 1
exit_code: 2
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because iniconfig was not found in the package registry and your project depends on iniconfig, we can conclude that your project's requirements are unsatisfiable.

hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to a lack of valid authentication credentials (401 Unauthorized).
"###);
error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/`
Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/
Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a regression to me?

Copy link
Contributor Author

@jtfmumm jtfmumm Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case where the fetch failed because there were no credentials, which is the reason the package was not found (since we were never authenticated, we were not able to search for it). So this case is intentional. Does it still seem like a regression?

Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? Doesn't this fail eagerly now? What if there was a second index with the package available?

Copy link
Member

@zanieb zanieb Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It should be easy to add a test for that, we should probably have coverage regardless)

Copy link
Contributor Author

@jtfmumm jtfmumm Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that test.

");

// Provide credentials via environment variables.
uv_snapshot!(context.filters(), context.lock()
Expand Down
Loading