Stop comparing timestamps as strings on UsageTimeLimitProcessor.

Bug: 908443
Change-Id: Icf41f1753450cee63b9296652599f5eb7c4de671
Reviewed-on: https://chromium-review.googlesource.com/c/1351149
Commit-Queue: Henrique Grandinetti <hgrandinetti@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611309}

chrome/browser/chromeos/child_accounts/usage_time_limit_processor.cc

@@ -991,7 +991,9 @@ TimeLimitOverride::TimeLimitOverride(const base::Value& override_list) {
     return;
   }
 
+  // The most recent override created.
   const base::Value* last_override = nullptr;
+  int64_t last_override_created_at_millis;
   for (const base::Value& override_value : override_list.GetList()) {
     if (!override_value.is_dict()) {
       LOG(ERROR) << "Override entry is not a dictionary";
@@ -1005,11 +1007,28 @@ TimeLimitOverride::TimeLimitOverride(const base::Value& override_list) {
       LOG(ERROR) << "Override entry is missing created_at_millis field.";
       continue;
     }
-    if (!last_override ||
-        created_at_value->GetString().compare(
-            last_override->FindKey(kOverrideActionCreatedAt)->GetString()) >
-            0) {
+
+    if (!last_override) {
+      // Check if the creation time is a valid number.
+      if (!base::StringToInt64(created_at_value->GetString(),
+                               &last_override_created_at_millis)) {
+        LOG(ERROR) << "Invalid override created_at_millis.";
+        continue;
+      }
+      last_override = &override_value;
+      continue;
+    }
+
+    int64_t current_override_created_at_millis;
+    if (!base::StringToInt64(created_at_value->GetString(),
+                             &current_override_created_at_millis)) {
+      LOG(ERROR) << "Invalid override created_at_millis.";
+      continue;
+    }
+
+    if (current_override_created_at_millis > last_override_created_at_millis) {
       last_override = &override_value;
+      last_override_created_at_millis = current_override_created_at_millis;
     }
   }
 
@@ -1021,20 +1040,10 @@ TimeLimitOverride::TimeLimitOverride(const base::Value& override_list) {
   if (!action_value)
     return;
 
-  const base::Value* created_at_value =
-      last_override->FindKey(kOverrideActionCreatedAt);
-
-  int64_t created_at_millis;
-  if (!base::StringToInt64(created_at_value->GetString(), &created_at_millis)) {
-    LOG(ERROR) << "Invalid override created_at_millis.";
-    // Cannot process entry without a valid creation time.
-    return;
-  }
-
   action = action_value->GetString() == kOverrideActionLock ? Action::kLock
                                                             : Action::kUnlock;
-  created_at = base::Time::UnixEpoch() +
-               base::TimeDelta::FromMilliseconds(created_at_millis);
+  created_at = base::Time::UnixEpoch() + base::TimeDelta::FromMilliseconds(
+                                             last_override_created_at_millis);
 
   const base::Value* duration_value = last_override->FindPath(
       {kOverrideActionSpecificData, kOverrideActionDurationMins});

chrome/browser/chromeos/child_accounts/usage_time_limit_processor_unittest.cc

@@ -182,7 +182,6 @@ namespace internal {
 
 using UsageTimeLimitProcessorInternalTest = testing::Test;
 
-// TODO: improve tests to cover bugs on crbug/894047
 TEST_F(UsageTimeLimitProcessorInternalTest, TimeLimitWindowValid) {
   std::string last_updated_millis =
       CreatePolicyTimestamp("1 Jan 1970 00:00:00");
@@ -286,11 +285,11 @@ TEST_F(UsageTimeLimitProcessorInternalTest, OverrideValid) {
   // Create policy information.
   std::string created_at_millis = CreatePolicyTimestamp("1 Jan 2018 10:00:00");
   base::Value override_one = base::Value(base::Value::Type::DICTIONARY);
-  override_one.SetKey("action", base::Value("UNLOCK"));
+  override_one.SetKey("action", base::Value(kUnlock));
   override_one.SetKey("created_at_millis", base::Value(created_at_millis));
 
   base::Value override_two = base::Value(base::Value::Type::DICTIONARY);
-  override_two.SetKey("action", base::Value("LOCK"));
+  override_two.SetKey("action", base::Value(kLock));
   override_two.SetKey("created_at_millis",
                       base::Value(CreatePolicyTimestamp("1 Jan 2018 9:00:00")));
 
@@ -307,6 +306,42 @@ TEST_F(UsageTimeLimitProcessorInternalTest, OverrideValid) {
   ASSERT_FALSE(override_struct.duration);
 }
 
+// Check that the most recent override is chosen when more than one is sent on
+// the policy. This test covers the corner case when the timestamp strings have
+// different sizes.
+TEST_F(UsageTimeLimitProcessorInternalTest, MultipleOverrides) {
+  // Create policy information.
+  base::Value override_one = base::Value(base::Value::Type::DICTIONARY);
+  override_one.SetKey("action", base::Value(kUnlock));
+  override_one.SetKey("created_at_millis", base::Value("1000000"));
+
+  base::Value override_two = base::Value(base::Value::Type::DICTIONARY);
+  override_two.SetKey("action", base::Value(kLock));
+  override_two.SetKey("created_at_millis", base::Value("999999"));
+
+  base::Value override_three = base::Value(base::Value::Type::DICTIONARY);
+  override_two.SetKey("action", base::Value(kLock));
+  override_two.SetKey("created_at_millis", base::Value("900000"));
+
+  base::Value override_four = base::Value(base::Value::Type::DICTIONARY);
+  override_two.SetKey("action", base::Value(kUnlock));
+  override_two.SetKey("created_at_millis", base::Value("1200000"));
+
+  base::Value overrides(base::Value::Type::LIST);
+  overrides.GetList().push_back(std::move(override_one));
+  overrides.GetList().push_back(std::move(override_two));
+  overrides.GetList().push_back(std::move(override_three));
+  overrides.GetList().push_back(std::move(override_four));
+
+  // Call tested functions.
+  TimeLimitOverride override_struct(overrides);
+
+  // Assert right fields are set.
+  ASSERT_EQ(override_struct.action, TimeLimitOverride::Action::kUnlock);
+  ASSERT_EQ(override_struct.created_at, base::Time::FromJavaTime(1200000));
+  ASSERT_FALSE(override_struct.duration);
+}
+
 }  // namespace internal
 
 // Tests the GetState for a policy that only have the time window limit set. It