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.

Author: zachlute

src/cargo/util/config.rs

@@ -689,56 +689,58 @@ 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<Option<PathBuf>> {
+        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<F>(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()>
     where
         F: FnMut(&Path) -> CargoResult<()>,
     {
         let mut stash: HashSet<PathBuf> = 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(&current.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<PathBuf> {
 }
 
 pub fn save_credentials(cfg: &Config, token: String, registry: Option<String>) -> 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) = {

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();