From c4fd34f0631d42d77e26667a496cc0651ba00328 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 4 Apr 2025 19:07:51 -0400 Subject: [PATCH] Use `Credentials` abstraction in `uv-publish` (#12682) ## Summary I noticed that we aren't using these here -- we have a separate username and password situation. --- Cargo.lock | 1 + crates/uv-auth/src/credentials.rs | 8 +- crates/uv-publish/Cargo.toml | 1 + crates/uv-publish/src/lib.rs | 177 +++++++++++++++--------------- crates/uv/src/commands/publish.rs | 66 +++++------ 5 files changed, 131 insertions(+), 122 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 70054639b..f32236a91 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5462,6 +5462,7 @@ dependencies = [ "tokio-util", "tracing", "url", + "uv-auth", "uv-cache", "uv-client", "uv-configuration", diff --git a/crates/uv-auth/src/credentials.rs b/crates/uv-auth/src/credentials.rs index 88f49425c..a015e5b3d 100644 --- a/crates/uv-auth/src/credentials.rs +++ b/crates/uv-auth/src/credentials.rs @@ -70,7 +70,7 @@ impl From> for Username { impl Credentials { /// Create a set of HTTP Basic Authentication credentials. #[allow(dead_code)] - pub(crate) fn basic(username: Option, password: Option) -> Self { + pub fn basic(username: Option, password: Option) -> Self { Self::Basic { username: Username::new(username), password, @@ -79,7 +79,7 @@ impl Credentials { /// Create a set of Bearer Authentication credentials. #[allow(dead_code)] - pub(crate) fn bearer(token: Vec) -> Self { + pub fn bearer(token: Vec) -> Self { Self::Bearer { token } } @@ -245,7 +245,7 @@ impl Credentials { /// Create an HTTP Basic Authentication header for the credentials. /// /// Panics if the username or password cannot be base64 encoded. - pub(crate) fn to_header_value(&self) -> HeaderValue { + pub fn to_header_value(&self) -> HeaderValue { match self { Self::Basic { .. } => { // See: @@ -291,7 +291,7 @@ impl Credentials { /// /// Any existing credentials will be overridden. #[must_use] - pub(crate) fn authenticate(&self, mut request: Request) -> Request { + pub fn authenticate(&self, mut request: Request) -> Request { request .headers_mut() .insert(reqwest::header::AUTHORIZATION, Self::to_header_value(self)); diff --git a/crates/uv-publish/Cargo.toml b/crates/uv-publish/Cargo.toml index be2a627b6..254d83d03 100644 --- a/crates/uv-publish/Cargo.toml +++ b/crates/uv-publish/Cargo.toml @@ -13,6 +13,7 @@ license.workspace = true doctest = false [dependencies] +uv-auth = { workspace = true } uv-cache = { workspace = true } uv-client = { workspace = true } uv-configuration = { workspace = true } diff --git a/crates/uv-publish/src/lib.rs b/crates/uv-publish/src/lib.rs index ba8a6ba8d..ee54cfcb1 100644 --- a/crates/uv-publish/src/lib.rs +++ b/crates/uv-publish/src/lib.rs @@ -1,8 +1,10 @@ mod trusted_publishing; -use crate::trusted_publishing::TrustedPublishingError; -use base64::prelude::BASE64_STANDARD; -use base64::Engine; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::{Duration, SystemTime}; +use std::{env, fmt, io}; + use fs_err::tokio::File; use futures::TryStreamExt; use glob::{glob, GlobError, PatternError}; @@ -15,10 +17,6 @@ use reqwest_retry::policies::ExponentialBackoff; use reqwest_retry::{RetryPolicy, Retryable, RetryableStrategy}; use rustc_hash::FxHashSet; use serde::Deserialize; -use std::path::{Path, PathBuf}; -use std::sync::Arc; -use std::time::{Duration, SystemTime}; -use std::{env, fmt, io}; use thiserror::Error; use tokio::io::{AsyncReadExt, BufReader}; use tokio::sync::Semaphore; @@ -26,6 +24,8 @@ use tokio_util::io::ReaderStream; use tracing::{debug, enabled, trace, warn, Level}; use trusted_publishing::TrustedPublishingToken; use url::Url; + +use uv_auth::Credentials; use uv_cache::{Cache, Refresh}; use uv_client::{ BaseClient, MetadataFormat, OwnedArchive, RegistryClientBuilder, UvRetryableStrategy, @@ -41,6 +41,8 @@ use uv_pypi_types::{HashAlgorithm, HashDigest, Metadata23, MetadataError}; use uv_static::EnvVars; use uv_warnings::{warn_user, warn_user_once}; +use crate::trusted_publishing::TrustedPublishingError; + #[derive(Error, Debug)] pub enum PublishError { #[error("The publish path is not a valid glob pattern: `{0}`")] @@ -370,8 +372,7 @@ pub async fn upload( filename: &DistFilename, registry: &Url, client: &BaseClient, - username: Option<&str>, - password: Option<&str>, + credentials: &Credentials, check_url_client: Option<&CheckUrlClient<'_>>, download_concurrency: &Semaphore, reporter: Arc, @@ -391,8 +392,7 @@ pub async fn upload( filename, registry, client, - username, - password, + credentials, &form_metadata, reporter.clone(), ) @@ -744,8 +744,7 @@ async fn build_request( filename: &DistFilename, registry: &Url, client: &BaseClient, - username: Option<&str>, - password: Option<&str>, + credentials: &Credentials, form_metadata: &[(&'static str, String)], reporter: Arc, ) -> Result<(RequestBuilder, usize), PublishPrepareError> { @@ -767,17 +766,15 @@ async fn build_request( let part = Part::stream_with_length(file_reader, file_size).file_name(raw_filename.to_string()); form = form.part("content", part); - let url = if let Some(username) = username { - if password.is_none() { - // Attach the username to the URL so the authentication middleware can find the matching - // password. - let mut url = registry.clone(); - let _ = url.set_username(username); - url - } else { - // We set the authorization header below. - registry.clone() - } + // If we have a username but no password, attach the username to the URL so the authentication + // middleware can find the matching password. + let url = if let Some(username) = credentials + .username() + .filter(|_| credentials.password().is_none()) + { + let mut url = registry.clone(); + let _ = url.set_username(username); + url } else { registry.clone() }; @@ -793,11 +790,20 @@ async fn build_request( reqwest::header::ACCEPT, "application/json;q=0.9, text/plain;q=0.8, text/html;q=0.7", ); - if let (Some(username), Some(password)) = (username, password) { - debug!("Using username/password basic auth"); - let credentials = BASE64_STANDARD.encode(format!("{username}:{password}")); - request = request.header(AUTHORIZATION, format!("Basic {credentials}")); + + match credentials { + Credentials::Basic { password, .. } => { + if password.is_some() { + debug!("Using HTTP Basic authentication"); + request = request.header(AUTHORIZATION, credentials.to_header_value()); + } + } + Credentials::Bearer { .. } => { + debug!("Using Bearer token authentication"); + request = request.header(AUTHORIZATION, credentials.to_header_value()); + } } + Ok((request, idx)) } @@ -875,6 +881,7 @@ mod tests { use std::path::PathBuf; use std::sync::Arc; use url::Url; + use uv_auth::Credentials; use uv_client::BaseClientBuilder; use uv_distribution_filename::DistFilename; @@ -958,8 +965,7 @@ mod tests { &filename, &Url::parse("https://example.org/upload").unwrap(), &BaseClientBuilder::new().build(), - Some("ferris"), - Some("F3RR!S"), + &Credentials::basic(Some("ferris".to_string()), Some("F3RR!S".to_string())), &form_metadata, Arc::new(DummyReporter), ) @@ -969,35 +975,35 @@ mod tests { insta::with_settings!({ filters => [("boundary=[0-9a-f-]+", "boundary=[...]")], }, { - assert_debug_snapshot!(&request, @r###" - RequestBuilder { - inner: RequestBuilder { - method: POST, - url: Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "example.org", + assert_debug_snapshot!(&request, @r#" + RequestBuilder { + inner: RequestBuilder { + method: POST, + url: Url { + scheme: "https", + cannot_be_a_base: false, + username: "", + password: None, + host: Some( + Domain( + "example.org", + ), ), - ), - port: None, - path: "/upload", - query: None, - fragment: None, + port: None, + path: "/upload", + query: None, + fragment: None, + }, + headers: { + "content-type": "multipart/form-data; boundary=[...]", + "content-length": "6803", + "accept": "application/json;q=0.9, text/plain;q=0.8, text/html;q=0.7", + "authorization": Sensitive, + }, }, - headers: { - "content-type": "multipart/form-data; boundary=[...]", - "content-length": "6803", - "accept": "application/json;q=0.9, text/plain;q=0.8, text/html;q=0.7", - "authorization": "Basic ZmVycmlzOkYzUlIhUw==", - }, - }, - .. - } - "###); + .. + } + "#); }); } @@ -1109,8 +1115,7 @@ mod tests { &filename, &Url::parse("https://example.org/upload").unwrap(), &BaseClientBuilder::new().build(), - Some("ferris"), - Some("F3RR!S"), + &Credentials::basic(Some("ferris".to_string()), Some("F3RR!S".to_string())), &form_metadata, Arc::new(DummyReporter), ) @@ -1120,35 +1125,35 @@ mod tests { insta::with_settings!({ filters => [("boundary=[0-9a-f-]+", "boundary=[...]")], }, { - assert_debug_snapshot!(&request, @r###" - RequestBuilder { - inner: RequestBuilder { - method: POST, - url: Url { - scheme: "https", - cannot_be_a_base: false, - username: "", - password: None, - host: Some( - Domain( - "example.org", + assert_debug_snapshot!(&request, @r#" + RequestBuilder { + inner: RequestBuilder { + method: POST, + url: Url { + scheme: "https", + cannot_be_a_base: false, + username: "", + password: None, + host: Some( + Domain( + "example.org", + ), ), - ), - port: None, - path: "/upload", - query: None, - fragment: None, + port: None, + path: "/upload", + query: None, + fragment: None, + }, + headers: { + "content-type": "multipart/form-data; boundary=[...]", + "content-length": "19330", + "accept": "application/json;q=0.9, text/plain;q=0.8, text/html;q=0.7", + "authorization": Sensitive, + }, }, - headers: { - "content-type": "multipart/form-data; boundary=[...]", - "content-length": "19330", - "accept": "application/json;q=0.9, text/plain;q=0.8, text/html;q=0.7", - "authorization": "Basic ZmVycmlzOkYzUlIhUw==", - }, - }, - .. - } - "###); + .. + } + "#); }); } } diff --git a/crates/uv/src/commands/publish.rs b/crates/uv/src/commands/publish.rs index 16568373f..f86de83a9 100644 --- a/crates/uv/src/commands/publish.rs +++ b/crates/uv/src/commands/publish.rs @@ -9,7 +9,7 @@ use owo_colors::OwoColorize; use tokio::sync::Semaphore; use tracing::{debug, info}; use url::Url; - +use uv_auth::Credentials; use uv_cache::Cache; use uv_client::{AuthIntegration, BaseClient, BaseClientBuilder, RegistryClientBuilder}; use uv_configuration::{KeyringProviderType, TrustedPublishing}; @@ -74,7 +74,7 @@ pub(crate) async fn publish( // We're only checking a single URL and one at a time, so 1 permit is sufficient let download_concurrency = Arc::new(Semaphore::new(1)); - let (publish_url, username, password) = gather_credentials( + let (publish_url, credentials) = gather_credentials( publish_url, username, password, @@ -137,8 +137,7 @@ pub(crate) async fn publish( &filename, &publish_url, &upload_client, - username.as_deref(), - password.as_deref(), + &credentials, check_url_client.as_ref(), &download_concurrency, // Needs to be an `Arc` because the reqwest `Body` static lifetime requirement @@ -207,7 +206,7 @@ async fn gather_credentials( check_url: Option<&IndexUrl>, prompt: Prompt, printer: Printer, -) -> Result<(Url, Option, Option)> { +) -> Result<(Url, Credentials)> { // Support reading username and password from the URL, for symmetry with the index API. if let Some(url_password) = publish_url.password() { if password.is_some_and(|password| password != url_password) { @@ -318,7 +317,10 @@ async fn gather_credentials( // We may be using the keyring for the simple index. } } - Ok((publish_url, username, password)) + + let credentials = Credentials::basic(username, password); + + Ok((publish_url, credentials)) } fn prompt_username_and_password() -> Result<(Option, Option)> { @@ -343,11 +345,11 @@ mod tests { use insta::assert_snapshot; use url::Url; - async fn credentials( + async fn get_credentials( url: Url, username: Option, password: Option, - ) -> Result<(Url, Option, Option)> { + ) -> Result<(Url, Credentials)> { let client = BaseClientBuilder::new().build(); gather_credentials( url, @@ -370,30 +372,30 @@ mod tests { let example_url_username_password = Url::from_str("https://ferris:f3rr1s@example.com").unwrap(); - let (publish_url, username, password) = - credentials(example_url.clone(), None, None).await.unwrap(); + let (publish_url, credentials) = get_credentials(example_url.clone(), None, None) + .await + .unwrap(); assert_eq!(publish_url, example_url); - assert_eq!(username, None); - assert_eq!(password, None); + assert_eq!(credentials.username(), None); + assert_eq!(credentials.password(), None); - let (publish_url, username, password) = - credentials(example_url_username.clone(), None, None) + let (publish_url, credentials) = get_credentials(example_url_username.clone(), None, None) + .await + .unwrap(); + assert_eq!(publish_url, example_url); + assert_eq!(credentials.username(), Some("ferris")); + assert_eq!(credentials.password(), None); + + let (publish_url, credentials) = + get_credentials(example_url_username_password.clone(), None, None) .await .unwrap(); assert_eq!(publish_url, example_url); - assert_eq!(username.as_deref(), Some("ferris")); - assert_eq!(password, None); - - let (publish_url, username, password) = - credentials(example_url_username_password.clone(), None, None) - .await - .unwrap(); - assert_eq!(publish_url, example_url); - assert_eq!(username.as_deref(), Some("ferris")); - assert_eq!(password.as_deref(), Some("f3rr1s")); + assert_eq!(credentials.username(), Some("ferris")); + assert_eq!(credentials.password(), Some("f3rr1s")); // Ok: The username is the same between CLI/env vars and URL - let (publish_url, username, password) = credentials( + let (publish_url, credentials) = get_credentials( example_url_username_password.clone(), Some("ferris".to_string()), None, @@ -401,11 +403,11 @@ mod tests { .await .unwrap(); assert_eq!(publish_url, example_url); - assert_eq!(username.as_deref(), Some("ferris")); - assert_eq!(password.as_deref(), Some("f3rr1s")); + assert_eq!(credentials.username(), Some("ferris")); + assert_eq!(credentials.password(), Some("f3rr1s")); // Err: There are two different usernames between CLI/env vars and URL - let err = credentials( + let err = get_credentials( example_url_username_password.clone(), Some("packaging-platypus".to_string()), None, @@ -418,7 +420,7 @@ mod tests { ); // Ok: The username and password are the same between CLI/env vars and URL - let (publish_url, username, password) = credentials( + let (publish_url, credentials) = get_credentials( example_url_username_password.clone(), Some("ferris".to_string()), Some("f3rr1s".to_string()), @@ -426,11 +428,11 @@ mod tests { .await .unwrap(); assert_eq!(publish_url, example_url); - assert_eq!(username.as_deref(), Some("ferris")); - assert_eq!(password.as_deref(), Some("f3rr1s")); + assert_eq!(credentials.username(), Some("ferris")); + assert_eq!(credentials.password(), Some("f3rr1s")); // Err: There are two different passwords between CLI/env vars and URL - let err = credentials( + let err = get_credentials( example_url_username_password.clone(), Some("ferris".to_string()), Some("secret".to_string()),