Coupling AllowFileSelectionDialogs-Pollicy and PromptForDownload.

When file-selection dialogs are forbidden by policy it is better to force the PromptForDownload preference to false and automatically download to the default location, since otherwise the user would just be prompted with a message saying that file-selection dialogs are disallowed.

Also cleaned up ConfigurationPolicyPrefStore* tests.

BUG=73174
TEST=Manually set the policy to false and verify that the PromptForDownload preference is false and cannot be turned on. Added ConfigurationPolicyPrefStorePromptDownloadTest.*.


Review URL: http://codereview.chromium.org/6927008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84279 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 128 insertions, 12 deletions

diff --git a/chrome/browser/download/download_prefs.cc b/chrome/browser/download/download_prefs.cc
index 281adf4..6d1cd67 100644
--- a/chrome/browser/download/download_prefs.cc
+++ b/chrome/browser/download/download_prefs.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/download/download_prefs.h"
 
 #include "base/file_util.h"
+#include "base/logging.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
@@ -79,7 +80,10 @@ void DownloadPrefs::RegisterUserPrefs(PrefService* prefs) {
 }
 
 bool DownloadPrefs::PromptForDownload() const {
-  return *prompt_for_download_ && !download_path_.IsManaged();
+  // If the DownloadDirectory policy is set, then |prompt_for_download_| should
+  // always be false.
+  DCHECK(!download_path_.IsManaged() || !prompt_for_download_.GetValue());
+  return *prompt_for_download_;
 }
 
 bool DownloadPrefs::IsDownloadPathManaged() const {
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index b5ab3d0..0d71a3b 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -92,6 +92,13 @@ class ConfigurationPolicyPrefKeeper
   // ownership of |value| in the case that the policy is recognized.
   bool ApplyDownloadDirPolicy(ConfigurationPolicyType policy, Value* value);
 
+  // Processes file-selection dialogs policy. Returns true if the specified
+  // policy is the file-selection dialogs policy.
+  // ApplyFileSelectionDialogsPolicy assumes the ownership of |value| in the
+  // case that the policy is recognized.
+  bool ApplyFileSelectionDialogsPolicy(ConfigurationPolicyType policy,
+                                       Value* value);
+
   // Make sure that the |path| if present in |prefs_|.  If not, set it to
   // a blank string.
   void EnsureStringPrefExists(const std::string& path);
@@ -335,6 +342,9 @@ void ConfigurationPolicyPrefKeeper::Apply(ConfigurationPolicyType policy,
   if (ApplyDownloadDirPolicy(policy, value))
     return;
 
+  if (ApplyFileSelectionDialogsPolicy(policy, value))
+    return;
+
   if (ApplyPolicyFromMap(policy, value, kDefaultSearchPolicyMap,
                          arraysize(kDefaultSearchPolicyMap)))
     return;
@@ -443,6 +453,27 @@ bool ConfigurationPolicyPrefKeeper::ApplyDownloadDirPolicy(
   return false;
 }
 
+bool ConfigurationPolicyPrefKeeper::ApplyFileSelectionDialogsPolicy(
+    ConfigurationPolicyType policy,
+    Value* value) {
+  if (policy == kPolicyAllowFileSelectionDialogs) {
+    prefs_.SetValue(prefs::kAllowFileSelectionDialogs, value);
+    // If file-selection dialogs are not allowed we forbid the user to be
+    // prompted for the download location, since this would end up in an Infobar
+    // explaining that file-selection dialogs are forbidden anyways.
+    bool allow_file_selection_dialogs = true;
+    bool result = value->GetAsBoolean(&allow_file_selection_dialogs);
+    DCHECK(result);
+    if (!allow_file_selection_dialogs) {
+      prefs_.SetValue(prefs::kPromptForDownload,
+                      Value::CreateBooleanValue(false));
+    }
+    return true;
+  }
+  // We are not interested in this policy.
+  return false;
+}
+
 void ConfigurationPolicyPrefKeeper::EnsureStringPrefExists(
     const std::string& path) {
   std::string value;
diff --git a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
index d4f515a..f780430 100644
--- a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
@@ -61,9 +61,10 @@ TEST_P(ConfigurationPolicyPrefStoreListTest, SetValue) {
   in_value->Append(Value::CreateStringValue("test2,"));
   provider_.AddPolicy(GetParam().type(), in_value);
   store_->OnUpdatePolicy();
-  const Value* value;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
+  ASSERT_TRUE(value);
   EXPECT_TRUE(in_value->Equals(value));
 }
 
@@ -101,9 +102,10 @@ TEST_P(ConfigurationPolicyPrefStoreStringTest, SetValue) {
   provider_.AddPolicy(GetParam().type(),
                       Value::CreateStringValue("http://chromium.org"));
   store_->OnUpdatePolicy();
-  const Value* value;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
+  ASSERT_TRUE(value);
   EXPECT_TRUE(StringValue("http://chromium.org").Equals(value));
 }
 
@@ -142,18 +144,24 @@ TEST_P(ConfigurationPolicyPrefStoreBooleanTest, GetDefault) {
 TEST_P(ConfigurationPolicyPrefStoreBooleanTest, SetValue) {
   provider_.AddPolicy(GetParam().type(), Value::CreateBooleanValue(false));
   store_->OnUpdatePolicy();
-  const Value* value;
-  bool result = true;
+  const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
-  EXPECT_TRUE(FundamentalValue(false).Equals(value));
+  ASSERT_TRUE(value);
+  bool boolean_value = true;
+  bool result = value->GetAsBoolean(&boolean_value);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(boolean_value);
 
   provider_.AddPolicy(GetParam().type(), Value::CreateBooleanValue(true));
   store_->OnUpdatePolicy();
-  result = false;
+  value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(GetParam().pref_name(), &value));
-  EXPECT_TRUE(FundamentalValue(true).Equals(value));
+  boolean_value = false;
+  result = value->GetAsBoolean(&boolean_value);
+  ASSERT_TRUE(result);
+  EXPECT_TRUE(boolean_value);
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -673,8 +681,72 @@ TEST_F(ConfigurationPolicyPrefStoreSyncTest, Disabled) {
   // Sync should be flagged as managed.
   const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK, store_->GetValue(prefs::kSyncManaged, &value));
-  ASSERT_TRUE(value != NULL);
-  EXPECT_TRUE(FundamentalValue(true).Equals(value));
+  ASSERT_TRUE(value);
+  bool sync_managed = false;
+  bool result = value->GetAsBoolean(&sync_managed);
+  ASSERT_TRUE(result);
+  EXPECT_TRUE(sync_managed);
+}
+
+// Test cases for how the DownloadDirectory and AllowFileSelectionDialogs policy
+// influence the PromptForDownload preference.
+class ConfigurationPolicyPrefStorePromptDownloadTest
+    : public ConfigurationPolicyPrefStoreTestBase<testing::Test> {
+};
+
+TEST_F(ConfigurationPolicyPrefStorePromptDownloadTest, Default) {
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            store_->GetValue(prefs::kPromptForDownload, NULL));
+}
+
+TEST_F(ConfigurationPolicyPrefStorePromptDownloadTest, SetDownloadDirectory) {
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            store_->GetValue(prefs::kPromptForDownload, NULL));
+  provider_.AddPolicy(kPolicyDownloadDirectory, Value::CreateStringValue(""));
+  store_->OnUpdatePolicy();
+
+  // Setting a DownloadDirectory should disable the PromptForDownload pref.
+  const Value* value = NULL;
+  EXPECT_EQ(PrefStore::READ_OK, store_->GetValue(prefs::kPromptForDownload,
+                                                 &value));
+  ASSERT_TRUE(value);
+  bool prompt_for_download = true;
+  bool result = value->GetAsBoolean(&prompt_for_download);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(prompt_for_download);
+}
+
+TEST_F(ConfigurationPolicyPrefStorePromptDownloadTest,
+       EnableFileSelectionDialogs) {
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            store_->GetValue(prefs::kPromptForDownload, NULL));
+  provider_.AddPolicy(kPolicyAllowFileSelectionDialogs,
+                      Value::CreateBooleanValue(true));
+  store_->OnUpdatePolicy();
+
+  // Allowing file-selection dialogs should not influence the PromptForDownload
+  // pref.
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            store_->GetValue(prefs::kPromptForDownload, NULL));
+}
+
+TEST_F(ConfigurationPolicyPrefStorePromptDownloadTest,
+       DisableFileSelectionDialogs) {
+  EXPECT_EQ(PrefStore::READ_NO_VALUE,
+            store_->GetValue(prefs::kPromptForDownload, NULL));
+  provider_.AddPolicy(kPolicyAllowFileSelectionDialogs,
+                      Value::CreateBooleanValue(false));
+  store_->OnUpdatePolicy();
+
+  // Disabling file-selection dialogs should disable the PromptForDownload pref.
+  const Value* value = NULL;
+  EXPECT_EQ(PrefStore::READ_OK, store_->GetValue(prefs::kPromptForDownload,
+                                                 &value));
+  ASSERT_TRUE(value);
+  bool prompt_for_download = true;
+  bool result = value->GetAsBoolean(&prompt_for_download);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(prompt_for_download);
 }
 
 // Test cases for the Autofill policy setting.
