Remove DenyCertForHost from SSLHostStateDelegate API.

chrome/browser/ssl/chrome_ssl_host_state_delegate.cc

Index: chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
diff --git a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
index c306b22f0a9295b7f8e582a8e977fa859be4bc42..6a34bceb69d231c0cde0003e518aa4a6487e5d2f 100644
--- a/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
+++ b/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
@@ -38,7 +38,7 @@ const char kRememberCertificateErrorDecisionsFieldTrialDefaultGroup[] =
     "Default";
 const char kRememberCertificateErrorDecisionsFieldTrialLengthParam[] = "length";
 
-// Keys for the per-site error + certificate finger to judgement content
+// Keys for the per-site error + certificate finger to judgment content
 // settings map.
 const char kSSLCertDecisionCertErrorMapKey[] = "cert_exceptions_map";
 const char kSSLCertDecisionExpirationTimeKey[] = "decision_expiration_time";
@@ -49,9 +49,9 @@ const int kDefaultSSLCertDecisionVersion = 1;
 // Closes all idle network connections for the given URLRequestContext. This is
 // a big hammer and should be wielded with extreme caution as it can have a big,
 // negative impact on network performance. In this case, it is used by
-// RevokeUserDecisionsHard, which should only be called by rare, user initiated
-// events. See the comment before RevokeUserDecisionsHard implementation for
-// more information.
+// RevokeUserAllowExceptionsHard, which should only be called by rare, user
+// initiated events. See the comment before RevokeUserAllowExceptionsHard
+// implementation for more information.

[Ryan Sleevi, 2014/09/04 00:17:25]

These last two sentences are 'layering' violations, in that a good comment
should try to not focus on how it's used/called, but how it should/can be
used/called.

I feel like they don't add much value, and you're better off documenting any
concerns here in the ExceptionsHard call site.

[jww, 2014/09/04 01:06:30]

