Repository: mozilla/cargo-vet (rust)

Issue:
cargo vet trust --all papercuts
Two papercuts I've discovered:

(1) It only seems to work on crates for which you have exemptions, but not for crates which are actively failing to vet. This seems like an oversight and not what we want.

(2) The "skipping X because it has multiple authors" can be kind of annoying. I think we should add a `--force` to override it. 

Pull request:
Improve handling of trust --all
This patch makes a few changes to how trust --all is handled.

1. `trust --all` will now also trust crates which fail to vet, even if they don't have any exemptions.

2. A new flag, `--allow-multiple-publishers`, can be specified to also trust packages with multiple publishers.

3. `trust --all` will now more intelligently pick the default criteira based on your existing audit graph, providing better support for custom criteria.

Fixes #499

Patch:
diff --git a/src/cli.rs b/src/cli.rs
index 38d8215..ec1991b 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -576,10 +576,15 @@ pub struct TrustArgs {
     /// If not provided, there will be no notes.
     #[clap(long, action)]
     pub notes: Option<String>,
-    /// If specified, trusts all packages with exemptions which are solely
-    /// published by the given user.
+    /// If specified, trusts all packages with exemptions or failures which are
+    /// solely published by the given user.
     #[clap(long, action, conflicts_with("package"))]
     pub all: Option<String>,
+    /// If specified along with --all, also trusts packages with multiple
+    /// publishers, so long as at least one version was published by the given
+    /// user.
+    #[clap(long, action, requires("all"))]
+    pub allow_multiple_publishers: bool,
 }
 
 /// Forbids the given version
diff --git a/src/main.rs b/src/main.rs
index b668e93..23c0d1d 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1273,7 +1273,7 @@ fn do_cmd_trust(
             } else {
                 return Err(miette!(
                     "The package '{}' has multiple known publishers, \
-                please explicitly specify which publisher to trust",
+                    please explicitly specify which publisher to trust",
                     package
                 ));
             }
