Merge branch 'master' into yubikey

Conflicts:
	OpenKeychain/src/main/java/org/sufficientlysecure/keychain/helper/Preferences.java
	OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java
	OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java

10 files changed, 628 insertions, 110 deletions

diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java
index 8e7d395fd..6a023c486 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java
@@ -1,3 +1,21 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 package org.sufficientlysecure.keychain.pgp;
 
 import junit.framework.AssertionFailedError;
@@ -16,11 +34,13 @@ import org.spongycastle.bcpg.SecretSubkeyPacket;
 import org.spongycastle.bcpg.SignaturePacket;
 import org.spongycastle.bcpg.UserIDPacket;
 import org.spongycastle.bcpg.sig.KeyFlags;
+import org.spongycastle.jce.provider.BouncyCastleProvider;
 import org.spongycastle.openpgp.PGPSignature;
-import org.spongycastle.bcpg.PublicKeyAlgorithmTags;
-import org.sufficientlysecure.keychain.Constants;
+import org.sufficientlysecure.keychain.service.OperationResultParcel.LogType;
 import org.sufficientlysecure.keychain.service.OperationResultParcel.OperationLog;
+import org.sufficientlysecure.keychain.service.OperationResults.EditKeyResult;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
+import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyAdd;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyChange;
 import org.sufficientlysecure.keychain.support.KeyringBuilder;
@@ -31,6 +51,7 @@ import org.sufficientlysecure.keychain.util.ProgressScaler;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.security.Security;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.Iterator;
@@ -42,7 +63,7 @@ import java.util.Random;
 public class PgpKeyOperationTest {
 
     static UncachedKeyRing staticRing;
-    static String passphrase;
+    final static String passphrase = genPassphrase();
 
     UncachedKeyRing ring;
     PgpKeyOperation op;
@@ -50,37 +71,29 @@ public class PgpKeyOperationTest {
     ArrayList<RawPacket> onlyA = new ArrayList<RawPacket>();
     ArrayList<RawPacket> onlyB = new ArrayList<RawPacket>();
 
-    @BeforeClass public static void setUpOnce() throws Exception {
+    @BeforeClass
+    public static void setUpOnce() throws Exception {
+        Security.insertProviderAt(new BouncyCastleProvider(), 1);
         ShadowLog.stream = System.out;
 
-        {
-            String chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789!@#$%^&*()-_=";
-            Random r = new Random();
-            StringBuilder passbuilder = new StringBuilder();
-            // 20% chance for an empty passphrase
-            for(int i = 0, j = r.nextInt(10) > 2 ? r.nextInt(20) : 0; i < j; i++) {
-                passbuilder.append(chars.charAt(r.nextInt(chars.length())));
-            }
-            passphrase = passbuilder.toString();
-            System.out.println("Passphrase is '" + passphrase + "'");
-        }
-
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                Algorithm.DSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.ENCRYPT_COMMS, null));
+                Algorithm.ELGAMAL, 1024, null, KeyFlags.ENCRYPT_COMMS, 0L));
 
         parcel.mAddUserIds.add("twi");
         parcel.mAddUserIds.add("pink");
         parcel.mNewPassphrase = passphrase;
         PgpKeyOperation op = new PgpKeyOperation(null);
 
-        staticRing = op.createSecretKeyRing(parcel).getRing();
+        EditKeyResult result = op.createSecretKeyRing(parcel);
+        Assert.assertTrue("initial test key creation must succeed", result.success());
+        Assert.assertNotNull("initial test key creation must succeed", result.getRing());
 
-        Assert.assertNotNull("initial test key creation must succeed", staticRing);
+        staticRing = result.getRing();
 
         // we sleep here for a second, to make sure all new certificates have different timestamps
         Thread.sleep(1000);
@@ -107,57 +120,55 @@ public class PgpKeyOperationTest {
         {
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, new Random().nextInt(256)+255, KeyFlags.CERTIFY_OTHER, null));
+                    Algorithm.RSA, new Random().nextInt(256)+255, null, KeyFlags.CERTIFY_OTHER, 0L));
             parcel.mAddUserIds.add("shy");
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-
-            Assert.assertNull("creating ring with < 512 bytes keysize should fail", ring);
+            assertFailure("creating ring with < 512 bytes keysize should fail", parcel,
+                    LogType.MSG_CR_ERROR_KEYSIZE_512);
         }
 
         {
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.ELGAMAL_ENCRYPT, 1024, KeyFlags.CERTIFY_OTHER, null));
+                    Algorithm.ELGAMAL, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
             parcel.mAddUserIds.add("shy");
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-
-            Assert.assertNull("creating ring with ElGamal master key should fail", ring);
+            assertFailure("creating ring with ElGamal master key should fail", parcel,
+                    LogType.MSG_CR_ERROR_FLAGS_ELGAMAL);
         }
 
         {
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    12345, 1024, KeyFlags.CERTIFY_OTHER, null));
-            parcel.mAddUserIds.add("shy");
+                    Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, null));
+            parcel.mAddUserIds.add("lotus");
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-            Assert.assertNull("creating ring with bad algorithm choice should fail", ring);
+            assertFailure("creating master key with null expiry should fail", parcel,
+                    LogType.MSG_CR_ERROR_NULL_EXPIRY);
         }
 
         {
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
             parcel.mAddUserIds.add("shy");
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-            Assert.assertNull("creating ring with non-certifying master key should fail", ring);
+            assertFailure("creating ring with non-certifying master key should fail", parcel,
+                    LogType.MSG_CR_ERROR_NO_CERTIFY);
         }
 
         {
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-            Assert.assertNull("creating ring without user ids should fail", ring);
+            assertFailure("creating ring without user ids should fail", parcel,
+                    LogType.MSG_CR_ERROR_NO_USER_ID);
         }
 
         {
@@ -165,8 +176,8 @@ public class PgpKeyOperationTest {
             parcel.mAddUserIds.add("shy");
             parcel.mNewPassphrase = passphrase;
 
-            UncachedKeyRing ring = op.createSecretKeyRing(parcel).getRing();
-            Assert.assertNull("creating ring without subkeys should fail", ring);
+            assertFailure("creating ring with no master key should fail", parcel,
+                    LogType.MSG_CR_ERROR_NO_MASTER);
         }
 
     }
