Don't create a link when it already exists

- When adding a link, check if the active link has the same
    parameters, if so don't update the link.  If the parameters
    changed, show a diff.

src/commands/link.rs

@@ -16,6 +16,7 @@ use cert_store::StoreUpdate;
 use cert_store::store::UserIDQueryParams;
 
 use crate::Config;
+use crate::commands::active_certification;
 use crate::commands::get_certification_keys;
 use crate::parse_duration;
 use crate::parse_notations;
@@ -198,6 +199,117 @@ pub fn check_userids(config: &Config, cert: &Cert, self_signed: bool,
     }
 }
 
+// Returns whether two signatures have the same parameters.
+//
+// This does some normalization and only considers things that are
+// relevant to links.
+fn diff_link(old: &SignatureBuilder, new: &SignatureBuilder) -> bool {
+
+    let mut changed = false;
+
+    let a_expiration = old.signature_expiration_time();
+    let b_expiration = new.signature_expiration_time();
+    if a_expiration != b_expiration {
+        eprintln!(
+            "  Updating expiration time: {} -> {}.",
+            if let Some(a_expiration) = a_expiration {
+                chrono::DateTime::<chrono::offset::Utc>::from(
+                    a_expiration).to_string()
+            } else {
+                "no expiration".to_string()
+            },
+            if let Some(b_expiration) = b_expiration {
+                chrono::DateTime::<chrono::offset::Utc>::from(
+                    b_expiration).to_string()
+            } else {
+                "no expiration".to_string()
+            });
+    }
+
+    let (a_depth, a_amount) = old.trust_signature().unwrap_or((0, 120));
+    let (b_depth, b_amount) = new.trust_signature().unwrap_or((0, 120));
+
+    if a_amount != b_amount {
+        changed = true;
+        eprintln!("  Updating trust amount: {} -> {}.",
+                  a_amount, b_amount);
+    }
+    if a_depth != b_depth {
+        changed = true;
+        eprintln!("  Update trust depth: {} -> {}.",
+                  a_depth, b_depth);
+    }
+
+    let mut a_regex: Vec<_> = old.regular_expressions().collect();
+    a_regex.sort();
+    a_regex.dedup();
+    let mut b_regex: Vec<_> = new.regular_expressions().collect();
+    b_regex.sort();
+    b_regex.dedup();
+
+    if a_regex != b_regex {
+        changed = true;
+        eprintln!("  Updating regular expressions:");
+        let a_regex: Vec<String> = a_regex.into_iter()
+            .enumerate()
+            .map(|(i, r)| {
+                format!("{}. {:?}",
+                        i + 1, String::from_utf8_lossy(r))
+            })
+            .collect();
+        eprintln!("    Current link:\n      {}",
+                  a_regex.join("\n    "));
+
+        let b_regex: Vec<String> = b_regex.into_iter()
+            .enumerate()
+            .map(|(i, r)| {
+                format!("{}. {:?}",
+                        i + 1, String::from_utf8_lossy(r))
+            })
+            .collect();
+        eprintln!("    Updated link:\n      {}",
+                  b_regex.join("\n    "));
+    }
+
+    let a_notations: Vec<_> = old.notation_data()
+        .filter(|n| n.name() != "salt@notations.sequoia-pgp.org")
+        .collect();
+    let b_notations: Vec<_> = new.notation_data()
+        .filter(|n| n.name() != "salt@notations.sequoia-pgp.org")
+        .collect();
+    if a_notations != b_notations {
+        changed = true;
+        eprintln!("  Updating notations.");
+        let a_notations: Vec<String> = a_notations.into_iter()
+            .enumerate()
+            .map(|(i, n)| {
+                format!("{}. {:?}", i + 1, n)
+            })
+            .collect();
+        eprintln!("    Current link:\n      {}",
+                  a_notations.join("\n    "));
+
+        let b_notations: Vec<String> = b_notations.into_iter()
+            .enumerate()
+            .map(|(i, n)| {
+                format!("{}. {:?}", i + 1, n)
+            })
+            .collect();
+        eprintln!("    Updated link:\n       {}",
+                  b_notations.join("\n    "));
+    }
+
+    let a_exportable = old.exportable_certification().unwrap_or(true);
+    let b_exportable = new.exportable_certification().unwrap_or(true);
+    if a_exportable != b_exportable {
+        changed = true;
+        eprintln!("  Updating exportable flag: {} -> {}.",
+                  a_exportable, b_exportable);
+    }
+
+    changed
+}
+
 pub fn link(config: Config, c: link::Command) -> Result<()> {
     use link::Subcommands::*;
     match c.subcommand {
@@ -351,11 +463,14 @@ pub fn add(mut config: Config, c: link::AddCommand)
     assert_eq!(signers.len(), 1);
     let mut signer = signers.into_iter().next().unwrap();
 
-    let certifications = userids.iter()
+    let certifications = active_certification(
-        .map(|userid| {
+            &config, &vc.fingerprint(), userids,
+            signer.public())
+        .into_iter()
+        .map(|(userid, active_certification)| {
             let userid_str = || String::from_utf8_lossy(userid.value());
 
-            if let Some(ua) = vc.userids().find(|ua| ua.userid() == userid) {
+            if let Some(ua) = vc.userids().find(|ua| ua.userid() == &userid) {
                 if let RevocationStatus::Revoked(_) = ua.revocation_status() {
                     // It's revoked.
                     if user_supplied_userids {
@@ -377,13 +492,47 @@ pub fn add(mut config: Config, c: link::AddCommand)
                           userid_str(), cert.fingerprint(), userid);
             }
 
-            eprintln!("Linking {:?} and {}.",
+            if let Some(active_certification) = active_certification {
-                      userid_str(), cert.fingerprint());
+                let active_certification_ct
+                    = active_certification.signature_creation_time()
+                    .expect("valid signature");
 
-            // XXX: If we already have exactly this signature (modulo
+                let retracted = matches!(active_certification.trust_signature(),
-            // the creation time), then don't add it!  Note: it is
+                                         Some((_depth, 0)));
-            // explicitly NOT enough to check that there is a
+                if retracted {
-            // certification from the local trust root.
+                    eprintln!("{}, {} was retracted at {}.",
+                              cert.fingerprint(), userid_str(),
+                              chrono::DateTime::<chrono::offset::Utc>::from(
+                                  active_certification_ct));
+                } else {
+                    eprintln!("{}, {} was already linked at {}.",
+                              cert.fingerprint(), userid_str(),
+                              chrono::DateTime::<chrono::offset::Utc>::from(
+                                  active_certification_ct));
+                }
+
+                let changed = diff_link(
+                    &SignatureBuilder::from(active_certification),
+                    &builder);
+
+                if ! changed && config.force {
+                    eprintln!("  Link parameters are unchanged, but \
+                               updating anyway as \"--force\" was specified.");
+                } else if ! changed {
+                    eprintln!("  Link parameters are unchanged, no update \
+                               needed (specify \"--force\" to update anyway).");
+
+                    // Return a signature packet to indicate that we
+                    // processed something.  But don't return a
+                    // signature.
+                    return Ok(vec![ Packet::from(userid.clone()) ]);
+                } else {
+                    eprintln!("  Link parameters changed, updating link.");
+                }
+            }
+
+            eprintln!("Linking {} and {:?}.",
+                      cert.fingerprint(), userid_str());
 
             let sig = builder.clone().sign_userid_binding(
                 &mut signer,
@@ -393,6 +542,7 @@ pub fn add(mut config: Config, c: link::AddCommand)
                     format!("Creating certification for {:?}", userid_str())
                 })?;
 
+            eprintln!();
             Ok(vec![ Packet::from(userid.clone()), Packet::from(sig) ])
         })
         .collect::<Result<Vec<Vec<Packet>>>>()?
@@ -407,6 +557,11 @@ pub fn add(mut config: Config, c: link::AddCommand)
             cert.fingerprint()));
     }
 
+    if certifications.iter().all(|p| matches!(p, Packet::UserID(_))) {
+        // There are no signatures to insert.  We're done.
+        return Ok(());
+    }
+
     let cert = cert.insert_packets(certifications.clone())?;
 
     let cert_store = config.cert_store_mut_or_else()?;
@@ -460,11 +615,13 @@ pub fn retract(mut config: Config, c: link::RetractCommand)
     assert_eq!(signers.len(), 1);
     let mut signer = signers.into_iter().next().unwrap();
 
-    let certifications = userids.iter()
+    let certifications = active_certification(
-        .map(|userid| {
+            &config, &cert.fingerprint(), userids, signer.public())
+        .into_iter()
+        .map(|(userid, active_certification)| {
             let userid_str = || String::from_utf8_lossy(userid.value());
 
-            if let Some(ua) = cert.userids().find(|ua| ua.userid() == userid) {
+            if let Some(ua) = cert.userids().find(|ua| ua.userid() == &userid) {
                 if ! ua.certifications().any(|c| {
                     c.get_issuers().into_iter()
                         .any(|issuer| issuer.aliases(&trust_root_kh))
@@ -477,8 +634,60 @@ pub fn retract(mut config: Config, c: link::RetractCommand)
                 }
             }
 
-            eprintln!("Breaking link between {:?} and {}.",
+            if let Some(active_certification) = active_certification {
-                      userid_str(), cert.fingerprint());
+                let active_certification_ct
+                    = active_certification.signature_creation_time()
+                    .expect("valid signature");
+
+                let retracted = matches!(active_certification.trust_signature(),
+                                         Some((_depth, 0)));
+                if retracted {
+                    eprintln!("{}, {} was already retracted at {}.",
+                              cert.fingerprint(), userid_str(),
+                              chrono::DateTime::<chrono::offset::Utc>::from(
+                                  active_certification_ct));
+                } else {
+                    eprintln!("{}, {} was linked at {}.",
+                              cert.fingerprint(), userid_str(),
+                              chrono::DateTime::<chrono::offset::Utc>::from(
+                                  active_certification_ct));
+                }
+
+                let changed = diff_link(
+                    &SignatureBuilder::from(active_certification),
+                    &builder);
+
+                if ! changed && config.force {
+                    eprintln!("  Link parameters are unchanged, but \
+                               updating anyway as \"--force\" was specified.");
+                } else if ! changed {
+                    eprintln!("  Link parameters are unchanged, no update \
+                               needed (specify \"--force\" to update anyway).");
+
+                    // Return a signature packet to indicate that we
+                    // processed something.  But don't return a
+                    // signature.
+                    return Ok(vec![ Packet::from(userid.clone()) ]);
+                } else {
+                    eprintln!("  Link parameters changed, updating link.");
+                }
+            } else if config.force {
+                eprintln!("There is no link to retract between {} and {:?}, \
+                           retracting anyways as \"--force\" was specified.",
+                          cert.fingerprint(), userid_str());
+            } else {
+                eprintln!("There is no link to retract between {} and {:?} \
+                           (specify \"--force\" to mark as retracted anyways).",
+                          cert.fingerprint(), userid_str());
+
+                // Return a signature packet to indicate that we
+                // processed something.  But don't return a
+                // signature.
+                return Ok(vec![ Packet::from(userid.clone()) ]);
+            }
+
+            eprintln!("Breaking link between {} and {:?}.",
+                      cert.fingerprint(), userid_str());
 
             // XXX: If we already have exactly this signature (modulo
             // the creation time), then don't add it!  Note: it is

tests/sq-link.rs

@@ -1,3 +1,5 @@
+use std::path::Path;
+use std::process::ExitStatus;
 use std::sync::Mutex;
 
 use tempfile::TempDir;
@@ -124,6 +126,7 @@ fn sq_verify(cert_store: Option<&str>,
 fn sq_link(cert_store: &str,
            cert: &str, userids: &[&str], more_args: &[&str],
            success: bool)
+    -> (ExitStatus, String, String)
 {
     let mut cmd = Command::cargo_bin("sq").expect("have sq");
     cmd.args(&["--cert-store", cert_store]);
@@ -147,9 +150,12 @@ fn sq_link(cert_store: &str,
                  \nstdout:\n{}\nstderr:\n{}",
                 stdout, stderr);
     }
+
+    (output.status, stdout, stderr)
 }
 
 fn sq_retract(cert_store: &str, cert: &str, userids: &[&str])
+    -> (ExitStatus, String, String)
 {
     let mut cmd = Command::cargo_bin("sq").expect("have sq");
     cmd.args(&["--cert-store", cert_store]);
@@ -163,6 +169,8 @@ fn sq_retract(cert_store: &str, cert: &str, userids: &[&str])
     assert!(output.status.success(),
             "sq link retract\nstdout:\n{}\nstderr:\n{}",
             stdout, stderr);
+
+    (output.status, stdout, stderr)
 }
 
 // Certifies a binding.
@@ -452,3 +460,126 @@ fn sq_link_add_retract() -> Result<()> {
 
     Ok(())
 }
+
+// Set the different parameters.  When the parameters are the same,
+// make sure no certifications are written; when they are different
+// make sure the file changed.
+#[test]
+fn sq_link_update_detection() -> Result<()> {
+    let dir = TempDir::new()?;
+
+    let certd = dir.path().join("cert.d").display().to_string();
+    std::fs::create_dir(&certd).expect("mkdir works");
+
+    let alice_pgp = dir.path().join("alice.pgp").display().to_string();
+    let alice_userid = "<alice@example.org>";
+    let alice = sq_gen_key(Some(&certd), &[ alice_userid ], &alice_pgp);
+    let alice_fpr = alice.fingerprint().to_string();
+    let alice_cert_pgp = dir.path().join("cert.d")
+        .join(&alice_fpr[0..2].to_ascii_lowercase())
+        .join(&alice_fpr[2..].to_ascii_lowercase());
+
+    // Reads and returns file.  Asserts that old and the new contexts
+    // are the same (or not).
+    let compare = |old: Vec<u8>, file: &Path, same: bool| -> Vec<u8> {
+        let new = std::fs::read(file).unwrap();
+        if same {
+            assert_eq!(old, new, "file unexpectedly changed");
+        } else {
+            assert_ne!(old, new, "file unexpectedly stayed the same");
+        }
+        new
+    };
+    let bytes = std::fs::read(&alice_cert_pgp).unwrap();
+
+    // Retract it.  There is nothing to retract (but this doesn't fail).
+    let output = sq_retract(&certd, &alice_fpr, &[]);
+    assert!(output.2.contains("You never linked"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // Link it.
+    sq_link(&certd, &alice_fpr, &[], &[], true);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    // As no parameters changed, this should succeeded, but no
+    // certification should be written.
+    let output = sq_link(&certd, &alice_fpr, &[], &[], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // Make Alice a CA.
+    let output = sq_link(&certd, &alice_fpr, &[], &["--ca", "*"], true);
+    assert!(output.2.contains("was already linked at"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_link(&certd, &alice_fpr, &[], &["--ca", "*"], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // Make her a partially trusted CA.
+    let output = sq_link(&certd, &alice_fpr, &[], &["--amount", "30"], true);
+    assert!(output.2.contains("was already linked at"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_link(&certd, &alice_fpr, &[], &["--amount", "30"], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // Retract the link.
+    let output = sq_retract(&certd, &alice_fpr, &[]);
+    assert!(output.2.contains("was linked at"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_retract(&certd, &alice_fpr, &[]);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+
+    // Link it again.
+    let output = sq_link(&certd, &alice_fpr, &[],
+                         &["--depth", "10", "--amount", "10"], true);
+    assert!(output.2.contains("was retracted"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_link(&certd, &alice_fpr, &[],
+                         &["--depth", "10", "--amount", "10"], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // Use a notation.
+    let output = sq_link(&certd, &alice_fpr, &[],
+                         &["--notation", "foo", "10"], true);
+    assert!(output.2.contains("was already linked"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_link(&certd, &alice_fpr, &[],
+                         &["--notation", "foo", "10"], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    // The default link again.
+    let output = sq_link(&certd, &alice_fpr, &[], &[], true);
+    assert!(output.2.contains("was already linked"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, false);
+
+    let output = sq_link(&certd, &alice_fpr, &[], &[], true);
+    assert!(output.2.contains("Link parameters are unchanged, no update needed"),
+            "stdout:\n{}\nstderr:\n{}", output.1, output.2);
+    let bytes = compare(bytes, &alice_cert_pgp, true);
+
+    let _ = bytes;
+    Ok(())
+}