@@ -1297,17 +1297,82 @@ fn do_cmd_trust(
             sub_args.notes.as_ref(),
         )
     } else if let Some(publisher_login) = &sub_args.all {
+        // Run the resolver against the store in "suggest" mode to discover the
+        // set of packages which either fail to audit or need exemptions.
+        let suggest_store = store.clone_for_suggest(true);
+        let report =
+            resolver::resolve(&cfg.metadata, cfg.cli.filter_graph.as_ref(), &suggest_store);
+        let resolver::Conclusion::FailForVet(fail) = &report.conclusion else {
+            return Err(miette!("No failing or exempted crates, trust --all will do nothing"));
+        };
+
+        // Enumerate the failed packages to collect the set of packages which
+        // will be trusted.
+        let mut failed_criteria = report.criteria_mapper.no_criteria();
+        let mut trust = Vec::new();
+        let mut skipped = Vec::new();
+        for (failure_idx, audit_failure) in &fail.failures {
+            let package = &report.graph.nodes[*failure_idx];
+
+            // Ensure the store has publisher information for this package. This
+            // is a no-op if called multiple times for the same package.
+            let publishers = store.ensure_publisher_versions(cfg, network, package.name)?;
+            let by_user = publishers
+                .iter()
+                .filter(|p| &p.user_login == publisher_login)
+                .count();
+            if by_user == 0 {
+                continue; // never published by this user
+            }
+
+            // Record if we're skipping this package due to multiple publishers.
+            if by_user != publishers.len() && !sub_args.allow_multiple_publishers {
+                skipped.push(package.name);
+            } else {
+                trust.push(package.name);
+                failed_criteria.unioned_with(&audit_failure.criteria_failures);
+            }
+        }
+        trust.sort();
+        trust.dedup();
+
+        // Delay warning about skipped entries until after `criteria_picker`, as
+        // that may clear the terminal.
+        let maybe_warn_skipped = || {
+            if !skipped.is_empty() {
+                skipped.sort();
+                skipped.dedup();
+                warn!(
+                    "Skipped {} due to multiple publishers",
+                    string_format::FormatShortList::new(skipped)
+                );
+                warn!("  Run with --allow-multiple-publishers to also trust these packages");
+            }
+        };
+
+        if trust.is_empty() {
+            maybe_warn_skipped();
+            return Err(miette!(
+                "No failing or exempted packages published by {published_login}"
+            ));
+        }
+
         let criteria_names = criteria_picker(
             out,
             &store.audits.criteria,
             if sub_args.criteria.is_empty() {
-                vec![format::SAFE_TO_DEPLOY.to_owned()]
+                report
+                    .criteria_mapper
+                    .criteria_names(&failed_criteria)
+                    .map(|s| s.to_owned())
+                    .collect()
             } else {
                 sub_args.criteria.clone()
             },
             if sub_args.criteria.is_empty() {
                 Some(format!(
-                    "choose trusted criteria for packages published by {publisher_login}"
+                    "choose trusted criteria for packages published by {publisher_login} ({})",
+                    string_format::FormatShortList::new(trust.clone())
                 ))
             } else {
                 None
@@ -1315,28 +1380,9 @@ fn do_cmd_trust(
             .as_ref(),
         )?;
 
-        let exemptions = store.config.exemptions.clone();
-        for package in exemptions.keys() {
-            let publishers = store.ensure_publisher_versions(cfg, network, package)?;
-
-            // Skip crates whose exempted versions were not published by this user.
-            if !exemptions[package]
-                .iter()
-                .map(|exemption| publishers.iter().find(|p| p.version == exemption.version))
-                .any(|publisher| publisher.map_or(false, |p| p.user_login == publisher_login[..]))
-            {
-                continue;
-            }
-
-            // Skip crates published partially but not exclusively but this user (with a note).
-            if publishers
-                .iter()
-                .any(|publisher| publisher.user_login != publisher_login[..])
-            {
-                warn!("Skipping {} because it has multiple publishers", package);
-                continue;
-            }
+        maybe_warn_skipped();
 
+        for package in &trust {
             apply_cmd_trust(
                 out,
                 cfg,


Test patch:
diff --git a/src/tests/certify.rs b/src/tests/certify.rs
index 26ce3f3..2de83bd 100644
--- a/src/tests/certify.rs
+++ b/src/tests/certify.rs
@@ -674,3 +674,154 @@ fn mock_trust_flow_all() {
 
     insta::assert_snapshot!(result);
 }
+
+#[test]
+fn mock_trust_flow_all_allow_multiple() {
+    let mock = MockMetadata::simple();
+
+    let _enter = TEST_RUNTIME.enter();
+    let metadata = mock.metadata();
+
+    let (config, audits, imports) = files_inited(&metadata);
+
+    let output = BasicTestOutput::with_callbacks(|_| Ok("\n".to_owned()), |_| unimplemented!());
+
+    let cfg = mock_cfg_args(
+        &metadata,
+        [
+            "cargo",
+            "vet",
+            "trust",
+            "--all",
+            "testuser",
+            "--allow-multiple-publishers",
+        ],
+    );
+    let sub_args = if let Some(crate::cli::Commands::Trust(sub_args)) = &cfg.cli.command {
+        sub_args
+    } else {
+        unreachable!();
+    };
+
+    let mut network = Network::new_mock();
+    network.mock_serve_json(
+        "https://crates.io/api/v1/crates/third-party1",
+        &serde_json::json!({
+            "crate": { "description": "description" },
+            "versions": [
+                {
+                    "crate": "third-party1",
+                    "created_at": "2022-10-12T04:51:37.251648+00:00",
+                    "num": "1.0.0",
+                },
+                {
+                    "crate": "third-party1",
+                    "created_at": "2022-10-12T04:51:37.251648+00:00",
+                    "num": "9.0.0",
+                    "published_by": {
+                        "id": 5,
+                        "login": "otheruser",
+                        "name": "Other user",
+                        "url": "https://github.com/otheruser"
+                    }
+                },
+                {
+                    "crate": "third-party1",
+                    "created_at": "2022-12-12T04:51:37.251648+00:00",
+                    "num": "10.0.0",
+                    "published_by": {
+                        "id": 2,
+                        "login": "testuser",
+                        "name": "Test user",
+                        "url": "https://github.com/testuser"
+                    }
+                },
+            ]
+        }),
+    );
+    network_mock_index(&mut network, "third-party1", &["1.0.0", "9.0.0", "10.0.0"]);
+
+    network.mock_serve_json(
+        "https://crates.io/api/v1/crates/transitive-third-party1",
+        &serde_json::json!({
+            "crate": { "description": "description" },
+            "versions": [
+                {
+                    "crate": "transitive-third-party1",
+                    "created_at": "2022-10-12T04:51:37.251648+00:00",
+                    "num": "1.0.0",
+                },
+                {
+                    "crate": "transitive-third-party1",
+                    "created_at": "2022-10-12T04:51:37.251648+00:00",
+                    "num": "9.0.0",
+                    "published_by": {
+                        "id": 2,
+                        "login": "testuser",
+                        "name": "Test user",
+                        "url": "https://github.com/testuser"
+                    }
+                },
+                {
+                    "crate": "transitive-third-party1",
+                    "created_at": "2022-12-12T04:51:37.251648+00:00",
+                    "num": "10.0.0",
+                    "published_by": {
+                        "id": 2,
+                        "login": "testuser",
+                        "name": "Test user",
+                        "url": "https://github.com/testuser"
+                    }
+                },
+            ]
+        }),
+    );
+    network_mock_index(
+        &mut network,
+        "transitive-third-party1",
+        &["1.0.0", "9.0.0", "10.0.0"],
+    );
+
+    network.mock_serve_json(
+        "https://crates.io/api/v1/crates/third-party2",
+        &serde_json::json!({
+            "crate": { "description": "description" },
+            "versions": [
+                {
+                    "crate": "third-party2",
+                    "created_at": "2022-12-12T04:51:37.251648+00:00",
+                    "num": "10.0.0",
+                    "published_by": {
+                        "id": 2,
+                        "login": "testuser",
+                        "name": "Test user",
+                        "url": "https://github.com/testuser"
+                    }
+                },
+            ]
+        }),
+    );
+    network_mock_index(&mut network, "third-party2", &["10.0.0"]);
+
+    let mut store = Store::mock_online(&cfg, config, audits, imports, &network, true)
+        .expect("store acquisition failed");
+
+    crate::do_cmd_trust(
+        &output.clone().as_dyn(),
+        &cfg,
+        sub_args,
+        &mut store,
+        Some(&network),
+    )
+    .expect("do_cmd_trust failed");
+
+    let audits = crate::serialization::to_formatted_toml(
+        &store.audits,
+        Some(&crate::storage::user_info_map(&store.imports)),
+    )
+    .unwrap();
+
+    let result = format!("OUTPUT:\n{output}\nAUDITS:\n{audits}");
+
+    insta::assert_snapshot!(result);
+}
diff --git a/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all.snap b/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all.snap
index 4c2ee12..33859c7 100644
--- a/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all.snap
+++ b/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all.snap
@@ -4,7 +4,7 @@ expression: result
 ---
 OUTPUT:
 <<<CLEAR SCREEN>>>
-choose trusted criteria for packages published by testuser
+choose trusted criteria for packages published by testuser (third-party2 and transitive-third-party1)
   1. safe-to-run
   2. safe-to-deploy
   3. fuzzed
@@ -12,7 +12,7 @@ choose trusted criteria for packages published by testuser
   5. strong-reviewed
   6. weak-reviewed
 
-current selection: ["safe-to-deploy"]
+current selection: ["reviewed"]
 (press ENTER to accept the current criteria)
 > 
 
@@ -36,14 +36,14 @@ description = "weakly reviewed"
 [audits]
 
 [[trusted.third-party2]]
-criteria = "safe-to-deploy"
-user-id = 2
+criteria = "reviewed"
+user-id = 2 # Test user (testuser)
 start = "2022-12-12"
 end = "2024-01-01"
 
 [[trusted.transitive-third-party1]]
-criteria = "safe-to-deploy"
-user-id = 2
+criteria = "reviewed"
+user-id = 2 # Test user (testuser)
 start = "2022-10-12"
 end = "2024-01-01"
 
diff --git a/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all_allow_multiple.snap b/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all_allow_multiple.snap
new file mode 100644
index 0000000..7d27994
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__certify__mock_trust_flow_all_allow_multiple.snap
@@ -0,0 +1,55 @@
+---
+source: src/tests/certify.rs
+expression: result
+---
+OUTPUT:
+<<<CLEAR SCREEN>>>
+choose trusted criteria for packages published by testuser (third-party1, third-party2, and 1 other)
+  1. safe-to-run
+  2. safe-to-deploy
+  3. fuzzed
+  4. reviewed
+  5. strong-reviewed
+  6. weak-reviewed
+
+current selection: ["reviewed"]
+(press ENTER to accept the current criteria)
+> 
+
+
+AUDITS:
+
+[criteria.fuzzed]
+description = "fuzzed"
+
+[criteria.reviewed]
+description = "reviewed"
+implies = "weak-reviewed"
+
+[criteria.strong-reviewed]
+description = "strongly reviewed"
+implies = "reviewed"
+
+[criteria.weak-reviewed]
+description = "weakly reviewed"
+
+[audits]
+
+[[trusted.third-party1]]
+criteria = "reviewed"
+user-id = 2 # Test user (testuser)
+start = "2022-12-12"
+end = "2024-01-01"
+
+[[trusted.third-party2]]
+criteria = "reviewed"
+user-id = 2 # Test user (testuser)
+start = "2022-12-12"
+end = "2024-01-01"
+
+[[trusted.transitive-third-party1]]
+criteria = "reviewed"
+user-id = 2 # Test user (testuser)
+start = "2022-10-12"
+end = "2024-01-01"
+
diff --git a/tests/snapshots/test_cli__markdown-help.snap b/tests/snapshots/test_cli__markdown-help.snap
index a5fcdf3..cceb38d 100644
--- a/tests/snapshots/test_cli__markdown-help.snap
+++ b/tests/snapshots/test_cli__markdown-help.snap
@@ -470,8 +470,12 @@ A free-form string to include with the new audit entry
 If not provided, there will be no notes.
 
 #### `--all <ALL>`
-If specified, trusts all packages with exemptions which are solely published by the
-given user
+If specified, trusts all packages with exemptions or failures which are solely published
+by the given user
+
+#### `--allow-multiple-publishers`
+If specified along with --all, also trusts packages with multiple publishers, so long as
+at least one version was published by the given user
 
 #### `-h, --help`
 Print help information