Basic implentation of the client auth UI

A first implementation of the SSL client auth UI.This uses the Windows API that prompts the user for a cert.R=wtcBUG=http://crbug.com/318TEST=Visit a site that requires client auth. A dialog to select a certificate should be shown. Try selecting no cert. Try again this time select a cert.

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

[wtc, 2009-06-26 21:53:32]

LGTM!

http://codereview.chromium.org/147233/diff/1017/1018
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/147233/diff/1017/1018#newcode2822
Line 2822: <!-- SSL client auth certificate selection dialog -->
Since this will be used by translators, let's not abbreviate
"authentication" and "certificate" in the comments.

http://codereview.chromium.org/147233/diff/1017/1018#newcode2828
Line 2828: Select a certificate to authenticate to <ph
name="HOST_NAME">$1<ex>www.google.com</ex></ph>
"authenticate yourself"?  Is that too wordy?

http://codereview.chromium.org/147233/diff/1017/1019
File chrome/browser/ssl/ssl_client_auth_handler.cc (right):

http://codereview.chromium.org/147233/diff/1017/1019#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Nit: 2009.

http://codereview.chromium.org/147233/diff/1017/1019#newcode8
Line 8: #include "Cryptuiapi.h"
Nit: lowercase and with <>:
  #include <cryptuiapi.h>

Please add
  #pragma comment(lib, "cryptui.lib")

http://codereview.chromium.org/147233/diff/1017/1019#newcode41
Line 41: if (!request_)  // We have been canceled.
Can this happen in this method?!

http://codereview.chromium.org/147233/diff/1017/1019#newcode44
Line 44: if (cert_request_info_->client_certs.empty()) {
Perhaps we should do this in ResourceDispatcherHost::OnCertificateRequested
so that we don't even need to create an SSLClientAuthHandler
in this case.

http://codereview.chromium.org/147233/diff/1017/1019#newcode64
Line 64: // Windows messages), which is scarry.
Typo: scary.

http://codereview.chromium.org/147233/diff/1017/1019#newcode80
Line 80: std::wstring title =
l10n_util::GetString(IDS_CLIENT_CERT_DIALOG_TITLE);
Since our title is the same as the default title used by
Windows, perhaps we should just pass NULL.  This way
translators don't need to translate the title string.

Or are you preparing for the future when we have our own
cert selection dialog?

http://codereview.chromium.org/147233/diff/1017/1019#newcode82
Line 82: ASCIIToWide(url_.host()));
Please use cert_request_info_->host_and_port instead.

http://codereview.chromium.org/147233/diff/1017/1020
File chrome/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/147233/diff/1017/1020#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Nit: 2009.

http://codereview.chromium.org/147233/diff/1017/1020#newcode14
Line 14: class SSLCertRequestInfo;
This forward declaration is redundent with the
"net/base/ssl_cert_request_info.h" inclusion above.  Please
pick one.

http://codereview.chromium.org/147233/diff/1017/1020#newcode20
Line 20: // This class handles the prompting of a certificate for SSL client
Nit: "prompting" => "selection" ?

Perhaps we should also mention the "approval" aspect.

http://codereview.chromium.org/147233/diff/1017/1020#newcode32
Line 32: // Prompts the user for a certificate and resumes the URL request with
"Asks the user to select a certificate"?

http://codereview.chromium.org/147233/diff/1017/1020#newcode35
Line 35: void QueryCertificate();
Should we name this method SelectCertificate?

http://codereview.chromium.org/147233/diff/1017/1020#newcode54
Line 54: GURL url_;
We don't need this because cert_request_info_ contains
the host_and_port we need for the UI.

[jcampan, 2009-06-26 22:35:35]

I also removed the temporary auto-ssl-client-auth switch which is not user
anymore.

http://codereview.chromium.org/147233/diff/1017/1018
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/147233/diff/1017/1018#newcode2822
Line 2822: <!-- SSL client auth certificate selection dialog -->
On 2009/06/26 21:53:32, wtc wrote:
> Since this will be used by translators, let's not abbreviate
> "authentication" and "certificate" in the comments.

Done.

http://codereview.chromium.org/147233/diff/1017/1018#newcode2828
Line 2828: Select a certificate to authenticate to <ph
name="HOST_NAME">$1<ex>www.google.com</ex></ph>
On 2009/06/26 21:53:32, wtc wrote:
> "authenticate yourself"?  Is that too wordy?

Done.

http://codereview.chromium.org/147233/diff/1017/1019
File chrome/browser/ssl/ssl_client_auth_handler.cc (right):

http://codereview.chromium.org/147233/diff/1017/1019#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/06/26 21:53:32, wtc wrote:
> Nit: 2009.

