Refactor: Inject StatisticsProvider as a dependency of DeviceCloudPolicyInitializer.

chromeos/system/statistics_provider.cc

 // Copyright 2013 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/system/statistics_provider.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 63 matching lines @@
 
 // The location of OEM manifest file used to trigger OOBE flow for kiosk mode.
 const base::CommandLine::CharType kOemManifestFilePath[] =
     FILE_PATH_LITERAL("/usr/share/oem/oobe/manifest.json");
 
 // Items in region dictionary.
 const char kKeyboardsPath[] = "keyboards";
 const char kLocalesPath[] = "locales";
 const char kTimeZonesPath[] = "time_zones";
 
+// These are the machine serial number keys that we check in order until we
+// find a non-empty serial number. The VPD spec says the serial number should be
+// in the "serial_number" key for v2+ VPDs. However, legacy devices used a
+// different key to report their serial number, which we fall back to if
+// "serial_number" is not present.
+//
+// Product_S/N is still special-cased due to inconsistencies with serial
+// numbers on Lumpy devices: On these devices, serial_number is identical to
+// Product_S/N with an appended checksum. Unfortunately, the sticker on the
+// packaging doesn't include that checksum either (the sticker on the device
+// does though!). The former sticker is the source of the serial number used by
+// device management service, so we prefer Product_S/N over serial number to
+// match the server.
+const char* const kMachineInfoSerialNumberKeys[] = {
+    "Product_S/N",     // Lumpy/Alex devices

[Mattias Nissler (ping if slow), 2016/09/30 12:01:55]

Note that the Product_S/N preference is a kludge that's purely specific to
device enrollment - I don't think exposing a GetMachineID() in
StatisticsProvider that follows this behavior is a good idea, since this is
likely wrong for all other consumers.

[Thiemo Nagel, 2016/09/30 13:03:00]

Actually I pulled it out of policy because its being used in VersionInfoUpdater.

[Mattias Nissler (ping if slow), 2016/09/30 13:16:44]

VersionInfoUpdater is specifically for enterprise use. If you really like to
keep this here, I suggest you rename the function to GetEnterpriseMachineID()
and add a comment that this is kludgy and may *not* return the actual serial
number.

[Thiemo Nagel, 2016/09/30 13:22:10]

It looks like I'm missing something here.  My reading of the comment is that
GetMachineID() actually does return the real serial number (whereas the
Product_S/N entry has a checksum appended).  Could you maybe elaborate?

[Mattias Nissler (ping if slow), 2016/09/30 13:30:25]

The "correct" implementation of this would be to keep kSerialNumberKey at the
top, so all devices following the standard will return what is actually
considered the serial number. However, "Product_S/N" is at highest precedence
for enterprise code due to a previous buggy version of this that we started
with, causing server-side device records use the "wrong" serial number. We can't
fix the code here since that would cause the server-side records to become
mismatched with the device.

If any other code were to start using this as is, they'd get the incorrect (i.e.
checksum-appended) version of the serial number for Lumpy/Alex, which is likely
to cause grief for them.

[Thiemo Nagel, 2016/09/30 16:13:11]

Thanks a lot.  I had to read your review comment and the source code comment
many times and even take a look at physical lumpy and alex until I finally got
it right in my head.
Minor correction: The check-digit-appended version is actually the correct one
(printed on the device).  The method (now renamed to GetEnterpriseMachineID())
returns the version without the check digit.

+    kSerialNumberKey,  // VPD v2+ devices
+    "Product_SN",      // Mario
+    "sn",              // old ZGB devices (more recent ones use serial_number)
+};
+
 // Gets ListValue from given |dictionary| by given |key| and (unless |result| is
 // nullptr) sets |result| to a string with all list values joined by ','.
 // Returns true on success.
 bool JoinListValuesToString(const base::DictionaryValue* dictionary,
                             const std::string key,
                             std::string* result) {
   const base::ListValue* list = nullptr;
   if (!dictionary->GetListWithoutPathExpansion(key, &list))
     return false;
 
@@ skipping 67 matching lines @@
 const char kIsVmKey[] = "is_vm";
 const char kIsVmValueTrue[] = "1";
 const char kIsVmValueFalse[] = "0";
 const char kOffersCouponCodeKey[] = "ubind_attribute";
 const char kOffersGroupCodeKey[] = "gbind_attribute";
 const char kRlzBrandCodeKey[] = "rlz_brand_code";
 const char kWriteProtectSwitchBootKey[] = "wpsw_boot";
 const char kWriteProtectSwitchBootValueOff[] = "0";
 const char kWriteProtectSwitchBootValueOn[] = "1";
 const char kRegionKey[] = "region";
+const char kSerialNumberKey[] = "serial_number";
 const char kInitialLocaleKey[] = "initial_locale";
 const char kInitialTimezoneKey[] = "initial_timezone";
 const char kKeyboardLayoutKey[] = "keyboard_layout";
 
 // OEM specific statistics. Must be prefixed with "oem_".
 const char kOemCanExitEnterpriseEnrollmentKey[] = "oem_can_exit_enrollment";
 const char kOemDeviceRequisitionKey[] = "oem_device_requisition";
 const char kOemIsEnterpriseManagedKey[] = "oem_enterprise_managed";
 const char kOemKeyboardDrivenOobeKey[] = "oem_keyboard_driven_oobe";
 
 bool HasOemPrefix(const std::string& name) {
   return name.substr(0, 4) == "oem_";
 }
 
+std::string StatisticsProvider::GetMachineID() {
+  std::string machine_id;
+  for (size_t i = 0; i < arraysize(kMachineInfoSerialNumberKeys); i++) {

[emaxx, 2016/09/30 16:17:59]

nit: Use range-based for loop?

[Thiemo Nagel, 2016/09/30 16:57:58]

Great idea!  I never knew that was even possible...

+    if (GetMachineStatistic(kMachineInfoSerialNumberKeys[i], &machine_id) &&
+        !machine_id.empty()) {
+      break;
+    }
+  }
+  return machine_id;
+}
+
 // The StatisticsProvider implementation used in production.
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
   // StatisticsProvider implementation:
   void StartLoadingMachineStatistics(
       const scoped_refptr<base::TaskRunner>& file_task_runner,
       bool load_oem_manifest) override;
   bool GetMachineStatistic(const std::string& name,
                            std::string* result) override;
   bool GetMachineFlag(const std::string& name, bool* result) override;
@@ skipping 375 matching lines @@
   oem_manifest_loaded_ = true;
   VLOG(1) << "Loaded OEM Manifest statistics from " << file.value();
 }
 
 StatisticsProviderImpl* StatisticsProviderImpl::GetInstance() {
   return base::Singleton<
       StatisticsProviderImpl,
       base::DefaultSingletonTraits<StatisticsProviderImpl>>::get();
 }
 
-bool StatisticsProvider::HasMachineStatistic(const std::string& name) {
-  return GetMachineStatistic(name, nullptr);
-}
-
 static StatisticsProvider* g_test_statistics_provider = NULL;
 
 // static
 StatisticsProvider* StatisticsProvider::GetInstance() {
   if (g_test_statistics_provider)
     return g_test_statistics_provider;
   return StatisticsProviderImpl::GetInstance();
 }
 
 // static
 void StatisticsProvider::SetTestProvider(StatisticsProvider* test_provider) {
   g_test_statistics_provider = test_provider;
 }
 
 }  // namespace system
 }  // namespace chromeos