Auto merge of #12622 - arlosi:cred-cache, r=ehuss

cargo-credential: change serialization of cache expiration

Serde has [multiple options](https://serde.rs/enum-representations.html) for serialization of enum types. The default is to use the field name in a map (for variants with data), or a string (for variants without data). This causes forward compatibility problems when switching between these two cases. To avoid this, all `enum`s within `cargo-credential` used the "internally tagged" approach, but `CacheControl` did not.

* Changes `CacheControl` to be internally tagged and `flattened` within `CredentialResponse::Get`
* Adds forward compatibility tests to ensure adding additional fields will not break deserialization

Within a credential response, this is the change:
```diff
- "cache":{"expires":1684251794},
+ "cache":"expires", "expiration":1684251794,
```
Other variants, such as `"cache":"session"` remain the same.

## How to review
This PR contains two commits, one for the breaking change for `CacheControl` serialization, and one non-breaking change that makes several other fields skipped if none/empty.

r? `@ehuss`

Author: bors

Cargo.lock

@@ -20,7 +20,7 @@ anyhow = "1.0.75"
 base64 = "0.21.3"
 bytesize = "1.3"
 cargo = { path = "" }
-cargo-credential = { version = "0.3.0", path = "credential/cargo-credential" }
+cargo-credential = { version = "0.4.0", path = "credential/cargo-credential" }
 cargo-credential-libsecret = { version = "0.3.1", path = "credential/cargo-credential-libsecret" }
 cargo-credential-wincred = { version = "0.3.0", path = "credential/cargo-credential-wincred" }
 cargo-credential-macos-keychain = { version = "0.3.0", path = "credential/cargo-credential-macos-keychain" }

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "cargo-credential"
-version = "0.3.1"
+version = "0.4.0"
 edition.workspace = true
 license.workspace = true
 repository = "https://github.com/rust-lang/cargo"

credential/cargo-credential/Cargo.toml

@@ -18,7 +18,7 @@ Create a Cargo project with this as a dependency:
 # Add this to your Cargo.toml:
 
 [dependencies]
-cargo-credential = "0.3"
+cargo-credential = "0.4"
 ```
 
 And then include a `main.rs` binary which implements the `Credential` trait, and calls

credential/cargo-credential/README.md

@@ -50,7 +50,7 @@ pub use secret::Secret;
 use stdio::stdin_stdout_to_console;
 
 /// Message sent by the credential helper on startup
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 pub struct CredentialHello {
     // Protocol versions supported by the credential process.
     pub v: Vec<u32>,
@@ -70,7 +70,7 @@ impl Credential for UnsupportedCredential {
 }
 
 /// Message sent by Cargo to the credential helper after the hello
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[serde(rename_all = "kebab-case")]
 pub struct CredentialRequest<'a> {
     // Cargo will respond with the highest common protocol supported by both.
@@ -80,23 +80,25 @@ pub struct CredentialRequest<'a> {
     #[serde(borrow, flatten)]
     pub action: Action<'a>,
     /// Additional command-line arguments passed to the credential provider.
+    #[serde(skip_serializing_if = "Vec::is_empty", default)]
     pub args: Vec<&'a str>,
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[serde(rename_all = "kebab-case")]
 pub struct RegistryInfo<'a> {
     /// Registry index url
     pub index_url: &'a str,
     /// Name of the registry in configuration. May not be available.
     /// The crates.io registry will be `crates-io` (`CRATES_IO_REGISTRY`).
+    #[serde(skip_serializing_if = "Option::is_none")]
     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", default)]
     pub headers: Vec<String>,
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[non_exhaustive]
 #[serde(tag = "kind", rename_all = "kebab-case")]
 pub enum Action<'a> {
@@ -119,17 +121,19 @@ impl<'a> Display for Action<'a> {
     }
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[serde(rename_all = "kebab-case")]
 pub struct LoginOptions<'a> {
     /// Token passed on the command line via --token or from stdin
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub token: Option<Secret<&'a str>>,
     /// Optional URL that the user can visit to log in to the registry
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub login_url: Option<&'a str>,
 }
 
 /// A record of what kind of operation is happening that we should generate a token for.
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[non_exhaustive]
 #[serde(tag = "operation", rename_all = "kebab-case")]
 pub enum Operation<'a> {
@@ -168,12 +172,13 @@ pub enum Operation<'a> {
 }
 
 /// Message sent by the credential helper
-#[derive(Serialize, Deserialize, Clone, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 #[serde(tag = "kind", rename_all = "kebab-case")]
 #[non_exhaustive]
 pub enum CredentialResponse {
     Get {
         token: Secret<String>,
+        #[serde(flatten)]
         cache: CacheControl,
         operation_independent: bool,
     },
@@ -183,14 +188,17 @@ pub enum CredentialResponse {
     Unknown,
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug)]
-#[serde(rename_all = "kebab-case")]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
+#[serde(tag = "cache", rename_all = "kebab-case")]
 #[non_exhaustive]
 pub enum CacheControl {
     /// Do not cache this result.
     Never,
     /// Cache this result and use it for subsequent requests in the current Cargo invocation until the specified time.
-    Expires(#[serde(with = "time::serde::timestamp")] OffsetDateTime),
+    Expires {
+        #[serde(with = "time::serde::timestamp")]
+        expiration: OffsetDateTime,
+    },
     /// Cache this result and use it for all subsequent requests in the current Cargo invocation.
     Session,
     #[serde(other)]
@@ -238,11 +246,7 @@ fn doit(
         if len == 0 {
             return Ok(());
         }
-        let request: CredentialRequest = serde_json::from_str(&buffer)?;
-        if request.v != PROTOCOL_VERSION_1 {
-            return Err(format!("unsupported protocol version {}", request.v).into());
-        }
-
+        let request = deserialize_request(&buffer)?;
         let response = stdin_stdout_to_console(|| {
             credential.perform(&request.registry, &request.action, &request.args)
         })?;
@@ -252,6 +256,17 @@ fn doit(
     }
 }
 
+/// Deserialize a request from Cargo.
+fn deserialize_request(
+    value: &str,
+) -> Result<CredentialRequest<'_>, Box<dyn std::error::Error + Send + Sync>> {
+    let request: CredentialRequest = serde_json::from_str(&value)?;
+    if request.v != PROTOCOL_VERSION_1 {
+        return Err(format!("unsupported protocol version {}", request.v).into());
+    }
+    Ok(request)
+}
+
 /// Read a line of text from stdin.
 pub fn read_line() -> Result<String, io::Error> {
     let mut buf = String::new();
@@ -278,3 +293,142 @@ pub fn read_token(
 
     Ok(Secret::from(read_line().map_err(Box::new)?))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn unsupported_version() {
+        // This shouldn't ever happen in practice, since the credential provider signals to Cargo which
+        // protocol versions it supports, and Cargo should only attempt to use one of those.
+        let msg = r#"{"v":999, "registry": {"index-url":""}, "args":[], "kind": "unexpected"}"#;
+        assert_eq!(
+            "unsupported protocol version 999",
+            deserialize_request(msg).unwrap_err().to_string()
+        );
+    }
+
+    #[test]
+    fn cache_control() {
+        let cc = CacheControl::Expires {
+            expiration: OffsetDateTime::from_unix_timestamp(1693928537).unwrap(),
+        };
+        let json = serde_json::to_string(&cc).unwrap();
+        assert_eq!(json, r#"{"cache":"expires","expiration":1693928537}"#);
+
+        let cc = CacheControl::Session;
+        let json = serde_json::to_string(&cc).unwrap();
+        assert_eq!(json, r#"{"cache":"session"}"#);
+
+        let cc: CacheControl = serde_json::from_str(r#"{"cache":"unknown-kind"}"#).unwrap();
+        assert_eq!(cc, CacheControl::Unknown);
+
+        assert_eq!(
+            "missing field `expiration`",
+            serde_json::from_str::<CacheControl>(r#"{"cache":"expires"}"#)
+                .unwrap_err()
+                .to_string()
+        );
+    }
+
+    #[test]
+    fn credential_response() {
+        let cr = CredentialResponse::Get {
+            cache: CacheControl::Never,
+            operation_independent: true,
+            token: Secret::from("value".to_string()),
+        };
+        let json = serde_json::to_string(&cr).unwrap();
+        assert_eq!(
+            json,
+            r#"{"kind":"get","token":"value","cache":"never","operation_independent":true}"#
+        );
+
+        let cr = CredentialResponse::Login;
+        let json = serde_json::to_string(&cr).unwrap();
+        assert_eq!(json, r#"{"kind":"login"}"#);
+
+        let cr: CredentialResponse =
+            serde_json::from_str(r#"{"kind":"unknown-kind","extra-data":true}"#).unwrap();
+        assert_eq!(cr, CredentialResponse::Unknown);
+
+        let cr: CredentialResponse =
+            serde_json::from_str(r#"{"kind":"login","extra-data":true}"#).unwrap();
+        assert_eq!(cr, CredentialResponse::Login);
+
+        let cr: CredentialResponse = serde_json::from_str(r#"{"kind":"get","token":"value","cache":"never","operation_independent":true,"extra-field-ignored":123}"#).unwrap();
+        assert_eq!(
+            cr,
+            CredentialResponse::Get {
+                cache: CacheControl::Never,
+                operation_independent: true,
+                token: Secret::from("value".to_string())
+            }
+        );
+    }
+
+    #[test]
+    fn credential_request() {
+        let get_oweners = CredentialRequest {
+            v: PROTOCOL_VERSION_1,
+            args: vec![],
+            registry: RegistryInfo {
+                index_url: "url",
+                name: None,
+                headers: vec![],
+            },
+            action: Action::Get(Operation::Owners { name: "pkg" }),
+        };
+
+        let json = serde_json::to_string(&get_oweners).unwrap();
+        assert_eq!(
+            json,
+            r#"{"v":1,"registry":{"index-url":"url"},"kind":"get","operation":"owners","name":"pkg"}"#
+        );
+
+        let cr: CredentialRequest =
+            serde_json::from_str(r#"{"extra-1":true,"v":1,"registry":{"index-url":"url","extra-2":true},"kind":"get","operation":"owners","name":"pkg","args":[]}"#).unwrap();
+        assert_eq!(cr, get_oweners);
+    }
+
+    #[test]
+    fn credential_request_logout() {
+        let unknown = CredentialRequest {
+            v: PROTOCOL_VERSION_1,
+            args: vec![],
+            registry: RegistryInfo {
+                index_url: "url",
+                name: None,
+                headers: vec![],
+            },
+            action: Action::Logout,
+        };
+
+        let cr: CredentialRequest = serde_json::from_str(
+            r#"{"v":1,"registry":{"index-url":"url"},"kind":"logout","extra-1":true,"args":[]}"#,
+        )
+        .unwrap();
+        assert_eq!(cr, unknown);
+    }
+
+    #[test]
+    fn credential_request_unknown() {
+        let unknown = CredentialRequest {
+            v: PROTOCOL_VERSION_1,
+            args: vec![],
+            registry: RegistryInfo {
+                index_url: "",
+                name: None,
+                headers: vec![],
+            },
+            action: Action::Unknown,
+        };
+
+        let cr: CredentialRequest = serde_json::from_str(
+            r#"{"v":1,"registry":{"index-url":""},"kind":"unexpected-1","extra-1":true,"args":[]}"#,
+        )
+        .unwrap();
+        assert_eq!(cr, unknown);
+    }
+}

credential/cargo-credential/src/lib.rs

@@ -563,7 +563,7 @@ fn auth_token_optional(
     let token = Secret::from(token);
     tracing::trace!("found token");
     let expiration = match cache_control {
-        CacheControl::Expires(expiration) => Some(expiration),
+        CacheControl::Expires { expiration } => Some(expiration),
         CacheControl::Session => None,
         CacheControl::Never | _ => return Ok(Some(token)),
     };

src/cargo/util/auth/mod.rs

@@ -1154,7 +1154,7 @@ Actual messages must not contain newlines.
     "operation":"read",
     // Registry information
     "registry":{"index-url":"sparse+https://registry-url/index/", "name": "my-registry"},
-    // Additional command-line args
+    // Additional command-line args (optional)
     "args":[]
 }
 ```
@@ -1178,7 +1178,7 @@ Actual messages must not contain newlines.
     "cksum":"...",
     // Registry information
     "registry":{"index-url":"sparse+https://registry-url/index/", "name": "my-registry"},
-    // Additional command-line args
+    // Additional command-line args (optional)
     "args":[]
 }
 ```
@@ -1195,8 +1195,10 @@ Actual messages must not contain newlines.
     // Cache control. Can be one of the following:
     // * "never"
     // * "session"
-    // * { "expires": UNIX timestamp }
-    "cache":{"expires":1684251794},
+    // * "expires"
+    "cache":"expires",
+    // Unix timestamp (only for "cache": "expires")
+    "expiration":1693942857,
     // Is the token operation independent?
     "operation_independent":true
 }}