Add a method to the HostContentSettings map to return the |Value| of a content setting

- Add a method to the HostContentSettings map to return the |Value| of a content setting instead of the type ContentSetting.

The HostContentSettingsMap should allow storing arbitrary |Value|s instead of only |ContentSettings| for content settings types (BTW: content settings should be called website settings). This allows to add other content settings type(website settings types) to the host content settings map that require more complex settings than ALLOW, BLOCK, ASK, ... . CONTENT_TYPE_AUTO_SELECT_CERTIFICATE is one example of such a content settings type. CONTENT_TYPE_AUTO_SELECT_CERTIFICATE requires a map with certain criteria that a certificate must match to be selected. 

- Add filter to the AutoSelectCertificate policy.

Instead of only giving a list of origin patterns that specify origins for which client certificates should be auto selected, each origin pattern now also requires a filter with certificate criteria.


This is the second CL in a series and follows CL http://codereview.chromium.org/7831004/

BUG=63656, 81825
TEST=host_content_settings_map_unittest.cc,
     content_settings_policy_provider_unittests.cc

TBR=pam@chromium.org

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

[markusheintz_, 2011-09-01 19:45:54]

@pam: Please review this CL.

@bauerb: Please feel free to comment as well

@wtc,
@mnissler: FYI again.

[wtc, 2011-09-01 22:42:34]

markusheintz: this is a "drive-by" review.  I didn't review
the unit test.

