consolidate: make it work

4 files changed, 126 insertions, 105 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
index 5b353efbf..c06255830 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
@@ -27,11 +27,13 @@ import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.util.Arrays;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 import java.util.Vector;
 
 /** Wrapper around PGPKeyRing class, to be constructed from bytes.
@@ -190,10 +192,6 @@ public class UncachedKeyRing {
      */
     @SuppressWarnings("ConstantConditions")
     public UncachedKeyRing canonicalize(OperationLog log, int indent) {
-        if (isSecret()) {
-            throw new RuntimeException("Tried to public-canonicalize non-public keyring. " +
-                    "This is a programming error and should never happen!");
-        }
 
         log.add(LogLevel.START, isSecret() ? LogType.MSG_KC_SECRET : LogType.MSG_KC_PUBLIC,
                 new String[]{PgpKeyHelper.convertKeyIdToHex(getMasterKeyId())}, indent);
@@ -611,126 +609,130 @@ public class UncachedKeyRing {
         return new UncachedKeyRing(ring);
     }
 
-    /** This operation consolidates a list of UncachedKeyRings into a single, combined
+    /** This operation merges information from a different keyring, returning a combined
      * UncachedKeyRing.
      *
-     * The combined keyring contains the subkeys and user ids of all input keyrings. Even if all
-     * input keyrings were canonicalized at some point, the resulting keyring will not necessarily
-     * have that property.
-     *
-     * TODO work with secret keys
+     * The combined keyring contains the subkeys and user ids of both input keyrings, but it does
+     * not necessarily have the canonicalized property.
      *
-     * @param list The list of UncachedKeyRings. Must not be empty, and all of the same masterKeyId
-     * @return A consolidated UncachedKeyRing with the data of all input keyrings.
+     * @param other The UncachedKeyRing to merge. Must not be empty, and of the same masterKeyId
+     * @return A consolidated UncachedKeyRing with the data of both input keyrings. Same type as
+     * this object, or null on error.
      *
      */
-    public static UncachedKeyRing consolidate(List<UncachedKeyRing> list,
-                                              OperationLog log, int indent) {
+    public UncachedKeyRing merge(UncachedKeyRing other, OperationLog log, int indent) {
 
-        long masterKeyId = list.get(0).getMasterKeyId();
-
-        log.add(LogLevel.START, LogType.MSG_KO,
-                new String[]{
-                        Integer.toString(list.size()),
-                        PgpKeyHelper.convertKeyIdToHex(masterKeyId)
-                }, indent);
+        log.add(LogLevel.START, isSecret() ? LogType.MSG_MG_SECRET : LogType.MSG_MG_PUBLIC,
+                new String[]{ PgpKeyHelper.convertKeyIdToHex(getMasterKeyId()) }, indent);
         indent += 1;
 
-        // remember which certs we already added
-        HashSet<Integer> certs = new HashSet<Integer>();
+        long masterKeyId = other.getMasterKeyId();
 
-        try {
-            PGPPublicKeyRing result = null;
-            int num = 1;
-            for (UncachedKeyRing uring : new IterableIterator<UncachedKeyRing>(list.iterator())) {
-
-                PGPPublicKeyRing ring = (PGPPublicKeyRing) uring.mRing;
-                if (uring.getMasterKeyId() != masterKeyId) {
-                    log.add(LogLevel.ERROR, LogType.MSG_KO_HETEROGENEOUS, null, indent);
-                    return null;
+        if (getMasterKeyId() != masterKeyId) {
+            log.add(LogLevel.ERROR, LogType.MSG_MG_HETEROGENEOUS, null, indent);
+            return null;
+        }
+
+        // remember which certs we already added. this is cheaper than semantic deduplication
+        Set<byte[]> certs = new TreeSet<byte[]>(new Comparator<byte[]>() {
+            public int compare(byte[] left, byte[] right) {
+                // check for length equality
+                if (left.length != right.length) {
+                    return left.length - right.length;
+                }
+                // compare byte-by-byte
+                for (int i = 0; i < left.length && i < right.length; i++) {
+                    if (left[i] != right[i]) {
+                        return (left[i] & 0xff) - (right[i] & 0xff);
+                    }
                 }
+                // ok they're the same
+                return 0;
+        }});
 
-                // If this is the first ring, just take it
-                if (result == null) {
-                    result = ring;
-                    continue;
+        try {
+            PGPKeyRing result = mRing;
+            PGPKeyRing candidate = other.mRing;
+
+            // Pre-load all existing certificates
+            for (PGPPublicKey key : new IterableIterator<PGPPublicKey>(result.getPublicKeys())) {
+                for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignatures())) {
+                    certs.add(cert.getEncoded());
                 }
+            }
 
-                log.add(LogLevel.DEBUG, LogType.MSG_KO_MERGING,
-                        new String[] { Integer.toString(num++) }, indent);
-                indent += 1;
+            // keep track of the number of new certs we add
+            int newCerts = 0;
 
-                // keep track of the number of new certs we add
-                int newCerts = 0;
+            for (PGPPublicKey key : new IterableIterator<PGPPublicKey>(candidate.getPublicKeys())) {
 
-                for (PGPPublicKey key : new IterableIterator<PGPPublicKey>(ring.getPublicKeys())) {
+                final PGPPublicKey resultKey = result.getPublicKey(key.getKeyID());
+                if (resultKey == null) {
+                    log.add(LogLevel.DEBUG, LogType.MSG_MG_NEW_SUBKEY, null, indent);
+                    result = replacePublicKey(result, key);
+                    continue;
+                }
 
-                    final PGPPublicKey resultkey = result.getPublicKey(key.getKeyID());
-                    if (resultkey == null) {
-                        log.add(LogLevel.DEBUG, LogType.MSG_KO_NEW_SUBKEY, null, indent);
-                        result = PGPPublicKeyRing.insertPublicKey(result, key);
+                // Modifiable version of the old key, which we merge stuff into (keep old for comparison)
+                PGPPublicKey modified = resultKey;
+
+                // Iterate certifications
+                for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignatures())) {
+                    int type = cert.getSignatureType();
+                    // Disregard certifications on user ids, we will deal with those later
+                    if (type == PGPSignature.NO_CERTIFICATION
+                            || type == PGPSignature.DEFAULT_CERTIFICATION
+                            || type == PGPSignature.CASUAL_CERTIFICATION
+                            || type == PGPSignature.POSITIVE_CERTIFICATION
+                            || type == PGPSignature.CERTIFICATION_REVOCATION) {
                         continue;
                     }
 
-                    // The key old key, which we merge stuff into
-                    PGPPublicKey modified = resultkey;
-                    for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignatures())) {
-                        int type = cert.getSignatureType();
-                        // Disregard certifications on user ids, we will deal with those later
-                        if (type == PGPSignature.NO_CERTIFICATION
-                                || type == PGPSignature.DEFAULT_CERTIFICATION
-                                || type == PGPSignature.CASUAL_CERTIFICATION
-                                || type == PGPSignature.POSITIVE_CERTIFICATION
-                                || type == PGPSignature.CERTIFICATION_REVOCATION) {
-                            continue;
-                        }
-
-                        int hash = Arrays.hashCode(cert.getEncoded());
-                        // Known cert, skip it
-                        if (certs.contains(hash)) {
-                            continue;
-                        }
-                        certs.add(hash);
-                        modified = PGPPublicKey.addCertification(modified, cert);
-                        newCerts += 1;
+                    byte[] encoded = cert.getEncoded();
+                    // Known cert, skip it
+                    if (certs.contains(encoded)) {
+                        continue;
                     }
+                    certs.add(encoded);
+                    modified = PGPPublicKey.addCertification(modified, cert);
+                    newCerts += 1;
+                }
 
-                    // If this is a subkey, stop here
-                    if (!key.isMasterKey()) {
-                        result = PGPPublicKeyRing.insertPublicKey(result, modified);
-                        continue;
+                // If this is a subkey, merge it in and stop here
+                if (!key.isMasterKey()) {
+                    if (modified != resultKey) {
+                        result = replacePublicKey(result, modified);
                     }
+                    continue;
+                }
 
-                    // Copy over all user id certificates
-                    for (String userId : new IterableIterator<String>(key.getUserIDs())) {
-                        for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignaturesForID(userId))) {
-                            int hash = Arrays.hashCode(cert.getEncoded());
-                            // Known cert, skip it
-                            if (certs.contains(hash)) {
-                                continue;
-                            }
-                            newCerts += 1;
-                            certs.add(hash);
-                            modified = PGPPublicKey.addCertification(modified, userId, cert);
+                // Copy over all user id certificates
+                for (String userId : new IterableIterator<String>(key.getUserIDs())) {
+                    for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignaturesForID(userId))) {
+                        byte[] encoded = cert.getEncoded();
+                        // Known cert, skip it
+                        if (certs.contains(encoded)) {
+                            continue;
                         }
+                        newCerts += 1;
+                        certs.add(encoded);
+                        modified = PGPPublicKey.addCertification(modified, userId, cert);
                     }
-                    // If anything changed, save the updated (sub)key
-                    if (modified != resultkey) {
-                        result = PGPPublicKeyRing.insertPublicKey(result, modified);
-                    }
-
                 }
-
-                log.add(LogLevel.DEBUG, LogType.MSG_KO_FOUND_NEW,
-                        new String[] { Integer.toString(newCerts) }, indent);
-
+                // If anything changed, save the updated (sub)key
+                if (modified != resultKey) {
+                    result = replacePublicKey(result, modified);
+                }
 
             }
 
+            log.add(LogLevel.DEBUG, LogType.MSG_MG_FOUND_NEW,
+                    new String[] { Integer.toString(newCerts) }, indent);
+
             return new UncachedKeyRing(result);
 
         } catch (IOException e) {
-            log.add(LogLevel.ERROR, LogType.MSG_KO_FATAL_ENCODE, null, indent);
+            log.add(LogLevel.ERROR, LogType.MSG_MG_FATAL_ENCODE, null, indent);
             return null;
         }
 
@@ -749,6 +751,10 @@ public class UncachedKeyRing {
         }
         PGPSecretKeyRing secRing = (PGPSecretKeyRing) ring;
         PGPSecretKey sKey = secRing.getSecretKey(key.getKeyID());
+        // TODO generate secret key with S2K dummy, if none exists! for now, just die.
+        if (sKey == null) {
+            throw new RuntimeException("dummy secret key generation not yet implemented");
+        }
         sKey = PGPSecretKey.replacePublicKey(sKey, key);
         return PGPSecretKeyRing.insertSecretKey(secRing, sKey);
     }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
index 248d15ba6..457ef1ec8 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
@@ -677,6 +677,19 @@ public class ProviderHelper {
             secretRing = getWrappedSecretKeyRing(keyRing.getMasterKeyId()).getUncached();
             log(LogLevel.DEBUG, LogType.MSG_IP_PRESERVING_SECRET);
             progress.setProgress(LogType.MSG_IP_PRESERVING_SECRET.getMsgId(), 10, 100);
+            mIndent += 1;
+
+            // Merge data from new public ring into secret one
+            secretRing = secretRing.merge(keyRing, mLog, mIndent);
+            if (secretRing == null) {
+                return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+            }
+
+            // NOTE that the info from this secret keyring will implicitly be merged into the
+            // new public keyring, since that one is merged with the old public keyring.
+
+            mIndent -= 1;
+
         } catch (NotFoundException e) {
             secretRing = null;
         }
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 408715fdf..4010ab108 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
@@ -222,12 +222,12 @@ public class OperationResultParcel implements Parcelable {
         MSG_KC_UID_REVOKE_OLD (R.string.msg_kc_uid_revoke_old),
 
         // keyring consolidation
-        MSG_KO (R.string.msg_ko),
-        MSG_KO_FATAL_ENCODE (R.string.msg_ko_fatal_encode),
-        MSG_KO_HETEROGENEOUS (R.string.msg_ko_heterogeneous),
-        MSG_KO_MERGING (R.string.msg_ko_merging),
-        MSG_KO_NEW_SUBKEY (R.string.msg_ko_new_subkey),
-        MSG_KO_FOUND_NEW (R.string.msg_ko_found_new),
+        MSG_MG_PUBLIC (R.string.msg_mg_public),
+        MSG_MG_SECRET (R.string.msg_mg_secret),
+        MSG_MG_FATAL_ENCODE (R.string.msg_mg_fatal_encode),
+        MSG_MG_HETEROGENEOUS (R.string.msg_mg_heterogeneous),
+        MSG_MG_NEW_SUBKEY (R.string.msg_mg_new_subkey),
+        MSG_MG_FOUND_NEW (R.string.msg_mg_found_new),
         ;
 
         private final int mMsgId;
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index 60c323df4..4282d6abd 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -598,12 +598,14 @@
     <string name="msg_kc_uid_revoke_old">Removing outdated revocation certificate for user id "%s"</string>
     <string name="msg_kc_uid_no_cert">No valid self-certificate found for user id %s, removing from ring</string>
 
-    <string name="msg_ko">Consolidating %1$s keyrings into %2$s</string>
-    <string name="msg_ko_fatal_encode">Fatal error encoding signature</string>
-    <string name="msg_ko_heterogeneous">Tried to consolidate heterogeneous keyrings</string>
-    <string name="msg_ko_merging">Merging keyring #%s</string>
-    <string name="msg_ko_new_subkey">Adding new subkey %s</string>
-    <string name="msg_ko_found_new">Found %s new certificates in keyring</string>
+
+    <!-- Keyring merging log entries -->
+    <string name="msg_mg_public">Merging into secret keyring %s</string>
+    <string name="msg_mg_secret">Merging into public keyring %s</string>
+    <string name="msg_mg_fatal_encode">Fatal error encoding signature</string>
+    <string name="msg_mg_heterogeneous">Tried to consolidate heterogeneous keyrings</string>
+    <string name="msg_mg_new_subkey">Adding new subkey %s</string>
+    <string name="msg_mg_found_new">Found %s new certificates in keyring</string>
 
     <!-- unsorted -->
     <string name="section_certifier_id">Certifier</string>