From e716ce107de6e7bcc1beddc3aa0d2326dd0a2b59 Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Tue, 4 Jun 2019 19:41:52 +0000 Subject: [PATCH 1/2] Revert "Add generateChallenge() in onResume()" This reverts commit d59150eca6d7c0d0ea8a3fcba2a1add4beb36f05. Bug: 133498264 Bug: 133440610 Reason for revert: Fixing in a different way Change-Id: Ie444278a9e8b8aac259c31d311757ab8a39567f4 --- .../biometrics/face/FaceSettings.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/com/android/settings/biometrics/face/FaceSettings.java b/src/com/android/settings/biometrics/face/FaceSettings.java index 05e37781c70..48370d9fb78 100644 --- a/src/com/android/settings/biometrics/face/FaceSettings.java +++ b/src/com/android/settings/biometrics/face/FaceSettings.java @@ -145,11 +145,7 @@ public class FaceSettings extends DashboardFragment { if (savedInstanceState != null) { mToken = savedInstanceState.getByteArray(KEY_TOKEN); } - } - @Override - public void onResume() { - super.onResume(); if (mToken == null) { final long challenge = mFaceManager.generateChallenge(); ChooseLockSettingsHelper helper = new ChooseLockSettingsHelper(getActivity(), this); @@ -159,7 +155,13 @@ public class FaceSettings extends DashboardFragment { Log.e(TAG, "Password not set"); finish(); } - } else { + } + } + + @Override + public void onResume() { + super.onResume(); + if (mToken != null) { mAttentionController.setToken(mToken); mEnrollController.setToken(mToken); } @@ -194,12 +196,13 @@ public class FaceSettings extends DashboardFragment { } @Override - public void onStop() { - super.onStop(); - mToken = null; - final int result = mFaceManager.revokeChallenge(); - if (result < 0) { - Log.w(TAG, "revokeChallenge failed, result: " + result); + public void onDestroy() { + super.onDestroy(); + if (getActivity().isFinishing()) { + final int result = mFaceManager.revokeChallenge(); + if (result < 0) { + Log.w(TAG, "revokeChallenge failed, result: " + result); + } } } From f20bb1c635ebda997a785f4829e3791b4179a29e Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Tue, 4 Jun 2019 13:29:40 -0700 Subject: [PATCH 2/2] Resolve challenge lifecycle race conditions 1) FaceSettings now gets closed when it loses foreground. This prevents A) Keyguard/LockSettingsService's resetLockout's revokeChallenge from leaving FaceSettings with a stale HAT which prevents users from enrolling or toggling elements that require the HAT. B) generateChallenge has a timeout, which may already have been met C) User may have forgotten FaceSettings was open and lost context. Thus it makes no sense to show ConfirmLock* since the user may have no idea why it's showing anymore. 2) FaceSettings now generatesChallenge in onResume. onCreate is too early since again, FaceSettings can be launched via intent while on Keyguard. Similarly, we must ensure that Settings's challenge is generated late enough (e.g. when it actually gains foregroundness) Fixes: 133440610 Fixes: 133498264 Test: Open face settings, confirm password, lock screen. After unlocking, user needs to re-open face settings. Test: Modify HAL/framework to show re-enroll notification Tap re-enroll notificaton on keyguard Delete and re-enroll in settings, successful Test: FaceSettings enroll works accross orientation change Test: Tapping enroll button doesn't cause challenge to be revoked due to onStop() Change-Id: I60f606314c458a61e9c1b4f4b66bc27bc44287da --- .../biometrics/face/FaceSettings.java | 42 ++++++++++++------- ...tingsEnrollButtonPreferenceController.java | 9 ++++ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/com/android/settings/biometrics/face/FaceSettings.java b/src/com/android/settings/biometrics/face/FaceSettings.java index 48370d9fb78..9766f34cdad 100644 --- a/src/com/android/settings/biometrics/face/FaceSettings.java +++ b/src/com/android/settings/biometrics/face/FaceSettings.java @@ -69,6 +69,8 @@ public class FaceSettings extends DashboardFragment { private Preference mRemoveButton; private Preference mEnrollButton; + private boolean mConfirmingPassword; + private final FaceSettingsRemoveButtonPreferenceController.Listener mRemovalListener = () -> { // Disable the toggles until the user re-enrolls @@ -145,23 +147,27 @@ public class FaceSettings extends DashboardFragment { if (savedInstanceState != null) { mToken = savedInstanceState.getByteArray(KEY_TOKEN); } + } - if (mToken == null) { + @Override + public void onResume() { + super.onResume(); + + if (mToken == null && !mConfirmingPassword) { + // Generate challenge in onResume instead of onCreate, since FaceSettings can be + // created while Keyguard is showing, in which case the resetLockout revokeChallenge + // will invalidate the too-early created challenge here. final long challenge = mFaceManager.generateChallenge(); ChooseLockSettingsHelper helper = new ChooseLockSettingsHelper(getActivity(), this); + + mConfirmingPassword = true; if (!helper.launchConfirmationActivity(CONFIRM_REQUEST, getString(R.string.security_settings_face_preference_title), null, null, challenge, mUserId)) { Log.e(TAG, "Password not set"); finish(); } - } - } - - @Override - public void onResume() { - super.onResume(); - if (mToken != null) { + } else { mAttentionController.setToken(mToken); mEnrollController.setToken(mToken); } @@ -175,6 +181,7 @@ public class FaceSettings extends DashboardFragment { public void onActivityResult(int requestCode, int resultCode, Intent data) { super.onActivityResult(requestCode, resultCode, data); if (requestCode == CONFIRM_REQUEST) { + mConfirmingPassword = false; if (resultCode == RESULT_FINISHED || resultCode == RESULT_OK) { mFaceManager.setActiveUser(mUserId); // The pin/pattern/password was set. @@ -196,13 +203,20 @@ public class FaceSettings extends DashboardFragment { } @Override - public void onDestroy() { - super.onDestroy(); - if (getActivity().isFinishing()) { - final int result = mFaceManager.revokeChallenge(); - if (result < 0) { - Log.w(TAG, "revokeChallenge failed, result: " + result); + public void onStop() { + super.onStop(); + + if (!mEnrollController.isClicked() && !getActivity().isChangingConfigurations() + && !mConfirmingPassword) { + // Revoke challenge and finish + if (mToken != null) { + final int result = mFaceManager.revokeChallenge(); + if (result < 0) { + Log.w(TAG, "revokeChallenge failed, result: " + result); + } + mToken = null; } + getActivity().finish(); } } diff --git a/src/com/android/settings/biometrics/face/FaceSettingsEnrollButtonPreferenceController.java b/src/com/android/settings/biometrics/face/FaceSettingsEnrollButtonPreferenceController.java index ec7b1948fea..a087ccc78e4 100644 --- a/src/com/android/settings/biometrics/face/FaceSettingsEnrollButtonPreferenceController.java +++ b/src/com/android/settings/biometrics/face/FaceSettingsEnrollButtonPreferenceController.java @@ -42,6 +42,7 @@ public class FaceSettingsEnrollButtonPreferenceController extends BasePreference private byte[] mToken; private SettingsActivity mActivity; private Button mButton; + private boolean mIsClicked; public FaceSettingsEnrollButtonPreferenceController(Context context) { this(context, KEY); @@ -63,6 +64,7 @@ public class FaceSettingsEnrollButtonPreferenceController extends BasePreference @Override public void onClick(View v) { + mIsClicked = true; final Intent intent = new Intent(); intent.setClassName("com.android.settings", FaceEnrollIntroduction.class.getName()); intent.putExtra(Intent.EXTRA_USER_ID, mUserId); @@ -83,6 +85,13 @@ public class FaceSettingsEnrollButtonPreferenceController extends BasePreference mToken = token; } + // Return the click state, then clear its state. + public boolean isClicked() { + final boolean wasClicked = mIsClicked; + mIsClicked = false; + return wasClicked; + } + public void setActivity(SettingsActivity activity) { mActivity = activity; }