Done.

http://codereview.chromium.org/147233/diff/1017/1019#newcode8
Line 8: #include "Cryptuiapi.h"
On 2009/06/26 21:53:32, wtc wrote:
> Nit: lowercase and with <>:
>   #include <cryptuiapi.h>
> 
> Please add
>   #pragma comment(lib, "cryptui.lib")
>   

Done.

http://codereview.chromium.org/147233/diff/1017/1019#newcode41
Line 41: if (!request_)  // We have been canceled.
On 2009/06/26 21:53:32, wtc wrote:
> Can this happen in this method?!
No you are right. Removed.

http://codereview.chromium.org/147233/diff/1017/1019#newcode44
Line 44: if (cert_request_info_->client_certs.empty()) {
On 2009/06/26 21:53:32, wtc wrote:
> Perhaps we should do this in ResourceDispatcherHost::OnCertificateRequested
> so that we don't even need to create an SSLClientAuthHandler
> in this case.
Good point. Done.

http://codereview.chromium.org/147233/diff/1017/1019#newcode64
Line 64: // Windows messages), which is scarry.
On 2009/06/26 21:53:32, wtc wrote:
> Typo: scary.

Done.

http://codereview.chromium.org/147233/diff/1017/1019#newcode80
Line 80: std::wstring title =
l10n_util::GetString(IDS_CLIENT_CERT_DIALOG_TITLE);
On 2009/06/26 21:53:32, wtc wrote:
> Since our title is the same as the default title used by
> Windows, perhaps we should just pass NULL.  This way
> translators don't need to translate the title string.
> 
> Or are you preparing for the future when we have our own
> cert selection dialog?
Yes, preparing for the future.
Also ensuring the dialog title is in whatever language you have selected for
Chrome. (This is to avoid mismatch in languages in a same dialog if the OS and
Chrome are using different languages.)

http://codereview.chromium.org/147233/diff/1017/1019#newcode82
Line 82: ASCIIToWide(url_.host()));
On 2009/06/26 21:53:32, wtc wrote:
> Please use cert_request_info_->host_and_port instead.

Done.

http://codereview.chromium.org/147233/diff/1017/1020
File chrome/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/147233/diff/1017/1020#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/06/26 21:53:32, wtc wrote:
> Nit: 2009.

Done.

http://codereview.chromium.org/147233/diff/1017/1020#newcode14
Line 14: class SSLCertRequestInfo;
On 2009/06/26 21:53:32, wtc wrote:
> This forward declaration is redundent with the
> "net/base/ssl_cert_request_info.h" inclusion above.  Please
> pick one.
You are right. The include is needed because of the scoped_ptr.

http://codereview.chromium.org/147233/diff/1017/1020#newcode20
Line 20: // This class handles the prompting of a certificate for SSL client
On 2009/06/26 21:53:32, wtc wrote:
> Nit: "prompting" => "selection" ?
> 
> Perhaps we should also mention the "approval" aspect.
OK, I improved the wording a bit, but it is still not great.

http://codereview.chromium.org/147233/diff/1017/1020#newcode32
Line 32: // Prompts the user for a certificate and resumes the URL request with
On 2009/06/26 21:53:32, wtc wrote:
> "Asks the user to select a certificate"?

Done.

http://codereview.chromium.org/147233/diff/1017/1020#newcode35
Line 35: void QueryCertificate();
On 2009/06/26 21:53:32, wtc wrote:
> Should we name this method SelectCertificate?

Done.

http://codereview.chromium.org/147233/diff/1017/1020#newcode54
Line 54: GURL url_;
On 2009/06/26 21:53:32, wtc wrote:
> We don't need this because cert_request_info_ contains
> the host_and_port we need for the UI.
Ah, great!
Removed.

[wtc, 2009-06-26 23:03:52]

LGTM.

http://codereview.chromium.org/147233/diff/1029/26
File chrome/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/147233/diff/1029/26#newcode30
Line 30: // Asks the user for a certificate and resumes the URL request with
that
Nit: "for" => "to select" because we already found the certs
for the user, all he needs to do is to select one.

This is why I asked you to change "prompt" because it suggests
that the user needs to find a cert.

http://codereview.chromium.org/147233/diff/1029/26#newcode42
Line 42: void AskUserForCertificate();
You can just name this DoSelectCertificate -- we seem to use
this naming convention a lot.

[wtc, 2009-06-27 01:42:48]

On 2009/06/26 22:35:35, jcampan wrote:
> I also removed the temporary auto-ssl-client-auth switch which is not user
> anymore.

I just logged in again to remind you to do this, and am happy
to see that you already did it :-)  For some reason I didn't
notice this comment this afternoon.