openpgp: TPKBuilder::autocrypt should not default to an empty UID

  - TPKBuilder::autocrypt created a TPK with a single User ID, as
    required by the Autocrypt specification.  Since no User ID was
    passed, it used the empty string.

  - An empty User ID is a bit surprising, and it is unclear if it is
    even a reasonable default (GnuPG rejects it).  But, even if the
    programmer is aware of this, adding a new user ID does not replace
    the empty User ID, and removing the empty User ID is a pain.

  - Change the API to better match typical usage: have the constructor
    take the User ID.

  - Nevertheless, preserve the flexibility by making the User ID
    optional to allow the caller to add a User ID later.  In this
    case, a non-autocrypt compliant TPK with no User ID is created
    instead of an empty User ID.

  - Closes #146.

4 files changed, 90 insertions, 21 deletions

diff --git a/openpgp-ffi/src/tpk.rs b/openpgp-ffi/src/tpk.rs
index b4026604..1649a6ae 100644
--- a/openpgp-ffi/src/tpk.rs
+++ b/openpgp-ffi/src/tpk.rs
@@ -574,10 +574,25 @@ pub extern "system" fn pgp_tpk_builder_default() -> *mut TPKBuilder {
 
 /// Generates a key compliant to [Autocrypt Level 1].
 ///
+/// Autocrypt requires a user id, however, if `uid` is NULL, a TPK is
+/// created without any user ids.  It is then the caller's
+/// responsibility to ensure that a user id is added later.
+///
+/// `uid` must contain valid UTF-8.  If it does not contain valid
+/// UTF-8, then the invalid code points are silently replaced with
+/// `U+FFFD REPLACEMENT CHARACTER`.
+///
 ///   [Autocrypt Level 1]: https://autocrypt.org/level1.html
 #[::ffi_catch_abort] #[no_mangle]
-pub extern "system" fn pgp_tpk_builder_autocrypt() -> *mut TPKBuilder {
-    box_raw!(TPKBuilder::autocrypt(Autocrypt::V1))
+pub extern "system" fn pgp_tpk_builder_autocrypt(uid: *const c_char)
+    -> *mut TPKBuilder
+{
+    let uid = if uid.is_null() {
+        None
+    } else {
+        Some(ffi_param_cstr!(uid).to_string_lossy())
+    };
+    box_raw!(TPKBuilder::autocrypt(Autocrypt::V1, uid))
 }
 
 /// Frees an `pgp_tpk_builder_t`.
@@ -607,6 +622,10 @@ pub extern "system" fn pgp_tpk_builder_set_cipher_suite
 
 /// Adds a new user ID. The first user ID added replaces the default
 /// ID that is just the empty string.
+///
+/// `uid` must contain valid UTF-8.  If it does not contain valid
+/// UTF-8, then the invalid code points are silently replaced with
+/// `U+FFFD REPLACEMENT CHARACTER`.
 #[::ffi_catch_abort] #[no_mangle]
 pub extern "system" fn pgp_tpk_builder_add_userid
     (tpkb: *mut *mut TPKBuilder, uid: *const c_char)
diff --git a/openpgp/src/tpk/builder.rs b/openpgp/src/tpk/builder.rs
index 014ec5e4..8ab86cd4 100644
--- a/openpgp/src/tpk/builder.rs
+++ b/openpgp/src/tpk/builder.rs
@@ -89,12 +89,19 @@ impl Default for TPKBuilder {
 
 impl TPKBuilder {
     /// Generates a key compliant to
-    /// [Autocrypt](https://autocrypt.org/). If no version is given the latest
-    /// one is used.
-    pub fn autocrypt<V>(_: V)
-        -> Self where V: Into<Option<Autocrypt>>
+    /// [Autocrypt](https://autocrypt.org/).
+    ///
+    /// If no version is given the latest one is used.
+    ///
+    /// The autocrypt specification requires a UserID.  However,
+    /// because it can be useful to add the UserID later, it is
+    /// permitted to be none.
+    pub fn autocrypt<'a, V, S>(_: V, userid: S)
+        -> Self
+        where V: Into<Option<Autocrypt>>,
+            S: Into<Option<Cow<'a, str>>>
     {
-        TPKBuilder{
+        let builder = TPKBuilder{
             ciphersuite: CipherSuite::RSA3k,
             primary: KeyBlueprint{
                 flags: KeyFlags::default()
@@ -108,8 +115,14 @@ impl TPKBuilder {
                         .set_encrypt_at_rest(true)
                 }
             ],
-            userids: vec!["".into()],
+            userids: vec![],
             password: None,
+        };
+
+        if let Some(userid) = userid.into() {
+            builder.add_userid(userid)
+        } else {
+            builder
         }
     }
 
@@ -474,12 +487,13 @@ mod tests {
 
     #[test]
     fn autocrypt() {
-        let (tpk1, _) = TPKBuilder::autocrypt(None)
+        let (tpk1, _) = TPKBuilder::autocrypt(None, Some("Foo".into()))
             .generate().unwrap();
         assert_eq!(tpk1.primary().pk_algo(),
                    PublicKeyAlgorithm::RSAEncryptSign);
         assert_eq!(tpk1.subkeys().next().unwrap().subkey().pk_algo(),
                    PublicKeyAlgorithm::RSAEncryptSign);
+        assert_eq!(tpk1.userids().count(), 1);
     }
 
     #[test]
diff --git a/openpgp/src/tpk/mod.rs b/openpgp/src/tpk/mod.rs
index 8e9b5f14..7ac044f6 100644
--- a/openpgp/src/tpk/mod.rs
+++ b/openpgp/src/tpk/mod.rs
@@ -3624,9 +3624,7 @@ mod test {
     // subkeys with and without a private key are merged.
     #[test]
     fn public_private_merge() {
-        use std::borrow::Cow;
-
-        let (tsk, _) = TSK::new(Cow::Borrowed("foo@example.com")).unwrap();
+        let (tsk, _) = TSK::new(Some("foo@example.com".into())).unwrap();
         // tsk is now a tpk, but it still has its private bits.
         let tsk = tsk.into_tpk();
         assert!(tsk.primary.secret().is_some());
diff --git a/openpgp/src/tsk.rs b/openpgp/src/tsk.rs
index fff2a7ed..57a6ff2d 100644
--- a/openpgp/src/tsk.rs
+++ b/openpgp/src/tsk.rs
@@ -77,17 +77,12 @@ impl TSK {
     /// Generates a new key OpenPGP key. The key will be capable of encryption
     /// and signing. If no user id is given the primary self signature will be
     /// a direct key signature.
-    pub fn new<'a, O: Into<Option<Cow<'a,str>>>>(primary_uid: O)
-                                                 -> Result<(TSK, Signature)> {
+    pub fn new<'a, O>(primary_uid: O) -> Result<(TSK, Signature)>
+        where O: Into<Option<Cow<'a, str>>>
+    {
         use tpk::TPKBuilder;
 
-        let mut key = TPKBuilder::autocrypt(None);
-
-        match primary_uid.into() {
-            Some(uid) => { key = key.add_userid(uid); }
-            None => {}
-        }
-
+        let key = TPKBuilder::autocrypt(None, primary_uid);
         let (tpk, revocation) = key.generate()?;
         Ok((TSK::from_tpk(tpk), revocation))
     }
@@ -212,6 +207,7 @@ impl Serialize for TSK {
 
 #[cfg(test)]
 mod tests {
+    use super::*;
     use tpk::TPKBuilder;
 
     #[test]
@@ -251,4 +247,46 @@ mod tests {
                 tpk2.userids().next().unwrap().userid()).unwrap(),
             true);
     }
+
+    #[test]
+    fn user_ids() {
+        let (tpk, _) = TPKBuilder::default()
+            .add_userid("test1@example.com")
+            .add_userid("test2@example.com")
+            .generate().unwrap();
+
+        let userids = tpk
+            .userids()
+            .map(|binding| binding.userid().userid())
+            .collect::<Vec<_>>();
+        assert_eq!(userids.len(), 2);
+        assert!((userids[0] == b"test1@example.com"
+                 && userids[1] == b"test2@example.com")
+                || (userids[0] == b"test2@example.com"
+                    && userids[1] == b"test1@example.com"),
+                "User ids: {:?}", userids);
+
+
+        let (tpk, _) = TPKBuilder::autocrypt(None, Some("Foo".into()))
+            .generate()
+            .unwrap();
+
+        let userids = tpk
+            .userids()
+            .map(|binding| binding.userid().userid())
+            .collect::<Vec<_>>();
+        assert_eq!(userids.len(), 1);
+        assert_eq!(userids[0], b"Foo");
+
+
+        let (tsk, _) = TSK::new(Some("test@example.com".into())).unwrap();
+        let tpk = tsk.into_tpk();
+        let userids = tpk
+            .userids()
+            .map(|binding| binding.userid().userid())
+            .collect::<Vec<_>>();
+        assert_eq!(userids.len(), 1);
+        assert_eq!(userids[0], b"test@example.com",
+                   "User ids: {:?}", userids);
+    }
 }