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

chrome/browser/content_settings/content_settings_policy_provider.cc

Index: chrome/browser/content_settings/content_settings_policy_provider.cc
diff --git a/chrome/browser/content_settings/content_settings_policy_provider.cc b/chrome/browser/content_settings/content_settings_policy_provider.cc
index 294973d5e16cc713d01f7d76a94f3891c031e878..a47af2bf88bd9a1399489a3cb23ae023a485b77b 100644
--- a/chrome/browser/content_settings/content_settings_policy_provider.cc
+++ b/chrome/browser/content_settings/content_settings_policy_provider.cc
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/json/json_reader.h"
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -32,8 +33,8 @@ const char* kPrefToManageType[CONTENT_SETTINGS_NUM_TYPES] = {
   prefs::kManagedDefaultPopupsSetting,
   prefs::kManagedDefaultGeolocationSetting,
   prefs::kManagedDefaultNotificationsSetting,
-  NULL,
-  prefs::kManagedDefaultAutoSelectCertificateSetting,
+  NULL,  // No policy for default value of content type intents
+  NULL,  // No policy for default value of content type auto-select-certificate
 };
 
 struct PrefsForManagedContentSettingsMapEntry {
@@ -45,10 +46,6 @@ struct PrefsForManagedContentSettingsMapEntry {
 const PrefsForManagedContentSettingsMapEntry
     kPrefsForManagedContentSettingsMap[] = {
   {
-    prefs::kManagedAutoSelectCertificateForUrls,
-    CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE,
-    CONTENT_SETTING_ALLOW
-  }, {
     prefs::kManagedCookiesAllowedForUrls,
     CONTENT_SETTINGS_TYPE_COOKIES,
     CONTENT_SETTING_ALLOW
@@ -121,8 +118,6 @@ PolicyDefaultProvider::PolicyDefaultProvider(PrefService* prefs)
   pref_change_registrar_.Add(prefs::kManagedDefaultPopupsSetting, this);
   pref_change_registrar_.Add(prefs::kManagedDefaultGeolocationSetting, this);
   pref_change_registrar_.Add(prefs::kManagedDefaultNotificationsSetting, this);
-  pref_change_registrar_.Add(
-      prefs::kManagedDefaultAutoSelectCertificateSetting, this);
 }
 
 PolicyDefaultProvider::~PolicyDefaultProvider() {
@@ -173,9 +168,6 @@ void PolicyDefaultProvider::Observe(int type,
       UpdateManagedDefaultSetting(CONTENT_SETTINGS_TYPE_GEOLOCATION);
     } else if (*name == prefs::kManagedDefaultNotificationsSetting) {
       UpdateManagedDefaultSetting(CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
-    } else if (*name == prefs::kManagedDefaultAutoSelectCertificateSetting) {
-      UpdateManagedDefaultSetting(
-          CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
     } else {
       NOTREACHED() << "Unexpected preference observed";
       return;
@@ -247,9 +239,6 @@ void PolicyDefaultProvider::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterIntegerPref(prefs::kManagedDefaultNotificationsSetting,
                              CONTENT_SETTING_DEFAULT,
                              PrefService::UNSYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kManagedDefaultAutoSelectCertificateSetting,
-                             CONTENT_SETTING_ASK,
-                             PrefService::UNSYNCABLE_PREF);
 }
 
 // ////////////////////////////////////////////////////////////////////////////
@@ -341,6 +330,7 @@ void PolicyProvider::GetContentSettingsFromPreferences(
 
       ContentSettingsType content_type =
           kPrefsForManagedContentSettingsMap[i].content_type;
+      DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
       // If only one pattern was defined auto expand it to a pattern pair.
       ContentSettingsPattern secondary_pattern =
           !pattern_pair.second.IsValid() ? ContentSettingsPattern::Wildcard()
@@ -356,12 +346,90 @@ void PolicyProvider::GetContentSettingsFromPreferences(
   }
 }
 
+void PolicyProvider::GetAutoSelectCertificateSettingsFromPreferences(
+    OriginIdentifierValueMap* value_map) {
+  const char* pref_name = prefs::kManagedAutoSelectCertificateForUrls;
+
+  if (!prefs_->HasPrefPath(pref_name)) {
+    VLOG(2) << "Skipping unset preference: " << pref_name;
+    return;
+  }
+
+  const PrefService::Preference* pref = prefs_->FindPreference(pref_name);
+  DCHECK(pref);
+  DCHECK(pref->IsManaged());
+
+  const ListValue* pattern_filter_str_list = NULL;
+  if (!pref->GetValue()->GetAsList(&pattern_filter_str_list)) {
+    NOTREACHED();
+    return;
+  }
+
+  // Parse the list of pattern filter strings. A pattern filter string has
+  // the following JSON format:
+  //
+  // {
+  //   "pattern": <content settings pattern string>,
+  //   "filter" : <certificate filter in JSON format>
+  // }
+  //
+  // e.g.
+  // {
+  //   "pattern": "[*.]example.com",
+  //   "filter": {
+  //      "ISSUER": {
+  //        "CN": "some name"
+  //      }
+  // }
+  for (size_t j = 0; j < pattern_filter_str_list->GetSize(); ++j) {
+    std::string pattern_filter_json;
+    pattern_filter_str_list->GetString(j, &pattern_filter_json);
+
+    scoped_ptr<Value> value(base::JSONReader::Read(pattern_filter_json, true));
+    if (!value.get()) {
+      VLOG(1) << "Ignoring invalid certificate auto select setting. Reason:"
+              << " Invalid JSON format: " << pattern_filter_json;
+      continue;
+    }
+
+    scoped_ptr<DictionaryValue> pattern_filter_pair(
+        static_cast<DictionaryValue*>(value.release()));
+    std::string pattern_str;
+    bool pattern_read = pattern_filter_pair->GetString("pattern", &pattern_str);
+    Value* cert_filter_ptr = NULL;
+    bool filter_read = pattern_filter_pair->Remove("filter", &cert_filter_ptr);
+    scoped_ptr<Value> cert_filter(cert_filter_ptr);
+    if (!pattern_read || !filter_read) {

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

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

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

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

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

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=... "
 
"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]

Yes, that's how a TLS server requests client authentication.
Yes.  I'm sorry I used inaccurate terminology.
A TLS server can specify the list of acceptable
certificate authorities in the CertificateRequest
message.
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.

+      VLOG(1) << "Ignoring invalid certificate auto select setting. Reason:"
+              << " Missing pattern or filtern.";

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

Typo: filtern => filter

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

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

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

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

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

+      continue;
+    }
+
+    ContentSettingsPattern pattern =
+        ContentSettingsPattern::FromString(pattern_str);
+    // Ignore invalid patterns.
+    if (!pattern.IsValid()) {
+      VLOG(1) << "Ignoring invalid certificate auto select setting:"
+              << " Invalid content settings pattern: " << pattern;
+      continue;
+    }
+
+    DCHECK(cert_filter->IsType(Value::TYPE_DICTIONARY));
+    value_map->SetValue(pattern,
+                        ContentSettingsPattern::Wildcard(),
+                        CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE,
+                        std::string(),
+                        cert_filter.release());
+  }
+}
+
 void PolicyProvider::ReadManagedContentSettings(bool overwrite) {
   {
     base::AutoLock auto_lock(lock_);
     if (overwrite)
       value_map_.clear();
     GetContentSettingsFromPreferences(&value_map_);
+    GetAutoSelectCertificateSettingsFromPreferences(&value_map_);
   }

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

Remove the curly braces on lines 427 and 433.

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

Done.

 }
 
@@ -380,6 +448,7 @@ ContentSetting PolicyProvider::GetContentSetting(
     const GURL& secondary_url,
     ContentSettingsType content_type,
     const ResourceIdentifier& resource_identifier) const {
+  DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
   // Resource identifier are not supported by policies as long as the feature is
   // behind a flag. So resource identifiers are simply ignored.
   scoped_ptr<Value> value(GetContentSettingValue(primary_url,