Skip to content

Commit 29a6f2f

Browse files
committed
Auto merge of #12424 - arlosi:credential-thiserror, r=arlosi
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.
2 parents 8d1d20d + 70b584e commit 29a6f2f

File tree

13 files changed

+295
-158
lines changed

13 files changed

+295
-158
lines changed

Cargo.lock

+2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

credential/cargo-credential-1password/src/lib.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ impl OnePasswordKeychain {
8080
let mut cmd = Command::new("op");
8181
cmd.args(["signin", "--raw"]);
8282
cmd.stdout(Stdio::piped());
83-
cmd.stdin(cargo_credential::tty()?);
83+
cmd.stdin(cargo_credential::tty().map_err(Box::new)?);
8484
let mut child = cmd
8585
.spawn()
8686
.map_err(|e| format!("failed to spawn `op`: {}", e))?;
@@ -228,7 +228,7 @@ impl OnePasswordKeychain {
228228
// For unknown reasons, `op item create` seems to not be happy if
229229
// stdin is not a tty. Otherwise it returns with a 0 exit code without
230230
// doing anything.
231-
cmd.stdin(cargo_credential::tty()?);
231+
cmd.stdin(cargo_credential::tty().map_err(Box::new)?);
232232
self.run_cmd(cmd)?;
233233
Ok(())
234234
}
@@ -243,7 +243,7 @@ impl OnePasswordKeychain {
243243
Some(password) => password
244244
.value
245245
.map(Secret::from)
246-
.ok_or_else(|| format!("missing password value for entry").into()),
246+
.ok_or("missing password value for entry".into()),
247247
None => Err("could not find password field".into()),
248248
}
249249
}

credential/cargo-credential-macos-keychain/src/lib.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ mod macos {
1717
format!("cargo-registry:{}", index_url)
1818
}
1919

