From 4b70f14903b12d400c8a3914a2269d63672831a8 Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 7 Jan 2020 23:02:33 +0900 Subject: [PATCH 1/5] Load credentials only when needed Credentials are always loaded, even if these are not used. If access to confidential files such as credentials is not given, `cargo build` fails despite not using credentials. Fixes #7624. --- src/bin/cargo/commands/publish.rs | 2 ++ src/cargo/util/config/mod.rs | 29 +++++++++++++++++++----- tests/testsuite/build.rs | 26 ++++++++++++++++++++++ tests/testsuite/login.rs | 26 +++------------------- tests/testsuite/publish.rs | 37 +++++++++++++++++++++++++++++++ 5 files changed, 92 insertions(+), 28 deletions(-) diff --git a/src/bin/cargo/commands/publish.rs b/src/bin/cargo/commands/publish.rs index be5dcffddf4..51c2532d5b7 100644 --- a/src/bin/cargo/commands/publish.rs +++ b/src/bin/cargo/commands/publish.rs @@ -26,6 +26,8 @@ pub fn cli() -> App { } pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { + config.load_credentials()?; + let registry = args.registry(config)?; let ws = args.workspace(config)?; let index = args.index(config)?; diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index f2284947ac9..63e88e78133 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -746,7 +746,6 @@ impl Config { }) .chain_err(|| "could not load Cargo configuration")?; - self.load_credentials(&mut cfg)?; match cfg { CV::Table(map, _) => Ok(map), _ => unreachable!(), @@ -979,9 +978,8 @@ impl Config { Ok(url) } - /// Loads credentials config from the credentials file into the `ConfigValue` object, if - /// present. - fn load_credentials(&self, cfg: &mut ConfigValue) -> CargoResult<()> { + /// Loads credentials config from the credentials file, if present. + pub fn load_credentials(&mut self) -> CargoResult<()> { let home_path = self.home_path.clone().into_path_unlocked(); let credentials = match self.get_file_path(&home_path, "credentials", true)? { Some(credentials) => credentials, @@ -1006,7 +1004,28 @@ impl Config { } } - cfg.merge(value, true)?; + if let CV::Table(mut map, _) = value { + if map.contains_key("registry") { + if let Some(mut new_map) = self.values_mut()?.remove("registry") { + let token = map.remove("registry").unwrap(); + new_map.merge(token, true)?; + self.values_mut()?.insert("registry".into(), new_map); + } else { + self.values_mut()? + .insert("registry".into(), map.remove("registry").unwrap()); + } + } + if map.contains_key("registries") { + if let Some(mut new_map) = self.values_mut()?.remove("registries") { + let token = map.remove("registries").unwrap(); + new_map.merge(token, true)?; + self.values_mut()?.insert("registries".into(), new_map); + } else { + self.values_mut()? + .insert("registries".into(), map.remove("registries").unwrap()); + } + } + } Ok(()) } diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 292b81a8590..13f7f3a6248 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -2237,6 +2237,32 @@ fn recompile_space_in_name() { foo.cargo("build").with_stdout("").run(); } +#[cfg(unix)] +#[cargo_test] +fn credentials_is_unreadable() { + use cargo_test_support::paths::home; + use std::os::unix::prelude::*; + let p = project() + .file("Cargo.toml", &basic_manifest("foo", "0.1.0")) + .file("src/lib.rs", "") + .build(); + + let credentials = home().join(".cargo/credentials"); + t!(fs::create_dir_all(credentials.parent().unwrap())); + t!(t!(File::create(&credentials)).write_all( + br#" + [registry] + token = "api-token" + "# + )); + let stat = fs::metadata(credentials.as_path()).unwrap(); + let mut perms = stat.permissions(); + perms.set_mode(0o000); + fs::set_permissions(credentials, perms.clone()).unwrap(); + + p.cargo("build").run(); +} + #[cfg(unix)] #[cargo_test] fn ignore_bad_directories() { diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index 461f1b155ae..836f53054d6 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -111,28 +111,6 @@ fn credentials_work_with_extension() { 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(); @@ -161,7 +139,9 @@ fn new_credentials_is_used_instead_old() { .arg(TOKEN) .run(); - let config = Config::new(Shell::new(), cargo_home(), cargo_home()); + let mut config = Config::new(Shell::new(), cargo_home(), cargo_home()); + let _ = config.values(); + let _ = config.load_credentials(); let token = config.get_string("registry.token").unwrap().map(|p| p.val); assert_eq!(token.unwrap(), TOKEN); diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 5287f040f71..3b0b7d353bf 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1261,3 +1261,40 @@ repository = "foo" )], ); } + +#[cargo_test] +fn credentials_ambiguous_filename() { + registry::init(); + + let credentials_toml = paths::home().join(".cargo/credentials.toml"); + File::create(&credentials_toml) + .unwrap() + .write_all(br#"token = "api-token""#) + .unwrap(); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("publish --no-verify --index") + .arg(registry_url().to_string()) + .with_stderr_contains( + "\ +[WARNING] Both `[..]/credentials` and `[..]/credentials.toml` exist. Using `[..]/credentials` +", + ) + .run(); + + validate_upload_foo(); +} From 5e152863f4d37789e6d511505d33e278fcb65f6b Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Thu, 9 Jan 2020 22:18:45 +0900 Subject: [PATCH 2/5] Load credentials and add tests for `yank` and `owner` commands --- src/bin/cargo/commands/owner.rs | 2 ++ src/bin/cargo/commands/yank.rs | 2 ++ tests/testsuite/main.rs | 2 ++ tests/testsuite/owner.rs | 53 +++++++++++++++++++++++++++++++++ tests/testsuite/yank.rs | 47 +++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+) create mode 100644 tests/testsuite/owner.rs create mode 100644 tests/testsuite/yank.rs diff --git a/src/bin/cargo/commands/owner.rs b/src/bin/cargo/commands/owner.rs index e9d5d85213b..e7ac30da58a 100644 --- a/src/bin/cargo/commands/owner.rs +++ b/src/bin/cargo/commands/owner.rs @@ -39,6 +39,8 @@ Explicitly named owners can also modify the set of owners, so take care! } pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { + config.load_credentials()?; + let registry = args.registry(config)?; let opts = OwnersOptions { krate: args.value_of("crate").map(|s| s.to_string()), diff --git a/src/bin/cargo/commands/yank.rs b/src/bin/cargo/commands/yank.rs index 3079c544fb9..35ded936911 100644 --- a/src/bin/cargo/commands/yank.rs +++ b/src/bin/cargo/commands/yank.rs @@ -29,6 +29,8 @@ crates to be locked to any yanked version. } pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult { + config.load_credentials()?; + let registry = args.registry(config)?; ops::yank( diff --git a/tests/testsuite/main.rs b/tests/testsuite/main.rs index 9c1c1913e36..bbea3cc022e 100644 --- a/tests/testsuite/main.rs +++ b/tests/testsuite/main.rs @@ -68,6 +68,7 @@ mod net_config; mod new; mod offline; mod out_dir; +mod owner; mod package; mod patch; mod path; @@ -106,6 +107,7 @@ mod verify_project; mod version; mod warn_on_failure; mod workspaces; +mod yank; #[cargo_test] fn aaa_trigger_cross_compile_disabled_check() { diff --git a/tests/testsuite/owner.rs b/tests/testsuite/owner.rs new file mode 100644 index 00000000000..77fa8865735 --- /dev/null +++ b/tests/testsuite/owner.rs @@ -0,0 +1,53 @@ +//! Tests for the `cargo owner` command. + +use std::fs::{self, File}; +use std::io::prelude::*; + +use cargo_test_support::project; +use cargo_test_support::registry::{self, api_path, registry_url}; + +fn setup(name: &str) { + fs::create_dir_all(&api_path().join(format!("api/v1/crates/{}", name))).unwrap(); + + let dest = api_path().join(format!("api/v1/crates/{}/owners", name)); + + let content = r#"{ + "users": [ + { + "id": 70, + "login": "github:rust-lang:core", + "name": "Core" + } + ] + }"#; + + File::create(&dest) + .unwrap() + .write_all(content.as_bytes()) + .unwrap(); +} + +#[cargo_test] +fn simple_list() { + registry::init(); + setup("foo"); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("owner -l --index") + .arg(registry_url().to_string()) + .run(); +} diff --git a/tests/testsuite/yank.rs b/tests/testsuite/yank.rs new file mode 100644 index 00000000000..f836ff76d63 --- /dev/null +++ b/tests/testsuite/yank.rs @@ -0,0 +1,47 @@ +//! Tests for the `cargo yank` command. + +use std::fs::{self, File}; +use std::io::prelude::*; + +use cargo_test_support::project; +use cargo_test_support::registry::{self, api_path, registry_url}; + +fn setup(name: &str, version: &str) { + fs::create_dir_all(&api_path().join(format!("api/v1/crates/{}/{}", name, version))).unwrap(); + + let dest = api_path().join(format!("api/v1/crates/{}/{}/yank", name, version)); + + let content = r#"{ + "ok": true + }"#; + + File::create(&dest) + .unwrap() + .write_all(content.as_bytes()) + .unwrap(); +} + +#[cargo_test] +fn simple() { + registry::init(); + setup("foo", "0.0.1"); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("yank --vers 0.0.1 --index") + .arg(registry_url().to_string()) + .run(); +} From 3c673cb5731eb2514e7da7e39e0c41d214d00a9b Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Fri, 10 Jan 2020 13:44:00 +0900 Subject: [PATCH 3/5] Refactoring to loop over the key/values instead of hard-coding --- src/cargo/util/config/mod.rs | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 63e88e78133..86a91409b54 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1004,25 +1004,13 @@ impl Config { } } - if let CV::Table(mut map, _) = value { - if map.contains_key("registry") { - if let Some(mut new_map) = self.values_mut()?.remove("registry") { - let token = map.remove("registry").unwrap(); - new_map.merge(token, true)?; - self.values_mut()?.insert("registry".into(), new_map); + if let CV::Table(map, _) = value { + for (k, v) in map { + if let Some(mut base_map) = self.values_mut()?.remove(&k) { + base_map.merge(v, true)?; + self.values_mut()?.insert(k.into(), base_map); } else { - self.values_mut()? - .insert("registry".into(), map.remove("registry").unwrap()); - } - } - if map.contains_key("registries") { - if let Some(mut new_map) = self.values_mut()?.remove("registries") { - let token = map.remove("registries").unwrap(); - new_map.merge(token, true)?; - self.values_mut()?.insert("registries".into(), new_map); - } else { - self.values_mut()? - .insert("registries".into(), map.remove("registries").unwrap()); + self.values_mut()?.insert(k.into(), v); } } } From 8076f578a3108683b600312a9a8bba1276be0f6e Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 14 Jan 2020 16:15:29 +0900 Subject: [PATCH 4/5] Refactoring of creating files in tests Use `mkdir_p` and `fs::write`. --- tests/testsuite/owner.rs | 23 ++++++++++------------- tests/testsuite/publish.rs | 5 +---- tests/testsuite/yank.rs | 19 +++++-------------- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/tests/testsuite/owner.rs b/tests/testsuite/owner.rs index 77fa8865735..0a7e7154b00 100644 --- a/tests/testsuite/owner.rs +++ b/tests/testsuite/owner.rs @@ -1,17 +1,17 @@ //! Tests for the `cargo owner` command. -use std::fs::{self, File}; -use std::io::prelude::*; +use std::fs; +use cargo_test_support::paths::CargoPathExt; use cargo_test_support::project; use cargo_test_support::registry::{self, api_path, registry_url}; fn setup(name: &str) { - fs::create_dir_all(&api_path().join(format!("api/v1/crates/{}", name))).unwrap(); - - let dest = api_path().join(format!("api/v1/crates/{}/owners", name)); - - let content = r#"{ + let dir = api_path().join(format!("api/v1/crates/{}", name)); + dir.mkdir_p(); + fs::write( + dir.join("owners"), + r#"{ "users": [ { "id": 70, @@ -19,12 +19,9 @@ fn setup(name: &str) { "name": "Core" } ] - }"#; - - File::create(&dest) - .unwrap() - .write_all(content.as_bytes()) - .unwrap(); + }"#, + ) + .unwrap(); } #[cargo_test] diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 3b0b7d353bf..345f269764d 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1267,10 +1267,7 @@ fn credentials_ambiguous_filename() { registry::init(); let credentials_toml = paths::home().join(".cargo/credentials.toml"); - File::create(&credentials_toml) - .unwrap() - .write_all(br#"token = "api-token""#) - .unwrap(); + fs::write(credentials_toml, r#"token = "api-token""#).unwrap(); let p = project() .file( diff --git a/tests/testsuite/yank.rs b/tests/testsuite/yank.rs index f836ff76d63..8b528046fd1 100644 --- a/tests/testsuite/yank.rs +++ b/tests/testsuite/yank.rs @@ -1,24 +1,15 @@ //! Tests for the `cargo yank` command. -use std::fs::{self, File}; -use std::io::prelude::*; +use std::fs; +use cargo_test_support::paths::CargoPathExt; use cargo_test_support::project; use cargo_test_support::registry::{self, api_path, registry_url}; fn setup(name: &str, version: &str) { - fs::create_dir_all(&api_path().join(format!("api/v1/crates/{}/{}", name, version))).unwrap(); - - let dest = api_path().join(format!("api/v1/crates/{}/{}/yank", name, version)); - - let content = r#"{ - "ok": true - }"#; - - File::create(&dest) - .unwrap() - .write_all(content.as_bytes()) - .unwrap(); + let dir = api_path().join(format!("api/v1/crates/{}/{}", name, version)); + dir.mkdir_p(); + fs::write(dir.join("yank"), r#"{"ok": true}"#).unwrap(); } #[cargo_test] From 438d005b2cc49836d41c223ee01f2dbe739224ac Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Tue, 14 Jan 2020 22:11:16 +0900 Subject: [PATCH 5/5] Refactoring to use `Vacant`/`Occupied` instead of remove/insert --- src/cargo/util/config/mod.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 86a91409b54..4e97a1cb136 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1005,12 +1005,15 @@ impl Config { } if let CV::Table(map, _) = value { + let base_map = self.values_mut()?; for (k, v) in map { - if let Some(mut base_map) = self.values_mut()?.remove(&k) { - base_map.merge(v, true)?; - self.values_mut()?.insert(k.into(), base_map); - } else { - self.values_mut()?.insert(k.into(), v); + match base_map.entry(k) { + Vacant(entry) => { + entry.insert(v); + } + Occupied(mut entry) => { + entry.get_mut().merge(v, true)?; + } } } }