NSS: Unlock crypto devices when populating cert manager.

NSS: Unlock crypto devices when populating cert manager.

BUG=42073
TEST=try to use cert manager with "unfriendly" device.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77024

[mattm, 2011-02-25 03:48:07]

With this I'm able to view certs on an Aladdin eToken Pro.  Exporting fails but
the card may just not allow it (Firefox can't export from it either).

Also if you've used the cert manager to unlock it, then it works for SSL client
auth.  Supporting unlocking before the client auth cert selection will be a bit
more work...

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

LGTM.  I have some comments and suggested changes below.  Also note the
comment marked with "BUG" below.  I am not sure if that's a bug.

I'm sorry about the delay in reviewing this CL.

http://codereview.chromium.org/6580058/diff/1012/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6383: <message
name="IDS_CRYPTO_MODULE_AUTH_DIALOG_TEXT_CERT_MANAGER" desc="The text in the
dialog that prompts for security device password when using the certificate
manager without taking a specific action (eg, just loading the cert manager).">
Why does loading the cert manager require logging into some security devices?

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog.h (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:21: typedef
std::vector<scoped_refptr<CryptoModule> > CryptoModuleList;
I hope we can avoid repeating this typedef in multiple headers.  Not a big deal
though.

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:32:
kCryptoModulePasswordCertManager,
Perhaps this and the associated string ID in the .grd file should be named
ListCerts rather than CertManager?

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:58: // called on UI thread.
Please update the comment.

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_nss.cc (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_nss.cc:38: size_t cur_;
Nit: the Style Guide seems to discourage this kind of abbreviation.  What
do you think?

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_nss.cc:125: return;
BUG: this return statement causes us to not unlock the remaining slots.  Is this
wrong?

http://codereview.chromium.org/6580058/diff/1012/content/browser/certificate_...
File content/browser/certificate_manager_model.h (right):

http://codereview.chromium.org/6580058/diff/1012/content/browser/certificate_...
content/browser/certificate_manager_model.h:101: void RefreshUnlocked();
Nit: document this function.  Why is it unlocked?  This class doesn't have a
Lock
member.

Update: after I looked at the .cc file, I realized "unlocked" means "slots
unlocked".

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:128: NULL);  // wincx
Please make sure it's OK to pass NULL as the wincx argument.  If not, we
should add a TODO comment.

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_opens...
File net/base/cert_database_openssl.cc (right):

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_opens...
net/base/cert_database_openssl.cc:51: NOTIMPLEMENTED();
Let's at least call modules->clear().

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

http://codereview.chromium.org/6580058/diff/1012/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6383: <message
name="IDS_CRYPTO_MODULE_AUTH_DIALOG_TEXT_CERT_MANAGER" desc="The text in the
dialog that prompts for security device password when using the certificate
manager without taking a specific action (eg, just loading the cert manager).">
On 2011/03/02 03:33:28, wtc wrote:
> Why does loading the cert manager require logging into some security devices?

Some devices require logging in to even list the certs.

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog.h (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:21: typedef
std::vector<scoped_refptr<CryptoModule> > CryptoModuleList;
On 2011/03/02 03:33:28, wtc wrote:
> I hope we can avoid repeating this typedef in multiple headers.  Not a big
deal
> though.

Yeah, it's a bit ugly.  Just a question of how far we want to go in forward
declaring things instead of including the headers..

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:32:
kCryptoModulePasswordCertManager,
On 2011/03/02 03:33:28, wtc wrote:
> Perhaps this and the associated string ID in the .grd file should be named
> ListCerts rather than CertManager?

Done.

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog.h:58: // called on UI thread.
On 2011/03/02 03:33:28, wtc wrote:
> Please update the comment.

Done.

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
File chrome/browser/ui/crypto_module_password_dialog_nss.cc (right):

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_nss.cc:38: size_t cur_;
On 2011/03/02 03:33:28, wtc wrote:
> Nit: the Style Guide seems to discourage this kind of abbreviation.  What
> do you think?

yeah, changed to current_

http://codereview.chromium.org/6580058/diff/1012/chrome/browser/ui/crypto_mod...
chrome/browser/ui/crypto_module_password_dialog_nss.cc:125: return;
On 2011/03/02 03:33:28, wtc wrote:
> BUG: this return statement causes us to not unlock the remaining slots.  Is
this
> wrong?

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..

http://codereview.chromium.org/6580058/diff/1012/content/browser/certificate_...
File content/browser/certificate_manager_model.h (right):

http://codereview.chromium.org/6580058/diff/1012/content/browser/certificate_...
content/browser/certificate_manager_model.h:101: void RefreshUnlocked();
On 2011/03/02 03:33:28, wtc wrote:
> Nit: document this function.  Why is it unlocked?  This class doesn't have a
> Lock
> member.
> 
> Update: after I looked at the .cc file, I realized "unlocked" means "slots
> unlocked".

Done.

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:128: NULL);  // wincx
On 2011/03/02 03:33:28, wtc wrote:
> Please make sure it's OK to pass NULL as the wincx argument.  If not, we
> should add a TODO comment.

Just double-checked.  It's only used if PK11_SetIsLoggedInFunc is used.  Added
comment.

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_opens...
File net/base/cert_database_openssl.cc (right):

http://codereview.chromium.org/6580058/diff/1012/net/base/cert_database_opens...
net/base/cert_database_openssl.cc:51: NOTIMPLEMENTED();
On 2011/03/02 03:33:28, wtc wrote:
> Let's at least call modules->clear().

Done.

[wtc, 2011-03-08 19:51:48]

LGTM!

http://codereview.chromium.org/6580058/diff/11001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6580058/diff/11001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6439: <message
name="IDS_CRYPTO_MODULE_AUTH_DIALOG_TEXT_LIST_CERTS" desc="The text in the
dialog that prompts for security device password when using the certificate
manager without taking a specific action (eg, just loading the cert manager).">
Nit: since the name of the message has changed to say
"LIST_CERTS", the description of the message should be updated
to say "listing certificates" instead of "loading the
cert manager".