From 82d8e2eaba5b23c413c681f39eed546f2495b884 Mon Sep 17 00:00:00 2001
From: Joshua Mccloskey <joshmccloskey@google.com>
Date: Tue, 1 Mar 2022 02:13:36 +0000
Subject: RemovalClient handles null authenticator

Test: atest FaceREmovalClientTest
Test: called onRemoved({}) in FaceHAL and system_server doesn't crash
Bug: 219081263

This change fixes an issue which was presented by an AIDL workaround.
There is currently no API to indicate removeEnrollments() failed. So
onRemoved({}) is called instead. The framework does not correctly
handle this case, this change adds the necessary logic.

Change-Id: Iaf907f7afc00e5b897ac73947d0b5d5576621bdf
---
 .../sensors/ClientMonitorCallbackConverter.java    |   3 +-
 .../server/biometrics/sensors/RemovalClient.java   |  19 ++++
 .../sensors/face/aidl/FaceRemovalClientTest.java   | 124 +++++++++++++++++++++
 3 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 services/tests/servicestests/src/com/android/server/biometrics/sensors/face/aidl/FaceRemovalClientTest.java

diff --git a/services/core/java/com/android/server/biometrics/sensors/ClientMonitorCallbackConverter.java b/services/core/java/com/android/server/biometrics/sensors/ClientMonitorCallbackConverter.java
index f1c786b4977c..46d863d7aaec 100644
--- a/services/core/java/com/android/server/biometrics/sensors/ClientMonitorCallbackConverter.java
+++ b/services/core/java/com/android/server/biometrics/sensors/ClientMonitorCallbackConverter.java
@@ -123,7 +123,8 @@ public class ClientMonitorCallbackConverter {
         }
     }
 
