Merge branch 'development' of github.com:open-keychain/open-keychain into development

8 files changed, 169 insertions, 20 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 52115a76d..3ea88aac6 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
@@ -34,6 +34,8 @@ import org.spongycastle.bcpg.S2K;
 import org.spongycastle.bcpg.SecretKeyPacket;
 import org.spongycastle.bcpg.SecretSubkeyPacket;
 import org.spongycastle.bcpg.SignaturePacket;
+import org.spongycastle.bcpg.UserAttributePacket;
+import org.spongycastle.bcpg.UserAttributeSubpacket;
 import org.spongycastle.bcpg.UserIDPacket;
 import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.jce.provider.BouncyCastleProvider;
@@ -865,6 +867,70 @@ public class PgpKeyOperationTest {
     }
 
     @Test
+    public void testUserAttributeAdd() throws Exception {
+
+        {
+            parcel.mAddUserAttribute.add(WrappedUserAttribute.fromData(new byte[0]));
+            assertModifyFailure("adding an empty user attribute should fail", ring, parcel,
+                    LogType.MSG_MF_UAT_ERROR_EMPTY);
+        }
+
+        parcel.reset();
+
+        Random r = new Random();
+        int type = r.nextInt(110)+1;
+        byte[] data = new byte[r.nextInt(2000)];
+        new Random().nextBytes(data);
+
+        WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data);
+        parcel.mAddUserAttribute.add(uat);
+
+        UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB);
+
+        Assert.assertEquals("no extra packets in original", 0, onlyA.size());
+        Assert.assertEquals("exactly two extra packets in modified", 2, onlyB.size());
+
+        Assert.assertTrue("keyring must contain added user attribute",
+                modified.getPublicKey().getUnorderedUserAttributes().contains(uat));
+
+        Packet p;
+
+        p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket();
+        Assert.assertTrue("first new packet must be user attribute", p instanceof UserAttributePacket);
+        {
+            UserAttributeSubpacket[] subpackets = ((UserAttributePacket) p).getSubpackets();
+            Assert.assertEquals("user attribute packet must contain one subpacket",
+                    1, subpackets.length);
+            Assert.assertEquals("user attribute subpacket type must be as specified above",
+                    type, subpackets[0].getType());
+            Assert.assertArrayEquals("user attribute subpacket data must be as specified above",
+                    data, subpackets[0].getData());
+        }
+
+        p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(1).buf)).readPacket();
+        Assert.assertTrue("second new packet must be signature", p instanceof SignaturePacket);
+        Assert.assertEquals("signature type must be positive certification",
+                PGPSignature.POSITIVE_CERTIFICATION, ((SignaturePacket) p).getSignatureType());
+
+        // make sure packets can be distinguished by timestamp
+        Thread.sleep(1000);
+
+        // applying the same modification AGAIN should not add more certifications but drop those
+        // as duplicates
+        modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, passphrase, true, false);
+
+        Assert.assertEquals("duplicate modification: one extra packet in original", 1, onlyA.size());
+        Assert.assertEquals("duplicate modification: one extra packet in modified", 1, onlyB.size());
+
+        p = new BCPGInputStream(new ByteArrayInputStream(onlyA.get(0).buf)).readPacket();
+        Assert.assertTrue("lost packet must be signature", p instanceof SignaturePacket);
+        p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket();
+        Assert.assertTrue("new packet must be signature", p instanceof SignaturePacket);
+
+    }
+
+
+    @Test
     public void testUserIdPrimary() throws Exception {
 
         UncachedKeyRing modified = ring;
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 721d1a51d..f9e0d52c3 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
@@ -104,6 +104,12 @@ public class UncachedKeyringCanonicalizeTest {
 
         parcel.mAddUserIds.add("twi");
         parcel.mAddUserIds.add("pink");
+        {
+            WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(100,
+                    "sunshine, sunshine, ladybugs awake~".getBytes());
+            parcel.mAddUserAttribute.add(uat);
+        }
+
         // passphrase is tested in PgpKeyOperationTest, just use empty here
         parcel.mNewUnlock = new ChangeUnlockParcel("");
         PgpKeyOperation op = new PgpKeyOperation(null);
@@ -116,7 +122,7 @@ public class UncachedKeyringCanonicalizeTest {
         staticRing = staticRing.canonicalize(new OperationLog(), 0).getUncachedKeyRing();
 
         // just for later reference
-        totalPackets = 9;
+        totalPackets = 11;
 
         // we sleep here for a second, to make sure all new certificates have different timestamps
         Thread.sleep(1000);
@@ -150,8 +156,8 @@ public class UncachedKeyringCanonicalizeTest {
         Assert.assertEquals("packet #4 should be signature",
                 PacketTags.SIGNATURE, it.next().tag);
 
-        Assert.assertEquals("packet #5 should be secret subkey",
-                PacketTags.SECRET_SUBKEY, it.next().tag);
+        Assert.assertEquals("packet #5 should be user id",
+                PacketTags.USER_ATTRIBUTE, it.next().tag);
         Assert.assertEquals("packet #6 should be signature",
                 PacketTags.SIGNATURE, it.next().tag);
 
@@ -160,7 +166,12 @@ public class UncachedKeyringCanonicalizeTest {
         Assert.assertEquals("packet #8 should be signature",
                 PacketTags.SIGNATURE, it.next().tag);
 
-        Assert.assertFalse("exactly 9 packets total", it.hasNext());
+        Assert.assertEquals("packet #9 should be secret subkey",
+                PacketTags.SECRET_SUBKEY, it.next().tag);
+        Assert.assertEquals("packet #10 should be signature",
+                PacketTags.SIGNATURE, it.next().tag);
+
+        Assert.assertFalse("exactly 11 packets total", it.hasNext());
 
         Assert.assertArrayEquals("created keyring should be constant through canonicalization",
                 ring.getEncoded(), ring.canonicalize(log, 0).getEncoded());
@@ -380,7 +391,7 @@ public class UncachedKeyringCanonicalizeTest {
     @Test public void testSubkeyDestroy() throws Exception {
 
         // signature for second key (first subkey)
-        UncachedKeyRing modified = KeyringTestingHelper.removePacket(ring, 6);
+        UncachedKeyRing modified = KeyringTestingHelper.removePacket(ring, 8);
 
         // canonicalization should fail, because there are no valid uids left
         CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0);
@@ -424,7 +435,7 @@ public class UncachedKeyringCanonicalizeTest {
                     secretKey.getPublicKey(), pKey.getPublicKey());
 
             // inject in the right position
-            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 6);
+            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 8);
 
             // canonicalize, and check if we lose the bad signature
             CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0);
@@ -449,7 +460,7 @@ public class UncachedKeyringCanonicalizeTest {
                     secretKey.getPublicKey(), pKey.getPublicKey());
 
             // inject in the right position
-            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 6);
+            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 8);
 
             // canonicalize, and check if we lose the bad signature
             CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0);
