If requested, report network interfaces to management server.

If requested, report network interfaces to management server.

This adds a device policy that allows the administrator to request
Chrome OS devices to report a list of their network interfaces and
corresponding device hardware addresses to the device management server
as part of device status reporting.

BUG=chromium:218410
TEST=unit test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211432

[Mattias Nissler (ping if slow), 2013-07-04 10:53:30]

Please review.

pneubeck: Main reviewer
pastarmovj: chrome/browser/chromeos/settings OWNERS approval
gspencer: chromeos/network OWNERS approval
jiachen: Please take a look at chrome/browser/policy/proto and tell me if you
have any objections/input

[pneubeck (no reviews), 2013-07-04 15:51:48]

mostly nits.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:424:
chromeos::NetworkStateHandler::DeviceStateList::const_iterator device;
nit:
alternatively, typedef chromeos::NetworkStateHandler::DeviceStateList locally
and declare 'device' inside the 'for'.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:428: continue;
in this case, an empty NetworkInterface would have been added.
Is that useful?

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:601:
"112233445566", NULL },
nit: maybe test also a device without mac_address

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:623:
base::StringValue value(dev.mac_address);
nit: no need for local variable declaration |value|

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:628:
base::StringValue value(dev.meid);
ditto

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:660: if
(dev.mac_address)
you're testing the logic in the implementation with the same logic reimplemented
in the test.

wouldn't it have been an alternative to serialize the protobuf message to a JSON
string and compare that to an expected string?

that could also be used in other tests of this kind.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings_names.cc:81: // Determines
whether the device reports network interface types and address in
typeS and address[]

