Skip to content

Commit

Permalink
Apply from-URL credentials in authentication middleware (astral-sh#2449)
Browse files Browse the repository at this point in the history
## Summary

Right now, the middleware doesn't apply credentials that were
_originally_ sourced from a URL. This requires that we call
`with_url_encoded_auth` whenever we create a request to ensure that any
credentials that were passed in as part of an index URL (for example)
are respected.

This PR modifies `uv-auth` to instead apply those credentials in the
middleware itself. This seems preferable to me. As far as I can tell, we
can _only_ add in-URL credentials to the store ourselves (since in-URL
credentials are converted to headers by the time they reach the
middleware). And if we ever _didn't_ apply those credentials to new
URLs, it'd be a bug in the logic that precedes the middleware (i.e., us
forgetting to call `with_url_encoded_auth`).

## Test Plan

`cargo run pip install` with an authenticated index.
  • Loading branch information
charliermarsh authored Mar 15, 2024
1 parent 10abeae commit 8463d6d
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 62 deletions.
10 changes: 2 additions & 8 deletions crates/distribution-types/src/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@ use std::path::PathBuf;

use serde::{Deserialize, Serialize};
use thiserror::Error;
use url::Url;

use pep440_rs::{VersionSpecifiers, VersionSpecifiersParseError};
use pypi_types::{DistInfoMetadata, Hashes, Yanked};
use url::Url;
use uv_auth::GLOBAL_AUTH_STORE;

