Refactored the DeviceManagementService to get its parameters from a delegate.

The DeviceManagementService will be moved along with the policy/ code into a new
component, and thus it can't access //chrome nor //chromeos code. A recent
refactoring in that direction introduced a regression because it called
StatisticsProvider::GetMachineStatistic() before the StatisticsProvider was
initialized.

This used to work before because GetMachineStatistic() was called only once the
first request was sent to the server (which, on enrolled devices, happens in
a task that is immediately posted but the StatisticsProvider is already
initialized by then).

This change introduces a delegate that the DeviceManagementService uses to
get its configuration parameters, so that the call to GetMachineStatistic()
can again be performed only when a request is sent.

BUG=271392,302798

Review URL: https://codereview.chromium.org/25690003

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

chrome/browser/policy/browser_policy_connector.cc

@@ -122,56 +122,67 @@ base::FilePath GetManagedPolicyPath() {
 }
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
-std::string GetDeviceManagementUrl() {
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDeviceManagementUrl))
-    return command_line->GetSwitchValueASCII(switches::kDeviceManagementUrl);
-  else
-    return kDefaultDeviceManagementServerUrl;
-}
+class DeviceManagementServiceConfiguration
+    : public DeviceManagementService::Configuration {
+ public:
+  DeviceManagementServiceConfiguration() {}
+  virtual ~DeviceManagementServiceConfiguration() {}
+
+  virtual std::string GetServerUrl() OVERRIDE {
+    CommandLine* command_line = CommandLine::ForCurrentProcess();
+    if (command_line->HasSwitch(switches::kDeviceManagementUrl))
+      return command_line->GetSwitchValueASCII(switches::kDeviceManagementUrl);
+    else
+      return kDefaultDeviceManagementServerUrl;
+  }
 
-std::string GetUserAgentParameter() {
-  chrome::VersionInfo version_info;
-  return base::StringPrintf("%s %s(%s)",
-                            version_info.Name().c_str(),
-                            version_info.Version().c_str(),
-                            version_info.LastChange().c_str());
-}
+  virtual std::string GetUserAgent() OVERRIDE {
+    return content::GetUserAgent(GURL(GetServerUrl()));
+  }
 
-std::string GetPlatformParameter() {
-  std::string os_name = base::SysInfo::OperatingSystemName();
-  std::string os_hardware = base::SysInfo::OperatingSystemArchitecture();
+  virtual std::string GetAgentParameter() OVERRIDE {
+    chrome::VersionInfo version_info;
+    return base::StringPrintf("%s %s(%s)",
+                              version_info.Name().c_str(),
+                              version_info.Version().c_str(),
+                              version_info.LastChange().c_str());
+  }
 
-#if defined(OS_CHROMEOS)
-  chromeos::system::StatisticsProvider* provider =
-      chromeos::system::StatisticsProvider::GetInstance();
+  virtual std::string GetPlatformParameter() OVERRIDE {
+    std::string os_name = base::SysInfo::OperatingSystemName();
+    std::string os_hardware = base::SysInfo::OperatingSystemArchitecture();
 
-  std::string hwclass;
-  if (!provider->GetMachineStatistic(chromeos::system::kHardwareClass,
-                                     &hwclass)) {
-    LOG(ERROR) << "Failed to get machine information";
-  }
-  os_name += ",CrOS," + base::SysInfo::GetLsbReleaseBoard();
-  os_hardware += "," + hwclass;
+#if defined(OS_CHROMEOS)
+    chromeos::system::StatisticsProvider* provider =
+        chromeos::system::StatisticsProvider::GetInstance();
+
+    std::string hwclass;
+    if (!provider->GetMachineStatistic(chromeos::system::kHardwareClass,
+                                       &hwclass)) {
+      LOG(ERROR) << "Failed to get machine information";
+    }
+    os_name += ",CrOS," + base::SysInfo::GetLsbReleaseBoard();
+    os_hardware += "," + hwclass;
 #endif
 
-  std::string os_version("-");
+    std::string os_version("-");
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
-  int32 os_major_version = 0;
-  int32 os_minor_version = 0;
-  int32 os_bugfix_version = 0;
-  base::SysInfo::OperatingSystemVersionNumbers(&os_major_version,
-                                               &os_minor_version,
-                                               &os_bugfix_version);
-  os_version = base::StringPrintf("%d.%d.%d",
-                                  os_major_version,
-                                  os_minor_version,
-                                  os_bugfix_version);
+    int32 os_major_version = 0;
+    int32 os_minor_version = 0;
+    int32 os_bugfix_version = 0;
+    base::SysInfo::OperatingSystemVersionNumbers(&os_major_version,
+                                                 &os_minor_version,
+                                                 &os_bugfix_version);
+    os_version = base::StringPrintf("%d.%d.%d",
+                                    os_major_version,
+                                    os_minor_version,
+                                    os_bugfix_version);
 #endif
 
-  return base::StringPrintf(
-      "%s|%s|%s", os_name.c_str(), os_hardware.c_str(), os_version.c_str());
-}
+    return base::StringPrintf(
+        "%s|%s|%s", os_name.c_str(), os_hardware.c_str(), os_version.c_str());
+  }
+};
 
 }  // namespace
 
