Remove secure mode error logic in DnsOverHttpsMode policy
The error logic for DnsOverHttpsMode policy's secure mode is no longer
needed. Remove the logic from the policy handler and update tests to
match the new behavior.
Also, correct the handler's header comment.
Bug: 955454
Change-Id: I6c471116c27f8c25c2f8222b58d4767ab1ef3b56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2020606
Commit-Queue: Steven Bingler <[email protected]>
Reviewed-by: Julian Pastarmov <[email protected]>
Reviewed-by: Eric Orth <[email protected]>
Cr-Commit-Position: refs/heads/master@{#747917}
diff --git a/chrome/browser/net/secure_dns_policy_handler.cc b/chrome/browser/net/secure_dns_policy_handler.cc
index 8e89e6d..90e7cc92 100644
--- a/chrome/browser/net/secure_dns_policy_handler.cc
+++ b/chrome/browser/net/secure_dns_policy_handler.cc
@@ -44,11 +44,9 @@
if (mode_str.size() == 0) {
errors->AddError(key::kDnsOverHttpsMode, IDS_POLICY_NOT_SPECIFIED_ERROR);
mode_is_applicable = false;
- } else if (mode_str == chrome_browser_net::kDnsOverHttpsModeSecure) {
- errors->AddError(key::kDnsOverHttpsMode,
- IDS_POLICY_SECURE_DNS_MODE_NOT_SUPPORTED_ERROR);
} else if (mode_str != chrome_browser_net::kDnsOverHttpsModeOff &&
- mode_str != chrome_browser_net::kDnsOverHttpsModeAutomatic) {
+ mode_str != chrome_browser_net::kDnsOverHttpsModeAutomatic &&
+ mode_str != chrome_browser_net::kDnsOverHttpsModeSecure) {
errors->AddError(key::kDnsOverHttpsMode,
IDS_POLICY_INVALID_SECURE_DNS_MODE_ERROR);
mode_is_applicable = false;
@@ -94,12 +92,11 @@
base::StringPiece mode_str;
if (mode && mode->is_string()) {
mode_str = mode->GetString();
- // TODO(http://crbug.com/955454): Include secure in conditional when
- // support is implemented.
- if (mode_str == chrome_browser_net::kDnsOverHttpsModeAutomatic) {
+ if (mode_str == chrome_browser_net::kDnsOverHttpsModeAutomatic ||
+ mode_str == chrome_browser_net::kDnsOverHttpsModeSecure) {
prefs->SetString(prefs::kDnsOverHttpsMode, mode_str.as_string());
} else {
- // Captures "off" and "secure".
+ // Captures "off".
prefs->SetString(prefs::kDnsOverHttpsMode,
chrome_browser_net::kDnsOverHttpsModeOff);
}
diff --git a/chrome/browser/net/secure_dns_policy_handler.h b/chrome/browser/net/secure_dns_policy_handler.h
index 8684d896..ee42cd2 100644
--- a/chrome/browser/net/secure_dns_policy_handler.h
+++ b/chrome/browser/net/secure_dns_policy_handler.h
@@ -13,7 +13,7 @@
namespace policy {
-// Handles DnsOverHttpsMode policy.
+// Handles DnsOverHttpsMode and DnsOverHttpsTemplates policies.
class SecureDnsPolicyHandler : public ConfigurationPolicyHandler {
public:
SecureDnsPolicyHandler();
diff --git a/chrome/browser/net/secure_dns_policy_handler_unittest.cc b/chrome/browser/net/secure_dns_policy_handler_unittest.cc
index 2507f53f..08ab428 100644
--- a/chrome/browser/net/secure_dns_policy_handler_unittest.cc
+++ b/chrome/browser/net/secure_dns_policy_handler_unittest.cc
@@ -180,30 +180,27 @@
EXPECT_EQ(mode, test_policy_value);
}
-// TODO(http://crbug.com/955454) This test should be modified once secure is a
-// valid policy value for DnsOverHttpsMode.
-TEST_F(SecureDnsPolicyHandlerTest, ModePolicySecureShouldError) {
- // Secure will eventually be a valid option, but for the moment it should
- // error.
+TEST_F(SecureDnsPolicyHandlerTest, ValidModePolicySecure) {
+ const std::string test_policy_value =
+ chrome_browser_net::kDnsOverHttpsModeSecure;
+
SetPolicyValue(key::kDnsOverHttpsMode,
- std::make_unique<base::Value>(
- chrome_browser_net::kDnsOverHttpsModeSecure));
+ std::make_unique<base::Value>(test_policy_value));
+
+ // The template policy requires a value if the mode is set to secure, so set
+ // it to anything.
+ SetPolicyValue(key::kDnsOverHttpsTemplates,
+ std::make_unique<base::Value>("https://foo.test/"));
CheckAndApplyPolicySettings();
- // Should have errors.
- auto expected_error =
- l10n_util::GetStringUTF16(IDS_POLICY_SECURE_DNS_MODE_NOT_SUPPORTED_ERROR);
- // The templates policy will also throw an error but we're not interested in
- // it for this test. Check the total count, but only look at the error we
- // want.
- EXPECT_EQ(errors().size(), 2U);
- EXPECT_EQ(errors().begin()->second, expected_error);
+ // Shouldn't error.
+ EXPECT_EQ(errors().size(), 0U);
std::string mode;
EXPECT_TRUE(prefs().GetString(prefs::kDnsOverHttpsMode, &mode));
- // Pref should have changed to "off."
- EXPECT_EQ(mode, chrome_browser_net::kDnsOverHttpsModeOff);
+ // Pref should now be the test value.
+ EXPECT_EQ(mode, test_policy_value);
}
TEST_F(SecureDnsPolicyHandlerTest, InvalidTemplatesPolicyValue) {
@@ -332,8 +329,6 @@
EXPECT_EQ(templates, test_policy_value);
}
-// TODO(http://crbug.com/955454) These tests should be modified once secure is a
-// valid policy value for DnsOverHttpsMode.
TEST_F(SecureDnsPolicyHandlerTest, TemplatesNotSetWithModeSecure) {
SetPolicyValue(key::kDnsOverHttpsMode,
std::make_unique<base::Value>(
@@ -341,17 +336,12 @@
CheckAndApplyPolicySettings();
- // Should have errors.
- // TODO(http://crbug.com/955454) Secure will eventually be a valid option, but
- // for the moment it should error.
- auto expected_error1 =
- l10n_util::GetStringUTF16(IDS_POLICY_SECURE_DNS_MODE_NOT_SUPPORTED_ERROR);
- auto expected_error2 = l10n_util::GetStringUTF16(
+ // Should have an error.
+ auto expected_error = l10n_util::GetStringUTF16(
IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR);
- ASSERT_EQ(errors().size(), 2U);
+ ASSERT_EQ(errors().size(), 1U);
auto it = errors().begin();
- EXPECT_EQ(it++->second, expected_error1);
- EXPECT_EQ(it->second, expected_error2);
+ EXPECT_EQ(it->second, expected_error);
// Pref should be set.
std::string templates;
@@ -369,17 +359,12 @@
CheckAndApplyPolicySettings();
- // Should have errors.
- // TODO(http://crbug.com/955454) Secure will eventually be a valid option, but
- // for the moment it should error.
- auto expected_error1 =
- l10n_util::GetStringUTF16(IDS_POLICY_SECURE_DNS_MODE_NOT_SUPPORTED_ERROR);
- auto expected_error2 = l10n_util::GetStringUTF16(
+ // Should have an error.
+ auto expected_error = l10n_util::GetStringUTF16(
IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR);
- ASSERT_EQ(errors().size(), 2U);
+ ASSERT_EQ(errors().size(), 1U);
auto it = errors().begin();
- EXPECT_EQ(it++->second, expected_error1);
- EXPECT_EQ(it->second, expected_error2);
+ EXPECT_EQ(it->second, expected_error);
// Pref should be set.
std::string templates;
@@ -398,16 +383,11 @@
CheckAndApplyPolicySettings();
- // Should have errors.
- // TODO(http://crbug.com/955454) For now the value "secure" for the mode
- // policy will throw an error but we're not interested in it for this test.
- // Check the total count but only look at the error we want. Modify this test
- // when "secure" becomes valid.
+ // Should have an error.
auto expected_error = l10n_util::GetStringUTF16(
IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR);
- ASSERT_EQ(errors().size(), 2U);
+ ASSERT_EQ(errors().size(), 1U);
auto it = errors().begin();
- ++it;
EXPECT_EQ(it->second, expected_error);
// Pref should be set.
@@ -461,8 +441,6 @@
EXPECT_EQ(templates, test_policy_value);
}
-// TODO(http://crbug.com/955454) This test should be modified once secure is a
-// valid policy value for DnsOverHttpsMode.
TEST_F(SecureDnsPolicyHandlerTest, TemplatesPolicyWithModeSecure) {
// The templates policy requires a valid Mode policy or it will give an error
// we're not testing for.
@@ -477,11 +455,8 @@
CheckAndApplyPolicySettings();
- // Should have an error
- auto expected_error =
- l10n_util::GetStringUTF16(IDS_POLICY_SECURE_DNS_MODE_NOT_SUPPORTED_ERROR);
- EXPECT_EQ(errors().size(), 1U);
- EXPECT_EQ(errors().begin()->second, expected_error);
+ // Shouldn't error.
+ EXPECT_EQ(errors().size(), 0U);
std::string templates;
EXPECT_TRUE(prefs().GetString(prefs::kDnsOverHttpsTemplates, &templates));