@@ -702,7 +774,11 @@ TEST_F(ConfigurationPolicyPrefStoreAutofillTest, Disabled) {
   const Value* value = NULL;
   EXPECT_EQ(PrefStore::READ_OK,
             store_->GetValue(prefs::kAutofillEnabled, &value));
-  EXPECT_TRUE(FundamentalValue(false).Equals(value));
+  ASSERT_TRUE(value);
+  bool autofill_enabled = true;
+  bool result = value->GetAsBoolean(&autofill_enabled);
+  ASSERT_TRUE(result);
+  EXPECT_FALSE(autofill_enabled);
 }
 
 // Exercises the policy refresh mechanism.
diff --git a/chrome/browser/policy/managed_prefs_banner_base.cc b/chrome/browser/policy/managed_prefs_banner_base.cc
index 2c70167..ac105c5 100644
--- a/chrome/browser/policy/managed_prefs_banner_base.cc
+++ b/chrome/browser/policy/managed_prefs_banner_base.cc
@@ -94,6 +94,7 @@ void ManagedPrefsBannerBase::Init(PrefService* local_state,
       AddUserPref(prefs::kProxy);
       AddUserPref(prefs::kCloudPrintProxyEnabled);
       AddUserPref(prefs::kDownloadDefaultDirectory);
+      AddUserPref(prefs::kPromptForDownload);
       AddUserPref(prefs::kEnableTranslate);
       AddUserPref(prefs::kChromotingEnabled);
       AddUserPref(prefs::kChromotingHostEnabled);
diff --git a/chrome/browser/ui/webui/options/advanced_options_handler.cc b/chrome/browser/ui/webui/options/advanced_options_handler.cc
index b0ac033..071ff37 100644
--- a/chrome/browser/ui/webui/options/advanced_options_handler.cc
+++ b/chrome/browser/ui/webui/options/advanced_options_handler.cc
@@ -576,7 +576,11 @@ void AdvancedOptionsHandler::SetupDownloadLocationPath() {
 
 void AdvancedOptionsHandler::SetupPromptForDownload() {
   FundamentalValue checked(ask_for_save_location_.GetValue());
-  FundamentalValue disabled(default_download_location_.IsManaged());
+  // If either the DownloadDirectory is managed or if file-selection dialogs are
+  // disallowed then |ask_for_save_location_| must currently be false and cannot
+  // be changed.
+  FundamentalValue disabled(default_download_location_.IsManaged() ||
+                            !allow_file_selection_dialogs_.GetValue());
   web_ui_->CallJavascriptFunction(
       "options.AdvancedOptions.SetPromptForDownload", checked, disabled);
 }