Make TextCredentialStore.get_credentials fail on ambiguous matches

This commit is contained in:
Zsolt Dollenstein 2025-12-04 15:44:44 +00:00
parent 0c5391a7c7
commit c8fa990ddd
No known key found for this signature in database
4 changed files with 65 additions and 25 deletions

View File

@ -744,7 +744,10 @@ impl AuthMiddleware {
// Text credential store support. // Text credential store support.
} else if let Some(credentials) = self.text_store.get().await.and_then(|text_store| { } else if let Some(credentials) = self.text_store.get().await.and_then(|text_store| {
debug!("Checking text store for credentials for {url}"); debug!("Checking text store for credentials for {url}");
text_store.get_credentials(url, credentials.as_ref().and_then(|credentials| credentials.username())).cloned() text_store.get_credentials(
url,
credentials.as_ref().and_then(|credentials| credentials.username()),
).ok().unwrap_or_default().cloned()
}) { }) {
debug!("Found credentials in plaintext store for {url}"); debug!("Found credentials in plaintext store for {url}");
Some(credentials) Some(credentials)

View File

@ -122,6 +122,12 @@ pub enum BearerAuthError {
UnexpectedPassword, UnexpectedPassword,
} }
#[derive(Debug, Error, PartialEq)]
pub enum LookupError {
#[error("Multiple credentials found for URL '{0}', specify which username to use")]
AmbiguousUsername(DisplaySafeUrl),
}
/// A single credential entry in a TOML credentials file. /// A single credential entry in a TOML credentials file.
#[derive(Debug, Clone, Serialize, Deserialize)] #[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(try_from = "TomlCredentialWire", into = "TomlCredentialWire")] #[serde(try_from = "TomlCredentialWire", into = "TomlCredentialWire")]
@ -334,7 +340,7 @@ impl TextCredentialStore {
&self, &self,
url: &DisplaySafeUrl, url: &DisplaySafeUrl,
username: Option<&str>, username: Option<&str>,
) -> Option<&Credentials> { ) -> Result<Option<&Credentials>, LookupError> {
let request_realm = Realm::from(url); let request_realm = Realm::from(url);
// Perform an exact lookup first // Perform an exact lookup first
@ -345,7 +351,7 @@ impl TextCredentialStore {
url_service.clone(), url_service.clone(),
Username::from(username.map(str::to_string)), Username::from(username.map(str::to_string)),
)) { )) {
return Some(credential); return Ok(Some(credential));
} }
} }
@ -376,15 +382,17 @@ impl TextCredentialStore {
let specificity = service.url().path().len(); let specificity = service.url().path().len();
if best.is_none_or(|(best_specificity, _, _)| specificity > best_specificity) { if best.is_none_or(|(best_specificity, _, _)| specificity > best_specificity) {
best = Some((specificity, service, credential)); best = Some((specificity, service, credential));
} else if best.is_some_and(|(best_specificity, _, _)| specificity == best_specificity) {
return Err(LookupError::AmbiguousUsername(url.clone()));
} }
} }
// Return the most specific match // Return the most specific match
if let Some((_, _, credential)) = best { if let Some((_, _, credential)) = best {
return Some(credential); return Ok(Some(credential));
} }
None Ok(None)
} }
/// Store credentials for a given service. /// Store credentials for a given service.
@ -455,10 +463,10 @@ mod tests {
let service = Service::from_str("https://example.com").unwrap(); let service = Service::from_str("https://example.com").unwrap();
store.insert(service.clone(), credentials.clone()); store.insert(service.clone(), credentials.clone());
let url = DisplaySafeUrl::parse("https://example.com/").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/").unwrap();
assert!(store.get_credentials(&url, None).is_some()); assert!(store.get_credentials(&url, None).unwrap().is_some());
let url = DisplaySafeUrl::parse("https://example.com/path").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/path").unwrap();
let retrieved = store.get_credentials(&url, None).unwrap(); let retrieved = store.get_credentials(&url, None).unwrap().unwrap();
assert_eq!(retrieved.username(), Some("user")); assert_eq!(retrieved.username(), Some("user"));
assert_eq!(retrieved.password(), Some("pass")); assert_eq!(retrieved.password(), Some("pass"));
@ -468,7 +476,7 @@ mod tests {
.is_some() .is_some()
); );
let url = DisplaySafeUrl::parse("https://example.com/").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/").unwrap();
assert!(store.get_credentials(&url, None).is_none()); assert!(store.get_credentials(&url, None).unwrap().is_none());
} }
#[tokio::test] #[tokio::test]
@ -494,12 +502,12 @@ password = "pass2"
let store = TextCredentialStore::from_file(temp_file.path()).unwrap(); let store = TextCredentialStore::from_file(temp_file.path()).unwrap();
let url = DisplaySafeUrl::parse("https://example.com/").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/").unwrap();
assert!(store.get_credentials(&url, None).is_some()); assert!(store.get_credentials(&url, None).unwrap().is_some());
let url = DisplaySafeUrl::parse("https://test.org/").unwrap(); let url = DisplaySafeUrl::parse("https://test.org/").unwrap();
assert!(store.get_credentials(&url, None).is_some()); assert!(store.get_credentials(&url, None).unwrap().is_some());
let url = DisplaySafeUrl::parse("https://example.com").unwrap(); let url = DisplaySafeUrl::parse("https://example.com").unwrap();
let cred = store.get_credentials(&url, None).unwrap(); let cred = store.get_credentials(&url, None).unwrap().unwrap();
assert_eq!(cred.username(), Some("testuser")); assert_eq!(cred.username(), Some("testuser"));
assert_eq!(cred.password(), Some("testpass")); assert_eq!(cred.password(), Some("testpass"));
@ -535,7 +543,7 @@ password = "pass2"
for url_str in matching_urls { for url_str in matching_urls {
let url = DisplaySafeUrl::parse(url_str).unwrap(); let url = DisplaySafeUrl::parse(url_str).unwrap();
let cred = store.get_credentials(&url, None); let cred = store.get_credentials(&url, None).unwrap();
assert!(cred.is_some(), "Failed to match URL with prefix: {url_str}"); assert!(cred.is_some(), "Failed to match URL with prefix: {url_str}");
} }
@ -548,7 +556,7 @@ password = "pass2"
for url_str in non_matching_urls { for url_str in non_matching_urls {
let url = DisplaySafeUrl::parse(url_str).unwrap(); let url = DisplaySafeUrl::parse(url_str).unwrap();
let cred = store.get_credentials(&url, None); let cred = store.get_credentials(&url, None).unwrap();
assert!(cred.is_none(), "Should not match non-prefix URL: {url_str}"); assert!(cred.is_none(), "Should not match non-prefix URL: {url_str}");
} }
} }
@ -572,7 +580,7 @@ password = "pass2"
for url_str in matching_urls { for url_str in matching_urls {
let url = DisplaySafeUrl::parse(url_str).unwrap(); let url = DisplaySafeUrl::parse(url_str).unwrap();
let cred = store.get_credentials(&url, None); let cred = store.get_credentials(&url, None).unwrap();
assert!( assert!(
cred.is_some(), cred.is_some(),
"Failed to match URL in same realm: {url_str}" "Failed to match URL in same realm: {url_str}"
@ -588,7 +596,7 @@ password = "pass2"
for url_str in non_matching_urls { for url_str in non_matching_urls {
let url = DisplaySafeUrl::parse(url_str).unwrap(); let url = DisplaySafeUrl::parse(url_str).unwrap();
let cred = store.get_credentials(&url, None); let cred = store.get_credentials(&url, None).unwrap();
assert!( assert!(
cred.is_none(), cred.is_none(),
"Should not match URL in different realm: {url_str}" "Should not match URL in different realm: {url_str}"
@ -612,12 +620,12 @@ password = "pass2"
// Should match the most specific prefix // Should match the most specific prefix
let url = DisplaySafeUrl::parse("https://example.com/api/v1/users").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/api/v1/users").unwrap();
let cred = store.get_credentials(&url, None).unwrap(); let cred = store.get_credentials(&url, None).unwrap().unwrap();
assert_eq!(cred.username(), Some("specific")); assert_eq!(cred.username(), Some("specific"));
// Should match the general prefix for non-specific paths // Should match the general prefix for non-specific paths
let url = DisplaySafeUrl::parse("https://example.com/api/v2").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/api/v2").unwrap();
let cred = store.get_credentials(&url, None).unwrap(); let cred = store.get_credentials(&url, None).unwrap().unwrap();
assert_eq!(cred.username(), Some("general")); assert_eq!(cred.username(), Some("general"));
} }
@ -630,17 +638,17 @@ password = "pass2"
store.insert(service.clone(), user1_creds.clone()); store.insert(service.clone(), user1_creds.clone());
// Should return credentials when username matches // Should return credentials when username matches
let result = store.get_credentials(&url, Some("user1")); let result = store.get_credentials(&url, Some("user1")).unwrap();
assert!(result.is_some()); assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("user1")); assert_eq!(result.unwrap().username(), Some("user1"));
assert_eq!(result.unwrap().password(), Some("pass1")); assert_eq!(result.unwrap().password(), Some("pass1"));
// Should not return credentials when username doesn't match // Should not return credentials when username doesn't match
let result = store.get_credentials(&url, Some("user2")); let result = store.get_credentials(&url, Some("user2")).unwrap();
assert!(result.is_none()); assert!(result.is_none());
// Should return credentials when no username is specified // Should return credentials when no username is specified
let result = store.get_credentials(&url, None); let result = store.get_credentials(&url, None).unwrap();
assert!(result.is_some()); assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("user1")); assert_eq!(result.unwrap().username(), Some("user1"));
} }
@ -668,12 +676,12 @@ password = "pass2"
let url = DisplaySafeUrl::parse("https://example.com/api/v1/users").unwrap(); let url = DisplaySafeUrl::parse("https://example.com/api/v1/users").unwrap();
// Should match specific credentials when username matches // Should match specific credentials when username matches
let result = store.get_credentials(&url, Some("specific_user")); let result = store.get_credentials(&url, Some("specific_user")).unwrap();
assert!(result.is_some()); assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("specific_user")); assert_eq!(result.unwrap().username(), Some("specific_user"));
// Should match the general credentials when requesting general_user (falls back to less specific prefix) // Should match the general credentials when requesting general_user (falls back to less specific prefix)
let result = store.get_credentials(&url, Some("general_user")); let result = store.get_credentials(&url, Some("general_user")).unwrap();
assert!( assert!(
result.is_some(), result.is_some(),
"Should match general_user from less specific prefix" "Should match general_user from less specific prefix"
@ -681,8 +689,37 @@ password = "pass2"
assert_eq!(result.unwrap().username(), Some("general_user")); assert_eq!(result.unwrap().username(), Some("general_user"));
// Should match most specific when no username specified // Should match most specific when no username specified
let result = store.get_credentials(&url, None); let result = store.get_credentials(&url, None).unwrap();
assert!(result.is_some()); assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("specific_user")); assert_eq!(result.unwrap().username(), Some("specific_user"));
} }
#[test]
fn test_ambiguous_username_error() {
let mut store = TextCredentialStore::default();
// Add two credentials for the same service with different usernames
let service = Service::from_str("https://example.com/api").unwrap();
let user1_creds = Credentials::basic(Some("user1".to_string()), Some("pass1".to_string()));
let user2_creds = Credentials::basic(Some("user2".to_string()), Some("pass2".to_string()));
store.insert(service.clone(), user1_creds);
store.insert(service.clone(), user2_creds);
let url = DisplaySafeUrl::parse("https://example.com/api/v1").unwrap();
// When no username is specified, should return an error because there are multiple matches with same specificity
let result = store.get_credentials(&url, None);
assert!(result.is_err());
assert_eq!(result, Err(LookupError::AmbiguousUsername(url.clone())));
// When a specific username is provided, should return the correct credentials
let result = store.get_credentials(&url, Some("user1")).unwrap();
assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("user1"));
let result = store.get_credentials(&url, Some("user2")).unwrap();
assert!(result.is_some());
assert_eq!(result.unwrap().username(), Some("user2"));
}
} }

View File

@ -105,7 +105,7 @@ async fn credentials_for_url(
let backend = AuthBackend::from_settings(preview).await?; let backend = AuthBackend::from_settings(preview).await?;
let credentials = match &backend { let credentials = match &backend {
AuthBackend::System(provider) => provider.fetch(url, username).await, AuthBackend::System(provider) => provider.fetch(url, username).await,
AuthBackend::TextStore(store, _lock) => store.get_credentials(url, username).cloned(), AuthBackend::TextStore(store, _lock) => store.get_credentials(url, username)?.cloned(),
}; };
Ok(credentials) Ok(credentials)
} }

View File

@ -74,7 +74,7 @@ pub(crate) async fn token(
.await .await
.ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?, .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?,
AuthBackend::TextStore(store, _lock) => store AuthBackend::TextStore(store, _lock) => store
.get_credentials(url, Some(&username)) .get_credentials(url, Some(&username))?
.cloned() .cloned()
.ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?, .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?,
}; };