Skip to content

Commit

Permalink
Remove obsolete SECURITY_WARNING security level
Browse files Browse the repository at this point in the history
The SECURITY_WARNING state has been deprecated for some time and no
longer has any legitimate uses. Remove it entirely from the code.
Update handling of SECURE_WITH_POLICY_INSTALLED_CERT as appropriate,
although this state is currently only present on ChromeOS devices.

TBR: pkasting@chromium.org
Bug: 645698, 297249
Change-Id: I273b2a025ae3ecc1d0f8ad06ae423cd2efea942c
Reviewed-on: https://chromium-review.googlesource.com/613806
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Ted Choc (OOO 8.21-25) <tedchoc@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Rohit Rao (OOO until 8-30) <rohitrao@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497452}
  • Loading branch information
ericlaw1979 authored and Commit Bot committed Aug 25, 2017
1 parent da94c9e commit ec1bba9
Show file tree
Hide file tree
Showing 29 changed files with 54 additions and 114 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,7 @@ public void onDidDetachInterstitialPage(Tab tab) {

private boolean hasSecurityWarningOrError(Tab tab) {
int securityLevel = tab.getSecurityLevel();
return securityLevel == ConnectionSecurityLevel.DANGEROUS
|| securityLevel == ConnectionSecurityLevel.SECURITY_WARNING
|| securityLevel
== ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT;
return securityLevel == ConnectionSecurityLevel.DANGEROUS;
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1319,10 +1319,9 @@ public static int getSecurityIconResource(
return isSmallDevice ? 0 : R.drawable.omnibox_info;
case ConnectionSecurityLevel.HTTP_SHOW_WARNING:
return R.drawable.omnibox_info;
case ConnectionSecurityLevel.SECURITY_WARNING:
return R.drawable.omnibox_info;
case ConnectionSecurityLevel.DANGEROUS:
return R.drawable.omnibox_https_invalid;
case ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT:
case ConnectionSecurityLevel.SECURE:
case ConnectionSecurityLevel.EV_SECURE:
return R.drawable.omnibox_https_valid;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,11 @@ static class UrlEmphasisSecurityErrorSpan extends StrikethroughSpan
* @param isInternalPage Whether this page is an internal Chrome page.
* @param useDarkColors Whether the text colors should be dark (i.e.
* appropriate for use on a light background).
* @param emphasizeHttpsScheme Whether the https scheme should be emphasized.
* @param emphasizeScheme Whether the scheme should be emphasized.
*/
public static void emphasizeUrl(Spannable url, Resources resources, Profile profile,
int securityLevel, boolean isInternalPage,
boolean useDarkColors, boolean emphasizeHttpsScheme) {
int securityLevel, boolean isInternalPage, boolean useDarkColors,
boolean emphasizeScheme) {
String urlString = url.toString();
EmphasizeComponentsResponse emphasizeResponse =
parseForEmphasizeComponents(profile, urlString);
Expand All @@ -154,7 +154,7 @@ public static void emphasizeUrl(Spannable url, Resources resources, Profile prof
int startHostIndex = emphasizeResponse.hostStart;
int endHostIndex = emphasizeResponse.hostStart + emphasizeResponse.hostLength;

// Color the HTTPS scheme.
// Format the scheme, if present, based on the security level.
ForegroundColorSpan span;
if (emphasizeResponse.hasScheme()) {
int colorId = nonEmphasizedColorId;
Expand All @@ -164,8 +164,6 @@ public static void emphasizeUrl(Spannable url, Resources resources, Profile prof
case ConnectionSecurityLevel.NONE:
// Intentional fall-through:
case ConnectionSecurityLevel.HTTP_SHOW_WARNING:
// Intentional fall-through:
case ConnectionSecurityLevel.SECURITY_WARNING:
// Draw attention to the data: URI scheme for anti-spoofing reasons.
if (UrlConstants.DATA_SCHEME.equals(
emphasizeResponse.extractScheme(urlString))) {
Expand All @@ -174,13 +172,13 @@ public static void emphasizeUrl(Spannable url, Resources resources, Profile prof
}
break;
case ConnectionSecurityLevel.DANGEROUS:
if (emphasizeHttpsScheme) colorId = R.color.google_red_700;
if (emphasizeScheme) colorId = R.color.google_red_700;
strikeThroughScheme = true;
break;
case ConnectionSecurityLevel.EV_SECURE:
// Intentional fall-through:
case ConnectionSecurityLevel.SECURE:
if (emphasizeHttpsScheme) colorId = R.color.google_green_700;
if (emphasizeScheme) colorId = R.color.google_green_700;
break;
default:
assert false;
Expand All @@ -196,9 +194,8 @@ public static void emphasizeUrl(Spannable url, Resources resources, Profile prof
url.setSpan(
span, startSchemeIndex, endSchemeIndex, Spannable.SPAN_EXCLUSIVE_EXCLUSIVE);

// Highlight the portion of the URL visible between the scheme and the host. For
// https, this will be ://. For normal pages, this will be empty as we trim off
// http://.
// Highlight the portion of the URL visible between the scheme and the host,
// typically :// or : depending on the scheme.
if (emphasizeResponse.hasHost()) {
span = new UrlEmphasisColorSpan(
ApiCompatibilityUtils.getColor(resources, nonEmphasizedColorId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,6 @@ private int calculateThemeColor(boolean didWebContentsThemeColorChange) {
// Do not apply the theme color if there are any security issues on the page.
int securityLevel = getSecurityLevel();
if (securityLevel == ConnectionSecurityLevel.DANGEROUS
|| securityLevel == ConnectionSecurityLevel.SECURITY_WARNING
|| securityLevel == ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT) {
themeColor = getDefaultThemeColor();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ public boolean canAutoHideBrowserControls() {
enableHidingBrowserControls &= !url.startsWith(UrlConstants.CHROME_NATIVE_URL_PREFIX);

int securityState = mTab.getSecurityLevel();
enableHidingBrowserControls &= (securityState != ConnectionSecurityLevel.DANGEROUS
&& securityState != ConnectionSecurityLevel.SECURITY_WARNING);
enableHidingBrowserControls &= (securityState != ConnectionSecurityLevel.DANGEROUS);

enableHidingBrowserControls &= !AccessibilityUtil.isAccessibilityEnabled();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ protected boolean shouldShowBrowserControlsForUrl(WebappInfo info, String url) {
}

private boolean shouldShowBrowserControlsForSecurityLevel(int securityLevel) {
return securityLevel == ConnectionSecurityLevel.DANGEROUS
|| securityLevel == ConnectionSecurityLevel.SECURITY_WARNING;
return securityLevel == ConnectionSecurityLevel.DANGEROUS;
}

private boolean shouldShowBrowserControlsForDisplayMode(WebappInfo info) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,29 +214,26 @@ public void testLongInsecureHTTPSUrl() throws Throwable {
}

/**
* Verify that a very short, warning HTTPS URL is colored correctly by
* Verify that a very short, HTTP Warning URL is colored correctly by
* OmniboxUrlEmphasizer.emphasizeUrl().
*/
@Test
@MediumTest
@UiThreadTest
@Feature({"Browser", "Main"})
public void testVeryShortWarningHTTPSUrl() throws Throwable {
Spannable url = new SpannableStringBuilder("https://www.dodgysite.com");
public void testVeryShortHTTPWarningUrl() throws Throwable {
Spannable url = new SpannableStringBuilder("m.w.co/p");
OmniboxUrlEmphasizer.emphasizeUrl(url, mResources, mProfile,
ConnectionSecurityLevel.SECURITY_WARNING, false, true, true);
ConnectionSecurityLevel.HTTP_SHOW_WARNING, false, true, false);
EmphasizedUrlSpanHelper[] spans = EmphasizedUrlSpanHelper.getSpansForEmphasizedUrl(url);

Assert.assertEquals("Unexpected number of spans:", 3, spans.length);
spans[0].assertIsColoredSpan("https", 0,
Assert.assertEquals("Unexpected number of spans:", 2, spans.length);
spans[0].assertIsColoredSpan("m.w.co", 0,
ApiCompatibilityUtils.getColor(
mResources, R.color.url_emphasis_non_emphasized_text));
spans[1].assertIsColoredSpan("://", 5,
mResources, R.color.url_emphasis_domain_and_registry));
spans[1].assertIsColoredSpan("/p", 6,
ApiCompatibilityUtils.getColor(
mResources, R.color.url_emphasis_non_emphasized_text));
spans[2].assertIsColoredSpan("www.dodgysite.com", 8,
ApiCompatibilityUtils.getColor(
mResources, R.color.url_emphasis_domain_and_registry));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,11 @@ public void testUrlDisplay() {
final String urlExpectedWhenIconShown = host;
final int[] securityLevels = {ConnectionSecurityLevel.NONE,
ConnectionSecurityLevel.EV_SECURE, ConnectionSecurityLevel.SECURE,
ConnectionSecurityLevel.SECURITY_WARNING,
ConnectionSecurityLevel.HTTP_SHOW_WARNING,
ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT,
ConnectionSecurityLevel.DANGEROUS};

for (int i : securityLevels) {
// TODO(palmer): http://crbug.com/297249
if (i == ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT) continue;
urlBar.update(url, i);

int iconResource = urlBar.getCurrentIconResourceForTests();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ private static void testCanAutoHideBrowserControls(Type type) {
Assert.assertTrue(
canAutoHideBrowserControls(type, ConnectionSecurityLevel.HTTP_SHOW_WARNING));
Assert.assertFalse(canAutoHideBrowserControls(type, ConnectionSecurityLevel.DANGEROUS));
Assert.assertFalse(
canAutoHideBrowserControls(type, ConnectionSecurityLevel.SECURITY_WARNING));
}

private static void testShouldShowBrowserControls(Type type, int displayMode) {
Expand Down Expand Up @@ -104,19 +102,13 @@ private static void testShouldShowBrowserControls(Type type, int displayMode) {
Assert.assertFalse(shouldShowBrowserControls(
WEBAPP_URL, "", ConnectionSecurityLevel.NONE, type, displayMode));

// Show browser controls for non secure URLs.
// Show browser controls for Dangerous URLs.
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, WEBAPP_URL,
ConnectionSecurityLevel.SECURITY_WARNING, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, WEBAPP_URL + "/things.html",
ConnectionSecurityLevel.SECURITY_WARNING, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, WEBAPP_URL + "/stuff.html",
ConnectionSecurityLevel.SECURITY_WARNING, type, displayMode));
ConnectionSecurityLevel.DANGEROUS, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, WEBAPP_URL + "/stuff.html",
ConnectionSecurityLevel.DANGEROUS, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, WEBAPP_URL + "/things.html",
ConnectionSecurityLevel.DANGEROUS, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, "http://sub.originalwebsite.com",
ConnectionSecurityLevel.SECURITY_WARNING, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, "http://notoriginalwebsite.com",
ConnectionSecurityLevel.DANGEROUS, type, displayMode));
Assert.assertTrue(shouldShowBrowserControls(WEBAPP_URL, "http://otherwebsite.com",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@
public final class LocationBarLayoutTest {
private static final boolean IS_SMALL_DEVICE = true;
private static final boolean IS_OFFLINE_PAGE = true;
private static final int[] SECURITY_LEVELS =
new int[] {ConnectionSecurityLevel.NONE, ConnectionSecurityLevel.HTTP_SHOW_WARNING,
ConnectionSecurityLevel.SECURITY_WARNING, ConnectionSecurityLevel.DANGEROUS,
ConnectionSecurityLevel.SECURE, ConnectionSecurityLevel.EV_SECURE};
private static final int[] SECURITY_LEVELS = new int[] {ConnectionSecurityLevel.NONE,
ConnectionSecurityLevel.HTTP_SHOW_WARNING, ConnectionSecurityLevel.DANGEROUS,
ConnectionSecurityLevel.SECURE, ConnectionSecurityLevel.EV_SECURE};

@Mock
private Tab mTab;
Expand Down Expand Up @@ -82,20 +81,22 @@ public void testGetSecurityIconResource() {
LocationBarLayout.getSecurityIconResource(ConnectionSecurityLevel.HTTP_SHOW_WARNING,
!IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));

assertEquals(R.drawable.omnibox_info,
LocationBarLayout.getSecurityIconResource(ConnectionSecurityLevel.SECURITY_WARNING,
IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));
assertEquals(R.drawable.omnibox_info,
LocationBarLayout.getSecurityIconResource(ConnectionSecurityLevel.SECURITY_WARNING,
!IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));

assertEquals(R.drawable.omnibox_https_invalid,
LocationBarLayout.getSecurityIconResource(
ConnectionSecurityLevel.DANGEROUS, IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));
assertEquals(R.drawable.omnibox_https_invalid,
LocationBarLayout.getSecurityIconResource(
ConnectionSecurityLevel.DANGEROUS, !IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));

assertEquals(R.drawable.omnibox_https_valid,
LocationBarLayout.getSecurityIconResource(
ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT, IS_SMALL_DEVICE,
!IS_OFFLINE_PAGE));
assertEquals(R.drawable.omnibox_https_valid,
LocationBarLayout.getSecurityIconResource(
ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT, !IS_SMALL_DEVICE,
!IS_OFFLINE_PAGE));

assertEquals(R.drawable.omnibox_https_valid,
LocationBarLayout.getSecurityIconResource(
ConnectionSecurityLevel.SECURE, IS_SMALL_DEVICE, !IS_OFFLINE_PAGE));
Expand Down
12 changes: 2 additions & 10 deletions chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,6 @@
return in_dark_mode ? skia::SkColorToSRGBNSColor(SK_ColorWHITE)
: skia::SkColorToSRGBNSColor(gfx::kGoogleGreen700);
}
NSColor* SecurityWarningSchemeColor(bool in_dark_mode) {
return in_dark_mode
? skia::SkColorToSRGBNSColor(SkColorSetA(SK_ColorWHITE, 0x7F))
: skia::SkColorToSRGBNSColor(gfx::kGoogleYellow700);
}
NSColor* SecurityErrorSchemeColor(bool in_dark_mode) {
return in_dark_mode
? skia::SkColorToSRGBNSColor(SkColorSetA(SK_ColorWHITE, 0x7F))
Expand Down Expand Up @@ -161,11 +156,8 @@ void StoreStateToTab(WebContents* tab,
return SecureSchemeColor(in_dark_mode);
}

if (security_level == security_state::DANGEROUS)
return SecurityErrorSchemeColor(in_dark_mode);

DCHECK_EQ(security_state::SECURITY_WARNING, security_level);
return SecurityWarningSchemeColor(in_dark_mode);
DCHECK_EQ(security_state::DANGEROUS, security_level);
return SecurityErrorSchemeColor(in_dark_mode);
}

OmniboxViewMac::OmniboxViewMac(OmniboxEditController* controller,
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/vr/elements/url_bar_texture.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "chrome/browser/vr/elements/url_bar_texture.h"

#include <utility>

#include "base/strings/utf_string_conversions.h"
#include "cc/paint/skia_paint_canvas.h"
#include "chrome/browser/vr/color_scheme.h"
Expand Down Expand Up @@ -46,8 +48,6 @@ SkColor GetSchemeColor(SecurityLevel level, const ColorScheme& color_scheme) {
case SecurityLevel::EV_SECURE:
case SecurityLevel::SECURE:
return color_scheme.secure;
case SecurityLevel::SECURITY_WARNING:
return color_scheme.url_deemphasized;
case SecurityLevel::SECURE_WITH_POLICY_INSTALLED_CERT: // ChromeOS only.
return color_scheme.insecure;
case SecurityLevel::DANGEROUS:
Expand Down
2 changes: 0 additions & 2 deletions components/security_state/content/content_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ blink::WebSecurityStyle SecurityLevelToSecurityStyle(
case security_state::NONE:
case security_state::HTTP_SHOW_WARNING:
return blink::kWebSecurityStyleNeutral;
case security_state::SECURITY_WARNING:
case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
return blink::kWebSecurityStyleWarning;
case security_state::EV_SECURE:
case security_state::SECURE:
return blink::kWebSecurityStyleSecure;
Expand Down
5 changes: 2 additions & 3 deletions components/security_state/core/security_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,10 @@ enum SecurityLevel {
// HTTPS (non-EV) with valid cert.
SECURE,

// Obsolete, do not use. TODO(lgarron): Remove via https://crbug.com/645698.
SECURITY_WARNING,

// HTTPS, but the certificate verification chain is anchored on a
// certificate that was installed by the system administrator.
//
// Currently used only on ChromeOS.
SECURE_WITH_POLICY_INSTALLED_CERT,

// Attempted HTTPS and failed, page not authenticated, HTTPS with
Expand Down
1 change: 0 additions & 1 deletion components/security_state/core/security_state_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ bool ShouldAlwaysShowIcon(SecurityLevel security_level) {
case HTTP_SHOW_WARNING:
case EV_SECURE:
case SECURE:
case SECURITY_WARNING:
case SECURE_WITH_POLICY_INSTALLED_CERT:
case DANGEROUS:
return true;
Expand Down
3 changes: 0 additions & 3 deletions components/toolbar/toolbar_model_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,6 @@ const gfx::VectorIcon& ToolbarModelImpl::GetVectorIcon() const {
case security_state::EV_SECURE:
case security_state::SECURE:
return toolbar::kHttpsValidIcon;
case security_state::SECURITY_WARNING:
// Surface Dubious as Neutral.
return toolbar::kHttpIcon;
case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
return vector_icons::kBusinessIcon;
case security_state::DANGEROUS:
Expand Down
5 changes: 3 additions & 2 deletions content/browser/devtools/protocol/security_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@

#include "content/browser/devtools/protocol/security_handler.h"

#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "base/base64.h"
#include "content/browser/devtools/devtools_session.h"
Expand Down Expand Up @@ -34,8 +37,6 @@ std::string SecurityStyleToProtocolSecurityState(
return Security::SecurityStateEnum::Neutral;
case blink::kWebSecurityStyleInsecure:
return Security::SecurityStateEnum::Insecure;
case blink::kWebSecurityStyleWarning:
return Security::SecurityStateEnum::Warning;
case blink::kWebSecurityStyleSecure:
return Security::SecurityStateEnum::Secure;
default:
Expand Down
3 changes: 0 additions & 3 deletions ios/chrome/browser/ui/omnibox/omnibox_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ int GetIconForSecurityState(security_state::SecurityLevel security_level) {
case security_state::EV_SECURE:
case security_state::SECURE:
return IDR_IOS_OMNIBOX_HTTPS_VALID;
case security_state::SECURITY_WARNING:
// Surface Dubious as Neutral.
return IDR_IOS_OMNIBOX_HTTP;
case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
return IDR_IOS_OMNIBOX_HTTPS_POLICY_WARNING;
case security_state::DANGEROUS:
Expand Down
1 change: 0 additions & 1 deletion ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,6 @@ - (void)onDeleteBackward {
return;
}

DCHECK_NE(security_state::SECURITY_WARNING, security_level);
DCHECK_NE(security_state::SECURE_WITH_POLICY_INSTALLED_CERT, security_level);

if (security_level == security_state::DANGEROUS) {
Expand Down
3 changes: 0 additions & 3 deletions ios/web/net/request_tracker_impl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,6 @@ - (NSString*)description {
case web::SECURITY_STYLE_AUTHENTICATION_BROKEN:
sslInfo = @"Not secure ";
break;
case web::SECURITY_STYLE_WARNING:
sslInfo = @"Security warning";
break;
case web::SECURITY_STYLE_AUTHENTICATED:
if (status_.content_status ==
web::SSLStatus::DISPLAYED_INSECURE_CONTENT)
Expand Down
5 changes: 0 additions & 5 deletions ios/web/public/security_style.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ enum SecurityStyle {
// this object in an authenticated manner but were unable to do so.
SECURITY_STYLE_AUTHENTICATION_BROKEN,

// SECURITY_STYLE_WARNING means that the object was retrieved in an
// authenticated manner, but there were security issues with the retrieval or
// the object interacted with less secure objects.
SECURITY_STYLE_WARNING,

// SECURITY_STYLE_AUTHENTICATED indicates that we successfully retrieved this
// object over an authenticated protocol, such as HTTPS.
SECURITY_STYLE_AUTHENTICATED,
Expand Down
Loading

0 comments on commit ec1bba9

Please sign in to comment.