device/fido/mac: integrate with browsing data deletion

This updates browsing data deletion for DATA_TYPE_PASSWORDS to call
device::fido::mac::DeleteWebAuthnCredentials, which deletes credentials
created by the macOS platform authenticator from the OS keychain.

Also combine the two Touch ID specific configuration methods in
{Chrome,}AuthenticatorRequestDelegate and introduce a static variant for
Chrome.

Bug: 678128
Change-Id: I0034e0815da068fb27c1ea60cad95f958956838e
Reviewed-on: https://chromium-review.googlesource.com/1125177
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574697}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -56,6 +56,7 @@
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/web_data_service_factory.h"
+#include "chrome/browser/webauthn/chrome_authenticator_request_delegate.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -129,6 +130,10 @@
 #include "components/user_manager/user.h"
 #endif  // defined(OS_CHROMEOS)
 
+#if defined(OS_MACOSX)
+#include "device/fido/mac/browsing_data_deletion.h"
+#endif  // defined(OS_MACOSX)
+
 #if BUILDFLAG(ENABLE_PLUGINS)
 #include "chrome/browser/browsing_data/browsing_data_flash_lso_helper.h"
 #endif  // BUILDFLAG(ENABLE_PLUGINS)
@@ -782,6 +787,14 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
         ->GetNetworkContext()
         ->ClearHttpAuthCache(delete_begin_,
                              CreatePendingTaskCompletionClosureForMojo());
