Settings UI: Split up security key-related browser proxies and message handlers

The Security Key settings subpage currently has a single monolithic
browser proxy and associated message handler for three sets of logically
independent functionality (PIN, credential management and reset). This
change, splits them into three individual parts.

This is a structural change only. No functional/behavior changes
intended.

Change-Id: Ie167ed58b4bd293764d6e784d80f922a43296551
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1688206
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680566}

chrome/browser/resources/settings/privacy_page/security_keys_browser_proxy.js

@@ -28,7 +28,7 @@ let Credential;
 
 cr.define('settings', function() {
   /** @interface */
-  class SecurityKeysBrowserProxy {
+  class SecurityKeysPINBrowserProxy {
     /**
      * Starts a PIN set/change operation by flashing all security keys. Resolves
      * with a pair of numbers. The first is one if the process has immediately
@@ -50,6 +50,12 @@ cr.define('settings', function() {
      */
     setPIN(oldPIN, newPIN) {}
 
+    /** Cancels all outstanding operations. */
+    close() {}
+  }
+
+  /** @interface */
+  class SecurityKeysCredentialBrowserProxy {
     /**
      * Starts a credential management operation.
      *
@@ -70,24 +76,30 @@ cr.define('settings', function() {
      * @return {!Promise<?number>} a promise that resolves with null if the PIN
      *     was correct or the number of retries remaining otherwise.
      */
-    credentialManagementProvidePIN(pin) {}
+    providePIN(pin) {}
 
     /**
      * Enumerates credentials on the authenticator. A correct PIN must have
-     * previously been supplied via credentialManagementProvidePIN() before this
+     * previously been supplied via providePIN() before this
      * method may be called.
      * @return {!Promise<!Array<!Credential>>}
      */
-    credentialManagementEnumerate() {}
+    enumerateCredentials() {}
 
     /**
      * Deletes the credentials with the given IDs from the security key.
-     * @param {!Array<!string>} ids
-     * @return {!Promise<!string>} a localized response message to display to
+     * @param {!Array<string>} ids
+     * @return {!Promise<string>} a localized response message to display to
      *     the user (on either success or error)
      */
-    credentialManagementDeleteCredentials(ids) {}
+    deleteCredentials(ids) {}
 
+    /** Cancels all outstanding operations. */
+    close() {}
+  }
+
+  /** @interface */
+  class SecurityKeysResetBrowserProxy {
     /**
      * Starts a reset operation by flashing all security keys and sending a
      * reset command to the one that the user activates. Resolves with a CTAP
@@ -102,16 +114,12 @@ cr.define('settings', function() {
      */
     completeReset() {}
 
-    /**
-     * Cancel all outstanding operations.
-     */
+    /** Cancels all outstanding operations. */
     close() {}
   }
 
-  /**
-   * @implements {settings.SecurityKeysBrowserProxy}
-   */
-  class SecurityKeysBrowserProxyImpl {
+  /** @implements {settings.SecurityKeysPINBrowserProxy} */
+  class SecurityKeysPINBrowserProxyImpl {
     /** @override */
     startSetPIN() {
       return cr.sendWithPromise('securityKeyStartSetPIN');
@@ -122,26 +130,42 @@ cr.define('settings', function() {
       return cr.sendWithPromise('securityKeySetPIN', oldPIN, newPIN);
     }
 
+    /** @override */
+    close() {
+      return chrome.send('securityKeyPINClose');
+    }
+  }
+
+  /** @implements {settings.SecurityKeysCredentialBrowserProxy} */
+  class SecurityKeysCredentialBrowserProxyImpl {
     /** @override */
     startCredentialManagement() {
-      return cr.sendWithPromise('securityKeyCredentialManagement');
+      return cr.sendWithPromise('securityKeyCredentialManagementStart');
     }
 
     /** @override */
-    credentialManagementProvidePIN(pin) {
+    providePIN(pin) {
       return cr.sendWithPromise('securityKeyCredentialManagementPIN', pin);
     }
 
     /** @override */
-    credentialManagementEnumerate() {
+    enumerateCredentials() {
       return cr.sendWithPromise('securityKeyCredentialManagementEnumerate');
     }
 
     /** @override */
-    credentialManagementDeleteCredentials(ids) {
+    deleteCredentials(ids) {
       return cr.sendWithPromise('securityKeyCredentialManagementDelete', ids);
     }
 
+    /** @override */
+    close() {
+      return chrome.send('securityKeyCredentialManagementClose');
+    }
+  }
+
+  /** @implements {settings.SecurityKeysResetBrowserProxy} */
+  class SecurityKeysResetBrowserProxyImpl {
     /** @override */
     reset() {
       return cr.sendWithPromise('securityKeyReset');
@@ -154,16 +178,23 @@ cr.define('settings', function() {
 
     /** @override */
     close() {
-      return chrome.send('securityKeyClose');
+      return chrome.send('securityKeyResetClose');
     }
   }
 
   // The singleton instance_ is replaced with a test version of this wrapper
   // during testing.
-  cr.addSingletonGetter(SecurityKeysBrowserProxyImpl);
+  cr.addSingletonGetter(SecurityKeysPINBrowserProxyImpl);
+  cr.addSingletonGetter(SecurityKeysCredentialBrowserProxyImpl);
+  cr.addSingletonGetter(SecurityKeysResetBrowserProxyImpl);
 
   return {
-    SecurityKeysBrowserProxy: SecurityKeysBrowserProxy,
-    SecurityKeysBrowserProxyImpl: SecurityKeysBrowserProxyImpl,
+    SecurityKeysPINBrowserProxy: SecurityKeysPINBrowserProxy,
+    SecurityKeysPINBrowserProxyImpl: SecurityKeysPINBrowserProxyImpl,
+    SecurityKeysCredentialBrowserProxy: SecurityKeysCredentialBrowserProxy,
+    SecurityKeysCredentialBrowserProxyImpl:
+        SecurityKeysCredentialBrowserProxyImpl,
+    SecurityKeysResetBrowserProxy: SecurityKeysResetBrowserProxy,
+    SecurityKeysResetBrowserProxyImpl: SecurityKeysResetBrowserProxyImpl,
   };
 });

chrome/browser/resources/settings/privacy_page/security_keys_credential_management_dialog.js

@@ -59,7 +59,7 @@ Polymer({
     deleteInProgress_: Boolean,
   },
 
-  /** @private {?settings.SecurityKeysBrowserProxy} */
+  /** @private {?settings.SecurityKeysCredentialBrowserProxy} */
   browserProxy_: null,
 
   /** @private {?Set<string>} */
@@ -72,7 +72,8 @@ Polymer({
         'security-keys-credential-management-finished',
         this.onError_.bind(this));
     this.checkedCredentialIds_ = new Set();
-    this.browserProxy_ = settings.SecurityKeysBrowserProxyImpl.getInstance();
+    this.browserProxy_ =
+        settings.SecurityKeysCredentialBrowserProxyImpl.getInstance();
     this.browserProxy_.startCredentialManagement().then(
         this.collectPin_.bind(this));
   },
@@ -97,14 +98,13 @@ Polymer({
     if (!this.$.pin.validate()) {
       return;
     }
-    this.browserProxy_.credentialManagementProvidePIN(this.$.pin.value)
-        .then((retries) => {
+    this.browserProxy_.providePIN(this.$.pin.value).then((retries) => {
       if (retries != null) {
         this.$.pin.showIncorrectPINError(retries);
         this.collectPin_();
         return;
       }
-          this.browserProxy_.credentialManagementEnumerate().then(
+      this.browserProxy_.enumerateCredentials().then(
           this.onCredentials_.bind(this));
     });
   },
@@ -257,9 +257,7 @@ Polymer({
     assert(this.checkedCredentialIds_.size > 0);
 
     this.deleteInProgress_ = true;
-    this.browserProxy_
-        .credentialManagementDeleteCredentials(
-            Array.from(this.checkedCredentialIds_))
+    this.browserProxy_.deleteCredentials(Array.from(this.checkedCredentialIds_))
         .then((err) => {
           this.deleteInProgress_ = false;
           this.onError_(err);

chrome/browser/resources/settings/privacy_page/security_keys_reset_dialog.js

@@ -42,13 +42,14 @@ Polymer({
     title_: String,
   },
 
-  /** @private {?settings.SecurityKeysBrowserProxy} */
+  /** @private {?settings.SecurityKeysResetBrowserProxy} */
   browserProxy_: null,
 
   /** @override */
   attached: function() {
     this.title_ = this.i18n('securityKeysResetTitle');
-    this.browserProxy_ = settings.SecurityKeysBrowserProxyImpl.getInstance();
+    this.browserProxy_ =
+        settings.SecurityKeysResetBrowserProxyImpl.getInstance();
     this.$.dialog.showModal();
 
     this.browserProxy_.reset().then(code => {

chrome/browser/resources/settings/privacy_page/security_keys_set_pin_dialog.js

@@ -140,13 +140,13 @@ Polymer({
     title_: String,
   },
 
-  /** @private {?settings.SecurityKeysBrowserProxy} */
+  /** @private {?settings.SecurityKeysPINBrowserProxy} */
   browserProxy_: null,
 
   /** @override */
   attached: function() {
     this.title_ = this.i18n('securityKeysSetPINInitialTitle');
-    this.browserProxy_ = settings.SecurityKeysBrowserProxyImpl.getInstance();
+    this.browserProxy_ = settings.SecurityKeysPINBrowserProxyImpl.getInstance();
     this.$.dialog.showModal();
 
     this.browserProxy_.startSetPIN().then(([success, errorCode]) => {

chrome/browser/ui/webui/settings/settings_security_key_handler.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/optional.h"
 #include "base/strings/string_number_conversions.h"
@@ -34,81 +35,54 @@ base::flat_set<device::FidoTransportProtocol> supported_transports() {
   return {device::FidoTransportProtocol::kUsbHumanInterfaceDevice};
 }
 
+void HandleClose(base::Closure close_callback, const base::ListValue* args) {
+  DCHECK_EQ(0u, args->GetSize());
+  close_callback.Run();
+}
+
 }  // namespace
 
 namespace settings {
 
-SecurityKeysHandler::SecurityKeysHandler()
-    : state_(State::kNone),
-      weak_factory_(new base::WeakPtrFactory<SecurityKeysHandler>(this)) {}
+void SecurityKeysHandlerBase::OnJavascriptAllowed() {}
+
+void SecurityKeysHandlerBase::OnJavascriptDisallowed() {
+  // If Javascript is disallowed, |Close| will invalidate all current WeakPtrs
+  // and thus drop all pending callbacks. This means that
+  // |IsJavascriptAllowed| doesn't need to be tested before each callback
+  // because, if the callback into this object happened, then Javascript is
+  // allowed.
+  Close();
+}
 
-SecurityKeysHandler::~SecurityKeysHandler() = default;
+SecurityKeysPINHandler::SecurityKeysPINHandler() = default;
+SecurityKeysPINHandler::~SecurityKeysPINHandler() = default;
 
-void SecurityKeysHandler::RegisterMessages() {
+void SecurityKeysPINHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "securityKeyStartSetPIN",
-      base::BindRepeating(&SecurityKeysHandler::HandleStartSetPIN,
+      base::BindRepeating(&SecurityKeysPINHandler::HandleStartSetPIN,
                           base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "securityKeySetPIN",
-      base::BindRepeating(&SecurityKeysHandler::HandleSetPIN,
-                          base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyClose", base::BindRepeating(&SecurityKeysHandler::HandleClose,
-                                              base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyReset", base::BindRepeating(&SecurityKeysHandler::HandleReset,
-                                              base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyCompleteReset",
-      base::BindRepeating(&SecurityKeysHandler::HandleCompleteReset,
+      base::BindRepeating(&SecurityKeysPINHandler::HandleSetPIN,
                           base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
-      "securityKeyCredentialManagement",
-      base::BindRepeating(&SecurityKeysHandler::HandleCredentialManagement,
-                          base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyCredentialManagementPIN",
-      base::BindRepeating(&SecurityKeysHandler::HandleCredentialManagementPIN,
-                          base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyCredentialManagementEnumerate",
-      base::BindRepeating(
-          &SecurityKeysHandler::HandleCredentialManagementEnumerate,
-          base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(
-      "securityKeyCredentialManagementDelete",
-      base::BindRepeating(
-          &SecurityKeysHandler::HandleCredentialManagementDelete,
-          base::Unretained(this)));
+      "securityKeyPINClose",
+      base::BindRepeating(&HandleClose,
+                          base::BindRepeating(&SecurityKeysPINHandler::Close,
+                                              base::Unretained(this))));
 }
 
-void SecurityKeysHandler::OnJavascriptAllowed() {}
-
-void SecurityKeysHandler::OnJavascriptDisallowed() {
-  // If Javascript is disallowed, |Close| will invalidate all current WeakPtrs
-  // and thus drop all pending callbacks. This means that
-  // |IsJavascriptAllowed| doesn't need to be tested before each callback
-  // because, if the callback into this object happened, then Javascript is
-  // allowed.
-  Close();
-}
-
-void SecurityKeysHandler::Close() {
+void SecurityKeysPINHandler::Close() {
   // Invalidate all existing WeakPtrs so that no stale callbacks occur.
-  weak_factory_ =
-      std::make_unique<base::WeakPtrFactory<SecurityKeysHandler>>(this);
+  weak_factory_.InvalidateWeakPtrs();
   state_ = State::kNone;
   set_pin_.reset();
-  reset_.reset();
-  discovery_factory_.reset();
-  credential_management_.reset();
   callback_id_.clear();
-  credential_management_provide_pin_cb_.Reset();
-  DCHECK(!credential_management_provide_pin_cb_);
 }
 
-void SecurityKeysHandler::HandleStartSetPIN(const base::ListValue* args) {
+void SecurityKeysPINHandler::HandleStartSetPIN(const base::ListValue* args) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(State::kNone, state_);
   DCHECK_EQ(1u, args->GetSize());
@@ -119,13 +93,13 @@ void SecurityKeysHandler::HandleStartSetPIN(const base::ListValue* args) {
   state_ = State::kStartSetPIN;
   set_pin_ = std::make_unique<device::SetPINRequestHandler>(
       content::GetSystemConnector(), supported_transports(),
-      base::BindOnce(&SecurityKeysHandler::OnGatherPIN,
-                     weak_factory_->GetWeakPtr()),
-      base::BindRepeating(&SecurityKeysHandler::OnSetPINComplete,
-                          weak_factory_->GetWeakPtr()));
+      base::BindOnce(&SecurityKeysPINHandler::OnGatherPIN,
+                     weak_factory_.GetWeakPtr()),
+      base::BindRepeating(&SecurityKeysPINHandler::OnSetPINComplete,
+                          weak_factory_.GetWeakPtr()));
 }
 
-void SecurityKeysHandler::OnGatherPIN(base::Optional<int64_t> num_retries) {
+void SecurityKeysPINHandler::OnGatherPIN(base::Optional<int64_t> num_retries) {
   DCHECK_EQ(State::kStartSetPIN, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -143,7 +117,7 @@ void SecurityKeysHandler::OnGatherPIN(base::Optional<int64_t> num_retries) {
                             base::Value(std::move(list)));
 }
 
-void SecurityKeysHandler::OnSetPINComplete(
+void SecurityKeysPINHandler::OnSetPINComplete(
     device::CtapDeviceResponseCode code) {
   DCHECK(state_ == State::kStartSetPIN || state_ == State::kSettingPIN);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -163,7 +137,7 @@ void SecurityKeysHandler::OnSetPINComplete(
                             base::Value(std::move(list)));
 }
 
-void SecurityKeysHandler::HandleSetPIN(const base::ListValue* args) {
+void SecurityKeysPINHandler::HandleSetPIN(const base::ListValue* args) {
   DCHECK(state_ == State::kGatherNewPIN || state_ == State::kGatherChangePIN);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(3u, args->GetSize());
@@ -180,7 +154,34 @@ void SecurityKeysHandler::HandleSetPIN(const base::ListValue* args) {
   set_pin_->ProvidePIN(old_pin, new_pin);
 }
 
-void SecurityKeysHandler::HandleReset(const base::ListValue* args) {
+SecurityKeysResetHandler::SecurityKeysResetHandler() = default;
+SecurityKeysResetHandler::~SecurityKeysResetHandler() = default;
+
+void SecurityKeysResetHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      "securityKeyReset",
+      base::BindRepeating(&SecurityKeysResetHandler::HandleReset,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCompleteReset",
+      base::BindRepeating(&SecurityKeysResetHandler::HandleCompleteReset,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyResetClose",
+      base::BindRepeating(&HandleClose,
+                          base::BindRepeating(&SecurityKeysResetHandler::Close,
+                                              base::Unretained(this))));
+}
+
+void SecurityKeysResetHandler::Close() {
+  // Invalidate all existing WeakPtrs so that no stale callbacks occur.
+  weak_factory_.InvalidateWeakPtrs();
+  state_ = State::kNone;
+  reset_.reset();
+  callback_id_.clear();
+}
+
+void SecurityKeysResetHandler::HandleReset(const base::ListValue* args) {
   DCHECK_EQ(State::kNone, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(1u, args->GetSize());
@@ -192,13 +193,13 @@ void SecurityKeysHandler::HandleReset(const base::ListValue* args) {
   state_ = State::kStartReset;
   reset_ = std::make_unique<device::ResetRequestHandler>(
       content::GetSystemConnector(), supported_transports(),
-      base::BindOnce(&SecurityKeysHandler::OnResetSent,
-                     weak_factory_->GetWeakPtr()),
-      base::BindOnce(&SecurityKeysHandler::OnResetFinished,
-                     weak_factory_->GetWeakPtr()));
+      base::BindOnce(&SecurityKeysResetHandler::OnResetSent,
+                     weak_factory_.GetWeakPtr()),
+      base::BindOnce(&SecurityKeysResetHandler::OnResetFinished,
+                     weak_factory_.GetWeakPtr()));
 }
 
-void SecurityKeysHandler::OnResetSent() {
+void SecurityKeysResetHandler::OnResetSent() {
   DCHECK_EQ(State::kStartReset, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -210,7 +211,8 @@ void SecurityKeysHandler::OnResetSent() {
                             base::Value(0 /* success */));
 }
 
-void SecurityKeysHandler::HandleCompleteReset(const base::ListValue* args) {
+void SecurityKeysResetHandler::HandleCompleteReset(
+    const base::ListValue* args) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(1u, args->GetSize());
 
@@ -238,7 +240,7 @@ void SecurityKeysHandler::HandleCompleteReset(const base::ListValue* args) {
   }
 }
 
-void SecurityKeysHandler::OnResetFinished(
+void SecurityKeysResetHandler::OnResetFinished(
     device::CtapDeviceResponseCode result) {
   switch (state_) {
     case State::kWaitingForResetNoCallbackYet:
@@ -267,8 +269,46 @@ void SecurityKeysHandler::OnResetFinished(
   }
 }
 
-void SecurityKeysHandler::HandleCredentialManagement(
-    const base::ListValue* args) {
+SecurityKeysCredentialHandler::SecurityKeysCredentialHandler() = default;
+SecurityKeysCredentialHandler::~SecurityKeysCredentialHandler() = default;
+
+void SecurityKeysCredentialHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCredentialManagementStart",
+      base::BindRepeating(&SecurityKeysCredentialHandler::HandleStart,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCredentialManagementPIN",
+      base::BindRepeating(&SecurityKeysCredentialHandler::HandlePIN,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCredentialManagementEnumerate",
+      base::BindRepeating(&SecurityKeysCredentialHandler::HandleEnumerate,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCredentialManagementDelete",
+      base::BindRepeating(&SecurityKeysCredentialHandler::HandleDelete,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "securityKeyCredentialManagementClose",
+      base::BindRepeating(
+          &HandleClose,
+          base::BindRepeating(&SecurityKeysCredentialHandler::Close,
+                              base::Unretained(this))));
+}
+
+void SecurityKeysCredentialHandler::Close() {
+  // Invalidate all existing WeakPtrs so that no stale callbacks occur.
+  weak_factory_.InvalidateWeakPtrs();
+  state_ = State::kNone;
+  discovery_factory_.reset();
+  credential_management_.reset();
+  callback_id_.clear();
+  credential_management_provide_pin_cb_.Reset();
+  DCHECK(!credential_management_provide_pin_cb_);
+}
+
+void SecurityKeysCredentialHandler::HandleStart(const base::ListValue* args) {
   DCHECK_EQ(State::kNone, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(1u, args->GetSize());
@@ -279,24 +319,23 @@ void SecurityKeysHandler::HandleCredentialManagement(
   DCHECK(callback_id_.empty());
   callback_id_ = args->GetList()[0].GetString();
 
-  state_ = State::kCredentialManagementStart;
+  state_ = State::kStart;
   discovery_factory_ = std::make_unique<device::FidoDiscoveryFactory>();
   credential_management_ =
       std::make_unique<device::CredentialManagementHandler>(
           content::ServiceManagerConnection::GetForProcess()->GetConnector(),
           discovery_factory_.get(), supported_transports(),
-          base::BindOnce(&SecurityKeysHandler::OnCredentialManagementReady,
-                         weak_factory_->GetWeakPtr()),
-          base::BindRepeating(
-              &SecurityKeysHandler::OnCredentialManagementGatherPIN,
-              weak_factory_->GetWeakPtr()),
-          base::BindOnce(&SecurityKeysHandler::OnCredentialManagementFinished,
-                         weak_factory_->GetWeakPtr()));
+          base::BindOnce(
+              &SecurityKeysCredentialHandler::OnCredentialManagementReady,
+              weak_factory_.GetWeakPtr()),
+          base::BindRepeating(&SecurityKeysCredentialHandler::OnGatherPIN,
+                              weak_factory_.GetWeakPtr()),
+          base::BindOnce(&SecurityKeysCredentialHandler::OnFinished,
+                         weak_factory_.GetWeakPtr()));
 }
 
-void SecurityKeysHandler::HandleCredentialManagementPIN(
-    const base::ListValue* args) {
-  DCHECK_EQ(State::kCredentialManagementPIN, state_);
+void SecurityKeysCredentialHandler::HandlePIN(const base::ListValue* args) {
+  DCHECK_EQ(State::kPIN, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(2u, args->GetSize());
   DCHECK(credential_management_);
@@ -309,29 +348,29 @@ void SecurityKeysHandler::HandleCredentialManagementPIN(
   std::move(credential_management_provide_pin_cb_).Run(pin);
 }
 
-void SecurityKeysHandler::HandleCredentialManagementEnumerate(
+void SecurityKeysCredentialHandler::HandleEnumerate(
     const base::ListValue* args) {
-  DCHECK_EQ(state_, State::kCredentialManagementReady);
+  DCHECK_EQ(state_, State::kReady);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(1u, args->GetSize());
   DCHECK(credential_management_);
   DCHECK(callback_id_.empty());
 
-  state_ = State::kCredentialManagementGettingCredentials;
+  state_ = State::kGettingCredentials;
   callback_id_ = args->GetList()[0].GetString();
-  credential_management_->GetCredentials(base::BindOnce(
-      &SecurityKeysHandler::OnHaveCredentials, weak_factory_->GetWeakPtr()));
+  credential_management_->GetCredentials(
+      base::BindOnce(&SecurityKeysCredentialHandler::OnHaveCredentials,
+                     weak_factory_.GetWeakPtr()));
 }
 
-void SecurityKeysHandler::HandleCredentialManagementDelete(
-    const base::ListValue* args) {
-  DCHECK_EQ(State::kCredentialManagementReady, state_);
+void SecurityKeysCredentialHandler::HandleDelete(const base::ListValue* args) {
+  DCHECK_EQ(State::kReady, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(2u, args->GetSize());
   DCHECK(credential_management_);
   DCHECK(callback_id_.empty());
 
-  state_ = State::kCredentialManagementDeleting;
+  state_ = State::kDeletingCredentials;
   callback_id_ = args->GetList()[0].GetString();
   std::vector<std::vector<uint8_t>> credential_ids;
   for (const base::Value& el : args->GetList()[1].GetList()) {
@@ -344,40 +383,38 @@ void SecurityKeysHandler::HandleCredentialManagementDelete(
   }
   credential_management_->DeleteCredentials(
       std::move(credential_ids),
-      base::BindOnce(&SecurityKeysHandler::OnCredentialsDeleted,
-                     weak_factory_->GetWeakPtr()));
+      base::BindOnce(&SecurityKeysCredentialHandler::OnCredentialsDeleted,
+                     weak_factory_.GetWeakPtr()));
 }
-void SecurityKeysHandler::OnCredentialManagementReady() {
-  DCHECK(state_ == State::kCredentialManagementStart ||
-         state_ == State::kCredentialManagementPIN);
+void SecurityKeysCredentialHandler::OnCredentialManagementReady() {
+  DCHECK(state_ == State::kStart || state_ == State::kPIN);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(credential_management_);
   DCHECK(!callback_id_.empty());
 
-  state_ = State::kCredentialManagementReady;
+  state_ = State::kReady;
   ResolveJavascriptCallback(base::Value(std::move(callback_id_)),
                             base::Value());
 }
 
-void SecurityKeysHandler::OnHaveCredentials(
+void SecurityKeysCredentialHandler::OnHaveCredentials(
     device::CtapDeviceResponseCode status,
     base::Optional<std::vector<device::AggregatedEnumerateCredentialsResponse>>
         responses,
     base::Optional<size_t> remaining_credentials) {
-  DCHECK_EQ(State::kCredentialManagementGettingCredentials, state_);
+  DCHECK_EQ(State::kGettingCredentials, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(credential_management_);
   DCHECK(!callback_id_.empty());
 
   if (status != device::CtapDeviceResponseCode::kSuccess) {
-    OnCredentialManagementFinished(
-        device::FidoReturnCode::kAuthenticatorResponseInvalid);
+    OnFinished(device::FidoReturnCode::kAuthenticatorResponseInvalid);
     return;
   }
   DCHECK(responses);
   DCHECK(remaining_credentials);
 
-  state_ = State::kCredentialManagementReady;
+  state_ = State::kReady;
 
   base::Value::ListStorage credentials;
   for (const auto& response : *responses) {
@@ -403,7 +440,7 @@ void SecurityKeysHandler::OnHaveCredentials(
                             base::ListValue(std::move(credentials)));
 }
 
-void SecurityKeysHandler::OnCredentialManagementGatherPIN(
+void SecurityKeysCredentialHandler::OnGatherPIN(
     int64_t num_retries,
     base::OnceCallback<void(std::string)> callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -412,29 +449,29 @@ void SecurityKeysHandler::OnCredentialManagementGatherPIN(
 
   credential_management_provide_pin_cb_ = std::move(callback);
 
-  if (state_ == State::kCredentialManagementStart) {
+  if (state_ == State::kStart) {
     // Resolve the promise to startCredentialManagement().
-    state_ = State::kCredentialManagementPIN;
+    state_ = State::kPIN;
     ResolveJavascriptCallback(base::Value(std::move(callback_id_)),
                               base::Value());
     return;
   }
 
   // Resolve the promise to credentialManagementProvidePIN().
-  DCHECK_EQ(state_, State::kCredentialManagementPIN);
+  DCHECK_EQ(state_, State::kPIN);
   ResolveJavascriptCallback(base::Value(std::move(callback_id_)),
                             base::Value(static_cast<int>(num_retries)));
 }
 
-void SecurityKeysHandler::OnCredentialsDeleted(
+void SecurityKeysCredentialHandler::OnCredentialsDeleted(
     device::CtapDeviceResponseCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(State::kCredentialManagementDeleting, state_);
+  DCHECK_EQ(State::kDeletingCredentials, state_);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(credential_management_);
   DCHECK(!callback_id_.empty());
 
-  state_ = State::kCredentialManagementReady;
+  state_ = State::kReady;
 
   ResolveJavascriptCallback(
       base::Value(std::move(callback_id_)),
@@ -444,8 +481,7 @@ void SecurityKeysHandler::OnCredentialsDeleted(
               : IDS_SETTINGS_SECURITY_KEYS_CREDENTIAL_MANAGEMENT_FAILED)));
 }
 
-void SecurityKeysHandler::OnCredentialManagementFinished(
-    device::FidoReturnCode status) {
+void SecurityKeysCredentialHandler::OnFinished(device::FidoReturnCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   int error;
@@ -479,9 +515,4 @@ void SecurityKeysHandler::OnCredentialManagementFinished(
                     base::Value(l10n_util::GetStringUTF8(std::move(error))));
 }
 
-void SecurityKeysHandler::HandleClose(const base::ListValue* args) {
-  DCHECK_EQ(0u, args->GetSize());
-  Close();
-}
-
 }  // namespace settings

chrome/browser/ui/webui/settings/settings_security_key_handler.h

@@ -28,59 +28,118 @@ class ResetRequestHandler;
 
 namespace settings {
 
-// SecurityKeysHandler processes messages from the "Security Keys" section of a
-// settings page. An instance of this class is created for each settings tab and
-// is destroyed when the tab is closed. See the comments in
-// security_keys_browser_proxy.js about the interface.
-class SecurityKeysHandler : public SettingsPageUIHandler {
- public:
-  SecurityKeysHandler();
-  ~SecurityKeysHandler() override;
+// Base class for message handlers on the "Security Keys" settings subpage.
+class SecurityKeysHandlerBase : public SettingsPageUIHandler {
+ protected:
+  SecurityKeysHandlerBase() = default;
 
-  void RegisterMessages() override;
+  // Subclasses must implement close to invalidate all pending callbacks.
+  virtual void Close() = 0;
+
+ private:
   void OnJavascriptAllowed() override;
   void OnJavascriptDisallowed() override;
 
+  SecurityKeysHandlerBase(const SecurityKeysHandlerBase&) = delete;
+  SecurityKeysHandlerBase& operator=(const SecurityKeysHandlerBase&) = delete;
+};
+
+// SecurityKeysPINHandler processes messages from the "Create a PIN" dialog of
+// the "Security Keys" settings subpage. An instance of this class is created
+// for each settings tab and is destroyed when the tab is closed. See
+// SecurityKeysPINBrowserProxy about the interface.
+class SecurityKeysPINHandler : public SecurityKeysHandlerBase {
+ public:
+  SecurityKeysPINHandler();
+  ~SecurityKeysPINHandler() override;
+
  private:
   enum class State {
     kNone,
-
     kStartSetPIN,
     kGatherNewPIN,
     kGatherChangePIN,
     kSettingPIN,
-
-    kStartReset,
-    kWaitingForResetNoCallbackYet,
-    kWaitingForResetHaveCallback,
-    kWaitingForCompleteReset,
-
-    kCredentialManagementStart,
-    kCredentialManagementPIN,
-    kCredentialManagementReady,
-    kCredentialManagementGettingCredentials,
-    kCredentialManagementDeleting,
   };
 
-  void Close();
+  void RegisterMessages() override;
+  void Close() override;
 
-  // PIN
   void HandleStartSetPIN(const base::ListValue* args);
   void OnGatherPIN(base::Optional<int64_t> num_retries);
   void OnSetPINComplete(device::CtapDeviceResponseCode code);
   void HandleSetPIN(const base::ListValue* args);
 
-  // Reset
+  State state_ = State::kNone;
+
+  std::unique_ptr<device::SetPINRequestHandler> set_pin_;
+
+  std::string callback_id_;
+  base::WeakPtrFactory<SecurityKeysPINHandler> weak_factory_{this};
+};
+
+// SecurityKeysResetHandler processes messages from the "Reset your Security
+// Key" dialog of the "Security Keys" settings subpage. An instance of this
+// class is created for each settings tab and is destroyed when the tab is
+// closed. See SecurityKeysResetBrowserProxy about the interface.
+class SecurityKeysResetHandler : public SecurityKeysHandlerBase {
+ public:
+  SecurityKeysResetHandler();
+  ~SecurityKeysResetHandler() override;
+
+ private:
+  enum class State {
+    kNone,
+    kStartReset,
+    kWaitingForResetNoCallbackYet,
+    kWaitingForResetHaveCallback,
+    kWaitingForCompleteReset,
+  };
+
+  void RegisterMessages() override;
+  void Close() override;
+
   void HandleReset(const base::ListValue* args);
   void OnResetSent();
   void HandleCompleteReset(const base::ListValue* args);
   void OnResetFinished(device::CtapDeviceResponseCode result);
 
-  // Credential Management
-  void HandleCredentialManagement(const base::ListValue* args);
-  void HandleCredentialManagementPIN(const base::ListValue* args);
-  void HandleCredentialManagementEnumerate(const base::ListValue* args);
-  void HandleCredentialManagementDelete(const base::ListValue* args);
+  State state_ = State::kNone;
+
+  std::unique_ptr<device::ResetRequestHandler> reset_;
+  base::Optional<device::CtapDeviceResponseCode> reset_result_;
+
+  std::string callback_id_;
+  base::WeakPtrFactory<SecurityKeysResetHandler> weak_factory_{this};
+};
+
+// SecurityKeysCredentialHandler processes messages from the "Manage
+// sign-in data" dialog of the "Security Keys" settings subpage. An instance of
+// this class is created for each settings tab and is destroyed when the tab is
+// closed. See SecurityKeysCredentialBrowserProxy about the interface.
+class SecurityKeysCredentialHandler : public SecurityKeysHandlerBase {
+ public:
+  SecurityKeysCredentialHandler();
+  ~SecurityKeysCredentialHandler() override;
+
+ private:
+  enum class State {
+    kNone,
+    kStart,
+    kPIN,
+    kReady,
+    kGettingCredentials,
+    kDeletingCredentials,
+  };
+
+  void RegisterMessages() override;
+  void Close() override;
+
+  void HandleStart(const base::ListValue* args);
+  void HandlePIN(const base::ListValue* args);
+  void HandleEnumerate(const base::ListValue* args);
+  void HandleDelete(const base::ListValue* args);
+
   void OnCredentialManagementReady();
   void OnHaveCredentials(
       device::CtapDeviceResponseCode status,
@@ -88,24 +147,18 @@ class SecurityKeysHandler : public SettingsPageUIHandler {
           std::vector<device::AggregatedEnumerateCredentialsResponse>>
           responses,
       base::Optional<size_t> remaining_credentials);
-  void OnCredentialManagementGatherPIN(int64_t num_retries,
-                                       base::OnceCallback<void(std::string)>);
+  void OnGatherPIN(int64_t num_retries, base::OnceCallback<void(std::string)>);
   void OnCredentialsDeleted(device::CtapDeviceResponseCode status);
-  void OnCredentialManagementFinished(device::FidoReturnCode status);
+  void OnFinished(device::FidoReturnCode status);
 
-  void HandleClose(const base::ListValue* args);
-
-  State state_;
+  State state_ = State::kNone;
   base::OnceCallback<void(std::string)> credential_management_provide_pin_cb_;
+
   std::unique_ptr<device::FidoDiscoveryFactory> discovery_factory_;
-  std::unique_ptr<device::SetPINRequestHandler> set_pin_;
   std::unique_ptr<device::CredentialManagementHandler> credential_management_;
-  std::unique_ptr<device::ResetRequestHandler> reset_;
-  base::Optional<device::CtapDeviceResponseCode> reset_result_;
-  std::string callback_id_;
-  std::unique_ptr<base::WeakPtrFactory<SecurityKeysHandler>> weak_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(SecurityKeysHandler);
+  std::string callback_id_;
+  base::WeakPtrFactory<SecurityKeysCredentialHandler> weak_factory_{this};
 };
 
 }  // namespace settings

chrome/browser/ui/webui/settings/settings_ui.cc

@@ -230,7 +230,9 @@ SettingsUI::SettingsUI(content::WebUI* web_ui)
   AddSettingsPageUIHandler(std::make_unique<SearchEnginesHandler>(profile));
   AddSettingsPageUIHandler(std::make_unique<SiteSettingsHandler>(profile));
   AddSettingsPageUIHandler(std::make_unique<StartupPagesHandler>(web_ui));
-  AddSettingsPageUIHandler(std::make_unique<SecurityKeysHandler>());
+  AddSettingsPageUIHandler(std::make_unique<SecurityKeysPINHandler>());
+  AddSettingsPageUIHandler(std::make_unique<SecurityKeysResetHandler>());
+  AddSettingsPageUIHandler(std::make_unique<SecurityKeysCredentialHandler>());
 
 #if defined(OS_WIN) || defined(OS_MACOSX)
   AddSettingsPageUIHandler(std::make_unique<CaptionsHandler>());

chrome/test/data/webui/settings/security_keys_subpage_test.js

@@ -2,18 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-/** @implements {settings.SecurityKeysBrowserProxy} */
-class TestSecurityKeysBrowserProxy extends TestBrowserProxy {
+/** @implements {settings.SecurityKeysPINBrowserProxy} */
+class TestSecurityKeysPINBrowserProxy extends TestBrowserProxy {
   constructor() {
     super([
       'startSetPIN',
       'setPIN',
-      'reset',
-      'completeReset',
-      'startCredentialManagement',
-      'credentialManagementProvidePIN',
-      'credentialManagementEnumerate',
-      'credentialManagementDeleteCredentials',
       'close',
     ]);
 
@@ -62,6 +56,56 @@ class TestSecurityKeysBrowserProxy extends TestBrowserProxy {
     return this.handleMethod_('setPIN', {oldPIN, newPIN});
   }
 
+  /** @override */
+  close() {
+    this.methodCalled('close');
+  }
+}
+
+/** @implements {settings.SecurityKeysResetBrowserProxy} */
+class TestSecurityKeysResetBrowserProxy extends TestBrowserProxy {
+  constructor() {
+    super([
+      'reset',
+      'completeReset',
+      'close',
+    ]);
+
+    /**
+     * A map from method names to a promise to return when that method is
+     * called. (If no promise is installed, a never-resolved promise is
+     * returned.)
+     * @private {!Map<string, !Promise>}
+     */
+    this.promiseMap_ = new Map();
+  }
+
+  /**
+   * @param {string} methodName
+   * @param {!Promise} promise
+   */
+  setResponseFor(methodName, promise) {
+    this.promiseMap_.set(methodName, promise);
+  }
+
+  /**
+   * @param {string} methodName
+   * @param {*} opt_arg
+   * @return {!Promise}
+   * @private
+   */
+  handleMethod_(methodName, opt_arg) {
+    this.methodCalled(methodName, opt_arg);
+    const promise = this.promiseMap_.get(methodName);
+    if (promise != undefined) {
+      this.promiseMap_.delete(methodName);
+      return promise;
+    }
+
+    // Return a Promise that never resolves.
+    return new Promise(() => {});
+  }
+
   /** @override */
   reset() {
     return this.handleMethod_('reset');
@@ -72,24 +116,76 @@ class TestSecurityKeysBrowserProxy extends TestBrowserProxy {
     return this.handleMethod_('completeReset');
   }
 
+  /** @override */
+  close() {
+    this.methodCalled('close');
+  }
+}
+
+/** @implements {settings.SecurityKeysCredentialBrowserProxy} */
+class TestSecurityKeysCredentialBrowserProxy extends TestBrowserProxy {
+  constructor() {
+    super([
+      'startCredentialManagement',
+      'providePIN',
+      'enumerateCredentials',
+      'deleteCredentials',
+      'close',
+    ]);
+
+    /**
+     * A map from method names to a promise to return when that method is
+     * called. (If no promise is installed, a never-resolved promise is
+     * returned.)
+     * @private {!Map<string, !Promise>}
+     */
+    this.promiseMap_ = new Map();
+  }
+
+  /**
+   * @param {string} methodName
+   * @param {!Promise} promise
+   */
+  setResponseFor(methodName, promise) {
+    this.promiseMap_.set(methodName, promise);
+  }
+
+  /**
+   * @param {string} methodName
+   * @param {*} opt_arg
+   * @return {!Promise}
+   * @private
+   */
+  handleMethod_(methodName, opt_arg) {
+    this.methodCalled(methodName, opt_arg);
+    const promise = this.promiseMap_.get(methodName);
+    if (promise != undefined) {
+      this.promiseMap_.delete(methodName);
+      return promise;
+    }
+
+    // Return a Promise that never resolves.
+    return new Promise(() => {});
+  }
+
   /** @override */
   startCredentialManagement() {
     return this.handleMethod_('startCredentialManagement');
   }
 
   /** @override */
-  credentialManagementProvidePIN(pin) {
-    return this.handleMethod_('credentialManagementProvidePIN', pin);
+  providePIN(pin) {
+    return this.handleMethod_('providePIN', pin);
   }
 
   /** @override */
-  credentialManagementEnumerate() {
-    return this.handleMethod_('credentialManagementEnumerate');
+  enumerateCredentials() {
+    return this.handleMethod_('enumerateCredentials');
   }
 
   /** @override */
-  credentialManagementDeleteCredentials(ids) {
-    return this.handleMethod_('credentialManagementDeleteCredentials', ids);
+  deleteCredentials(ids) {
+    return this.handleMethod_('deleteCredentials', ids);
   }
 
   /** @override */
@@ -102,8 +198,8 @@ suite('SecurityKeysResetDialog', function() {
   let dialog = null;
 
   setup(function() {
-    browserProxy = new TestSecurityKeysBrowserProxy();
-    settings.SecurityKeysBrowserProxyImpl.instance_ = browserProxy;
+    browserProxy = new TestSecurityKeysResetBrowserProxy();
+    settings.SecurityKeysResetBrowserProxyImpl.instance_ = browserProxy;
     PolymerTest.clearBody();
     dialog = document.createElement('settings-security-keys-reset-dialog');
   });
@@ -221,8 +317,8 @@ suite('SecurityKeysSetPINDialog', function() {
   let dialog = null;
 
   setup(function() {
-    browserProxy = new TestSecurityKeysBrowserProxy();
-    settings.SecurityKeysBrowserProxyImpl.instance_ = browserProxy;
+    browserProxy = new TestSecurityKeysPINBrowserProxy();
+    settings.SecurityKeysPINBrowserProxyImpl.instance_ = browserProxy;
     PolymerTest.clearBody();
     dialog = document.createElement('settings-security-keys-set-pin-dialog');
   });
@@ -476,8 +572,8 @@ suite('SecurityKeysCredentialManagement', function() {
   let dialog = null;
 
   setup(function() {
-    browserProxy = new TestSecurityKeysBrowserProxy();
-    settings.SecurityKeysBrowserProxyImpl.instance_ = browserProxy;
+    browserProxy = new TestSecurityKeysCredentialBrowserProxy();
+    settings.SecurityKeysCredentialBrowserProxyImpl.instance_ = browserProxy;
     PolymerTest.clearBody();
     dialog = document.createElement(
         'settings-security-keys-credential-management-dialog');
@@ -530,14 +626,12 @@ suite('SecurityKeysCredentialManagement', function() {
     browserProxy.setResponseFor(
         'startCredentialManagement', startCredentialManagementResolver.promise);
     const pinResolver = new PromiseResolver();
-    browserProxy.setResponseFor(
-        'credentialManagementProvidePIN', pinResolver.promise);
+    browserProxy.setResponseFor('providePIN', pinResolver.promise);
     const enumerateResolver = new PromiseResolver();
     browserProxy.setResponseFor(
-        'credentialManagementEnumerate', enumerateResolver.promise);
+        'enumerateCredentials', enumerateResolver.promise);
     const deleteResolver = new PromiseResolver();
-    browserProxy.setResponseFor(
-        'credentialManagementDeleteCredentials', deleteResolver.promise);
+    browserProxy.setResponseFor('deleteCredentials', deleteResolver.promise);
 
     document.body.appendChild(dialog);
     await browserProxy.whenCalled('startCredentialManagement');
@@ -551,12 +645,12 @@ suite('SecurityKeysCredentialManagement', function() {
     assertShown('pinPrompt');
     dialog.$.pin.value = '0000';
     dialog.$.confirmButton.click();
-    const pin = await browserProxy.whenCalled('credentialManagementProvidePIN');
+    const pin = await browserProxy.whenCalled('providePIN');
     assertEquals(pin, '0000');
 
     // Show a list of three credentials.
     pinResolver.resolve();
-    await browserProxy.whenCalled('credentialManagementEnumerate');
+    await browserProxy.whenCalled('enumerateCredentials');
     uiReady = test_util.eventToPromise(
         'credential-management-dialog-ready-for-testing', dialog);
     const credentials = [
@@ -596,8 +690,7 @@ suite('SecurityKeysCredentialManagement', function() {
     assertFalse(dialog.$.confirmButton.disabled);
 
     dialog.$.confirmButton.click();
-    const credentialIds =
-        await browserProxy.whenCalled('credentialManagementDeleteCredentials');
+    const credentialIds = await browserProxy.whenCalled('deleteCredentials');
     assertDeepEquals(credentialIds, ['bbbbbb', 'cccccc']);
     uiReady = test_util.eventToPromise(
         'credential-management-dialog-ready-for-testing', dialog);