webauthn: replace FidoAuthenticator::IsFooAuthenticator with an enum.

We've grown multiple virtual calls to ask whether an authenticator is a specific type. Rather than add another, this change introduces `GetType`, which subsumes them all. Change-Id: I2edddd084e2aa39ec2e95065a3f6766c735c0110 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3554482 Auto-Submit: Adam Langley <agl@chromium.org> Reviewed-by: Martin Kreichgauer <martinkr@google.com> Commit-Queue: Martin Kreichgauer <martinkr@google.com> Cr-Commit-Position: refs/heads/main@{#987118}
chromium · Mar 30, 2022 · 413e555 · 413e555
1 parent 81af487
commit 413e555
Show file tree

Hide file tree

Showing 17 changed files with 44 additions and 65 deletions.
diff --git a/chrome/browser/webauthn/authenticator_request_dialog_model.cc b/chrome/browser/webauthn/authenticator_request_dialog_model.cc
@@ -525,7 +525,8 @@ void AuthenticatorRequestDialogModel::AddAuthenticator(
     const device::FidoAuthenticator& authenticator) {
   if (!authenticator.AuthenticatorTransport()) {
 #if BUILDFLAG(IS_WIN)
-    DCHECK(authenticator.IsWinNativeApiAuthenticator());
+    DCHECK_EQ(authenticator.GetType(),
+              device::FidoAuthenticator::Type::kWinNative);
 #endif  // BUILDFLAG(IS_WIN)
     return;
   }

diff --git a/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc b/chrome/browser/webauthn/chrome_authenticator_request_delegate.cc
@@ -531,7 +531,7 @@ void ChromeAuthenticatorRequestDelegate::ShouldReturnAttestation(
   }
 
 #if BUILDFLAG(IS_WIN)
-  if (authenticator->IsWinNativeApiAuthenticator() &&
+  if (authenticator->GetType() == device::FidoAuthenticator::Type::kWinNative &&
       static_cast<const device::WinWebAuthnApiAuthenticator*>(authenticator)
           ->ShowsPrivacyNotice()) {
     // The OS' native API includes an attestation prompt.

diff --git a/content/browser/webauth/authenticator_common.cc b/content/browser/webauth/authenticator_common.cc
@@ -1308,7 +1308,8 @@ void AuthenticatorCommon::OnRegisterResponse(
 
 #if BUILDFLAG(IS_WIN)
       GetWebAuthenticationDelegate()->OperationSucceeded(
-          GetBrowserContext(), authenticator->IsWinNativeApiAuthenticator());
+          GetBrowserContext(), authenticator->GetType() ==
+                                   device::FidoAuthenticator::Type::kWinNative);
 #endif
 
       absl::optional<device::FidoTransportProtocol> transport =
@@ -1533,7 +1534,8 @@ void AuthenticatorCommon::OnSignResponse(
 
 #if BUILDFLAG(IS_WIN)
   GetWebAuthenticationDelegate()->OperationSucceeded(
-      GetBrowserContext(), authenticator->IsWinNativeApiAuthenticator());
+      GetBrowserContext(),
+      authenticator->GetType() == device::FidoAuthenticator::Type::kWinNative);
 #endif
 
   // Show an account picker for requests with empty allow lists.

diff --git a/device/fido/cros/authenticator.cc b/device/fido/cros/authenticator.cc
@@ -34,6 +34,10 @@ ChromeOSAuthenticator::ChromeOSAuthenticator(
 
 ChromeOSAuthenticator::~ChromeOSAuthenticator() {}
 
+FidoAuthenticator::Type ChromeOSAuthenticator::GetType() const {
+  return Type::kChromeOS;
+}
+
 std::string ChromeOSAuthenticator::GetId() const {
   return "ChromeOSAuthenticator";
 }
@@ -421,10 +425,6 @@ bool ChromeOSAuthenticator::RequiresBlePairingPin() const {
   return false;
 }
 
-bool ChromeOSAuthenticator::IsChromeOSAuthenticator() const {
-  return true;
-}
-
 base::WeakPtr<FidoAuthenticator> ChromeOSAuthenticator::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

diff --git a/device/fido/cros/authenticator.h b/device/fido/cros/authenticator.h
@@ -77,6 +77,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ChromeOSAuthenticator
                     GetAssertionCallback callback) override;
   void GetNextAssertion(GetAssertionCallback callback) override {}
   void Cancel() override;
+  Type GetType() const override;
   std::string GetId() const override;
   const absl::optional<AuthenticatorSupportedOptions>& Options() const override;
 
@@ -86,8 +87,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ChromeOSAuthenticator
   bool IsPaired() const override;
   bool RequiresBlePairingPin() const override;
 
-  bool IsChromeOSAuthenticator() const override;
-
   void GetTouch(base::OnceClosure callback) override {}
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 

diff --git a/device/fido/fido_authenticator.cc b/device/fido/fido_authenticator.cc
@@ -193,6 +193,10 @@ void FidoAuthenticator::Reset(ResetCallback callback) {
                           absl::nullopt);
 }
 
+FidoAuthenticator::Type FidoAuthenticator::GetType() const {
+  return Type::kOther;
+}
+
 std::string FidoAuthenticator::GetDisplayName() const {
   return GetId();
 }

diff --git a/device/fido/fido_authenticator.h b/device/fido/fido_authenticator.h
@@ -266,6 +266,16 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   // stored resident keys and any configured PIN.
   virtual void Reset(ResetCallback callback);
   virtual void Cancel() = 0;
+
+  enum class Type {
+    kWinNative,  // i.e. webauthn.dll
+    kTouchID,    // the Chrome-native Touch ID integration on macOS
+    kChromeOS,   // the platform authenticator on Chrome OS
+    kOther,
+  };
+  // GetType returns the type of the authenticator.
+  virtual Type GetType() const;
+
   // GetId returns a unique string representing this device. This string should
   // be distinct from all other devices concurrently discovered.
   virtual std::string GetId() const = 0;
@@ -286,15 +296,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
   virtual bool IsInPairingMode() const = 0;
   virtual bool IsPaired() const = 0;
   virtual bool RequiresBlePairingPin() const = 0;
-#if BUILDFLAG(IS_WIN)
-  virtual bool IsWinNativeApiAuthenticator() const = 0;
-#endif  // BUILDFLAG(IS_WIN)
-#if BUILDFLAG(IS_MAC)
-  virtual bool IsTouchIdAuthenticator() const = 0;
-#endif  // BUILDFLAG(IS_MAC)
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  virtual bool IsChromeOSAuthenticator() const = 0;
-#endif
   virtual base::WeakPtr<FidoAuthenticator> GetWeakPtr() = 0;
 };
 

diff --git a/device/fido/fido_device_authenticator.cc b/device/fido/fido_device_authenticator.cc
@@ -1108,24 +1108,6 @@ bool FidoDeviceAuthenticator::RequiresBlePairingPin() const {
   return device_->RequiresBlePairingPin();
 }
 
-#if BUILDFLAG(IS_WIN)
-bool FidoDeviceAuthenticator::IsWinNativeApiAuthenticator() const {
-  return false;
-}
-#endif  // BUILDFLAG(IS_WIN)
-
-#if BUILDFLAG(IS_MAC)
-bool FidoDeviceAuthenticator::IsTouchIdAuthenticator() const {
-  return false;
-}
-#endif  // BUILDFLAG(IS_MAC)
-
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-bool FidoDeviceAuthenticator::IsChromeOSAuthenticator() const {
-  return false;
-}
-#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
-
 void FidoDeviceAuthenticator::SetTaskForTesting(
     std::unique_ptr<FidoTask> task) {
   task_ = std::move(task);

diff --git a/device/fido/fido_device_authenticator.h b/device/fido/fido_device_authenticator.h
@@ -138,15 +138,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   bool IsInPairingMode() const override;
   bool IsPaired() const override;
   bool RequiresBlePairingPin() const override;
-#if BUILDFLAG(IS_WIN)
-  bool IsWinNativeApiAuthenticator() const override;
-#endif  // BUILDFLAG(IS_WIN)
-#if BUILDFLAG(IS_MAC)
-  bool IsTouchIdAuthenticator() const override;
-#endif  // BUILDFLAG(IS_MAC)
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  bool IsChromeOSAuthenticator() const override;
-#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
   FidoDevice* device() { return device_.get(); }

diff --git a/device/fido/fido_request_handler_base.cc b/device/fido/fido_request_handler_base.cc
@@ -375,7 +375,7 @@ void FidoRequestHandlerBase::AuthenticatorAdded(
   }
 
 #if BUILDFLAG(IS_WIN)
-  if (authenticator->IsWinNativeApiAuthenticator()) {
+  if (authenticator->GetType() == FidoAuthenticator::Type::kWinNative) {
     DCHECK(transport_availability_info_.has_win_native_api_authenticator);
     transport_availability_info_.win_native_api_authenticator_id =
         authenticator->GetId();

diff --git a/device/fido/get_assertion_handler_unittest.cc b/device/fido/get_assertion_handler_unittest.cc
@@ -852,9 +852,8 @@ TEST(GetAssertionRequestHandlerWinTest, TestWinUsbDiscovery) {
 
     ASSERT_FALSE(cb.was_called());
     EXPECT_EQ(handler->AuthenticatorsForTesting().size(), 1u);
-    EXPECT_EQ(handler->AuthenticatorsForTesting()
-                  .begin()
-                  ->second->IsWinNativeApiAuthenticator(),
+    EXPECT_EQ(handler->AuthenticatorsForTesting().begin()->second->GetType() ==
+                  FidoAuthenticator::Type::kWinNative,
               enable_api);
   }
 }

diff --git a/device/fido/get_assertion_request_handler.cc b/device/fido/get_assertion_request_handler.cc
@@ -421,7 +421,7 @@ void GetAssertionRequestHandler::GetPlatformCredentialStatus(
 
 #if BUILDFLAG(IS_MAC)
   // In tests the platform authenticator may be a virtual device.
-  if (!platform_authenticator->IsTouchIdAuthenticator()) {
+  if (platform_authenticator->GetType() != FidoAuthenticator::Type::kTouchID) {
     FidoRequestHandlerBase::GetPlatformCredentialStatus(platform_authenticator);
     return;
   }
@@ -573,7 +573,7 @@ void GetAssertionRequestHandler::HandleResponse(
   }
 
 #if BUILDFLAG(IS_WIN)
-  if (authenticator->IsWinNativeApiAuthenticator()) {
+  if (authenticator->GetType() == FidoAuthenticator::Type::kWinNative) {
     state_ = State::kFinished;
     CancelActiveAuthenticators(authenticator->GetId());
     if (status != CtapDeviceResponseCode::kSuccess) {

diff --git a/device/fido/mac/authenticator.h b/device/fido/mac/authenticator.h
@@ -69,13 +69,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
                     GetAssertionCallback callback) override;
   void GetNextAssertion(GetAssertionCallback callback) override;
   void Cancel() override;
+  Type GetType() const override;
   std::string GetId() const override;
   const absl::optional<AuthenticatorSupportedOptions>& Options() const override;
   absl::optional<FidoTransportProtocol> AuthenticatorTransport() const override;
   bool IsInPairingMode() const override;
   bool IsPaired() const override;
   bool RequiresBlePairingPin() const override;
-  bool IsTouchIdAuthenticator() const override;
   void GetTouch(base::OnceClosure callback) override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 

diff --git a/device/fido/mac/authenticator.mm b/device/fido/mac/authenticator.mm
@@ -157,6 +157,10 @@ new TouchIdAuthenticator(std::move(config.keychain_access_group),
   operation_.reset();
 }
 
+FidoAuthenticator::Type TouchIdAuthenticator::GetType() const {
+  return Type::kTouchID;
+}
+
 std::string TouchIdAuthenticator::GetId() const {
   return "TouchIdAuthenticator";
 }
@@ -199,10 +203,6 @@ AuthenticatorSupportedOptions TouchIdAuthenticatorOptions() {
   return false;
 }
 
-bool TouchIdAuthenticator::IsTouchIdAuthenticator() const {
-  return true;
-}
-
 void TouchIdAuthenticator::GetTouch(base::OnceClosure callback) {
   NOTREACHED();
   std::move(callback).Run();

diff --git a/device/fido/make_credential_request_handler.cc b/device/fido/make_credential_request_handler.cc
@@ -456,7 +456,7 @@ void MakeCredentialRequestHandler::DispatchRequest(
     // If the Windows API cannot handle a request, just reject the request
     // outright. There are no other authenticators to attempt, so calling
     // GetTouch() would not make sense.
-    if (authenticator->IsWinNativeApiAuthenticator()) {
+    if (authenticator->GetType() == FidoAuthenticator::Type::kWinNative) {
       HandleInapplicableAuthenticator(authenticator, post_touch_status);
       return;
     }
@@ -721,7 +721,7 @@ void MakeCredentialRequestHandler::HandleResponse(
   }
 
 #if BUILDFLAG(IS_WIN)
-  if (authenticator->IsWinNativeApiAuthenticator()) {
+  if (authenticator->GetType() == FidoAuthenticator::Type::kWinNative) {
     state_ = State::kFinished;
     if (status != CtapDeviceResponseCode::kSuccess) {
       std::move(completion_callback_)
@@ -961,7 +961,7 @@ void MakeCredentialRequestHandler::SpecializeRequestForAuthenticator(
       request->resident_key_required =
 #if BUILDFLAG(IS_WIN)
           // Windows does not yet support rk=preferred.
-          !authenticator->IsWinNativeApiAuthenticator() &&
+          authenticator->GetType() != FidoAuthenticator::Type::kWinNative &&
 #endif
           auth_options && auth_options->supports_resident_key &&
           !authenticator->DiscoverableCredentialStorageFull() &&

diff --git a/device/fido/win/authenticator.cc b/device/fido/win/authenticator.cc
@@ -167,6 +167,10 @@ void WinWebAuthnApiAuthenticator::Cancel() {
   win_api_->CancelCurrentOperation(&cancellation_id_);
 }
 
+FidoAuthenticator::Type WinWebAuthnApiAuthenticator::GetType() const {
+  return Type::kWinNative;
+}
+
 std::string WinWebAuthnApiAuthenticator::GetId() const {
   return "WinWebAuthnApiAuthenticator";
 }
@@ -190,10 +194,6 @@ WinWebAuthnApiAuthenticator::AuthenticatorTransport() const {
   return absl::nullopt;
 }
 
-bool WinWebAuthnApiAuthenticator::IsWinNativeApiAuthenticator() const {
-  return true;
-}
-
 bool WinWebAuthnApiAuthenticator::SupportsCredProtectExtension() const {
   return win_api_->Version() >= WEBAUTHN_API_VERSION_2;
 }

diff --git a/device/fido/win/authenticator.h b/device/fido/win/authenticator.h
@@ -64,6 +64,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinWebAuthnApiAuthenticator
                     GetAssertionCallback callback) override;
   void GetTouch(base::OnceClosure callback) override;
   void Cancel() override;
+  Type GetType() const override;
   std::string GetId() const override;
   bool IsInPairingMode() const override;
   bool IsPaired() const override;
@@ -76,7 +77,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) WinWebAuthnApiAuthenticator
   bool SupportsCredBlobOfSize(size_t num_bytes) const override;
   const absl::optional<AuthenticatorSupportedOptions>& Options() const override;
   absl::optional<FidoTransportProtocol> AuthenticatorTransport() const override;
-  bool IsWinNativeApiAuthenticator() const override;
   base::WeakPtr<FidoAuthenticator> GetWeakPtr() override;
 
   void MakeCredentialDone(