Add new Content settings type AUTO-SUBMIT-CERTIFICATE

- Add a new content settings type AUTO-SELECT-CERTIFICATE. The default value of the new content settings type AUTO-SELECT-CERTIFICATE is CONTENT_SETTING_ASK
- Add a policy for whitelisting origins for which client certificates should be auto selected.
- Add a policy to set a default setting for the auto select certificates setting.


BUG=81825
TEST=host_content_settings_map_unittest.cc,
          content_settings_policy_provider_unit_test.cc,
          configuration_policy_pref_store_unittest.cc 


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

[markusheintz_, 2011-08-05 17:50:22]

Please review my CL.

@wtc: Please review the certificate and ssl related parts

@mnissler: I plan to remove the policy for the default setting as is doesn't
make sense. wdyt?

[Mattias Nissler (ping if slow), 2011-08-09 15:37:25]

Overall, looks reasonable to me, but you'll want an OWNER to look at the
content/ net/ and content settings stuff :)

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1276: 'name':
'AutoSubmitCertificateForUrls',
How are you planning to extend that to matching also the cert? Would it be
possible to prefix the URL pattern with some cert pattern which is TBD? I'm
asking since I'd like the changes to make later be backward-compatible and old
patterns forward-compatible.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:7: #include <string>
you already have added it in the corresponding header?

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:240:
TestingPrefService* prefs = profile.GetTestingPrefService();
Are you actually using |profile|? If not, why not use a local
TestingPrefService?

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:259:
EXPECT_EQ(CONTENT_SETTING_DEFAULT,
indentation

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:57:
CONTENT_SETTING_BLOCK,  // CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
Shouldn't it be ASK?

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1267:
TEST_F(HostContentSettingsMapTest, AutoSubmitCertificate) {
Isn't this exactly the same test you had before, but additionally going through
a HCSM? If you're meaning to test HCSM, you should mock out the providers.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:199: // query the
HostContentSettingsMap.
Maybe https:// ?

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:392: const char kEnableAutoSubmitCertificate[] 
 = "enable-auto-submit-certificate";
What's the reason we develop this behind a flag? It has no user-facing UI
anyway.

[wtc, 2011-08-11 18:33:55]

Review comments on Patch Set 4:

I reviewed the entire CL.  It is very straightforward.

The main issue is whether the default setting for
CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE should be
called CONTENT_SETTING_BLOCK or CONTENT_SETTING_ASK.  I'm
assuming you're using CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
as a boolean, so this is just a matter of what the call
the default behavior of not automatically submitting
certificate.

All the other comments are on coding style nits or
updating stale comments.  I also have some questions
due to my unfamiliarity of the content settings code.

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1266: },
I am not familiar with the json format of this file, so these
may be stupid questions.

Can two items have the same name (AllowAutoSubmitCertificate)?

What are the values 1 and 2 of these items?

On line 1268, the supported chrome versions should be 15-.

The example_value on line 1270 is 0.  Should it be a value
listed on lines 1259 and 1264 above?

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1280: 'example_value':
['http://www.example.com', '[*.]example.edu'],
Use https:// in this example_value.  http:// does not use SSL.

Please search for "http://" in this CL and fix all
occurrences. There are several http:// URLs in unit tests.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:249:
CONTENT_SETTING_BLOCK,
It seems that the default should be CONTENT_SETTING_ASK.

To me, CONTENT_SETTING_BLOCK means we block client certificates,
without asking the user.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:250:
""));
Nit: please pick either "" or std::string() for an empty
string argument, and use it consistently in this CL.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:73:
"auto-submit-certificate"
Nit: Add a comma (,) at the end of the line.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:522:
std::string()));
Why is the expected value CONTENT_SETTING_DEFAULT here?
(I am not familiar content settings.)

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:190: // Hide the auto
submit certificate feature behind a cmd line flag.
Nit: cmd => command

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:202: <<
cert_request_info->host_and_port;
Use https:// instead of ssl:// on lines 200 and 201.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:213: // criterias.
Nit: criterias => criteria

