[webauthn] Clean up adjusted_timeout

Remove adjusted_timeout from the renderer - browser pipe and use the
raw timeout on android instead, adjusting it on java. Add unit tests for
the adjustment.

This aligns the android and browser implementation and cleans up
leftover implementation from crrev.com/c/2084725

Fixed: 976428
Change-Id: I974dc8ec8852ba2216a3cb615c53b25c802f7b42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128529
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754984}

chrome/android/chrome_test_java_sources.gni

@@ -553,6 +553,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/webauth/AuthenticatorTest.java",
   "javatests/src/org/chromium/chrome/browser/webauth/Fido2ApiTestHelper.java",
   "javatests/src/org/chromium/chrome/browser/webauth/Fido2CredentialRequestTest.java",
+  "javatests/src/org/chromium/chrome/browser/webauth/Fido2HelperTest.java",
   "javatests/src/org/chromium/chrome/browser/webshare/WebShareTest.java",
   "javatests/src/org/chromium/chrome/browser/widget/ScrimTest.java",
   "javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java",

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

@@ -45,7 +45,6 @@ import org.chromium.ui.base.WindowAndroid;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.security.NoSuchAlgorithmException;
-import java.util.concurrent.TimeUnit;
 
 /**
  * Uses the Google Play Services Fido2 APIs.
@@ -60,7 +59,6 @@ public class Fido2CredentialRequest implements WindowAndroid.IntentCallback {
     private ActivityWindowAndroid mWindow;
     private @RequestStatus int mRequestStatus;
     private boolean mAppIdExtensionUsed;
-    private long mTimeoutMs;
     private long mStartTimeMs;
 
     @IntDef({
@@ -148,7 +146,6 @@ public class Fido2CredentialRequest implements WindowAndroid.IntentCallback {
         }
 
         mRequestStatus = REGISTER_REQUEST;
-        mTimeoutMs = TimeUnit.MICROSECONDS.toMillis(options.adjustedTimeout.microseconds);
 
         if (!initFido2ApiClient()) {
             Log.e(TAG, "Google Play Services' Fido2PrivilegedApi is not available.");
@@ -190,7 +187,6 @@ public class Fido2CredentialRequest implements WindowAndroid.IntentCallback {
         }
 
         mRequestStatus = SIGN_REQUEST;
-        mTimeoutMs = TimeUnit.MICROSECONDS.toMillis(options.adjustedTimeout.microseconds);
 
         if (!initFido2ApiClient()) {
             Log.e(TAG, "Google Play Services' Fido2PrivilegedApi is not available.");

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

@@ -35,6 +35,7 @@ import org.chromium.blink.mojom.GetAssertionAuthenticatorResponse;
 import org.chromium.blink.mojom.MakeCredentialAuthenticatorResponse;
 import org.chromium.blink.mojom.PublicKeyCredentialRequestOptions;
 import org.chromium.blink.mojom.UvmEntry;
+import org.chromium.mojo_base.mojom.TimeDelta;
 
 import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
@@ -57,6 +58,8 @@ public final class Fido2Helper {
     private static final String CREDENTIAL_EXISTS_ERROR_MSG =
             "One of the excluded credentials exists on the local device";
     private static final String LOW_LEVEL_ERROR_MSG = "Low level error 0x6a80";
+    static final double MIN_TIMEOUT_SECONDS = 10;
+    static final double MAX_TIMEOUT_SECONDS = 600;
 
     /**
      * Converts mojo options to gmscore options.
@@ -95,9 +98,6 @@ public final class Fido2Helper {
         List<PublicKeyCredentialDescriptor> excludeCredentials =
                 convertCredentialDescriptor(options.excludeCredentials);
 
-        double timeoutSeconds =
-                TimeUnit.MICROSECONDS.toSeconds(options.adjustedTimeout.microseconds);
-
         AuthenticatorSelectionCriteria selection =
                 convertSelectionCriteria(options.authenticatorSelection);
 
@@ -110,7 +110,7 @@ public final class Fido2Helper {
                         .setUser(user)
                         .setChallenge(options.challenge)
                         .setParameters(parameters)
-                        .setTimeoutSeconds(timeoutSeconds)
+                        .setTimeoutSeconds(adjustTimeout(options.timeout))
                         .setExcludeList(excludeCredentials)
                         .setAuthenticatorSelection(selection)
                         .setAttestationConveyancePreference(attestationPreference)
@@ -153,9 +153,6 @@ public final class Fido2Helper {
      */
     public static com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialRequestOptions
     toGetAssertionOptions(PublicKeyCredentialRequestOptions options) {
-        double timeoutSeconds =
-                TimeUnit.MICROSECONDS.toSeconds(options.adjustedTimeout.microseconds);
-
         List<PublicKeyCredentialDescriptor> allowCredentials =
                 convertCredentialDescriptor(options.allowCredentials);
 
@@ -175,7 +172,7 @@ public final class Fido2Helper {
                 new com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialRequestOptions
                         .Builder()
                         .setChallenge(options.challenge)
-                        .setTimeoutSeconds(timeoutSeconds)
+                        .setTimeoutSeconds(adjustTimeout(options.timeout))
                         .setRpId(options.relyingPartyId)
                         .setAllowList(allowCredentials)
                         /* TODO add back UserVerificationRequirement when the FIDO2 API supports it
@@ -377,4 +374,18 @@ public final class Fido2Helper {
                 return AttestationConveyancePreference.NONE;
         }
     }
+
+    /**
+     * Adjusts a timeout between a reasonable minimum and maximum.
+     *
+     * @param timeout The unadjusted timeout as specified by the website. May be null.
+     * @return The adjusted timeout in seconds.
+     */
+    private static double adjustTimeout(TimeDelta timeout) {
+        if (timeout == null) return MAX_TIMEOUT_SECONDS;
+
+        return Math.max(MIN_TIMEOUT_SECONDS,
+                Math.min(MAX_TIMEOUT_SECONDS,
+                        TimeUnit.MICROSECONDS.toSeconds(timeout.microseconds)));
+    }
 }

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

@@ -231,8 +231,8 @@ public class Fido2ApiTestHelper {
         options.user.displayName = "Avery A. Jones";
         options.user.icon = createUrl("https://usericon.example.test");
 
-        options.adjustedTimeout = new TimeDelta();
-        options.adjustedTimeout.microseconds = TimeUnit.MILLISECONDS.toMicros(TIMEOUT_MS);
+        options.timeout = new TimeDelta();
+        options.timeout.microseconds = TimeUnit.MILLISECONDS.toMicros(TIMEOUT_MS);
 
         PublicKeyCredentialParameters parameters = new PublicKeyCredentialParameters();
         parameters.algorithmIdentifier = -7;
@@ -274,8 +274,8 @@ public class Fido2ApiTestHelper {
             throws Exception {
         PublicKeyCredentialRequestOptions options = new PublicKeyCredentialRequestOptions();
         options.challenge = "climb a mountain".getBytes("UTF8");
-        options.adjustedTimeout = new TimeDelta();
-        options.adjustedTimeout.microseconds = TimeUnit.MILLISECONDS.toMicros(TIMEOUT_MS);
+        options.timeout = new TimeDelta();
+        options.timeout.microseconds = TimeUnit.MILLISECONDS.toMicros(TIMEOUT_MS);
         options.relyingPartyId = "subdomain.example.test";
 
         PublicKeyCredentialDescriptor descriptor = new PublicKeyCredentialDescriptor();

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

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.webauth;
+
+import android.support.test.filters.SmallTest;
+
+import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialCreationOptions;
+import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialRequestOptions;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.mojo_base.mojom.TimeDelta;
+
+/**
+ * Unit tests for Fido2Helper.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+public class Fido2HelperTest {
+    private org.chromium.blink.mojom.PublicKeyCredentialCreationOptions mCreationOptions;
+    private org.chromium.blink.mojom.PublicKeyCredentialRequestOptions mRequestOptions;
+
+    @Before
+    public void setUp() throws Exception {
+        mCreationOptions = Fido2ApiTestHelper.createDefaultMakeCredentialOptions();
+        mRequestOptions = Fido2ApiTestHelper.createDefaultGetAssertionOptions();
+    }
+
+    @Test
+    @SmallTest
+    public void testToMakeCredentialOptions_nullTimeout() throws Exception {
+        mCreationOptions.timeout = null;
+        PublicKeyCredentialCreationOptions options =
+                Fido2Helper.toMakeCredentialOptions(mCreationOptions);
+        Assert.assertEquals(Fido2Helper.MAX_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToMakeCredentialOptions_tinyTimeout() throws Exception {
+        mCreationOptions.timeout = new TimeDelta();
+        mCreationOptions.timeout.microseconds = 1;
+        PublicKeyCredentialCreationOptions options =
+                Fido2Helper.toMakeCredentialOptions(mCreationOptions);
+        Assert.assertEquals(Fido2Helper.MIN_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToMakeCredentialOptions_hugeTimeout() throws Exception {
+        mCreationOptions.timeout = new TimeDelta();
+        mCreationOptions.timeout.microseconds = 1_000_000L * 60 * 60; // One hour.
+        PublicKeyCredentialCreationOptions options =
+                Fido2Helper.toMakeCredentialOptions(mCreationOptions);
+        Assert.assertEquals(Fido2Helper.MAX_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToMakeCredentialOptions_reasonableTimeout() throws Exception {
+        mCreationOptions.timeout = new TimeDelta();
+        mCreationOptions.timeout.microseconds = 1_000_000L * 60 * 2; // Two minutes.
+        PublicKeyCredentialCreationOptions options =
+                Fido2Helper.toMakeCredentialOptions(mCreationOptions);
+        Assert.assertEquals(2 * 60, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToGetAssertionOptions_nullTimeout() throws Exception {
+        mRequestOptions.timeout = null;
+        PublicKeyCredentialRequestOptions options =
+                Fido2Helper.toGetAssertionOptions(mRequestOptions);
+        Assert.assertEquals(Fido2Helper.MAX_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToGetAssertionOptions_tinyTimeout() throws Exception {
+        mRequestOptions.timeout = new TimeDelta();
+        mRequestOptions.timeout.microseconds = 1;
+        PublicKeyCredentialRequestOptions options =
+                Fido2Helper.toGetAssertionOptions(mRequestOptions);
+        Assert.assertEquals(Fido2Helper.MIN_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToGetAssertionOptions_hugeTimeout() throws Exception {
+        mRequestOptions.timeout = new TimeDelta();
+        mRequestOptions.timeout.microseconds = 1_000_000L * 60 * 60; // One hour.
+        PublicKeyCredentialRequestOptions options =
+                Fido2Helper.toGetAssertionOptions(mRequestOptions);
+        Assert.assertEquals(Fido2Helper.MAX_TIMEOUT_SECONDS, options.getTimeoutSeconds(), 0);
+    }
+
+    @Test
+    @SmallTest
+    public void testToGetAssertionOptions_reasonableTimeout() throws Exception {
+        mRequestOptions.timeout = new TimeDelta();
+        mRequestOptions.timeout.microseconds = 1_000_000L * 60 * 2; // Two minutes.
+        PublicKeyCredentialRequestOptions options =
+                Fido2Helper.toGetAssertionOptions(mRequestOptions);
+        Assert.assertEquals(2 * 60, options.getTimeoutSeconds(), 0);
+    }
+}

content/browser/webauth/webauth_browsertest.cc

@@ -504,7 +504,6 @@ class WebAuthLocalClientBrowserTest : public WebAuthBrowserTestBase {
     std::vector<uint8_t> kTestChallenge{0, 0, 0};
     auto mojo_options = blink::mojom::PublicKeyCredentialCreationOptions::New(
         rp, user, kTestChallenge, parameters, base::TimeDelta::FromSeconds(30),
-        base::TimeDelta::FromSeconds(30),
         std::vector<device::PublicKeyCredentialDescriptor>(),
         device::AuthenticatorSelectionCriteria(),
         device::AttestationConveyancePreference::kNone, nullptr,
@@ -529,10 +528,9 @@ class WebAuthLocalClientBrowserTest : public WebAuthBrowserTestBase {
 
     std::vector<uint8_t> kTestChallenge{0, 0, 0};
     auto mojo_options = blink::mojom::PublicKeyCredentialRequestOptions::New(
-        kTestChallenge, base::TimeDelta::FromSeconds(30),
-        base::TimeDelta::FromSeconds(30), "acme.com", std::move(credentials),
-        device::UserVerificationRequirement::kPreferred, base::nullopt,
-        std::vector<device::CableDiscoveryData>());
+        kTestChallenge, base::TimeDelta::FromSeconds(30), "acme.com",
+        std::move(credentials), device::UserVerificationRequirement::kPreferred,
+        base::nullopt, std::vector<device::CableDiscoveryData>());
     return mojo_options;
   }
 

third_party/blink/public/mojom/webauthn/authenticator.mojom

@@ -214,9 +214,6 @@ struct PublicKeyCredentialRequestOptions {
   // relying party.
   mojo_base.mojom.TimeDelta? timeout;
 
-  // TODO(nsatragno): remove this after updating android-internal.
-  mojo_base.mojom.TimeDelta adjusted_timeout;
-
   // An ASCII serialization of the origin claimed by the relying party.
   string relying_party_id;
 
@@ -306,9 +303,6 @@ struct PublicKeyCredentialCreationOptions {
   // relying party.
   mojo_base.mojom.TimeDelta? timeout;
 
-  // TODO(nsatragno): remove this after updating android-internal.
-  mojo_base.mojom.TimeDelta adjusted_timeout;
-
   // A list of credentials the relying party knows about. If an
   // authenticator has one of these credentials, it should not
   // create a new one.

third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc

@@ -25,21 +25,6 @@
 #include "third_party/blink/renderer/modules/credentialmanager/password_credential.h"
 #include "third_party/blink/renderer/modules/credentialmanager/public_key_credential.h"
 
-namespace {
-// Time to wait for an authenticator to successfully complete an operation.
-constexpr base::TimeDelta kAdjustedTimeoutLower =
-    base::TimeDelta::FromSeconds(10);
-constexpr base::TimeDelta kAdjustedTimeoutUpper =
-    base::TimeDelta::FromMinutes(10);
-
-base::TimeDelta AdjustTimeout(uint32_t timeout) {
-  base::TimeDelta adjusted_timeout;
-  adjusted_timeout = base::TimeDelta::FromMilliseconds(timeout);
-  return std::max(kAdjustedTimeoutLower,
-                  std::min(kAdjustedTimeoutUpper, adjusted_timeout));
-}
-}  // namespace
-
 namespace mojo {
 
 using blink::mojom::blink::AttestationConveyancePreference;
@@ -410,9 +395,6 @@ TypeConverter<PublicKeyCredentialCreationOptionsPtr,
   if (options->hasTimeout()) {
     mojo_options->timeout =
         base::TimeDelta::FromMilliseconds(options->timeout());
-    mojo_options->adjusted_timeout = AdjustTimeout(options->timeout());
-  } else {
-    mojo_options->adjusted_timeout = kAdjustedTimeoutUpper;
   }
 
   // Steps 8 and 9 of
@@ -556,9 +538,6 @@ TypeConverter<PublicKeyCredentialRequestOptionsPtr,
   if (options->hasTimeout()) {
     mojo_options->timeout =
         base::TimeDelta::FromMilliseconds(options->timeout());
-    mojo_options->adjusted_timeout = AdjustTimeout(options->timeout());
-  } else {
-    mojo_options->adjusted_timeout = kAdjustedTimeoutUpper;
   }
 
   mojo_options->relying_party_id = options->rpId();