[Autofill Auth] Fixing crashes caused by cancelling WebAuthn dialog

This CL maintains the reference to the C++ instance, and handles the
null response case better.

Bug: 949269
Change-Id: I3bb453286d8a1f8c8f087787235a664570e2c259
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2200045Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Commit-Queue: Manas Verma <manasverma@google.com>
Cr-Commit-Position: refs/heads/master@{#772413}

chrome/android/BUILD.gn

@@ -896,6 +896,7 @@ android_library("chrome_test_java") {
     "$google_play_services_package:google_play_services_tasks_java",
     "//base:base_java",
     "//base:base_java_test_support",
+    "//base:jni_java",
     "//base/test:test_support_java",
     "//chrome/android:app_hooks_java",
     "//chrome/android:chrome_java",

chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorImpl.java

@@ -8,6 +8,7 @@ import android.annotation.TargetApi;
 import android.content.Context;
 import android.os.Build;
 
+import org.chromium.base.ContextUtils;
 import org.chromium.base.PackageUtils;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.NativeMethods;
@@ -17,11 +18,8 @@ import org.chromium.blink.mojom.GetAssertionAuthenticatorResponse;
 import org.chromium.blink.mojom.MakeCredentialAuthenticatorResponse;
 import org.chromium.blink.mojom.PublicKeyCredentialCreationOptions;
 import org.chromium.blink.mojom.PublicKeyCredentialRequestOptions;
-import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.content_public.browser.RenderFrameHost;
-import org.chromium.content_public.browser.WebContents;
-import org.chromium.content_public.browser.WebContentsStatics;
 import org.chromium.mojo.system.MojoException;
 import org.chromium.url.Origin;
 
@@ -37,7 +35,6 @@ import java.util.Queue;
  */
 public class AuthenticatorImpl extends HandlerResponseCallback implements Authenticator {
     private final RenderFrameHost mRenderFrameHost;
-    private final WebContents mWebContents;
 
     private static final String GMSCORE_PACKAGE_NAME = "com.google.android.gms";
 
@@ -71,7 +68,6 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
         assert renderFrameHost != null;
         mRenderFrameHost = renderFrameHost;
         mOrigin = mRenderFrameHost.getLastCommittedOrigin();
-        mWebContents = WebContentsStatics.fromRenderFrameHost(renderFrameHost);
     }
 
     private AuthenticatorImpl(
@@ -105,7 +101,7 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
         }
 
         mMakeCredentialCallback = callback;
-        Context context = ChromeActivity.fromWebContents(mWebContents);
+        Context context = ContextUtils.getApplicationContext();
         if (PackageUtils.getPackageVersion(context, GMSCORE_PACKAGE_NAME)
                 < Fido2ApiHandler.GMSCORE_MIN_VERSION) {
             onError(AuthenticatorStatus.NOT_IMPLEMENTED);
@@ -127,7 +123,7 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
                 (status, response)
                         -> AuthenticatorImplJni.get().invokeMakeCredentialResponse(
                                 mNativeInternalAuthenticatorAndroid, status.intValue(),
-                                response == null ? ByteBuffer.allocate(0) : response.serialize()));
+                                response == null ? null : response.serialize()));
     }
 
     @Override
@@ -139,7 +135,8 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
         }
 
         mGetAssertionCallback = callback;