"criteria" is the plural form of "criterion".

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:223: }
If the possible values of 'setting' are ALLOW and BLOCK/ASK
(two possible values), this code is correct.

If the possible values of 'setting' are ALLOW, BLOCK, and ASK
(three possible values), then BLOCK should be handled as
not submitting any certificate, probably as follows:
    handler->CertificateSelected(NULL);

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:24: void
SelectClientCertificate(scoped_refptr<SSLClientAuthHandler> handler);
Please add "and returns it to |handler|".  See also my comment
below.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:54: // returning them to
|handler|.
Please update this (stale) comment because the |request_info|
argument is gone.

I think |request_info| is now a member of |handler|.

Also, the comment should say "select a client certificate"
and "returning it to |handler|".

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:397: continue;
Why CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE is skipped
is not obvious to me.  Please add a comment to explain it.

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:392: const char kEnableAutoSubmitCertificate[] 
 = "enable-auto-submit-certificate";
Add a comment to explain what this command-line option means.

http://codereview.chromium.org/7537025/diff/6011/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7537025/diff/6011/content/browser/content_brow...
content/browser/content_browser_client.h:176: // dialog, we call  with a NULL
certificate.
Please update this comment -- it still mentions "dialog",
but the function name is no longer ShowClientCertificateRequestDialog.

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
File content/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
content/browser/ssl/ssl_client_auth_handler.h:66: void
SelectClientCertificate(int render_process_host_id,
"SelectClientCertificate" is too similar to the public method
"SelectCertificate".

There are several conventions used in the Chromium tree to
indicate this relationship, including these two:

1. Foo() posts a task, and DoFoo() does the real work.

2. Foo() posts a task (to the UI Thread), and FooOnUIThread()
does the real work.

So, please rename this function DoSelectCertificate or
SelectCertificateOnUIThread.

Also, the "SSL helper" in the comment is not clear and may
be a little out of date.  In the .cc file, we are calling
content::GetContentClient()->browser() now, but it seems
that it ultimately calls TabContentsSSLHelper.  So,
"SSL helper" probably should be changed to "ContentBrowserClient"
or "TabContentsSSLHelper".

[markusheintz_, 2011-08-15 19:09:04]

I removed the command line switch and changed the CL to also support
CONTENT_SETTING_ASK as value.

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1266: },
On 2011/08/11 18:33:55, wtc wrote:
> I am not familiar with the json format of this file, so these
> may be stupid questions.
> 
> Can two items have the same name (AllowAutoSubmitCertificate)?
> 
> What are the values 1 and 2 of these items?
> 
> On line 1268, the supported chrome versions should be 15-.
> 
> The example_value on line 1270 is 0.  Should it be a value
> listed on lines 1259 and 1264 above?

Sorry I didn't complete this yet.

You are absolutely right. I can not have the same names. I fixed that.

The integer values are the values of the content settings ALLOW (1), BLOCK (2)
and ASK (3)

I also changed the Chrome version.

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1276: 'name':
'AutoSubmitCertificateForUrls',
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> How are you planning to extend that to matching also the cert? Would it be
> possible to prefix the URL pattern with some cert pattern which is TBD? I'm
> asking since I'd like the changes to make later be backward-compatible and old
> patterns forward-compatible.

I don't see another way than appending the string version of the filter to the
pattern. I really don't have a better solution yet. A missing filter would
default to a wildcard that matches anything. So we can stay backwards
compatible.

http://codereview.chromium.org/7537025/diff/6011/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1280: 'example_value':
['http://www.example.com', '[*.]example.edu'],
On 2011/08/11 18:33:55, wtc wrote:
> Use https:// in this example_value.  http:// does not use SSL.
> 
> Please search for "http://" in this CL and fix all
> occurrences. There are several http:// URLs in unit tests.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:7: #include <string>
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> you already have added it in the corresponding header?

