NSS: Unlock crypto devices when populating cert manager.

chrome/browser/ui/crypto_module_password_dialog_nss.cc

Index: chrome/browser/ui/crypto_module_password_dialog_nss.cc
diff --git a/chrome/browser/ui/crypto_module_password_dialog_nss.cc b/chrome/browser/ui/crypto_module_password_dialog_nss.cc
index d0b1357dc2c5379033ecbd48e4c91f56555fc6fb..268b08b50a48061308e48658c25ce032ea714955 100644
--- a/chrome/browser/ui/crypto_module_password_dialog_nss.cc
+++ b/chrome/browser/ui/crypto_module_password_dialog_nss.cc
@@ -13,12 +13,18 @@
 
 namespace {
 
+bool ShouldShowDialog(const net::CryptoModule* module) {
+  // The wincx arg is unused since we don't call PK11_SetIsLoggedInFunc.
+  return (PK11_NeedLogin(module->os_module_handle()) &&
+          !PK11_IsLoggedIn(module->os_module_handle(), NULL /* wincx */));
+}
+
 // Basically an asynchronous implementation of NSS's PK11_DoPassword.
 // Note: This currently handles only the simple case.  See the TODOs in
 // GotPassword for what is yet unimplemented.
 class SlotUnlocker {
  public:
-  SlotUnlocker(net::CryptoModule* module,
+  SlotUnlocker(const net::CryptoModuleList& modules,
                browser::CryptoModulePasswordReason reason,
                const std::string& host,
                Callback0::Type* callback);
@@ -29,18 +35,20 @@ class SlotUnlocker {
   void GotPassword(const char* password);
   void Done();
 
-  scoped_refptr<net::CryptoModule> module_;
+  size_t cur_;

[wtc, 2011/03/02 03:33:28]

Nit: the Style Guide seems to discourage this kind of abbreviation.  What
do you think?

[mattm, 2011/03/05 02:16:32]

yeah, changed to current_

+  net::CryptoModuleList modules_;
   browser::CryptoModulePasswordReason reason_;
   std::string host_;
   Callback0::Type* callback_;
   PRBool retry_;
 };
 
-SlotUnlocker::SlotUnlocker(net::CryptoModule* module,
+SlotUnlocker::SlotUnlocker(const net::CryptoModuleList& modules,
                            browser::CryptoModulePasswordReason reason,
                            const std::string& host,
                            Callback0::Type* callback)
-    : module_(module),
+    : cur_(0),
+      modules_(modules),
       reason_(reason),
       host_(host),
       callback_(callback),
@@ -51,12 +59,18 @@ SlotUnlocker::SlotUnlocker(net::CryptoModule* module,
 void SlotUnlocker::Start() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  ShowCryptoModulePasswordDialog(
-      module_->GetTokenName(),
-      retry_,
-      reason_,
-      host_,
-      NewCallback(this, &SlotUnlocker::GotPassword));
+  for (; cur_ < modules_.size(); ++cur_) {
+    if (ShouldShowDialog(modules_[cur_].get())) {
+      ShowCryptoModulePasswordDialog(
+          modules_[cur_]->GetTokenName(),
+          retry_,
+          reason_,
+          host_,
+          NewCallback(this, &SlotUnlocker::GotPassword));
+      return;
+    }
+  }
+  Done();
 }
 
 void SlotUnlocker::GotPassword(const char* password) {
@@ -66,12 +80,13 @@ void SlotUnlocker::GotPassword(const char* password) {
 
   if (!password) {
     // User cancelled entering password.  Oh well.
-    Done();
+    ++cur_;
+    Start();
     return;
   }
 
   // TODO(mattm): handle protectedAuthPath
-  SECStatus rv = PK11_CheckUserPassword(module_->os_module_handle(),
+  SECStatus rv = PK11_CheckUserPassword(modules_[cur_]->os_module_handle(),
                                         password);
   if (rv == SECWouldBlock) {
     // Incorrect password.  Try again.
@@ -84,11 +99,13 @@ void SlotUnlocker::GotPassword(const char* password) {
   // non-friendly slots.  How important is that?
 
   // Correct password (SECSuccess) or too many attempts/other failure
-  // (SECFailure).  Either way we're done.
-  Done();
+  // (SECFailure).  Either way we're done with this slot.
+  ++cur_;
+  Start();
 }
 
 void SlotUnlocker::Done() {
+  DCHECK_EQ(cur_, modules_.size());
   callback_->Run();
   delete this;
 }
@@ -97,27 +114,28 @@ void SlotUnlocker::Done() {
 
 namespace browser {
 
-void UnlockSlotIfNecessary(net::CryptoModule* module,
-                           browser::CryptoModulePasswordReason reason,
-                           const std::string& host,
-                           Callback0::Type* callback) {
+void UnlockSlotsIfNecessary(const net::CryptoModuleList& modules,
+                            browser::CryptoModulePasswordReason reason,
+                            const std::string& host,
+                            Callback0::Type* callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // The wincx arg is unused since we don't call PK11_SetIsLoggedInFunc.
-  if (PK11_NeedLogin(module->os_module_handle()) &&
-      !PK11_IsLoggedIn(module->os_module_handle(), NULL /* wincx */)) {
-    (new SlotUnlocker(module, reason, host, callback))->Start();
-  } else {
-    callback->Run();
+  for (size_t i = 0; i < modules.size(); ++i) {
+    if (ShouldShowDialog(modules[i].get())) {
+      (new SlotUnlocker(modules, reason, host, callback))->Start();
+      return;

[wtc, 2011/03/02 03:33:28]

BUG: this return statement causes us to not unlock the remaining slots.  Is this
wrong?

[mattm, 2011/03/05 02:16:32]

The SlotUnlocker will unlock all the slots, this loop is just checking whether
it's necessary to unlock anything.  I guess it could be more obvious to just
always create the unlocker and just let it finish immediately if there isn't any
work to do..

+    }
   }
+  callback->Run();
 }
 
 void UnlockCertSlotIfNecessary(net::X509Certificate* cert,
                                browser::CryptoModulePasswordReason reason,
                                const std::string& host,
                                Callback0::Type* callback) {
-  scoped_refptr<net::CryptoModule> module(net::CryptoModule::CreateFromHandle(
+  net::CryptoModuleList modules;
+  modules.push_back(net::CryptoModule::CreateFromHandle(
       cert->os_cert_handle()->slot));
-  UnlockSlotIfNecessary(module.get(), reason, host, callback);
+  UnlockSlotsIfNecessary(modules, reason, host, callback);
 }
 
 }  // namespace browser