-    void onRemoved(BiometricAuthenticator.Identifier identifier, int remaining)
+    /** Called when a user has been removed. */
+    public void onRemoved(BiometricAuthenticator.Identifier identifier, int remaining)
             throws RemoteException {
         if (mFaceServiceReceiver != null) {
             mFaceServiceReceiver.onRemoved((Face) identifier, remaining);
diff --git a/services/core/java/com/android/server/biometrics/sensors/RemovalClient.java b/services/core/java/com/android/server/biometrics/sensors/RemovalClient.java
index 07ce841a7cac..e0d519469e32 100644
--- a/services/core/java/com/android/server/biometrics/sensors/RemovalClient.java
+++ b/services/core/java/com/android/server/biometrics/sensors/RemovalClient.java
@@ -19,6 +19,7 @@ package com.android.server.biometrics.sensors;
 import android.annotation.NonNull;
 import android.content.Context;
 import android.hardware.biometrics.BiometricAuthenticator;
+import android.hardware.biometrics.BiometricConstants;
 import android.os.IBinder;
 import android.os.RemoteException;
 import android.util.Slog;
@@ -71,6 +72,24 @@ public abstract class RemovalClient<S extends BiometricAuthenticator.Identifier,
 
     @Override
     public void onRemoved(@NonNull BiometricAuthenticator.Identifier identifier, int remaining) {
+        // This happens when we have failed to remove a biometric.
+        if (identifier == null) {
+            Slog.e(TAG, "identifier was null, skipping onRemove()");
+            try {
+                if (getListener() != null) {
+                    getListener().onError(getSensorId(), getCookie(),
+                            BiometricConstants.BIOMETRIC_ERROR_UNABLE_TO_REMOVE,
+                            0 /* vendorCode */);
+                } else {
+                    Slog.e(TAG, "Error, listener was null, not sending onError callback");
+                }
+            } catch (RemoteException e) {
+                Slog.w(TAG, "Failed to send error to client for onRemoved", e);
+            }
+            mCallback.onClientFinished(this, false /* success */);
+            return;
+        }
+
         Slog.d(TAG, "onRemoved: " + identifier.getBiometricId() + " remaining: " + remaining);
         mBiometricUtils.removeBiometricForUser(getContext(), getTargetUserId(),
                 identifier.getBiometricId());
diff --git a/services/tests/servicestests/src/com/android/server/biometrics/sensors/face/aidl/FaceRemovalClientTest.java b/services/tests/servicestests/src/com/android/server/biometrics/sensors/face/aidl/FaceRemovalClientTest.java
new file mode 100644
index 000000000000..76a5accc5fe9
--- /dev/null
+++ b/services/tests/servicestests/src/com/android/server/biometrics/sensors/face/aidl/FaceRemovalClientTest.java
@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.biometrics.sensors.face.aidl;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.hardware.biometrics.BiometricAuthenticator;
+import android.hardware.biometrics.BiometricConstants;
+import android.hardware.biometrics.face.ISession;
+import android.hardware.face.Face;
+import android.os.IBinder;
+import android.os.RemoteException;
+import android.platform.test.annotations.Presubmit;
+import android.testing.TestableContext;
+
+import androidx.test.filters.SmallTest;
+import androidx.test.platform.app.InstrumentationRegistry;
+
+import com.android.server.biometrics.log.BiometricContext;
+import com.android.server.biometrics.log.BiometricLogger;
+import com.android.server.biometrics.sensors.BiometricUtils;
+import com.android.server.biometrics.sensors.ClientMonitorCallback;
+import com.android.server.biometrics.sensors.ClientMonitorCallbackConverter;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+
+import java.util.HashMap;
+import java.util.Map;
+
+@Presubmit
+@SmallTest
+public class FaceRemovalClientTest {
+
+    private static final int USER_ID = 12;
+
+    @Rule
+    public final TestableContext mContext = new TestableContext(
+            InstrumentationRegistry.getInstrumentation().getTargetContext(), null);
+    @Rule
+    public final MockitoRule mockito = MockitoJUnit.rule();
+
+    @Mock
+    private ISession mHal;
+    @Mock
+    private IBinder mToken;
+    @Mock
+    private ClientMonitorCallbackConverter mClientMonitorCallbackConverter;
+    @Mock
+    private BiometricLogger mBiometricLogger;
+    @Mock
+    private BiometricContext mBiometricContext;
+    @Mock
+    private ClientMonitorCallback mCallback;
+    @Mock
+    private Sensor.HalSessionCallback mHalSessionCallback;
+    @Mock
+    private BiometricUtils<Face> mUtils;
+    @Mock
+    private BiometricAuthenticator.Identifier mIdentifier;
+    private Map<Integer, Long> mAuthenticatorIds = new HashMap<Integer, Long>();
+
+    @Before
+    public void setup() {
+        when(mBiometricContext.updateContext(any(), anyBoolean())).thenAnswer(
+                i -> i.getArgument(0));
+    }
+
+    @Test
+    public void testFaceRemovalClient() throws RemoteException {
+        final int authenticatorId = 1;
+        int[] authenticatorIds = new int[]{authenticatorId};
+        final FaceRemovalClient client = createClient(1, authenticatorIds);
+        when(mIdentifier.getBiometricId()).thenReturn(authenticatorId);
+        client.start(mCallback);
+        verify(mHal).removeEnrollments(authenticatorIds);
+        client.onRemoved(mIdentifier, 0 /* remaining */);
+        verify(mClientMonitorCallbackConverter).onRemoved(
+                eq(mIdentifier) /* identifier */, eq(0) /* remaining */);
+        verify(mCallback).onClientFinished(client, true);
+    }
+
+    @Test
+    public void clientSendsErrorWhenHALFailsToRemoveEnrollment() throws RemoteException {
+        final FaceRemovalClient client = createClient(1, new int[0]);
+        client.start(mCallback);
+        client.onRemoved(null, 0 /* remaining */);
+        verify(mClientMonitorCallbackConverter).onError(eq(5) /* sensorId */, anyInt(),
+                eq(BiometricConstants.BIOMETRIC_ERROR_UNABLE_TO_REMOVE), eq(0) /* vendorCode*/);
+        verify(mCallback).onClientFinished(client, false);
+    }
+
+    private FaceRemovalClient createClient(int version, int[] biometricIds) throws RemoteException {
+        when(mHal.getInterfaceVersion()).thenReturn(version);
+        final AidlSession aidl = new AidlSession(version, mHal, USER_ID, mHalSessionCallback);
+        return new FaceRemovalClient(mContext, () -> aidl, mToken,
+                mClientMonitorCallbackConverter, biometricIds, USER_ID,
+                "own-it", mUtils /* utils */, 5 /* sensorId */, mBiometricLogger, mBiometricContext,
+                mAuthenticatorIds);
+    }
+}
-- 
cgit v1.2.3-59-g8ed1b