True don't know way lint complained about this. Maybe lint just said .h and I
read .cc. Anyway. Removed.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:249:
CONTENT_SETTING_BLOCK,
On 2011/08/11 18:33:55, wtc wrote:
> It seems that the default should be CONTENT_SETTING_ASK.
> 
> To me, CONTENT_SETTING_BLOCK means we block client certificates,
> without asking the user.

Yes you are right. Initially I only used two values: ALLOW (enabled) and BLOCK
(disabled). 

I changed this and I'm using now three values: ALLOW, BLOCK, ASK.  BLOCK is now
blocking certificates from being sent.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:240:
TestingPrefService* prefs = profile.GetTestingPrefService();
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> Are you actually using |profile|? If not, why not use a local
> TestingPrefService?

If I use the TestingPrefService I can not set test the loading of the managed
preference. The test fails then.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:250:
""));
On 2011/08/11 18:33:55, wtc wrote:
> Nit: please pick either "" or std::string() for an empty
> string argument, and use it consistently in this CL.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:259:
EXPECT_EQ(CONTENT_SETTING_DEFAULT,
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:57:
CONTENT_SETTING_BLOCK,  // CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> Shouldn't it be ASK?

Yes. I agree that this would be better.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:73:
"auto-submit-certificate"
On 2011/08/11 18:33:55, wtc wrote:
> Nit: Add a comma (,) at the end of the line.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:522:
std::string()));
On 2011/08/11 18:33:55, wtc wrote:
> Why is the expected value CONTENT_SETTING_DEFAULT here?
> (I am not familiar content settings.)

We have two kinds of content settings provider: DefaultContentSettingsProvider
and ContentSettingsProvider for origin specific settings. (FYI: I'm working on
getting rid of the (legacy) default content settings provider as they are not
unnecessary anymore. They will be integrated into the ContentSettingsProvider).
If the content_settings::PrefProvider does not contain an origin specific
setting, then it returns CONTENT_SETTING_DEFAULT to indicate that the default
setting applies (which will be do not auto-submit a cert).

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1267:
TEST_F(HostContentSettingsMapTest, AutoSubmitCertificate) {
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> Isn't this exactly the same test you had before, but additionally going
through
> a HCSM? If you're meaning to test HCSM, you should mock out the providers.

You are right. I removed the duplicated test.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:190: // Hide the auto
submit certificate feature behind a cmd line flag.
On 2011/08/11 18:33:55, wtc wrote:
> Nit: cmd => command

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:199: // query the
HostContentSettingsMap.
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> Maybe https:// ?

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:202: <<
cert_request_info->host_and_port;
On 2011/08/11 18:33:55, wtc wrote:
> Use https:// instead of ssl:// on lines 200 and 201.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:213: // criterias.
On 2011/08/11 18:33:55, wtc wrote:
> Nit: criterias => criteria
> 
> "criteria" is the plural form of "criterion".

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:223: }
On 2011/08/11 18:33:55, wtc wrote:
> If the possible values of 'setting' are ALLOW and BLOCK/ASK
> (two possible values), this code is correct.
Only two values are possible in version #4 of this CL. Initially I thought about
only having two values ALLOW and BLOCK. But having ASK as third value would be
better.

> 
> If the possible values of 'setting' are ALLOW, BLOCK, and ASK
> (three possible values), then BLOCK should be handled as
> not submitting any certificate, probably as follows:
>     handler->CertificateSelected(NULL);

