Repository: mozilla/cargo-vet (rust)

Issue:
Support an array for the "who" field
It's been pointed out that some large audits might be multi-person efforts. It wouldn't be a disaster to just put one of the names in there, or to use comma-separation, but I think it should be trivial with our existing machinery to support making the field an optional array, similar to the authors field in Cargo.toml.

This might also be a good exercise to kick the tires on format changes in the wild. How would an older version of cargo-vet behave when it encounters the array, and how do we want it to behave?

Pull request description:
Support multiple names in 'who'
This causes changes in the JSON output for violation errors when the `who` field is missing.

The JSON output for violations unfortunately exposes a lot of internal details and is quite unstable, so I don't think this should be a serious issue for any consumers.

Fixes #320

Patch:
diff --git a/src/cli.rs b/src/cli.rs
index f19d89d..021b518 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -459,7 +459,7 @@ pub struct CertifyArgs {
     ///
     /// If not provided, we will collect this information from the local git.
     #[clap(long, action)]
-    pub who: Option<String>,
+    pub who: Vec<String>,
     /// A free-form string to include with the new audit entry
     ///
     /// If not provided, there will be no notes.
@@ -494,7 +494,7 @@ pub struct RecordViolationArgs {
     ///
     /// If not provided, we will collect this information from the local git.
     #[clap(long, action)]
-    pub who: Option<String>,
+    pub who: Vec<String>,
     /// A free-form string to include with the new forbid entry
     ///
     /// If not provided, there will be no notes.
diff --git a/src/format.rs b/src/format.rs
index bb57f78..6ef16ff 100644
--- a/src/format.rs
+++ b/src/format.rs
@@ -155,7 +155,7 @@ pub struct CriteriaEntry {
 #[serde(try_from = "serialization::audit::AuditEntryAll")]
 #[serde(into = "serialization::audit::AuditEntryAll")]
 pub struct AuditEntry {
-    pub who: Option<String>,
+    pub who: Vec<Spanned<String>>,
     pub criteria: Vec<Spanned<CriteriaName>>,
     pub kind: AuditKind,
     pub notes: Option<String>,
diff --git a/src/main.rs b/src/main.rs
index 98de29f..c4f6629 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -20,6 +20,7 @@ use miette::{miette, Context, Diagnostic, IntoDiagnostic};
 use network::Network;
 use reqwest::Url;
 use serde::de::Deserialize;
+use serialization::spanned::Spanned;
 use thiserror::Error;
 use tracing::{error, info, trace, warn};
 
@@ -705,12 +706,19 @@ fn do_cmd_certify(
         return Err(CertifyError::CouldntGuessVersion(package));
     };
 
-    let (username, who) = if let Some(who) = &sub_args.who {
-        (who.clone(), Some(who.clone()))
-    } else {
+    let (username, who) = if sub_args.who.is_empty() {
         let user_info = get_user_info()?;
         let who = format!("{} <{}>", user_info.username, user_info.email);
-        (user_info.username, Some(who))
+        (user_info.username, vec![Spanned::from(who)])
+    } else {
+        (
+            sub_args.who.join(", "),
+            sub_args
+                .who
+                .iter()
+                .map(|w| Spanned::from(w.clone()))
+                .collect(),
+        )
     };
 
     let criteria_mapper = CriteriaMapper::new(
@@ -1102,12 +1110,19 @@ fn cmd_record_violation(
         violation: sub_args.versions.clone(),
     };
 
-    let (_username, who) = if let Some(who) = &sub_args.who {
-        (who.clone(), Some(who.clone()))
-    } else {
+    let (_username, who) = if sub_args.who.is_empty() {
         let user_info = get_user_info()?;
         let who = format!("{} <{}>", user_info.username, user_info.email);
-        (user_info.username, Some(who))
+        (user_info.username, vec![Spanned::from(who)])
+    } else {
+        (
+            sub_args.who.join(", "),
+            sub_args
+                .who
+                .iter()
+                .map(|w| Spanned::from(w.clone()))
+                .collect(),
+        )
     };
 
     let notes = sub_args.notes.clone();
diff --git a/src/resolver.rs b/src/resolver.rs
index f8090d1..e3b6af7 100644
--- a/src/resolver.rs
+++ b/src/resolver.rs
@@ -3134,8 +3134,15 @@ impl FailForViolationConflict {
                 }
             }
             writeln!(out, "      criteria: {:?}", entry.criteria);
-            if let Some(who) = &entry.who {
-                writeln!(out, "      who: {who}");
+            for (idx, who) in entry.who.iter().enumerate() {
+                if idx == 0 {
+                    write!(out, "      who: {who}");
+                } else {
+                    write!(out, ", {who}");
+                }
+            }
+            if !entry.who.is_empty() {
+                writeln!(out, "");
             }
             if let Some(notes) = &entry.notes {
                 writeln!(out, "      notes: {notes}");
diff --git a/src/serialization.rs b/src/serialization.rs
index cad7be1..2b50b9b 100644
--- a/src/serialization.rs
+++ b/src/serialization.rs
@@ -190,7 +190,10 @@ pub mod audit {
 
     #[derive(Serialize, Deserialize)]
     pub struct AuditEntryAll {
-        who: Option<String>,
+        #[serde(default)]
+        #[serde(skip_serializing_if = "Vec::is_empty")]
+        #[serde(with = "string_or_vec")]
+        who: Vec<Spanned<String>>,
         #[serde(default)]
         #[serde(with = "string_or_vec")]
         criteria: Vec<Spanned<CriteriaName>>,
@@ -617,7 +620,7 @@ mod test {
             "test".to_owned(),
             vec![
                 AuditEntry {
-                    who: None,
+                    who: vec![],
                     criteria: vec!["long-criteria".to_owned().into()],
                     kind: AuditKind::Full {
                         version: "1.0.0".parse().unwrap(),
@@ -627,7 +630,7 @@ mod test {
                     is_fresh_import: false, // ignored
                 },
                 AuditEntry {
-                    who: None,
+                    who: vec![],
                     criteria: vec!["short-criteria".to_owned().into()],
                     kind: AuditKind::Full {
                         version: "1.0.0".parse().unwrap(),


Test patch:
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index a04af06..dc48061 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -307,7 +307,7 @@ fn exemptions_dep(
 
 fn delta_audit(from: Version, to: Version, criteria: CriteriaStr) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Delta {
@@ -332,7 +332,7 @@ fn delta_audit_dep(
     >,
 ) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Delta {
@@ -354,7 +354,7 @@ fn delta_audit_dep(
 
 fn full_audit(version: Version, criteria: CriteriaStr) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Full {
@@ -370,7 +370,7 @@ fn full_audit_m(
     criteria: impl IntoIterator<Item = impl Into<CriteriaName>>,
 ) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: criteria.into_iter().map(|s| s.into().into()).collect(),
         kind: AuditKind::Full {
@@ -392,7 +392,7 @@ fn full_audit_dep(
     >,
 ) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Full {
@@ -413,7 +413,7 @@ fn full_audit_dep(
 
 fn violation_hard(version: VersionReq) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![SAFE_TO_RUN.to_string().into()],
         kind: AuditKind::Violation { violation: version },
@@ -423,7 +423,7 @@ fn violation_hard(version: VersionReq) -> AuditEntry {
 #[allow(dead_code)]
 fn violation(version: VersionReq, criteria: CriteriaStr) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: vec![criteria.to_string().into()],
         kind: AuditKind::Violation { violation: version },
@@ -436,7 +436,7 @@ fn violation_m(
     criteria: impl IntoIterator<Item = impl Into<CriteriaName>>,
 ) -> AuditEntry {
     AuditEntry {
-        who: None,
+        who: vec![],
         notes: None,
         criteria: criteria.into_iter().map(|s| s.into().into()).collect(),
         kind: AuditKind::Violation { violation: version },
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
index e90e056..ea7db69 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-high-hit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
index 4e7fbf7..a5db1c2 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-imply-hit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
index 90c5fea..06ac9c2 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-low-hit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
index 6d9828f..627ba1d 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__builtin-simple-deps-violation-redundant-low-hit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -25,8 +24,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
index dde3c3c..0c432f0 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-full-audit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=10",
-            "who": null
+            "violation": "=10"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
index 82db503..1b59640 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-cur-unaudited.json.snap
@@ -18,8 +18,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=10",
-            "who": null
+            "violation": "=10"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
index c669ceb..166a098 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-delta.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": "3.0.0 -> 5.0.0",
             "notes": null,
             "version": null,
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=5.0.0",
-            "who": null
+            "violation": "=5.0.0"
           },
           "violation_source": "Local"
         }
@@ -35,8 +33,7 @@ expression: json
             "delta": "5.0.0 -> 10.0.0",
             "notes": null,
             "version": null,
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -44,8 +41,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=5.0.0",
-            "who": null
+            "violation": "=5.0.0"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
index 017ee33..f8076fe 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-full-audit.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "3.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=3.0.0",
-            "who": null
+            "violation": "=3.0.0"
           },
           "violation_source": "Local"
         }
@@ -35,8 +33,7 @@ expression: json
             "delta": "3.0.0 -> 5.0.0",
             "notes": null,
             "version": null,
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -44,8 +41,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "=3.0.0",
-            "who": null
+            "violation": "=3.0.0"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
index 17df0ba..f2dd7f0 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-hit-with-extra-junk.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -25,8 +24,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
index 556610f..b136094 100644
--- a/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
+++ b/src/tests/snapshots/cargo_vet__tests__violations__mock-simple-violation-wildcard.json.snap
@@ -13,8 +13,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": "10.0.0",
-            "violation": null,
-            "who": null
+            "violation": null
           },
           "audit_source": "Local",
           "violation": {
@@ -22,8 +21,7 @@ expression: json
             "delta": null,
             "notes": null,
             "version": null,
-            "violation": "*",
-            "who": null
+            "violation": "*"
           },
           "violation_source": "Local"
         }
diff --git a/tests/test-project/supply-chain/audits.toml b/tests/test-project/supply-chain/audits.toml
index 3e46c75..cd39618 100644
--- a/tests/test-project/supply-chain/audits.toml
+++ b/tests/test-project/supply-chain/audits.toml
@@ -23,10 +23,15 @@ version = "0.1.0"
 notes = "test for basic resolution"
 
 [[audits.base64]]
+who = "Person One <personone@example.com>"
 criteria = "safe-to-deploy"
 version = "0.5.0"
 
 [[audits.base64]]
+who = [
+    "Person One <personone@example.com>",
+    "Person Two <persontwo@example.com>",
+]
 criteria = "safe-to-deploy"
 delta = "0.1.0 -> 0.4.0"