createKey: better logging, handle empty user id case

3 files changed, 66 insertions, 44 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
index 8dbc2208c..3e7e9d98e 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
@@ -102,11 +102,12 @@ public class PgpKeyOperation {
     }
 
     /** Creates new secret key. */
-    private PGPKeyPair createKey(int algorithmChoice, int keySize) throws PgpGeneralMsgIdException {
+    private PGPKeyPair createKey(int algorithmChoice, int keySize, OperationLog log, int indent) {
 
         try {
             if (keySize < 512) {
-                throw new PgpGeneralMsgIdException(R.string.error_key_size_minimum512bit);
+                log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_KEYSIZE_512, indent);
+                return null;
             }
 
             int algorithm;
@@ -141,7 +142,8 @@ public class PgpKeyOperation {
                 }
 
                 default: {
-                    throw new PgpGeneralMsgIdException(R.string.error_unknown_algorithm_choice);
+                    log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_UNKNOWN_ALGO, indent);
+                    return null;
                 }
             }
 
@@ -155,7 +157,9 @@ public class PgpKeyOperation {
         } catch(InvalidAlgorithmParameterException e) {
             throw new RuntimeException(e);
         } catch(PGPException e) {
-            throw new PgpGeneralMsgIdException(R.string.msg_mf_error_pgp, e);
+            Log.e(Constants.TAG, "internal pgp error", e);
+            log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_INTERNAL_PGP, indent);
+            return null;
         }
     }
 
@@ -168,21 +172,32 @@ public class PgpKeyOperation {
             indent += 1;
             updateProgress(R.string.progress_building_key, 0, 100);
 
-            if (saveParcel.mAddSubKeys == null || saveParcel.mAddSubKeys.isEmpty()) {
+            if (saveParcel.mAddSubKeys.isEmpty()) {
                 log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_NO_MASTER, indent);
                 return null;
             }
 
+            if (saveParcel.mAddUserIds.isEmpty()) {
+                log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_NO_USER_ID, indent);
+                return null;
+            }
+
             SubkeyAdd add = saveParcel.mAddSubKeys.remove(0);
             if ((add.mFlags & KeyFlags.CERTIFY_OTHER) != KeyFlags.CERTIFY_OTHER) {
                 log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_NO_CERTIFY, indent);
                 return null;
             }
 
-            PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize);
+            PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize, log, indent);
 
             if (add.mAlgorithm == Constants.choice.algorithm.elgamal) {
-                throw new PgpGeneralMsgIdException(R.string.error_master_key_must_not_be_el_gamal);
+                log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_MASTER_ELGAMAL, indent);
+                return null;
+            }
+
+            // return null if this failed (it will already have been logged by createKey)
+            if (keyPair == null) {
+                return null;
             }
 
             // define hashing and signing algos
@@ -201,14 +216,12 @@ public class PgpKeyOperation {
             return internal(sKR, masterSecretKey, add.mFlags, saveParcel, "", log, indent);
 
         } catch (PGPException e) {
+            log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_INTERNAL_PGP, indent);
             Log.e(Constants.TAG, "pgp error encoding key", e);
             return null;
         } catch (IOException e) {
             Log.e(Constants.TAG, "io error encoding key", e);
             return null;
-        } catch (PgpGeneralMsgIdException e) {
-            Log.e(Constants.TAG, "pgp msg id error", e);
-            return null;
         }
 
     }
