From 25e3fee3c5d6d6d4a747d5316f91a9c3dadfbf02 Mon Sep 17 00:00:00 2001 From: Zach Lute Date: Wed, 28 Aug 2019 23:29:31 -0700 Subject: [PATCH] Allow using 'credentials.toml' instead of just 'credentials' files. This matches a similar change to config[.toml]. Note that this change only makes 'credentials.toml' optional to use instead of 'credentials'. If both exist, we will print a warning and prefer 'credentials', since that would be the existing behavior if both existed. --- src/cargo/util/config.rs | 96 +++++++++++++++++++++++----------------- tests/testsuite/login.rs | 46 +++++++++++++++++++ 2 files changed, 101 insertions(+), 41 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index a7659288bb8..bdc344b83a0 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -689,6 +689,38 @@ impl Config { } } + /// The purpose of this function is to aid in the transition to using + /// .toml extensions on Cargo's config files, which were historically not used. + /// Both 'config.toml' and 'credentials.toml' should be valid with or without extension. + /// When both exist, we want to prefer the one without an extension for + /// backwards compatibility, but warn the user appropriately. + fn get_file_path( + &self, + dir: &Path, + filename_without_extension: &str, + warn: bool, + ) -> CargoResult> { + let possible = dir.join(filename_without_extension); + let possible_with_extension = dir.join(format!("{}.toml", filename_without_extension)); + + if fs::metadata(&possible).is_ok() { + if warn && fs::metadata(&possible_with_extension).is_ok() { + self.shell().warn(format!( + "Both `{}` and `{}` exist. Using `{}`", + possible.display(), + possible_with_extension.display(), + possible.display() + ))?; + } + + Ok(Some(possible)) + } else if fs::metadata(&possible_with_extension).is_ok() { + Ok(Some(possible_with_extension)) + } else { + Ok(None) + } + } + fn walk_tree(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> where F: FnMut(&Path) -> CargoResult<()>, @@ -696,49 +728,19 @@ impl Config { let mut stash: HashSet = HashSet::new(); for current in paths::ancestors(pwd) { - let possible = current.join(".cargo").join("config"); - let possible_with_extension = current.join(".cargo").join("config.toml"); - - // If both 'config' and 'config.toml' exist, we should use 'config' - // for backward compatibility, but we should warn the user. - if fs::metadata(&possible).is_ok() { - if fs::metadata(&possible_with_extension).is_ok() { - self.shell().warn(format!( - "Both `{}` and `{}` exist. Using `{}`", - possible.display(), - possible_with_extension.display(), - possible.display() - ))?; - } - - walk(&possible)?; - stash.insert(possible); - } else if fs::metadata(&possible_with_extension).is_ok() { - walk(&possible_with_extension)?; - stash.insert(possible); + if let Some(path) = self.get_file_path(¤t.join(".cargo"), "config", true)? { + walk(&path)?; + stash.insert(path); } } // Once we're done, also be sure to walk the home directory even if it's not // in our history to be sure we pick up that standard location for // information. - let config = home.join("config"); - let config_with_extension = home.join("config.toml"); - if !stash.contains(&config) && fs::metadata(&config).is_ok() { - if fs::metadata(&config_with_extension).is_ok() { - self.shell().warn(format!( - "Both `{}` and `{}` exist. Using `{}`", - config.display(), - config_with_extension.display(), - config.display() - ))?; + if let Some(path) = self.get_file_path(home, "config", true)? { + if !stash.contains(&path) { + walk(&path)?; } - - walk(&config)?; - } else if !stash.contains(&config_with_extension) - && fs::metadata(&config_with_extension).is_ok() - { - walk(&config_with_extension)?; } Ok(()) @@ -781,10 +783,10 @@ impl Config { /// present. fn load_credentials(&self, cfg: &mut ConfigValue) -> CargoResult<()> { let home_path = self.home_path.clone().into_path_unlocked(); - let credentials = home_path.join("credentials"); - if fs::metadata(&credentials).is_err() { - return Ok(()); - } + let credentials = match self.get_file_path(&home_path, "credentials", true)? { + Some(credentials) => credentials, + None => return Ok(()), + }; let mut contents = String::new(); let mut file = File::open(&credentials)?; @@ -1729,10 +1731,22 @@ pub fn homedir(cwd: &Path) -> Option { } pub fn save_credentials(cfg: &Config, token: String, registry: Option) -> CargoResult<()> { + // If 'credentials.toml' exists, we should write to that, otherwise + // use the legacy 'credentials'. There's no need to print the warning + // here, because it would already be printed at load time. + let home_path = cfg.home_path.clone().into_path_unlocked(); + let filename = match cfg.get_file_path(&home_path, "credentials", false)? { + Some(path) => match path.file_name() { + Some(filename) => Path::new(filename).to_owned(), + None => Path::new("credentials").to_owned(), + }, + None => Path::new("credentials").to_owned(), + }; + let mut file = { cfg.home_path.create_dir()?; cfg.home_path - .open_rw(Path::new("credentials"), cfg, "credentials' config file")? + .open_rw(filename, cfg, "credentials' config file")? }; let (key, value) = { diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index a9476159e44..d203d818fdb 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -1,5 +1,6 @@ use std::fs::{self, File}; use std::io::prelude::*; +use std::path::PathBuf; use crate::support::cargo_process; use crate::support::install::cargo_home; @@ -13,6 +14,15 @@ const ORIGINAL_TOKEN: &str = "api-token"; fn setup_new_credentials() { let config = cargo_home().join("credentials"); + setup_new_credentials_at(config); +} + +fn setup_new_credentials_toml() { + let config = cargo_home().join("credentials.toml"); + setup_new_credentials_at(config); +} + +fn setup_new_credentials_at(config: PathBuf) { t!(fs::create_dir_all(config.parent().unwrap())); t!(t!(File::create(&config)) .write_all(format!(r#"token = "{token}""#, token = ORIGINAL_TOKEN).as_bytes())); @@ -84,6 +94,42 @@ fn login_with_new_credentials() { assert!(check_token(TOKEN, None)); } +#[cargo_test] +fn credentials_work_with_extension() { + registry::init(); + setup_new_credentials_toml(); + + cargo_process("login --host") + .arg(registry_url().to_string()) + .arg(TOKEN) + .run(); + + // Ensure that we get the new token for the registry + assert!(check_token(TOKEN, None)); +} + +#[cargo_test] +fn credentials_ambiguous_filename() { + registry::init(); + setup_new_credentials(); + setup_new_credentials_toml(); + + cargo_process("login --host") + .arg(registry_url().to_string()) + .arg(TOKEN) + .with_stderr_contains( + "\ +[WARNING] Both `[..]/credentials` and `[..]/credentials.toml` exist. Using `[..]/credentials` +", + ) + .run(); + + // It should use the value from the one without the extension + // for backwards compatibility. check_token explicitly checks + // 'credentials' without the extension, which is what we want. + assert!(check_token(TOKEN, None)); +} + #[cargo_test] fn login_with_old_and_new_credentials() { setup_new_credentials();