From 2d1c3cf60283f4e5144ce04e299a615cde850b46 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Fri, 12 Apr 2024 13:21:33 -0500 Subject: [PATCH] Minor cleanup --- crates/uv-auth/src/cache.rs | 55 +++++++++------- crates/uv-auth/src/middleware.rs | 65 +++++++++++-------- crates/uv-configuration/src/authentication.rs | 1 - 3 files changed, 68 insertions(+), 53 deletions(-) diff --git a/crates/uv-auth/src/cache.rs b/crates/uv-auth/src/cache.rs index 257298baff5b..653ebc90bca5 100644 --- a/crates/uv-auth/src/cache.rs +++ b/crates/uv-auth/src/cache.rs @@ -3,8 +3,8 @@ use std::{collections::HashMap, sync::Mutex}; use crate::credentials::Credentials; use crate::NetLoc; -use reqwest::Request; -use tracing::debug; + +use tracing::trace; use url::Url; type CacheKey = (NetLoc, Option); @@ -15,11 +15,11 @@ pub struct CredentialsCache { #[derive(Debug, Clone)] pub enum CheckResponse { - /// Credentials are already on the request. - OnRequest(Arc), + /// The given credentials should be used and are not present in the cache. + Uncached(Arc), /// Credentials were found in the cache. Cached(Arc), - // Credentials were not found in the cache or request. + // Credentials were not found in the cache and none were provided. None, } @@ -28,7 +28,7 @@ impl CheckResponse { pub fn get(&self) -> Option<&Credentials> { match self { Self::Cached(credentials) => Some(credentials.as_ref()), - Self::OnRequest(credentials) => Some(credentials.as_ref()), + Self::Uncached(credentials) => Some(credentials.as_ref()), Self::None => None, } } @@ -59,17 +59,20 @@ impl CredentialsCache { (NetLoc::from(url), username) } - /// Return the credentials that should be used for a request, if any. + /// Return the credentials that should be used for a URL, if any. + /// + /// The [`Url`] is not checked for credentials. Existing credentials should be extracted and passed + /// separately. /// - /// If complete credentials are already present on the request, they will be returned. + /// If complete credentials are provided, they will be returned as [`CheckResponse::Existing`] /// If the credentials are partial, i.e. missing a password, the cache will be checked /// for a corresponding entry. - pub(crate) fn check_request(&self, request: &Request) -> CheckResponse { + pub(crate) fn check(&self, url: &Url, credentials: Option) -> CheckResponse { let store = self.store.lock().unwrap(); - let credentials = Credentials::from_request(request).map(Arc::new); + let credentials = credentials.map(Arc::new); let key = CredentialsCache::key( - request.url(), + url, credentials .as_ref() .and_then(|credentials| credentials.username().map(str::to_string)), @@ -77,27 +80,31 @@ impl CredentialsCache { if let Some(credentials) = credentials { if credentials.password().is_some() { - debug!("Request already has password, skipping cache"); + trace!("Existing credentials include password, skipping cache"); // No need to look-up, we have a password already - return CheckResponse::OnRequest(credentials); + return CheckResponse::Uncached(credentials); } - debug!("No password found on request, checking cache"); + trace!("Existing credentials missing password, checking cache"); let existing = store.get(&key); existing .cloned() .map(CheckResponse::Cached) - .unwrap_or(CheckResponse::OnRequest(credentials)) + .inspect(|_| trace!("Found cached credentials.")) + .unwrap_or_else(|| { + trace!("No credentials in cache, using existing credentials"); + CheckResponse::Uncached(credentials) + }) } else { - debug!("No credentials on request, checking cache"); - let credentials = store.get(&key).cloned(); - if credentials.is_some() { - debug!("Found cached credentials: {credentials:?}"); - } else { - debug!("No credentials in cache"); - } - credentials + trace!("No credentials on request, checking cache..."); + store + .get(&key) + .cloned() .map(CheckResponse::Cached) - .unwrap_or(CheckResponse::None) + .inspect(|_| trace!("Found cached credentials.")) + .unwrap_or_else(|| { + trace!("No credentials in cache."); + CheckResponse::None + }) } } diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 19157d4a5713..14d77be7aff3 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -75,13 +75,28 @@ impl Middleware for AuthMiddleware { extensions: &mut Extensions, next: Next<'_>, ) -> reqwest_middleware::Result { - let url = request.url().clone(); - debug!("Handling request for {url}"); + // Check for credentials attached to (1) the request itself + let credentials = Credentials::from_request(&request); + // 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(); + credentials + .as_ref() + .and_then(|credentials| credentials.username()) + .and_then(|username| url.set_username(username).ok()); + credentials + .as_ref() + .and_then(|credentials| credentials.password()) + .and_then(|_| url.set_password(Some("****")).ok()); + url.to_string() + } else { + request.url().to_string() + }; + trace!("Handling request for {url}"); - // Check for credentials attached to: - // (1) The request itself - // (2) The cache - let credentials = self.cache().check_request(&request); + // Then check for credentials in (2) the cache + let credentials = self.cache().check(request.url(), credentials); // Track credentials that we might want to insert into the cache let mut new_credentials = None; @@ -93,15 +108,13 @@ impl Middleware for AuthMiddleware { CheckResponse::Cached(credentials) => request = credentials.authenticate(request), // If we get credentials from the request, we should update the cache // but don't need to update the request - CheckResponse::OnRequest(credentials) => { - new_credentials = Some(credentials.clone()) - } + CheckResponse::Uncached(credentials) => new_credentials = Some(credentials.clone()), CheckResponse::None => unreachable!("No credentials cannot be authenticated"), } // Otherwise, look for complete credentials in: // (3) The netrc file } else if let Some(credentials) = self.netrc.as_ref().and_then(|netrc| { - debug!("Checking netrc for credentials for `{}`", url.to_string()); + trace!("Checking netrc for credentials for {url}"); Credentials::from_netrc( netrc, request.url(), @@ -110,7 +123,7 @@ impl Middleware for AuthMiddleware { .and_then(|credentials| credentials.username()), ) }) { - debug!("Adding credentials from the netrc file to {url}"); + debug!("Found credentials in netrc file for {url}"); request = credentials.authenticate(request); new_credentials = Some(Arc::new(credentials)); // (4) The keyring @@ -119,46 +132,42 @@ impl Middleware for AuthMiddleware { .get() .and_then(|credentials| credentials.username()) { - debug!("Checking keyring for credentials for `{}`", url.to_string()); + debug!("Checking keyring for credentials for {url}"); keyring.fetch(request.url(), username) } else { - trace!( - "Skipping keyring lookup for `{}`: no username found", - url.to_string() - ); + trace!("Skipping keyring lookup for {url} with no username"); None } }) { - debug!("Adding credentials from the keyring to {url}"); + debug!("Found credentials in keyring for {url}"); request = credentials.authenticate(request); new_credentials = Some(Arc::new(credentials)) + // No additional credentials were found } else { match credentials { CheckResponse::Cached(credentials) => request = credentials.authenticate(request), - CheckResponse::OnRequest(credentials) => { - new_credentials = Some(credentials.clone()) - } + CheckResponse::Uncached(credentials) => new_credentials = Some(credentials.clone()), CheckResponse::None => { - debug!("No credentials found.") + debug!("No credentials found for {url}") } } } - // Perform the request - let result = next.run(request, extensions).await; - - // Only update the cache with new credentials on a successful request if let Some(credentials) = new_credentials { + // Only update the cache with new credentials on a successful request + let url = request.url().clone(); + let result = next.run(request, extensions).await; if result .as_ref() .is_ok_and(|response| response.error_for_status_ref().is_ok()) { - debug!("Updating credentials for {url} to {credentials:?}"); + trace!("Updating cached credentials for {url}"); self.cache().insert(&url, credentials) }; + result + } else { + next.run(request, extensions).await } - - result } } diff --git a/crates/uv-configuration/src/authentication.rs b/crates/uv-configuration/src/authentication.rs index 0e0955343790..b4dc82320b43 100644 --- a/crates/uv-configuration/src/authentication.rs +++ b/crates/uv-configuration/src/authentication.rs @@ -1,4 +1,3 @@ - use uv_auth::{self, KeyringProvider}; /// Keyring provider type to use for credential lookup.