@@ -177,9 +188,9 @@ public class PgpKeyOperationTest {
     public void testMasterFlags() throws Exception {
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER | KeyFlags.SIGN_DATA, null));
+                Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER | KeyFlags.SIGN_DATA, 0L));
         parcel.mAddUserIds.add("luna");
-        ring = op.createSecretKeyRing(parcel).getRing();
+        ring = assertCreateSuccess("creating ring with master key flags must succeed", parcel);
 
         Assert.assertEquals("the keyring should contain only the master key",
                 1, KeyringTestingHelper.itToList(ring.getPublicKeys()).size());
@@ -239,10 +250,8 @@ public class PgpKeyOperationTest {
             parcel.mMasterKeyId = ring.getMasterKeyId() -1;
             parcel.mFingerprint = ring.getFingerprint();
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("keyring modification with bad master key id should fail", modified);
+            assertModifyFailure("keyring modification with bad master key id should fail",
+                    ring, parcel, LogType.MSG_MF_ERROR_KEYID);
         }
 
         {
@@ -251,10 +260,8 @@ public class PgpKeyOperationTest {
             parcel.mMasterKeyId = null;
             parcel.mFingerprint = ring.getFingerprint();
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("keyring modification with null master key id should fail", modified);
+            assertModifyFailure("keyring modification with null master key id should fail",
+                    ring, parcel, LogType.MSG_MF_ERROR_KEYID);
         }
 
         {
@@ -264,10 +271,8 @@ public class PgpKeyOperationTest {
             // some byte, off by one
             parcel.mFingerprint[5] += 1;
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("keyring modification with bad fingerprint should fail", modified);
+            assertModifyFailure("keyring modification with bad fingerprint should fail",
+                    ring, parcel, LogType.MSG_MF_ERROR_FINGERPRINT);
         }
 
         {
@@ -275,10 +280,8 @@ public class PgpKeyOperationTest {
             parcel.mMasterKeyId = ring.getMasterKeyId();
             parcel.mFingerprint = null;
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("keyring modification with null fingerprint should fail", modified);
+            assertModifyFailure("keyring modification with null fingerprint should fail",
+                    ring, parcel, LogType.MSG_MF_ERROR_FINGERPRINT);
         }
 
         {
@@ -286,10 +289,9 @@ public class PgpKeyOperationTest {
             if (badphrase.equals(passphrase)) {
                 badphrase = "a";
             }
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, badphrase).getRing();
 
-            Assert.assertNull("keyring modification with bad passphrase should fail", modified);
+            assertModifyFailure("keyring modification with bad passphrase should fail",
+                    ring, parcel, badphrase, LogType.MSG_MF_UNLOCK_ERROR);
         }
 
     }
@@ -300,7 +302,7 @@ public class PgpKeyOperationTest {
         long expiry = new Date().getTime() / 1000 + 159;
         int flags = KeyFlags.SIGN_DATA;
         int bits = 1024 + new Random().nextInt(8);
-        parcel.mAddSubKeys.add(new SubkeyAdd(PublicKeyAlgorithmTags.RSA_GENERAL, bits, flags, expiry));
+        parcel.mAddSubKeys.add(new SubkeyAdd(Algorithm.RSA, bits, null, flags, expiry));
 
         UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB);
 
@@ -336,28 +338,32 @@ public class PgpKeyOperationTest {
         Assert.assertEquals("added key must have expected flags",
                 flags, newKey.getKeyUsage());
         Assert.assertEquals("added key must have expected bitsize",
-                bits, newKey.getBitStrength());
+                bits, (int) newKey.getBitStrength());
 
         { // bad keysize should fail
             parcel.reset();
             parcel.mAddSubKeys.add(new SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, new Random().nextInt(512), KeyFlags.SIGN_DATA, null));
+                    Algorithm.RSA, new Random().nextInt(512), null, KeyFlags.SIGN_DATA, 0L));
+            assertModifyFailure("creating a subkey with keysize < 512 should fail", ring, parcel,
+                    LogType.MSG_CR_ERROR_KEYSIZE_512);
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
+        }
 
-            Assert.assertNull("creating a subkey with keysize < 512 should fail", modified);
+        {
+            parcel.reset();
+            parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
+                    Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, null));
+
+            assertModifyFailure("creating master key with null expiry should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_NULL_EXPIRY);
         }
 
         { // a past expiry should fail
             parcel.reset();
-            parcel.mAddSubKeys.add(new SubkeyAdd(PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA,
+            parcel.mAddSubKeys.add(new SubkeyAdd(Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA,
                     new Date().getTime()/1000-10));
-
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("creating subkey with past expiry date should fail", modified);
+            assertModifyFailure("creating subkey with past expiry date should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_PAST_EXPIRY);
         }
 
     }
@@ -394,6 +400,20 @@ public class PgpKeyOperationTest {
                     ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage());
         }
 
+        { // change expiry
+            expiry += 60*60*24;
+
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, expiry));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+            Assert.assertNotNull("modified key must have an expiry date",
+                    modified.getPublicKey(keyId).getExpiryTime());
+            Assert.assertEquals("modified key must have expected expiry date",
+                    expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000);
+            Assert.assertEquals("modified key must have same flags as before",
+                    ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage());
+        }
+
         {
             int flags = KeyFlags.SIGN_DATA | KeyFlags.ENCRYPT_COMMS;
             parcel.reset();
@@ -418,26 +438,169 @@ public class PgpKeyOperationTest {
                     expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000);
         }
 
