Skip to content

Commit

Permalink
Auto merge of #12424 - arlosi:credential-thiserror, r=arlosi
Browse files Browse the repository at this point in the history
Use thiserror for credential provider errors

### What does this PR try to resolve?
Errors from credential providers currently must a single string. This leads to a lot of `.map_err(|e|cargo_credential::Error::Other(e.to_string())`, which loses the `source()` of these errors.

This changes the `cargo_credential::Error` to use `thiserror` and adds a custom serialization for `std::error::Error` that preserves the source error chain across serialization / deserialization.

A unit test is added to verify serialization / deserialization.
  • Loading branch information
bors committed Jul 31, 2023
2 parents 8d1d20d + 70b584e commit 29a6f2f
Show file tree
Hide file tree
Showing 13 changed files with 295 additions and 158 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions credential/cargo-credential-1password/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))?;
Expand Down Expand Up @@ -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(())
}
Expand All @@ -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()),
}
}
Expand Down
16 changes: 5 additions & 11 deletions credential/cargo-credential-macos-keychain/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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)
Expand Down
23 changes: 10 additions & 13 deletions credential/cargo-credential-wincred/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,23 +58,20 @@ 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,
(*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();
Expand All @@ -100,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)
}
Expand All @@ -112,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)
}
Expand Down
2 changes: 2 additions & 0 deletions credential/cargo-credential/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
206 changes: 206 additions & 0 deletions credential/cargo-credential/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
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 {
/// 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,

/// The provider failed to perform the operation. Other
/// providers will not be attempted
#[error(transparent)]
#[serde(with = "error_serialize")]
Other(Box<dyn StdError + Sync + Send>),

/// A new variant was added to this enum since Cargo was built
#[error("unknown error kind; try updating Cargo?")]
#[serde(other)]
Unknown,
}

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 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
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
);
}
}
Loading

0 comments on commit 29a6f2f

Please sign in to comment.