@@ -236,13 +247,10 @@ void BrowserPolicyConnector::Init(
   local_state_ = local_state;
   request_context_ = request_context;
 
-  std::string server_url = GetDeviceManagementUrl();
+  scoped_ptr<DeviceManagementService::Configuration> configuration(
+      new DeviceManagementServiceConfiguration);
   device_management_service_.reset(
-      new DeviceManagementService(request_context,
-                                  server_url,
-                                  content::GetUserAgent(GURL(server_url)),
-                                  GetUserAgentParameter(),
-                                  GetPlatformParameter()));
+      new DeviceManagementService(configuration.Pass(), request_context));
   device_management_service_->ScheduleInitialization(
       kServiceInitializationStartupDelay);
 

chrome/browser/policy/cloud/device_management_service.cc

@@ -485,7 +485,10 @@ DeviceManagementService::~DeviceManagementService() {
 DeviceManagementRequestJob* DeviceManagementService::CreateJob(
     DeviceManagementRequestJob::JobType type) {
   return new DeviceManagementRequestJobImpl(
-      type, agent_parameter_, platform_parameter_, this);
+      type,
+      configuration_->GetAgentParameter(),
+      configuration_->GetPlatformParameter(),
+      this);
 }
 
 void DeviceManagementService::ScheduleInitialization(int64 delay_milliseconds) {
@@ -502,8 +505,8 @@ void DeviceManagementService::Initialize() {
   if (initialized_)
     return;
   DCHECK(!request_context_getter_.get());
-  request_context_getter_ =
-      new DeviceManagementRequestContextGetter(request_context_, user_agent_);
+  request_context_getter_ = new DeviceManagementRequestContextGetter(
+      request_context_, configuration_->GetUserAgent());
   initialized_ = true;
 
   while (!queued_jobs_.empty()) {
@@ -523,23 +526,19 @@ void DeviceManagementService::Shutdown() {
 }
 
 DeviceManagementService::DeviceManagementService(
-    scoped_refptr<net::URLRequestContextGetter> request_context,
-    const std::string& server_url,
-    const std::string& user_agent,
-    const std::string& agent_parameter,
-    const std::string& platform_parameter)
-    : request_context_(request_context),
-      server_url_(server_url),
-      user_agent_(user_agent),
-      agent_parameter_(agent_parameter),
-      platform_parameter_(platform_parameter),
+    scoped_ptr<Configuration> configuration,
+    scoped_refptr<net::URLRequestContextGetter> request_context)
+    : configuration_(configuration.Pass()),
+      request_context_(request_context),
       initialized_(false),
       weak_ptr_factory_(this) {
+  DCHECK(configuration_);
 }
 
 void DeviceManagementService::StartJob(DeviceManagementRequestJobImpl* job) {
+  std::string server_url = configuration_->GetServerUrl();
   net::URLFetcher* fetcher = net::URLFetcher::Create(
-      kURLFetcherID, job->GetURL(server_url_), net::URLFetcher::POST, this);
+      kURLFetcherID, job->GetURL(server_url), net::URLFetcher::POST, this);
   fetcher->SetRequestContext(request_context_getter_.get());
   job->ConfigureRequest(fetcher);
   pending_jobs_[fetcher] = job;

chrome/browser/policy/cloud/device_management_service.h

@@ -101,12 +101,30 @@ class DeviceManagementRequestJob {
 // requests.
 class DeviceManagementService : public net::URLFetcherDelegate {
  public:
+  // Obtains the parameters used to contact the server.
+  // This allows creating the DeviceManagementService early and getting these
+  // parameters later. Passing the parameters directly in the ctor isn't
+  // possible because some aren't ready during startup. http://crbug.com/302798
+  class Configuration {
+   public:
+    virtual ~Configuration() {}
+
+    // Server at which to contact the service.
+    virtual std::string GetServerUrl() = 0;
+
+    // Value for the User-Agent header.
+    virtual std::string GetUserAgent() = 0;
+
+    // Agent reported in the "agent" query parameter.
+    virtual std::string GetAgentParameter() = 0;
+
+    // The platform reported in the "platform" query parameter.
+    virtual std::string GetPlatformParameter() = 0;
+  };
+
   DeviceManagementService(
-      scoped_refptr<net::URLRequestContextGetter> request_context,
-      const std::string& server_url,
-      const std::string& user_agent,
-      const std::string& agent_parameter,
-      const std::string& platform_parameter);
+      scoped_ptr<Configuration> configuration,
+      scoped_refptr<net::URLRequestContextGetter> request_context);
   virtual ~DeviceManagementService();
 
   // The ID of URLFetchers created by the DeviceManagementService. This can be
@@ -151,21 +169,13 @@ class DeviceManagementService : public net::URLFetcherDelegate {
   // callback.
   void RemoveJob(DeviceManagementRequestJobImpl* job);
 
+  // A Configuration implementation that is used to obtain various parameters
+  // used to talk to the device management server.
+  scoped_ptr<Configuration> configuration_;
+
   // The request context is wrapped by the |request_context_getter_|.
   scoped_refptr<net::URLRequestContextGetter> request_context_;
 
-  // Server at which to contact the service.
-  const std::string server_url_;
-
-  // Value for the User-Agent header.
-  const std::string user_agent_;
-
-  // Agent reported in the "agent" query parameter.
-  const std::string agent_parameter_;
-
-  // The platform reported in the "platform" query parameter.
-  const std::string platform_parameter_;
-
   // The request context we use. This is a wrapper of |request_context_|.
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 

chrome/browser/policy/cloud/device_management_service_browsertest.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
 #include "chrome/browser/policy/cloud/device_management_service.h"
+#include "chrome/browser/policy/cloud/mock_device_management_service.h"
 #include "chrome/browser/policy/cloud/test_request_interceptor.h"
 #include "chrome/browser/policy/test/local_policy_test_server.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -140,11 +141,9 @@ class DeviceManagementServiceIntegrationTest
   virtual void SetUpOnMainThread() OVERRIDE {
     std::string service_url((this->*(GetParam()))());
     service_.reset(new DeviceManagementService(
-        g_browser_process->system_request_context(),
-        service_url,
-        "UserAgent",
-        "UserAgentParam",
-        "PlatformParam"));
+        scoped_ptr<DeviceManagementService::Configuration>(
+            new MockDeviceManagementServiceConfiguration(service_url)),
+        g_browser_process->system_request_context()));
     service_->ScheduleInitialization(0);
   }
 

chrome/browser/policy/cloud/device_management_service_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/string_split.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
 #include "chrome/browser/policy/cloud/device_management_service.h"
+#include "chrome/browser/policy/cloud/mock_device_management_service.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
@@ -30,8 +31,6 @@ namespace em = enterprise_management;
 namespace policy {
 
 const char kServiceUrl[] = "https://example.com/management_service";
-const char kUserAgent[] = "Chrome 1.2.3(456)";
-const char kPlatform[] = "Test|Unit|1.2.3";
 
 // Encoded empty response messages for testing the error code paths.
 const char kResponseEmpty[] = "\x08\x00";
@@ -63,12 +62,10 @@ class DeviceManagementServiceTestBase : public testing::Test {
   }
 
   void ResetService() {
-    service_.reset(new DeviceManagementService(
-        request_context_,
-        kServiceUrl,
-        kUserAgent,
-        kUserAgent,
-        kPlatform));
+    scoped_ptr<DeviceManagementService::Configuration> configuration(
+        new MockDeviceManagementServiceConfiguration(kServiceUrl));
+    service_.reset(
+        new DeviceManagementService(configuration.Pass(), request_context_));
   }
 
   void InitializeService() {

chrome/browser/policy/cloud/mock_device_management_service.cc

@@ -16,6 +16,10 @@ namespace em = enterprise_management;
 namespace policy {
 namespace {
 
+const char kServerUrl[] = "https://example.com/management_service";
+const char kUserAgent[] = "Chrome 1.2.3(456)";
+const char kPlatform[] = "Test|Unit|1.2.3";
+
 // Common mock request job functionality.
 class MockRequestJobBase : public DeviceManagementRequestJob {
  public:
@@ -118,14 +122,38 @@ ACTION_P2(CreateAsyncMockDeviceManagementJob, service, mock_job) {
 
 MockDeviceManagementJob::~MockDeviceManagementJob() {}
 
+MockDeviceManagementServiceConfiguration::
+    MockDeviceManagementServiceConfiguration()
+    : server_url_(kServerUrl) {}
+
+MockDeviceManagementServiceConfiguration::
+    MockDeviceManagementServiceConfiguration(const std::string& server_url)
+    : server_url_(server_url) {}
+
+MockDeviceManagementServiceConfiguration::
+    ~MockDeviceManagementServiceConfiguration() {}
+
+std::string MockDeviceManagementServiceConfiguration::GetServerUrl() {
+  return server_url_;
+}
+
+std::string MockDeviceManagementServiceConfiguration::GetUserAgent() {
+  return kUserAgent;
+}
+
+std::string MockDeviceManagementServiceConfiguration::GetAgentParameter() {
+  return kUserAgent;
+}
+
+std::string MockDeviceManagementServiceConfiguration::GetPlatformParameter() {
+  return kPlatform;
+}
+
 MockDeviceManagementService::MockDeviceManagementService()
-    : DeviceManagementService(
-          new net::TestURLRequestContextGetter(
-              base::MessageLoopProxy::current()),
-          std::string(),
-          std::string(),
-          std::string(),
-          std::string()) {}
+    : DeviceManagementService(scoped_ptr<Configuration>(
+                                  new MockDeviceManagementServiceConfiguration),
+                              new net::TestURLRequestContextGetter(
+                                  base::MessageLoopProxy::current())) {}
 
 MockDeviceManagementService::~MockDeviceManagementService() {}
 

chrome/browser/policy/cloud/mock_device_management_service.h

@@ -23,6 +23,25 @@ class MockDeviceManagementJob {
       const enterprise_management::DeviceManagementResponse& response) = 0;
 };
 
+class MockDeviceManagementServiceConfiguration
+    : public DeviceManagementService::Configuration {
+ public:
+  MockDeviceManagementServiceConfiguration();
+  explicit MockDeviceManagementServiceConfiguration(
+      const std::string& server_url);
+  virtual ~MockDeviceManagementServiceConfiguration();
+
+  virtual std::string GetServerUrl() OVERRIDE;
+  virtual std::string GetUserAgent() OVERRIDE;
+  virtual std::string GetAgentParameter() OVERRIDE;
+  virtual std::string GetPlatformParameter() OVERRIDE;
+
+ private:
+  const std::string server_url_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockDeviceManagementServiceConfiguration);
+};
+
 class MockDeviceManagementService : public DeviceManagementService {
  public:
   MockDeviceManagementService();

chrome/chrome_tests_unit.gypi

@@ -154,6 +154,8 @@
         'browser/policy/cloud/mock_cloud_policy_client.h',
         'browser/policy/cloud/mock_cloud_policy_store.cc',
         'browser/policy/cloud/mock_cloud_policy_store.h',
+        'browser/policy/cloud/mock_device_management_service.cc',
+        'browser/policy/cloud/mock_device_management_service.h',
         'browser/policy/cloud/policy_builder.cc',
         'browser/policy/cloud/policy_builder.h',
         'browser/policy/mock_configuration_policy_provider.cc',
@@ -1064,8 +1066,6 @@
         'browser/policy/cloud/device_management_service_unittest.cc',
         'browser/policy/cloud/external_policy_data_fetcher_unittest.cc',
         'browser/policy/cloud/external_policy_data_updater_unittest.cc',
-        'browser/policy/cloud/mock_device_management_service.cc',
-        'browser/policy/cloud/mock_device_management_service.h',
         'browser/policy/cloud/mock_user_cloud_policy_store.cc',
         'browser/policy/cloud/mock_user_cloud_policy_store.h',
         'browser/policy/cloud/rate_limiter_unittest.cc',