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.

Sign up for GitHub

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

fix: don't print _TOKEN suggestion when not applicable #12644

Merged
merged 1 commit into from
Sep 8, 2023
Merged

fix: don't print _TOKEN suggestion when not applicable #12644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions src/cargo/sources/registry/http_remote.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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