@@ -521,47 +534,46 @@ public class PgpKeyOperation {
 
             // 5. Generate and add new subkeys
             for (SaveKeyringParcel.SubkeyAdd add : saveParcel.mAddSubKeys) {
-                try {
-
-                    if (add.mExpiry != null && new Date(add.mExpiry*1000).before(new Date())) {
-                        log.add(LogLevel.ERROR, LogType.MSG_MF_SUBKEY_PAST_EXPIRY, indent +1);
-                        return null;
-                    }
-
-                    log.add(LogLevel.INFO, LogType.MSG_MF_SUBKEY_NEW, indent);
 
-                    // generate a new secret key (privkey only for now)
-                    PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize);
-
-                    // add subkey binding signature (making this a sub rather than master key)
-                    PGPPublicKey pKey = keyPair.getPublicKey();
-                    PGPSignature cert = generateSubkeyBindingSignature(
-                            masterPublicKey, masterPrivateKey, keyPair.getPrivateKey(), pKey,
-                            add.mFlags, add.mExpiry == null ? 0 : add.mExpiry);
-                    pKey = PGPPublicKey.addSubkeyBindingCertification(pKey, cert);
+                if (add.mExpiry != null && new Date(add.mExpiry*1000).before(new Date())) {
+                    log.add(LogLevel.ERROR, LogType.MSG_MF_SUBKEY_PAST_EXPIRY, indent +1);
+                    return null;
+                }
 
-                    PGPSecretKey sKey; {
-                        // define hashing and signing algos
-                        PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder()
-                                .build().get(HashAlgorithmTags.SHA1);
+                log.add(LogLevel.INFO, LogType.MSG_MF_SUBKEY_NEW, indent);
 
-                        // Build key encrypter and decrypter based on passphrase
-                        PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder(
-                                PGPEncryptedData.CAST5, sha1Calc)
-                                .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray());
+                // generate a new secret key (privkey only for now)
+                PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize, log, indent);
+                if(keyPair == null) {
+                    return null;
+                }
 
-                        sKey = new PGPSecretKey(keyPair.getPrivateKey(), pKey,
-                                sha1Calc, false, keyEncryptor);
-                    }
+                // add subkey binding signature (making this a sub rather than master key)
+                PGPPublicKey pKey = keyPair.getPublicKey();
+                PGPSignature cert = generateSubkeyBindingSignature(
+                        masterPublicKey, masterPrivateKey, keyPair.getPrivateKey(), pKey,
+                        add.mFlags, add.mExpiry == null ? 0 : add.mExpiry);
+                pKey = PGPPublicKey.addSubkeyBindingCertification(pKey, cert);
+
+                PGPSecretKey sKey; {
+                    // define hashing and signing algos
+                    PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder()
+                            .build().get(HashAlgorithmTags.SHA1);
+
+                    // Build key encrypter and decrypter based on passphrase
+                    PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder(
+                            PGPEncryptedData.CAST5, sha1Calc)
+                            .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray());
+
+                    sKey = new PGPSecretKey(keyPair.getPrivateKey(), pKey,
+                            sha1Calc, false, keyEncryptor);
+                }
 
-                    log.add(LogLevel.DEBUG, LogType.MSG_MF_SUBKEY_NEW_ID,
-                            indent+1, PgpKeyHelper.convertKeyIdToHex(sKey.getKeyID()));
+                log.add(LogLevel.DEBUG, LogType.MSG_MF_SUBKEY_NEW_ID,
+                        indent+1, PgpKeyHelper.convertKeyIdToHex(sKey.getKeyID()));
 
-                    sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
+                sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
 
-                } catch (PgpGeneralMsgIdException e) {
-                    return null;
-                }
             }
 
             // 6. If requested, change passphrase
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
index c160da483..67386da06 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
@@ -246,7 +246,12 @@ public class OperationResultParcel implements Parcelable {
         // secret key create
         MSG_CR (R.string.msg_cr),
         MSG_CR_ERROR_NO_MASTER (R.string.msg_cr_error_no_master),
+        MSG_CR_ERROR_NO_USER_ID (R.string.msg_cr_error_no_user_id),
         MSG_CR_ERROR_NO_CERTIFY (R.string.msg_cr_error_no_certify),
+        MSG_CR_ERROR_KEYSIZE_512 (R.string.msg_cr_error_keysize_512),
+        MSG_CR_ERROR_UNKNOWN_ALGO (R.string.msg_cr_error_unknown_algo),
+        MSG_CR_ERROR_INTERNAL_PGP (R.string.msg_cr_error_internal_pgp),
+        MSG_CR_ERROR_MASTER_ELGAMAL (R.string.msg_cr_error_master_elgamal),
 
         // secret key modify
         MSG_MF (R.string.msg_mr),
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index d8e102b04..5efe51aa9 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -640,7 +640,12 @@
     <!-- createSecretKeyRing -->
     <string name="msg_cr">Generating new master key</string>
     <string name="msg_cr_error_no_master">No master key options specified!</string>
+    <string name="msg_cr_error_no_user_id">Keyrings must be created with at least one user id!</string>
     <string name="msg_cr_error_no_certify">Master key must have certify flag!</string>
+    <string name="msg_cr_error_keysize_512">Key size must be greater or equal 512!</string>
+    <string name="msg_cr_error_internal_pgp">Internal PGP error!</string>
+    <string name="msg_cr_error_unknown_algo">Bad algorithm choice!</string>
+    <string name="msg_cr_error_master_elgamal">Master key must not be of type ElGamal!</string>
 
     <!-- modifySecretKeyRing -->
     <string name="msg_mr">Modifying keyring %s</string>