+
+#if defined(OS_MACOSX)
+    auto authenticator_config = ChromeAuthenticatorRequestDelegate::
+        TouchIdAuthenticatorConfigForProfile(profile_);
+    device::fido::mac::DeleteWebAuthnCredentials(
+        authenticator_config.keychain_access_group,
+        authenticator_config.metadata_secret, delete_begin_, delete_end_);
+#endif  // defined(OS_MACOSX)
   }
 
   if (remove_mask & content::BrowsingDataRemover::DATA_TYPE_COOKIES) {

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -178,21 +178,13 @@ bool ChromeAuthenticatorRequestDelegate::IsFocused() {
 }
 
 #if defined(OS_MACOSX)
-std::string
-ChromeAuthenticatorRequestDelegate::TouchIdAuthenticatorKeychainAccessGroup() {
-  // This exact value must be whitelisted in the keychain-access-group section
-  // of the entitlements plist file with which Chrome is signed. Note that
-  // even though the bundle identifier for the Canary channel differs from that
-  // of the other channels, Canary still uses the same keychain access group.
-  static const char* access_group = "EQHXZ8M8AV.com.google.Chrome.webauthn";
-  return access_group;
-}
-#endif
+static constexpr char kTouchIdKeychainAccessGroup[] =
+    "EQHXZ8M8AV.com.google.Chrome.webauthn";
 
-#if defined(OS_MACOSX)
-std::string ChromeAuthenticatorRequestDelegate::TouchIdMetadataSecret() {
-  PrefService* prefs =
-      Profile::FromBrowserContext(browser_context())->GetPrefs();
+namespace {
+
+std::string TouchIdMetadataSecret(Profile* profile) {
+  PrefService* prefs = profile->GetPrefs();
   std::string key = prefs->GetString(kWebAuthnTouchIdMetadataSecretPrefName);
   if (key.empty() || !base::Base64Decode(key, &key)) {
     key = device::fido::mac::CredentialMetadata::GenerateRandomSecret();
@@ -202,6 +194,24 @@ std::string ChromeAuthenticatorRequestDelegate::TouchIdMetadataSecret() {
   }
   return key;
 }
+
+}  // namespace
+
+// static
+content::AuthenticatorRequestClientDelegate::TouchIdAuthenticatorConfig
+ChromeAuthenticatorRequestDelegate::TouchIdAuthenticatorConfigForProfile(
+    Profile* profile) {
+  return content::AuthenticatorRequestClientDelegate::
+      TouchIdAuthenticatorConfig{kTouchIdKeychainAccessGroup,
+                                 TouchIdMetadataSecret(profile)};
+}
+
+base::Optional<
+    content::AuthenticatorRequestClientDelegate::TouchIdAuthenticatorConfig>
+ChromeAuthenticatorRequestDelegate::GetTouchIdAuthenticatorConfig() const {
+  return TouchIdAuthenticatorConfigForProfile(
+      Profile::FromBrowserContext(browser_context()));
+}
 #endif
 
 void ChromeAuthenticatorRequestDelegate::OnModelDestroyed() {

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -10,6 +10,8 @@
 #include "chrome/browser/webauthn/authenticator_request_dialog_model.h"
 #include "content/public/browser/authenticator_request_client_delegate.h"
 
+class Profile;
+
 namespace content {
 class BrowserContext;
 class RenderFrameHost;
@@ -26,18 +28,22 @@ class ChromeAuthenticatorRequestDelegate
       public AuthenticatorRequestDialogModel::Observer {
  public:
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+#if defined(OS_MACOSX)
+  static TouchIdAuthenticatorConfig TouchIdAuthenticatorConfigForProfile(
+      Profile* profile);
+#endif  // defined(OS_MACOSX)
 
   // The |render_frame_host| must outlive this instance.
   explicit ChromeAuthenticatorRequestDelegate(
       content::RenderFrameHost* render_frame_host);
   ~ChromeAuthenticatorRequestDelegate() override;
 
-  base::WeakPtr<ChromeAuthenticatorRequestDelegate> AsWeakPtr();
-
 #if defined(OS_MACOSX)
-  std::string TouchIdAuthenticatorKeychainAccessGroup() override;
-  std::string TouchIdMetadataSecret() override;
-#endif
+  base::Optional<TouchIdAuthenticatorConfig> GetTouchIdAuthenticatorConfig()
+      const override;
+#endif  // defined(OS_MACOSX)
+
+  base::WeakPtr<ChromeAuthenticatorRequestDelegate> AsWeakPtr();
 
  private:
   content::RenderFrameHost* render_frame_host() const {

chrome/browser/webauthn/chrome_authenticator_request_delegate_unittest.cc

@@ -17,11 +17,18 @@ class ChromeAuthenticatorRequestDelegateTest
     : public ChromeRenderViewHostTestHarness {};
 
 #if defined(OS_MACOSX)
+std::string TouchIdMetadataSecret(
+    const ChromeAuthenticatorRequestDelegate& delegate) {
+  base::Optional<AuthenticatorRequestClientDelegate::TouchIdAuthenticatorConfig>
+      config = delegate.GetTouchIdAuthenticatorConfig();
+  return config->metadata_secret;
+}
+
 TEST_F(ChromeAuthenticatorRequestDelegateTest, TouchIdMetadataSecret) {
   ChromeAuthenticatorRequestDelegate delegate(main_rfh());
-  std::string secret = delegate.TouchIdMetadataSecret();
+  std::string secret = TouchIdMetadataSecret(delegate);
   EXPECT_EQ(secret.size(), 32u);
-  EXPECT_EQ(secret, delegate.TouchIdMetadataSecret());
+  EXPECT_EQ(secret, TouchIdMetadataSecret(delegate));
 }
 
 TEST_F(ChromeAuthenticatorRequestDelegateTest,
@@ -29,8 +36,8 @@ TEST_F(ChromeAuthenticatorRequestDelegateTest,
   // Different delegates on the same BrowserContext (Profile) should return the
   // same secret.
   EXPECT_EQ(
-      ChromeAuthenticatorRequestDelegate(main_rfh()).TouchIdMetadataSecret(),
-      ChromeAuthenticatorRequestDelegate(main_rfh()).TouchIdMetadataSecret());
+      TouchIdMetadataSecret(ChromeAuthenticatorRequestDelegate(main_rfh())),
+      TouchIdMetadataSecret(ChromeAuthenticatorRequestDelegate(main_rfh())));
 }
 
 TEST_F(ChromeAuthenticatorRequestDelegateTest,
@@ -41,14 +48,13 @@ TEST_F(ChromeAuthenticatorRequestDelegateTest,
   auto web_contents =
       WebContentsTester::CreateTestWebContents(browser_context.get(), nullptr);
   EXPECT_NE(
-      ChromeAuthenticatorRequestDelegate(main_rfh()).TouchIdMetadataSecret(),
-      ChromeAuthenticatorRequestDelegate(web_contents->GetMainFrame())
-          .TouchIdMetadataSecret());
+      TouchIdMetadataSecret(ChromeAuthenticatorRequestDelegate(main_rfh())),
+      TouchIdMetadataSecret(
+          ChromeAuthenticatorRequestDelegate(web_contents->GetMainFrame())));
   // Ensure this second secret is actually valid.
-  EXPECT_EQ(32u,
-            ChromeAuthenticatorRequestDelegate(web_contents->GetMainFrame())
-                .TouchIdMetadataSecret()
-                .size());
+  EXPECT_EQ(32u, TouchIdMetadataSecret(ChromeAuthenticatorRequestDelegate(
+                                           web_contents->GetMainFrame()))
+                     .size());
 }
 #endif
 

content/browser/webauth/authenticator_impl.cc

@@ -816,12 +816,15 @@ void AuthenticatorImpl::Cleanup() {
 std::unique_ptr<device::FidoAuthenticator>
 AuthenticatorImpl::MaybeCreatePlatformAuthenticator() {
 #if defined(OS_MACOSX)
-  return device::fido::mac::TouchIdAuthenticator::CreateIfAvailable(
-      request_delegate_->TouchIdAuthenticatorKeychainAccessGroup(),
-      request_delegate_->TouchIdMetadataSecret());
-#else
+  auto opt_authenticator_config =
+      request_delegate_->GetTouchIdAuthenticatorConfig();
+  if (opt_authenticator_config) {
+    return device::fido::mac::TouchIdAuthenticator::CreateIfAvailable(
+        std::move(opt_authenticator_config->keychain_access_group),
+        std::move(opt_authenticator_config->metadata_secret));
+  }
+#endif  // defined(OS_MACOSX)
   return nullptr;
-#endif
 }
 
 }  // namespace content

content/public/browser/authenticator_request_client_delegate.cc

@@ -32,15 +32,9 @@ bool AuthenticatorRequestClientDelegate::IsFocused() {
 }
 
 #if defined(OS_MACOSX)
-std::string
-AuthenticatorRequestClientDelegate::TouchIdAuthenticatorKeychainAccessGroup() {
-  return std::string();
-}
-#endif
-
-#if defined(OS_MACOSX)
-std::string AuthenticatorRequestClientDelegate::TouchIdMetadataSecret() {
-  return std::string();
+base::Optional<AuthenticatorRequestClientDelegate::TouchIdAuthenticatorConfig>
+AuthenticatorRequestClientDelegate::GetTouchIdAuthenticatorConfig() const {
+  return base::nullopt;
 }
 #endif
 

content/public/browser/authenticator_request_client_delegate.h

@@ -9,6 +9,7 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
 
@@ -55,20 +56,24 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate {
   virtual bool IsFocused();
 
 #if defined(OS_MACOSX)
-  // Returns the kechain-access-group value used for WebAuthn credentials
-  // stored in the macOS keychain by the built-in Touch ID authenticator. For
-  // more information on this, refer to |device::fido::TouchIdAuthenticator|.
-  // This method may to return empty string or some other placeholder value on
-  // platforms where |TouchIdAuthenticator| is not used.
-  virtual std::string TouchIdAuthenticatorKeychainAccessGroup();
+  struct TouchIdAuthenticatorConfig {
+    // The keychain-access-group value used for WebAuthn credentials
+    // stored in the macOS keychain by the built-in Touch ID
+    // authenticator. For more information on this, refer to
+    // |device::fido::TouchIdAuthenticator|.
+    std::string keychain_access_group;
+    // The secret used to derive key material when encrypting WebAuthn
+    // credential metadata for storage in the macOS keychain. Chrome returns
+    // different secrets for each user profile in order to logically separate
+    // credentials per profile.
+    std::string metadata_secret;
+  };
 
-  // Returns the secret used to derive key material when encrypting WebAuthn
-  // credential metadata for storage in the macOS keychain. Chrome returns
-  // different secrets for each user profile in order to logically separate
-  // credentials per profile. This method may to return empty string or some
-  // other placeholder value on platforms where |TouchIdAuthenticator| is not
-  // used.
-  virtual std::string TouchIdMetadataSecret();
+  // Returns configuration data for the built-in Touch ID platform
+  // authenticator. May return nullopt if the authenticator is not used or not
+  // available.
+  virtual base::Optional<TouchIdAuthenticatorConfig>
+  GetTouchIdAuthenticatorConfig() const;
 #endif
 
  private:

device/fido/mac/authenticator.h

@@ -28,13 +28,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   static std::unique_ptr<TouchIdAuthenticator> CreateIfAvailable(
       std::string keychain_access_group,
       std::string metadata_secret);
+
   static std::unique_ptr<TouchIdAuthenticator> CreateForTesting(
       std::string keychain_access_group,
       std::string metadata_secret);
 
   ~TouchIdAuthenticator() override;
 
-  // TouchIdAuthenticator
+  // FidoAuthenticator
   void MakeCredential(
       AuthenticatorSelectionCriteria authenticator_selection_criteria,
       CtapMakeCredentialRequest request,
@@ -42,7 +43,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   void GetAssertion(CtapGetAssertionRequest request,
                     GetAssertionCallback callback) override;
   void Cancel() override;
-
   std::string GetId() const override;
 
  private:
@@ -52,7 +52,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   // The keychain access group under which credentials are stored in the macOS
   // keychain for access control. The set of all access groups that the
   // application belongs to is stored in the entitlements file that gets
-  // embedded into the application during code signing. For more information see
+  // embedded into the application during code signing. For more information
+  // see
   // https://developer.apple.com/documentation/security/ksecattraccessgroup?language=objc.
   std::string keychain_access_group_;