From 5321146c7e4c733ff6e443a170065f5d29fe6e5c Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Mon, 31 Jul 2023 13:41:47 -0500 Subject: [PATCH 1/4] Add serde(default) to cargo-credential RegistryInfo headers --- credential/cargo-credential/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/credential/cargo-credential/src/lib.rs b/credential/cargo-credential/src/lib.rs index 3bae2f40a50..1ee049c67d9 100644 --- a/credential/cargo-credential/src/lib.rs +++ b/credential/cargo-credential/src/lib.rs @@ -63,7 +63,7 @@ pub struct RegistryInfo<'a> { /// The crates.io registry will be `crates-io` (`CRATES_IO_REGISTRY`). pub name: Option<&'a str>, /// Headers from attempting to access a registry that resulted in a HTTP 401. - #[serde(skip_serializing_if = "Vec::is_empty")] + #[serde(skip_serializing_if = "Vec::is_empty", default)] pub headers: Vec, } From a81d5589416c0141302eb4f4ab1d471867552a1a Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Mon, 31 Jul 2023 11:33:06 -0500 Subject: [PATCH 2/4] Use thiserror for credential provider errors --- Cargo.lock | 2 + .../cargo-credential-1password/src/lib.rs | 2 +- .../src/lib.rs | 16 +- .../cargo-credential-wincred/src/lib.rs | 17 +- credential/cargo-credential/Cargo.toml | 2 + credential/cargo-credential/src/error.rs | 193 ++++++++++++++++++ credential/cargo-credential/src/lib.rs | 68 +----- src/cargo/util/credential/adaptor.rs | 23 +-- src/cargo/util/credential/paseto.rs | 27 +-- src/cargo/util/credential/process.rs | 13 +- src/cargo/util/credential/token.rs | 20 +- tests/testsuite/credential_process.rs | 6 +- tests/testsuite/login.rs | 6 +- 13 files changed, 251 insertions(+), 144 deletions(-) create mode 100644 credential/cargo-credential/src/error.rs diff --git a/Cargo.lock b/Cargo.lock index 55b5c6acb4b..125f6263400 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -347,8 +347,10 @@ dependencies = [ name = "cargo-credential" version = "0.3.0" dependencies = [ + "anyhow", "serde", "serde_json", + "thiserror", "time", ] diff --git a/credential/cargo-credential-1password/src/lib.rs b/credential/cargo-credential-1password/src/lib.rs index 7732dda6da2..b5116da8353 100644 --- a/credential/cargo-credential-1password/src/lib.rs +++ b/credential/cargo-credential-1password/src/lib.rs @@ -243,7 +243,7 @@ impl OnePasswordKeychain { Some(password) => password .value .map(Secret::from) - .ok_or_else(|| format!("missing password value for entry").into()), + .ok_or("missing password value for entry".into()), None => Err("could not find password field".into()), } } diff --git a/credential/cargo-credential-macos-keychain/src/lib.rs b/credential/cargo-credential-macos-keychain/src/lib.rs index d7d9da164d3..9e6d55472e0 100644 --- a/credential/cargo-credential-macos-keychain/src/lib.rs +++ b/credential/cargo-credential-macos-keychain/src/lib.rs @@ -17,10 +17,6 @@ mod macos { format!("cargo-registry:{}", index_url) } - fn to_credential_error(e: security_framework::base::Error) -> Error { - Error::Other(format!("security framework ({}): {e}", e.code())) - } - impl Credential for MacKeychain { fn perform( &self, @@ -34,11 +30,9 @@ mod macos { match action { Action::Get(_) => match keychain.find_generic_password(&service_name, ACCOUNT) { Err(e) if e.code() == not_found => Err(Error::NotFound), - Err(e) => Err(to_credential_error(e)), + Err(e) => Err(Box::new(e).into()), Ok((pass, _)) => { - let token = String::from_utf8(pass.as_ref().to_vec()).map_err(|_| { - Error::Other("failed to convert token to UTF8".to_string()) - })?; + let token = String::from_utf8(pass.as_ref().to_vec()).map_err(Box::new)?; Ok(CredentialResponse::Get { token: token.into(), cache: CacheControl::Session, @@ -57,19 +51,19 @@ mod macos { ACCOUNT, token.expose().as_bytes(), ) - .map_err(to_credential_error)?; + .map_err(Box::new)?; } } Ok((_, mut item)) => { item.set_password(token.expose().as_bytes()) - .map_err(to_credential_error)?; + .map_err(Box::new)?; } } Ok(CredentialResponse::Login) } Action::Logout => match keychain.find_generic_password(&service_name, ACCOUNT) { Err(e) if e.code() == not_found => Err(Error::NotFound), - Err(e) => Err(to_credential_error(e)), + Err(e) => Err(Box::new(e).into()), Ok((_, item)) => { item.delete(); Ok(CredentialResponse::Logout) diff --git a/credential/cargo-credential-wincred/src/lib.rs b/credential/cargo-credential-wincred/src/lib.rs index 3d67e4613cd..5f75a0f64a9 100644 --- a/credential/cargo-credential-wincred/src/lib.rs +++ b/credential/cargo-credential-wincred/src/lib.rs @@ -65,16 +65,13 @@ mod win { (*p_credential).CredentialBlobSize as usize, ) }; - let result = match String::from_utf8(bytes.to_vec()) { - Err(_) => Err("failed to convert token to UTF8".into()), - Ok(token) => Ok(CredentialResponse::Get { - token: token.into(), - cache: CacheControl::Session, - operation_independent: true, - }), - }; - let _ = unsafe { CredFree(p_credential as *mut _) }; - result + let token = String::from_utf8(bytes.to_vec()).map_err(Box::new); + unsafe { CredFree(p_credential as *mut _) }; + Ok(CredentialResponse::Get { + token: token?.into(), + cache: CacheControl::Session, + operation_independent: true, + }) } Action::Login(options) => { let token = read_token(options, registry)?.expose(); diff --git a/credential/cargo-credential/Cargo.toml b/credential/cargo-credential/Cargo.toml index ca24406b5cf..9ac9168ffe6 100644 --- a/credential/cargo-credential/Cargo.toml +++ b/credential/cargo-credential/Cargo.toml @@ -7,6 +7,8 @@ repository = "https://github.com/rust-lang/cargo" description = "A library to assist writing Cargo credential helpers." [dependencies] +anyhow.workspace = true serde = { workspace = true, features = ["derive"] } serde_json.workspace = true +thiserror.workspace = true time.workspace = true diff --git a/credential/cargo-credential/src/error.rs b/credential/cargo-credential/src/error.rs new file mode 100644 index 00000000000..39083cbcf38 --- /dev/null +++ b/credential/cargo-credential/src/error.rs @@ -0,0 +1,193 @@ +use serde::{Deserialize, Serialize}; +use std::error::Error as StdError; +use thiserror::Error as ThisError; + +/// Credential provider error type. +/// +/// `UrlNotSupported` and `NotFound` errors both cause Cargo +/// to attempt another provider, if one is available. The other +/// variants are fatal. +/// +/// Note: Do not add a tuple variant, as it cannot be serialized. +#[derive(Serialize, Deserialize, ThisError, Debug)] +#[serde(rename_all = "kebab-case", tag = "kind")] +#[non_exhaustive] +pub enum Error { + #[error("registry not supported")] + UrlNotSupported, + #[error("credential not found")] + NotFound, + #[error("requested operation not supported")] + OperationNotSupported, + #[error("protocol version {version} not supported")] + ProtocolNotSupported { version: u32 }, + #[error(transparent)] + #[serde(with = "error_serialize")] + Other(Box), +} + +impl From for Error { + fn from(err: std::io::Error) -> Self { + Box::new(err).into() + } +} + +impl From for Error { + fn from(err: serde_json::Error) -> Self { + Box::new(err).into() + } +} + +impl From for Error { + fn from(err: String) -> Self { + Box::new(StringTypedError { + message: err.to_string(), + source: None, + }) + .into() + } +} + +impl From<&str> for Error { + fn from(err: &str) -> Self { + err.to_string().into() + } +} + +impl From for Error { + fn from(value: anyhow::Error) -> Self { + let mut prev = None; + for e in value.chain().rev() { + prev = Some(Box::new(StringTypedError { + message: e.to_string(), + source: prev, + })); + } + Error::Other(prev.unwrap()) + } +} + +impl From> for Error { + fn from(value: Box) -> Self { + Error::Other(value) + } +} + +/// String-based error type with an optional source +#[derive(Debug)] +struct StringTypedError { + message: String, + source: Option>, +} + +impl StdError for StringTypedError { + fn source(&self) -> Option<&(dyn StdError + 'static)> { + self.source.as_ref().map(|err| err as &dyn StdError) + } +} + +impl std::fmt::Display for StringTypedError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.message.fmt(f) + } +} + +/// Serializer / deserializer for any boxed error. +/// The string representation of the error, and its `source` chain can roundtrip across +/// the serialization. The actual types are lost (downcast will not work). +mod error_serialize { + use std::error::Error as StdError; + use std::ops::Deref; + + use serde::{ser::SerializeStruct, Deserialize, Deserializer, Serializer}; + + use crate::error::StringTypedError; + + pub fn serialize( + e: &Box, + serializer: S, + ) -> Result + where + S: Serializer, + { + let mut state = serializer.serialize_struct("StringTypedError", 2)?; + state.serialize_field("message", &format!("{}", e))?; + + // Serialize the source error chain recursively + let mut current_source: &dyn StdError = e.deref(); + let mut sources = Vec::new(); + while let Some(err) = current_source.source() { + sources.push(err.to_string()); + current_source = err; + } + state.serialize_field("caused-by", &sources)?; + state.end() + } + + pub fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> + where + D: Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(rename_all = "kebab-case")] + struct ErrorData { + message: String, + caused_by: Option>, + } + let data = ErrorData::deserialize(deserializer)?; + let mut prev = None; + if let Some(source) = data.caused_by { + for e in source.into_iter().rev() { + prev = Some(Box::new(StringTypedError { + message: e, + source: prev, + })); + } + } + let e = Box::new(StringTypedError { + message: data.message, + source: prev, + }); + Ok(e) + } +} + +#[cfg(test)] +mod tests { + use super::Error; + + #[test] + pub fn roundtrip() { + // Construct an error with context + let e = anyhow::anyhow!("E1").context("E2").context("E3"); + // Convert to a string with contexts. + let s1 = format!("{:?}", e); + // Convert the error into an `Error` + let e: Error = e.into(); + // Convert that error into JSON + let json = serde_json::to_string_pretty(&e).unwrap(); + // Convert that error back to anyhow + let e: anyhow::Error = e.into(); + let s2 = format!("{:?}", e); + assert_eq!(s1, s2); + + // Convert the error back from JSON + let e: Error = serde_json::from_str(&json).unwrap(); + // Convert to back to anyhow + let e: anyhow::Error = e.into(); + let s3 = format!("{:?}", e); + assert_eq!(s2, s3); + + assert_eq!( + r#"{ + "kind": "other", + "message": "E3", + "caused-by": [ + "E2", + "E1" + ] +}"#, + json + ); + } +} diff --git a/credential/cargo-credential/src/lib.rs b/credential/cargo-credential/src/lib.rs index 1ee049c67d9..9e2929452f5 100644 --- a/credential/cargo-credential/src/lib.rs +++ b/credential/cargo-credential/src/lib.rs @@ -17,7 +17,9 @@ use std::{ }; use time::OffsetDateTime; +mod error; mod secret; +pub use error::Error; pub use secret::Secret; /// Message sent by the credential helper on startup @@ -163,70 +165,6 @@ pub enum CacheControl { /// this version will prevent new credential providers /// from working with older versions of Cargo. pub const PROTOCOL_VERSION_1: u32 = 1; - -#[derive(Serialize, Deserialize, Clone, Debug)] -#[serde(rename_all = "kebab-case", tag = "kind", content = "detail")] -#[non_exhaustive] -pub enum Error { - UrlNotSupported, - ProtocolNotSupported(u32), - Subprocess(String), - Io(String), - Serde(String), - Other(String), - OperationNotSupported, - NotFound, -} - -impl From for Error { - fn from(err: serde_json::Error) -> Self { - Error::Serde(err.to_string()) - } -} - -impl From for Error { - fn from(err: std::io::Error) -> Self { - Error::Io(err.to_string()) - } -} - -impl From for Error { - fn from(err: String) -> Self { - Error::Other(err) - } -} - -impl From<&str> for Error { - fn from(err: &str) -> Self { - Error::Other(err.to_string()) - } -} - -impl std::error::Error for Error {} - -impl core::fmt::Display for Error { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Error::UrlNotSupported => { - write!(f, "credential provider does not support this registry") - } - Error::ProtocolNotSupported(v) => write!( - f, - "credential provider does not support protocol version {v}" - ), - Error::Io(msg) => write!(f, "i/o error: {msg}"), - Error::Serde(msg) => write!(f, "serialization error: {msg}"), - Error::Other(msg) => write!(f, "error: {msg}"), - Error::Subprocess(msg) => write!(f, "subprocess failed: {msg}"), - Error::OperationNotSupported => write!( - f, - "credential provider does not support the requested operation" - ), - Error::NotFound => write!(f, "credential not found"), - } - } -} - pub trait Credential { /// Retrieves a token for the given registry. fn perform( @@ -262,7 +200,7 @@ fn doit(credential: impl Credential) -> Result<(), Error> { } let request: CredentialRequest = serde_json::from_str(&buffer)?; if request.v != PROTOCOL_VERSION_1 { - return Err(Error::ProtocolNotSupported(request.v)); + return Err(Error::ProtocolNotSupported { version: request.v }); } serde_json::to_writer( std::io::stdout(), diff --git a/src/cargo/util/credential/adaptor.rs b/src/cargo/util/credential/adaptor.rs index 5d8f7931322..1a71fea9a00 100644 --- a/src/cargo/util/credential/adaptor.rs +++ b/src/cargo/util/credential/adaptor.rs @@ -22,7 +22,7 @@ impl Credential for BasicProcessCredential { Action::Get(_) => { let mut args = args.iter(); let exe = args.next() - .ok_or_else(||cargo_credential::Error::Other(format!("The first argument to the `cargo:basic` adaptor must be the path to the credential provider executable.")))?; + .ok_or("The first argument to the `cargo:basic` adaptor must be the path to the credential provider executable.")?; let args = args.map(|arg| arg.replace("{index_url}", registry.index_url)); let mut cmd = Command::new(exe); @@ -32,32 +32,23 @@ impl Credential for BasicProcessCredential { cmd.env("CARGO_REGISTRY_NAME_OPT", name); } cmd.stdout(Stdio::piped()); - let mut child = cmd - .spawn() - .map_err(|e| cargo_credential::Error::Subprocess(e.to_string()))?; + let mut child = cmd.spawn()?; let mut buffer = String::new(); - child - .stdout - .take() - .unwrap() - .read_to_string(&mut buffer) - .map_err(|e| cargo_credential::Error::Subprocess(e.to_string()))?; + child.stdout.take().unwrap().read_to_string(&mut buffer)?; if let Some(end) = buffer.find('\n') { if buffer.len() > end + 1 { - return Err(cargo_credential::Error::Other(format!( + return Err(format!( "process `{}` returned more than one line of output; \ expected a single token", exe - ))); + ) + .into()); } buffer.truncate(end); } let status = child.wait().expect("process was started"); if !status.success() { - return Err(cargo_credential::Error::Subprocess(format!( - "process `{}` failed with status `{status}`", - exe - ))); + return Err(format!("process `{}` failed with status `{status}`", exe).into()); } Ok(CredentialResponse::Get { token: Secret::from(buffer), diff --git a/src/cargo/util/credential/paseto.rs b/src/cargo/util/credential/paseto.rs index 39a47100188..0eef3f1403e 100644 --- a/src/cargo/util/credential/paseto.rs +++ b/src/cargo/util/credential/paseto.rs @@ -1,5 +1,6 @@ //! Credential provider that implements PASETO asymmetric tokens stored in Cargo's config. +use anyhow::Context; use cargo_credential::{ Action, CacheControl, Credential, CredentialResponse, Error, Operation, RegistryInfo, Secret, }; @@ -61,16 +62,14 @@ impl<'a> Credential for PasetoCredential<'a> { action: &Action<'_>, _args: &[&str], ) -> Result { - let index_url = Url::parse(registry.index_url).map_err(|e| e.to_string())?; + let index_url = Url::parse(registry.index_url).context("parsing index url")?; let sid = if let Some(name) = registry.name { SourceId::for_alt_registry(&index_url, name) } else { SourceId::for_registry(&index_url) - } - .map_err(|e| e.to_string())?; + }?; - let reg_cfg = registry_credential_config_raw(self.config, &sid) - .map_err(|e| Error::Other(e.to_string()))?; + let reg_cfg = registry_credential_config_raw(self.config, &sid)?; match action { Action::Get(operation) => { @@ -87,14 +86,12 @@ impl<'a> Credential for PasetoCredential<'a> { .as_ref() .map(|key| key.as_str().try_into()) .transpose() - .map_err(|e| Error::Other(format!("failed to load private key: {e}")))?; + .context("failed to load private key")?; let public: AsymmetricPublicKey = secret .as_ref() .map(|key| key.try_into()) .transpose() - .map_err(|e| { - Error::Other(format!("failed to load public key from private key: {e}")) - })? + .context("failed to load public key from private key")? .expose(); let kip: pasetors::paserk::Id = (&public).into(); @@ -157,7 +154,7 @@ impl<'a> Credential for PasetoCredential<'a> { ) }) .transpose() - .map_err(|e| Error::Other(format!("failed to sign request: {e}")))?; + .context("failed to sign request")?; Ok(CredentialResponse::Get { token, @@ -181,18 +178,14 @@ impl<'a> Credential for PasetoCredential<'a> { if let Some(p) = paserk_public_from_paserk_secret(secret_key.as_deref()) { eprintln!("{}", &p); } else { - return Err(Error::Other( - "not a validly formatted PASERK secret key".to_string(), - )); + return Err("not a validly formatted PASERK secret key".into()); } new_token = RegistryCredentialConfig::AsymmetricKey((secret_key, None)); - config::save_credentials(self.config, Some(new_token), &sid) - .map_err(|e| Error::Other(e.to_string()))?; + config::save_credentials(self.config, Some(new_token), &sid)?; Ok(CredentialResponse::Login) } Action::Logout => { - config::save_credentials(self.config, None, &sid) - .map_err(|e| Error::Other(e.to_string()))?; + config::save_credentials(self.config, None, &sid)?; Ok(CredentialResponse::Logout) } _ => Err(Error::OperationNotSupported), diff --git a/src/cargo/util/credential/process.rs b/src/cargo/util/credential/process.rs index aafb2574ea8..1e1fc37c71c 100644 --- a/src/cargo/util/credential/process.rs +++ b/src/cargo/util/credential/process.rs @@ -7,6 +7,7 @@ use std::{ process::{Command, Stdio}, }; +use anyhow::Context; use cargo_credential::{ Action, Credential, CredentialHello, CredentialRequest, CredentialResponse, RegistryInfo, }; @@ -35,11 +36,11 @@ impl<'a> Credential for CredentialProcessCredential { cmd.stdin(Stdio::piped()); cmd.arg("--cargo-plugin"); log::debug!("credential-process: {cmd:?}"); - let mut child = cmd.spawn().map_err(|e| { - cargo_credential::Error::Subprocess(format!( - "failed to spawn credential process `{}`: {e}", + let mut child = cmd.spawn().with_context(|| { + format!( + "failed to spawn credential process `{}`", self.path.display() - )) + ) })?; let mut output_from_child = BufReader::new(child.stdout.take().unwrap()); let mut input_to_child = child.stdin.take().unwrap(); @@ -66,11 +67,11 @@ impl<'a> Credential for CredentialProcessCredential { drop(input_to_child); let status = child.wait().expect("credential process never started"); if !status.success() { - return Err(cargo_credential::Error::Subprocess(format!( + return Err(anyhow::anyhow!( "credential process `{}` failed with status {}`", self.path.display(), status - )) + ) .into()); } log::trace!("credential process exited successfully"); diff --git a/src/cargo/util/credential/token.rs b/src/cargo/util/credential/token.rs index 7cd6e1e3503..7a29e6360d9 100644 --- a/src/cargo/util/credential/token.rs +++ b/src/cargo/util/credential/token.rs @@ -1,5 +1,6 @@ //! Credential provider that uses plaintext tokens in Cargo's config. +use anyhow::Context; use cargo_credential::{Action, CacheControl, Credential, CredentialResponse, Error, RegistryInfo}; use url::Url; @@ -27,16 +28,14 @@ impl<'a> Credential for TokenCredential<'a> { action: &Action<'_>, _args: &[&str], ) -> Result { - let index_url = Url::parse(registry.index_url).map_err(|e| e.to_string())?; + let index_url = Url::parse(registry.index_url).context("parsing index url")?; let sid = if let Some(name) = registry.name { SourceId::for_alt_registry(&index_url, name) } else { SourceId::for_registry(&index_url) - } - .map_err(|e| e.to_string())?; - let previous_token = registry_credential_config_raw(self.config, &sid) - .map_err(|e| Error::Other(e.to_string()))? - .and_then(|c| c.token); + }?; + let previous_token = + registry_credential_config_raw(self.config, &sid)?.and_then(|c| c.token); match action { Action::Get(_) => { @@ -53,14 +52,12 @@ impl<'a> Credential for TokenCredential<'a> { let new_token = cargo_credential::read_token(options, registry)? .map(|line| line.replace("cargo login", "").trim().to_string()); - crates_io::check_token(new_token.as_ref().expose()) - .map_err(|e| Error::Other(e.to_string()))?; + crates_io::check_token(new_token.as_ref().expose()).map_err(Box::new)?; config::save_credentials( self.config, Some(RegistryCredentialConfig::Token(new_token)), &sid, - ) - .map_err(|e| Error::Other(e.to_string()))?; + )?; let _ = self.config.shell().status( "Login", format!("token for `{}` saved", sid.display_registry_name()), @@ -72,8 +69,7 @@ impl<'a> Credential for TokenCredential<'a> { return Err(Error::NotFound); } let reg_name = sid.display_registry_name(); - config::save_credentials(self.config, None, &sid) - .map_err(|e| Error::Other(e.to_string()))?; + config::save_credentials(self.config, None, &sid)?; let _ = self.config.shell().status( "Logout", format!("token for `{reg_name}` has been removed from local storage"), diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index c04ddcf421d..cae453f9bce 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -161,7 +161,7 @@ fn basic_unsupported() { [ERROR] credential provider `cargo:basic false` failed action `login` Caused by: - credential provider does not support the requested operation + requested operation not supported ", ) .run(); @@ -175,7 +175,7 @@ Caused by: [ERROR] credential provider `cargo:basic false` failed action `logout` Caused by: - credential provider does not support the requested operation + requested operation not supported ", ) .run(); @@ -280,7 +280,7 @@ fn invalid_token_output() { [ERROR] credential provider `[..]test-cred[EXE]` failed action `get` Caused by: - error: process `[..]` returned more than one line of output; expected a single token + process `[..]` returned more than one line of output; expected a single token ", ) .run(); diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index ec4d6b8b6f0..ee6bc0873de 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -116,7 +116,7 @@ fn empty_login_token() { [ERROR] credential provider `cargo:token` failed action `login` Caused by: - [ERROR] please provide a non-empty token + please provide a non-empty token ", ) .with_status(101) @@ -130,7 +130,7 @@ Caused by: [ERROR] credential provider `cargo:token` failed action `login` Caused by: - [ERROR] please provide a non-empty token + please provide a non-empty token ", ) .with_status(101) @@ -160,7 +160,7 @@ fn invalid_login_token() { "[ERROR] credential provider `cargo:token` failed action `login` Caused by: - [ERROR] token contains invalid characters. + token contains invalid characters. Only printable ISO-8859-1 characters are allowed as it is sent in a HTTPS header.", 101, ) From 7918c7fc7b5f3c3a2728adb66641c8339cfbdee8 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Mon, 31 Jul 2023 16:01:20 -0500 Subject: [PATCH 3/4] Remove from impls --- .../cargo-credential-1password/src/lib.rs | 4 +- .../cargo-credential-wincred/src/lib.rs | 6 +-- credential/cargo-credential/src/error.rs | 39 ++++++++++++------- credential/cargo-credential/src/lib.rs | 13 ++++--- src/cargo/util/credential/adaptor.rs | 12 ++++-- src/cargo/util/credential/process.rs | 26 ++++++------- 6 files changed, 60 insertions(+), 40 deletions(-) diff --git a/credential/cargo-credential-1password/src/lib.rs b/credential/cargo-credential-1password/src/lib.rs index b5116da8353..76e01cb794f 100644 --- a/credential/cargo-credential-1password/src/lib.rs +++ b/credential/cargo-credential-1password/src/lib.rs @@ -80,7 +80,7 @@ impl OnePasswordKeychain { let mut cmd = Command::new("op"); cmd.args(["signin", "--raw"]); cmd.stdout(Stdio::piped()); - cmd.stdin(cargo_credential::tty()?); + cmd.stdin(cargo_credential::tty().map_err(Box::new)?); let mut child = cmd .spawn() .map_err(|e| format!("failed to spawn `op`: {}", e))?; @@ -228,7 +228,7 @@ impl OnePasswordKeychain { // For unknown reasons, `op item create` seems to not be happy if // stdin is not a tty. Otherwise it returns with a 0 exit code without // doing anything. - cmd.stdin(cargo_credential::tty()?); + cmd.stdin(cargo_credential::tty().map_err(Box::new)?); self.run_cmd(cmd)?; Ok(()) } diff --git a/credential/cargo-credential-wincred/src/lib.rs b/credential/cargo-credential-wincred/src/lib.rs index 5f75a0f64a9..9200ca58fd6 100644 --- a/credential/cargo-credential-wincred/src/lib.rs +++ b/credential/cargo-credential-wincred/src/lib.rs @@ -58,7 +58,7 @@ mod win { if err.raw_os_error() == Some(ERROR_NOT_FOUND as i32) { return Err(Error::NotFound); } - return Err(err.into()); + return Err(Box::new(err).into()); } std::slice::from_raw_parts( (*p_credential).CredentialBlob, @@ -97,7 +97,7 @@ mod win { let result = unsafe { CredWriteW(&credential, 0) }; if result != TRUE { let err = std::io::Error::last_os_error(); - return Err(err.into()); + return Err(Box::new(err).into()); } Ok(CredentialResponse::Login) } @@ -109,7 +109,7 @@ mod win { if err.raw_os_error() == Some(ERROR_NOT_FOUND as i32) { return Err(Error::NotFound); } - return Err(err.into()); + return Err(Box::new(err).into()); } Ok(CredentialResponse::Logout) } diff --git a/credential/cargo-credential/src/error.rs b/credential/cargo-credential/src/error.rs index 39083cbcf38..2ebaf99777b 100644 --- a/credential/cargo-credential/src/error.rs +++ b/credential/cargo-credential/src/error.rs @@ -13,29 +13,32 @@ use thiserror::Error as ThisError; #[serde(rename_all = "kebab-case", tag = "kind")] #[non_exhaustive] pub enum Error { + /// Registry URL is not supported. This should be used if + /// the provider only works for some registries. Cargo will + /// try another provider, if available #[error("registry not supported")] UrlNotSupported, + + /// Credentials could not be found. Cargo will try another + /// provider, if available #[error("credential not found")] NotFound, + + /// The provider doesn't support this operation, such as + /// a provider that can't support 'login' / 'logout' #[error("requested operation not supported")] OperationNotSupported, - #[error("protocol version {version} not supported")] - ProtocolNotSupported { version: u32 }, + + /// The provider failed to perform the operation. Other + /// providers will not be attempted #[error(transparent)] #[serde(with = "error_serialize")] Other(Box), -} - -impl From for Error { - fn from(err: std::io::Error) -> Self { - Box::new(err).into() - } -} -impl From for Error { - fn from(err: serde_json::Error) -> Self { - Box::new(err).into() - } + /// A new variant was added to this enum since Cargo was built + #[error("unknown error kind; try updating Cargo?")] + #[serde(other)] + Unknown, } impl From for Error { @@ -156,6 +159,16 @@ mod error_serialize { mod tests { use super::Error; + #[test] + pub fn unknown_kind() { + let json = r#"{ + "kind": "unexpected-kind", + "unexpected-content": "test" + }"#; + let e: Error = serde_json::from_str(&json).unwrap(); + assert!(matches!(e, Error::Unknown)); + } + #[test] pub fn roundtrip() { // Construct an error with context diff --git a/credential/cargo-credential/src/lib.rs b/credential/cargo-credential/src/lib.rs index 9e2929452f5..138eca7ed2b 100644 --- a/credential/cargo-credential/src/lib.rs +++ b/credential/cargo-credential/src/lib.rs @@ -189,23 +189,24 @@ fn doit(credential: impl Credential) -> Result<(), Error> { let hello = CredentialHello { v: vec![PROTOCOL_VERSION_1], }; - serde_json::to_writer(std::io::stdout(), &hello)?; + serde_json::to_writer(std::io::stdout(), &hello).map_err(Box::new)?; println!(); loop { let mut buffer = String::new(); - let len = std::io::stdin().read_line(&mut buffer)?; + let len = std::io::stdin().read_line(&mut buffer).map_err(Box::new)?; if len == 0 { return Ok(()); } - let request: CredentialRequest = serde_json::from_str(&buffer)?; + let request: CredentialRequest = serde_json::from_str(&buffer).map_err(Box::new)?; if request.v != PROTOCOL_VERSION_1 { - return Err(Error::ProtocolNotSupported { version: request.v }); + return Err(format!("unsupported protocol version {}", request.v).into()); } serde_json::to_writer( std::io::stdout(), &credential.perform(&request.registry, &request.action, &request.args), - )?; + ) + .map_err(Box::new)?; println!(); } } @@ -248,5 +249,5 @@ pub fn read_token( eprintln!("please paste the token for {} below", registry.index_url); } - Ok(Secret::from(read_line()?)) + Ok(Secret::from(read_line().map_err(Box::new)?)) } diff --git a/src/cargo/util/credential/adaptor.rs b/src/cargo/util/credential/adaptor.rs index 1a71fea9a00..d9cbc6ffa14 100644 --- a/src/cargo/util/credential/adaptor.rs +++ b/src/cargo/util/credential/adaptor.rs @@ -5,6 +5,7 @@ use std::{ process::{Command, Stdio}, }; +use anyhow::Context; use cargo_credential::{ Action, CacheControl, Credential, CredentialResponse, RegistryInfo, Secret, }; @@ -32,9 +33,14 @@ impl Credential for BasicProcessCredential { cmd.env("CARGO_REGISTRY_NAME_OPT", name); } cmd.stdout(Stdio::piped()); - let mut child = cmd.spawn()?; + let mut child = cmd.spawn().context("failed to spawn credential process")?; let mut buffer = String::new(); - child.stdout.take().unwrap().read_to_string(&mut buffer)?; + child + .stdout + .take() + .unwrap() + .read_to_string(&mut buffer) + .context("failed to read from credential provider")?; if let Some(end) = buffer.find('\n') { if buffer.len() > end + 1 { return Err(format!( @@ -46,7 +52,7 @@ impl Credential for BasicProcessCredential { } buffer.truncate(end); } - let status = child.wait().expect("process was started"); + let status = child.wait().context("credential process never started")?; if !status.success() { return Err(format!("process `{}` failed with status `{status}`", exe).into()); } diff --git a/src/cargo/util/credential/process.rs b/src/cargo/util/credential/process.rs index 1e1fc37c71c..07551aee303 100644 --- a/src/cargo/util/credential/process.rs +++ b/src/cargo/util/credential/process.rs @@ -36,17 +36,15 @@ impl<'a> Credential for CredentialProcessCredential { cmd.stdin(Stdio::piped()); cmd.arg("--cargo-plugin"); log::debug!("credential-process: {cmd:?}"); - let mut child = cmd.spawn().with_context(|| { - format!( - "failed to spawn credential process `{}`", - self.path.display() - ) - })?; + let mut child = cmd.spawn().context("failed to spawn credential process")?; let mut output_from_child = BufReader::new(child.stdout.take().unwrap()); let mut input_to_child = child.stdin.take().unwrap(); let mut buffer = String::new(); - output_from_child.read_line(&mut buffer)?; - let credential_hello: CredentialHello = serde_json::from_str(&buffer)?; + output_from_child + .read_line(&mut buffer) + .context("failed to read hello from credential provider")?; + let credential_hello: CredentialHello = + serde_json::from_str(&buffer).context("failed to deserialize hello")?; log::debug!("credential-process > {credential_hello:?}"); let req = CredentialRequest { @@ -55,17 +53,19 @@ impl<'a> Credential for CredentialProcessCredential { registry: registry.clone(), args: args.to_vec(), }; - let request = serde_json::to_string(&req)?; + let request = serde_json::to_string(&req).context("failed to serialize request")?; log::debug!("credential-process < {req:?}"); - writeln!(input_to_child, "{request}")?; + writeln!(input_to_child, "{request}").context("failed to write to credential provider")?; buffer.clear(); - output_from_child.read_line(&mut buffer)?; + output_from_child + .read_line(&mut buffer) + .context("failed to read response from credential provider")?; let response: Result = - serde_json::from_str(&buffer)?; + serde_json::from_str(&buffer).context("failed to deserialize response")?; log::debug!("credential-process > {response:?}"); drop(input_to_child); - let status = child.wait().expect("credential process never started"); + let status = child.wait().context("credential process never started")?; if !status.success() { return Err(anyhow::anyhow!( "credential process `{}` failed with status {}`", From 70b584e4019abc74e9bc9316ccad4ffcb01f4a02 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Mon, 31 Jul 2023 16:30:38 -0500 Subject: [PATCH 4/4] Add serde(other) to credential protocol enums for future proofing --- credential/cargo-credential/src/lib.rs | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/credential/cargo-credential/src/lib.rs b/credential/cargo-credential/src/lib.rs index 138eca7ed2b..bbd437617aa 100644 --- a/credential/cargo-credential/src/lib.rs +++ b/credential/cargo-credential/src/lib.rs @@ -77,6 +77,8 @@ pub enum Action<'a> { Get(Operation<'a>), Login(LoginOptions<'a>), Logout, + #[serde(other)] + Unknown, } impl<'a> Display for Action<'a> { @@ -85,6 +87,7 @@ impl<'a> Display for Action<'a> { Action::Get(_) => f.write_str("get"), Action::Login(_) => f.write_str("login"), Action::Logout => f.write_str("logout"), + Action::Unknown => f.write_str(""), } } } @@ -133,6 +136,8 @@ pub enum Operation<'a> { /// The name of the crate name: &'a str, }, + #[serde(other)] + Unknown, } /// Message sent by the credential helper @@ -147,6 +152,8 @@ pub enum CredentialResponse { }, Login, Logout, + #[serde(other)] + Unknown, } #[derive(Serialize, Deserialize, Clone, Debug)] @@ -159,6 +166,8 @@ pub enum CacheControl { Expires(#[serde(with = "time::serde::timestamp")] OffsetDateTime), /// Cache this result and use it for all subsequent requests in the current Cargo invocation. Session, + #[serde(other)] + Unknown, } /// Credential process JSON protocol version. Incrementing @@ -177,7 +186,7 @@ pub trait Credential { /// Runs the credential interaction pub fn main(credential: impl Credential) { - let result = doit(credential); + let result = doit(credential).map_err(|e| Error::Other(e)); if result.is_err() { serde_json::to_writer(std::io::stdout(), &result) .expect("failed to serialize credential provider error"); @@ -185,28 +194,29 @@ pub fn main(credential: impl Credential) { } } -fn doit(credential: impl Credential) -> Result<(), Error> { +fn doit( + credential: impl Credential, +) -> Result<(), Box> { let hello = CredentialHello { v: vec![PROTOCOL_VERSION_1], }; - serde_json::to_writer(std::io::stdout(), &hello).map_err(Box::new)?; + serde_json::to_writer(std::io::stdout(), &hello)?; println!(); loop { let mut buffer = String::new(); - let len = std::io::stdin().read_line(&mut buffer).map_err(Box::new)?; + let len = std::io::stdin().read_line(&mut buffer)?; if len == 0 { return Ok(()); } - let request: CredentialRequest = serde_json::from_str(&buffer).map_err(Box::new)?; + let request: CredentialRequest = serde_json::from_str(&buffer)?; if request.v != PROTOCOL_VERSION_1 { return Err(format!("unsupported protocol version {}", request.v).into()); } serde_json::to_writer( std::io::stdout(), &credential.perform(&request.registry, &request.action, &request.args), - ) - .map_err(Box::new)?; + )?; println!(); } }