Revert of Enable Enterprise enrollment on desktop builds. (https://co…

…dereview.chromium.org/258743005/) Reason for revert: It seems to have broken the following test from base_unittests and unit_tests on chromium.webkit Android Tests (dbg) builder: PathServiceTest.Override Log: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/18922 Snippet from log: C 237s Main [FAIL] PathServiceTest.Override: C 237s Main ../../base/path_service_unittest.cc:184: Failure C 237s Main Value of: PathService::OverrideAndCreateIfNeeded(my_special_key, non_existent, false, false) C 237s Main Actual: true C 237s Main Expected: false Original issue's description: > Enable Enterprise enrollment on desktop builds. > > This change implements some of the DBus stub methods so that enterprise enrollment works on desktop builds. That will make development of features that depend on enrollment faster for developers that use this workflow (e.g. for kiosk enterprise apps, public accounts, testing some device policies, etc). > > - Override some of the directories and files involved with the enrollment state > - Simple stub implementation of the DBus calls involved > - Write a persistent cache of the install attributes > - Cleaned up the stub for user cloud policy and made them persistent too > - Updated some tests > > This change doesn't affect production code. > > TBR=jochen@chromium.org > BUG=240269, 367674 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267640 TBR=brettw@chromium.org,nkostylev@chromium.org,pastarmovj@chromium.org,satorux@chromium.org,jochen@chromium.org,joaodasilva@chromium.org NOTREECHECKS=true NOTRY=true BUG=240269, 367674 Review URL: https://codereview.chromium.org/265013002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267761 0039d316-1c4b-4281-b951-d872f2087c98
FairyWorld · May 2, 2014 · f87c00c · f87c00c
1 parent 1ae2e64
commit f87c00c
Show file tree

Hide file tree

