From 3e51da3afa542c62b82bbcf9a953cdcd379950a2 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 20 Mar 2015 18:45:00 +0100 Subject: fix unit tests (for real) --- .../keychain/pgp/PgpKeyOperationTest.java | 21 ++++++++++++++------- .../keychain/pgp/UncachedKeyringMergeTest.java | 12 ++++++------ .../operations/results/OperationResult.java | 4 +++- .../keychain/pgp/PgpKeyOperation.java | 12 ++++++++++-- .../keychain/service/SaveKeyringParcel.java | 2 +- .../keychain/service/input/CryptoInputParcel.java | 2 +- OpenKeychain/src/main/res/values/strings.xml | 6 ++++-- 7 files changed, 39 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 2ecc304e7..e6ada202b 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 @@ -75,7 +75,6 @@ public class PgpKeyOperationTest { static UncachedKeyRing staticRing; final static Passphrase passphrase = TestingUtils.genPassphrase(); - final static CryptoInputParcel cryptoInput = new CryptoInputParcel(new Date(), passphrase); UncachedKeyRing ring; PgpKeyOperation op; @@ -83,6 +82,8 @@ public class PgpKeyOperationTest { ArrayList onlyA = new ArrayList(); ArrayList onlyB = new ArrayList(); + static CryptoInputParcel cryptoInput; + @BeforeClass public static void setUpOnce() throws Exception { Security.insertProviderAt(new BouncyCastleProvider(), 1); @@ -110,6 +111,9 @@ public class PgpKeyOperationTest { // we sleep here for a second, to make sure all new certificates have different timestamps Thread.sleep(1000); + + cryptoInput = new CryptoInputParcel(new Date(), passphrase); + } @Before public void setUp() throws Exception { @@ -302,6 +306,7 @@ public class PgpKeyOperationTest { if (badphrase.equals(passphrase)) { badphrase = new Passphrase("a"); } + parcel.mAddUserIds.add("allure"); assertModifyFailure("keyring modification with bad passphrase should fail", ring, parcel, new CryptoInputParcel(badphrase), LogType.MSG_MF_UNLOCK_ERROR); @@ -659,7 +664,8 @@ public class PgpKeyOperationTest { parcel.reset(); parcel.mRevokeSubKeys.add(keyId); - modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB); + modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, + new CryptoInputParcel(new Date(), passphrase)); Assert.assertEquals("no extra packets in original", 0, onlyA.size()); Assert.assertEquals("exactly one extra packet in modified", 1, onlyB.size()); @@ -779,7 +785,7 @@ public class PgpKeyOperationTest { { // we should be able to change the stripped/divert status of subkeys without passphrase parcel.reset(); parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null)); - modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, null); + modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, new CryptoInputParcel()); Assert.assertEquals("one extra packet in modified", 1, onlyB.size()); Packet p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); Assert.assertEquals("new packet should have GNU_DUMMY S2K type", @@ -795,7 +801,7 @@ public class PgpKeyOperationTest { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }; parcel.mChangeSubKeys.add(new SubkeyChange(keyId, false, serial)); - modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, null); + modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, new CryptoInputParcel()); Assert.assertEquals("one extra packet in modified", 1, onlyB.size()); Packet p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); Assert.assertEquals("new packet should have GNU_DUMMY S2K type", @@ -972,12 +978,12 @@ public class PgpKeyOperationTest { 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, cryptoInput, true, false); + modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, + new CryptoInputParcel(new Date(), 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()); @@ -1055,7 +1061,8 @@ public class PgpKeyOperationTest { Passphrase otherPassphrase = TestingUtils.genPassphrase(true); CryptoInputParcel otherCryptoInput = new CryptoInputParcel(otherPassphrase); parcel.mNewUnlock = new ChangeUnlockParcel(otherPassphrase); - modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, new CryptoInputParcel(new Date())); + modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, + new CryptoInputParcel(new Date(), new Passphrase())); Assert.assertEquals("exactly three packets should have been modified (the secret keys)", 3, onlyB.size()); 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 732c13f62..755a0b00d 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 @@ -190,11 +190,11 @@ public class UncachedKeyringMergeTest { parcel.reset(); parcel.mAddUserIds.add("flim"); - modifiedA = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); + modifiedA = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); parcel.reset(); parcel.mAddUserIds.add("flam"); - modifiedB = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); + modifiedB = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); } { // merge A into base @@ -231,8 +231,8 @@ public class UncachedKeyringMergeTest { parcel.reset(); parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd( Algorithm.RSA, 1024, null, KeyFlags.SIGN_DATA, 0L)); - modifiedA = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); - modifiedB = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); + modifiedA = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); + modifiedB = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); subKeyIdA = KeyringTestingHelper.getSubkeyId(modifiedA, 2); subKeyIdB = KeyringTestingHelper.getSubkeyId(modifiedB, 2); @@ -273,7 +273,7 @@ public class UncachedKeyringMergeTest { parcel.mRevokeSubKeys.add(KeyringTestingHelper.getSubkeyId(ringA, 1)); CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing( ringA.getEncoded(), false, 0); - modified = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); + modified = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); } { @@ -372,7 +372,7 @@ public class UncachedKeyringMergeTest { CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing( ringA.getEncoded(), false, 0); - modified = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(), parcel).getRing(); + modified = op.modifySecretKeyRing(secretRing, new CryptoInputParcel(new Passphrase()), parcel).getRing(); } { 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 25abab25b..4646aef8a 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 @@ -530,7 +530,6 @@ public abstract class OperationResult implements Parcelable { MSG_MF_ERROR_REVOKED_PRIMARY (LogLevel.ERROR, R.string.msg_mf_error_revoked_primary), MSG_MF_ERROR_SIG (LogLevel.ERROR, R.string.msg_mf_error_sig), MSG_MF_ERROR_SUBKEY_MISSING(LogLevel.ERROR, R.string.msg_mf_error_subkey_missing), - MSG_MF_INPUT_REQUIRED (LogLevel.OK, R.string.msg_mf_input_required), MSG_MF_MASTER (LogLevel.DEBUG, R.string.msg_mf_master), MSG_MF_NOTATION_PIN (LogLevel.DEBUG, R.string.msg_mf_notation_pin), MSG_MF_NOTATION_EMPTY (LogLevel.DEBUG, R.string.msg_mf_notation_empty), @@ -540,6 +539,9 @@ public abstract class OperationResult implements Parcelable { MSG_MF_PASSPHRASE_FAIL (LogLevel.WARN, R.string.msg_mf_passphrase_fail), MSG_MF_PRIMARY_REPLACE_OLD (LogLevel.DEBUG, R.string.msg_mf_primary_replace_old), MSG_MF_PRIMARY_NEW (LogLevel.DEBUG, R.string.msg_mf_primary_new), + MSG_MF_RESTRICTED_MODE (LogLevel.INFO, R.string.msg_mf_restricted_mode), + MSG_MF_REQUIRE_DIVERT (LogLevel.OK, R.string.msg_mf_require_divert), + MSG_MF_REQUIRE_PASSPHRASE (LogLevel.OK, R.string.msg_mf_require_passphrase), MSG_MF_SUBKEY_CHANGE (LogLevel.INFO, R.string.msg_mf_subkey_change), MSG_MF_SUBKEY_NEW_ID (LogLevel.DEBUG, R.string.msg_mf_subkey_new_id), MSG_MF_SUBKEY_NEW (LogLevel.INFO, R.string.msg_mf_subkey_new), 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 f739cfb69..454a35cd6 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -395,12 +395,14 @@ public class PgpKeyOperation { return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } - if (saveParcel.isRestrictedOnly()) { + if (isDummy(masterSecretKey) || saveParcel.isRestrictedOnly()) { + log.add(LogType.MSG_MF_RESTRICTED_MODE, indent); return internalRestricted(sKR, saveParcel, log); } // Do we require a passphrase? If so, pass it along if (!isDivertToCard(masterSecretKey) && !cryptoInput.hasPassphrase()) { + log.add(LogType.MSG_MF_REQUIRE_PASSPHRASE, indent); return new PgpEditKeyResult(log, RequiredInputParcel.createRequiredPassphrase( masterSecretKey.getKeyID(), cryptoInput.getSignatureTime())); } @@ -971,7 +973,7 @@ public class PgpKeyOperation { progress(R.string.progress_done, 100); if (!nfcSignOps.isEmpty()) { - log.add(LogType.MSG_MF_INPUT_REQUIRED, indent); + log.add(LogType.MSG_MF_REQUIRE_DIVERT, indent); return new PgpEditKeyResult(log, nfcSignOps.build()); } @@ -1459,6 +1461,12 @@ public class PgpKeyOperation { return flags; } + private static boolean isDummy(PGPSecretKey secretKey) { + S2K s2k = secretKey.getS2K(); + return s2k.getType() == S2K.GNU_DUMMY_S2K + && s2k.getProtectionMode() == S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY; + } + private static boolean isDivertToCard(PGPSecretKey secretKey) { S2K s2k = secretKey.getS2K(); return s2k.getType() == S2K.GNU_DUMMY_S2K diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java index 9fd278c13..88f258c58 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java @@ -85,7 +85,7 @@ public class SaveKeyringParcel implements Parcelable { /** Returns true iff this parcel does not contain any operations which require a passphrase. */ public boolean isRestrictedOnly() { if (mNewUnlock != null || !mAddUserIds.isEmpty() || !mAddUserAttribute.isEmpty() - || !mAddSubKeys.isEmpty() || mChangePrimaryUserId != null || !mRevokeSubKeys .isEmpty() + || !mAddSubKeys.isEmpty() || mChangePrimaryUserId != null || !mRevokeUserIds.isEmpty() || !mRevokeSubKeys.isEmpty()) { return false; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/input/CryptoInputParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/input/CryptoInputParcel.java index 6edc48b09..21aacd1f0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/input/CryptoInputParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/input/CryptoInputParcel.java @@ -36,7 +36,7 @@ public class CryptoInputParcel implements Parcelable { public CryptoInputParcel(Passphrase passphrase) { mSignatureTime = new Date(); - mPassphrase = null; + mPassphrase = passphrase; } public CryptoInputParcel(Date signatureTime) { diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index a7e9cdf7e..76514575c 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -913,7 +913,7 @@ "Fatal error decrypting master key! This is likely a programming error, please file a bug report!" "Internal OpenPGP error!" "Signature exception!" - "Diverting to card/nfc for crypto operations" + "Tried to operate on missing subkey %s!" "Modifying master certifications" "Adding empty notation packet" "Adding PIN notation packet" @@ -923,8 +923,10 @@ "Passphrase for subkey could not be changed! (Does it have a different one from the other keys?)" "Replacing certificate of previous primary user ID" "Generating new certificate for new primary user ID" + "Changing to restricted operation mode" "Modifying subkey %s" - "Tried to operate on missing subkey %s!" + "Diverting to card/nfc for crypto operations" + "Passphrase required for operations" "Adding new subkey of type %s" "New subkey ID: %s" "Expiry date cannot be in the past!" -- cgit v1.2.3