Adding keytocard flag to SubkeyChange: UI sets this flag to initiate keytocard; operation unsets it and fills in dummyDivert to finish it.

4 files changed, 45 insertions, 32 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
index 93c479d5e..b6e978f48 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
@@ -412,7 +412,7 @@ public class PgpKeyOperation {
         }
 
         for(SaveKeyringParcel.SubkeyChange change : saveParcel.mChangeSubKeys) {
-            if (change.mDummyDivert != null && change.mDummyDivert.length == 0) {
+            if (change.mMoveKeyToCard) {
                 // If this is a keytocard operation, see if it was completed: look for a hash
                 // matching the given subkey ID in cryptoData.
                 byte[] subKeyId = new byte[8];
@@ -421,6 +421,7 @@ public class PgpKeyOperation {
 
                 byte[] serialNumber = cryptoInput.getCryptoData().get(buf);
                 if (serialNumber != null) {
+                    change.mMoveKeyToCard = false;
                     change.mDummyDivert = serialNumber;
                 }
             }
@@ -776,28 +777,27 @@ public class PgpKeyOperation {
                     // no really, it is. this operation irrevocably removes the private key data from the key
                     sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey());
                     sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
-                } else if (change.mDummyDivert != null) {
-                    if (change.mDummyDivert.length == 0) {
-                        // If serial number is 0 length, we're moving the key to a card.
-                        if (checkSmartCardCompatibility(sKey, log, indent + 1)) {
-                            log.add(LogType.MSG_MF_KEYTOCARD_START, indent + 1,
-                                    KeyFormattingUtils.convertKeyIdToHex(change.mKeyId));
-                            nfcKeyToCardOps.addSubkey(change.mKeyId);
-                        } else {
-                            return new PgpEditKeyResult(EditKeyResult.RESULT_ERROR, log, null);
-                        }
-                    } else if (change.mDummyDivert.length == 16) {
-                        // If serial number is 16 bytes long, we're associating the key with a card.
-                        log.add(LogType.MSG_MF_KEYTOCARD_FINISH, indent + 1,
-                                KeyFormattingUtils.convertKeyIdToHex(change.mKeyId),
-                                Hex.toHexString(change.mDummyDivert, 8, 6));
-                        sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), change.mDummyDivert);
-                        sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
+                } else if (change.mMoveKeyToCard) {
+                    if (checkSmartCardCompatibility(sKey, log, indent + 1)) {
+                        log.add(LogType.MSG_MF_KEYTOCARD_START, indent + 1,
+                                KeyFormattingUtils.convertKeyIdToHex(change.mKeyId));
+                        nfcKeyToCardOps.addSubkey(change.mKeyId);
                     } else {
+                        // Appropriate log message already set by checkSmartCardCompatibility
+                        return new PgpEditKeyResult(EditKeyResult.RESULT_ERROR, log, null);
+                    }
+                } else if (change.mDummyDivert != null) {
+                    // NOTE: Does this code get executed? Or always handled in internalRestricted?
+                    if (change.mDummyDivert.length != 16) {
                         log.add(LogType.MSG_MF_ERROR_DIVERT_SERIAL,
                                 indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId));
                         return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null);
                     }