+        { // expiry of 0 should be "no expiry"
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, 0L));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+            Assert.assertEquals("old packet must be signature",
+                    PacketTags.SIGNATURE, onlyA.get(0).tag);
+
+            Packet p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket();
+            Assert.assertTrue("first new packet must be signature", p instanceof SignaturePacket);
+            Assert.assertEquals("signature type must be subkey binding certificate",
+                    PGPSignature.SUBKEY_BINDING, ((SignaturePacket) p).getSignatureType());
+            Assert.assertEquals("signature must have been created by master key",
+                    ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID());
+
+            Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getExpiryTime());
+        }
+
         { // a past expiry should fail
             parcel.reset();
             parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, new Date().getTime()/1000-10));
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("setting subkey expiry to a past date should fail", modified);
+            assertModifyFailure("setting subkey expiry to a past date should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_PAST_EXPIRY);
         }
 
-        { // modifying nonexistent keyring should fail
+        { // modifying nonexistent subkey should fail
             parcel.reset();
             parcel.mChangeSubKeys.add(new SubkeyChange(123, null, null));
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
+            assertModifyFailure("modifying non-existent subkey should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_SUBKEY_MISSING);
+        }
+
+    }
+
+    @Test
+    public void testMasterModify() throws Exception {
+
+        long expiry = new Date().getTime()/1000 + 1024;
+        long keyId = ring.getMasterKeyId();
+
+        UncachedKeyRing modified = ring;
+
+        // to make this check less trivial, we add a user id, change the primary one and revoke one
+        parcel.mAddUserIds.add("aloe");
+        parcel.mChangePrimaryUserId = "aloe";
+        parcel.mRevokeUserIds.add("pink");
+        modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+        {
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, expiry));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+            // this implies that only the two non-revoked signatures were changed!
+            Assert.assertEquals("two extra packets in original", 2, onlyA.size());
+            Assert.assertEquals("two extra packets in modified", 2, onlyB.size());
+
+            Assert.assertEquals("first original packet must be a signature",
+                    PacketTags.SIGNATURE, onlyA.get(0).tag);
+            Assert.assertEquals("second original packet must be a signature",
+                    PacketTags.SIGNATURE, onlyA.get(1).tag);
+            Assert.assertEquals("first new packet must be signature",
+                    PacketTags.SIGNATURE, onlyB.get(0).tag);
+            Assert.assertEquals("first new packet must be signature",
+                    PacketTags.SIGNATURE, onlyB.get(1).tag);
+
+            Assert.assertNotNull("modified key must have an expiry date",
+                    modified.getPublicKey().getExpiryTime());
+            Assert.assertEquals("modified key must have expected expiry date",
+                    expiry, modified.getPublicKey().getExpiryTime().getTime() / 1000);
+            Assert.assertEquals("modified key must have same flags as before",
+                    ring.getPublicKey().getKeyUsage(), modified.getPublicKey().getKeyUsage());
+        }
+
+        { // change expiry
+            expiry += 60*60*24;
+
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, expiry));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
 
-            Assert.assertNull("modifying non-existent subkey should fail", modified);
+            Assert.assertNotNull("modified key must have an expiry date",
+                    modified.getPublicKey(keyId).getExpiryTime());
+            Assert.assertEquals("modified key must have expected expiry date",
+                    expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000);
+            Assert.assertEquals("modified key must have same flags as before",
+                    ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage());
         }
 
+        {
+            int flags = KeyFlags.CERTIFY_OTHER | KeyFlags.SIGN_DATA;
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, flags, null));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+            Assert.assertEquals("modified key must have expected flags",
+                    flags, modified.getPublicKey(keyId).getKeyUsage());
+            Assert.assertNotNull("key must retain its expiry",
+                    modified.getPublicKey(keyId).getExpiryTime());
+            Assert.assertEquals("key expiry must be unchanged",
+                    expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000);
+        }
+
+        { // expiry of 0 should be "no expiry"
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, 0L));
+            modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB);
+
+            Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getExpiryTime());
+        }
+
+        { // if we revoke everything, nothing is left to properly sign...
+            parcel.reset();
+            parcel.mRevokeUserIds.add("twi");
+            parcel.mRevokeUserIds.add("pink");
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, KeyFlags.CERTIFY_OTHER, null));
+
+            assertModifyFailure("master key modification with all user ids revoked should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_MASTER_NONE);
+        }
+
+        { // any flag not including CERTIFY_OTHER should fail
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, KeyFlags.SIGN_DATA, null));
+
+            assertModifyFailure("setting master key flags without certify should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_NO_CERTIFY);
+        }
+
+        { // a past expiry should fail
+            parcel.reset();
+            parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, new Date().getTime()/1000-10));
+
+            assertModifyFailure("setting subkey expiry to a past date should fail", ring, parcel,
+                    LogType.MSG_MF_ERROR_PAST_EXPIRY);
+        }
+
+    }
+
+    @Test
+    public void testMasterRevoke() throws Exception {
+
+        parcel.reset();
+        parcel.mRevokeSubKeys.add(ring.getMasterKeyId());
+
+        UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB);
+
+        Assert.assertEquals("no extra packets in original", 0, onlyA.size());
+        Assert.assertEquals("exactly one extra packet in modified", 1, onlyB.size());
+
+        Packet p;
+
+        p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket();
+        Assert.assertTrue("first new packet must be secret subkey", p instanceof SignaturePacket);
+        Assert.assertEquals("signature type must be subkey binding certificate",
+                PGPSignature.KEY_REVOCATION, ((SignaturePacket) p).getSignatureType());
+        Assert.assertEquals("signature must have been created by master key",
+                ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID());
+
+        Assert.assertTrue("subkey must actually be revoked",
+                modified.getPublicKey().isRevoked());
+
     }
 
     @Test
