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

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

BUG=chromium-os:24308
TEST=Modified DeviceStatusCollectorTest in unit_tests to check that
portions of the status are only included when the appropriate pref is
enabled.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119470

[Patrick Dubroy, 2012-01-25 17:25:02]

Joao, another review for you. :-)

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

Looks good to me. Adding Julian as reviewer for his comments on reading the
device policy values from local_state.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:14: #include
"chrome/common/pref_names.h"
Nit: alphabetic order

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/proto/chr...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/proto/chr...
chrome/browser/policy/proto/chrome_device_policy.proto:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Nit: update copyright year

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

Hey Patrick,
I made a drive-by review on this CL as I think there are some issues with the
way you read the value of the policy. We can discus this further offline but
I'll be glad to have this changed to maintain the invariant that policies should
be trusted on a higher level than the local_state is.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:73:
local_state->RegisterBooleanPref(prefs::kReportDeviceVersionInfo, false);
You won't need this once you regard my next comment on line 122.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:122: if
(!local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))
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.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:189: if
(local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))
Ditto.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:192: if
(local_state_->GetBoolean(prefs::kReportDeviceVersionInfo))
Ditto.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:195: if
(local_state_->GetBoolean(prefs::kReportDeviceBootMode))
Ditto.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector_unittest.cc:121:
prefs_.SetBoolean(prefs::kReportDeviceActivityTimes, true);
Those tests will then need some more mocking to work once you go through the
CrosSettings i-face.

http://codereview.chromium.org/9289017/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1612: const char kReportDeviceBootMode[] =
"device_status.report_boot_mode";
Those pref names will have to go to the cros_settings_names.* files and be
prefixed with "cros.".

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

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:14: #include
"chrome/common/pref_names.h"
On 2012/01/26 09:48:58, Joao da Silva wrote:
> Nit: alphabetic order

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:73:
local_state->RegisterBooleanPref(prefs::kReportDeviceVersionInfo, false);
On 2012/01/26 09:52:11, pastarmovj wrote:
> You won't need this once you regard my next comment on line 122.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:122: if
(!local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))
On 2012/01/26 09:52:11, pastarmovj wrote:
> 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.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:189: if
(local_state_->GetBoolean(prefs::kReportDeviceActivityTimes))
On 2012/01/26 09:52:11, pastarmovj wrote:
> Ditto.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:192: if
(local_state_->GetBoolean(prefs::kReportDeviceVersionInfo))
On 2012/01/26 09:52:11, pastarmovj wrote:
> Ditto.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector.cc:195: if
(local_state_->GetBoolean(prefs::kReportDeviceBootMode))
On 2012/01/26 09:52:11, pastarmovj wrote:
> Ditto.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/device_st...
chrome/browser/policy/device_status_collector_unittest.cc:121:
prefs_.SetBoolean(prefs::kReportDeviceActivityTimes, true);
On 2012/01/26 09:52:11, pastarmovj wrote:
> Those tests will then need some more mocking to work once you go through the
> CrosSettings i-face.

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/proto/chr...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/9289017/diff/1/chrome/browser/policy/proto/chr...
chrome/browser/policy/proto/chrome_device_policy.proto:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/01/26 09:48:58, Joao da Silva wrote:
> Nit: update copyright year

Done.

http://codereview.chromium.org/9289017/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/9289017/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1612: const char kReportDeviceBootMode[] =
"device_status.report_boot_mode";
On 2012/01/26 09:52:11, pastarmovj wrote:
> Those pref names will have to go to the cros_settings_names.* files and be
> prefixed with "cros.".

Done.

[pastarmovj, 2012-01-27 15:39:03]

LGTM with a few comments that I'd love to be addressed before you commit.

Thanks for taking the time to make those changes on such a tight schedule! :)

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:286: } else {
Please put a TODO here at least that those three policies are missing for now in
the NOTREACHED clause if we ever try to set them this should help the person
debugging why it doesn't work know what's wrong.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/stub...
File chrome/browser/chromeos/stub_cros_settings_provider.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/stub...
chrome/browser/chromeos/stub_cros_settings_provider.cc:83:
values_.SetBoolean(kReportDeviceVersionInfo, false);
Are you sure you need those defaults here. In a real world scenario the policy
fields will not be set until the admin changes them on the server. I don't mind
having those here but you should be sure to work correctly if those are actually
not present in the policy blob.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:10: #include
"chrome/browser/chromeos/cros/cros_library.h"
What do you need the cros_library for?

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:19: #include
"chrome/common/pref_names.h"
ditto.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:252: if (type ==
chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED) {
Curlies not needed?

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:17: #include
"chrome/common/pref_names.h"
Do you need this one?

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:297: // Test that boot
mode data is not report if the pref is not turned on.
s/report/reported/ ?

[Patrick Dubroy, 2012-01-27 16:18:12]

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_settings_provider.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_settings_provider.cc:286: } else {
On 2012/01/27 15:39:03, pastarmovj wrote:
> Please put a TODO here at least that those three policies are missing for now
in
> the NOTREACHED clause if we ever try to set them this should help the person
> debugging why it doesn't work know what's wrong.

Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/stub...
File chrome/browser/chromeos/stub_cros_settings_provider.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/chromeos/stub...
chrome/browser/chromeos/stub_cros_settings_provider.cc:83:
values_.SetBoolean(kReportDeviceVersionInfo, false);
On 2012/01/27 15:39:03, pastarmovj wrote:
> Are you sure you need those defaults here. In a real world scenario the policy
> fields will not be set until the admin changes them on the server. I don't
mind
> having those here but you should be sure to work correctly if those are
actually
> not present in the policy blob.

No, you're right. Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:10: #include
"chrome/browser/chromeos/cros/cros_library.h"
On 2012/01/27 15:39:03, pastarmovj wrote:
> What do you need the cros_library for?

Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:19: #include
"chrome/common/pref_names.h"
On 2012/01/27 15:39:03, pastarmovj wrote:
> ditto.

Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector.cc:252: if (type ==
chrome::NOTIFICATION_SYSTEM_SETTING_CHANGED) {
On 2012/01/27 15:39:03, pastarmovj wrote:
> Curlies not needed?

Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_status_collector_unittest.cc (right):

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:17: #include
"chrome/common/pref_names.h"
On 2012/01/27 15:39:03, pastarmovj wrote:
> Do you need this one?

Done.

http://codereview.chromium.org/9289017/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_status_collector_unittest.cc:297: // Test that boot
mode data is not report if the pref is not turned on.
On 2012/01/27 15:39:03, pastarmovj wrote:
> s/report/reported/ ?

Done.

[commit-bot: I haz the power, 2012-01-27 16:49:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/9289017/8002

[commit-bot: I haz the power, 2012-01-27 18:53:22]

Change committed as 119470