/// Error converting [`pypi_types::File`] to [`distribution_type::File`].
#[derive(Debug, Error)]
Expand Down Expand Up @@ -53,13 +52,8 @@ impl File {
size: file.size,
upload_time_utc_ms: file.upload_time.map(|dt| dt.timestamp_millis()),
url: if file.url.contains("://") {
// Copy over any credentials from the global store.
let url = Url::parse(&file.url)
.map_err(|err| FileConversionError::Url(file.url.clone(), err))?;
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(url);
FileLocation::AbsoluteUrl(url.to_string())
FileLocation::AbsoluteUrl(file.url)
} else {
// It's assumed that the base URL already contains any necessary credentials.
FileLocation::RelativeUrl(base.to_string(), file.url)
},
yanked: file.yanked,
Expand Down
23 changes: 10 additions & 13 deletions crates/uv-auth/src/middleware.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::path::Path;

use netrc::Netrc;
use reqwest::{header::HeaderValue, Request, Response};
use reqwest_middleware::{Middleware, Next};
use std::path::Path;
use task_local_extensions::Extensions;
use tracing::{debug, warn};

Expand Down Expand Up @@ -57,16 +58,10 @@ impl Middleware for AuthMiddleware {
// If we've already seen this URL, we can use the stored credentials
if let Some(auth) = stored_auth {
debug!("Adding authentication to already-seen URL: {url}");
match auth {
Credential::Basic(_) => {
req.headers_mut().insert(
reqwest::header::AUTHORIZATION,
basic_auth(auth.username(), auth.password()),
);
}
// Url must already have auth if before middleware runs - see `AuthenticationStore::with_url_encoded_auth`
Credential::UrlEncoded(_) => (),
}
req.headers_mut().insert(
reqwest::header::AUTHORIZATION,
basic_auth(auth.username(), auth.password()),
);
} else {
debug!("No credentials found for already-seen URL: {url}");
}
Expand Down Expand Up @@ -137,14 +132,16 @@ where

#[cfg(test)]
mod tests {
use super::*;
use std::io::Write;

use reqwest::Client;
use reqwest_middleware::ClientBuilder;
use std::io::Write;
use tempfile::NamedTempFile;
use wiremock::matchers::{basic_auth, method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};

use super::*;

const NETRC: &str = r#"default login myuser password mypassword"#;

#[tokio::test]
Expand Down
33 changes: 0 additions & 33 deletions crates/uv-auth/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,6 @@ impl AuthenticationStore {
credentials.insert(netloc, auth);
}

/// Copy authentication from one URL to another URL if applicable.
pub fn with_url_encoded_auth(&self, url: Url) -> Url {
let netloc = NetLoc::from(&url);
let credentials = self.credentials.lock().unwrap();
if let Some(Some(Credential::UrlEncoded(url_auth))) = credentials.get(&netloc) {
url_auth.apply_to_url(url)
} else {
url
}
}

/// Store in-URL credentials for future use.
pub fn save_from_url(&self, url: &Url) {
let netloc = NetLoc::from(url);
Expand Down Expand Up @@ -155,28 +144,6 @@ mod test {
assert!(not_found_res.is_none());
}

#[test]
fn store_with_url_encoded_auth() {
let store = AuthenticationStore::new();
let url = Url::parse("https://example.com/simple/").unwrap();
let auth = Credential::UrlEncoded(UrlAuthData {
username: "u".to_string(),
password: Some("p".to_string()),
});

// Before adding to the store there's no change
let url = store.with_url_encoded_auth(url);
assert_eq!(url.username(), "");
assert_eq!(url.password(), None);

store.set(&url, Some(auth.clone()));

// After adding to the store, the url is updated
let url = store.with_url_encoded_auth(url);
assert_eq!(url.username(), "u");
assert_eq!(url.password(), Some("p"));
}

#[test]
fn store_save_from_url() {
let store = AuthenticationStore::new();
Expand Down
6 changes: 2 additions & 4 deletions crates/uv-client/src/flat_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use pep440_rs::Version;
use pep508_rs::VerbatimUrl;
use platform_tags::Tags;
use pypi_types::Hashes;
use uv_auth::GLOBAL_AUTH_STORE;
use uv_cache::{Cache, CacheBucket};
use uv_normalize::PackageName;

Expand Down Expand Up @@ -157,17 +156,16 @@ impl<'a> FlatIndexClient<'a> {
async {
// Use the response URL, rather than the request URL, as the base for relative URLs.
// This ensures that we handle redirects and other URL transformations correctly.
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(response.url().clone());
let url = response.url().clone();

let text = response.text().await.map_err(ErrorKind::from)?;
let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url)
.map_err(|err| Error::from_html_err(err, url.clone()))?;

let base = GLOBAL_AUTH_STORE.with_url_encoded_auth(base.into_url());
let files: Vec<File> = files
.into_iter()
.filter_map(|file| {
match File::try_from(file, &base) {
match File::try_from(file, base.as_url()) {
Ok(file) => Some(file),
Err(err) => {
// Ignore files with unparsable version specifiers.
Expand Down
7 changes: 3 additions & 4 deletions crates/uv-client/src/registry_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use distribution_types::{BuiltDist, File, FileLocation, IndexUrl, IndexUrls, Nam
use install_wheel_rs::metadata::{find_archive_dist_info, is_metadata_entry};
use pep440_rs::Version;
use pypi_types::{Metadata23, SimpleJson};
use uv_auth::{AuthMiddleware, KeyringProvider, GLOBAL_AUTH_STORE};
use uv_auth::{AuthMiddleware, KeyringProvider};
use uv_cache::{Cache, CacheBucket, WheelCache};
use uv_fs::Simplified;
use uv_normalize::PackageName;
Expand Down Expand Up @@ -317,7 +317,7 @@ impl RegistryClient {
async {
// Use the response URL, rather than the request URL, as the base for relative URLs.
// This ensures that we handle redirects and other URL transformations correctly.
let url = GLOBAL_AUTH_STORE.with_url_encoded_auth(response.url().clone());
let url = response.url().clone();

let content_type = response
.headers()
Expand Down Expand Up @@ -346,9 +346,8 @@ impl RegistryClient {
let text = response.text().await.map_err(ErrorKind::from)?;
let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url)
.map_err(|err| Error::from_html_err(err, url.clone()))?;
let base = GLOBAL_AUTH_STORE.with_url_encoded_auth(base.into_url());

SimpleMetadata::from_files(files, package_name, &base)
SimpleMetadata::from_files(files, package_name, base.as_url())
}
};
OwnedArchive::from_unarchived(&unarchived)
Expand Down

0 comments on commit 8463d6d

Please sign in to comment.