db: correctly deal with same-email user ids with different revocation statuses

database/src/fs.rs

@@ -888,6 +888,13 @@ mod tests {
         db.check_consistency().expect("inconsistent database");
     }
 
+    #[test]
+    fn same_email_3() {
+        let (_tmp_dir, mut db, log_path) = open_db();
+        test::test_same_email_3(&mut db, &log_path);
+        db.check_consistency().expect("inconsistent database");
+    }
+
     #[test]
     fn no_selfsig() {
         let (_tmp_dir, mut db, log_path) = open_db();

database/src/lib.rs

@@ -103,11 +103,11 @@ impl Query {
     }
 }
 
-#[derive(Debug,PartialEq,Eq)]
+#[derive(Debug,PartialEq,Eq,PartialOrd,Ord)]
 pub enum EmailAddressStatus {
-    Revoked,
-    NotPublished,
     Published,
+    NotPublished,
+    Revoked,
 }
 
 pub enum ImportResult {
@@ -262,7 +262,9 @@ pub trait Database: Sync + Send {
                 }
             })
             .collect();
-        email_status.sort_by(|(e1,_),(e2,_)| e1.cmp(e2));
+        email_status.sort();
+        // EmailAddressStatus is ordered published, unpublished, revoked. if there are multiple for
+        // the same address, we keep the first.
         email_status.dedup_by(|(e1, _), (e2, _)| e1 == e2);
 
         // Abort if no changes were made
@@ -413,7 +415,9 @@ pub trait Database: Sync + Send {
                 }
             })
             .collect();
-        email_status.sort_by(|(e1,_),(e2,_)| e1.cmp(e2));
+        email_status.sort();
+        // EmailAddressStatus is ordered published, unpublished, revoked. if there are multiple for
+        // the same address, we keep the first.
         email_status.dedup_by(|(e1, _), (e2, _)| e1 == e2);
 
         Ok(TpkStatus { is_revoked, email_status, unparsed_uids })

database/src/test.rs

@@ -870,7 +870,9 @@ pub fn test_same_email_1(db: &mut impl Database, log_path: &Path) {
 }
 
 // If a key has multiple user ids with the same email address, make
-// sure things still work.
+// sure things still work. We do this twice (see above), to
+// make sure the order isn't relevant when revoking one user id
+// but leaving the other.
 pub fn test_same_email_2(db: &mut impl Database, log_path: &Path) {
     use std::{thread, time};
 
@@ -935,11 +937,80 @@ pub fn test_same_email_2(db: &mut impl Database, log_path: &Path) {
     }, tpk_status);
 
     // fetch by both user ids.  We should still get both user ids.
-    // TODO should this still deliver uid2.clone()?
     assert_eq!(get_userids(&db.by_email(&email).unwrap()[..]),
                vec![ uid1.clone() ]);
+}
+
+// If a key has multiple user ids with the same email address, make
+// sure things still work. We do this twice (see above), to
+// make sure the order isn't relevant when revoking one user id
+// but leaving the other.
+pub fn test_same_email_3(db: &mut impl Database, log_path: &Path) {
+    use std::{thread, time};
+
+    let str_uid1 = "A <test@example.com>";
+    let str_uid2 = "B <test@example.com>";
+    let tpk = CertBuilder::new()
+        .add_userid(str_uid1)
+        .add_userid(str_uid2)
+        .generate()
+        .unwrap()
+        .0;
+    let uid1 = UserID::from(str_uid1);
+    let uid2 = UserID::from(str_uid2);
+    let email = Email::from_str(str_uid1).unwrap();
+    let fpr = Fingerprint::try_from(tpk.fingerprint()).unwrap();
+
+    // upload key
+    let tpk_status = db.merge(tpk.clone()).unwrap().into_tpk_status();
+    check_log_entry(log_path, &fpr);
+
+    // verify uid1
+    assert_eq!(TpkStatus {
+        is_revoked: false,
+        email_status: vec!(
+            (email.clone(), EmailAddressStatus::NotPublished),
+        ),
+        unparsed_uids: 0,
+    }, tpk_status);
+    db.set_email_published(&fpr, &tpk_status.email_status[0].0).unwrap();
+
+    // fetch by both user ids.
     assert_eq!(get_userids(&db.by_email(&email).unwrap()[..]),
-               vec![ uid1.clone() ]);
+               vec![ uid1.clone(), uid2.clone() ]);
+
+    thread::sleep(time::Duration::from_secs(2));
+
+    // revoke one uid
+    let sig = {
+        let uid = tpk.userids()
+            .with_policy(&*POLICY, None)
+            .find(|b| *b.userid() == uid1).unwrap();
+        assert_eq!(RevocationStatus::NotAsFarAsWeKnow, uid.revoked());
+
+        let mut keypair = tpk.primary_key().bundle().key().clone()
+            .mark_parts_secret().unwrap()
+            .into_keypair().unwrap();
+        UserIDRevocationBuilder::new()
+            .set_reason_for_revocation(ReasonForRevocation::UIDRetired, b"It was the maid :/").unwrap()
+            .build(&mut keypair, &tpk, uid.userid(), None)
+            .unwrap()
+    };
+    assert_eq!(sig.typ(), SignatureType::CertificationRevocation);
+    let tpk = tpk.merge_packets(vec![sig.into()]).unwrap();
+    let tpk_status = db.merge(tpk).unwrap().into_tpk_status();
+    check_log_entry(log_path, &fpr);
+    assert_eq!(TpkStatus {
+        is_revoked: false,
+        email_status: vec!(
+            (email.clone(), EmailAddressStatus::Published),
+        ),
+        unparsed_uids: 0,
+    }, tpk_status);
+
+    // fetch by both user ids.  We should still get both user ids.
+    assert_eq!(get_userids(&db.by_email(&email).unwrap()[..]),
+               vec![ uid2.clone() ]);
 }
 
 pub fn test_bad_uids(db: &mut impl Database, log_path: &Path) {