Remove SSLStatus::security_style member and content::SecurityStyle

components/security_state/security_state_model.cc

Index: components/security_state/security_state_model.cc
diff --git a/components/security_state/security_state_model.cc b/components/security_state/security_state_model.cc
index 6239186adb55f7bb1ac73a4cdb58ecfab8ed1706..fad19e0708d2d163dc2ccac877adb8dacf0929a2 100644
--- a/components/security_state/security_state_model.cc
+++ b/components/security_state/security_state_model.cc
@@ -87,9 +87,8 @@ SecurityStateModel::SecurityLevel GetSecurityLevelForNonSecureFieldTrial(
 }
 
 SecurityStateModel::SHA1DeprecationStatus GetSHA1DeprecationStatus(
-    scoped_refptr<net::X509Certificate> cert,
     const SecurityStateModel::VisibleSecurityState& visible_security_state) {
-  if (!cert ||
+  if (!visible_security_state.certificate ||
       !(visible_security_state.cert_status &
         net::CERT_STATUS_SHA1_SIGNATURE_PRESENT))
     return SecurityStateModel::NO_DEPRECATED_SHA1;
@@ -97,10 +96,12 @@ SecurityStateModel::SHA1DeprecationStatus GetSHA1DeprecationStatus(
   // The internal representation of the dates for UI treatment of SHA-1.
   // See http://crbug.com/401365 for details.
   static const int64_t kJanuary2017 = INT64_C(13127702400000000);
-  if (cert->valid_expiry() >= base::Time::FromInternalValue(kJanuary2017))
+  if (visible_security_state.certificate->valid_expiry() >=
+      base::Time::FromInternalValue(kJanuary2017))
     return SecurityStateModel::DEPRECATED_SHA1_MAJOR;
   static const int64_t kJanuary2016 = INT64_C(13096080000000000);
-  if (cert->valid_expiry() >= base::Time::FromInternalValue(kJanuary2016))
+  if (visible_security_state.certificate->valid_expiry() >=
+      base::Time::FromInternalValue(kJanuary2016))
     return SecurityStateModel::DEPRECATED_SHA1_MINOR;
 
   return SecurityStateModel::NO_DEPRECATED_SHA1;
@@ -119,7 +120,6 @@ SecurityStateModel::ContentStatus GetContentStatus(bool displayed, bool ran) {
 SecurityStateModel::SecurityLevel GetSecurityLevelForRequest(

[felt, 2016/10/07 03:30:48]

The downside, I guess, is that this method gets slightly harder to follow.

[estark, 2016/10/07 06:34:31]

You think so? Oddly I find it easier to follow this way... but maybe that's only
because I just wrote it.

(In particular, one of the things I find confusing about the old way is that we
were implicitly assuming certain relationships between the
initial_security_level and other parts of the VisibleSecurityState, like we
weren't checking for active mixed content if the initial security level was
SECURITY_WARNING and just left it at SECURITY_WARNING. In practice this was fine
because we were never actually setting an initial security level of
SECURITY_WARNING anywhere, but it seems like a weird thing still.)

     const SecurityStateModel::VisibleSecurityState& visible_security_state,
     SecurityStateModelClient* client,
-    const scoped_refptr<net::X509Certificate>& cert,
     SecurityStateModel::SHA1DeprecationStatus sha1_status,
     SecurityStateModel::ContentStatus mixed_content_status,
     SecurityStateModel::ContentStatus content_with_cert_errors_status) {
@@ -132,93 +132,78 @@ SecurityStateModel::SecurityLevel GetSecurityLevelForRequest(
     return SecurityStateModel::DANGEROUS;
 
   GURL url = visible_security_state.url;
-  switch (visible_security_state.initial_security_level) {
-    case SecurityStateModel::NONE:
-    case SecurityStateModel::HTTP_SHOW_WARNING: {
-      if (!client->IsOriginSecure(url) && url.IsStandard()) {
-        return GetSecurityLevelForNonSecureFieldTrial(
-            visible_security_state.displayed_password_field_on_http ||
-            visible_security_state.displayed_credit_card_field_on_http);
-      }
-      return SecurityStateModel::NONE;
-    }
 
-    case SecurityStateModel::DANGEROUS:
-      return SecurityStateModel::DANGEROUS;
-
-    case SecurityStateModel::SECURITY_WARNING:
-    case SecurityStateModel::SECURE_WITH_POLICY_INSTALLED_CERT:
-      return visible_security_state.initial_security_level;
-
-    case SecurityStateModel::SECURE:
-    case SecurityStateModel::EV_SECURE: {
-      // Major cert errors and active mixed content will generally be
-      // downgraded by the embedder to DANGEROUS and handled above,
-      // but downgrade here just in case.
-      net::CertStatus cert_status = visible_security_state.cert_status;
-      if (net::IsCertStatusError(cert_status) &&
-          !net::IsCertStatusMinorError(cert_status)) {
-        return SecurityStateModel::DANGEROUS;
-      }
-      if (mixed_content_status == SecurityStateModel::CONTENT_STATUS_RAN ||
-          mixed_content_status ==
-              SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN ||
-          content_with_cert_errors_status ==
-              SecurityStateModel::CONTENT_STATUS_RAN ||
-          content_with_cert_errors_status ==
-              SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN) {
-        return SecurityStateModel::kRanInsecureContentLevel;
-      }
-
-      // Report if there is a policy cert first, before reporting any other
-      // authenticated-but-with-errors cases. A policy cert is a strong
-      // indicator of a MITM being present (the enterprise), while the
-      // other authenticated-but-with-errors indicate something may
-      // be wrong, or may be wrong in the future, but is unclear now.
-      if (client->UsedPolicyInstalledCertificate())
-        return SecurityStateModel::SECURE_WITH_POLICY_INSTALLED_CERT;
-
-      if (sha1_status == SecurityStateModel::DEPRECATED_SHA1_MAJOR)
-        return SecurityStateModel::DANGEROUS;
-      if (sha1_status == SecurityStateModel::DEPRECATED_SHA1_MINOR)
-        return SecurityStateModel::NONE;
-
-      // Active mixed content is handled above.
-      DCHECK_NE(SecurityStateModel::CONTENT_STATUS_RAN, mixed_content_status);
-      DCHECK_NE(SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN,
-                mixed_content_status);
-
-      if (mixed_content_status ==
-              SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
-          content_with_cert_errors_status ==
-              SecurityStateModel::CONTENT_STATUS_DISPLAYED) {
-        return SecurityStateModel::kDisplayedInsecureContentLevel;
-      }
-
-      if (net::IsCertStatusError(cert_status)) {
-        // Major cert errors are handled above.
-        DCHECK(net::IsCertStatusMinorError(cert_status));
-        return SecurityStateModel::NONE;
-      }
-      if (net::SSLConnectionStatusToVersion(
-              visible_security_state.connection_status) ==
-          net::SSL_CONNECTION_VERSION_SSL3) {
-        // SSLv3 will be removed in the future.
-        return SecurityStateModel::SECURITY_WARNING;
-      }
-      if ((cert_status & net::CERT_STATUS_IS_EV) && cert)
-        return SecurityStateModel::EV_SECURE;
-      return SecurityStateModel::SECURE;
+  bool is_cryptographic_with_certificate =
+      (url.SchemeIsCryptographic() && visible_security_state.certificate);
+
+  // Set the security level to DANGEROUS for major certificate errors.
+  if (is_cryptographic_with_certificate &&
+      net::IsCertStatusError(visible_security_state.cert_status) &&
+      !net::IsCertStatusMinorError(visible_security_state.cert_status)) {
+    return SecurityStateModel::DANGEROUS;
+  }
+
+  // Choose the appropriate security level for HTTP requests.
+  if (!is_cryptographic_with_certificate) {
+    if (!client->IsOriginSecure(url) && url.IsStandard()) {
+      return GetSecurityLevelForNonSecureFieldTrial(
+          visible_security_state.displayed_password_field_on_http ||
+          visible_security_state.displayed_credit_card_field_on_http);
     }
+    return SecurityStateModel::NONE;
+  }
+
+  // Downgrade the security level for active insecure subresources.
+  if (mixed_content_status == SecurityStateModel::CONTENT_STATUS_RAN ||
+      mixed_content_status ==
+          SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN ||
+      content_with_cert_errors_status ==
+          SecurityStateModel::CONTENT_STATUS_RAN ||
+      content_with_cert_errors_status ==
+          SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN) {
+    return SecurityStateModel::kRanInsecureContentLevel;

[felt, 2016/10/07 03:30:48]

Tangent: why do we have this constant instead of just directly marking the state
here?

[estark, 2016/10/07 06:34:31]

We send this constant to devtools so that devtools knows what icon to use when
it says that there's mixed content on the page. (See
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...)

e.g. if we were to change to start showing red triangle for passive mixed
content, we would change this constant and then devtools would start magically
showing red when it warns about passive mixed content.

[felt, 2016/10/07 15:08:10]

Ah, right on.

+  }
+
+  // Report if there is a policy cert first, before reporting any other
+  // authenticated-but-with-errors cases. A policy cert is a strong
+  // indicator of a MITM being present (the enterprise), while the
+  // other authenticated-but-with-errors indicate something may
+  // be wrong, or may be wrong in the future, but is unclear now.
+  if (client->UsedPolicyInstalledCertificate())
+    return SecurityStateModel::SECURE_WITH_POLICY_INSTALLED_CERT;
+
+  if (sha1_status == SecurityStateModel::DEPRECATED_SHA1_MAJOR)
+    return SecurityStateModel::DANGEROUS;
+  if (sha1_status == SecurityStateModel::DEPRECATED_SHA1_MINOR)
+    return SecurityStateModel::NONE;
+
+  // Active mixed content is handled above.
+  DCHECK_NE(SecurityStateModel::CONTENT_STATUS_RAN, mixed_content_status);
+  DCHECK_NE(SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN,
+            mixed_content_status);
+
+  if (mixed_content_status == SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
+      content_with_cert_errors_status ==
+          SecurityStateModel::CONTENT_STATUS_DISPLAYED) {
+    return SecurityStateModel::kDisplayedInsecureContentLevel;
+  }
+
+  if (net::IsCertStatusError(visible_security_state.cert_status)) {
+    // Major cert errors are handled above.
+    DCHECK(net::IsCertStatusMinorError(visible_security_state.cert_status));
+    return SecurityStateModel::NONE;
   }
 
-  return SecurityStateModel::NONE;
+  if ((visible_security_state.cert_status & net::CERT_STATUS_IS_EV) &&
+      visible_security_state.certificate) {
+    return SecurityStateModel::EV_SECURE;
+  }
+  return SecurityStateModel::SECURE;
 }
 
 void SecurityInfoForRequest(
     SecurityStateModelClient* client,
     const SecurityStateModel::VisibleSecurityState& visible_security_state,
-    const scoped_refptr<net::X509Certificate>& cert,
     SecurityStateModel::SecurityInfo* security_info) {
   if (!visible_security_state.connection_info_initialized) {
     *security_info = SecurityStateModel::SecurityInfo();
@@ -226,8 +211,7 @@ void SecurityInfoForRequest(
         visible_security_state.fails_malware_check;
     if (security_info->fails_malware_check) {
       security_info->security_level = GetSecurityLevelForRequest(
-          visible_security_state, client, cert,
-          SecurityStateModel::UNKNOWN_SHA1,
+          visible_security_state, client, SecurityStateModel::UNKNOWN_SHA1,
           SecurityStateModel::CONTENT_STATUS_UNKNOWN,
           SecurityStateModel::CONTENT_STATUS_UNKNOWN);
     }
@@ -235,7 +219,7 @@ void SecurityInfoForRequest(
   }
   security_info->certificate = visible_security_state.certificate;
   security_info->sha1_deprecation_status =
-      GetSHA1DeprecationStatus(cert, visible_security_state);
+      GetSHA1DeprecationStatus(visible_security_state);
   security_info->mixed_content_status =
       GetContentStatus(visible_security_state.displayed_mixed_content,
                        visible_security_state.ran_mixed_content);
@@ -258,8 +242,7 @@ void SecurityInfoForRequest(
       visible_security_state.fails_malware_check;
 
   security_info->security_level = GetSecurityLevelForRequest(
-      visible_security_state, client, cert,
-      security_info->sha1_deprecation_status,
+      visible_security_state, client, security_info->sha1_deprecation_status,
       security_info->mixed_content_status,
       security_info->content_with_cert_errors_status);
 }
@@ -295,12 +278,9 @@ SecurityStateModel::~SecurityStateModel() {}
 
 void SecurityStateModel::GetSecurityInfo(
     SecurityStateModel::SecurityInfo* result) const {
-  scoped_refptr<net::X509Certificate> cert = nullptr;
-  client_->RetrieveCert(&cert);
-
   VisibleSecurityState new_visible_state;
   client_->GetVisibleSecurityState(&new_visible_state);
-  SecurityInfoForRequest(client_, new_visible_state, cert, result);
+  SecurityInfoForRequest(client_, new_visible_state, result);
 }
 
 void SecurityStateModel::SetClient(SecurityStateModelClient* client) {
@@ -308,8 +288,7 @@ void SecurityStateModel::SetClient(SecurityStateModelClient* client) {
 }
 
 SecurityStateModel::VisibleSecurityState::VisibleSecurityState()
-    : initial_security_level(SecurityStateModel::NONE),
-      fails_malware_check(false),
+    : fails_malware_check(false),
       connection_info_initialized(false),
       cert_status(0),
       connection_status(0),
@@ -328,7 +307,6 @@ SecurityStateModel::VisibleSecurityState::~VisibleSecurityState() {}
 bool SecurityStateModel::VisibleSecurityState::operator==(
     const SecurityStateModel::VisibleSecurityState& other) const {
   return (url == other.url &&
-          initial_security_level == other.initial_security_level &&
           fails_malware_check == other.fails_malware_check &&
           !!certificate == !!other.certificate &&
           (certificate ? certificate->Equals(other.certificate.get()) : true) &&