@@ -555,10 +718,8 @@ public class PgpKeyOperationTest {
             parcel.reset();
             parcel.mChangePrimaryUserId = uid;
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0);
-            UncachedKeyRing otherModified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("setting primary user id to a revoked user id should fail", otherModified);
+            assertModifyFailure("setting primary user id to a revoked user id should fail", modified, parcel,
+                    LogType.MSG_MF_ERROR_REVOKED_PRIMARY);
 
         }
 
@@ -596,6 +757,14 @@ public class PgpKeyOperationTest {
                     ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID());
         }
 
+        { // revocation of non-existent user id should fail
+            parcel.reset();
+            parcel.mRevokeUserIds.add("nonexistent");
+
+            assertModifyFailure("revocation of nonexistent user id should fail", modified, parcel,
+                    LogType.MSG_MF_ERROR_NOEXIST_REVOKE);
+        }
+
     }
 
     @Test
@@ -603,9 +772,8 @@ public class PgpKeyOperationTest {
 
         {
             parcel.mAddUserIds.add("");
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            UncachedKeyRing modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-            Assert.assertNull("adding an empty user id should fail", modified);
+            assertModifyFailure("adding an empty user id should fail", ring, parcel,
+                    LogType.MSG_MF_UID_ERROR_EMPTY);
         }
 
         parcel.reset();
@@ -673,22 +841,87 @@ public class PgpKeyOperationTest {
                 parcel.mChangePrimaryUserId += "A";
             }
 
-            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
-            modified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-
-            Assert.assertNull("changing primary user id to a non-existent one should fail", modified);
+            assertModifyFailure("changing primary user id to a non-existent one should fail",
+                    ring, parcel, LogType.MSG_MF_ERROR_NOEXIST_PRIMARY);
         }
 
         // check for revoked primary user id already done in revoke test
 
     }
 
+    @Test
+    public void testPassphraseChange() throws Exception {
+
+        // change passphrase to empty
+        parcel.mNewPassphrase = "";
+        UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB);
+
+        Assert.assertEquals("exactly three packets should have been modified (the secret keys)",
+                3, onlyB.size());
+
+        // remember secret key packet with no passphrase for later
+        RawPacket sKeyNoPassphrase = onlyB.get(1);
+        Assert.assertEquals("extracted packet should be a secret subkey",
+                PacketTags.SECRET_SUBKEY, sKeyNoPassphrase.tag);
+
+        // modify keyring, change to non-empty passphrase
+        String otherPassphrase = genPassphrase(true);
+        parcel.mNewPassphrase = otherPassphrase;
+        modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, "");
+
+        RawPacket sKeyWithPassphrase = onlyB.get(1);
+        Assert.assertEquals("extracted packet should be a secret subkey",
+                PacketTags.SECRET_SUBKEY, sKeyNoPassphrase.tag);
+
+        String otherPassphrase2 = genPassphrase(true);
+        parcel.mNewPassphrase = otherPassphrase2;
+        {
+            // if we replace a secret key with one without passphrase
+            modified = KeyringTestingHelper.removePacket(modified, sKeyNoPassphrase.position);
+            modified = KeyringTestingHelper.injectPacket(modified, sKeyNoPassphrase.buf, sKeyNoPassphrase.position);
+
+            // we should still be able to modify it (and change its passphrase) without errors
+            PgpKeyOperation op = new PgpKeyOperation(null);
+            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0);
+            EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, otherPassphrase);
+            Assert.assertTrue("key modification must succeed", result.success());
+            Assert.assertFalse("log must not contain a warning",
+                    result.getLog().containsWarnings());
+            Assert.assertTrue("log must contain an empty passphrase retry notice",
+                result.getLog().containsType(LogType.MSG_MF_PASSPHRASE_EMPTY_RETRY));
+            modified = result.getRing();
+        }
+
+        {
+            // if we add one subkey with a different passphrase, that should produce a warning but also work
+            modified = KeyringTestingHelper.removePacket(modified, sKeyWithPassphrase.position);
+            modified = KeyringTestingHelper.injectPacket(modified, sKeyWithPassphrase.buf, sKeyWithPassphrase.position);
+
+            PgpKeyOperation op = new PgpKeyOperation(null);
+            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0);
+            EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, otherPassphrase2);
+            Assert.assertTrue("key modification must succeed", result.success());
+            Assert.assertTrue("log must contain a warning",
+                    result.getLog().containsWarnings());
+            Assert.assertTrue("log must contain a failed passphrase change warning",
+                    result.getLog().containsType(LogType.MSG_MF_PASSPHRASE_FAIL));
+        }
+
+    }
 
     private static UncachedKeyRing applyModificationWithChecks(SaveKeyringParcel parcel,
                                                                UncachedKeyRing ring,
                                                                ArrayList<RawPacket> onlyA,
                                                                ArrayList<RawPacket> onlyB) {
-        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, true, true);
+        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, passphrase, true, true);
+    }
+
+    private static UncachedKeyRing applyModificationWithChecks(SaveKeyringParcel parcel,
+                                                               UncachedKeyRing ring,
+                                                               ArrayList<RawPacket> onlyA,
+                                                               ArrayList<RawPacket> onlyB,
+                                                               String passphrase) {
+        return applyModificationWithChecks(parcel, ring, onlyA, onlyB, passphrase, true, true);
     }
 
     // applies a parcel modification while running some integrity checks
