From 50f265a6ed7c09c03752438db71795814d9738b3 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 10 Apr 2024 16:47:56 -0500 Subject: [PATCH] Refactor the keyring provider to allow testing --- crates/uv-auth/src/keyring.rs | 193 +++++++++++++++++++++++-------- crates/uv-auth/src/middleware.rs | 10 +- 2 files changed, 146 insertions(+), 57 deletions(-) diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs index f4622fe6fe82..bc1848aa23f6 100644 --- a/crates/uv-auth/src/keyring.rs +++ b/crates/uv-auth/src/keyring.rs @@ -9,8 +9,9 @@ use crate::credentials::Credentials; /// Keyring provider to use for authentication /// /// See -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] -#[cfg_attr(feature = "clap", derive(clap::ValueEnum))] +#[derive(Debug, Default, Clone, PartialEq, Eq)] +#[cfg_attr(all(feature = "clap", not(test)), derive(clap::ValueEnum))] +#[cfg_attr(not(test), derive(Copy))] pub enum KeyringProvider { /// Will not use keyring for authentication #[default] @@ -21,6 +22,10 @@ pub enum KeyringProvider { // Auto, // /// Not implemented yet. Maybe use for this? // Import, + /// A provider that returns preset credentials. + /// Only available during testing of this crate. + #[cfg(test)] + Dummy(std::collections::HashMap), } #[derive(Debug, Error)] @@ -33,72 +38,160 @@ pub(crate) enum Error { ParseFailed(#[from] std::string::FromUtf8Error), } -/// Get credentials from keyring for given url -/// -/// See `pip`'s KeyringCLIProvider -/// -pub(crate) fn get_keyring_subprocess_auth(url: &Url) -> Result, Error> { - let host = url.host_str(); - if host.is_none() { - return Err(Error::NotKeyringTarget( - "Should only use keyring for urls with host".to_string(), - )); +impl KeyringProvider { + /// Fetch credentials for the given [`Url`] from the keyring. + /// + /// Returns `None` if no password was found, even if a username is present on the URL. + pub(crate) fn fetch(&self, url: &Url) -> Result, Error> { + let host = url.host_str(); + if host.is_none() { + return Err(Error::NotKeyringTarget( + "Should only use keyring for urls with host".to_string(), + )); + } + + if url.password().is_some() { + return Err(Error::NotKeyringTarget( + "Url already contains password - keyring not required".to_string(), + )); + } + + debug!("Checking keyring for credentials for `{}`", url.to_string(),); + let password = match self { + Self::Disabled => Ok(None), + Self::Subprocess => self.fetch_subprocess(url), + #[cfg(test)] + Self::Dummy(provider) => self.fetch_dummy(provider, url), + }?; + + Ok(password.map(|password| Credentials::new(url.username().to_string(), Some(password)))) } - if url.password().is_some() { - return Err(Error::NotKeyringTarget( - "Url already contains password - keyring not required".to_string(), - )); + + /// Fetch from the `keyring` subprocess. + /// + /// See pip's implementation + /// + fn fetch_subprocess(&self, url: &Url) -> Result, Error> { + let output = match Command::new("keyring") + .arg("get") + .arg(url.to_string()) + .arg(url.username()) + .output() + { + Ok(output) if output.status.success() => Ok(Some( + String::from_utf8(output.stdout) + .map_err(Error::ParseFailed)? + .trim_end() + .to_owned(), + )), + Ok(_) => Ok(None), + Err(e) => Err(Error::CliFailure(e)), + }; + + output + } + + #[cfg(test)] + fn fetch_dummy( + &self, + provider: &std::collections::HashMap, + url: &Url, + ) -> Result, Error> { + Ok(provider.get(url).map(|password| password.to_string())) } - let username = match url.username() { - u if !u.is_empty() => u, - // this is the username keyring.get_credentials returns as username for GCP registry - _ => "oauth2accesstoken", - }; - debug!( - "Running `keyring get` for `{}` with username `{}`", - url.to_string(), - username - ); - let output = match Command::new("keyring") - .arg("get") - .arg(url.to_string()) - .arg(username) - .output() - { - Ok(output) if output.status.success() => Ok(Some( - String::from_utf8(output.stdout) - .map_err(Error::ParseFailed)? - .trim_end() - .to_owned(), - )), - Ok(_) => Ok(None), - Err(e) => Err(Error::CliFailure(e)), - }; - - output.map(|password| { - password.map(|password| Credentials::new(username.to_string(), Some(password))) - }) } #[cfg(test)] mod test { + use std::collections::HashMap; + use super::*; #[test] - fn hostless_url_should_err() { + fn fetch_url_no_host() { let url = Url::parse("file:/etc/bin/").unwrap(); - let res = get_keyring_subprocess_auth(&url); + let res = KeyringProvider::Dummy(HashMap::default()).fetch(&url); assert!(res.is_err()); assert!(matches!(res.unwrap_err(), Error::NotKeyringTarget(s) if s == "Should only use keyring for urls with host")); } #[test] - fn passworded_url_should_err() { - let url = Url::parse("https://u:p@example.com").unwrap(); - let res = get_keyring_subprocess_auth(&url); + fn fetch_url_with_password() { + let url = Url::parse("https://user:password@example.com").unwrap(); + let res = KeyringProvider::Dummy(HashMap::default()).fetch(&url); + assert!(res.is_err()); + assert!(matches!(res.unwrap_err(), + Error::NotKeyringTarget(s) if s == "Url already contains password - keyring not required")); + } + + #[test] + fn fetch_url_with_password_and_no_username() { + let url = Url::parse("https://:password@example.com").unwrap(); + let res = KeyringProvider::Dummy(HashMap::default()).fetch(&url); assert!(res.is_err()); assert!(matches!(res.unwrap_err(), Error::NotKeyringTarget(s) if s == "Url already contains password - keyring not required")); } + + #[test] + fn fetch_url_no_auth() -> Result<(), Error> { + let url = Url::parse("https://example.com").unwrap(); + let credentials = KeyringProvider::Dummy(HashMap::default()).fetch(&url)?; + assert!(credentials.is_none()); + Ok(()) + } + + #[test] + fn fetch_url() -> Result<(), Error> { + let url = Url::parse("https://example.com").unwrap(); + let credentials = + KeyringProvider::Dummy(HashMap::from_iter([(url.clone(), "password")])).fetch(&url)?; + assert_eq!( + credentials, + Some(Credentials::new( + "".to_string(), + Some("password".to_string()) + )) + ); + Ok(()) + } + + #[test] + fn fetch_url_no_match() -> Result<(), Error> { + let url = Url::parse("https://example.com").unwrap(); + let credentials = KeyringProvider::Dummy(HashMap::from_iter([( + Url::parse("https://other.com").unwrap(), + "password", + )])) + .fetch(&url)?; + assert_eq!(credentials, None); + Ok(()) + } + + #[test] + fn fetch_url_username() -> Result<(), Error> { + let url = Url::parse("https://user@example.com").unwrap(); + let credentials = + KeyringProvider::Dummy(HashMap::from_iter([(url.clone(), "password")])).fetch(&url)?; + assert_eq!( + credentials, + Some(Credentials::new( + "user".to_string(), + Some("password".to_string()) + )) + ); + Ok(()) + } + + #[test] + fn fetch_url_username_no_match() -> Result<(), Error> { + let foo_url = Url::parse("https://foo@example.com").unwrap(); + let bar_url = Url::parse("https://bar@example.com").unwrap(); + let credentials = + KeyringProvider::Dummy(HashMap::from_iter([(foo_url.clone(), "password")])) + .fetch(&bar_url)?; + assert_eq!(credentials, None,); + Ok(()) + } } diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 90829178ec5d..4bbfab00b152 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -6,11 +6,7 @@ use reqwest::{Request, Response}; use reqwest_middleware::{Middleware, Next}; use tracing::{debug, warn}; -use crate::{ - credentials::Credentials, - keyring::{get_keyring_subprocess_auth, KeyringProvider}, - GLOBAL_AUTH_STORE, -}; +use crate::{credentials::Credentials, keyring::KeyringProvider, GLOBAL_AUTH_STORE}; /// A middleware that adds basic authentication to requests based on the netrc file and the keyring. /// @@ -68,9 +64,9 @@ impl Middleware for AuthMiddleware { let credentials = Credentials::from(credentials.to_owned()); request = credentials.authenticated_request(request); GLOBAL_AUTH_STORE.set(&url, credentials); - } else if matches!(self.keyring_provider, KeyringProvider::Subprocess) { + } else { // If we have keyring support enabled, we check there as well - match get_keyring_subprocess_auth(&url) { + match self.keyring_provider.fetch(&url) { Ok(Some(credentials)) => { request = credentials.authenticated_request(request); GLOBAL_AUTH_STORE.set(&url, credentials);