The CL seems correct to me.  I have some questions and suggested
changes below.  Feel free to ignore my questions if the
answers are obvious to people familiar with the content
settings code.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:341:
DCHECK(content_type != CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
Nit: use DCHECK_NE.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:379: size_t
separator_pos = pattern_filter_str.find("|");
Nit: is there a version of find() that takes a character '|'
(instead of a string "|") as input?  That may be slightly
more efficient.

It would be nice to document the format of the pattern_filter_str.
By reading the code, I can tell it's:
  pattern|filter
where filter is a JSON dictionary.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:399:
DCHECK(filter_value->IsType(Value::TYPE_DICTIONARY));
The DCHECK should be done after the null pointer test
on line 400.

But, if the input comes from an external source, such as a file,
we should not use DCHECK here, because the file may be
corrupted.  We should handle the error.

I guess JSON by definition is a dictionary?  If so, then
it is appropriate to use a DCHECK here.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:244: Value* value =
NULL;
Nit: move the declaration of |value| inside the for loop and
merge it with line 248.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:257: return
Value::CreateIntegerValue(GetDefaultContentSetting(content_type));
It seems like you have four cases in this function.
1. ShouldAllowAllContent(secondary_url, content_type) is true.
2. The for loop.
3. content_type == CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE
4. The rest.

It's not clear to me why you handle the four cases in
this particular order.  If this order is obvious to the
people familiar with content settings, or if the order
is not important, then please ignore this question.
Otherwise, it would be nice to document why you handle
the cases in that order.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:85: // that require
an resource identifier to be specified, the
Nit: an => a

You should fix this on line 73 above, too.

[Pam (message me for reviews), 2011-09-02 11:05:29]

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:288:
static_cast<DictionaryValue*>(cert_filter.get())->HasKey("issuer"));
Can you test at more detail than this, i.e. look at the issuer value too?

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:248: value=
(*provider)->GetContentSettingValue(
Nit: space before =

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:254: //
TODO(markusheintz): Add a comment.
Please do todo.

[markusheintz_, 2011-09-02 14:55:59]

@nmissler: would you please review the policy changes as I'm not in the OWNERS
file. Thanks

@pam,
@wtc: I had to remove the policy for the default auto select certificate setting
as there is no such settings anymore.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:341:
DCHECK(content_type != CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
On 2011/09/01 22:42:34, wtc wrote:
> Nit: use DCHECK_NE.

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:379: size_t
separator_pos = pattern_filter_str.find("|");
On 2011/09/01 22:42:34, wtc wrote:
> Nit: is there a version of find() that takes a character '|'
> (instead of a string "|") as input?  That may be slightly
> more efficient.
> 
> It would be nice to document the format of the pattern_filter_str.
> By reading the code, I can tell it's:
>   pattern|filter
> where filter is a JSON dictionary.

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:399:
DCHECK(filter_value->IsType(Value::TYPE_DICTIONARY));
On 2011/09/01 22:42:34, wtc wrote:
> The DCHECK should be done after the null pointer test
> on line 400.
> 

Done.

> But, if the input comes from an external source, such as a file,
> we should not use DCHECK here, because the file may be
> corrupted.  We should handle the error.
> 

If the string is corrupted then |base::JSONReader::Read| should return NULL. I
guess I better ignore such strings then. Better asking the user than auto
selecting certs.

> I guess JSON by definition is a dictionary?  If so, then
> it is appropriate to use a DCHECK here.

Shouldn't we rm the DCHECK then?

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc
(right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:288:
static_cast<DictionaryValue*>(cert_filter.get())->HasKey("issuer"));
On 2011/09/02 11:05:29, Pam wrote:
> Can you test at more detail than this, i.e. look at the issuer value too?

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:244: Value* value =
NULL;
On 2011/09/01 22:42:34, wtc wrote:
> Nit: move the declaration of |value| inside the for loop and
> merge it with line 248.

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:248: value=
(*provider)->GetContentSettingValue(
On 2011/09/02 11:05:29, Pam wrote:
> Nit: space before =

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:254: //
TODO(markusheintz): Add a comment.
On 2011/09/02 11:05:29, Pam wrote:
> Please do todo.

Done.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:257: return
Value::CreateIntegerValue(GetDefaultContentSetting(content_type));
On 2011/09/01 22:42:34, wtc wrote:
> It seems like you have four cases in this function.
> 1. ShouldAllowAllContent(secondary_url, content_type) is true.
> 2. The for loop.
> 3. content_type == CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE
> 4. The rest.
> 
> It's not clear to me why you handle the four cases in
> this particular order.  If this order is obvious to the
> people familiar with content settings, or if the order
> is not important, then please ignore this question.
> Otherwise, it would be nice to document why you handle
> the cases in that order.

I added some comments. I hope they answer you questions.

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7828022/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:85: // that require
an resource identifier to be specified, the
On 2011/09/01 22:42:34, wtc wrote:
> Nit: an => a
> 
> You should fix this on line 73 above, too.

Done.

[Mattias Nissler (ping if slow), 2011-09-02 15:06:21]

I didn't look at the details of the content_settings part, but the policy part
is LGTM if you write proper documentation.

http://codereview.chromium.org/7828022/diff/6001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (left):

http://codereview.chromium.org/7828022/diff/6001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1294: 'desc': '''Allows you to set
whether a client certificate should be automatically selected by <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> (if possible), the user should
be asked to select a client certificate or no certificates should be sent to
sites requesting client certificates.''',
Need better documentation. When do we automatically select? When not? What
security implications may this have?

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

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:373: // e.g.
"[*.]example.com|{\"ISSUER\":{\"CN\":\"some name\"}}"
can we maybe just make the pattern also part of the JSON? i.e. adding a
'pattern' key and moving the selectors to a 'selectors' subdict?

http://codereview.chromium.org/7828022/diff/6001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (left):

http://codereview.chromium.org/7828022/diff/6001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1527:
"profile.managed_default_content_settings.auto_select_certificate";
btw., all these prefs are still in the wrong section of the file.

[wtc, 2011-09-02 18:53:22]

Drive-by review: Patch Set 2 looks good to me.

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

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:37: NULL,
Just wanted to make sure you do want a NULL entry here,
rather than removing the entry altogether.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:184: continue;
It may be a good idea to explain why we skip
CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE here and
on lines 316 and 328 below.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:250: // break when
the first non |NULL| value is returned.
Nit: thanks for the comments!  You can omit the last sentence
of this paragraph because it merely describes the code.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:73: // require an
resource identifier to be specified, the |resource_identifier|
Nit: an resource => a resource

[markusheintz_, 2011-09-02 19:39:12]

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

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:37: NULL,
On 2011/09/02 18:53:22, wtc wrote:
> Just wanted to make sure you do want a NULL entry here,
> rather than removing the entry altogether.

Yes I really want a NULL entry here bc the size has to match
CONTENT_SETTINGS_NUM_TYPES.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:373: // e.g.
"[*.]example.com|{\"ISSUER\":{\"CN\":\"some name\"}}"
On 2011/09/02 15:06:21, Mattias Nissler wrote:
> can we maybe just make the pattern also part of the JSON? i.e. adding a
> 'pattern' key and moving the selectors to a 'selectors' subdict?

Done.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:184: continue;
On 2011/09/02 18:53:22, wtc wrote:
> It may be a good idea to explain why we skip
> CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE here and
> on lines 316 and 328 below.

Done.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:250: // break when
the first non |NULL| value is returned.
On 2011/09/02 18:53:22, wtc wrote:
> Nit: thanks for the comments!  You can omit the last sentence
> of this paragraph because it merely describes the code.

Done.

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7828022/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:73: // require an
resource identifier to be specified, the |resource_identifier|
On 2011/09/02 18:53:22, wtc wrote:
> Nit: an resource => a resource

Done.

[wtc, 2011-09-02 19:58:50]

Drive-by review comments on Patch Set 4:

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:187: continue;
Thank you for adding these comments.

Rather than duplicating the comments, it may be a good idea
to add a boolean function that tells us whether a content
setting type should be skipped.  Then, the same of that
function will self-document why certain content setting
types are skipped.  Just a thought.

[Pam (message me for reviews), 2011-09-04 09:10:14]

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

http://codereview.chromium.org/7828022/diff/7022/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1280: 'id': 102,
Is it safe to renumber this item?

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

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:37: NULL,
It'd be nice to add a comment explaining why these NULL items are needed.

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:388: VLOG(1)
<< "TODO";
Please do TODO. Also two more below.

[markusheintz_, 2011-09-04 18:07:01]

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

http://codereview.chromium.org/7828022/diff/7022/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1280: 'id': 102,
On 2011/09/04 09:10:14, Pam wrote:
> Is it safe to renumber this item?

That is fine in my case, since AutoSelectCertificateForUrls was not released as
a policy yet. I added it some days ago.

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

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:37: NULL,
On 2011/09/04 09:10:14, Pam wrote:
> It'd be nice to add a comment explaining why these NULL items are needed.

Done.

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:388: VLOG(1)
<< "TODO";
On 2011/09/04 09:10:14, Pam wrote:
> Please do TODO. Also two more below.

Double *grrr* to myself. Fixing them got lost somehow along the way.

Done.

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/7022/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:187: continue;
On 2011/09/02 19:58:50, wtc wrote:
> Thank you for adding these comments.
> 
> Rather than duplicating the comments, it may be a good idea
> to add a boolean function that tells us whether a content
> setting type should be skipped.  Then, the same of that
> function will self-document why certain content setting
> types are skipped.  Just a thought.

Done.

[markusheintz_, 2011-09-05 12:22:41]

@joao: I need to change the numbering of the policies you just checked in. Maybe
you can double check and let me know if you have any concerns.

[Joao da Silva, 2011-09-05 12:35:49]

Changes in policy_templates.json LGTM.

[wtc, 2011-09-06 22:11:42]

markusheintz: I reviewed the diffs between Patch Sets 4 and
10.  I have some comments below.

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:343:
static_cast<Value*>(Value::CreateIntegerValue(
Is this static_cast necessary?

Value::CreateIntegerValue() returns FundamentalValue*.

FundamentalValue is derived from Value.

So FundamentalValue* should be converted to Value*
automatically.

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:404: << "
Missing pattern or filtern.";
Typo: filtern => filter

Nit: to concatendate string literals, you don't need the
<< operator on lines 391, 404, 413.

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:433: }
Remove the curly braces on lines 427 and 433.

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:192:
output.settings[i] = GetDefaultContentSetting(ContentSettingsType(i));
Indent by two spaces.

[wtc, 2011-09-07 17:27:48]

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:402: if
(!pattern_read || !filter_read) {
We need to allow a policy that specifies no additional cert
filtering.  So we need to decide how such a policy looks
like.

Should it have no "filter" entry in the JSON?

Or should it have an empty "filter" entry in the JSON?

If the former, then we may need to allow filter_read to
be false here.  Or does pattern_filter_pair->Remove return
true and set cert_filter_ptr to NULL in that case?

[markusheintz_, 2011-09-07 19:29:48]

I uploaded the next patchset as CL http://codereview.chromium.org/7837038/

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:343:
static_cast<Value*>(Value::CreateIntegerValue(
On 2011/09/06 22:11:42, wtc wrote:
> Is this static_cast necessary?
> 
> Value::CreateIntegerValue() returns FundamentalValue*.
> 
> FundamentalValue is derived from Value.
> 
> So FundamentalValue* should be converted to Value*
> automatically.
> 

Done

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:402: if
(!pattern_read || !filter_read) {
On 2011/09/07 17:27:48, wtc wrote:
> We need to allow a policy that specifies no additional cert
> filtering.  So we need to decide how such a policy looks
> like.
> 

I guess my question here is why do we need this? 
One reason I can think of is convenience. Are there others I don't see yet?  

My assumption is that admins want to say: certificate "foo" should be used for
"https://www.sample.com and certificate" "bar" should be used for "*" all other
origins. I assume foo and bar would be something specific rather than a
wildcard.

> Should it have no "filter" entry in the JSON?

I think the "filter" should be mandatory but there should be a match all filter.

Having a mandatory filter prevents admins from accidentally forgetting to
specify one. A match all pattern would be a conscious decision that is taken at
own "risk".  

> 
> Or should it have an empty "filter" entry in the JSON?
> 
> If the former, then we may need to allow filter_read to
> be false here.  Or does pattern_filter_pair->Remove return
> true and set cert_filter_ptr to NULL in that case?

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:404: << "
Missing pattern or filtern.";
On 2011/09/06 22:11:42, wtc wrote:
> Typo: filtern => filter
> 
> Nit: to concatendate string literals, you don't need the
> << operator on lines 391, 404, 413.

What the suggested style for this? Jost ommitting the leading << but indenting
by two more spaces

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:433: }
On 2011/09/06 22:11:42, wtc wrote:
> Remove the curly braces on lines 427 and 433.

Done.

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:192:
output.settings[i] = GetDefaultContentSetting(ContentSettingsType(i));
On 2011/09/06 22:11:42, wtc wrote:
> Indent by two spaces.

Done.

[wtc, 2011-09-07 21:31:35]

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:402: if
(!pattern_read || !filter_read) {
On 2011/09/07 19:29:48, markusheintz_ wrote:
>
> I guess my question here is why do we need this?

An SSL/TLS server can specify the list of acceptable
issuer names.  All we need on the Chrome side is a
policy that suppresses the prompting for that server.
It is not necessary to repeat the list of acceptable
issuer names in the policy.

> My assumption is that admins want to say: certificate "foo" should be used for
> "https://www.sample.com and certificate" "bar" should be used for "*" all
other
> origins. I assume foo and bar would be something specific rather than a
> wildcard.

If https://www.sample.com specifies a list of acceptable
issuer names that narrows the list of acceptable client
certificates to just certificate "foo", then the policy
can be a wildcard.

The policy for all other origins "*" should not be a
wildcard.  If that policy is a wildcard, then it implements
the old Firefox behavior of "Select Automatically".  (The
current Firefox probably still supports that option.)

> I think the "filter" should be mandatory but there should be a match all
filter.
> 
> Having a mandatory filter prevents admins from accidentally forgetting to
> specify one. A match all pattern would be a conscious decision that is taken
at
> own "risk".

This is fine.  Just wanted to reiterate that a match all
filter is not necessary a risk as I explained above.
It is only a risk for the origin pattern "*", but is not
a risk for the origin pattern https://www.sample.com if
the server already specifies a list of acceptable issuer
names properly.

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:404: << "
Missing pattern or filtern.";
On 2011/09/07 19:29:48, markusheintz_ wrote:
>
> What the suggested style for this?

There is no suggested style in the Style Guide.  Just
make it look nice

[markusheintz_, 2011-09-07 22:23:37]

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:402: if
(!pattern_read || !filter_read) {
On 2011/09/07 21:31:35, wtc wrote:
> On 2011/09/07 19:29:48, markusheintz_ wrote:
> >
> > I guess my question here is why do we need this?
> 
> An SSL/TLS server can specify the list of acceptable
> issuer names.  All we need on the Chrome side is a
> policy that suppresses the prompting for that server.
> It is not necessary to repeat the list of acceptable
> issuer names in the policy.

I'm not really a TLS expert but I'm trying to fill my gaps as fast a possible.
In the TSL handshake the TSL server can request the client certificate with a
CertificateRequest message right? And the CertificateRequest message contains
three fields: certificate_types, supported_signature_algorithme and
certificate_authorities. So I guess you mean the certificate_authorities field
which is list of distinguished names. Can a Distinguished Name in this message
contain a set of AttributetypeAndValue pairs like "CN=... O=... C=... "
 
> 
> > My assumption is that admins want to say: certificate "foo" should be used
for
> > "https://www.sample.com and certificate" "bar" should be used for "*" all
> other
> > origins. I assume foo and bar would be something specific rather than a
> > wildcard.
> 
> If https://www.sample.com specifies a list of acceptable
> issuer names that narrows the list of acceptable client
> certificates to just certificate "foo", then the policy
> can be a wildcard.
> 
> The policy for all other origins "*" should not be a
> wildcard.  If that policy is a wildcard, then it implements
> the old Firefox behavior of "Select Automatically".  (The
> current Firefox probably still supports that option.)
> 
> > I think the "filter" should be mandatory but there should be a match all
> filter.
> > 
> > Having a mandatory filter prevents admins from accidentally forgetting to
> > specify one. A match all pattern would be a conscious decision that is taken
> at
> > own "risk".
> 
> This is fine.  Just wanted to reiterate that a match all
> filter is not necessary a risk as I explained above.
> It is only a risk for the origin pattern "*", but is not
> a risk for the origin pattern https://www.sample.com if
> the server already specifies a list of acceptable issuer
> names properly.

"risk" was probably the wrong word fir what I wanted to say. What you are saying
here makes totally sense to me. Thanks for explaining.

[wtc, 2011-09-07 23:14:20]

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

http://codereview.chromium.org/7828022/diff/9003/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:402: if
(!pattern_read || !filter_read) {
On 2011/09/07 22:23:37, markusheintz_ wrote:
>
> In the TSL handshake the TSL server can request the client certificate with a
> CertificateRequest message right?

Yes, that's how a TLS server requests client authentication.

> And the CertificateRequest message contains
> three fields: certificate_types, supported_signature_algorithme and
> certificate_authorities. So I guess you mean the certificate_authorities field
> which is list of distinguished names.

Yes.  I'm sorry I used inaccurate terminology.
A TLS server can specify the list of acceptable
certificate authorities in the CertificateRequest
message.

> Can a Distinguished Name in this message
> contain a set of AttributetypeAndValue pairs like "CN=... O=... C=... "

Yes, a Distinguished Name is a set of AttributetypeAndValue
pairs like "CN=... O=... C=... ".

For this work, it's useful to understand the CertificateRequest
message.  The info inside the CertificateRequest message
helps the browser narrow down the acceptable client
certificates, ideally to just one certificate.