Use thiserror for credential provider errors

Cargo.lock

@@ -347,8 +347,10 @@ dependencies = [
 name = "cargo-credential"
 version = "0.3.0"
 dependencies = [
+ "anyhow",
  "serde",
  "serde_json",
+ "thiserror",
  "time",
 ]
 

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

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)

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

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

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<dyn StdError + Sync + Send>),
+}
+
+impl From<std::io::Error> for Error {
+    fn from(err: std::io::Error) -> Self {
+        Box::new(err).into()
+    }
+}
+
+impl From<serde_json::Error> for Error {
+    fn from(err: serde_json::Error) -> Self {
+        Box::new(err).into()
+    }
+}
+
+impl From<String> 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<anyhow::Error> 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<T: StdError + Send + Sync + 'static> From<Box<T>> for Error {
+    fn from(value: Box<T>) -> Self {
+        Error::Other(value)
+    }
+}
+
+/// String-based error type with an optional source
+#[derive(Debug)]
+struct StringTypedError {
+    message: String,
+    source: Option<Box<StringTypedError>>,
+}
+
+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<S>(
+        e: &Box<dyn StdError + Send + Sync>,
+        serializer: S,
+    ) -> Result<S::Ok, S::Error>
+    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<Box<dyn StdError + Sync + Send>, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        #[serde(rename_all = "kebab-case")]
+        struct ErrorData {
+            message: String,
+            caused_by: Option<Vec<String>>,
+        }
+        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
+        );
+    }
+}

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<serde_json::Error> for Error {
-    fn from(err: serde_json::Error) -> Self {
-        Error::Serde(err.to_string())
-    }
-}
-
-impl From<std::io::Error> for Error {
-    fn from(err: std::io::Error) -> Self {
-        Error::Io(err.to_string())
-    }
-}
-
-impl From<String> 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(),

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),

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<CredentialResponse, Error> {
-        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<pasetors::version3::V3> = 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),

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

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<CredentialResponse, Error> {
-        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"),

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

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,
         )