Change how a child's usage time is calculated for status report.

Bug: 881445
Change-Id: I745c35e6f3ba0dba1804ee5787a6dc811fb123db
Reviewed-on: https://chromium-review.googlesource.com/1238794
Commit-Queue: Henrique Grandinetti <hgrandinetti@google.com>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593578}

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc

@@ -57,6 +57,7 @@
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/session_manager/core/session_manager.h"
 #include "google_apis/gaia/gaia_oauth_client.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/url_request/url_request_test_util.h"
@@ -284,6 +285,9 @@ class DeviceCloudPolicyManagerChromeOSTest
  private:
   scoped_refptr<network::WeakWrapperSharedURLLoaderFactory>
       test_shared_loader_factory_;
+  // This property is required to instantiate the session manager, a singleton
+  // which is used by the device status collector.
+  session_manager::SessionManager session_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceCloudPolicyManagerChromeOSTest);
 };

chrome/browser/chromeos/policy/device_status_collector.cc

@@ -50,6 +50,7 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager/idle.pb.h"
 #include "chromeos/dbus/update_engine_client.h"
 #include "chromeos/dbus/util/version_loader.h"
 #include "chromeos/disks/disk_mount_manager.h"
@@ -71,6 +72,7 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "components/session_manager/session_manager_types.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_type.h"
@@ -712,12 +714,17 @@ DeviceStatusCollector::DeviceStatusCollector(
       max_stored_future_activity_interval_(kMaxStoredFutureActivityInterval),
       pref_service_(pref_service),
       last_idle_check_(Time()),
+      last_active_check_(base::Time()),
+      last_state_active_(true),
       volume_info_fetcher_(volume_info_fetcher),
       cpu_statistics_fetcher_(cpu_statistics_fetcher),
       cpu_temp_fetcher_(cpu_temp_fetcher),
       android_status_fetcher_(android_status_fetcher),
       statistics_provider_(provider),
       cros_settings_(chromeos::CrosSettings::Get()),
+      power_manager_(
+          chromeos::DBusThreadManager::Get()->GetPowerManagerClient()),
+      session_manager_(session_manager::SessionManager::Get()),
       is_enterprise_reporting_(is_enterprise_reporting),
       task_runner_(nullptr),
       weak_factory_(this) {
@@ -768,6 +775,10 @@ DeviceStatusCollector::DeviceStatusCollector(
   running_kiosk_app_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportRunningKioskApp, callback);
 
+  // Watch for changes on the device state to calculate the child's active time.
+  power_manager_->AddObserver(this);
+  session_manager_->AddObserver(this);
+
   // Fetch the current values of the policies.
   UpdateReportingSettings();
 
@@ -807,7 +818,10 @@ DeviceStatusCollector::DeviceStatusCollector(
       activity_day_start);
 }
 
-DeviceStatusCollector::~DeviceStatusCollector() {}
+DeviceStatusCollector::~DeviceStatusCollector() {
+  power_manager_->RemoveObserver(this);
+  session_manager_->RemoveObserver(this);
+}
 
 // static
 void DeviceStatusCollector::RegisterPrefs(PrefRegistrySimple* registry) {
@@ -906,18 +920,18 @@ void DeviceStatusCollector::ClearCachedResourceUsage() {
 }
 
 void DeviceStatusCollector::IdleStateCallback(ui::IdleState state) {
-  // Do nothing if device activity reporting is disabled.
-  if (!report_activity_times_)
+  // Do nothing if device activity reporting is disabled or if it's a child
+  // account. Usage time for child accounts are calculated differently.
+  if (!report_activity_times_ ||
+      user_manager::UserManager::Get()->IsLoggedInAsChildUser()) {
     return;
+  }
 
   Time now = GetCurrentTime();
 
   // For kiosk apps we report total uptime instead of active time.
   if (state == ui::IDLE_STATE_ACTIVE || IsKioskApp()) {
-    // Child user is the consumer user registered with DMServer and
-    // therefore eligible for non-enterprise reporting.
-    CHECK(is_enterprise_reporting_ ||
-          user_manager::UserManager::Get()->IsLoggedInAsChildUser());
+    CHECK(is_enterprise_reporting_);
     std::string user_email = GetUserForActivityReporting();
     // If it's been too long since the last report, or if the activity is
     // negative (which can happen when the clock changes), assume a single
@@ -939,6 +953,63 @@ void DeviceStatusCollector::IdleStateCallback(ui::IdleState state) {
   last_idle_check_ = now;
 }
 
+void DeviceStatusCollector::OnSessionStateChanged() {
+  UpdateChildUsageTime();
+  last_state_active_ =
+      session_manager::SessionManager::Get()->session_state() ==
+      session_manager::SessionState::ACTIVE;
+}
+
+void DeviceStatusCollector::ScreenIdleStateChanged(
+    const power_manager::ScreenIdleState& state) {
+  UpdateChildUsageTime();
+  // It is active if screen is on and if the session is also active.
+  last_state_active_ =
+      !state.off() && session_manager_->session_state() ==
+                          session_manager::SessionState::ACTIVE;
+}
+
+void DeviceStatusCollector::SuspendImminent(
+    power_manager::SuspendImminent::Reason reason) {
+  UpdateChildUsageTime();
+  // Device is going to be suspeded, so it won't be active.
+  last_state_active_ = false;
+}
+
+void DeviceStatusCollector::SuspendDone(const base::TimeDelta& sleep_duration) {
+  UpdateChildUsageTime();
+  // Device is returning from suspension, so it is considered active if the
+  // session is also active.
+  last_state_active_ = session_manager_->session_state() ==
+                       session_manager::SessionState::ACTIVE;
+}
+
+void DeviceStatusCollector::UpdateChildUsageTime() {
+  if (!report_activity_times_ ||
+      !user_manager::UserManager::Get()->IsLoggedInAsChildUser()) {
+    return;
+  }
+
+  if (last_active_check_.is_null()) {
+    last_active_check_ = GetCurrentTime();
+    return;
+  }
+
+  // Only child accounts should be using this method.
+  CHECK(user_manager::UserManager::Get()->IsLoggedInAsChildUser());
+
+  Time now = GetCurrentTime();
+  if (last_state_active_) {
+    activity_storage_->AddActivityPeriod(last_active_check_, now,
+                                         GetUserForActivityReporting());
+
+    activity_storage_->PruneActivityPeriods(
+        now, max_stored_past_activity_interval_,
+        max_stored_future_activity_interval_);
+  }
+  last_active_check_ = now;
+}
+
 std::unique_ptr<DeviceLocalAccount>
 DeviceStatusCollector::GetAutoLaunchedKioskSessionInfo() {
   std::unique_ptr<DeviceLocalAccount> account =
@@ -1069,6 +1140,9 @@ bool DeviceStatusCollector::IncludeEmailsInActivityReports() const {
 
 bool DeviceStatusCollector::GetActivityTimes(
     em::DeviceStatusReportRequest* status) {
+  if (user_manager::UserManager::Get()->IsLoggedInAsChildUser())
+    UpdateChildUsageTime();
+
   // If user reporting is off, data should be aggregated per day.
   // Signed-in user is reported in non-enterprise reporting.
   std::vector<ActivityStorage::ActivityPeriod> activity_times =

chrome/browser/chromeos/policy/device_status_collector.h

@@ -25,8 +25,11 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/dbus/cryptohome_client.h"
+#include "chromeos/dbus/power_manager_client.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_member.h"
+#include "components/session_manager/core/session_manager.h"
+#include "components/session_manager/core/session_manager_observer.h"
 #include "ui/base/idle/idle.h"
 
 namespace chromeos {
@@ -51,7 +54,8 @@ struct DeviceLocalAccount;
 class GetStatusState;
 
 // Collects and summarizes the status of an enterprised-managed ChromeOS device.
-class DeviceStatusCollector {
+class DeviceStatusCollector : public session_manager::SessionManagerObserver,
+                              public chromeos::PowerManagerClient::Observer {
  public:
   using VolumeInfoFetcher = base::Callback<
     std::vector<enterprise_management::VolumeInfo>(
@@ -104,7 +108,7 @@ class DeviceStatusCollector {
                         const AndroidStatusFetcher& android_status_fetcher,
                         base::TimeDelta activity_day_start,
                         bool is_enterprise_reporting);
-  virtual ~DeviceStatusCollector();
+  ~DeviceStatusCollector() override;
 
   // Gathers device and session status information and calls the passed response
   // callback. Null pointers passed into the response indicate errors or that
@@ -156,6 +160,19 @@ class DeviceStatusCollector {
   // next device status update.
   void SampleResourceUsage();
 
+  // session_manager::SessionManagerObserver:
+  void OnSessionStateChanged() override;
+
+  // power_manager::PowerManagerClient::Observer:
+  void ScreenIdleStateChanged(
+      const power_manager::ScreenIdleState& state) override;
+
+  // power_manager::PowerManagerClient::Observer:
+  void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
+
+  // power_manager::PowerManagerClient::Observer:
+  void SuspendDone(const base::TimeDelta& sleep_duration) override;
+
   // The timeout in the past to store device activity.
   // This is kept in case device status uploads fail for a number of days.
   base::TimeDelta max_stored_past_activity_interval_;
@@ -232,12 +249,22 @@ class DeviceStatusCollector {
   // reports.
   bool IncludeEmailsInActivityReports() const;
 
+  // Updates the child's active time.
+  void UpdateChildUsageTime();
+
   // Pref service that is mainly used to store activity periods for reporting.
   PrefService* const pref_service_;
 
   // The last time an idle state check was performed.
   base::Time last_idle_check_;
 
+  // The last time an active state check was performed.
+  base::Time last_active_check_;
+
+  // Whether the last state of the device was active. This is used for child
+  // accounts only. Active is defined as having the screen turned on.
+  bool last_state_active_;
+
   // The maximum key that went into the last report generated by
   // GetDeviceStatusAsync(), and the duration for it. This is used to trim
   // the stored data in OnSubmittedSuccessfully(). Trimming is delayed so
@@ -280,6 +307,12 @@ class DeviceStatusCollector {
 
   chromeos::CrosSettings* const cros_settings_;
 
+  // Power manager client. Used to listen to suspend and idle events.
+  chromeos::PowerManagerClient* const power_manager_;
+
+  // Session manager. Used to listen to session state changes.
+  session_manager::SessionManager* const session_manager_;
+
   // Stores and filters activity periods used for reporting.
   std::unique_ptr<ActivityStorage> activity_storage_;
 

chrome/browser/chromeos/policy/device_status_collector_browsertest.cc

@@ -46,7 +46,9 @@
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/cros_disks_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_power_manager_client.h"
 #include "chromeos/dbus/fake_update_engine_client.h"
+#include "chromeos/dbus/power_manager/idle.pb.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_ipconfig_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
@@ -382,6 +384,10 @@ class DeviceStatusCollectorTest : public testing::Test {
 
     chromeos::CrasAudioHandler::InitializeForTesting();
     chromeos::LoginState::Initialize();
+
+    fake_power_manager_client_ = new chromeos::FakePowerManagerClient;
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetPowerManagerClient(
+        base::WrapUnique(fake_power_manager_client_));
   }
 
   ~DeviceStatusCollectorTest() override {
@@ -410,6 +416,50 @@ class DeviceStatusCollectorTest : public testing::Test {
   void TearDown() override { settings_helper_.RestoreProvider(); }
 
  protected:
+  // States tracked to calculate a child's active time.
+  enum class DeviceStateTransitions {
+    kEnterIdleState,
+    kLeaveIdleState,
+    kEnterSleep,
+    kLeaveSleep,
+    kEnterSessionActive,
+    kLeaveSessionActive
+  };
+
+  void SimulateStateChanges(DeviceStateTransitions* states, int len) {
+    for (int i = 0; i < len; i++) {
+      switch (states[i]) {
+        case DeviceStateTransitions::kEnterIdleState: {
+          power_manager::ScreenIdleState state;
+          state.set_off(true);
+          fake_power_manager_client_->SendScreenIdleStateChanged(state);
+        } break;
+        case DeviceStateTransitions::kLeaveIdleState: {
+          power_manager::ScreenIdleState state;
+          state.set_off(false);
+          fake_power_manager_client_->SendScreenIdleStateChanged(state);
+        } break;
+        case DeviceStateTransitions::kEnterSleep:
+          fake_power_manager_client_->SendSuspendImminent(
+              power_manager::SuspendImminent_Reason_LID_CLOSED);
+          break;
+        case DeviceStateTransitions::kLeaveSleep:
+          fake_power_manager_client_->SendSuspendDone(
+              base::TimeDelta::FromSeconds(
+                  policy::DeviceStatusCollector::kIdlePollIntervalSeconds));
+          break;
+        case DeviceStateTransitions::kEnterSessionActive:
+          session_manager::SessionManager::Get()->SetSessionState(
+              session_manager::SessionState::ACTIVE);
+          break;
+        case DeviceStateTransitions::kLeaveSessionActive:
+          session_manager::SessionManager::Get()->SetSessionState(
+              session_manager::SessionState::LOCKED);
+          break;
+      }
+    }
+  }
+
   void AddMountPoint(const std::string& mount_point) {
     mount_point_map_.insert(DiskMountManager::MountPointMap::value_type(
         mount_point, DiskMountManager::MountPointInfo(
@@ -593,6 +643,13 @@ class DeviceStatusCollectorTest : public testing::Test {
   base::ScopedPathOverride user_data_dir_override_;
   chromeos::FakeUpdateEngineClient* const update_engine_client_;
   std::unique_ptr<base::RunLoop> run_loop_;
+
+  // Owned by chromeos::DBusThreadManager.
+  chromeos::FakePowerManagerClient* fake_power_manager_client_;
+
+  // This property is required to instantiate the session manager, a singleton
+  // which is used by the device status collector.
+  session_manager::SessionManager session_manager_;
 };
 
 TEST_F(DeviceStatusCollectorTest, AllIdle) {
@@ -2052,7 +2109,6 @@ class DeviceStatusCollectorNetworkInterfacesTest
 
   void TearDown() override {
     chromeos::NetworkHandler::Shutdown();
-    chromeos::DBusThreadManager::Shutdown();
   }
 
   void VerifyNetworkReporting() {
@@ -2356,16 +2412,56 @@ class ConsumerDeviceStatusCollectorTimeLimitEnabledTest
 };
 
 TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest,
-       ReportingActivityTimes) {
-  ui::IdleState test_states[] = {ui::IDLE_STATE_ACTIVE, ui::IDLE_STATE_ACTIVE,
-                                 ui::IDLE_STATE_ACTIVE};
-  status_collector_->Simulate(test_states,
-                              sizeof(test_states) / sizeof(ui::IdleState));
+       ReportingActivityTimesSessionTransistions) {
+  DeviceStateTransitions test_states[] = {
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive,
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive};
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
 
   GetStatus();
 
   ASSERT_EQ(1, device_status_.active_period_size());
-  EXPECT_EQ(3 * ActivePeriodMilliseconds(),
+  EXPECT_EQ(2 * ActivePeriodMilliseconds(),
+            GetActiveMilliseconds(device_status_));
+  EXPECT_EQ(user_account_id_.GetUserEmail(),
+            device_status_.active_period(0).user_email());
+}
+
+TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest,
+       ReportingActivityTimesSleepTransistions) {
+  DeviceStateTransitions test_states[] = {
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kEnterSleep, DeviceStateTransitions::kLeaveSleep,
+      DeviceStateTransitions::kLeaveSessionActive};
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
+
+  GetStatus();
+
+  ASSERT_EQ(1, device_status_.active_period_size());
+  EXPECT_EQ(2 * ActivePeriodMilliseconds(),
+            GetActiveMilliseconds(device_status_));
+  EXPECT_EQ(user_account_id_.GetUserEmail(),
+            device_status_.active_period(0).user_email());
+}
+
+TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest,
+       ReportingActivityTimesIdleTransitions) {
+  DeviceStateTransitions test_states[] = {
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kEnterIdleState,
+      DeviceStateTransitions::kLeaveIdleState,
+      DeviceStateTransitions::kLeaveSessionActive};
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
+
+  GetStatus();
+
+  ASSERT_EQ(1, device_status_.active_period_size());
+  EXPECT_EQ(2 * ActivePeriodMilliseconds(),
             GetActiveMilliseconds(device_status_));
   EXPECT_EQ(user_account_id_.GetUserEmail(),
             device_status_.active_period(0).user_email());
@@ -2375,11 +2471,15 @@ TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest, ActivityKeptInPref) {
   EXPECT_TRUE(
       profile_pref_service_.GetDictionary(prefs::kUserActivityTimes)->empty());
 
-  ui::IdleState test_states[] = {ui::IDLE_STATE_ACTIVE, ui::IDLE_STATE_IDLE,
-                                 ui::IDLE_STATE_ACTIVE, ui::IDLE_STATE_ACTIVE,
-                                 ui::IDLE_STATE_IDLE,   ui::IDLE_STATE_IDLE};
-  status_collector_->Simulate(test_states,
-                              sizeof(test_states) / sizeof(ui::IdleState));
+  DeviceStateTransitions test_states[] = {
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive,
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive,
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive};
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
   EXPECT_FALSE(
       profile_pref_service_.GetDictionary(prefs::kUserActivityTimes)->empty());
 
@@ -2390,8 +2490,8 @@ TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest, ActivityKeptInPref) {
                          base::BindRepeating(&GetEmptyCPUStatistics),
                          base::BindRepeating(&GetEmptyCPUTempInfo),
                          base::BindRepeating(&GetEmptyAndroidStatus));
-  status_collector_->Simulate(test_states,
-                              sizeof(test_states) / sizeof(ui::IdleState));
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
 
   GetStatus();
   EXPECT_EQ(6 * ActivePeriodMilliseconds(),
@@ -2402,10 +2502,15 @@ TEST_F(ConsumerDeviceStatusCollectorTimeLimitEnabledTest,
        ActivityNotWrittenToLocalState) {
   EXPECT_TRUE(local_state_.GetDictionary(prefs::kDeviceActivityTimes)->empty());
 
-  ui::IdleState test_states[] = {ui::IDLE_STATE_ACTIVE, ui::IDLE_STATE_ACTIVE,
-                                 ui::IDLE_STATE_ACTIVE};
-  status_collector_->Simulate(test_states,
-                              sizeof(test_states) / sizeof(ui::IdleState));
+  DeviceStateTransitions test_states[] = {
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive,
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive,
+      DeviceStateTransitions::kEnterSessionActive,
+      DeviceStateTransitions::kLeaveSessionActive};
+  SimulateStateChanges(test_states,
+                       sizeof(test_states) / sizeof(DeviceStateTransitions));
   GetStatus();
   EXPECT_EQ(1, device_status_.active_period_size());
   EXPECT_EQ(3 * ActivePeriodMilliseconds(),

chrome/browser/chromeos/policy/status_uploader_unittest.cc

@@ -21,6 +21,7 @@
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/core/common/cloud/mock_device_management_service.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/session_manager/core/session_manager.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -176,6 +177,9 @@ class StatusUploaderTest : public testing::Test {
   MockCloudPolicyClient client_;
   MockDeviceManagementService device_management_service_;
   TestingPrefServiceSimple prefs_;
+  // This property is required to instantiate the session manager, a singleton
+  // which is used by the device status collector.
+  session_manager::SessionManager session_manager_;
 };
 
 TEST_F(StatusUploaderTest, BasicTest) {