I changed the CL to support the three values ALLOW, BLOCK, ASK. So now you can
also block client certificates from being sent.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:24: void
SelectClientCertificate(scoped_refptr<SSLClientAuthHandler> handler);
On 2011/08/11 18:33:55, wtc wrote:
> Please add "and returns it to |handler|".  See also my comment
> below.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:54: // returning them to
|handler|.
On 2011/08/11 18:33:55, wtc wrote:
> Please update this (stale) comment because the |request_info|
> argument is gone.
> 
> I think |request_info| is now a member of |handler|.
> 
> Also, the comment should say "select a client certificate"
> and "returning it to |handler|".

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:397: continue;
On 2011/08/11 18:33:55, wtc wrote:
> Why CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE is skipped
> is not obvious to me.  Please add a comment to explain it.

Done.

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:392: const char kEnableAutoSubmitCertificate[] 
 = "enable-auto-submit-certificate";
On 2011/08/09 15:37:25, Mattias Nissler wrote:
> What's the reason we develop this behind a flag? It has no user-facing UI
> anyway.
@mnissler: I thought it might be good to have a switch to turn the policy off.
But you are right. I removed the switch again since we do not really need it.

http://codereview.chromium.org/7537025/diff/6011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:392: const char kEnableAutoSubmitCertificate[] 
 = "enable-auto-submit-certificate";
On 2011/08/11 18:33:55, wtc wrote:
> Add a comment to explain what this command-line option means.

I decided to remove the switch.

http://codereview.chromium.org/7537025/diff/6011/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7537025/diff/6011/content/browser/content_brow...
content/browser/content_browser_client.h:176: // dialog, we call  with a NULL
certificate.
On 2011/08/11 18:33:55, wtc wrote:
> Please update this comment -- it still mentions "dialog",
> but the function name is no longer ShowClientCertificateRequestDialog.