+                    log.add(LogType.MSG_MF_KEYTOCARD_FINISH, indent + 1,
+                            KeyFormattingUtils.convertKeyIdToHex(change.mKeyId),
+                            Hex.toHexString(change.mDummyDivert, 8, 6));
+                    sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), change.mDummyDivert);
+                    sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey);
                 }
 
 
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 6064c28fb..dd6697f21 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java
@@ -95,8 +95,8 @@ public class SaveKeyringParcel implements Parcelable {
         }
 
         for (SubkeyChange change : mChangeSubKeys) {
-            if (change.mRecertify || change.mFlags != null || change.mExpiry != null ||
-                    (change.mDummyDivert != null && change.mDummyDivert.length == 0)) {
+            if (change.mRecertify || change.mFlags != null || change.mExpiry != null
+                    || change.mMoveKeyToCard) {
                 return false;
             }
         }
@@ -143,6 +143,8 @@ public class SaveKeyringParcel implements Parcelable {
         public boolean mRecertify;
         // if this flag is true, the subkey should be changed to a stripped key
         public boolean mDummyStrip;
+        // if this flag is true, the subkey should be moved to a card
+        public boolean mMoveKeyToCard;
         // if this is non-null, the subkey will be changed to a divert-to-card
         // key for the given serial number
         public byte[] mDummyDivert;
@@ -174,12 +176,25 @@ public class SaveKeyringParcel implements Parcelable {
             mDummyDivert = dummyDivert;
         }
 
+        public SubkeyChange(long keyId, boolean dummyStrip, boolean moveKeyToCard) {
+            this(keyId, null, null);
+
+            // these flags are mutually exclusive!
+            if (dummyStrip && moveKeyToCard) {
+                throw new AssertionError(
+                        "cannot set strip and keytocard flags at the same time - this is a bug!");
+            }
+            mDummyStrip = dummyStrip;
+            mMoveKeyToCard = moveKeyToCard;
+        }
+
         @Override
         public String toString() {
             String out = "mKeyId: " + mKeyId + ", ";
             out += "mFlags: " + mFlags + ", ";
             out += "mExpiry: " + mExpiry + ", ";
             out += "mDummyStrip: " + mDummyStrip + ", ";
+            out += "mMoveKeyToCard: " + mMoveKeyToCard + ", ";
             out += "mDummyDivert: [" + (mDummyDivert == null ? 0 : mDummyDivert.length) + " bytes]";
 
             return out;
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java
index bc2fbff76..acc0c32b8 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java
@@ -429,14 +429,14 @@ public class EditKeyFragment extends CryptoOperationFragment implements
 
                         SubkeyChange change = mSaveKeyringParcel.getSubkeyChange(keyId);
                         if (change == null) {
-                            mSaveKeyringParcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null));
+                            mSaveKeyringParcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false));
                             break;
                         }
                         // toggle
                         change.mDummyStrip = !change.mDummyStrip;
-                        if (change.mDummyStrip && change.mDummyDivert != null) {
+                        if (change.mDummyStrip && change.mMoveKeyToCard) {
                             // User had chosen to divert key, but now wants to strip it instead.
-                            change.mDummyDivert = null;
+                            change.mMoveKeyToCard = false;
                         }
                         break;
                     }
@@ -455,17 +455,15 @@ public class EditKeyFragment extends CryptoOperationFragment implements
                         change = mSaveKeyringParcel.getSubkeyChange(keyId);
                         if (change == null) {
                             mSaveKeyringParcel.mChangeSubKeys.add(
-                                    new SubkeyChange(keyId, false, null)
+                                    new SubkeyChange(keyId, false, true)
                             );
-                            change = mSaveKeyringParcel.getSubkeyChange(keyId);
+                            break;
                         }
                         // toggle
-                        if (change.mDummyDivert == null) {
-                            change.mDummyDivert = new byte[0];
-                            // If user had chosen to strip key, we cancel that action now.
+                        change.mMoveKeyToCard = !change.mMoveKeyToCard;
+                        if (change.mMoveKeyToCard && change.mDummyStrip) {
+                            // User had chosen to strip key, but now wants to divert it.
                             change.mDummyStrip = false;
-                        } else {
-                            change.mDummyDivert = null;
                         }
                         break;
                     }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java
index dfa5a39fb..87539ea05 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java
@@ -179,7 +179,7 @@ public class SubkeysAdapter extends CursorAdapter {
                 ? mSaveKeyringParcel.getSubkeyChange(keyId)
                 : null;
 
-        if (change != null && (change.mDummyStrip || change.mDummyDivert != null)) {
+        if (change != null && (change.mDummyStrip || change.mMoveKeyToCard)) {
             if (change.mDummyStrip) {
                 algorithmStr.append(", ");
                 final SpannableString boldStripped = new SpannableString(
@@ -188,7 +188,7 @@ public class SubkeysAdapter extends CursorAdapter {
                 boldStripped.setSpan(new StyleSpan(Typeface.BOLD), 0, boldStripped.length(), Spannable.SPAN_EXCLUSIVE_EXCLUSIVE);
                 algorithmStr.append(boldStripped);
             }
-            if (change.mDummyDivert != null) {
+            if (change.mMoveKeyToCard) {
                 algorithmStr.append(", ");
                 final SpannableString boldDivert = new SpannableString(
                         context.getString(R.string.key_divert)