From 7880a86f50c858883910ba9e84f4611bce9254c3 Mon Sep 17 00:00:00 2001 From: Joe Bolinger Date: Wed, 30 Mar 2022 01:28:15 +0000 Subject: [PATCH] Prevent multiple calls to handleOnDialogAnimatedIn. This also adds the request id to the methods that hide the prompt to prevent stale requests from being handled. I wasn't able to reproduce the bug locally but it appears to be caused by these stale events. Fix: 225275518 Bug: 213899762 Test: atest BiometricServiceTest AuthContainerViewTest Change-Id: Iac414763dd0012e51b331abe649a50e353df5e1c --- .../internal/statusbar/IStatusBar.aidl | 2 +- .../internal/statusbar/IStatusBarService.aidl | 2 +- .../biometrics/AuthContainerView.java | 16 +++++- .../systemui/biometrics/AuthController.java | 7 ++- .../systemui/biometrics/AuthDialog.java | 3 ++ .../systemui/statusbar/CommandQueue.java | 15 ++++-- .../biometrics/AuthContainerViewTest.kt | 49 ++++++++++++++++--- .../biometrics/AuthControllerTest.java | 20 ++++++-- .../systemui/statusbar/CommandQueueTest.java | 5 +- .../server/biometrics/AuthSession.java | 10 ++-- .../statusbar/StatusBarManagerService.java | 4 +- .../biometrics/BiometricServiceTest.java | 16 +++--- 12 files changed, 112 insertions(+), 37 deletions(-) diff --git a/core/java/com/android/internal/statusbar/IStatusBar.aidl b/core/java/com/android/internal/statusbar/IStatusBar.aidl index 089179dbba273..634063a44b66b 100644 --- a/core/java/com/android/internal/statusbar/IStatusBar.aidl +++ b/core/java/com/android/internal/statusbar/IStatusBar.aidl @@ -169,7 +169,7 @@ oneway interface IStatusBar /** * Used to hide the authentication dialog, e.g. when the application cancels authentication. */ - void hideAuthenticationDialog(); + void hideAuthenticationDialog(long requestId); /* Used to notify the biometric service of events that occur outside of an operation. */ void setBiometicContextListener(in IBiometricContextListener listener); diff --git a/core/java/com/android/internal/statusbar/IStatusBarService.aidl b/core/java/com/android/internal/statusbar/IStatusBarService.aidl index 2ee5e797b4ab2..46b463074383e 100644 --- a/core/java/com/android/internal/statusbar/IStatusBarService.aidl +++ b/core/java/com/android/internal/statusbar/IStatusBarService.aidl @@ -131,7 +131,7 @@ interface IStatusBarService // Used to show an error - the dialog will dismiss after a certain amount of time void onBiometricError(int modality, int error, int vendorCode); // Used to hide the authentication dialog, e.g. when the application cancels authentication - void hideAuthenticationDialog(); + void hideAuthenticationDialog(long requestId); // Used to notify the biometric service of events that occur outside of an operation. void setBiometicContextListener(in IBiometricContextListener listener); diff --git a/packages/SystemUI/src/com/android/systemui/biometrics/AuthContainerView.java b/packages/SystemUI/src/com/android/systemui/biometrics/AuthContainerView.java index 6b6af4c7b52fc..b2673e9230086 100644 --- a/packages/SystemUI/src/com/android/systemui/biometrics/AuthContainerView.java +++ b/packages/SystemUI/src/com/android/systemui/biometrics/AuthContainerView.java @@ -126,7 +126,7 @@ public class AuthContainerView extends LinearLayout int[] mSensorIds; boolean mSkipIntro; long mOperationId; - long mRequestId; + long mRequestId = -1; boolean mSkipAnimation = false; @BiometricMultiSensorMode int mMultiSensorConfig = BIOMETRIC_MULTI_SENSOR_DEFAULT; } @@ -598,6 +598,11 @@ public class AuthContainerView extends LinearLayout return mConfig.mOpPackageName; } + @Override + public long getRequestId() { + return mConfig.mRequestId; + } + @Override public void animateToCredentialUI() { mBiometricView.startTransitionToCredentialUI(); @@ -678,7 +683,9 @@ public class AuthContainerView extends LinearLayout return; } mContainerState = STATE_GONE; - mWindowManager.removeView(this); + if (isAttachedToWindow()) { + mWindowManager.removeView(this); + } } private void onDialogAnimatedIn() { @@ -687,6 +694,11 @@ public class AuthContainerView extends LinearLayout animateAway(AuthDialogCallback.DISMISSED_USER_CANCELED); return; } + if (mContainerState == STATE_ANIMATING_OUT || mContainerState == STATE_GONE) { + Log.d(TAG, "onDialogAnimatedIn(): ignore, already animating out or gone - state: " + + mContainerState); + return; + } mContainerState = STATE_SHOWING; if (mBiometricView != null) { mConfig.mCallback.onDialogAnimatedIn(); diff --git a/packages/SystemUI/src/com/android/systemui/biometrics/AuthController.java b/packages/SystemUI/src/com/android/systemui/biometrics/AuthController.java index c100a0744b85b..aaf18b309db2e 100644 --- a/packages/SystemUI/src/com/android/systemui/biometrics/AuthController.java +++ b/packages/SystemUI/src/com/android/systemui/biometrics/AuthController.java @@ -768,7 +768,7 @@ public class AuthController extends CoreStartable implements CommandQueue.Callba } @Override - public void hideAuthenticationDialog() { + public void hideAuthenticationDialog(long requestId) { if (DEBUG) Log.d(TAG, "hideAuthenticationDialog: " + mCurrentDialog); if (mCurrentDialog == null) { @@ -777,6 +777,11 @@ public class AuthController extends CoreStartable implements CommandQueue.Callba if (DEBUG) Log.d(TAG, "dialog already gone"); return; } + if (requestId != mCurrentDialog.getRequestId()) { + Log.w(TAG, "ignore - ids do not match: " + requestId + " current: " + + mCurrentDialog.getRequestId()); + return; + } mCurrentDialog.dismissFromSystemServer(); diff --git a/packages/SystemUI/src/com/android/systemui/biometrics/AuthDialog.java b/packages/SystemUI/src/com/android/systemui/biometrics/AuthDialog.java index 59ed156bce338..4ff19f6adc115 100644 --- a/packages/SystemUI/src/com/android/systemui/biometrics/AuthDialog.java +++ b/packages/SystemUI/src/com/android/systemui/biometrics/AuthDialog.java @@ -150,6 +150,9 @@ public interface AuthDialog { */ String getOpPackageName(); + /** The requestId of the underlying operation within the framework. */ + long getRequestId(); + /** * Animate to credential UI. Typically called after biometric is locked out. */ diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/CommandQueue.java b/packages/SystemUI/src/com/android/systemui/statusbar/CommandQueue.java index d9a98b165795b..5585cde528faa 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/CommandQueue.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/CommandQueue.java @@ -323,7 +323,7 @@ public class CommandQueue extends IStatusBar.Stub implements default void onBiometricError(@Modality int modality, int error, int vendorCode) { } - default void hideAuthenticationDialog() { + default void hideAuthenticationDialog(long requestId) { } /** @@ -999,9 +999,11 @@ public class CommandQueue extends IStatusBar.Stub implements } @Override - public void hideAuthenticationDialog() { + public void hideAuthenticationDialog(long requestId) { synchronized (mLock) { - mHandler.obtainMessage(MSG_BIOMETRIC_HIDE).sendToTarget(); + final SomeArgs args = SomeArgs.obtain(); + args.argl1 = requestId; + mHandler.obtainMessage(MSG_BIOMETRIC_HIDE, args).sendToTarget(); } } @@ -1508,11 +1510,14 @@ public class CommandQueue extends IStatusBar.Stub implements someArgs.recycle(); break; } - case MSG_BIOMETRIC_HIDE: + case MSG_BIOMETRIC_HIDE: { + final SomeArgs someArgs = (SomeArgs) msg.obj; for (int i = 0; i < mCallbacks.size(); i++) { - mCallbacks.get(i).hideAuthenticationDialog(); + mCallbacks.get(i).hideAuthenticationDialog(someArgs.argl1 /* requestId */); } + someArgs.recycle(); break; + } case MSG_SET_BIOMETRICS_LISTENER: for (int i = 0; i < mCallbacks.size(); i++) { mCallbacks.get(i).setBiometicContextListener( diff --git a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthContainerViewTest.kt b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthContainerViewTest.kt index 483dbf51d4a94..666c9e481adc1 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthContainerViewTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthContainerViewTest.kt @@ -48,6 +48,7 @@ import org.junit.runner.RunWith import org.mockito.Mock import org.mockito.Mockito.anyInt import org.mockito.Mockito.eq +import org.mockito.Mockito.never import org.mockito.Mockito.verify import org.mockito.junit.MockitoJUnit import org.mockito.Mockito.`when` as whenever @@ -80,9 +81,30 @@ class AuthContainerViewTest : SysuiTestCase() { } } + @Test + fun testNotifiesAnimatedIn() { + initializeContainer() + verify(callback).onDialogAnimatedIn() + } + + @Test + fun testIgnoresAnimatedInWhenDismissed() { + val container = initializeContainer(addToView = false) + container.dismissFromSystemServer() + waitForIdleSync() + + verify(callback, never()).onDialogAnimatedIn() + + container.addToView() + waitForIdleSync() + + // attaching the view resets the state and allows this to happen again + verify(callback).onDialogAnimatedIn() + } + @Test fun testActionAuthenticated_sendsDismissedAuthenticated() { - val container = initializeContainer(BiometricManager.Authenticators.BIOMETRIC_WEAK) + val container = initializeContainer() container.mBiometricCallback.onAction( AuthBiometricView.Callback.ACTION_AUTHENTICATED ) @@ -97,7 +119,7 @@ class AuthContainerViewTest : SysuiTestCase() { @Test fun testActionUserCanceled_sendsDismissedUserCanceled() { - val container = initializeContainer(BiometricManager.Authenticators.BIOMETRIC_WEAK) + val container = initializeContainer() container.mBiometricCallback.onAction( AuthBiometricView.Callback.ACTION_USER_CANCELED ) @@ -115,7 +137,7 @@ class AuthContainerViewTest : SysuiTestCase() { @Test fun testActionButtonNegative_sendsDismissedButtonNegative() { - val container = initializeContainer(BiometricManager.Authenticators.BIOMETRIC_WEAK) + val container = initializeContainer() container.mBiometricCallback.onAction( AuthBiometricView.Callback.ACTION_BUTTON_NEGATIVE ) @@ -141,7 +163,7 @@ class AuthContainerViewTest : SysuiTestCase() { @Test fun testActionError_sendsDismissedError() { - val container = initializeContainer(BiometricManager.Authenticators.BIOMETRIC_WEAK) + val container = initializeContainer() authContainer!!.mBiometricCallback.onAction( AuthBiometricView.Callback.ACTION_ERROR ) @@ -183,7 +205,7 @@ class AuthContainerViewTest : SysuiTestCase() { @Test fun testShowBiometricUI() { - val container = initializeContainer(BiometricManager.Authenticators.BIOMETRIC_WEAK) + val container = initializeContainer() waitForIdleSync() @@ -252,7 +274,10 @@ class AuthContainerViewTest : SysuiTestCase() { assertThat((layoutParams.fitInsetsTypes and WindowInsets.Type.ime()) == 0).isTrue() } - private fun initializeContainer(authenticators: Int): TestAuthContainerView { + private fun initializeContainer( + authenticators: Int = BiometricManager.Authenticators.BIOMETRIC_WEAK, + addToView: Boolean = true + ): TestAuthContainerView { val config = AuthContainerView.Config() config.mContext = mContext config.mCallback = callback @@ -291,7 +316,11 @@ class AuthContainerViewTest : SysuiTestCase() { lockPatternUtils, Handler(TestableLooper.get(this).looper) ) - ViewUtils.attachView(authContainer) + + if (addToView) { + authContainer!!.addToView() + } + return authContainer!! } @@ -316,6 +345,12 @@ class AuthContainerViewTest : SysuiTestCase() { TestableLooper.get(this).processAllMessages() super.waitForIdleSync() } + + private fun AuthContainerView.addToView() { + ViewUtils.attachView(this) + waitForIdleSync() + assertThat(isAttachedToWindow).isTrue() + } } private fun AuthContainerView.hasBiometricPrompt() = diff --git a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthControllerTest.java index 42c3c7f899fab..190228d80cde8 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/biometrics/AuthControllerTest.java @@ -20,6 +20,8 @@ import static android.hardware.biometrics.BiometricAuthenticator.TYPE_FINGERPRIN import static android.hardware.biometrics.BiometricManager.Authenticators; import static android.hardware.biometrics.BiometricManager.BIOMETRIC_MULTI_SENSOR_FINGERPRINT_AND_FACE; +import static com.google.common.truth.Truth.assertThat; + import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertNull; @@ -104,6 +106,8 @@ import javax.inject.Provider; @SmallTest public class AuthControllerTest extends SysuiTestCase { + private static final long REQUEST_ID = 22; + @Rule public final MockitoRule mMockitoRule = MockitoJUnit.rule(); @@ -173,6 +177,9 @@ public class AuthControllerTest extends SysuiTestCase { when(mDialog1.isAllowDeviceCredentials()).thenReturn(false); when(mDialog2.isAllowDeviceCredentials()).thenReturn(false); + when(mDialog1.getRequestId()).thenReturn(REQUEST_ID); + when(mDialog2.getRequestId()).thenReturn(REQUEST_ID); + when(mFingerprintManager.isHardwareDetected()).thenReturn(true); final List componentInfo = new ArrayList<>(); @@ -482,7 +489,12 @@ public class AuthControllerTest extends SysuiTestCase { @Test public void testHideAuthenticationDialog_invokesDismissFromSystemServer() { showDialog(new int[] {1} /* sensorIds */, false /* credentialAllowed */); - mAuthController.hideAuthenticationDialog(); + + mAuthController.hideAuthenticationDialog(REQUEST_ID + 1); + verify(mDialog1, never()).dismissFromSystemServer(); + assertThat(mAuthController.mCurrentDialog).isSameInstanceAs(mDialog1); + + mAuthController.hideAuthenticationDialog(REQUEST_ID); verify(mDialog1).dismissFromSystemServer(); // In this case, BiometricService sends the error to the client immediately, without @@ -512,7 +524,7 @@ public class AuthControllerTest extends SysuiTestCase { eq(BiometricPrompt.DISMISSED_REASON_CREDENTIAL_CONFIRMED), AdditionalMatchers.aryEq(credentialAttestation)); - mAuthController.hideAuthenticationDialog(); + mAuthController.hideAuthenticationDialog(REQUEST_ID); } @Test @@ -648,7 +660,7 @@ public class AuthControllerTest extends SysuiTestCase { verify(mDisplayManager).registerDisplayListener(any(), eq(mHandler)); - mAuthController.hideAuthenticationDialog(); + mAuthController.hideAuthenticationDialog(REQUEST_ID); verify(mDisplayManager).unregisterDisplayListener(any()); } @@ -704,7 +716,7 @@ public class AuthControllerTest extends SysuiTestCase { 0 /* userId */, 0 /* operationId */, "testPackage", - 1 /* requestId */, + REQUEST_ID, BIOMETRIC_MULTI_SENSOR_FINGERPRINT_AND_FACE); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/CommandQueueTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/CommandQueueTest.java index 11f76a381ad40..fc4d9c42eb49e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/CommandQueueTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/CommandQueueTest.java @@ -475,9 +475,10 @@ public class CommandQueueTest extends SysuiTestCase { @Test public void testHideAuthenticationDialog() { - mCommandQueue.hideAuthenticationDialog(); + final long id = 4; + mCommandQueue.hideAuthenticationDialog(id); waitForIdleSync(); - verify(mCallbacks).hideAuthenticationDialog(); + verify(mCallbacks).hideAuthenticationDialog(eq(id)); } @Test diff --git a/services/core/java/com/android/server/biometrics/AuthSession.java b/services/core/java/com/android/server/biometrics/AuthSession.java index bf69284df2f61..cc49f07dd0e5d 100644 --- a/services/core/java/com/android/server/biometrics/AuthSession.java +++ b/services/core/java/com/android/server/biometrics/AuthSession.java @@ -462,7 +462,7 @@ public final class AuthSession implements IBinder.DeathRecipient { mState = STATE_SHOWING_DEVICE_CREDENTIAL; mStatusBarService.onBiometricError(modality, error, vendorCode); } else if (error == BiometricConstants.BIOMETRIC_ERROR_CANCELED) { - mStatusBarService.hideAuthenticationDialog(); + mStatusBarService.hideAuthenticationDialog(mRequestId); // TODO: If multiple authenticators are simultaneously running, this will // need to be modified. Send the error to the client here, instead of doing // a round trip to SystemUI. @@ -480,7 +480,7 @@ public final class AuthSession implements IBinder.DeathRecipient { // the client and clean up. The only error we should get here is // ERROR_CANCELED due to another client kicking us out. mClientReceiver.onError(modality, error, vendorCode); - mStatusBarService.hideAuthenticationDialog(); + mStatusBarService.hideAuthenticationDialog(mRequestId); return true; } @@ -489,7 +489,7 @@ public final class AuthSession implements IBinder.DeathRecipient { break; case STATE_CLIENT_DIED_CANCELLING: - mStatusBarService.hideAuthenticationDialog(); + mStatusBarService.hideAuthenticationDialog(mRequestId); return true; default: @@ -665,7 +665,7 @@ public final class AuthSession implements IBinder.DeathRecipient { cancelAllSensors(); return false; default: - mStatusBarService.hideAuthenticationDialog(); + mStatusBarService.hideAuthenticationDialog(mRequestId); return true; } } catch (RemoteException e) { @@ -832,7 +832,7 @@ public final class AuthSession implements IBinder.DeathRecipient { BiometricConstants.BIOMETRIC_ERROR_CANCELED, 0 /* vendorCode */ ); - mStatusBarService.hideAuthenticationDialog(); + mStatusBarService.hideAuthenticationDialog(mRequestId); return true; } catch (RemoteException e) { Slog.e(TAG, "Remote exception", e); diff --git a/services/core/java/com/android/server/statusbar/StatusBarManagerService.java b/services/core/java/com/android/server/statusbar/StatusBarManagerService.java index 80877382f75ae..b6855726c1227 100644 --- a/services/core/java/com/android/server/statusbar/StatusBarManagerService.java +++ b/services/core/java/com/android/server/statusbar/StatusBarManagerService.java @@ -903,11 +903,11 @@ public class StatusBarManagerService extends IStatusBarService.Stub implements D } @Override - public void hideAuthenticationDialog() { + public void hideAuthenticationDialog(long requestId) { enforceBiometricDialog(); if (mBar != null) { try { - mBar.hideAuthenticationDialog(); + mBar.hideAuthenticationDialog(requestId); } catch (RemoteException ex) { } } diff --git a/services/tests/servicestests/src/com/android/server/biometrics/BiometricServiceTest.java b/services/tests/servicestests/src/com/android/server/biometrics/BiometricServiceTest.java index 2ad5eaeb9aaff..85d8aba58dbbf 100644 --- a/services/tests/servicestests/src/com/android/server/biometrics/BiometricServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/biometrics/BiometricServiceTest.java @@ -192,7 +192,7 @@ public class BiometricServiceTest { waitForIdle(); assertNull(mBiometricService.mAuthSession); - verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(eq(TEST_REQUEST_ID)); verify(mReceiver1, never()).onError(anyInt(), anyInt(), anyInt()); } @@ -211,7 +211,8 @@ public class BiometricServiceTest { waitForIdle(); assertNotNull(mBiometricService.mAuthSession); - verify(mBiometricService.mStatusBarService, never()).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService, never()) + .hideAuthenticationDialog(eq(TEST_REQUEST_ID)); assertEquals(STATE_CLIENT_DIED_CANCELLING, mBiometricService.mAuthSession.getState()); @@ -225,7 +226,7 @@ public class BiometricServiceTest { BiometricConstants.BIOMETRIC_ERROR_CANCELED, 0 /* vendorCode */); waitForIdle(); - verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(eq(TEST_REQUEST_ID)); verify(mReceiver1, never()).onError(anyInt(), anyInt(), anyInt()); assertNull(mBiometricService.mAuthSession); } @@ -666,7 +667,7 @@ public class BiometricServiceTest { eq(BiometricAuthenticator.TYPE_FACE), eq(BiometricPrompt.BIOMETRIC_ERROR_CANCELED), eq(0) /* vendorCode */); - verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(eq(TEST_REQUEST_ID)); verify(mReceiver2, never()).onError(anyInt(), anyInt(), anyInt()); } @@ -745,7 +746,7 @@ public class BiometricServiceTest { eq(BiometricConstants.BIOMETRIC_ERROR_CANCELED), eq(0 /* vendorCode */)); // Dialog is hidden immediately - verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(eq(TEST_REQUEST_ID)); // Auth session is over assertNull(mBiometricService.mAuthSession); } @@ -773,7 +774,8 @@ public class BiometricServiceTest { eq(TYPE_FINGERPRINT), eq(BiometricConstants.BIOMETRIC_ERROR_UNABLE_TO_PROCESS), eq(0 /* vendorCode */)); - verify(mBiometricService.mStatusBarService, never()).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService, never()) + .hideAuthenticationDialog(eq(TEST_REQUEST_ID)); verify(mReceiver1, never()).onError(anyInt(), anyInt(), anyInt()); // SystemUI animation completed, client is notified, auth session is over @@ -1152,7 +1154,7 @@ public class BiometricServiceTest { verify(mReceiver1).onError(eq(TYPE_FINGERPRINT), eq(BiometricConstants.BIOMETRIC_ERROR_CANCELED), eq(0 /* vendorCode */)); - verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(); + verify(mBiometricService.mStatusBarService).hideAuthenticationDialog(eq(TEST_REQUEST_ID)); } @Test