From b055139948fe2a2071c03a6da33b8531223dcf73 Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Fri, 30 Jul 2021 15:16:48 -0700 Subject: [PATCH] Ensure authentication lifecycle ends for callers Bug: 194970282 Bug: 193089985 In coex, we introduced the capability to temporarily store face auth results if fingerprint auth was still in progress. However, upon successful fingerprint auth, we forgot to notify the caller that fingerprint lifecycle is ended. Since the purpose of coex is to distill auth callbacks for keyguard, we should send ERROR_CANCELED for face, since fingerprint sends auth success. Test: atest CoexCoordinatorTest Test: manual, see new "Removing from queue, canceling, and finishing" log. Keyguard face auth is no longer wedged Change-Id: If809a9338f7a96bb1f039626bff8efacbcec509e --- .../sensors/AuthenticationClient.java | 28 ++++++++++++++++++- .../biometrics/sensors/CoexCoordinator.java | 17 +++++++++-- .../sensors/CoexCoordinatorTest.java | 23 +++++++++------ 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/biometrics/sensors/AuthenticationClient.java b/services/core/java/com/android/server/biometrics/sensors/AuthenticationClient.java index f4327e8a104dc..705b3aa123917 100644 --- a/services/core/java/com/android/server/biometrics/sensors/AuthenticationClient.java +++ b/services/core/java/com/android/server/biometrics/sensors/AuthenticationClient.java @@ -180,7 +180,8 @@ public abstract class AuthenticationClient extends AcquisitionClient + ", isBP: " + isBiometricPrompt() + ", listener: " + listener + ", requireConfirmation: " + mRequireConfirmation - + ", user: " + getTargetUserId()); + + ", user: " + getTargetUserId() + + ", clientMonitor: " + toString()); final PerformanceTracker pm = PerformanceTracker.getInstanceForSensorId(getSensorId()); if (isCryptoOperation()) { @@ -304,6 +305,11 @@ public abstract class AuthenticationClient extends AcquisitionClient public void handleLifecycleAfterAuth() { AuthenticationClient.this.handleLifecycleAfterAuth(true /* authenticated */); } + + @Override + public void sendAuthenticationCanceled() { + sendCancelOnly(listener); + } }); } else { // Allow system-defined limit of number of attempts before giving up @@ -338,10 +344,30 @@ public abstract class AuthenticationClient extends AcquisitionClient public void handleLifecycleAfterAuth() { AuthenticationClient.this.handleLifecycleAfterAuth(false /* authenticated */); } + + @Override + public void sendAuthenticationCanceled() { + sendCancelOnly(listener); + } }); } } + private void sendCancelOnly(@Nullable ClientMonitorCallbackConverter listener) { + if (listener == null) { + Slog.e(TAG, "Unable to sendAuthenticationCanceled, listener null"); + return; + } + try { + listener.onError(getSensorId(), + getCookie(), + BiometricConstants.BIOMETRIC_ERROR_CANCELED, + 0 /* vendorCode */); + } catch (RemoteException e) { + Slog.e(TAG, "Remote exception", e); + } + } + @Override public void onAcquired(int acquiredInfo, int vendorCode) { super.onAcquired(acquiredInfo, vendorCode); diff --git a/services/core/java/com/android/server/biometrics/sensors/CoexCoordinator.java b/services/core/java/com/android/server/biometrics/sensors/CoexCoordinator.java index a15ecada3caed..6d32fdebe5826 100644 --- a/services/core/java/com/android/server/biometrics/sensors/CoexCoordinator.java +++ b/services/core/java/com/android/server/biometrics/sensors/CoexCoordinator.java @@ -73,6 +73,11 @@ public class CoexCoordinator { * from scheduler if auth was successful). */ void handleLifecycleAfterAuth(); + + /** + * Requests the owner to notify the caller that authentication was canceled. + */ + void sendAuthenticationCanceled(); } private static CoexCoordinator sInstance; @@ -356,7 +361,11 @@ public class CoexCoordinator { private SuccessfulAuth popSuccessfulFaceAuthIfExists(long currentTimeMillis) { for (SuccessfulAuth auth : mSuccessfulAuths) { if (currentTimeMillis - auth.mAuthTimestamp >= SUCCESSFUL_AUTH_VALID_DURATION_MS) { - Slog.d(TAG, "Removing stale auth: " + auth); + // TODO(b/193089985): This removes the auth but does not notify the client with + // an appropriate lifecycle event (such as ERROR_CANCELED), and violates the + // API contract. However, this might be OK for now since the validity duration + // is way longer than the time it takes to auth with fingerprint. + Slog.e(TAG, "Removing stale auth: " + auth); mSuccessfulAuths.remove(auth); } else if (auth.mSensorType == SENSOR_TYPE_FACE) { mSuccessfulAuths.remove(auth); @@ -367,9 +376,13 @@ public class CoexCoordinator { } private void removeAndFinishAllFaceFromQueue() { + // Note that these auth are all successful, but have never notified the client (e.g. + // keyguard). To comply with the authentication lifecycle, we must notify the client that + // auth is "done". The safest thing to do is to send ERROR_CANCELED. for (SuccessfulAuth auth : mSuccessfulAuths) { if (auth.mSensorType == SENSOR_TYPE_FACE) { - Slog.d(TAG, "Removing from queue and finishing: " + auth); + Slog.d(TAG, "Removing from queue, canceling, and finishing: " + auth); + auth.mCallback.sendAuthenticationCanceled(); auth.mCallback.handleLifecycleAfterAuth(); mSuccessfulAuths.remove(auth); } diff --git a/services/tests/servicestests/src/com/android/server/biometrics/sensors/CoexCoordinatorTest.java b/services/tests/servicestests/src/com/android/server/biometrics/sensors/CoexCoordinatorTest.java index 7f5f3c2df0777..1263f7b84db11 100644 --- a/services/tests/servicestests/src/com/android/server/biometrics/sensors/CoexCoordinatorTest.java +++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/CoexCoordinatorTest.java @@ -255,13 +255,16 @@ public class CoexCoordinatorTest { mCoexCoordinator.addAuthenticationClient(SENSOR_TYPE_FACE, faceClient); mCoexCoordinator.addAuthenticationClient(SENSOR_TYPE_UDFPS, udfpsClient); + // For easier reading + final CoexCoordinator.Callback faceCallback = mCallback; + mCoexCoordinator.onAuthenticationSucceeded(0 /* currentTimeMillis */, faceClient, - mCallback); - verify(mCallback, never()).sendHapticFeedback(); - verify(mCallback, never()).sendAuthenticationResult(anyBoolean()); + faceCallback); + verify(faceCallback, never()).sendHapticFeedback(); + verify(faceCallback, never()).sendAuthenticationResult(anyBoolean()); // CoexCoordinator requests the system to hold onto this AuthenticationClient until // UDFPS result is known - verify(mCallback, never()).handleLifecycleAfterAuth(); + verify(faceCallback, never()).handleLifecycleAfterAuth(); // Reset the mock CoexCoordinator.Callback udfpsCallback = mock(CoexCoordinator.Callback.class); @@ -274,6 +277,8 @@ public class CoexCoordinatorTest { verify(udfpsCallback).sendAuthenticationResult(true /* addAuthTokenIfStrong */); verify(udfpsCallback).handleLifecycleAfterAuth(); + verify(faceCallback).sendAuthenticationCanceled(); + assertTrue(mCoexCoordinator.mSuccessfulAuths.isEmpty()); } else { mCoexCoordinator.onAuthenticationRejected(udfpsRejectedAfterMs, udfpsClient, @@ -281,16 +286,16 @@ public class CoexCoordinatorTest { if (udfpsRejectedAfterMs <= CoexCoordinator.SUCCESSFUL_AUTH_VALID_DURATION_MS) { verify(udfpsCallback, never()).sendHapticFeedback(); - verify(mCallback).sendHapticFeedback(); - verify(mCallback).sendAuthenticationResult(eq(true) /* addAuthTokenIfStrong */); - verify(mCallback).handleLifecycleAfterAuth(); + verify(faceCallback).sendHapticFeedback(); + verify(faceCallback).sendAuthenticationResult(eq(true) /* addAuthTokenIfStrong */); + verify(faceCallback).handleLifecycleAfterAuth(); assertTrue(mCoexCoordinator.mSuccessfulAuths.isEmpty()); } else { assertTrue(mCoexCoordinator.mSuccessfulAuths.isEmpty()); - verify(mCallback, never()).sendHapticFeedback(); - verify(mCallback, never()).sendAuthenticationResult(anyBoolean()); + verify(faceCallback, never()).sendHapticFeedback(); + verify(faceCallback, never()).sendAuthenticationResult(anyBoolean()); verify(udfpsCallback).sendHapticFeedback(); verify(udfpsCallback)