Done.

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
File content/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
content/browser/ssl/ssl_client_auth_handler.h:66: void
SelectClientCertificate(int render_process_host_id,
On 2011/08/11 18:33:55, wtc wrote:
> "SelectClientCertificate" is too similar to the public method
> "SelectCertificate".
> 
> There are several conventions used in the Chromium tree to
> indicate this relationship, including these two:
> 
> 1. Foo() posts a task, and DoFoo() does the real work.
> 
> 2. Foo() posts a task (to the UI Thread), and FooOnUIThread()
> does the real work.
> 

Thanks a lot for the explanation.

> So, please rename this function DoSelectCertificate or
> SelectCertificateOnUIThread.
> 

I named it DoSelectCertificate since this convention is already by the
CertificateSelected/DoCertificateSelected method pair.

> Also, the "SSL helper" in the comment is not clear and may
> be a little out of date.  In the .cc file, we are calling
> content::GetContentClient()->browser() now, but it seems
> that it ultimately calls TabContentsSSLHelper.  So,
> "SSL helper" probably should be changed to "ContentBrowserClient"
> or "TabContentsSSLHelper".

Maybe we should not mention any implementation details in the comment at all and
just describe the overall functionality of the method. (It "Selects a client
Certificate ...") then it's less likely that the comment get's outdated when the
implementation changes. Would that be fine with you?

[Mattias Nissler (ping if slow), 2011-08-16 11:21:35]

policy stuff LGTM, I didn't deep-dive into the content settings and cert
selection code.

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:61:
CONTENT_SETTING_ASK,  // CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
align the comment

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h#...
chrome/common/pref_names.h:497: extern const char
kManagedAutoSubmitCertificateForUrls[];
It seems like it would be helpful to have the same order as in the previous
block?

[Mattias Nissler (ping if slow), 2011-08-16 11:21:35]

policy stuff LGTM, I didn't deep-dive into the content settings and cert
selection code.

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:61:
CONTENT_SETTING_ASK,  // CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
align the comment

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h#...
chrome/common/pref_names.h:497: extern const char
kManagedAutoSubmitCertificateForUrls[];
It seems like it would be helpful to have the same order as in the previous
block?

[markusheintz_, 2011-08-16 11:38:26]

Thanks for reviewing.

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:61:
CONTENT_SETTING_ASK,  // CONTENT_SETTINGS_TYPE_AUTO_SUBMIT_CERTIFICATE
On 2011/08/16 11:21:35, Mattias Nissler wrote:
> align the comment

Done.

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7537025/diff/20001/chrome/common/pref_names.h#...
chrome/common/pref_names.h:497: extern const char
kManagedAutoSubmitCertificateForUrls[];
On 2011/08/16 11:21:35, Mattias Nissler wrote:
> It seems like it would be helpful to have the same order as in the previous
> block?

Done.

[wtc, 2011-08-17 19:28:38]

Markus: I will review Patch Set 7 tomorrow (Thursday).

[wtc, 2011-08-18 19:34:20]

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
File content/browser/ssl/ssl_client_auth_handler.h (right):

http://codereview.chromium.org/7537025/diff/6011/content/browser/ssl/ssl_clie...
content/browser/ssl/ssl_client_auth_handler.h:66: void
SelectClientCertificate(int render_process_host_id,
On 2011/08/15 19:09:04, markusheintz_ wrote:
>
> Maybe we should not mention any implementation details in the comment at all
and
> just describe the overall functionality of the method. (It "Selects a client
> Certificate ...") then it's less likely that the comment get's outdated when
the
> implementation changes. Would that be fine with you?

Yes.  This is actually what the Style Guide recommends for
function comments in header files!

[wtc, 2011-08-18 22:03:38]

LGTM.

High-Level Comments

1. In the CL's commit message, please mention the new
content settings and their default values (ASK and empty list).

2. I think we only need to support ALLOW and ASK.  We can
add BLOCK when customers request it.

3. I suggest that we change AutoSubmitCertificate to
AutoSelectCertificate.  I think "select" is more accurate.
If you agree this is better, you'll need to search for
these strings and change them:
  AutoSubmit
  AUTO_SUBMIT
  auto-submit
  auto_submit

4. Are you sure the new content settings should be
UNSYNCABLE_PREF?  The AutoSubmitCertificateForUrls list
seems useful to sync.

5. This CL will make Chrome match roughly what Firefox
does in this area.  Firefox has only one preference,
security.default_personal_cert, which has two possible
values: "Ask Every Time" and "Select Automatically".
I believe this preference applies to all websites globally.

Firefox has another preference,
security.remember_cert_checkbox_default_setting.  As far
as I can tell, the "remember cert" checkbox allows a
selected cert (identified by its SHA-256 has) to be
remembered for a particular site.

Low-level comments follow.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1286: 'caption': '''Blocks certificates
from being transfered to sites that requests client certificates.'''
Nit: transfered => sent

sites that requests => sites that request

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1289: 'name': 'AskAutoSubmitCertificat',
Typo: AskAutoSubmitCertificat is missing an 'e' at the end.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1292: },
Can the names be simply "Auto" (or "AutoSelect"), "Block", and "Ask"?

If not, I suggest that you put those words last, like this:
  AutoSubmitCertificateAllow
  AutoSubmitCertificateBlock
  AutoSubmitCertificateAsk

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1299: 'desc': '''Allows you to set
whether a client certificate should be automatically selected by <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex> (if possible), the user should be
asked to select a client certificate or no certificates should be transmitted to
sites requesting client certificates.''',
Nit: transmitted => sent

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1308: 'caption': '''Auto select client
certificates for these sites''',
Nit: Auto select => Automatically select

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:130:
TEST_F(PolicyDefaultProviderTest, AutoSubmitCertificateList) {
Nit: Remove "List" from the unit test's name, or replace it
with "Enum".  This test does not test the URL list.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1260: 
Please add back this blank line.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:192:
DCHECK(requesting_url.is_valid()) << " Invalid URL string: https://"
Nit: does the error message string need to start with a space?

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:202: DCHECK(setting !=
CONTENT_SETTING_DEFAULT);
Nit: use DCHECK_NE, which prints the value of 'setting'
when the assertion fails.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:53: // Displays a dialog
for selecting a client certificate and returning it to
returning => returns

[markusheintz_, 2011-08-19 15:17:57]

Added Avi as review for the content related changes.


The Preferences should not be synced since their only purpose is to make the
polices available through the PrefService and we do not want to sync policies.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1286: 'caption': '''Blocks certificates
from being transfered to sites that requests client certificates.'''
On 2011/08/18 22:03:38, wtc wrote:
> Nit: transfered => sent
> 
> sites that requests => sites that request

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1289: 'name': 'AskAutoSubmitCertificat',
On 2011/08/18 22:03:38, wtc wrote:
> Typo: AskAutoSubmitCertificat is missing an 'e' at the end.

Uuups. Done

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1292: },
On 2011/08/18 22:03:38, wtc wrote:
> Can the names be simply "Auto" (or "AutoSelect"), "Block", and "Ask"?
> 
> If not, I suggest that you put those words last, like this:
>   AutoSubmitCertificateAllow
>   AutoSubmitCertificateBlock
>   AutoSubmitCertificateAsk

In general I would be fine with the names you suggest, but if you don't mind I'd
like to stick with the current naming convention in this file to be consistent
with other names.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1299: 'desc': '''Allows you to set
whether a client certificate should be automatically selected by <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex> (if possible), the user should be
asked to select a client certificate or no certificates should be transmitted to
sites requesting client certificates.''',
On 2011/08/18 22:03:38, wtc wrote:
> Nit: transmitted => sent

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1308: 'caption': '''Auto select client
certificates for these sites''',
On 2011/08/18 22:03:38, wtc wrote:
> Nit: Auto select => Automatically select

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:130:
TEST_F(PolicyDefaultProviderTest, AutoSubmitCertificateList) {
On 2011/08/18 22:03:38, wtc wrote:
> Nit: Remove "List" from the unit test's name, or replace it
> with "Enum".  This test does not test the URL list.

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1260: 
On 2011/08/18 22:03:38, wtc wrote:
> Please add back this blank line.

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents_ssl_helper.cc (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:192:
DCHECK(requesting_url.is_valid()) << " Invalid URL string: https://"
On 2011/08/18 22:03:38, wtc wrote:
> Nit: does the error message string need to start with a space?
 Not at all. Removed the leading space.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.cc:202: DCHECK(setting !=
CONTENT_SETTING_DEFAULT);
On 2011/08/18 22:03:38, wtc wrote:
> Nit: use DCHECK_NE, which prints the value of 'setting'
> when the assertion fails.

Done.

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents_ssl_helper.h (right):

http://codereview.chromium.org/7537025/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents_ssl_helper.h:53: // Displays a dialog
for selecting a client certificate and returning it to
On 2011/08/18 22:03:38, wtc wrote:
> returning => returns

Done.

[Avi (use Gerrit), 2011-08-19 15:19:46]

content change LGTM

[wtc, 2011-08-22 23:58:53]

LGTM.  I reviewed Patch Set 11.

http://codereview.chromium.org/7537025/diff/40001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7537025/diff/40001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:1285: 'value': 3,
Should we change the value of AskAutoSelectCertificate to 2,
after we deleted the "Block" option?

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider.cc:40: NULL,
IMPORTANT: why is there a NULL here?  I guess it is for the
Web intents.  Why doesn't it have a preference string?

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:276:
}  // namespace content_settings
Add a blank line before this line.

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:34: << "
actual: " << settings2.settings[i] << "]";
Are you sure this error message is useful in all cases?

If a test expects the settings to be different, then it will
be wrong to log an error message here.

I see that we only call SettingsEqual() inside EXPECT_TRUE(),
so I think it is OK to add this error message.

http://codereview.chromium.org/7537025/diff/40001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:629:
settings.settings[CONTENT_SETTINGS_TYPE_INTENTS]);
Should we add an EXPECT_EQ for CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE?

[commit-bot: I haz the power, 2011-08-24 11:37:37]

Change committed as 98026