Skip to content

Commit

Permalink
fix: don't print _TOKEN suggestion when not applicable
Browse files Browse the repository at this point in the history 
Cargo should not suggest the _TOKEN environment variable
when the cargo:token provider isn't available
  • Loading branch information
@arlosi
arlosi committed Sep 8, 2023
1 parent 5c77b5d commit 803fd69
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 33 deletions.
13 changes: 7 additions & 6 deletions src/cargo/sources/registry/http_remote.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
93 changes: 67 additions & 26 deletions src/cargo/util/auth/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,19 @@ impl RegistryConfigExtended {
}

/// Get the list of credential providers for a registry source.
fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<String>>> {
fn credential_provider(
config: &Config,
sid: &SourceId,
show_warnings: bool,
) -> CargoResult<Vec<Vec<String>>> {
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 {
Expand Down Expand Up @@ -111,7 +123,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<S
let provider = resolve_credential_alias(config, provider);
if let Some(token) = token {
if provider[0] != "cargo:token" {
config.shell().warn(format!(
warn(format!(
"{sid} has a token configured in {} that will be ignored \
because this registry is configured to use credential-provider `{}`",
token.definition, provider[0],
Expand All @@ -120,7 +132,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<S
}
if let Some(secret_key) = secret_key {
if provider[0] != "cargo:paseto" {
config.shell().warn(format!(
warn(format!(
"{sid} has a secret-key configured in {} that will be ignored \
because this registry is configured to use credential-provider `{}`",
secret_key.definition, provider[0],
Expand All @@ -145,14 +157,14 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<S
match (token_pos, paseto_pos) {
(Some(token_pos), Some(paseto_pos)) => {
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))?;
}
Expand All @@ -172,7 +184,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<S
.iter()
.any(|p| p.first().map(String::as_str) == Some("cargo:token"))
{
config.shell().warn(format!(
warn(format!(
"{sid} has a token configured in {} that will be ignored \
because the `cargo:token` credential provider is not listed in \
`registry.global-credential-providers`",
Expand All @@ -191,7 +203,7 @@ fn credential_provider(config: &Config, sid: &SourceId) -> CargoResult<Vec<Vec<S
.iter()
.any(|p| p.first().map(String::as_str) == Some("cargo:paseto"))
{
config.shell().warn(format!(
warn(format!(
"{sid} has a secret-key configured in {} that will be ignored \
because the `cargo:paseto` credential provider is not listed in \
`registry.global-credential-providers`",
Expand Down Expand Up @@ -360,14 +372,40 @@ impl fmt::Display for AuthorizationErrorReason {
#[derive(Debug)]
pub struct AuthorizationError {
/// Url that was attempted
pub sid: SourceId,
sid: SourceId,
/// The `registry.default` config value.
pub default_registry: Option<String>,
default_registry: Option<String>,
/// Url where the user could log in.
pub login_url: Option<Url>,
/// 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<Url>,
reason: AuthorizationErrorReason,
) -> CargoResult<Self> {
// 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 {
Expand All @@ -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,
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -511,12 +552,12 @@ pub fn auth_token(
) -> CargoResult<String> {
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()),
}
}
Expand Down
32 changes: 31 additions & 1 deletion tests/testsuite/credential_process.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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())
Expand All @@ -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();
Expand Down

0 comments on commit 803fd69

Please sign in to comment.