20-
fn to_credential_error(e: security_framework::base::Error) -> Error {
21-
Error::Other(format!("security framework ({}): {e}", e.code()))
22-
}
23-
2420
impl Credential for MacKeychain {
2521
fn perform(
2622
&self,
@@ -34,11 +30,9 @@ mod macos {
3430
match action {
3531
Action::Get(_) => match keychain.find_generic_password(&service_name, ACCOUNT) {
3632
Err(e) if e.code() == not_found => Err(Error::NotFound),
37-
Err(e) => Err(to_credential_error(e)),
33+
Err(e) => Err(Box::new(e).into()),
3834
Ok((pass, _)) => {
39-
let token = String::from_utf8(pass.as_ref().to_vec()).map_err(|_| {
40-
Error::Other("failed to convert token to UTF8".to_string())
41-
})?;
35+
let token = String::from_utf8(pass.as_ref().to_vec()).map_err(Box::new)?;
4236
Ok(CredentialResponse::Get {
4337
token: token.into(),
4438
cache: CacheControl::Session,
@@ -57,19 +51,19 @@ mod macos {
5751
ACCOUNT,
5852
token.expose().as_bytes(),
5953
)
60-
.map_err(to_credential_error)?;
54+
.map_err(Box::new)?;
6155
}
6256
}
6357
Ok((_, mut item)) => {
6458
item.set_password(token.expose().as_bytes())
65-
.map_err(to_credential_error)?;
59+
.map_err(Box::new)?;
6660
}
6761
}
6862
Ok(CredentialResponse::Login)
6963
}
7064
Action::Logout => match keychain.find_generic_password(&service_name, ACCOUNT) {
7165
Err(e) if e.code() == not_found => Err(Error::NotFound),
72-
Err(e) => Err(to_credential_error(e)),
66+
Err(e) => Err(Box::new(e).into()),
7367
Ok((_, item)) => {
7468
item.delete();
7569
Ok(CredentialResponse::Logout)

credential/cargo-credential-wincred/src/lib.rs

+10-13
Original file line numberDiff line numberDiff line change
@@ -58,23 +58,20 @@ mod win {
5858
if err.raw_os_error() == Some(ERROR_NOT_FOUND as i32) {
5959
return Err(Error::NotFound);
6060
}
61-
return Err(err.into());
61+
return Err(Box::new(err).into());
6262
}
6363
std::slice::from_raw_parts(
6464
(*p_credential).CredentialBlob,
6565
(*p_credential).CredentialBlobSize as usize,
6666
)
6767
};
68-
let result = match String::from_utf8(bytes.to_vec()) {
69-
Err(_) => Err("failed to convert token to UTF8".into()),
70-
Ok(token) => Ok(CredentialResponse::Get {
71-
token: token.into(),
72-
cache: CacheControl::Session,
73-
operation_independent: true,
74-
}),
75-
};
76-
let _ = unsafe { CredFree(p_credential as *mut _) };
77-
result
68+
let token = String::from_utf8(bytes.to_vec()).map_err(Box::new);
69+
unsafe { CredFree(p_credential as *mut _) };
70+
Ok(CredentialResponse::Get {
71+
token: token?.into(),
72+
cache: CacheControl::Session,
73+
operation_independent: true,
74+
})
7875
}
7976
Action::Login(options) => {
8077
let token = read_token(options, registry)?.expose();
@@ -100,7 +97,7 @@ mod win {
10097
let result = unsafe { CredWriteW(&credential, 0) };
10198
if result != TRUE {
10299
let err = std::io::Error::last_os_error();
103-
return Err(err.into());
100+
return Err(Box::new(err).into());
104101
}
105102
Ok(CredentialResponse::Login)
106103
}
@@ -112,7 +109,7 @@ mod win {
112109
if err.raw_os_error() == Some(ERROR_NOT_FOUND as i32) {
113110
return Err(Error::NotFound);
114111
}
115-
return Err(err.into());
112+
return Err(Box::new(err).into());
116113
}
117114
Ok(CredentialResponse::Logout)
118115
}

credential/cargo-credential/Cargo.toml

+2
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ repository = "https://github.com/rust-lang/cargo"
77
description = "A library to assist writing Cargo credential helpers."
88

99
[dependencies]
10+
anyhow.workspace = true
1011
serde = { workspace = true, features = ["derive"] }
1112
serde_json.workspace = true
13+
thiserror.workspace = true
1214
time.workspace = true
+206
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
use serde::{Deserialize, Serialize};
2+
use std::error::Error as StdError;
3+
use thiserror::Error as ThisError;
4+
5+
/// Credential provider error type.
6+
///
7+
/// `UrlNotSupported` and `NotFound` errors both cause Cargo
8+
/// to attempt another provider, if one is available. The other
9+
/// variants are fatal.
10+
///
11+
/// Note: Do not add a tuple variant, as it cannot be serialized.
12+
#[derive(Serialize, Deserialize, ThisError, Debug)]
13+
#[serde(rename_all = "kebab-case", tag = "kind")]
14+
#[non_exhaustive]
15+
pub enum Error {
16+
/// Registry URL is not supported. This should be used if
17+
/// the provider only works for some registries. Cargo will
18+
/// try another provider, if available
19+
#[error("registry not supported")]
20+
UrlNotSupported,
21+
22+
/// Credentials could not be found. Cargo will try another
23+
/// provider, if available
24+
#[error("credential not found")]
25+
NotFound,
26+
27+
/// The provider doesn't support this operation, such as
28+
/// a provider that can't support 'login' / 'logout'
29+
#[error("requested operation not supported")]
30+
OperationNotSupported,
31+
32+
/// The provider failed to perform the operation. Other
33+
/// providers will not be attempted
34+
#[error(transparent)]
35+
#[serde(with = "error_serialize")]
36+
Other(Box<dyn StdError + Sync + Send>),
37+
38+
/// A new variant was added to this enum since Cargo was built
39+
#[error("unknown error kind; try updating Cargo?")]
40+
#[serde(other)]
41+
Unknown,
42+
}
43+
44+
impl From<String> for Error {
45+
fn from(err: String) -> Self {
46+
Box::new(StringTypedError {
47+
message: err.to_string(),
48+
source: None,
49+
})
50+
.into()
51+
}
52+
}
53+
54+
impl From<&str> for Error {
55+
fn from(err: &str) -> Self {
56+
err.to_string().into()
57+
}
58+
}
59+
60+
impl From<anyhow::Error> for Error {
61+
fn from(value: anyhow::Error) -> Self {
62+
let mut prev = None;
63+
for e in value.chain().rev() {
64+
prev = Some(Box::new(StringTypedError {
65+
message: e.to_string(),
66+
source: prev,
67+
}));
68+
}
69+
Error::Other(prev.unwrap())
70+
}
71+
}
72+
73+
impl<T: StdError + Send + Sync + 'static> From<Box<T>> for Error {
74+
fn from(value: Box<T>) -> Self {
75+
Error::Other(value)
76+
}
77+
}
78+
79+
/// String-based error type with an optional source
80+
#[derive(Debug)]
81+
struct StringTypedError {
82+
message: String,
83+
source: Option<Box<StringTypedError>>,
84+
}
85+
86+
impl StdError for StringTypedError {
87+
fn source(&self) -> Option<&(dyn StdError + 'static)> {
88+
self.source.as_ref().map(|err| err as &dyn StdError)
89+
}
90+
}
91+
92+
impl std::fmt::Display for StringTypedError {
93+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
94+
self.message.fmt(f)
95+
}
96+
}
97+
98+
/// Serializer / deserializer for any boxed error.
99+
/// The string representation of the error, and its `source` chain can roundtrip across
100+
/// the serialization. The actual types are lost (downcast will not work).
101+
mod error_serialize {
102+
use std::error::Error as StdError;
103+
use std::ops::Deref;
104+
105+
use serde::{ser::SerializeStruct, Deserialize, Deserializer, Serializer};
106+
107+
use crate::error::StringTypedError;
108+
109+
pub fn serialize<S>(
110+
e: &Box<dyn StdError + Send + Sync>,
111+
serializer: S,
112+
) -> Result<S::Ok, S::Error>
113+
where
114+
S: Serializer,
115+
{
116+
let mut state = serializer.serialize_struct("StringTypedError", 2)?;
117+
state.serialize_field("message", &format!("{}", e))?;
118+
119+
// Serialize the source error chain recursively
120+
let mut current_source: &dyn StdError = e.deref();
121+
let mut sources = Vec::new();
122+
while let Some(err) = current_source.source() {
123+
sources.push(err.to_string());
124+
current_source = err;
125+
}
126+
state.serialize_field("caused-by", &sources)?;
127+
state.end()
128+
}
129+
130+
pub fn deserialize<'de, D>(deserializer: D) -> Result<Box<dyn StdError + Sync + Send>, D::Error>
131+
where
132+
D: Deserializer<'de>,
133+
{
134+
#[derive(Deserialize)]
135+
#[serde(rename_all = "kebab-case")]
136+
struct ErrorData {
137+
message: String,
138+
caused_by: Option<Vec<String>>,
139+
}
140+
let data = ErrorData::deserialize(deserializer)?;
141+
let mut prev = None;
142+
if let Some(source) = data.caused_by {
143+
for e in source.into_iter().rev() {
144+
prev = Some(Box::new(StringTypedError {
145+
message: e,
146+
source: prev,
147+
}));
148+
}
149+
}
150+
let e = Box::new(StringTypedError {
151+
message: data.message,
152+
source: prev,
153+
});
154+
Ok(e)
155+
}
156+
}
157+
158+
#[cfg(test)]
159+
mod tests {
160+
use super::Error;
161+
162+
#[test]
163+
pub fn unknown_kind() {
164+
let json = r#"{
165+
"kind": "unexpected-kind",
166+
"unexpected-content": "test"
167+
}"#;
168+
let e: Error = serde_json::from_str(&json).unwrap();
169+
assert!(matches!(e, Error::Unknown));
170+
}
171+
172+
#[test]
173+
pub fn roundtrip() {
174+
// Construct an error with context
175+
let e = anyhow::anyhow!("E1").context("E2").context("E3");
176+
// Convert to a string with contexts.
177+
let s1 = format!("{:?}", e);
178+
// Convert the error into an `Error`
179+
let e: Error = e.into();
180+
// Convert that error into JSON
181+
let json = serde_json::to_string_pretty(&e).unwrap();
182+
// Convert that error back to anyhow
183+
let e: anyhow::Error = e.into();
184+
let s2 = format!("{:?}", e);
185+
assert_eq!(s1, s2);
186+
187+
// Convert the error back from JSON
188+
let e: Error = serde_json::from_str(&json).unwrap();
189+
// Convert to back to anyhow
190+
let e: anyhow::Error = e.into();
191+
let s3 = format!("{:?}", e);
192+
assert_eq!(s2, s3);
193+
194+
assert_eq!(
195+
r#"{
196+
"kind": "other",
197+
"message": "E3",
198+
"caused-by": [
199+
"E2",
200+
"E1"
201+
]
202+
}"#,
203+
json
204+
);
205+
}
206+
}

0 commit comments

Comments
 (0)