@@ -696,6 +929,7 @@ public class PgpKeyOperationTest {
                                                                UncachedKeyRing ring,
                                                                ArrayList<RawPacket> onlyA,
                                                                ArrayList<RawPacket> onlyB,
+                                                               String passphrase,
                                                                boolean canonicalize,
                                                                boolean constantCanonicalize) {
 
@@ -705,8 +939,10 @@ public class PgpKeyOperationTest {
             CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
 
             PgpKeyOperation op = new PgpKeyOperation(null);
-            UncachedKeyRing rawModified = op.modifySecretKeyRing(secretRing, parcel, passphrase).getRing();
-            Assert.assertNotNull("key modification failed", rawModified);
+            EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, passphrase);
+            Assert.assertTrue("key modification must succeed", result.success());
+            UncachedKeyRing rawModified = result.getRing();
+            Assert.assertNotNull("key modification must not return null", rawModified);
 
             if (!canonicalize) {
                 Assert.assertTrue("keyring must differ from original", KeyringTestingHelper.diffKeyrings(
@@ -753,10 +989,75 @@ public class PgpKeyOperationTest {
      */
     @Test
     public void testConcat() throws Exception {
-        byte[] actual = TestDataUtil.concatAll(new byte[]{1}, new byte[]{2,-2}, new byte[]{5},new byte[]{3});
+        byte[] actual = TestDataUtil.concatAll(new byte[]{1}, new byte[]{2, -2}, new byte[]{5}, new byte[]{3});
         byte[] expected = new byte[]{1,2,-2,5,3};
         Assert.assertEquals(java.util.Arrays.toString(expected), java.util.Arrays.toString(actual));
     }
 
+    private void assertFailure(String reason, SaveKeyringParcel parcel, LogType expected) {
+
+        EditKeyResult result = op.createSecretKeyRing(parcel);
+
+        Assert.assertFalse(reason, result.success());
+        Assert.assertNull(reason, result.getRing());
+        Assert.assertTrue(reason + "(with correct error)",
+                result.getLog().containsType(expected));
+
+    }
+
+    private void assertModifyFailure(String reason, UncachedKeyRing ring,
+                                     SaveKeyringParcel parcel, String passphrase, LogType expected)
+            throws Exception {
+
+        CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
+        EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, passphrase);
+
+        Assert.assertFalse(reason, result.success());
+        Assert.assertNull(reason, result.getRing());
+        Assert.assertTrue(reason + "(with correct error)",
+                result.getLog().containsType(expected));
+
+    }
+
+    private void assertModifyFailure(String reason, UncachedKeyRing ring, SaveKeyringParcel parcel,
+                                     LogType expected)
+            throws Exception {
+
+        CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0);
+        EditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, passphrase);
+
+        Assert.assertFalse(reason, result.success());
+        Assert.assertNull(reason, result.getRing());
+        Assert.assertTrue(reason + "(with correct error)",
+                result.getLog().containsType(expected));
+
+    }
+
+    private UncachedKeyRing assertCreateSuccess(String reason, SaveKeyringParcel parcel) {
+
+        EditKeyResult result = op.createSecretKeyRing(parcel);
+
+        Assert.assertTrue(reason, result.success());
+        Assert.assertNotNull(reason, result.getRing());
+
+        return result.getRing();
+
+    }
+
+    private static String genPassphrase() {
+        return genPassphrase(false);
+    }
+
+    private static String genPassphrase(boolean noEmpty) {
+        String chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ123456789!@#$%^&*()-_=";
+        Random r = new Random();
+        StringBuilder passbuilder = new StringBuilder();
+        // 20% chance for an empty passphrase
+        for(int i = 0, j = noEmpty || r.nextInt(10) > 2 ? r.nextInt(20)+1 : 0; i < j; i++) {
+            passbuilder.append(chars.charAt(r.nextInt(chars.length())));
+        }
+        System.out.println("Generated passphrase: '" + passbuilder.toString() + "'");
+        return passbuilder.toString();
+    }
 
 }
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
index 535e9d01a..5792e8326 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
@@ -1,3 +1,21 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 package org.sufficientlysecure.keychain.pgp;
 
 import org.junit.BeforeClass;
@@ -13,6 +31,7 @@ import org.spongycastle.bcpg.PacketTags;
 import org.spongycastle.bcpg.UserIDPacket;
 import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.bcpg.PublicKeyAlgorithmTags;
+import org.spongycastle.jce.provider.BouncyCastleProvider;
 import org.spongycastle.openpgp.PGPPrivateKey;
 import org.spongycastle.openpgp.PGPPublicKey;
 import org.spongycastle.openpgp.PGPSecretKey;
@@ -30,10 +49,12 @@ import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.service.OperationResultParcel;
 import org.sufficientlysecure.keychain.service.OperationResults.EditKeyResult;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
+import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper.RawPacket;
 
 import java.io.ByteArrayInputStream;
+import java.security.Security;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.Iterator;
@@ -60,15 +81,16 @@ public class UncachedKeyringCanonicalizeTest {
 
     @BeforeClass
     public static void setUpOnce() throws Exception {
+        Security.insertProviderAt(new BouncyCastleProvider(), 1);
         ShadowLog.stream = System.out;
 
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.ENCRYPT_COMMS, null));
+                Algorithm.RSA, 1024, null, KeyFlags.ENCRYPT_COMMS, 0L));
 
         parcel.mAddUserIds.add("twi");
         parcel.mAddUserIds.add("pink");
