From 803fd6909cd41b38e0a40afecd09988ba567dca7 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Thu, 7 Sep 2023 20:56:56 -0500 Subject: [PATCH] fix: don't print _TOKEN suggestion when not applicable Cargo should not suggest the _TOKEN environment variable when the cargo:token provider isn't available --- src/cargo/sources/registry/http_remote.rs | 13 ++-- src/cargo/util/auth/mod.rs | 93 ++++++++++++++++------- tests/testsuite/credential_process.rs | 32 +++++++- 3 files changed, 105 insertions(+), 33 deletions(-) diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 7ce1e49cc48..1346b6995ab 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -588,12 +588,13 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { } .into()); if self.auth_required { - return Poll::Ready(err.context(auth::AuthorizationError { - sid: self.source_id.clone(), - default_registry: self.config.default_registry()?, - login_url: self.login_url.clone(), - reason: auth::AuthorizationErrorReason::TokenRejected, - })); + let auth_error = auth::AuthorizationError::new( + self.config, + self.source_id, + self.login_url.clone(), + auth::AuthorizationErrorReason::TokenRejected, + )?; + return Poll::Ready(err.context(auth_error)); } else { return Poll::Ready(err); } diff --git a/src/cargo/util/auth/mod.rs b/src/cargo/util/auth/mod.rs index ae8db442121..cc8128d6ca4 100644 --- a/src/cargo/util/auth/mod.rs +++ b/src/cargo/util/auth/mod.rs @@ -74,7 +74,19 @@ impl RegistryConfigExtended { } /// Get the list of credential providers for a registry source. -fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult>> { +fn credential_provider( + config: &Config, + sid: &SourceId, + show_warnings: bool, +) -> CargoResult>> { + let warn = |message: String| { + if show_warnings { + config.shell().warn(message) + } else { + Ok(()) + } + }; + let cfg = registry_credential_config_raw(config, sid)?; let default_providers = || { if config.cli_unstable().asymmetric_token { @@ -111,7 +123,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult CargoResult CargoResult { if token_pos < paseto_pos { - config.shell().warn(format!( + warn(format!( "{sid} has a `secret_key` configured in {} that will be ignored \ because a `token` is also configured, and the `cargo:token` provider is \ configured with higher precedence", secret_key.definition ))?; } else { - config.shell().warn(format!("{sid} has a `token` configured in {} that will be ignored \ + warn(format!("{sid} has a `token` configured in {} that will be ignored \ because a `secret_key` is also configured, and the `cargo:paseto` provider is \ configured with higher precedence", token.definition))?; } @@ -172,7 +184,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult CargoResult, + default_registry: Option, /// Url where the user could log in. pub login_url: Option, /// Specific reason indicating what failed - pub reason: AuthorizationErrorReason, + reason: AuthorizationErrorReason, + /// Should the _TOKEN environment variable name be included when displaying this error? + display_token_env_help: bool, } + +impl AuthorizationError { + pub fn new( + config: &Config, + sid: SourceId, + login_url: Option, + reason: AuthorizationErrorReason, + ) -> CargoResult { + // Only display the _TOKEN environment variable suggestion if the `cargo:token` credential + // provider is available for the source. Otherwise setting the environment variable will + // have no effect. + let display_token_env_help = credential_provider(config, &sid, false)? + .iter() + .any(|p| p.first().map(String::as_str) == Some("cargo:token")); + Ok(AuthorizationError { + sid, + default_registry: config.default_registry()?, + login_url, + reason, + display_token_env_help, + }) + } +} + impl Error for AuthorizationError {} impl fmt::Display for AuthorizationError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -377,20 +415,23 @@ impl fmt::Display for AuthorizationError { } else { "" }; - write!( - f, - "{}, please run `cargo login{args}`\nor use environment variable CARGO_REGISTRY_TOKEN", - self.reason - ) + write!(f, "{}, please run `cargo login{args}`", self.reason)?; + if self.display_token_env_help { + write!(f, "\nor use environment variable CARGO_REGISTRY_TOKEN")?; + } + Ok(()) } else if let Some(name) = self.sid.alt_registry_key() { let key = ConfigKey::from_str(&format!("registries.{name}.token")); write!( f, - "{} for `{}`, please run `cargo login --registry {name}`\nor use environment variable {}", + "{} for `{}`, please run `cargo login --registry {name}`", self.reason, self.sid.display_registry_name(), - key.as_env_key(), - ) + )?; + if self.display_token_env_help { + write!(f, "\nor use environment variable {}", key.as_env_key())?; + } + Ok(()) } else if self.reason == AuthorizationErrorReason::TokenMissing { write!( f, @@ -416,7 +457,7 @@ my-registry = {{ index = "{}" }} } } -// Store a token in the cache for future calls. +/// Store a token in the cache for future calls. pub fn cache_token_from_commandline(config: &Config, sid: &SourceId, token: Secret<&str>) { let url = sid.canonical_url(); config.credential_cache().insert( @@ -446,7 +487,7 @@ fn credential_action( name, headers, }; - let providers = credential_provider(config, sid)?; + let providers = credential_provider(config, sid, true)?; let mut any_not_found = false; for provider in providers { let args: Vec<&str> = provider @@ -511,12 +552,12 @@ pub fn auth_token( ) -> CargoResult { match auth_token_optional(config, sid, operation, headers)? { Some(token) => Ok(token.expose()), - None => Err(AuthorizationError { - sid: sid.clone(), - default_registry: config.default_registry()?, - login_url: login_url.cloned(), - reason: AuthorizationErrorReason::TokenMissing, - } + None => Err(AuthorizationError::new( + config, + *sid, + login_url.cloned(), + AuthorizationErrorReason::TokenMissing, + )? .into()), } } diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index 1bba38800b5..55a568dff18 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -320,6 +320,36 @@ fn build_provider(name: &str, response: &str) -> String { toml_bin(&cred_proj, name) } +#[cargo_test] +fn not_found() { + let registry = registry::RegistryBuilder::new() + .no_configure_token() + .http_index() + .auth_required() + .credential_provider(&[&build_provider( + "not_found", + r#"{"Err": {"kind": "not-found"}}"#, + )]) + .build(); + + // should not suggest a _TOKEN environment variable since the cargo:token provider isn't available. + cargo_process("install -v foo -Zcredential-process -Zregistry-auth") + .masquerade_as_nightly_cargo(&["credential-process", "registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_status(101) + .with_stderr( + r#"[UPDATING] [..] +[CREDENTIAL] [..]not_found[..] get crates-io +{"v":1[..] +[ERROR] failed to query replaced source registry `crates-io` + +Caused by: + no token found, please run `cargo login` +"#, + ) + .run(); +} + #[cargo_test] fn all_not_found() { let server = registry::RegistryBuilder::new() @@ -342,6 +372,7 @@ fn all_not_found() { ) .unwrap(); + // should not suggest a _TOKEN environment variable since the cargo:token provider isn't available. cargo_process("install -v foo -Zcredential-process -Zregistry-auth") .masquerade_as_nightly_cargo(&["credential-process", "registry-auth"]) .replace_crates_io(server.index_url()) @@ -354,7 +385,6 @@ fn all_not_found() { Caused by: no token found, please run `cargo login` - or use environment variable CARGO_REGISTRY_TOKEN "#, ) .run();