Showing 27 changed files with 164 additions and 445 deletions.
diff --git a/base/path_service.cc b/base/path_service.cc
@@ -233,15 +233,13 @@ bool PathService::Get(int key, FilePath* result) {
 
 // static
 bool PathService::Override(int key, const FilePath& path) {
-  // Just call the full function with true for the value of |create|, and
-  // assume that |path| may not be absolute yet.
-  return OverrideAndCreateIfNeeded(key, path, false, true);
+  // Just call the full function with true for the value of |create|.
+  return OverrideAndCreateIfNeeded(key, path, true);
 }
 
 // static
 bool PathService::OverrideAndCreateIfNeeded(int key,
                                             const FilePath& path,
-                                            bool is_absolute,
                                             bool create) {
   PathData* path_data = GetPathData();
   DCHECK(path_data);
@@ -261,12 +259,9 @@ bool PathService::OverrideAndCreateIfNeeded(int key,
   }
 
   // We need to have an absolute path.
-  if (!is_absolute) {
-    file_path = MakeAbsoluteFilePath(file_path);
-    if (file_path.empty())
-      return false;
-  }
-  DCHECK(file_path.IsAbsolute());
+  file_path = MakeAbsoluteFilePath(file_path);
+  if (file_path.empty())
+    return false;
 
   base::AutoLock scoped_lock(path_data->lock);
 

diff --git a/base/path_service.h b/base/path_service.h
@@ -46,18 +46,11 @@ class BASE_EXPORT PathService {
   // one test should not carry over to another.
   static bool Override(int key, const base::FilePath& path);
 
-  // This function does the same as PathService::Override but it takes extra
-  // parameters:
-  // - |is_absolute| indicates that |path| has already been expanded into an
-  // absolute path, otherwise MakeAbsoluteFilePath() will be used. This is
-  // useful to override paths that may not exist yet, since MakeAbsoluteFilePath
-  // fails for those. Note that MakeAbsoluteFilePath also expands symbolic
-  // links, even if path.IsAbsolute() is already true.
-  // - |create| guides whether the directory to be overriden must
+  // This function does the same as PathService::Override but it takes an extra
+  // parameter |create| which guides whether the directory to be overriden must
   // be created in case it doesn't exist already.
   static bool OverrideAndCreateIfNeeded(int key,
                                         const base::FilePath& path,
-                                        bool is_absolute,
                                         bool create);
 
   // To extend the set of supported keys, you can register a path provider,

diff --git a/base/path_service_unittest.cc b/base/path_service_unittest.cc
@@ -147,7 +147,7 @@ TEST_F(PathServiceTest, Get) {
 #endif
 }
 
-// Test that all versions of the Override function of PathService do what they
+// test that all versions of the Override function of PathService do what they
 // are supposed to do.
 TEST_F(PathServiceTest, Override) {
   int my_special_key = 666;
@@ -163,37 +163,12 @@ TEST_F(PathServiceTest, Override) {
   // PathService::OverrideAndCreateIfNeeded should obey the |create| parameter.
   PathService::OverrideAndCreateIfNeeded(my_special_key,
                                          fake_cache_dir2,
-                                         false,
                                          false);
   EXPECT_FALSE(base::PathExists(fake_cache_dir2));
   EXPECT_TRUE(PathService::OverrideAndCreateIfNeeded(my_special_key,
                                                      fake_cache_dir2,
-                                                     false,
                                                      true));
   EXPECT_TRUE(base::PathExists(fake_cache_dir2));
-
-#if defined(OS_POSIX)
-  base::FilePath non_existent(
-      base::MakeAbsoluteFilePath(temp_dir.path()).AppendASCII("non_existent"));
-  EXPECT_TRUE(non_existent.IsAbsolute());
-  EXPECT_FALSE(base::PathExists(non_existent));
-  // This fails because MakeAbsoluteFilePath fails for non-existent files.
-  EXPECT_FALSE(PathService::OverrideAndCreateIfNeeded(my_special_key,
-                                                      non_existent,
-                                                      false,
-                                                      false));
-  // This works because indicating that |non_existent| is absolute skips the
-  // internal MakeAbsoluteFilePath call.
-  EXPECT_TRUE(PathService::OverrideAndCreateIfNeeded(my_special_key,
-                                                     non_existent,
-                                                     true,
-                                                     false));
-  // Check that the path has been overridden and no directory was created.
-  EXPECT_FALSE(base::PathExists(non_existent));
-  base::FilePath path;
-  EXPECT_TRUE(PathService::Get(my_special_key, &path));
-  EXPECT_EQ(non_existent, path);
-#endif
 }
 
 // Check if multiple overrides can co-exist.

diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
@@ -350,7 +350,7 @@ void InitializeUserDataDir() {
 
   const bool specified_directory_was_invalid = !user_data_dir.empty() &&
       !PathService::OverrideAndCreateIfNeeded(chrome::DIR_USER_DATA,
-          user_data_dir, false, true);
+          user_data_dir, true);
   // Save inaccessible or invalid paths so the user may be prompted later.
   if (specified_directory_was_invalid)
     chrome::SetInvalidSpecifiedUserDataDir(user_data_dir);

diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -247,6 +247,16 @@ namespace internal {
 class DBusServices {
  public:
   explicit DBusServices(const content::MainFunctionParams& parameters) {
+    if (!base::SysInfo::IsRunningOnChromeOS()) {
+      // Override this path on the desktop, so that the user policy key can be
+      // stored by the stub SessionManagerClient.
+      base::FilePath user_data_dir;
+      if (PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
+        PathService::Override(chromeos::DIR_USER_POLICY_KEYS,
+                              user_data_dir.AppendASCII("stub_user_policy"));
+      }
+    }
+
     // Initialize DBusThreadManager for the browser. This must be done after
     // the main message loop is started, as it uses the message loop.
     DBusThreadManager::Initialize();
@@ -394,15 +404,6 @@ void ChromeBrowserMainPartsChromeos::PreMainMessageLoopStart() {
 }
 
 void ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() {
-  base::FilePath user_data_dir;
-  if (!base::SysInfo::IsRunningOnChromeOS() &&
-      PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
-    // Override some paths with stub locations so that cloud policy and
-    // enterprise enrollment work on desktop builds, for ease of
-    // development.
-    chromeos::RegisterStubPathOverrides(user_data_dir);
-  }
-
   dbus_services_.reset(new internal::DBusServices(parameters()));
 
   ChromeBrowserMainPartsLinux::PostMainMessageLoopStart();

diff --git a/chrome/browser/chromeos/login/kiosk_browsertest.cc b/chrome/browser/chromeos/login/kiosk_browsertest.cc
@@ -849,6 +849,15 @@ IN_PROC_BROWSER_TEST_F(KioskTest, KioskEnableAbortedWithAutoEnrollment) {
 }
 
 IN_PROC_BROWSER_TEST_F(KioskTest, KioskEnableAfter2ndSigninScreen) {
+  // Fake an auto enrollment is not going to be enforced.
+  CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kEnterpriseEnrollmentInitialModulus, "1");
+  CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kEnterpriseEnrollmentModulusLimit, "2");
+  g_browser_process->local_state()->SetBoolean(prefs::kShouldAutoEnroll, false);
+  g_browser_process->local_state()->SetInteger(
+      prefs::kAutoEnrollmentPowerLimit, -1);
+
   chromeos::WizardController::SkipPostLoginScreensForTesting();
   chromeos::WizardController* wizard_controller =
       chromeos::WizardController::default_controller();
@@ -1114,13 +1123,9 @@ class KioskEnterpriseTest : public KioskTest {
         device_policy_test_helper_.device_policy()->policy_data();
     policy_data.set_service_account_identity(kTestEnterpriseServiceAccountId);
     device_policy_test_helper_.device_policy()->Build();
-
-    base::RunLoop run_loop;
     DBusThreadManager::Get()->GetSessionManagerClient()->StoreDevicePolicy(
         device_policy_test_helper_.device_policy()->GetBlob(),
-        base::Bind(&KioskEnterpriseTest::StorePolicyCallback,
-                   run_loop.QuitClosure()));
-    run_loop.Run();
+        base::Bind(&KioskEnterpriseTest::StorePolicyCallback));
 
     DeviceSettingsService::Get()->Load();
 
@@ -1159,9 +1164,8 @@ class KioskEnterpriseTest : public KioskTest {
     base::RunLoop().RunUntilIdle();
   }
 
-  static void StorePolicyCallback(const base::Closure& callback, bool result) {
+  static void StorePolicyCallback(bool result) {
     ASSERT_TRUE(result);
-    callback.Run();
   }
 
   policy::DevicePolicyCrosTestHelper device_policy_test_helper_;

diff --git a/chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc b/chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc
@@ -14,7 +14,6 @@
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/browser/chromeos/policy/enterprise_install_attributes.h"
 #include "chrome/browser/chromeos/policy/proto/install_attributes.pb.h"
-#include "chrome/common/chrome_paths.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/fake_dbus_thread_manager.h"
 #include "chromeos/dbus/fake_session_manager_client.h"
@@ -28,13 +27,13 @@ using ::testing::Return;
 
 namespace policy {
 
-DevicePolicyCrosTestHelper::DevicePolicyCrosTestHelper() {}
+DevicePolicyCrosTestHelper::DevicePolicyCrosTestHelper() {
+  CHECK(temp_dir_.CreateUniqueTempDir());
+}
 
 DevicePolicyCrosTestHelper::~DevicePolicyCrosTestHelper() {}
 
 void DevicePolicyCrosTestHelper::MarkAsEnterpriseOwned() {
-  OverridePaths();
-
   cryptohome::SerializedInstallAttributes install_attrs_proto;
   cryptohome::SerializedInstallAttributes::Attribute* attribute = NULL;
 
@@ -46,22 +45,20 @@ void DevicePolicyCrosTestHelper::MarkAsEnterpriseOwned() {
   attribute->set_name(EnterpriseInstallAttributes::kAttrEnterpriseUser);
   attribute->set_value(device_policy_.policy_data().username());
 
-  base::FilePath install_attrs_file;
-  ASSERT_TRUE(
-      PathService::Get(chromeos::FILE_INSTALL_ATTRIBUTES, &install_attrs_file));
+  base::FilePath install_attrs_file =
+      temp_dir_.path().AppendASCII("install_attributes.pb");
   const std::string install_attrs_blob(
       install_attrs_proto.SerializeAsString());
   ASSERT_EQ(static_cast<int>(install_attrs_blob.size()),
             base::WriteFile(install_attrs_file,
                             install_attrs_blob.c_str(),
                             install_attrs_blob.size()));
+  ASSERT_TRUE(PathService::Override(chromeos::FILE_INSTALL_ATTRIBUTES,
+                                    install_attrs_file));
 }
 
 void DevicePolicyCrosTestHelper::InstallOwnerKey() {
-  OverridePaths();
-
-  base::FilePath owner_key_file;
-  ASSERT_TRUE(PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_file));
+  base::FilePath owner_key_file = temp_dir_.path().AppendASCII("owner.key");
   std::vector<uint8> owner_key_bits;
   ASSERT_TRUE(
       device_policy()->GetSigningKey()->ExportPublicKey(&owner_key_bits));
@@ -70,15 +67,7 @@ void DevicePolicyCrosTestHelper::InstallOwnerKey() {
           reinterpret_cast<const char*>(vector_as_array(&owner_key_bits)),
           owner_key_bits.size()),
       static_cast<int>(owner_key_bits.size()));
-}
-
-void DevicePolicyCrosTestHelper::OverridePaths() {
-  // This is usually done by ChromeBrowserMainChromeOS, but some tests
-  // use the overridden paths before ChromeBrowserMain starts. Make sure that
-  // the paths are overridden before using them.
-  base::FilePath user_data_dir;
-  ASSERT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
-  chromeos::RegisterStubPathOverrides(user_data_dir);
+  ASSERT_TRUE(PathService::Override(chromeos::FILE_OWNER_KEY, owner_key_file));
 }
 
 DevicePolicyCrosBrowserTest::DevicePolicyCrosBrowserTest()

diff --git a/chrome/browser/chromeos/policy/device_policy_cros_browser_test.h b/chrome/browser/chromeos/policy/device_policy_cros_browser_test.h
@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/files/scoped_temp_dir.h"
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/dbus/fake_dbus_thread_manager.h"
@@ -33,7 +34,8 @@ class DevicePolicyCrosTestHelper {
   DevicePolicyBuilder* device_policy() { return &device_policy_; }
 
  private:
-  void OverridePaths();
+  // Stores the device owner key and the install attributes.
+  base::ScopedTempDir temp_dir_;
 
   // Carries Chrome OS device policies for tests.
   DevicePolicyBuilder device_policy_;