@@ -277,7 +299,7 @@ public class UncachedKeyringCanonicalizeTest {
 
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
         parcel.mAddUserIds.add("trix");
         PgpKeyOperation op = new PgpKeyOperation(null);
 
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java
index 6e9381c06..64152a040 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java
@@ -1,3 +1,21 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 package org.sufficientlysecure.keychain.pgp;
 
 import org.junit.Assert;
@@ -10,14 +28,17 @@ import org.robolectric.shadows.ShadowLog;
 import org.spongycastle.bcpg.PacketTags;
 import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.bcpg.PublicKeyAlgorithmTags;
+import org.spongycastle.jce.provider.BouncyCastleProvider;
 import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.service.OperationResultParcel;
 import org.sufficientlysecure.keychain.service.OperationResults.EditKeyResult;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
+import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper.RawPacket;
 import org.sufficientlysecure.keychain.util.ProgressScaler;
 
+import java.security.Security;
 import java.util.ArrayList;
 import java.util.Iterator;
 
@@ -59,14 +80,15 @@ public class UncachedKeyringMergeTest {
 
     @BeforeClass
     public static void setUpOnce() throws Exception {
+        Security.insertProviderAt(new BouncyCastleProvider(), 1);
         ShadowLog.stream = System.out;
 
         {
             SaveKeyringParcel parcel = new SaveKeyringParcel();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
 
             parcel.mAddUserIds.add("twi");
             parcel.mAddUserIds.add("pink");
@@ -83,7 +105,7 @@ public class UncachedKeyringMergeTest {
         {
             SaveKeyringParcel parcel = new SaveKeyringParcel();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
 
             parcel.mAddUserIds.add("shy");
             // passphrase is tested in PgpKeyOperationTest, just use empty here
@@ -189,7 +211,7 @@ public class UncachedKeyringMergeTest {
 
             parcel.reset();
             parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                    PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                    Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
             modifiedA = op.modifySecretKeyRing(secretRing, parcel, "").getRing();
             modifiedB = op.modifySecretKeyRing(secretRing, parcel, "").getRing();
 
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java
index cbd1bc502..636dae90c 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java
@@ -1,3 +1,21 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 package org.sufficientlysecure.keychain.pgp;
 
 import org.junit.Assert;
@@ -13,6 +31,7 @@ import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException;
 import org.sufficientlysecure.keychain.service.OperationResults.EditKeyResult;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
+import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper.RawPacket;
 
 import java.io.ByteArrayInputStream;
@@ -37,11 +56,11 @@ public class UncachedKeyringTest {
 
         SaveKeyringParcel parcel = new SaveKeyringParcel();
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.CERTIFY_OTHER, null));
+                Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.SIGN_DATA, null));
+                Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L));
         parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd(
-                PublicKeyAlgorithmTags.RSA_GENERAL, 1024, KeyFlags.ENCRYPT_COMMS, null));
+                Algorithm.RSA, 1024, null, KeyFlags.ENCRYPT_COMMS, 0L));
 
         parcel.mAddUserIds.add("twi");
         parcel.mAddUserIds.add("pink");
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/provider/ProviderHelperSaveTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/provider/ProviderHelperSaveTest.java
new file mode 100644
index 000000000..b5d950f59
--- /dev/null
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/provider/ProviderHelperSaveTest.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package org.sufficientlysecure.keychain.provider;
+
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.Robolectric;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.shadows.ShadowLog;
+import org.sufficientlysecure.keychain.pgp.UncachedKeyRing;
+import org.sufficientlysecure.keychain.service.OperationResults.SaveKeyringResult;
+
+import java.io.IOException;
+import java.util.Iterator;
+
+@RunWith(RobolectricTestRunner.class)
+@org.robolectric.annotation.Config(emulateSdk = 18) // Robolectric doesn't yet support 19
+public class ProviderHelperSaveTest {
+
+    @BeforeClass
+    public static void setUpOnce() throws Exception {
+        ShadowLog.stream = System.out;
+    }
+
+    @Test
+    public void testLongKeyIdCollision() throws Exception {
+
+        UncachedKeyRing first =
+                readRingFromResource("/cooperpair/9E669861368BCA0BE42DAF7DDDA252EBB8EBE1AF.asc");
+        UncachedKeyRing second =
+                readRingFromResource("/cooperpair/A55120427374F3F7AA5F1166DDA252EBB8EBE1AF.asc");
+
+        SaveKeyringResult result;
+
+        // insert both keys, second should fail
+        result = new ProviderHelper(Robolectric.application).savePublicKeyRing(first);
+        Assert.assertTrue("first keyring import should succeed", result.success());
+        result = new ProviderHelper(Robolectric.application).savePublicKeyRing(second);
+        Assert.assertFalse("second keyring import should fail", result.success());
+
+        new KeychainDatabase(Robolectric.application).clearDatabase();
+
+        // and the other way around
+        result = new ProviderHelper(Robolectric.application).savePublicKeyRing(second);
+        Assert.assertTrue("first keyring import should succeed", result.success());
+        result = new ProviderHelper(Robolectric.application).savePublicKeyRing(first);
+        Assert.assertFalse("second keyring import should fail", result.success());
+
+    }
+
+    UncachedKeyRing readRingFromResource(String name) throws Exception {
+        return UncachedKeyRing.fromStream(ProviderHelperSaveTest.class.getResourceAsStream(name)).next();
+    }
+
+}
\ No newline at end of file
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/support/KeyringTestingHelper.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/support/KeyringTestingHelper.java
index 015e134ea..c0ba3c9bb 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/support/KeyringTestingHelper.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/support/KeyringTestingHelper.java
@@ -1,5 +1,6 @@
 /*
- * Copyright (C) Art O Cathain, Vincent Breitmoser
+ * Copyright (C) Art O Cathain
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -14,6 +15,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+
 package org.sufficientlysecure.keychain.support;
 
 import android.content.Context;
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/util/FileImportCacheTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/util/FileImportCacheTest.java
index b5708b46f..c5b53e95b 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/util/FileImportCacheTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/util/FileImportCacheTest.java
@@ -1,3 +1,21 @@
+/*
+ * Copyright (C) 2014 Dominik Schürmann <dominik@dominikschuermann.de>
+ * Copyright (C) 2014 Vincent Breitmoser <v.breitmoser@mugenguild.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 package org.sufficientlysecure.keychain.util;
 
 import android.os.Bundle;
@@ -25,7 +43,7 @@ public class FileImportCacheTest {
     @Test
     public void testInputOutput() throws Exception {
 
-        FileImportCache<Bundle> cache = new FileImportCache<Bundle>(Robolectric.application);
+        FileImportCache<Bundle> cache = new FileImportCache<Bundle>(Robolectric.application, "test.pcl");
 
         ArrayList<Bundle> list = new ArrayList<Bundle>();
 
diff --git a/OpenKeychain-Test/src/test/resources/cooperpair/9E669861368BCA0BE42DAF7DDDA252EBB8EBE1AF.asc b/OpenKeychain-Test/src/test/resources/cooperpair/9E669861368BCA0BE42DAF7DDDA252EBB8EBE1AF.asc
new file mode 100644
index 000000000..4f51252da
--- /dev/null
+++ b/OpenKeychain-Test/src/test/resources/cooperpair/9E669861368BCA0BE42DAF7DDDA252EBB8EBE1AF.asc
@@ -0,0 +1,29 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+Version: GnuPG v1
+
+mQINBFJtd/UBEACpw/psXoGNM8RHczviD7FnGdjMQPEJQ+nuWQ2AEGYouulg5hFv
+0ChuSQVLiqQht2k5K2liyW1MeXoJ8tr9nSn/Zi9nttc0Wo6K7pvrDD40r2HNg305
+qLCzItr5st3x8cq2cIXvN4LOm2rqpBLZ/sqMmNiW2Y7/aAQqV1xtR35joHqamWHD
+UPOmzBMs07YSUjXgC1EMx8kWQSV6cuARj93kxWj8R6eoYHHfrWCEGR313wov6QST
+zIfVU7FqQqOmdLW3LaPHxcrI/TjsnkUN99qdlpjJH/YW925LDPJHAkliqPP5AvhU
+F9KbY2F8mcIZBCDd8TH+xXynuN3BbIU4kCwVbdx/tcpO1npuJcKB1Go/udyow/Ei
+Z3nHzJsCVkezvopek77wnwPaP0nAb7f4iIY3gJCoGirOx6N075TgF6MBe00q9oFE
+y4rvnUnU9/QzOOes95eUMhM+9eK1cuLFEV5t47DfxRdq+fQip3FJ2l6v19sZvQ0G
+j06pjYqg0of273rG8oXcDrFjb1Zqhj8x1mLl6u7d/ide5wTm9HylBWcYKQjIJJAi
+WIScxEPIOINDJKgsKTuKtoyNvISJ3xUeS1yzxiIb3YGLIyPgFFx0vFyqJfbkXq70
+m1n2xnJlkTidfzbZvc6EA7vRGSDYK6FqqhlGhc7UypUEVW8FM/jZNAOS6QARAUGt
+tCg5RTY2OTg2MTM2OEJDQTBCRTQyREFGN0REREEyNTJFQkI4RUJFMUFGiQI3BBMB
+CgAhBQJSg/uTAhsDBQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJEN2iUuu46+Gv
++Z0P+wQhkLwm+WGcEsS98Lei9O7hit/k4g/VkLUUQV7BOR3n8uRZIFkdOtpvrFU3
+aKf246uCy6GM48Oh+1U2cv5InX/WEuKaFo5uF6t79wyt18BUn1weDcU+DQdOSG4f
+fSnNa55wkN0l0svW4fGIthjmDTz6HZFntYD+9A20wZAqpPIs+vyG9Jp+e9E9Y/W/
+EFQbNlxHHb9+BMT2+DtNP+HSl3MPFlQPKOLZxyLAU5uzT0Sa0LxhrQy5FgkW6Jog
+sbAJVM9z0pZw+grzGPciM66ZW1rxeICvbYsdWLytRjqxpY8GS8XudyseUGd+dZim
+ptarsrE5yfSMg2gW5Z1PTc0tEMXJLUwtpyzQjpFpbb7dPuo2TUp09LgZKX63WCbS
+Nb1RTaGfkeYudOTo2rh4Jfg+Tb/JRpO6clo0rxAq8nPH2WmG+9TB8Zbb7YRzGWuV
+/e5SeVNR+zY8tXZKnmUIH1HIprc+BtT6Bupdvd0CT14Mg9MmsFvUXofwHLa4gahr
+8/iG9y3uHSA6Rhz++yOpyOmNvO1LDxsYNaRCIXQJbqgNwF5YNYlMPsEeY/CG7FOb
+Afv7rHiYtRRQfz2P4OF900DJO7QL9gdNXJ1+Hajy/5Lvvl7qwqMG4GvVQEsgFc5O
+jjFCUhE2i20j2kEMxvA5RLBH/fOoGARn87tiKSfb+pqLNZQb
+=fDJ8
+-----END PGP PUBLIC KEY BLOCK-----
\ No newline at end of file
diff --git a/OpenKeychain-Test/src/test/resources/cooperpair/A55120427374F3F7AA5F1166DDA252EBB8EBE1AF.asc b/OpenKeychain-Test/src/test/resources/cooperpair/A55120427374F3F7AA5F1166DDA252EBB8EBE1AF.asc
new file mode 100644
index 000000000..549bc51a2
--- /dev/null
+++ b/OpenKeychain-Test/src/test/resources/cooperpair/A55120427374F3F7AA5F1166DDA252EBB8EBE1AF.asc
@@ -0,0 +1,29 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+Version: GnuPG v1
+
+mQINBFKD+38BEADSv5l4xOx9hCRJVcybq6yK5hTpGSFf3xo1bkhoMvyC62ehb4jD
+MDLwwNRyzCBEWQJLbq/LLizPFN2qXFJpXJcsuqsHNYRtDqDBEjtriRQwSqHnqTXt
+c0K46FYHldCJQ4/tBXxPI+WwtXjcNRWaV7n2BvR/Jk+B5e4Zz3LPnN0C4w5vORHs
+hN1jil8A3Hs/F+OmlQYrU8ZtNwTpSo2EXxe2fVgSDCsKRyNsPZj++OyujPzW+yaN
+lJ9I/q6s9gvX9o9o7nwZbqBETipWsdRK6RfBdTKpnyLNordbWwWTk6GxN8T5Ppit
+P6a3UlQ71VuflcswCTmEQ1pEfZrlRFKa9psBOW+cZLNxT9h0jGFMh6/B3w48Sag+
+cFcPBFWParC+cAXBIURDxT9G6bzNLogg7YKoaPsyiXnLDH2VJUCXs27D2wPJL24Q
+S7npvsg63MPPssWgG5cauLznmNR4y5pQi6oH/C10v0zrUJy6FPJzQhYRhWOvhtz6
+j88RGMrFNNCdB2VACtn699D+ixu3nRlXHIKCT+xLSfgslVYifmJOCNljBLGHOQ1e
+FJxQuNVpmmxjvk/8kqK+pHLB9Qn6M1ZYzip7OyUL3OAWabCabgEw2bQmUhiBWD3u
+buv0WAVOJEAFvBCAeYNQzrQMY+Rc3RnvynG4pI6Tbo8wC6/IJcDOw516JwARASB3
+tChBNTUxMjA0MjczNzRGM0Y3QUE1RjExNjZEREEyNTJFQkI4RUJFMUFGiQI3BBMB
+CgAhBQJSg/uTAhsDBQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJEN2iUuu46+Gv
+9L0P/3tFu0LOZ/dAPjUNfKJCZqcIuVnD5xShMTsUbVx+QoXMy7rt4iRLD7ofGi/I
+vTAZehxk3sk/Slx5nbews+3NItyw6mcaP9HlmwKNr6k7BC2kJHcCxH4DNzhmIx1H
+3T/CggtHX42JBYKlGf22y+M8jAbvsPOUfTznx96mYNrOY6s1dJyn0kRleqJ8+tGj
+/5+0y90iZnGCa0FtacQkKUPkXwVodeZVxk8z5OEipShYKc+8dl+5WsvOzHqLC/KY
+xCGRb4JaqEMwouLNg8dTNAXXUvFGqJNDX4+andggogmI1hdD9xExfSU9cAGegg2t
+vvveC4S+CCHd+zt88iK5ze6F61RxwYhhNbkuFGjdgNGCpHtG/BQhKnYJuKEbq3oi
+mgNyxJERlfgaWXveiMG0AmACXN+jCkTtqZjQnsg2N2QDL3tjY7usmuiwRL1aVOFG
+Kw5/Cc+2nDeANS3Xi1403Ni269b1c6kNSoLe4zd0WsbO3Kouds8F8EQfeheXQe97
+ZxuvBOMsR9wHC3f0sl/vfxCGdUC+khmKk5taKnUeUFJmVmh5ghlVy8FySHGB0QHO
+zd8GUl59rFpQJNpNFQW2YKDhrcjxIr2AeJrdoDI6NsQ02+Qtep/bbq53hqtAD4jF
+t3S8vBbTXtRk6g2qn4ojF4SOIc8SAiZcURgVFuSJX8ngFbO4
+=OEw/
+-----END PGP PUBLIC KEY BLOCK-----
\ No newline at end of file
diff --git a/OpenKeychain-Test/src/test/resources/cooperpair/readme b/OpenKeychain-Test/src/test/resources/cooperpair/readme
new file mode 100644
index 000000000..fecb372d9
--- /dev/null
+++ b/OpenKeychain-Test/src/test/resources/cooperpair/readme
@@ -0,0 +1,3 @@
+"Cooperpair" testcase under public domain license, by @coruus:
+
+https://github.com/coruus/cooperpair/tree/master/pgpv4