Extract factory ping embargo check

Moves the factory ping embargo check from rlz_lib::RlzValueStoreChromeOS
to the function chromeos::system::GetFactoryPingEmbargoState.
This avoids a dependency from chromeos to rlz_lib.

Bug: 839353
Test: chromeos_unittests && rlz_unittests
Change-Id: Ia20c9a268d1030a77b1e138632005866c24d0788
Reviewed-on: https://chromium-review.googlesource.com/1059409
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560170}

chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc

@@ -16,10 +16,10 @@
 #include "chromeos/dbus/cryptohome/rpc.pb.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/system/factory_ping_embargo_check.h"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "net/url_request/url_request_context_getter.h"
-#include "rlz/chromeos/lib/rlz_value_store_chromeos.h"
 
 namespace chromeos {
 
@@ -193,16 +193,15 @@ AutoEnrollmentController::InitialEnrollmentRequirement
 AutoEnrollmentController::GetInitialEnrollmentRequirement() {
   system::StatisticsProvider* provider =
       system::StatisticsProvider::GetInstance();
-  rlz_lib::RlzValueStoreChromeOS::EmbargoState embargo_state =
+  system::FactoryPingEmbargoState embargo_state =
-      rlz_lib::RlzValueStoreChromeOS::GetRlzEmbargoState();
+      system::GetFactoryPingEmbargoState(provider);
-  if (embargo_state == rlz_lib::RlzValueStoreChromeOS::EmbargoState::kInvalid) {
+  if (embargo_state == system::FactoryPingEmbargoState::kInvalid) {
     LOG(WARNING)
         << "Skip Initial Enrollment Check due to invalid embargo date.";
     // TODO(pmarko): UMA Stat.
     return InitialEnrollmentRequirement::kNotRequired;
   }
-  if (embargo_state ==
+  if (embargo_state == system::FactoryPingEmbargoState::kNotPassed) {
-      rlz_lib::RlzValueStoreChromeOS::EmbargoState::kNotPassed) {
     VLOG(1) << "Skip Initial Enrollment Check due to not-passed embargo date.";
     return InitialEnrollmentRequirement::kNotRequired;
   }

chromeos/BUILD.gn

@@ -518,6 +518,8 @@ component("chromeos") {
     "system/devicemode.h",
     "system/devicetype.cc",
     "system/devicetype.h",
+    "system/factory_ping_embargo_check.cc",
+    "system/factory_ping_embargo_check.h",
 
     # Used when running mash, both on Linux and on real devices.
     "system/fake_statistics_provider.cc",
@@ -767,6 +769,7 @@ test("chromeos_unittests") {
     "run_all_unittests.cc",
     "settings/timezone_settings_unittest.cc",
     "system/cpu_temperature_reader_unittest.cc",
+    "system/factory_ping_embargo_check_unittest.cc",
     "system/name_value_pairs_parser_unittest.cc",
     "system/version_loader_unittest.cc",
     "timezone/timezone_unittest.cc",

chromeos/system/factory_ping_embargo_check.cc

@@ -0,0 +1,45 @@
+// 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/system/factory_ping_embargo_check.h"
+
+#include <string>
+
+#include "chromeos/system/statistics_provider.h"
+
+namespace chromeos {
+namespace system {
+
+FactoryPingEmbargoState GetFactoryPingEmbargoState(
+    StatisticsProvider* statistics_provider) {
+  std::string rlz_embargo_end_date;
+  if (!statistics_provider->GetMachineStatistic(kRlzEmbargoEndDateKey,
+                                                &rlz_embargo_end_date)) {
+    // |rlz_embargo_end_date| only exists on new devices that have not yet
+    // launched.
+    return FactoryPingEmbargoState::kMissingOrMalformed;
+  }
+  base::Time parsed_time;
+  if (!base::Time::FromUTCString(rlz_embargo_end_date.c_str(), &parsed_time)) {
+    LOG(ERROR) << "|rlz_embargo_end_date| exists but cannot be parsed.";
+    return FactoryPingEmbargoState::kMissingOrMalformed;
+  }
+
+  if (parsed_time - base::Time::Now() >=
+      kRlzEmbargoEndDateGarbageDateThreshold) {
+    // If |rlz_embargo_end_date| is more than this many days in the future,
+    // ignore it. Because it indicates that the device is not connected to an
+    // ntp server in the factory, and its internal clock could be off when the
+    // date is written.
+    // TODO(pmarko): UMA stat for how often this happens
+    // (https://crbug.com/839353).
+    return FactoryPingEmbargoState::kInvalid;
+  }
+
+  return base::Time::Now() > parsed_time ? FactoryPingEmbargoState::kPassed
+                                         : FactoryPingEmbargoState::kNotPassed;
+}
+
+}  // namespace system
+}  // namespace chromeos

chromeos/system/factory_ping_embargo_check.h

@@ -0,0 +1,42 @@
+// 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_SYSTEM_FACTORY_PING_EMBARGO_CHECK_H_
+#define CHROMEOS_SYSTEM_FACTORY_PING_EMBARGO_CHECK_H_
+
+#include "base/time/time.h"
+#include "chromeos/chromeos_export.h"
+
+namespace chromeos {
+namespace system {
+
+class StatisticsProvider;
+
+// The RLZ embargo end date is considered invalid if it's more than this many
+// days in the future.
+constexpr base::TimeDelta kRlzEmbargoEndDateGarbageDateThreshold =
+    base::TimeDelta::FromDays(14);
+
+enum class FactoryPingEmbargoState {
+  // There is no correctly formatted factory ping embargo end date value in
+  // VPD.
+  kMissingOrMalformed,
+  // There is a correctly formatted factory ping embargo end date value in VPD
+  // which is too far in the future (indicating that the time source used in
+  // the factory to write the embargo end date was not based on a not
+  // synchronized clock).
+  kInvalid,
+  // The embargo period has not passed yet.
+  kNotPassed,
+  // The embargo period has passed.
+  kPassed
+};
+
+CHROMEOS_EXPORT FactoryPingEmbargoState
+GetFactoryPingEmbargoState(StatisticsProvider* statistics_provider);
+
+}  // namespace system
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SYSTEM_FACTORY_PING_EMBARGO_CHECK_H_

chromeos/system/factory_ping_embargo_check_unittest.cc

@@ -0,0 +1,98 @@
+// 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/system/factory_ping_embargo_check.h"
+
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/time.h"
+#include "chromeos/system/fake_statistics_provider.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace system {
+
+namespace {
+
+// Returns a string which can be put into the |kRlzEmbargoEndDateKey| VPD
+// variable. If |days_offset| is 0, the return value represents the current day.
+// If |days_offset| is positive, the return value represents |days_offset| days
+// in the future. If |days_offset| is negative, the return value represents
+// |days_offset| days in the past.
+// For example, if the test runs on 2018-01-22 and |days_offset| is 3, the
+// return value will be "2018-01-25". Similarly, if |days_offset| is -1, the
+// return value will be "2018-01-21".
+std::string GenerateEmbargoEndDate(int days_offset) {
+  base::Time::Exploded exploded;
+  const base::Time target_time =
+      base::Time::Now() + base::TimeDelta::FromDays(days_offset);
+  target_time.UTCExplode(&exploded);
+
+  const std::string rlz_embargo_end_date_string = base::StringPrintf(
+      "%04d-%02d-%02d", exploded.year, exploded.month, exploded.day_of_month);
+
+  // Sanity check that base::Time::FromUTCString can read back the format used
+  // here.
+  base::Time reparsed_time;
+  EXPECT_TRUE(base::Time::FromUTCString(rlz_embargo_end_date_string.c_str(),
+                                        &reparsed_time));
+  EXPECT_EQ(target_time.ToDeltaSinceWindowsEpoch().InMicroseconds() /
+                base::Time::kMicrosecondsPerDay,
+            reparsed_time.ToDeltaSinceWindowsEpoch().InMicroseconds() /
+                base::Time::kMicrosecondsPerDay);
+
+  return rlz_embargo_end_date_string;
+}
+
+}  // namespace
+
+class FactoryPingEmbargoCheckTest : public ::testing::Test {
+ protected:
+  FakeStatisticsProvider statistics_provider_;
+};
+
+// No embargo end date in VPD.
+TEST_F(FactoryPingEmbargoCheckTest, NoValue) {
+  EXPECT_EQ(FactoryPingEmbargoState::kMissingOrMalformed,
+            GetFactoryPingEmbargoState(&statistics_provider_));
+}
+
+// There is a malformed embargo end date in VPD.
+TEST_F(FactoryPingEmbargoCheckTest, MalformedValue) {
+  statistics_provider_.SetMachineStatistic(
+      chromeos::system::kRlzEmbargoEndDateKey, "blabla");
+  EXPECT_EQ(FactoryPingEmbargoState::kMissingOrMalformed,
+            GetFactoryPingEmbargoState(&statistics_provider_));
+}
+
+// There is an embargo end date in VPD which is too far in the future to be
+// plausible.
+TEST_F(FactoryPingEmbargoCheckTest, InvalidValue) {
+  statistics_provider_.SetMachineStatistic(
+      chromeos::system::kRlzEmbargoEndDateKey,
+      GenerateEmbargoEndDate(15 /* days_offset */));
+  EXPECT_EQ(FactoryPingEmbargoState::kInvalid,
+            GetFactoryPingEmbargoState(&statistics_provider_));
+}
+
+// The current time is before a (valid and plausible) embargo end date.
+TEST_F(FactoryPingEmbargoCheckTest, EmbargoNotPassed) {
+  statistics_provider_.SetMachineStatistic(
+      chromeos::system::kRlzEmbargoEndDateKey,
+      GenerateEmbargoEndDate(1 /* days_offset */));
+  EXPECT_EQ(FactoryPingEmbargoState::kNotPassed,
+            GetFactoryPingEmbargoState(&statistics_provider_));
+}
+
+// The current time is after a (valid and plausible) embargo end date.
+TEST_F(FactoryPingEmbargoCheckTest, EmbargoPassed) {
+  statistics_provider_.SetMachineStatistic(
+      chromeos::system::kRlzEmbargoEndDateKey,
+      GenerateEmbargoEndDate(-1 /* days_offset */));
+  EXPECT_EQ(FactoryPingEmbargoState::kPassed,
+            GetFactoryPingEmbargoState(&statistics_provider_));
+}
+
+}  // namespace system
+}  // namespace chromeos

rlz/chromeos/lib/rlz_value_store_chromeos.cc

@@ -19,6 +19,7 @@
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
+#include "chromeos/system/factory_ping_embargo_check.h"
 #include "chromeos/system/statistics_provider.h"
 #include "rlz/lib/financial_ping.h"
 #include "rlz/lib/lib_values.h"
@@ -86,9 +87,6 @@ std::string GetKeyName(const std::string& key, Product product) {
 
 }  // namespace
 
-const int RlzValueStoreChromeOS::kRlzEmbargoEndDateGarbageDateThresholdDays =
-    14;
-
 const int RlzValueStoreChromeOS::kMaxRetryCount = 3;
 
 RlzValueStoreChromeOS::RlzValueStoreChromeOS(const base::FilePath& store_path)
@@ -291,43 +289,12 @@ void RlzValueStoreChromeOS::CollectGarbage() {
   NOTIMPLEMENTED();
 }
 
-// static
-RlzValueStoreChromeOS::EmbargoState
-RlzValueStoreChromeOS::GetRlzEmbargoState() {
-  chromeos::system::StatisticsProvider* stats =
-      chromeos::system::StatisticsProvider::GetInstance();
-
-  std::string rlz_embargo_end_date;
-  if (!stats->GetMachineStatistic(chromeos::system::kRlzEmbargoEndDateKey,
-                                  &rlz_embargo_end_date)) {
-    // |rlz_embargo_end_date| only exists on new devices that have not yet
-    // launched.
-    return EmbargoState::kMissingOrMalformed;
-  }
-  base::Time parsed_time;
-  if (!base::Time::FromUTCString(rlz_embargo_end_date.c_str(), &parsed_time)) {
-    LOG(ERROR) << "|rlz_embargo_end_date| exists but cannot be parsed.";
-    return EmbargoState::kMissingOrMalformed;
-  }
-
-  if (parsed_time - base::Time::Now() >=
-      base::TimeDelta::FromDays(
-          RlzValueStoreChromeOS::kRlzEmbargoEndDateGarbageDateThresholdDays)) {
-    // If |rlz_embargo_end_date| is more than this many days in the future,
-    // ignore it. Because it indicates that the device is not connected to an
-    // ntp server in the factory, and its internal clock could be off when the
-    // date is written.
-    // TODO(pmarko): UMA stat for how often this happens.
-    return EmbargoState::kInvalid;
-  }
-
-  return base::Time::Now() > parsed_time ? EmbargoState::kPassed
-                                         : EmbargoState::kNotPassed;
-}
-
 // static
 bool RlzValueStoreChromeOS::HasRlzEmbargoEndDatePassed() {
-  return GetRlzEmbargoState() != EmbargoState::kNotPassed;
+  chromeos::system::StatisticsProvider* statistics_provider =
+      chromeos::system::StatisticsProvider::GetInstance();
+  return chromeos::system::GetFactoryPingEmbargoState(statistics_provider) !=
+         chromeos::system::FactoryPingEmbargoState::kNotPassed;
 }
 
 void RlzValueStoreChromeOS::ReadStore() {

rlz/chromeos/lib/rlz_value_store_chromeos.h

@@ -26,10 +26,6 @@ namespace rlz_lib {
 // An implementation of RlzValueStore for ChromeOS.
 class RlzValueStoreChromeOS : public RlzValueStore {
  public:
-  // Ignore |kRlzEmbargoEndDateKey| if it's more than this many days in the
-  // future.
-  static const int kRlzEmbargoEndDateGarbageDateThresholdDays;
-
   // The maximum retry times allowed for |SetRlzPingSent|.
   static const int kMaxRetryCount;
 
@@ -63,15 +59,6 @@ class RlzValueStoreChromeOS : public RlzValueStore {
 
   void CollectGarbage() override;
 
-  enum class EmbargoState {
-    kMissingOrMalformed,
-    kInvalid,
-    kNotPassed,
-    kPassed
-  };
-
-  static EmbargoState GetRlzEmbargoState();
-
  private:
   // Returns true if the |rlz_embargo_end_date| present in VPD has passed
   // compared to the current time.

rlz/lib/financial_ping_test.cc

@@ -39,6 +39,7 @@
 #endif
 
 #if defined(OS_CHROMEOS)
+#include "chromeos/system/factory_ping_embargo_check.h"
 #include "rlz/chromeos/lib/rlz_value_store_chromeos.h"
 #endif
 
@@ -352,9 +353,8 @@ TEST_F(FinancialPingTest, RlzEmbargoEndDate) {
   // |IsPingTime| is false.
   base::Time future_rlz_embargo_date =
       base::Time::NowFromSystemTime() +
-      base::TimeDelta::FromDays(rlz_lib::RlzValueStoreChromeOS::
+      chromeos::system::kRlzEmbargoEndDateGarbageDateThreshold -
-                                    kRlzEmbargoEndDateGarbageDateThresholdDays -
+      base::TimeDelta::FromDays(1);
-                                1);
   future_rlz_embargo_date.LocalExplode(&exploded);
   std::string future_rlz_embargo_date_value =
       ConvertExplodedToRlzEmbargoDate(exploded);
@@ -369,9 +369,8 @@ TEST_F(FinancialPingTest, RlzEmbargoEndDate) {
   // |IsPingTime| is true.
   future_rlz_embargo_date =
       base::Time::NowFromSystemTime() +
-      base::TimeDelta::FromDays(rlz_lib::RlzValueStoreChromeOS::
+      chromeos::system::kRlzEmbargoEndDateGarbageDateThreshold +
-                                    kRlzEmbargoEndDateGarbageDateThresholdDays +
+      base::TimeDelta::FromDays(1);
-                                1);
   future_rlz_embargo_date.LocalExplode(&exploded);
   future_rlz_embargo_date_value = ConvertExplodedToRlzEmbargoDate(exploded);
   statistics_provider_->SetMachineStatistic(