Apply individual policies for the various parts of device status reports.

chrome/browser/policy/device_status_collector.cc

Index: chrome/browser/policy/device_status_collector.cc
diff --git a/chrome/browser/policy/device_status_collector.cc b/chrome/browser/policy/device_status_collector.cc
index 3d82084c4e62ab1de5651146cb0b643bf5396426..99115229933ed76b47976448c2d0101989a4a0f7 100644
--- a/chrome/browser/policy/device_status_collector.cc
+++ b/chrome/browser/policy/device_status_collector.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/policy/proto/device_management_backend.pb.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/common/pref_names.h"

[Joao da Silva, 2012/01/26 09:48:58]

Nit: alphabetic order

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

 #include "chrome/common/chrome_version_info.h"
 
 using base::Time;
@@ -26,9 +27,6 @@ const unsigned int kIdleStateThresholdSeconds = 300;
 // The maximum number of time periods stored in the local state.
 const unsigned int kMaxStoredActivePeriods = 500;
 
-// Stores the baseline timestamp, to which the active periods are relative.
-const char* const kPrefBaselineTime = "device_status.baseline_timestamp";
-
 // Stores a list of timestamps representing device active periods.
 const char* const kPrefDeviceActivePeriods = "device_status.active_periods";
 
@@ -71,8 +69,10 @@ DeviceStatusCollector::~DeviceStatusCollector() {
 
 // static
 void DeviceStatusCollector::RegisterPrefs(PrefService* local_state) {
-  local_state->RegisterInt64Pref(kPrefBaselineTime, Time::Now().ToTimeT());
   local_state->RegisterListPref(kPrefDeviceActivePeriods, new ListValue);
+  local_state->RegisterBooleanPref(prefs::kReportDeviceVersionInfo, false);

[pastarmovj, 2012/01/26 09:52:11]

You won't need this once you regard my next comment on line 122.

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

+  local_state->RegisterBooleanPref(prefs::kReportDeviceActivityTimes, false);
+  local_state->RegisterBooleanPref(prefs::kReportDeviceBootMode, false);
 }
 
 void DeviceStatusCollector::CheckIdleState() {
@@ -118,6 +118,10 @@ void DeviceStatusCollector::AddActivePeriod(Time start, Time end) {
 }
 
 void DeviceStatusCollector::IdleStateCallback(IdleState state) {
+  // Do nothing if device activity reporting is disabled.
+  if (!local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))

[pastarmovj, 2012/01/26 09:52:11]

This is not the right way to read device policy. If you read it this way you
open up the possibility for an attacker to put arbitrary values in |local_state|
as long as this policy is not set on the server. The right way would be to use
the CrosSettings interface to DeviceSettingsProvider which can tell if the
values that it ships are really coming from the trusted policy blob so it will
be much harder to override those as the policy blob is root owned.

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

+    return;
+
   Time now = GetCurrentTime();
 
   if (state == IDLE_STATE_ACTIVE) {
@@ -134,8 +138,8 @@ void DeviceStatusCollector::IdleStateCallback(IdleState state) {
   last_idle_state_ = state;
 }
 
-void DeviceStatusCollector::GetStatus(em::DeviceStatusReportRequest* request) {
-  // Report device active periods.
+void DeviceStatusCollector::GetActivityTimes(
+    em::DeviceStatusReportRequest* request) {
   const ListValue* active_periods =
       local_state_->GetList(kPrefDeviceActivePeriods);
   em::TimePeriod* time_period;
@@ -159,13 +163,18 @@ void DeviceStatusCollector::GetStatus(em::DeviceStatusReportRequest* request) {
   }
   ListPrefUpdate update(local_state_, kPrefDeviceActivePeriods);
   update.Get()->Clear();
+}
 
+void DeviceStatusCollector::GetVersionInfo(
+    em::DeviceStatusReportRequest* request) {
   chrome::VersionInfo version_info;
   request->set_browser_version(version_info.Version());
   request->set_os_version(os_version_);
   request->set_firmware_version(firmware_version_);
+}
 
-  // Report the state of the dev switch at boot.
+void DeviceStatusCollector::GetBootMode(
+    em::DeviceStatusReportRequest* request) {
   std::string dev_switch_mode;
   if (statistics_provider_->GetMachineStatistic(
       "devsw_boot", &dev_switch_mode)) {
@@ -176,6 +185,17 @@ void DeviceStatusCollector::GetStatus(em::DeviceStatusReportRequest* request) {
   }
 }
 
+void DeviceStatusCollector::GetStatus(em::DeviceStatusReportRequest* request) {
+  if (local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))

[pastarmovj, 2012/01/26 09:52:11]

Ditto.

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

+    GetActivityTimes(request);
+
+  if (local_state_->GetBoolean(prefs::kReportDeviceVersionInfo))

[pastarmovj, 2012/01/26 09:52:11]

Ditto.

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

+    GetVersionInfo(request);
+
+  if (local_state_->GetBoolean(prefs::kReportDeviceBootMode))

[pastarmovj, 2012/01/26 09:52:11]

Ditto.

[Patrick Dubroy, 2012/01/27 15:20:29]

Done.

+    GetBootMode(request);
+}
+
 void DeviceStatusCollector::OnOSVersion(VersionLoader::Handle handle,
                                         std::string version) {
   os_version_ = version;