From fd7bdbf54f4805e0346972700dc2cd3680532059 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 7 Sep 2014 16:54:39 +0200 Subject: [PATCH 1/2] add proper async check for correct passphrase to passphrasedialog --- .../ui/dialog/PassphraseDialogFragment.java | 183 ++++++++++++------ .../src/main/res/layout/passphrase_dialog.xml | 69 +++++-- OpenKeychain/src/main/res/values/strings.xml | 1 + 3 files changed, 172 insertions(+), 81 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/PassphraseDialogFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/PassphraseDialogFragment.java index 47d689193..d433677a3 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/PassphraseDialogFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/PassphraseDialogFragment.java @@ -23,6 +23,7 @@ import android.app.Dialog; import android.content.Context; import android.content.DialogInterface; import android.content.DialogInterface.OnClickListener; +import android.os.AsyncTask; import android.os.Bundle; import android.os.Handler; import android.os.Message; @@ -64,6 +65,7 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor private Messenger mMessenger; private EditText mPassphraseEditText; + private View mInput, mProgress; /** * Shows passphrase dialog to cache a new passphrase the user enters for using it later for @@ -77,8 +79,8 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor DialogFragmentWorkaround.INTERFACE.runnableRunDelayed(new Runnable() { public void run() { try { - PassphraseDialogFragment passphraseDialog = PassphraseDialogFragment.newInstance(context, - messenger, keyId); + PassphraseDialogFragment passphraseDialog = + PassphraseDialogFragment.newInstance(messenger, keyId); passphraseDialog.show(context.getSupportFragmentManager(), "passphraseDialog"); } catch (PgpGeneralException e) { @@ -98,8 +100,8 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor * @return * @throws PgpGeneralException */ - public static PassphraseDialogFragment newInstance(Context context, Messenger messenger, - long secretKeyId) throws PgpGeneralException { + public static PassphraseDialogFragment newInstance(Messenger messenger, long secretKeyId) + throws PgpGeneralException { // do NOT check if the key even needs a passphrase. that's not our job here. PassphraseDialogFragment frag = new PassphraseDialogFragment(); Bundle args = new Bundle(); @@ -111,46 +113,48 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor return frag; } + CanonicalizedSecretKeyRing mSecretRing = null; + boolean mIsCancelled = false; + long mSubKeyId; + /** * Creates dialog */ @Override public Dialog onCreateDialog(Bundle savedInstanceState) { final Activity activity = getActivity(); - final long subKeyId = getArguments().getLong(ARG_SECRET_KEY_ID); + mSubKeyId = getArguments().getLong(ARG_SECRET_KEY_ID); mMessenger = getArguments().getParcelable(ARG_MESSENGER); CustomAlertDialogBuilder alert = new CustomAlertDialogBuilder(activity); alert.setTitle(R.string.title_authentication); - final CanonicalizedSecretKeyRing secretRing; String userId; - if (subKeyId == Constants.key.symmetric || subKeyId == Constants.key.none) { + if (mSubKeyId == Constants.key.symmetric || mSubKeyId == Constants.key.none) { alert.setMessage(R.string.passphrase_for_symmetric_encryption); - secretRing = null; } else { String message; try { ProviderHelper helper = new ProviderHelper(activity); - secretRing = helper.getCanonicalizedSecretKeyRing( - KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(subKeyId)); + mSecretRing = helper.getCanonicalizedSecretKeyRing( + KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(mSubKeyId)); // yes the inner try/catch block is necessary, otherwise the final variable // above can't be statically verified to have been set in all cases because // the catch clause doesn't return. try { - userId = secretRing.getPrimaryUserIdWithFallback(); + userId = mSecretRing.getPrimaryUserIdWithFallback(); } catch (PgpGeneralException e) { userId = null; } /* Get key type for message */ // find a master key id for our key - long masterKeyId = new ProviderHelper(getActivity()).getMasterKeyId(subKeyId); + long masterKeyId = new ProviderHelper(getActivity()).getMasterKeyId(mSubKeyId); CachedPublicKeyRing keyRing = new ProviderHelper(getActivity()).getCachedPublicKeyRing(masterKeyId); // get the type of key (from the database) - CanonicalizedSecretKey.SecretKeyType keyType = keyRing.getSecretKeyType(subKeyId); + CanonicalizedSecretKey.SecretKeyType keyType = keyRing.getSecretKeyType(mSubKeyId); switch (keyType) { case PASSPHRASE: message = getString(R.string.passphrase_for, userId); @@ -165,7 +169,7 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor } catch (ProviderHelper.NotFoundException e) { alert.setTitle(R.string.title_key_not_found); - alert.setMessage(getString(R.string.key_not_found, subKeyId)); + alert.setMessage(getString(R.string.key_not_found, mSubKeyId)); alert.setPositiveButton(android.R.string.ok, new OnClickListener() { public void onClick(DialogInterface dialog, int which) { dismiss(); @@ -183,54 +187,8 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor alert.setView(view); mPassphraseEditText = (EditText) view.findViewById(R.id.passphrase_passphrase); - - alert.setPositiveButton(android.R.string.ok, new DialogInterface.OnClickListener() { - - @Override - public void onClick(DialogInterface dialog, int id) { - dismiss(); - - String passphrase = mPassphraseEditText.getText().toString(); - - // Early breakout if we are dealing with a symmetric key - if (secretRing == null) { - PassphraseCacheService.addCachedPassphrase(activity, Constants.key.symmetric, - passphrase, getString(R.string.passp_cache_notif_pwd)); - // also return passphrase back to activity - Bundle data = new Bundle(); - data.putString(MESSAGE_DATA_PASSPHRASE, passphrase); - sendMessageToHandler(MESSAGE_OKAY, data); - return; - } - - try { - // make sure this unlocks - // TODO this is a very costly operation, we should not be doing this here! - secretRing.getSecretKey(subKeyId).unlock(passphrase); - } catch (PgpGeneralException e) { - Toast.makeText(activity, R.string.error_could_not_extract_private_key, - Toast.LENGTH_SHORT).show(); - - sendMessageToHandler(MESSAGE_CANCEL); - return; // ran out of keys to try - } - - // cache the new passphrase - Log.d(Constants.TAG, "Everything okay! Caching entered passphrase"); - - try { - PassphraseCacheService.addCachedPassphrase(activity, subKeyId, passphrase, - secretRing.getPrimaryUserIdWithFallback()); - } catch (PgpGeneralException e) { - Log.e(Constants.TAG, "adding of a passphrase failed", e); - } - - // also return passphrase back to activity - Bundle data = new Bundle(); - data.putString(MESSAGE_DATA_PASSPHRASE, passphrase); - sendMessageToHandler(MESSAGE_OKAY, data); - } - }); + mInput = view.findViewById(R.id.input); + mProgress = view.findViewById(R.id.progress); alert.setNegativeButton(android.R.string.cancel, new DialogInterface.OnClickListener() { @@ -263,13 +221,112 @@ public class PassphraseDialogFragment extends DialogFragment implements OnEditor mPassphraseEditText.setImeActionLabel(getString(android.R.string.ok), EditorInfo.IME_ACTION_DONE); mPassphraseEditText.setOnEditorActionListener(this); - return alert.show(); + AlertDialog dialog = alert.create(); + dialog.setButton(DialogInterface.BUTTON_POSITIVE, + activity.getString(android.R.string.ok), (DialogInterface.OnClickListener) null); + + return dialog; + } + + @Override + public void onStart() { + super.onStart(); + + // Override the default behavior so the dialog is NOT dismissed on click + final Button positive = ((AlertDialog) getDialog()).getButton(DialogInterface.BUTTON_POSITIVE); + positive.setOnClickListener(new View.OnClickListener() { + @Override + public void onClick(View v) { + final String passphrase = mPassphraseEditText.getText().toString(); + + // Early breakout if we are dealing with a symmetric key + if (mSecretRing == null) { + PassphraseCacheService.addCachedPassphrase(getActivity(), Constants.key.symmetric, + passphrase, getString(R.string.passp_cache_notif_pwd)); + // also return passphrase back to activity + Bundle data = new Bundle(); + data.putString(MESSAGE_DATA_PASSPHRASE, passphrase); + sendMessageToHandler(MESSAGE_OKAY, data); + dismiss(); + return; + } + + mInput.setVisibility(View.GONE); + mProgress.setVisibility(View.VISIBLE); + positive.setEnabled(false); + + new AsyncTask() { + @Override + protected Boolean doInBackground(Void... params) { + try { + // wait some 100ms here, give the user time to appreciate the progress bar + try { + Thread.sleep(100); + } catch (InterruptedException e) { + // never mind + } + // make sure this unlocks + return mSecretRing.getSecretKey(mSubKeyId).unlock(passphrase); + } catch (PgpGeneralException e) { + Toast.makeText(getActivity(), R.string.error_could_not_extract_private_key, + Toast.LENGTH_SHORT).show(); + + sendMessageToHandler(MESSAGE_CANCEL); + dismiss(); + return false; + } + } + + /** Handle a good or bad passphrase. This happens in the UI thread! */ + @Override + protected void onPostExecute(Boolean result) { + super.onPostExecute(result); + + // if we were cancelled in the meantime, the result isn't relevant anymore + if (mIsCancelled) { + return; + } + + // if the passphrase was wrong, reset and re-enable the dialogue + if (!result) { + // TODO add a "bad passphrase" dialogue? + mPassphraseEditText.setText(""); + mInput.setVisibility(View.VISIBLE); + mProgress.setVisibility(View.GONE); + positive.setEnabled(true); + return; + } + + // cache the new passphrase + Log.d(Constants.TAG, "Everything okay! Caching entered passphrase"); + + try { + PassphraseCacheService.addCachedPassphrase(getActivity(), mSubKeyId, + passphrase, mSecretRing.getPrimaryUserIdWithFallback()); + } catch (PgpGeneralException e) { + Log.e(Constants.TAG, "adding of a passphrase failed", e); + } + + // also return passphrase back to activity + Bundle data = new Bundle(); + data.putString(MESSAGE_DATA_PASSPHRASE, passphrase); + sendMessageToHandler(MESSAGE_OKAY, data); + dismiss(); + } + }.execute(); + } + }); + } @Override public void onCancel(DialogInterface dialog) { super.onCancel(dialog); + // note we need no synchronization here, this variable is only accessed in the ui thread + mIsCancelled = true; + + // dismiss the dialogue dismiss(); sendMessageToHandler(MESSAGE_CANCEL); } diff --git a/OpenKeychain/src/main/res/layout/passphrase_dialog.xml b/OpenKeychain/src/main/res/layout/passphrase_dialog.xml index 4b331f0f2..ebc5615e4 100644 --- a/OpenKeychain/src/main/res/layout/passphrase_dialog.xml +++ b/OpenKeychain/src/main/res/layout/passphrase_dialog.xml @@ -1,24 +1,57 @@ + android:layout_width="match_parent" + android:layout_height="wrap_content" + android:paddingLeft="16dp" + android:paddingRight="16dp" + android:orientation="vertical"> - + - + + + + + + + + + + + \ No newline at end of file diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index e39bca63c..c71447b70 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -87,6 +87,7 @@ File: No Passphrase Passphrase + Unlocking… Repeat Passphrase Algorithm File ASCII Armor From 31cc004fe144b654533f04ed5291ea77c80d5f1c Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 7 Sep 2014 16:55:07 +0200 Subject: [PATCH 2/2] make EditKeyFragment work with new PassphraseDialog --- .../keychain/ui/EditKeyFragment.java | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) 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 2137af65f..11d5292bb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java @@ -41,9 +41,9 @@ import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.compatibility.DialogFragmentWorkaround; import org.sufficientlysecure.keychain.helper.ActionBarHelper; -import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKeyRing; import org.sufficientlysecure.keychain.pgp.KeyRing; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; +import org.sufficientlysecure.keychain.provider.CachedPublicKeyRing; import org.sufficientlysecure.keychain.provider.KeychainContract; import org.sufficientlysecure.keychain.provider.ProviderHelper; import org.sufficientlysecure.keychain.service.KeychainIntentService; @@ -164,25 +164,56 @@ public class EditKeyFragment extends LoaderFragment implements Log.i(Constants.TAG, "mDataUri: " + mDataUri.toString()); + // load the secret key ring. we do verify here that the passphrase is correct, so cached won't do try { Uri secretUri = KeychainContract.KeyRings.buildUnifiedKeyRingUri(mDataUri); - CanonicalizedSecretKeyRing keyRing = - new ProviderHelper(getActivity()).getCanonicalizedSecretKeyRing(secretUri); + CachedPublicKeyRing keyRing = + new ProviderHelper(getActivity()).getCachedPublicKeyRing(secretUri); mSaveKeyringParcel = new SaveKeyringParcel(keyRing.getMasterKeyId(), - keyRing.getUncachedKeyRing().getFingerprint()); + keyRing.getFingerprint()); mPrimaryUserId = keyRing.getPrimaryUserIdWithFallback(); - } catch (ProviderHelper.NotFoundException e) { - Log.e(Constants.TAG, "Keyring not found", e); - Toast.makeText(getActivity(), R.string.error_no_secret_key_found, Toast.LENGTH_SHORT).show(); - getActivity().finish(); + } catch (PgpGeneralException e) { Log.e(Constants.TAG, "PgpGeneralException", e); Toast.makeText(getActivity(), R.string.error_no_secret_key_found, Toast.LENGTH_SHORT).show(); getActivity().finish(); + return; } - cachePassphraseForEdit(); + try { + mCurrentPassphrase = PassphraseCacheService.getCachedPassphrase(getActivity(), + mSaveKeyringParcel.mMasterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + Log.e(Constants.TAG, "Key not found!", e); + getActivity().finish(); + return; + } + + if (mCurrentPassphrase == null) { + PassphraseDialogFragment.show(getActivity(), mSaveKeyringParcel.mMasterKeyId, + new Handler() { + @Override + public void handleMessage(Message message) { + if (message.what == PassphraseDialogFragment.MESSAGE_OKAY) { + mCurrentPassphrase = message.getData().getString( + PassphraseDialogFragment.MESSAGE_DATA_PASSPHRASE); + // Prepare the loaders. Either re-connect with an existing ones, + // or start new ones. + getLoaderManager().initLoader(LOADER_ID_USER_IDS, null, EditKeyFragment.this); + getLoaderManager().initLoader(LOADER_ID_SUBKEYS, null, EditKeyFragment.this); + } else { + EditKeyFragment.this.getActivity().finish(); + } + } + } + ); + } else { + // Prepare the loaders. Either re-connect with an existing ones, + // or start new ones. + getLoaderManager().initLoader(LOADER_ID_USER_IDS, null, EditKeyFragment.this); + getLoaderManager().initLoader(LOADER_ID_SUBKEYS, null, EditKeyFragment.this); + } mChangePassphrase.setOnClickListener(new View.OnClickListener() { @Override @@ -232,10 +263,6 @@ public class EditKeyFragment extends LoaderFragment implements mSubkeysAddedAdapter = new SubkeysAddedAdapter(getActivity(), mSaveKeyringParcel.mAddSubKeys); mSubkeysAddedList.setAdapter(mSubkeysAddedAdapter); - // Prepare the loaders. Either re-connect with an existing ones, - // or start new ones. - getLoaderManager().initLoader(LOADER_ID_USER_IDS, null, this); - getLoaderManager().initLoader(LOADER_ID_SUBKEYS, null, this); } public Loader onCreateLoader(int id, Bundle args) { @@ -474,33 +501,6 @@ public class EditKeyFragment extends LoaderFragment implements addSubkeyDialogFragment.show(getActivity().getSupportFragmentManager(), "addSubkeyDialog"); } - private void cachePassphraseForEdit() { - try { - mCurrentPassphrase = PassphraseCacheService.getCachedPassphrase(getActivity(), - mSaveKeyringParcel.mMasterKeyId); - } catch (PassphraseCacheService.KeyNotFoundException e) { - Log.e(Constants.TAG, "Key not found!", e); - getActivity().finish(); - return; - } - if (mCurrentPassphrase == null) { - PassphraseDialogFragment.show(getActivity(), mSaveKeyringParcel.mMasterKeyId, - new Handler() { - @Override - public void handleMessage(Message message) { - if (message.what == PassphraseDialogFragment.MESSAGE_OKAY) { - mCurrentPassphrase = - message.getData().getString(PassphraseDialogFragment.MESSAGE_DATA_PASSPHRASE); - Log.d(Constants.TAG, "after caching passphrase"); - } else { - EditKeyFragment.this.getActivity().finish(); - } - } - } - ); - } - } - private void save(String passphrase) { Log.d(Constants.TAG, "mSaveKeyringParcel:\n" + mSaveKeyringParcel.toString());