-        Context context = ChromeActivity.fromWebContents(mWebContents);
+        Context context = ContextUtils.getApplicationContext();
+
         if (PackageUtils.getPackageVersion(context, GMSCORE_PACKAGE_NAME)
                 < Fido2ApiHandler.GMSCORE_MIN_VERSION) {
             onError(AuthenticatorStatus.NOT_IMPLEMENTED);
@@ -161,14 +158,14 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
                 (status, response)
                         -> AuthenticatorImplJni.get().invokeGetAssertionResponse(
                                 mNativeInternalAuthenticatorAndroid, status.intValue(),
-                                response == null ? ByteBuffer.allocate(0) : response.serialize()));
+                                response == null ? null : response.serialize()));
     }
 
     @Override
     @TargetApi(Build.VERSION_CODES.N)
     public void isUserVerifyingPlatformAuthenticatorAvailable(
             IsUserVerifyingPlatformAuthenticatorAvailableResponse callback) {
-        Context context = ChromeActivity.fromWebContents(mWebContents);
+        Context context = ContextUtils.getApplicationContext();
         // ChromeActivity could be null.
         if (context == null) {
             callback.call(false);
@@ -263,7 +260,6 @@ public class AuthenticatorImpl extends HandlerResponseCallback implements Authen
         mIsOperationPending = false;
         mMakeCredentialCallback = null;
         mGetAssertionCallback = null;
-        mNativeInternalAuthenticatorAndroid = null;
     }
 
     @Override

chrome/android/javatests/src/org/chromium/chrome/browser/webauth/Fido2ApiTestHelper.java

@@ -20,6 +20,7 @@ import org.junit.Assert;
 
 import org.chromium.blink.mojom.AuthenticatorAttachment;
 import org.chromium.blink.mojom.AuthenticatorSelectionCriteria;
+import org.chromium.blink.mojom.CableAuthentication;
 import org.chromium.blink.mojom.GetAssertionAuthenticatorResponse;
 import org.chromium.blink.mojom.MakeCredentialAuthenticatorResponse;
 import org.chromium.blink.mojom.PublicKeyCredentialCreationOptions;
@@ -283,6 +284,8 @@ public class Fido2ApiTestHelper {
         descriptor.id = new byte[] {8, 7, 6};
         descriptor.transports = new int[] {0};
         options.allowCredentials = new PublicKeyCredentialDescriptor[] {descriptor};
+
+        options.cableAuthenticationData = new CableAuthentication[] {};
         return options;
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/webauth/Fido2CredentialRequestTest.java

@@ -20,6 +20,7 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.Callback;
 import org.chromium.base.ContextUtils;
@@ -32,6 +33,7 @@ import org.chromium.base.test.params.ParameterSet;
 import org.chromium.base.test.params.ParameterizedRunner;
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
+import org.chromium.base.test.util.JniMocker;
 import org.chromium.blink.mojom.AuthenticatorStatus;
 import org.chromium.blink.mojom.GetAssertionAuthenticatorResponse;
 import org.chromium.blink.mojom.MakeCredentialAuthenticatorResponse;
@@ -43,8 +45,10 @@ import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4RunnerDelegate;
+import org.chromium.content_public.browser.RenderFrameHost;
 import org.chromium.content_public.browser.test.mock.MockRenderFrameHost;
 import org.chromium.content_public.browser.test.mock.MockWebContents;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.content_public.common.ContentSwitches;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.ServerCertificate;
@@ -53,6 +57,7 @@ import org.chromium.ui.base.WindowAndroid;
 import org.chromium.url.GURL;
 import org.chromium.url.Origin;
 
+import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.List;
 
@@ -70,12 +75,16 @@ public class Fido2CredentialRequestTest {
     @Rule
     public ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
             new ChromeActivityTestRule<>(ChromeActivity.class);
+    @Rule
+    public JniMocker mocker = new JniMocker();
 
     private MockActivityWindowAndroid mWindowAndroid;
     private EmbeddedTestServer mTestServer;
     private String mUrl;
     private MockAuthenticatorRenderFrameHost mFrameHost;
     private Origin mOrigin;
+    private AuthenticatorImpl.Natives mTestAuthenticatorImplJni;
+    private AuthenticatorImpl mAuthenticatorImpl;
     private Fido2CredentialRequest mRequest;
     private PublicKeyCredentialCreationOptions mCreationOptions;
     private PublicKeyCredentialRequestOptions mRequestOptions;
@@ -256,6 +265,62 @@ public class Fido2CredentialRequestTest {
         }
     }
 
+    private static class MockFido2ApiHandler extends Fido2ApiHandler {
+        private Fido2CredentialRequest mRequest;
+
+        MockFido2ApiHandler(Fido2CredentialRequest request) {
+            mRequest = request;
+        }
+
+        @Override
+        protected void makeCredential(PublicKeyCredentialCreationOptions options,
+                RenderFrameHost frameHost, Origin origin, HandlerResponseCallback callback) {
+            mRequest.handleMakeCredentialRequest(options, frameHost, origin, callback);
+        }
+
+        @Override
+        protected void getAssertion(PublicKeyCredentialRequestOptions options,
+                RenderFrameHost frameHost, Origin origin, HandlerResponseCallback callback) {
+            mRequest.handleGetAssertionRequest(options, frameHost, origin, callback);
+        }
+
+        @Override
+        protected void isUserVerifyingPlatformAuthenticatorAvailable(
+                RenderFrameHost frameHost, HandlerResponseCallback callback) {
+            mRequest.handleIsUserVerifyingPlatformAuthenticatorAvailableRequest(
+                    frameHost, callback);
+        }
+    }
+
+    private static class TestAuthenticatorImplJni implements AuthenticatorImpl.Natives {
+        private AuthenticatorCallback mCallback;
+
+        TestAuthenticatorImplJni(AuthenticatorCallback callback) {
+            mCallback = callback;
+        }
+
+        @Override
+        public void invokeMakeCredentialResponse(
+                long nativeInternalAuthenticatorAndroid, int status, ByteBuffer byteBuffer) {
+            mCallback.onRegisterResponse(status,
+                    byteBuffer == null
+                            ? null
+                            : MakeCredentialAuthenticatorResponse.deserialize(byteBuffer));
+        }
+
+        @Override
+        public void invokeGetAssertionResponse(
+                long nativeInternalAuthenticatorAndroid, int status, ByteBuffer byteBuffer) {
+            mCallback.onSignResponse(status,
+                    byteBuffer == null ? null
+                                       : GetAssertionAuthenticatorResponse.deserialize(byteBuffer));
+        }
+
+        @Override
+        public void invokeIsUserVerifyingPlatformAuthenticatorAvailableResponse(
+                long nativeInternalAuthenticatorAndroid, boolean isUVPAA) {}
+    }
+
     private static class MockOrigin extends Origin {
         private GURL mUrl;
 
@@ -321,6 +386,13 @@ public class Fido2CredentialRequestTest {
         mOrigin = mFrameHost.getLastCommittedOrigin();
         mRequest = new Fido2CredentialRequest();
         mRequest.setWebContentsForTesting(new MockWebContents());
+        Fido2ApiHandler.overrideInstanceForTesting(new MockFido2ApiHandler(mRequest));
+
+        MockitoAnnotations.initMocks(this);
+        mTestAuthenticatorImplJni = new TestAuthenticatorImplJni(mCallback);
+        mocker.mock(AuthenticatorImplJni.TEST_HOOKS, mTestAuthenticatorImplJni);
+
+        mAuthenticatorImpl = AuthenticatorImpl.create(0, mFrameHost);
         mCreationOptions = Fido2ApiTestHelper.createDefaultMakeCredentialOptions();
         mRequestOptions = Fido2ApiTestHelper.createDefaultGetAssertionOptions();
         ThreadUtils.runOnUiThreadBlocking(()
@@ -541,6 +613,68 @@ public class Fido2CredentialRequestTest {
         Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
     }
 
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplMakeCredential_success() {
+        mWindowAndroid.setResponseIntent(Fido2ApiTestHelper.createSuccessfulMakeCredentialIntent());
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.makeCredential(mCreationOptions,
+                    (status, response) -> mCallback.onRegisterResponse(status, response));
+        });
+
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.SUCCESS));
+        Fido2ApiTestHelper.validateMakeCredentialResponse(mCallback.getMakeCredentialResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplMakeCredential_resultCanceled() {
+        mWindowAndroid.setResultCode(Activity.RESULT_CANCELED);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.makeCredential(mCreationOptions,
+                    (status, response) -> mCallback.onRegisterResponse(status, response));
+        });
+
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(
+                mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.NOT_ALLOWED_ERROR));
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplMakeCredentialBridge_success() {
+        mWindowAndroid.setResponseIntent(Fido2ApiTestHelper.createSuccessfulMakeCredentialIntent());
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.makeCredentialBridge(mCreationOptions.serialize());
+        });
+
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.SUCCESS));
+        Fido2ApiTestHelper.validateMakeCredentialResponse(mCallback.getMakeCredentialResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplMakeCredentialBridge_resultCanceled() {
+        mWindowAndroid.setResultCode(Activity.RESULT_CANCELED);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.makeCredentialBridge(mCreationOptions.serialize());
+        });
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(
+                mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.NOT_ALLOWED_ERROR));
+        Assert.assertNull(mCallback.getMakeCredentialResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
     @Test
     @SmallTest
     public void testGetAssertionWithoutUvmRequested_success() {
@@ -690,6 +824,72 @@ public class Fido2CredentialRequestTest {
         Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
     }
 
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplGetAssertionWithUvmRequestedWithUvmResponded_success() {
+        mWindowAndroid.setResponseIntent(
+                Fido2ApiTestHelper.createSuccessfulGetAssertionIntentWithUvm());
+        mRequestOptions.userVerificationMethods = true;
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.getAssertion(mRequestOptions,
+                    (status, response) -> mCallback.onSignResponse(status, response));
+        });
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.SUCCESS));
+        Fido2ApiTestHelper.validateGetAssertionResponse(mCallback.getGetAssertionResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplGetAssertion_resultCanceled() {
+        mWindowAndroid.setResultCode(Activity.RESULT_CANCELED);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.getAssertion(mRequestOptions,
+                    (status, response) -> mCallback.onSignResponse(status, response));
+        });
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(
+                mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.NOT_ALLOWED_ERROR));
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplGetAssertionBridgeWithUvmRequestedWithUvmResponded_success() {
+        mWindowAndroid.setResponseIntent(
+                Fido2ApiTestHelper.createSuccessfulGetAssertionIntentWithUvm());
+        mRequestOptions.userVerificationMethods = true;
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.getAssertionBridge(mRequestOptions.serialize());
+        });
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.SUCCESS));
+        Fido2ApiTestHelper.validateGetAssertionResponse(mCallback.getGetAssertionResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
+    @Test
+    @SmallTest
+    public void testAuthenticatorImplGetAssertionBridge_resultCanceled() {
+        mWindowAndroid.setResultCode(Activity.RESULT_CANCELED);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mRequest.setActivityWindowForTesting(mWindowAndroid);
+            mAuthenticatorImpl.getAssertionBridge(mRequestOptions.serialize());
+        });
+
+        mCallback.blockUntilCalled();
+        Assert.assertEquals(
+                mCallback.getStatus(), Integer.valueOf(AuthenticatorStatus.NOT_ALLOWED_ERROR));
+        Assert.assertNull(mCallback.getGetAssertionResponse());
+        Fido2ApiTestHelper.verifyRespondedBeforeTimeout(mStartTimeMs);
+    }
+
     @Test
     @SmallTest
     public void testMakeCredential_attestationNone() {

chrome/browser/autofill/android/internal_authenticator_android.cc

@@ -123,10 +123,14 @@ void InternalAuthenticatorAndroid::InvokeMakeCredentialResponse(
     jint status,
     const base::android::JavaParamRef<jobject>& byte_buffer) {
   blink::mojom::MakeCredentialAuthenticatorResponsePtr response;
+
+  // |byte_buffer| may be null if authentication failed.
+  if (byte_buffer) {
     blink::mojom::MakeCredentialAuthenticatorResponse::Deserialize(
         std::move(payments::android::JavaByteBufferToNativeByteVector(
             env, byte_buffer)),
         &response);
+  }
 
   std::move(make_credential_response_callback_)
       .Run(static_cast<blink::mojom::AuthenticatorStatus>(status),
@@ -138,10 +142,14 @@ void InternalAuthenticatorAndroid::InvokeGetAssertionResponse(
     jint status,
     const base::android::JavaParamRef<jobject>& byte_buffer) {
   blink::mojom::GetAssertionAuthenticatorResponsePtr response;
+
+  // |byte_buffer| may be null if authentication failed.
+  if (byte_buffer) {
     blink::mojom::GetAssertionAuthenticatorResponse::Deserialize(
         std::move(payments::android::JavaByteBufferToNativeByteVector(
             env, byte_buffer)),
         &response);
+  }
 
   std::move(get_assertion_response_callback_)
       .Run(static_cast<blink::mojom::AuthenticatorStatus>(status),