Clean up selecting caBLE with high priority for GetAssertion calls.

Bug: 849323
Change-Id: I45aa37db4cdd396b8eac174e0a82a2a11abacb4c
Reviewed-on: https://chromium-review.googlesource.com/1196526Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587716}

chrome/browser/webauthn/authenticator_request_dialog_model.cc

@@ -15,40 +15,55 @@ namespace {
 // Attempts to auto-select the most likely transport that will be used to
 // service this request, or returns base::nullopt if unsure.
 base::Optional<device::FidoTransportProtocol> SelectMostLikelyTransport(
-    device::FidoRequestHandlerBase::TransportAvailabilityInfo
+    const device::FidoRequestHandlerBase::TransportAvailabilityInfo&
         transport_availability,
     base::Optional<device::FidoTransportProtocol> last_used_transport) {
+  base::flat_set<AuthenticatorTransport> candidate_transports(
+      transport_availability.available_transports);
+
+  // As an exception, we can tell in advance if using Touch Id will succeed. If
+  // yes, always auto-select that transport over all other considerations for
+  // GetAssertion operations; and de-select it if it will not work.
   if (transport_availability.request_type ==
           device::FidoRequestHandlerBase::RequestType::kGetAssertion &&
-      base::ContainsKey(transport_availability.available_transports,
+      base::ContainsKey(candidate_transports,
                         device::FidoTransportProtocol::kInternal)) {
     // For GetAssertion requests, auto advance to Touch ID if the keychain
     // contains one of the allowedCredentials.
-    if (transport_availability.has_recognized_mac_touch_id_credential) {
+    if (transport_availability.has_recognized_mac_touch_id_credential)
       return device::FidoTransportProtocol::kInternal;
-    }
+
     // Otherwise make sure we never return Touch ID (unless there is no other
     // option, in which case we auto advance to a special error screen).
-    if (transport_availability.available_transports.size() == 1) {
+    candidate_transports.erase(device::FidoTransportProtocol::kInternal);
+    if (candidate_transports.empty())
       return device::FidoTransportProtocol::kInternal;
-    }
-    transport_availability.available_transports.erase(
-        device::FidoTransportProtocol::kInternal);
   }
 
-  // For GetAssertion call, if the |last_used_transport| is available, use that.
+  // If caBLE is listed as one of the allowed transports, it indicates that the
+  // RP has bothered to supply the |cable_extension|. Respect that and always
+  // select caBLE in that case for GetAssertion operations.
+  if (transport_availability.request_type ==
+          device::FidoRequestHandlerBase::RequestType::kGetAssertion &&
+      base::ContainsKey(
+          candidate_transports,
+          AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy)) {
+    return AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy;
+  }
+
+  // Otherwise, for GetAssertion calls, if the |last_used_transport| is
+  // available, use that.
   if (transport_availability.request_type ==
           device::FidoRequestHandlerBase::RequestType::kGetAssertion &&
       last_used_transport &&
-      base::ContainsKey(transport_availability.available_transports,
-                        *last_used_transport)) {
+      base::ContainsKey(candidate_transports, *last_used_transport)) {
     return *last_used_transport;
   }
 
-  // If there is only one transport available we can use, select that, instead
-  // of showing a transport selection screen with only a single transport.
-  if (transport_availability.available_transports.size() == 1) {
-    return *transport_availability.available_transports.begin();
+  // Finally, if there is only one transport available we can use, select that,
+  // instead of showing a transport selection screen with only a single item.
+  if (candidate_transports.size() == 1) {
+    return *candidate_transports.begin();
   }
 
   return base::nullopt;

chrome/browser/webauthn/authenticator_request_dialog_model_unittest.cc

@@ -28,6 +28,13 @@ const base::flat_set<AuthenticatorTransport> kAllTransports = {
     AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy,
 };
 
+const base::flat_set<AuthenticatorTransport> kAllTransportsWithoutCable = {
+    AuthenticatorTransport::kUsbHumanInterfaceDevice,
+    AuthenticatorTransport::kNearFieldCommunication,
+    AuthenticatorTransport::kBluetoothLowEnergy,
+    AuthenticatorTransport::kInternal,
+};
+
 using TransportAvailabilityInfo =
     ::device::FidoRequestHandlerBase::TransportAvailabilityInfo;
 
@@ -94,7 +101,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
     bool has_touch_id_credential;
     Step expected_first_step;
   } kTestCases[] = {
-      // Only a single transport is available.
+      // Only a single transport is available for a GetAssertion call.
       {RequestType::kGetAssertion,
        {AuthenticatorTransport::kUsbHumanInterfaceDevice},
        base::nullopt,
@@ -110,11 +117,6 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
        base::nullopt,
        false,
        Step::kBleActivate},
-      {RequestType::kMakeCredential,
-       {AuthenticatorTransport::kInternal},
-       base::nullopt,
-       false,
-       Step::kTouchId},
       {RequestType::kGetAssertion,
        {AuthenticatorTransport::kInternal},
        base::nullopt,
@@ -131,14 +133,14 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
        false,
        Step::kCableActivate},
 
-      // The last used transport is available.
-      {RequestType::kGetAssertion, kAllTransports,
+      // The last used transport is available (and caBLE is not).
+      {RequestType::kGetAssertion, kAllTransportsWithoutCable,
        AuthenticatorTransport::kUsbHumanInterfaceDevice, false,
        Step::kUsbInsertAndActivate},
 
       // The last used transport is not available.
       {RequestType::kGetAssertion,
-       {AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy,
+       {AuthenticatorTransport::kBluetoothLowEnergy,
         AuthenticatorTransport::kUsbHumanInterfaceDevice},
        AuthenticatorTransport::kNearFieldCommunication,
        false,
@@ -154,7 +156,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
        AuthenticatorTransport::kUsbHumanInterfaceDevice,
        false,
        Step::kErrorInternalUnrecognized},
-      {RequestType::kGetAssertion, kAllTransports,
+      {RequestType::kGetAssertion, kAllTransportsWithoutCable,
        AuthenticatorTransport::kUsbHumanInterfaceDevice, false,
        Step::kUsbInsertAndActivate},
       {RequestType::kGetAssertion,
@@ -163,8 +165,8 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
        AuthenticatorTransport::kInternal,
        false,
        Step::kUsbInsertAndActivate},
-      {RequestType::kGetAssertion, kAllTransports, base::nullopt, false,
-       Step::kTransportSelection},
+      {RequestType::kGetAssertion, kAllTransportsWithoutCable, base::nullopt,
+       false, Step::kTransportSelection},
 
       // The KeyChain contains an allowed Touch ID credential, but Touch ID is
       // not enabled by the relying party.
@@ -175,11 +177,28 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
        Step::kUsbInsertAndActivate},
       {RequestType::kGetAssertion,
        {AuthenticatorTransport::kUsbHumanInterfaceDevice,
-        AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy},
+        AuthenticatorTransport::kBluetoothLowEnergy},
        base::nullopt,
        true,
        Step::kTransportSelection},
 
+      // If caBLE is one of the allowed transports, it has second-highest
+      // priority after Touch ID, and is auto-selected for GetAssertion
+      // operations even if the last used transport was something else.
+      {RequestType::kGetAssertion, kAllTransports, base::nullopt, false,
+       Step::kCableActivate},
+      {RequestType::kGetAssertion,
+       {AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy,
+        AuthenticatorTransport::kUsbHumanInterfaceDevice},
+       AuthenticatorTransport::kUsbHumanInterfaceDevice,
+       false,
+       Step::kCableActivate},
+
+      // caBLE should not enjoy this same high priority for MakeCredential
+      // calls.
+      {RequestType::kMakeCredential, kAllTransports, base::nullopt, false,
+       Step::kTransportSelection},
+
       // No transports available.
       {RequestType::kGetAssertion,
        {},
@@ -197,12 +216,27 @@ TEST_F(AuthenticatorRequestDialogModelTest, TransportAutoSelection) {
       // selection view for MakeCredential call.
       {RequestType::kMakeCredential,
        {AuthenticatorTransport::kUsbHumanInterfaceDevice},
-       AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy,
+       base::nullopt,
        false,
        Step::kUsbInsertAndActivate},
+      {RequestType::kMakeCredential,
+       {AuthenticatorTransport::kInternal},
+       base::nullopt,
+       false,
+       Step::kTouchId},
+      {RequestType::kMakeCredential,
+       {AuthenticatorTransport::kBluetoothLowEnergy},
+       base::nullopt,
+       false,
+       Step::kBleActivate},
+      {RequestType::kMakeCredential,
+       {AuthenticatorTransport::kCloudAssistedBluetoothLowEnergy},
+       base::nullopt,
+       false,
+       Step::kCableActivate},
       {RequestType::kMakeCredential,
        {AuthenticatorTransport::kUsbHumanInterfaceDevice},
-       AuthenticatorTransport::kUsbHumanInterfaceDevice,
+       base::nullopt,
        false,
        Step::kUsbInsertAndActivate},
   };
@@ -284,7 +318,7 @@ TEST_F(AuthenticatorRequestDialogModelTest, PostMortems) {
     model.AddObserver(&mock_observer);
 
     TransportAvailabilityInfo transports_info;
-    transports_info.available_transports = kAllTransports;
+    transports_info.available_transports = kAllTransportsWithoutCable;
 
     EXPECT_CALL(mock_observer, OnStepTransition());
     model.StartFlow(std::move(transports_info), base::nullopt);

content/browser/webauth/authenticator_impl.cc

@@ -574,12 +574,6 @@ void AuthenticatorImpl::GetAssertion(
       ConstructClientDataHash(client_data_json_), std::move(options),
       alternative_application_parameter_);
 
-  if (ctap_request.cable_extension()) {
-    // Always skip directly to the caBLE screen if the caBLE extension is used.
-    request_delegate_->UpdateLastTransportUsed(
-        device::FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy);
-  }
-
   auto opt_platform_authenticator_info =
       CreatePlatformAuthenticatorIfAvailableAndCheckIfCredentialExists(
           ctap_request);