Skip to content

Commit

Permalink
Publish: Warn when keyring has no password
Browse files Browse the repository at this point in the history
  • Loading branch information
konstin committed Nov 27, 2024
1 parent ee84620 commit 183454f
Show file tree
Hide file tree
Showing 5 changed files with 213 additions and 8 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,7 @@ jobs:
- name: "Add password to keyring"
run: |
# `keyrings.alt` contains the plaintext keyring
./uv tool install --with keyrings.alt "keyring<25.4.0" # TODO(konsti): Remove upper bound once fix is released
./uv tool install --with keyrings.alt keyring
echo $UV_TEST_PUBLISH_KEYRING | keyring set https://test.pypi.org/legacy/?astral-test-keyring __token__
env:
UV_TEST_PUBLISH_KEYRING: ${{ secrets.UV_TEST_PUBLISH_KEYRING }}
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-auth/src/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ impl Credentials {
}
}

pub(crate) fn username(&self) -> Option<&str> {
pub fn username(&self) -> Option<&str> {
self.username.as_deref()
}

pub(crate) fn to_username(&self) -> Username {
self.username.clone()
}

pub(crate) fn password(&self) -> Option<&str> {
pub fn password(&self) -> Option<&str> {
self.password.as_deref()
}

Expand Down
2 changes: 1 addition & 1 deletion crates/uv-auth/src/keyring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ impl KeyringProvider {
/// Returns [`None`] if no password was found for the username or if any errors
/// are encountered in the keyring backend.
#[instrument(skip_all, fields(url = % url.to_string(), username))]
pub(crate) async fn fetch(&self, url: &Url, username: &str) -> Option<Credentials> {
pub async fn fetch(&self, url: &Url, username: &str) -> Option<Credentials> {
// Validate the request
debug_assert!(
url.host_str().is_some(),
Expand Down
37 changes: 33 additions & 4 deletions crates/uv/src/commands/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::fmt::Write;
use std::iter;
use std::sync::Arc;
use std::time::Duration;
use tracing::info;
use tracing::{debug, info};
use url::Url;
use uv_cache::Cache;
use uv_client::{AuthIntegration, BaseClientBuilder, Connectivity, RegistryClientBuilder};
Expand All @@ -17,6 +17,7 @@ use uv_distribution_types::{Index, IndexCapabilities, IndexLocations, IndexUrl};
use uv_publish::{
check_trusted_publishing, files_for_publishing, upload, CheckUrlClient, TrustedPublishResult,
};
use uv_warnings::warn_user_once;

pub(crate) async fn publish(
paths: Vec<String>,
Expand Down Expand Up @@ -68,7 +69,7 @@ pub(crate) async fn publish(
.wrap_existing(&upload_client);

// Initialize the registry client.
let check_url_client = if let Some(index_url) = check_url {
let check_url_client = if let Some(index_url) = &check_url {
let index_urls = IndexLocations::new(
vec![Index::from_index_url(index_url.clone())],
Vec::new(),
Expand All @@ -82,7 +83,7 @@ pub(crate) async fn publish(
.keyring(keyring_provider)
.allow_insecure_host(allow_insecure_host.to_vec());
Some(CheckUrlClient {
index_url,
index_url: index_url.clone(),
registry_client_builder,
client: &upload_client,
index_capabilities: IndexCapabilities::default(),
Expand All @@ -103,7 +104,7 @@ pub(crate) async fn publish(
)
.await?;

let (username, password) =
let (username, mut password) =
if let TrustedPublishResult::Configured(password) = &trusted_publishing_token {
(Some("__token__".to_string()), Some(password.to_string()))
} else {
Expand Down Expand Up @@ -151,6 +152,34 @@ pub(crate) async fn publish(
}
}

// If applicable, fetch the password from the keyring eagerly to avoid user confusion about
// missing keyring entries later.
if let Some(keyring_provider) = keyring_provider.to_provider() {
if password.is_none() {
if let Some(username) = &username {
debug!("Fetching password from keyring");
if let Some(keyring_password) = keyring_provider
.fetch(&publish_url, username)
.await
.as_ref()
.and_then(|credentials| credentials.password())
{
password = Some(keyring_password.to_string());
} else {
warn_user_once!(
"Keyring has no password for URL `{publish_url}` and username `{username}`"
);
}
}
} else if check_url.is_none() {
warn_user_once!(
"Using `--keyring-provider` with a password or token and no check url has no effect"
);
} else {
// We may be using the keyring for the simple index.
}
}

for (file, raw_filename, filename) in files {
if let Some(check_url_client) = &check_url_client {
if uv_publish::check_url(check_url_client, &file, &filename).await? {
Expand Down
176 changes: 176 additions & 0 deletions crates/uv/tests/it/publish.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
use crate::common::{uv_snapshot, TestContext};
use assert_cmd::assert::OutputAssertExt;
use assert_fs::fixture::{FileTouch, PathChild};
use std::io::Write;
use std::process::{Command, Stdio};
use std::{env, iter};
use uv_static::EnvVars;

#[test]
Expand Down Expand Up @@ -197,3 +201,175 @@ fn dubious_filenames() {
"###
);
}

/// Check that we (don't) use the keyring and warn for missing keyring behaviors correctly.
#[test]
fn check_keyring_behaviours() {
let context = TestContext::new("3.12");

// No upper bound, since we are recommending `uv tool install keyring` without upper bound to
// users. The keyring package had a bad release in the past, but so it hits us before it hits
// users.
context
.pip_install()
.arg("keyring")
// Contains the plaintext keyring
.arg("keyrings.alt")
.env_remove(EnvVars::UV_EXCLUDE_NEWER)
.assert()
.success();

// Ok: The keyring may be used for the index page.
uv_snapshot!(context.filters(), context.publish()
.arg("-u")
.arg("dummy")
.arg("-p")
.arg("dummy")
.arg("--keyring-provider")
.arg("subprocess")
.arg("--check-url")
.arg("https://test.pypi.org/simple/")
.arg("--publish-url")
.arg("https://test.pypi.org/legacy/?ok")
.arg("../../scripts/links/ok-1.0.0-py3-none-any.whl")
// Avoid `gi.repository.GLib.GError`
.env(
"PYTHON_KEYRING_BACKEND",
"keyrings.alt.file.PlaintextKeyring",
), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv publish` is experimental and may change without warning
Publishing 1 file to https://test.pypi.org/legacy/?ok
error: Failed to query check URL
Caused by: Package `ok` was not found in the registry
"###
);

// Warn: The keyring is unused.
uv_snapshot!(context.filters(), context.publish()
.arg("-u")
.arg("dummy")
.arg("-p")
.arg("dummy")
.arg("--keyring-provider")
.arg("subprocess")
.arg("--publish-url")
.arg("https://test.pypi.org/legacy/?ok")
.arg("../../scripts/links/ok-1.0.0-py3-none-any.whl")
// Avoid `gi.repository.GLib.GError`
.env(
"PYTHON_KEYRING_BACKEND",
"keyrings.alt.file.PlaintextKeyring",
), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv publish` is experimental and may change without warning
Publishing 1 file to https://test.pypi.org/legacy/?ok
warning: Using `--keyring-provider` with a password or token and no check url has no effect
Uploading ok-1.0.0-py3-none-any.whl ([SIZE])
error: Failed to publish `../../scripts/links/ok-1.0.0-py3-none-any.whl` to https://test.pypi.org/legacy/?ok
Caused by: Upload failed with status code 403 Forbidden. Server says: 403 Username/Password authentication is no longer supported. Migrate to API Tokens or Trusted Publishers instead. See https://test.pypi.org/help/#apitoken and https://test.pypi.org/help/#trusted-publishers
"###
);

// Warn: There is no keyring entry for the user dummy.
// https://github.com/astral-sh/uv/issues/7963#issuecomment-2453558043
uv_snapshot!(context.filters(), context.publish()
.arg("-u")
.arg("dummy")
.arg("--keyring-provider")
.arg("subprocess")
.arg("--check-url")
.arg("https://test.pypi.org/simple/")
.arg("--publish-url")
.arg("https://test.pypi.org/legacy/?ok")
.arg("../../scripts/links/ok-1.0.0-py3-none-any.whl")
// Avoid `gi.repository.GLib.GError`
.env(
"PYTHON_KEYRING_BACKEND",
"keyrings.alt.file.PlaintextKeyring",
), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv publish` is experimental and may change without warning
Publishing 1 file to https://test.pypi.org/legacy/?ok
warning: Keyring has no password for URL `https://test.pypi.org/legacy/?ok` and username `dummy`
error: Failed to query check URL
Caused by: Package `ok` was not found in the registry
"###
);

let mut child = Command::new(context.interpreter())
.arg("-m")
.arg("keyring")
.arg("set")
.arg("https://test.pypi.org/legacy/?ok")
.arg("dummy")
// Don't save the dummy to the host's actual keyring!
.env(
"PYTHON_KEYRING_BACKEND",
"keyrings.alt.file.PlaintextKeyring",
)
// Configure keyring file location
.env("XDG_DATA_HOME", context.temp_dir.path())
.env("LOCALAPPDATA", context.temp_dir.path())
.stdin(Stdio::piped())
.spawn()
.unwrap();
let mut stdin = child.stdin.take().expect("Failed to open stdin");
std::thread::spawn(move || {
stdin
.write_all("dummy\n".as_bytes())
.expect("Failed to write to stdin");
stdin.flush().expect("Failed to flush stdin");
});
let status = child.wait().expect("Failed to wait on child");
assert!(status.success(), "Setting password in keyring failed");

// Ok: There is a keyring entry for the user dummy.
// https://github.com/astral-sh/uv/issues/7963#issuecomment-2453558043
let path_with_venv = env::join_paths(
iter::once(context.interpreter().parent().unwrap().to_path_buf())
.chain(env::split_paths(&env::var_os("PATH").unwrap())),
)
.unwrap();
uv_snapshot!(context.filters(), context.publish()
.arg("-u")
.arg("dummy")
.arg("--keyring-provider")
.arg("subprocess")
.arg("--publish-url")
.arg("https://test.pypi.org/legacy/?ok")
.arg("../../scripts/links/ok-1.0.0-py3-none-any.whl")
// Don't save the dummy to the host's actual keyring!
.env(
"PYTHON_KEYRING_BACKEND",
"keyrings.alt.file.PlaintextKeyring",
)
// Configure keyring file location
.env("XDG_DATA_HOME", context.temp_dir.path())
.env("LOCALAPPDATA", context.temp_dir.path())
.env("PATH", path_with_venv), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv publish` is experimental and may change without warning
Publishing 1 file to https://test.pypi.org/legacy/?ok
Uploading ok-1.0.0-py3-none-any.whl ([SIZE])
error: Failed to publish `../../scripts/links/ok-1.0.0-py3-none-any.whl` to https://test.pypi.org/legacy/?ok
Caused by: Upload failed with status code 403 Forbidden. Server says: 403 Username/Password authentication is no longer supported. Migrate to API Tokens or Trusted Publishers instead. See https://test.pypi.org/help/#apitoken and https://test.pypi.org/help/#trusted-publishers
"###
);
}

0 comments on commit 183454f

Please sign in to comment.