inconsistent cardinality.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:638: if
(policy.has_device_reporting()) {
nit:
could be negated and early return.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:639: if
(policy.device_reporting().has_report_version_info()) {
nit:
and policy.device_reporting() could be assigned to a local variable.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
File chrome/browser/policy/proto/cloud/device_management_backend.proto (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:364: // String
describing the interface type.
This should refer to the list of possible strings.

Description of the respective Shill property: shill/doc/device-api.txt

List of constants: third_party/cros_system_api/dbus/service_constants.h

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:368: optional
string mac_address = 2;
encoding of the MAC address?

https://codereview.chromium.org/18348016/diff/3001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:1856:
"ReportDeviceNetworkInterfaces": {
I have no idea what this file is about.

[pastarmovj, 2013-07-05 08:44:27]

Agree with Philipp for the nits but otherwise owner lgtm for settings.

[Greg Spencer (Chromium), 2013-07-08 21:44:17]

LGTM modulo nits (mine and pneubeck's)

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:656: // if
(policy.device_reporting().has_report_location()) {
nit: I know you didn't add it here, but commented out code is not typically left
in.

[Mattias Nissler (ping if slow), 2013-07-10 09:40:27]

Bin, Jiayun, any thoughts?

[Bin, 2013-07-10 18:02:49]

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
File chrome/browser/policy/proto/cloud/device_management_backend.proto (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:365: optional
string type = 1;
Once we start collecting network interface address, we will show them on CPanel,
and maybe add a function to export them. We need to document the possible values
for the type.

The type will be Flimflam types defined below? If yes, can we document that in
the proto? Ideally, the type should be an enum. But if they are taken from the
type constants below, then I am fine with that too.

// Flimflam type options.
const char kTypeEthernet[] = "ethernet";
const char kTypeWifi[] = "wifi";
const char kTypeWimax[] = "wimax";
const char kTypeBluetooth[] = "bluetooth";
const char kTypeCellular[] = "cellular";
const char kTypeVPN[] = "vpn";

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:367: // MAC
address (if applicable) of the corresponding network device.
There are three MAC address formats:

MM:MM:MM:SS:SS:SS
MM-MM-MM-SS-SS-SS
MMM.MMM.SSS.SSS

We are using the first one? Can we document that?

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:371: optional
string meid = 3;
If I remember correctly, MEID is used for CDMA devices and IMEI is used for GSM
devices. Are all 3G Chrome devices come with CDMA only and will be that case in
the future too? Or should we rename it to mobile identifier to cover both cases?

[Mattias Nissler (ping if slow), 2013-07-11 17:41:46]

All feedback addressed, PTAL!

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:424:
chromeos::NetworkStateHandler::DeviceStateList::const_iterator device;
On 2013/07/04 15:51:48, pneubeck wrote:
> nit:
> alternatively, typedef chromeos::NetworkStateHandler::DeviceStateList locally
> and declare 'device' inside the 'for'.

I only need the type once, and I'd have to break the loop declaration line, so
no.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:428: continue;
On 2013/07/04 15:51:48, pneubeck wrote:
> in this case, an empty NetworkInterface would have been added.
> Is that useful?

I don't think so, hence I skip this case. The updated code now filters for types
that are actually relevant. In particular, VPN devices don't really make sense
to be reported to the server as they don't have hardware addresses.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:601:
"112233445566", NULL },
On 2013/07/04 15:51:48, pneubeck wrote:
> nit: maybe test also a device without mac_address

Done.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:623:
base::StringValue value(dev.mac_address);
On 2013/07/04 15:51:48, pneubeck wrote:
> nit: no need for local variable declaration |value|

Done.

Clang used to complain in these cases. That was because of the copy constructor
required to pass the temporary not existing, which is no longer the case.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:628:
base::StringValue value(dev.meid);
On 2013/07/04 15:51:48, pneubeck wrote:
> ditto

Done.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:660: if
(dev.mac_address)
On 2013/07/04 15:51:48, pneubeck wrote:
> you're testing the logic in the implementation with the same logic
reimplemented
> in the test.
> 
> wouldn't it have been an alternative to serialize the protobuf message to a
JSON
> string and compare that to an expected string?
> 
> that could also be used in other tests of this kind.

That doesn't make a difference, I'd just be swapping one format of specifying
test expectations for another without gaining any test coverage quality.
Ideally, I'd test this against what shill gives me, but that'd require an
integration test with Chrome OS that can't be done as browser_test.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings_names.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings_names.cc:81: // Determines
whether the device reports network interface types and address in
On 2013/07/04 15:51:48, pneubeck wrote:
> typeS and address[]
> 
> inconsistent cardinality.

Done.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:638: if
(policy.has_device_reporting()) {
On 2013/07/04 15:51:48, pneubeck wrote:
> nit:
> could be negated and early return.

That would make it inconsistent with the pattern all other functions in this
file use, and that have more than one top-level conditional. I instead fixed the
AU decoding function to comply with the dominant style.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:639: if
(policy.device_reporting().has_report_version_info()) {
On 2013/07/04 15:51:48, pneubeck wrote:
> nit:
> and policy.device_reporting() could be assigned to a local variable.

Done. Kept the formatting for consistency (the new block needs two line breaks).

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_provider.cc:656: // if
(policy.device_reporting().has_report_location()) {
On 2013/07/08 21:44:18, Greg Spencer (Chromium) wrote:
> nit: I know you didn't add it here, but commented out code is not typically
left
> in.

Removed.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
File chrome/browser/policy/proto/cloud/device_management_backend.proto (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:364: // String
describing the interface type.
On 2013/07/04 15:51:48, pneubeck wrote:
> This should refer to the list of possible strings.
> 
> Description of the respective Shill property: shill/doc/device-api.txt
> 
> List of constants: third_party/cros_system_api/dbus/service_constants.h

Made an enum and translation table instead.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:365: optional
string type = 1;
On 2013/07/10 18:02:49, Bin wrote:
> Once we start collecting network interface address, we will show them on
CPanel,
> and maybe add a function to export them. We need to document the possible
values
> for the type.
> 
> The type will be Flimflam types defined below? If yes, can we document that in
> the proto? Ideally, the type should be an enum. But if they are taken from the
> type constants below, then I am fine with that too.
> 
> // Flimflam type options.
> const char kTypeEthernet[] = "ethernet";
> const char kTypeWifi[] = "wifi";
> const char kTypeWimax[] = "wimax";
> const char kTypeBluetooth[] = "bluetooth";
> const char kTypeCellular[] = "cellular";
> const char kTypeVPN[] = "vpn";

Made an enum.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:367: // MAC
address (if applicable) of the corresponding network device.
On 2013/07/10 18:02:49, Bin wrote:
> There are three MAC address formats:
> 
> MM:MM:MM:SS:SS:SS
> MM-MM-MM-SS-SS-SS
> MMM.MMM.SSS.SSS
> 
> We are using the first one? Can we document that?

Done.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:368: optional
string mac_address = 2;
On 2013/07/04 15:51:48, pneubeck wrote:
> encoding of the MAC address?

Done.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/policy/prot...
chrome/browser/policy/proto/cloud/device_management_backend.proto:371: optional
string meid = 3;
On 2013/07/10 18:02:49, Bin wrote:
> If I remember correctly, MEID is used for CDMA devices and IMEI is used for
GSM
> devices. Are all 3G Chrome devices come with CDMA only and will be that case
in
> the future too? Or should we rename it to mobile identifier to cover both
cases?

I added a separate field for IMEI, so you have whatever identifier is applicable
and also know which one it is.

Caveat: These IDs come directly from the connection manager, and I can't confirm
the format for all Chrome devices. Greg, do you know whether these are in
standard format?

https://codereview.chromium.org/18348016/diff/3001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:1856:
"ReportDeviceNetworkInterfaces": {
On 2013/07/04 15:51:48, pneubeck wrote:
> I have no idea what this file is about.

pref mapping / chrome://settings UI test cases, not applicable for most device
policies.

[pneubeck (no reviews), 2013-07-11 18:49:31]

lgtm

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:424:
chromeos::NetworkStateHandler::DeviceStateList::const_iterator device;
On 2013/07/11 17:41:46, Mattias Nissler wrote:
> On 2013/07/04 15:51:48, pneubeck wrote:
> > nit:
> > alternatively, typedef chromeos::NetworkStateHandler::DeviceStateList
locally
> > and declare 'device' inside the 'for'.
> 
> I only need the type once, and I'd have to break the loop declaration line, so
> no.
FWIW, chromeos::NetworkStateHandler::DeviceStateList is used twice, not once.

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_status_collector.cc:415: // Maps flimflam
device type strings to proto enum constants.
nit: although the namespace is still 'flimflam', we may refer to Shill's device
types instead.

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_status_collector.cc:419: }
kDeviceTypeMap[] = {
nit: could be an array of _const_ entries.

[pneubeck (no reviews), 2013-07-12 07:57:54]

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_status_collector.cc:419: }
kDeviceTypeMap[] = {
On 2013/07/11 18:49:32, pneubeck wrote:
> nit: could be an array of _const_ entries.

would be in accordance with this thread (they discussed const char* list[] vs.
const char* const list[])
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/uninit...

[Mattias Nissler (ping if slow), 2013-07-12 08:11:55]

Addressed pneubeck's latest comments.

Bin, can you take another look, preferably today? I'll be on vacation next week,
and would like to get this in before I leave if possible.

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_status_collector.cc:424:
chromeos::NetworkStateHandler::DeviceStateList::const_iterator device;
On 2013/07/11 18:49:31, pneubeck wrote:
> On 2013/07/11 17:41:46, Mattias Nissler wrote:
> > On 2013/07/04 15:51:48, pneubeck wrote:
> > > nit:
> > > alternatively, typedef chromeos::NetworkStateHandler::DeviceStateList
> locally
> > > and declare 'device' inside the 'for'.
> > 
> > I only need the type once, and I'd have to break the loop declaration line,
so
> > no.
> FWIW, chromeos::NetworkStateHandler::DeviceStateList is used twice, not once.

Oh right, I misread your comment to typedef the iterator. In any case, this
version is more readable than the typedef version IMHO.

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/18348016/diff/17001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_status_collector.cc:419: }
kDeviceTypeMap[] = {
On 2013/07/12 07:57:54, pneubeck wrote:
> On 2013/07/11 18:49:32, pneubeck wrote:
> > nit: could be an array of _const_ entries.
> 
> would be in accordance with this thread (they discussed const char* list[] vs.
> const char* const list[])
>
https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/unin...

Valid point, done.

[Bin, 2013-07-12 16:54:02]

lgtm

[commit-bot: I haz the power, 2013-07-12 17:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/18348016/36001

[commit-bot: I haz the power, 2013-07-12 18:50:19]

Change committed as 211432