Done.

 void CloseIdleConnections(
     scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
   url_request_context_getter->
@@ -262,16 +262,43 @@ ChromeSSLHostStateDelegate::~ChromeSSLHostStateDelegate() {
     Clear();
 }
 
-void ChromeSSLHostStateDelegate::DenyCert(const std::string& host,
-                                          net::X509Certificate* cert,
-                                          net::CertStatus error) {
-  ChangeCertPolicy(host, cert, error, net::CertPolicy::DENIED);
-}
-
 void ChromeSSLHostStateDelegate::AllowCert(const std::string& host,
                                            net::X509Certificate* cert,
                                            net::CertStatus error) {
-  ChangeCertPolicy(host, cert, error, net::CertPolicy::ALLOWED);
+  GURL url = GetSecureGURLForHost(host);

[Ryan Sleevi, 2014/09/04 00:17:25]

how does this (or not) handle websockets? (wss:// vs https://)

I believe the *intent* is that wss:// exceptions behave the same as https://
exceptions, but since you're synthesizing to a ContentSettingsPattern, I want to
make sure that the scheme consideration in content settings takes this into
consideration.

[jww, 2014/09/04 01:06:30]

Great question. Yes, wss:// is taken care of by all of this. In fact, there was
a bug earlier where wss:// was *not* taken care of, back when we passed around
GURLs instead of just host names, and there was a unit test somewhere (I don't
remember right now where) that caught that problem. Thus, now, we just query
about hosts, which is why we make the GetSecureGURLForHost call to create a
content setting friendly GURL.

So in short, wss:// is most definitely taken care of.

+  const ContentSettingsPattern pattern =
+      ContentSettingsPattern::FromURLNoWildcard(url);
+  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+  scoped_ptr<base::Value> value(map->GetWebsiteSetting(
+      url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, std::string(), NULL));
+
+  if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
+    value.reset(new base::DictionaryValue());
+
+  base::DictionaryValue* dict;
+  bool success = value->GetAsDictionary(&dict);
+  DCHECK(success);
+
+  bool expired_previous_decision;  // unused value in this function
+  base::DictionaryValue* cert_dict = GetValidCertDecisionsDict(
+      dict, CreateDictionaryEntries, &expired_previous_decision);
+  // If a a valid certificate dictionary cannot be extracted from the content
+  // setting, that means it's in an unknown format. Unfortunately, there's
+  // nothing to be done in that case, so a silent fail is the only option.
+  if (!cert_dict)
+    return;
+
+  dict->SetIntegerWithoutPathExpansion(kSSLCertDecisionVersionKey,
+                                       kDefaultSSLCertDecisionVersion);
+  cert_dict->SetIntegerWithoutPathExpansion(GetKey(cert, error), ALLOWED);
+
+  // The map takes ownership of the value, so it is released in the call to
+  // SetWebsiteSetting.
+  map->SetWebsiteSetting(pattern,
+                         pattern,
+                         CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS,
+                         std::string(),
+                         value.release());
 }
 
 void ChromeSSLHostStateDelegate::Clear() {
@@ -279,11 +306,11 @@ void ChromeSSLHostStateDelegate::Clear() {
       CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS);
 }
 
-net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
-    const std::string& host,
-    net::X509Certificate* cert,
-    net::CertStatus error,
-    bool* expired_previous_decision) {
+content::SSLHostStateDelegate::CertJudgment
+ChromeSSLHostStateDelegate::QueryPolicy(const std::string& host,
+                                        net::X509Certificate* cert,
+                                        net::CertStatus error,
+                                        bool* expired_previous_decision) {
   HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
   GURL url = GetSecureGURLForHost(host);
   scoped_ptr<base::Value> value(map->GetWebsiteSetting(
@@ -293,7 +320,7 @@ net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
   // full query.
   *expired_previous_decision = false;
   if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
-    return net::CertPolicy::UNKNOWN;
+    return DENIED;
 
   base::DictionaryValue* dict;  // Owned by value
   int policy_decision;
@@ -306,24 +333,23 @@ net::CertPolicy::Judgment ChromeSSLHostStateDelegate::QueryPolicy(
   if (!cert_error_dict) {
     // This revoke is necessary to clear any old expired setting that may
     // lingering in the case that an old decision expried.
-    RevokeUserDecisions(host);
-    return net::CertPolicy::UNKNOWN;
+    RevokeUserAllowExceptions(host);
+    return DENIED;
   }
 
   success = cert_error_dict->GetIntegerWithoutPathExpansion(GetKey(cert, error),
                                                             &policy_decision);
 
   // If a policy decision was successfully retrieved and it's a valid value of
-  // ALLOWED or DENIED, return the valid value. Otherwise, return UNKNOWN.
-  if (success && policy_decision == net::CertPolicy::Judgment::ALLOWED)
-    return net::CertPolicy::Judgment::ALLOWED;
-  else if (success && policy_decision == net::CertPolicy::Judgment::DENIED)
-    return net::CertPolicy::Judgment::DENIED;
+  // ALLOWED, return the valid value. Otherwise, return DENIED.
+  if (success && policy_decision == ALLOWED)
+    return ALLOWED;
 
-  return net::CertPolicy::Judgment::UNKNOWN;
+  return DENIED;
 }
 
-void ChromeSSLHostStateDelegate::RevokeUserDecisions(const std::string& host) {
+void ChromeSSLHostStateDelegate::RevokeUserAllowExceptions(
+    const std::string& host) {
   GURL url = GetSecureGURLForHost(host);
   const ContentSettingsPattern pattern =
       ContentSettingsPattern::FromURLNoWildcard(url);
@@ -348,19 +374,20 @@ void ChromeSSLHostStateDelegate::RevokeUserDecisions(const std::string& host) {
 // showing the interstitial. We probably need to introduce into the networking
 // stack a way revoke SSLConfig's allowed_bad_certs lists per socket.
 //
-// For now, RevokeUserDecisionsHard is our solution for the rare case where it
+// For now, RevokeUserAllowExceptionsHard is our solution for the rare case
+// where it
 // is necessary to revoke the preferences immediately. It does so by flushing
 // idle sockets.
-void ChromeSSLHostStateDelegate::RevokeUserDecisionsHard(
+void ChromeSSLHostStateDelegate::RevokeUserAllowExceptionsHard(
     const std::string& host) {
-  RevokeUserDecisions(host);
+  RevokeUserAllowExceptions(host);
   scoped_refptr<net::URLRequestContextGetter> getter(
       profile_->GetRequestContext());
   profile_->GetRequestContext()->GetNetworkTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&CloseIdleConnections, getter));
 }
 
-bool ChromeSSLHostStateDelegate::HasUserDecision(const std::string& host) {
+bool ChromeSSLHostStateDelegate::HasAllowed(const std::string& host) {
   GURL url = GetSecureGURLForHost(host);
   const ContentSettingsPattern pattern =
       ContentSettingsPattern::FromURLNoWildcard(url);
@@ -379,8 +406,7 @@ bool ChromeSSLHostStateDelegate::HasUserDecision(const std::string& host) {
   for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
     int policy_decision;  // Owned by dict
     success = it.value().GetAsInteger(&policy_decision);
-    if (success && (static_cast<net::CertPolicy::Judgment>(policy_decision) !=
-                    net::CertPolicy::UNKNOWN))
+    if (success && (static_cast<CertJudgment>(policy_decision) == ALLOWED))
       return true;
   }
 
@@ -400,44 +426,3 @@ bool ChromeSSLHostStateDelegate::DidHostRunInsecureContent(
 void ChromeSSLHostStateDelegate::SetClock(scoped_ptr<base::Clock> clock) {
   clock_.reset(clock.release());
 }
-
-void ChromeSSLHostStateDelegate::ChangeCertPolicy(
-    const std::string& host,
-    net::X509Certificate* cert,
-    net::CertStatus error,
-    net::CertPolicy::Judgment judgment) {
-  GURL url = GetSecureGURLForHost(host);
-  const ContentSettingsPattern pattern =
-      ContentSettingsPattern::FromURLNoWildcard(url);
-  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
-  scoped_ptr<base::Value> value(map->GetWebsiteSetting(
-      url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, std::string(), NULL));
-
-  if (!value.get() || !value->IsType(base::Value::TYPE_DICTIONARY))
-    value.reset(new base::DictionaryValue());
-
-  base::DictionaryValue* dict;
-  bool success = value->GetAsDictionary(&dict);
-  DCHECK(success);
-
-  bool expired_previous_decision;  // unused value in this function
-  base::DictionaryValue* cert_dict = GetValidCertDecisionsDict(
-      dict, CreateDictionaryEntries, &expired_previous_decision);
-  // If a a valid certificate dictionary cannot be extracted from the content
-  // setting, that means it's in an unknown format. Unfortunately, there's
-  // nothing to be done in that case, so a silent fail is the only option.
-  if (!cert_dict)
-    return;
-
-  dict->SetIntegerWithoutPathExpansion(kSSLCertDecisionVersionKey,
-                                       kDefaultSSLCertDecisionVersion);
-  cert_dict->SetIntegerWithoutPathExpansion(GetKey(cert, error), judgment);
-
-  // The map takes ownership of the value, so it is released in the call to
-  // SetWebsiteSetting.
-  map->SetWebsiteSetting(pattern,
-                         pattern,
-                         CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS,
-                         std::string(),
-                         value.release());
-}