Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load credentials only when needed #7774

Merged
merged 5 commits into from
Jan 15, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/bin/cargo/commands/owner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down
2 changes: 2 additions & 0 deletions src/bin/cargo/commands/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down
2 changes: 2 additions & 0 deletions src/bin/cargo/commands/yank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
17 changes: 12 additions & 5 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(),
Expand Down Expand Up @@ -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,
Expand All @@ -1006,7 +1004,16 @@ impl Config {
}
}

cfg.merge(value, true)?;
if let CV::Table(map, _) = value {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this maybe be simplified a little instead of removing and reinserting? Maybe something like this?

        if let CV::Table(map, _) = value {
            let base_map = self.values_mut()?;
            for (k, v) in map {
                match base_map.entry(k) {
                    Vacant(entry) => { entry.insert(v); }
                    Occupied(mut entry) => { entry.get_mut().merge(v, true)? }
                }
            }
        }

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

Ok(())
}
Expand Down
26 changes: 26 additions & 0 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
26 changes: 3 additions & 23 deletions tests/testsuite/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ mod net_config;
mod new;
mod offline;
mod out_dir;
mod owner;
mod package;
mod patch;
mod path;
Expand Down Expand Up @@ -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() {
Expand Down
53 changes: 53 additions & 0 deletions tests/testsuite/owner.rs
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to yank.

    let dir = api_path().join(format!("api/v1/crates/{}", name));
    dir.mkdir_p();
    // ...
    fs::write(dir.join("owners"), content).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();
}
37 changes: 37 additions & 0 deletions tests/testsuite/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs::write here too.

.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();
}
47 changes: 47 additions & 0 deletions tests/testsuite/yank.rs
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified a little with:

    let dir = api_path().join(format!("api/v1/crates/{}/{}", name, version));
    dir.mkdir_p();
    fs::write(dir.join("yank"), r#"{"ok": true}"#).unwrap();

mkdir_p needs use cargo_test_support::paths::CargoPathExt;


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