@@ -481,10 +492,10 @@ public class UncachedKeyringCanonicalizeTest {
                 secretKey, PGPSignature.SUBKEY_BINDING, subHashedPacketsGen,
                 secretKey.getPublicKey(), pKey.getPublicKey());
 
-        UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig1.getEncoded(), 8);
-        modified = KeyringTestingHelper.injectPacket(modified, sig2.getEncoded(), 9);
-        modified = KeyringTestingHelper.injectPacket(modified, sig1.getEncoded(), 10);
-        modified = KeyringTestingHelper.injectPacket(modified, sig3.getEncoded(), 11);
+        UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig1.getEncoded(), 10);
+        modified = KeyringTestingHelper.injectPacket(modified, sig2.getEncoded(), 11);
+        modified = KeyringTestingHelper.injectPacket(modified, sig1.getEncoded(), 12);
+        modified = KeyringTestingHelper.injectPacket(modified, sig3.getEncoded(), 13);
 
         // canonicalize, and check if we lose the bad signature
         CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0);
@@ -512,13 +523,13 @@ public class UncachedKeyringCanonicalizeTest {
 
             // get subkey packets
             Iterator<RawPacket> it = KeyringTestingHelper.parseKeyring(ring.getEncoded());
-            RawPacket subKey = KeyringTestingHelper.getNth(it, 5);
+            RawPacket subKey = KeyringTestingHelper.getNth(it, 7);
             RawPacket subSig = it.next();
 
             // inject at a second position
             UncachedKeyRing modified = ring;
-            modified = KeyringTestingHelper.injectPacket(modified, subKey.buf, 7);
-            modified = KeyringTestingHelper.injectPacket(modified, subSig.buf, 8);
+            modified = KeyringTestingHelper.injectPacket(modified, subKey.buf, 9);
+            modified = KeyringTestingHelper.injectPacket(modified, subSig.buf, 10);
 
             // canonicalize, and check if we lose the bad signature
             OperationLog log = new OperationLog();
@@ -557,7 +568,7 @@ public class UncachedKeyringCanonicalizeTest {
                 sKey = new PGPSecretKey(masterSecretKey.getPrivateKey(), subPubKey, sha1Calc, false, keyEncryptor);
             }
 
-            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sKey.getEncoded(), 5);
+            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sKey.getEncoded(), 7);
 
             // canonicalize, and check if we lose the bad signature
             OperationLog log = new OperationLog();
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 7f6f480d4..ccd47d0ee 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
@@ -46,6 +46,7 @@ import java.io.ByteArrayInputStream;
 import java.security.Security;
 import java.util.ArrayList;
 import java.util.Iterator;
