Migrate OOBE configuration loading to DBus service

Fake implementation still reads configuration provided
via command line.

Bug: 854101
Change-Id: I88cb4466cbc5118218251e14b20c92f2806acf30
Reviewed-on: https://chromium-review.googlesource.com/1183497
Commit-Queue: Denis Kuznetsov <antrim@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584987}

chrome/browser/chromeos/arc/arc_util_unittest.cc

@@ -32,6 +32,8 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_oobe_configuration_client.h"
 #include "components/account_id/account_id.h"
 #include "components/arc/arc_prefs.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
@@ -1009,14 +1011,17 @@ INSTANTIATE_TEST_CASE_P(
 
 class ArcOobeTest : public ChromeArcUtilTest {
  public:
-  ArcOobeTest()
-      : oobe_configuration_(std::make_unique<chromeos::OobeConfiguration>()) {}
+  ArcOobeTest() {
+    chromeos::DBusThreadManager::GetSetterForTesting();
+    oobe_configuration_ = std::make_unique<chromeos::OobeConfiguration>();
+  }
 
   ~ArcOobeTest() override {
     // Fake display host have to be shut down first, as it may access
     // configuration.
     fake_login_display_host_.reset();
     oobe_configuration_.reset();
+    chromeos::DBusThreadManager::Shutdown();
   }
 
  protected:

chrome/browser/chromeos/login/oobe_configuration.cc

@@ -6,35 +6,13 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
-#include "base/task/post_task.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/oobe_configuration_client.h"
 
 namespace chromeos {
 
-namespace {
-
-std::unique_ptr<base::Value> LoadOOBEConfigurationFile(base::FilePath path) {
-  std::string configuration_data;
-  if (!base::ReadFileToString(path, &configuration_data)) {
-    DLOG(WARNING) << "Can't read OOBE Configuration";
-    return std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
-  }
-  int error_code, row, col;
-  std::string error_message;
-  auto value = base::JSONReader::ReadAndReturnError(
-      configuration_data, base::JSONParserOptions::JSON_ALLOW_TRAILING_COMMAS,
-      &error_code, &error_message, &row, &col);
-  if (!value || !value->is_dict()) {
-    LOG(ERROR) << "Error parsing OOBE configuration: " << error_message;
-    return std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
-  }
-  return value;
-}
-
-}  // namespace
-
 // static
 OobeConfiguration* OobeConfiguration::instance = nullptr;
 
@@ -74,23 +52,32 @@ void OobeConfiguration::ResetConfiguration() {
     observer.OnOobeConfigurationChanged();
 }
 
-void OobeConfiguration::OnConfigurationLoaded(
-    std::unique_ptr<base::Value> configuration) {
-  LOG(ERROR) << " DEBUG OUT *****************OnConfigurationLoaded";
-  if (!configuration)
+void OobeConfiguration::CheckConfiguration() {
+  DBusThreadManager::Get()
+      ->GetOobeConfigurationClient()
+      ->CheckForOobeConfiguration(
+          base::BindOnce(&OobeConfiguration::OnConfigurationCheck,
+                         weak_factory_.GetWeakPtr()));
+}
+
+void OobeConfiguration::OnConfigurationCheck(bool has_configuration,
+                                             const std::string& configuration) {
+  if (!has_configuration)
+    return;
+
+  int error_code, row, col;
+  std::string error_message;
+  auto value = base::JSONReader::ReadAndReturnError(
+      configuration, base::JSONParserOptions::JSON_ALLOW_TRAILING_COMMAS,
+      &error_code, &error_message, &row, &col);
+  if (!value || !value->is_dict()) {
+    LOG(ERROR) << "Error parsing OOBE configuration: " << error_message;
     return;
+  }
 
-  configuration_ = std::move(configuration);
+  configuration_ = std::move(value);
   for (auto& observer : observer_list_)
     observer.OnOobeConfigurationChanged();
 }
 
-void OobeConfiguration::LoadConfiguration(const base::FilePath& path) {
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
-      base::BindOnce(&LoadOOBEConfigurationFile, path),
-      base::BindOnce(&OobeConfiguration::OnConfigurationLoaded,
-                     weak_factory_.GetWeakPtr()));
-}
-
 }  // namespace chromeos

chrome/browser/chromeos/login/oobe_configuration.h

@@ -36,14 +36,15 @@ class OobeConfiguration {
   void RemoveObserver(Observer* observer);
 
   void ResetConfiguration();
-  void LoadConfiguration(const base::FilePath& path);
+  void CheckConfiguration();
 
   // Returns current OobeConfiguration instance and NULL if it hasn't been
   // initialized yet.
   static OobeConfiguration* Get();
 
  private:
-  void OnConfigurationLoaded(std::unique_ptr<base::Value> configuration);
+  void OnConfigurationCheck(bool has_configuration,
+                            const std::string& configuration);
 
   // Pointer to the existing OobeConfiguration instance (if any).
   // Set in ctor, reset in dtor. Not owned since we need to control the lifetime

chrome/browser/chromeos/login/session/chrome_session_manager.cc

@@ -222,11 +222,11 @@ void ChromeSessionManager::Initialize(
   if (parsed_command_line.HasSwitch(switches::kLoginManager) &&
       (!is_running_test || force_login_screen_in_test)) {
     VLOG(1) << "Starting Chrome with login/oobe screen.";
-    LoadOobeConfiguration();
+    oobe_configuration_->CheckConfiguration();
     StartLoginOobeSession();
     return;
   } else if (is_running_test) {
-    LoadOobeConfiguration();
+    oobe_configuration_->CheckConfiguration();
   }
 
   if (!base::SysInfo::IsRunningOnChromeOS() &&
@@ -269,21 +269,10 @@ void ChromeSessionManager::SessionStarted() {
   chromeos::WebUIScreenLocker::RequestPreload();
 }
 
-void ChromeSessionManager::LoadOobeConfiguration() {
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kOobeConfiguration))
-    return;
-  VLOG(1) << "Loading OOBE configuration ";
-  oobe_configuration_->LoadConfiguration(
-      base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
-          chromeos::switches::kOobeConfiguration));
-}
-
 void ChromeSessionManager::NotifyUserLoggedIn(const AccountId& user_account_id,
                                               const std::string& user_id_hash,
                                               bool browser_restart,
                                               bool is_child) {
-  oobe_configuration_->ResetConfiguration();
   BootTimesRecorder* btl = BootTimesRecorder::Get();
   btl->AddLoginTimeMarker("UserLoggedIn-Start", false);
   session_manager::SessionManager::NotifyUserLoggedIn(

chrome/browser/chromeos/login/session/chrome_session_manager.h

@@ -42,7 +42,6 @@ class ChromeSessionManager : public session_manager::SessionManager {
                           bool is_child) override;
 
  private:
-  void LoadOobeConfiguration();
   std::unique_ptr<chromeos::OobeConfiguration> oobe_configuration_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeSessionManager);

chrome/browser/chromeos/login/wizard_controller_browsertest.cc

@@ -64,6 +64,7 @@
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/chromeos_test_utils.h"
+#include "chromeos/dbus/dbus_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_session_manager_client.h"
 #include "chromeos/dbus/fake_shill_manager_client.h"
@@ -2510,7 +2511,7 @@ class WizardControllerOobeConfigurationTest : public WizardControllerTest {
     ASSERT_TRUE(chromeos::test_utils::GetTestDataPath(
         "oobe_configuration", "non_empty_configuration.json",
         &configuration_file));
-    command_line->AppendSwitchPath(chromeos::switches::kOobeConfiguration,
+    command_line->AppendSwitchPath(chromeos::switches::kFakeOobeConfiguration,
                                    configuration_file);
   }
 

chromeos/BUILD.gn

@@ -224,6 +224,8 @@ component("chromeos") {
     "dbus/fake_media_analytics_client.h",
     "dbus/fake_modem_messaging_client.cc",
     "dbus/fake_modem_messaging_client.h",
+    "dbus/fake_oobe_configuration_client.cc",
+    "dbus/fake_oobe_configuration_client.h",
     "dbus/fake_permission_broker_client.cc",
     "dbus/fake_permission_broker_client.h",
     "dbus/fake_power_manager_client.cc",
@@ -268,6 +270,8 @@ component("chromeos") {
     "dbus/media_analytics_client.h",
     "dbus/modem_messaging_client.cc",
     "dbus/modem_messaging_client.h",
+    "dbus/oobe_configuration_client.cc",
+    "dbus/oobe_configuration_client.h",
     "dbus/permission_broker_client.cc",
     "dbus/permission_broker_client.h",
     "dbus/pipe_reader.cc",

chromeos/chromeos_switches.cc

@@ -518,9 +518,6 @@ const char kNoteTakingAppIds[] = "note-taking-app-ids";
 // Indicates that if we should start bootstrapping Master OOBE.
 const char kOobeBootstrappingMaster[] = "oobe-bootstrapping-master";
 
-// Path to a OOBE configuration JSON file.
-const char kOobeConfiguration[] = "oobe-configuration-file";
-
 // Forces OOBE/login to force show a comma-separated list of screens from
 // chromeos::kScreenNames in oobe_screen.cc. Supported screens are:
 //   user-image

chromeos/chromeos_switches.h

@@ -145,7 +145,6 @@ CHROMEOS_EXPORT extern const char kNaturalScrollDefault[];
 CHROMEOS_EXPORT extern const char kNeedArcMigrationPolicyCheck[];
 CHROMEOS_EXPORT extern const char kNoteTakingAppIds[];
 CHROMEOS_EXPORT extern const char kOobeBootstrappingMaster[];
-CHROMEOS_EXPORT extern const char kOobeConfiguration[];
 CHROMEOS_EXPORT extern const char kOobeForceShowScreen[];
 CHROMEOS_EXPORT extern const char kOobeGuestSession[];
 CHROMEOS_EXPORT extern const char kOobeSkipPostLogin[];

chromeos/dbus/dbus_clients_browser.cc

@@ -30,12 +30,14 @@
 #include "chromeos/dbus/fake_image_loader_client.h"
 #include "chromeos/dbus/fake_lorgnette_manager_client.h"
 #include "chromeos/dbus/fake_media_analytics_client.h"
+#include "chromeos/dbus/fake_oobe_configuration_client.h"
 #include "chromeos/dbus/fake_smb_provider_client.h"
 #include "chromeos/dbus/fake_virtual_file_provider_client.h"
 #include "chromeos/dbus/image_burner_client.h"
 #include "chromeos/dbus/image_loader_client.h"
 #include "chromeos/dbus/lorgnette_manager_client.h"
 #include "chromeos/dbus/media_analytics_client.h"
+#include "chromeos/dbus/oobe_configuration_client.h"
 #include "chromeos/dbus/smb_provider_client.h"
 #include "chromeos/dbus/virtual_file_provider_client.h"
 
@@ -113,6 +115,11 @@ DBusClientsBrowser::DBusClientsBrowser(bool use_real_clients) {
   else
     media_analytics_client_.reset(new FakeMediaAnalyticsClient);
 
+  if (use_real_clients)
+    oobe_configuration_client_ = OobeConfigurationClient::Create();
+  else
+    oobe_configuration_client_.reset(new FakeOobeConfigurationClient);
+
   if (use_real_clients)
     smb_provider_client_.reset(SmbProviderClient::Create());
   else
@@ -143,6 +150,7 @@ void DBusClientsBrowser::Initialize(dbus::Bus* system_bus) {
   image_loader_client_->Init(system_bus);
   lorgnette_manager_client_->Init(system_bus);
   media_analytics_client_->Init(system_bus);
+  oobe_configuration_client_->Init(system_bus);
   smb_provider_client_->Init(system_bus);
   virtual_file_provider_client_->Init(system_bus);
 }

chromeos/dbus/dbus_clients_browser.h

@@ -30,6 +30,7 @@ class ImageBurnerClient;
 class ImageLoaderClient;
 class LorgnetteManagerClient;
 class MediaAnalyticsClient;
+class OobeConfigurationClient;
 class SmbProviderClient;
 class VirtualFileProviderClient;
 
@@ -62,6 +63,7 @@ class CHROMEOS_EXPORT DBusClientsBrowser {
   std::unique_ptr<ImageLoaderClient> image_loader_client_;
   std::unique_ptr<LorgnetteManagerClient> lorgnette_manager_client_;
   std::unique_ptr<MediaAnalyticsClient> media_analytics_client_;
+  std::unique_ptr<OobeConfigurationClient> oobe_configuration_client_;
   std::unique_ptr<SmbProviderClient> smb_provider_client_;
   std::unique_ptr<VirtualFileProviderClient> virtual_file_provider_client_;
 

chromeos/dbus/dbus_switches.cc

@@ -14,6 +14,9 @@ const char kAttestationServer[] = "attestation-server";
 // Forces the stub implementation of D-Bus clients.
 const char kDbusStub[] = "dbus-stub";
 
+// Path to a OOBE configuration JSON file (used by FakeOobeConfigurationClient).
+const char kFakeOobeConfiguration[] = "fake-oobe-configuration-file";
+
 // Overrides Shill stub behavior. By default, ethernet, wifi and vpn are
 // enabled, and transitions occur instantaneously. Multiple options can be
 // comma separated (no spaces). Note: all options are in the format 'foo=x'.

chromeos/dbus/dbus_switches.h

@@ -12,6 +12,7 @@ namespace switches {
 
 CHROMEOS_EXPORT extern const char kAttestationServer[];
 CHROMEOS_EXPORT extern const char kDbusStub[];
+CHROMEOS_EXPORT extern const char kFakeOobeConfiguration[];
 CHROMEOS_EXPORT extern const char kShillStub[];
 CHROMEOS_EXPORT extern const char kSmsTestMessages[];
 CHROMEOS_EXPORT extern const char kSystemDevMode[];

chromeos/dbus/dbus_thread_manager.cc

@@ -242,6 +242,10 @@ ModemMessagingClient* DBusThreadManager::GetModemMessagingClient() {
   return clients_common_->modem_messaging_client_.get();
 }
 
+OobeConfigurationClient* DBusThreadManager::GetOobeConfigurationClient() {
+  return clients_browser_->oobe_configuration_client_.get();
+}
+
 PermissionBrokerClient* DBusThreadManager::GetPermissionBrokerClient() {
   return clients_common_->permission_broker_client_.get();
 }

chromeos/dbus/dbus_thread_manager.h

@@ -49,6 +49,7 @@ class LorgnetteManagerClient;
 class MachineLearningClient;
 class MediaAnalyticsClient;
 class ModemMessagingClient;
+class OobeConfigurationClient;
 class PermissionBrokerClient;
 class PowerManagerClient;
 class SessionManagerClient;
@@ -154,6 +155,7 @@ class CHROMEOS_EXPORT DBusThreadManager {
   MachineLearningClient* GetMachineLearningClient();
   MediaAnalyticsClient* GetMediaAnalyticsClient();
   ModemMessagingClient* GetModemMessagingClient();
+  OobeConfigurationClient* GetOobeConfigurationClient();
   PermissionBrokerClient* GetPermissionBrokerClient();
   PowerManagerClient* GetPowerManagerClient();
   SessionManagerClient* GetSessionManagerClient();

chromeos/dbus/fake_easy_unlock_client.h

@@ -12,7 +12,7 @@
 
 namespace chromeos {
 
-// A fake implemetation of EasyUnlockClient.
+// A fake implementation of EasyUnlockClient.
 class CHROMEOS_EXPORT FakeEasyUnlockClient : public EasyUnlockClient {
  public:
   // Tests if the provided keys belong to the same (fake) EC P256 key pair

chromeos/dbus/fake_oobe_configuration_client.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/dbus/fake_oobe_configuration_client.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
+#include "base/task/post_task.h"
+#include "chromeos/dbus/dbus_switches.h"
+
+namespace {
+
+std::string LoadConfigurationFile(base::FilePath path) {
+  std::string configuration_data;
+  if (!base::ReadFileToString(path, &configuration_data)) {
+    DLOG(WARNING) << "Can't read OOBE Configuration";
+    return std::string();
+  }
+  return configuration_data;
+}
+
+void OnConfigurationLoaded(
+    chromeos::OobeConfigurationClient::ConfigurationCallback callback,
+    const std::string& configuration) {
+  std::move(callback).Run(!configuration.empty(), configuration);
+}
+
+}  // namespace
+
+namespace chromeos {
+
+FakeOobeConfigurationClient::FakeOobeConfigurationClient() = default;
+
+FakeOobeConfigurationClient::~FakeOobeConfigurationClient() = default;
+
+void FakeOobeConfigurationClient::Init(dbus::Bus* bus) {}
+
+void FakeOobeConfigurationClient::CheckForOobeConfiguration(
+    ConfigurationCallback callback) {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kFakeOobeConfiguration)) {
+    std::move(callback).Run(false, std::string());
+    return;
+  }
+
+  const base::FilePath path =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+          chromeos::switches::kFakeOobeConfiguration);
+
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
+      base::BindOnce(&LoadConfigurationFile, path),
+      base::BindOnce(&OnConfigurationLoaded, std::move(callback)));
+}
+
+}  // namespace chromeos

chromeos/dbus/fake_oobe_configuration_client.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_DBUS_FAKE_OOBE_CONFIGURATION_CLIENT_H_
+#define CHROMEOS_DBUS_FAKE_OOBE_CONFIGURATION_CLIENT_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "chromeos/dbus/oobe_configuration_client.h"
+
+namespace chromeos {
+
+// A fake implementation of OobeConfigurationClient, provides configuration
+// specified via command-line flag.
+class CHROMEOS_EXPORT FakeOobeConfigurationClient
+    : public OobeConfigurationClient {
+ public:
+  FakeOobeConfigurationClient();
+  ~FakeOobeConfigurationClient() override;
+
+  void Init(dbus::Bus* bus) override;
+
+  // EasyUnlockClient overrides
+  void CheckForOobeConfiguration(ConfigurationCallback callback) override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakeOobeConfigurationClient);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_FAKE_OOBE_CONFIGURATION_CLIENT_H_

chromeos/dbus/oobe_configuration_client.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/dbus/oobe_configuration_client.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/compiler_specific.h"
+#include "base/macros.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "dbus/object_proxy.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+// The OobeConfigurationClient used in production.
+
+class OobeConfigurationClientImpl : public OobeConfigurationClient {
+ public:
+  OobeConfigurationClientImpl() : weak_ptr_factory_(this) {}
+
+  ~OobeConfigurationClientImpl() override = default;
+
+  // OobeConfigurationClient override:
+  void CheckForOobeConfiguration(ConfigurationCallback callback) override {
+    // TODO (antrim): do a method call once https://crbug.com/869209 is fixed.
+    OnData(std::move(callback), nullptr);
+  }
+
+ protected:
+  void Init(dbus::Bus* bus) override {
+    // TODO (antrim): get proxy object once https://crbug.com/869209 is fixed.
+  }
+
+ private:
+  void OnData(ConfigurationCallback callback, dbus::Response* response) {
+    if (!response) {
+      std::move(callback).Run(false, std::string());
+      return;
+    }
+
+    // TODO (antrim): read response once https://crbug.com/869209 is fixed.
+    NOTREACHED();
+  }
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<OobeConfigurationClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(OobeConfigurationClientImpl);
+};
+
+// static
+std::unique_ptr<OobeConfigurationClient> OobeConfigurationClient::Create() {
+  return std::make_unique<OobeConfigurationClientImpl>();
+}
+
+}  // namespace chromeos

chromeos/dbus/oobe_configuration_client.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_DBUS_OOBE_CONFIGURATION_CLIENT_H_
+#define CHROMEOS_DBUS_OOBE_CONFIGURATION_CLIENT_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_client.h"
+
+namespace chromeos {
+
+// Client for calling OobeConfiguration dbus service. The service provides
+// verified OOBE configuration, that allows to automate out-of-box experience.
+// This configuration comes either from the state before power wash, or from
+// USB stick during USB-based enrollment flow.
+
+class CHROMEOS_EXPORT OobeConfigurationClient : public DBusClient {
+ public:
+  using ConfigurationCallback =
+      base::OnceCallback<void(bool has_configuration,
+                              const std::string& configuration)>;
+
+  ~OobeConfigurationClient() override = default;
+
+  // Factory function.
+  static std::unique_ptr<OobeConfigurationClient> Create();
+
+  // Checks if valid OOBE configuration exists.
+  virtual void CheckForOobeConfiguration(ConfigurationCallback callback) = 0;
+
+ protected:
+  // Create() should be used instead.
+  OobeConfigurationClient() = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(OobeConfigurationClient);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_OOBE_CONFIGURATION_CLIENT_H_