+import java.util.Random;
 
 /** Tests for the UncachedKeyring.merge method.
  *
@@ -97,6 +98,12 @@ public class UncachedKeyringMergeTest {
 
             parcel.mAddUserIds.add("twi");
             parcel.mAddUserIds.add("pink");
+            {
+                WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(100,
+                        "sunshine, sunshine, ladybugs awake~".getBytes());
+                parcel.mAddUserAttribute.add(uat);
+            }
+
             // passphrase is tested in PgpKeyOperationTest, just use empty here
             parcel.mNewUnlock = new ChangeUnlockParcel("");
             PgpKeyOperation op = new PgpKeyOperation(null);
@@ -339,6 +346,36 @@ public class UncachedKeyringMergeTest {
         }
     }
 
+    @Test
+    public void testAddedUserAttributeSignature() throws Exception {
+
+        final UncachedKeyRing modified; {
+            parcel.reset();
+
+            Random r = new Random();
+            int type = r.nextInt(110)+1;
+            byte[] data = new byte[r.nextInt(2000)];
+            new Random().nextBytes(data);
+
+            WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data);
+            parcel.mAddUserAttribute.add(uat);
+
+            CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(
+                    ringA.getEncoded(), false, 0);
+            modified = op.modifySecretKeyRing(secretRing, parcel, "").getRing();
+        }
+
+        {
+            UncachedKeyRing merged = ringA.merge(modified, log, 0);
+            Assert.assertNotNull("merge must succeed", merged);
+            Assert.assertFalse(
+                    "merging keyring with extra user attribute into its base should yield that same keyring",
+                    KeyringTestingHelper.diffKeyrings(merged.getEncoded(), modified.getEncoded(), onlyA, onlyB)
+            );
+        }
+
+    }
+
     private UncachedKeyRing mergeWithChecks(UncachedKeyRing a, UncachedKeyRing b)
             throws Exception {
         return mergeWithChecks(a, b, a);
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 a3c58a5c8..65395f1ab 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
@@ -37,6 +37,7 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.Iterator;
+import java.util.Random;
 
 @RunWith(RobolectricTestRunner.class)
 @org.robolectric.annotation.Config(emulateSdk = 18) // Robolectric doesn't yet support 19
@@ -59,6 +60,15 @@ public class UncachedKeyringTest {
 
         parcel.mAddUserIds.add("twi");
         parcel.mAddUserIds.add("pink");
+        {
+            Random r = new Random();
+            int type = r.nextInt(110)+1;
+            byte[] data = new byte[r.nextInt(2000)];
+            new Random().nextBytes(data);
+
+            WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data);
+            parcel.mAddUserAttribute.add(uat);
+        }
         // passphrase is tested in PgpKeyOperationTest, just use empty here
         parcel.mNewUnlock = new ChangeUnlockParcel("");
         PgpKeyOperation op = new PgpKeyOperation(null);
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
index 556d70cb2..8c76ebb8a 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
@@ -507,6 +507,7 @@ public abstract class OperationResult implements Parcelable {
         MSG_MF_UID_PRIMARY (LogLevel.INFO, R.string.msg_mf_uid_primary),
         MSG_MF_UID_REVOKE (LogLevel.INFO, R.string.msg_mf_uid_revoke),
         MSG_MF_UID_ERROR_EMPTY (LogLevel.ERROR, R.string.msg_mf_uid_error_empty),
+        MSG_MF_UAT_ERROR_EMPTY (LogLevel.ERROR, R.string.msg_mf_uat_error_empty),
         MSG_MF_UAT_ADD_IMAGE (LogLevel.INFO, R.string.msg_mf_uat_add_image),
         MSG_MF_UAT_ADD_UNKNOWN (LogLevel.INFO, R.string.msg_mf_uat_add_unknown),
         MSG_MF_UNLOCK_ERROR (LogLevel.ERROR, R.string.msg_mf_unlock_error),
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 18a5410bf..4bab7f2b9 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
@@ -531,12 +531,16 @@ public class PgpKeyOperation {
                     WrappedUserAttribute attribute = saveParcel.mAddUserAttribute.get(i);
 
                     switch (attribute.getType()) {
+                        // the 'none' type must not succeed
                         case WrappedUserAttribute.UAT_NONE:
-                            log.add(LogType.MSG_MF_UAT_ADD_UNKNOWN, indent);
-                            break;
+                            log.add(LogType.MSG_MF_UAT_ERROR_EMPTY, indent);
+                            return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null);
                         case WrappedUserAttribute.UAT_IMAGE:
                             log.add(LogType.MSG_MF_UAT_ADD_IMAGE, indent);
                             break;
+                        default:
+                            log.add(LogType.MSG_MF_UAT_ADD_UNKNOWN, indent);
+                            break;
                     }
 
                     PGPUserAttributeSubpacketVector vector = attribute.getVector();
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java
index 248ef11aa..da6d8b287 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java
@@ -22,6 +22,7 @@ import org.spongycastle.bcpg.BCPGOutputStream;
 import org.spongycastle.bcpg.Packet;
 import org.spongycastle.bcpg.UserAttributePacket;
 import org.spongycastle.bcpg.UserAttributeSubpacket;
+import org.spongycastle.bcpg.UserAttributeSubpacketInputStream;
 import org.spongycastle.bcpg.UserAttributeSubpacketTags;
 import org.spongycastle.openpgp.PGPUserAttributeSubpacketVector;
 
@@ -30,6 +31,8 @@ import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.ObjectStreamException;
 import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
 
 public class WrappedUserAttribute implements Serializable {
 
@@ -72,9 +75,17 @@ public class WrappedUserAttribute implements Serializable {
         return out.toByteArray();
     }
 
-    public static WrappedUserAttribute fromData (byte[] data) {
-        // TODO
-        return null;
+    public static WrappedUserAttribute fromData (byte[] data) throws IOException {
+        UserAttributeSubpacketInputStream in =
+                new UserAttributeSubpacketInputStream(new ByteArrayInputStream(data));
+        ArrayList<UserAttributeSubpacket> list = new ArrayList<UserAttributeSubpacket>();
+        while (in.available() > 0) {
+            list.add(in.readPacket());
+        }
+        UserAttributeSubpacket[] result = new UserAttributeSubpacket[list.size()];
+        list.toArray(result);
+        return new WrappedUserAttribute(
+                new PGPUserAttributeSubpacketVector(result));
     }
 
     /** Writes this object to an ObjectOutputStream. */
@@ -102,4 +113,12 @@ public class WrappedUserAttribute implements Serializable {
     private void readObjectNoData() throws ObjectStreamException {
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (!WrappedUserAttribute.class.isInstance(o)) {
+            return false;
+        }
+        return mVector.equals(((WrappedUserAttribute) o).mVector);
+    }
+
 }
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index deb2f05f3..5163f16e6 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -858,6 +858,7 @@
     <string name="msg_mf_uid_primary">"Changing primary user ID to %s"</string>
     <string name="msg_mf_uid_revoke">"Revoking user ID %s"</string>
     <string name="msg_mf_uid_error_empty">"User ID must not be empty!"</string>
+    <string name="msg_mf_uat_error_empty">"User attribute must not be empty!"</string>
     <string name="msg_mf_uat_add_image">"Adding user attribute of type image"</string>
     <string name="msg_mf_uat_add_unknown">"Adding user attribute of unknown type"</string>
     <string name="msg_mf_unlock_error">"Error unlocking keyring!"</string>