Allow interfaceClass USB device permissions

Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
Committed: https://crrev.com/2d0dff8d82ef6b4ba129f77f542e02391b7b04fb
Cr-Commit-Position: refs/heads/master@{#428154}

[tbarzic, 2016-10-20 06:38:18]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 06:38:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 06:57:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 06:57:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tbarzic, 2016-10-20 17:52:35]

Description was changed from

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
If interfaceClass is set, allow vendorId and productId to be absent
from the permission. Though, do not allow vendorId to be absent if
productId is set.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

BUG=629223
==========

to

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by the active USB device configuration.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

[tbarzic, 2016-10-20 17:54:06]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 17:54:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 18:12:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 18:12:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbarzic, 2016-10-20 18:19:28]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 18:20:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2016-10-20 19:47:33]

tbarzic@chromium.org changed reviewers:
+ reillyg@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:34:49]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-10-20 23:16:52]

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission.cc:76: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission.cc:80: }
nit: No braces around single-line loops.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_data.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.cc:34: int temp = 0;
nit: It isn't necessary to initialize this variable.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.cc:129: // Reject the
permissoin if neither interface class nor vendor ID, product ID
s/permissoin/permission/

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_data.h (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.h:43: int interface_id,
Since |interface_id| is ignored please make it invalid to create a new
permission entry that contains both |interface_id| and |interface_class|.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.h:68: int&
interface_id() { return interface_id_; }
This field is unused and I've only not gotten rid of it to avoid triggering
"permissions changed" warnings for existing extensions.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_unittest.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_unittest.cc:41: }
nit: No braces around single-line ifs.

[tbarzic, 2016-10-21 00:05:05]

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission.cc:76: }
On 2016/10/20 23:16:52, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission.cc:80: }
On 2016/10/20 23:16:52, Reilly Grant wrote:
> nit: No braces around single-line loops.

Done.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_data.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.cc:34: int temp = 0;
On 2016/10/20 23:16:52, Reilly Grant wrote:
> nit: It isn't necessary to initialize this variable.

Done.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.cc:129: // Reject the
permissoin if neither interface class nor vendor ID, product ID
On 2016/10/20 23:16:52, Reilly Grant wrote:
> s/permissoin/permission/

Done.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_data.h (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.h:43: int interface_id,
On 2016/10/20 23:16:52, Reilly Grant wrote:
> Since |interface_id| is ignored please make it invalid to create a new
> permission entry that contains both |interface_id| and |interface_class|.

Done.

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_data.h:68: int&
interface_id() { return interface_id_; }
On 2016/10/20 23:16:52, Reilly Grant wrote:
> This field is unused and I've only not gotten rid of it to avoid triggering
> "permissions changed" warnings for existing extensions.

OK, I'll also add a comment about that.

Note that it's still taken into consideration when checking the permission (used
with findDevice method). I assume this is not to break existing apps?

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
File extensions/common/permissions/usb_device_permission_unittest.cc (right):

https://codereview.chromium.org/2418353002/diff/80001/extensions/common/permi...
extensions/common/permissions/usb_device_permission_unittest.cc:41: }
On 2016/10/20 23:16:52, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

[Reilly Grant (use Gerrit), 2016-10-25 18:33:08]

https://codereview.chromium.org/2418353002/diff/120001/extensions/common/perm...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/2418353002/diff/120001/extensions/common/perm...
extensions/common/permissions/usb_device_permission.cc:77: if
(device->active_configuration()) {
We probably want to check all of the device's configurations for compatible
interfaces. Other mechanisms will prevent the app from changing it if we don't
want it to.

[tbarzic, 2016-10-25 18:38:14]

Description was changed from

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by the active USB device configuration.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

to

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

[tbarzic, 2016-10-25 18:46:01]

https://codereview.chromium.org/2418353002/diff/120001/extensions/common/perm...
File extensions/common/permissions/usb_device_permission.cc (right):

https://codereview.chromium.org/2418353002/diff/120001/extensions/common/perm...
extensions/common/permissions/usb_device_permission.cc:77: if
(device->active_configuration()) {
On 2016/10/25 18:33:08, Reilly Grant wrote:
> We probably want to check all of the device's configurations for compatible
> interfaces. Other mechanisms will prevent the app from changing it if we don't
> want it to.

Done.

[Reilly Grant (use Gerrit), 2016-10-25 20:44:03]

https://codereview.chromium.org/2418353002/diff/140001/extensions/common/perm...
File extensions/common/permissions/usb_device_permission_unittest.cc (right):

https://codereview.chromium.org/2418353002/diff/140001/extensions/common/perm...
extensions/common/permissions/usb_device_permission_unittest.cc:196:
EXPECT_TRUE(permission_data.Check(param.get()));
Can you add a comment explaining why this is expected to be true? I can see that
it has to do with the implementation of findDevices but it's non-intuitive.

[tbarzic, 2016-10-25 21:17:08]

https://codereview.chromium.org/2418353002/diff/140001/extensions/common/perm...
File extensions/common/permissions/usb_device_permission_unittest.cc (right):

https://codereview.chromium.org/2418353002/diff/140001/extensions/common/perm...
extensions/common/permissions/usb_device_permission_unittest.cc:196:
EXPECT_TRUE(permission_data.Check(param.get()));
On 2016/10/25 20:44:03, Reilly Grant wrote:
> Can you add a comment explaining why this is expected to be true? I can see
that
> it has to do with the implementation of findDevices but it's non-intuitive.

Done.

Added more detailed comment to ForDeviceWithAnyInterfaceClass (since I agree
this is a bit non-intuitive)

[Reilly Grant (use Gerrit), 2016-10-25 22:10:13]

lgtm

[tbarzic, 2016-10-25 22:39:02]

tbarzic@chromium.org changed reviewers:
+ meacer@chromium.org, xiyuan@chromium.org

[tbarzic, 2016-10-25 22:39:03]

adding missing owners
xiyuan: chrome/browser/chromeos
meacer: extension/common/extension_messages.h

[xiyuan, 2016-10-26 17:56:22]

chrome/browser/chromeos LGTM

[meacer, 2016-10-27 00:14:20]

On 2016/10/26 17:56:22, xiyuan wrote:
> chrome/browser/chromeos LGTM

extension/common/extension_messages.h lgtm

[meacer, 2016-10-27 00:14:35]

(forgot to send this comment)

https://codereview.chromium.org/2418353002/diff/160001/extensions/common/perm...
File extensions/common/permissions/usb_device_permission.h (right):

https://codereview.chromium.org/2418353002/diff/160001/extensions/common/perm...
extensions/common/permissions/usb_device_permission.h:55: CheckParam(const
Extension* extesnion,
extesnion -> extension

[tbarzic, 2016-10-27 17:58:30]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 17:59:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 19:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 19:19:28]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2016-10-27 21:29:03]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2016-10-27 21:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
reillyg@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2418353002/#ps180001
(title: ".")

[commit-bot: I haz the power, 2016-10-27 21:29:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 22:09:25]

Description was changed from

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

to

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

[commit-bot: I haz the power, 2016-10-27 22:09:26]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-27 22:11:14]

Description was changed from

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223
==========

to

==========
Allow interfaceClass USB device permissions

Introduces interfaceClass parameter to usbDevice permission.
The parameter is used to match permission to all USB device that
expose an interface with the provided class.
The interfaceClass permission will be matched against all interfaces
supported by any of USB device configurations.

USB device permission with interfaceClass set will only be taken
into account when determining device availability in kiosk sessions.

To support filtering devices that specify (only) device level class
(e.g. hub class is device descriptor class), when testing
interfaceClass permission parameter, take device class into account
(in addition to all interface classes).

Since extracting set of supported interfaces or a device is not as
trivial as getting vendor/product ID, introduce helper factory methods
for UsbDevicePermisison::CheckParam that will create check param for a
USB device. Also, since usbDevice permission is used by hid API too,
add a method for creating check param for HID devices. For those,
set of interface classes will be set to HID interface class: 3.

BUG=629223

Committed: https://crrev.com/2d0dff8d82ef6b4ba129f77f542e02391b7b04fb
Cr-Commit-Position: refs/heads/master@{#428154}
==========

[commit-bot: I haz the power, 2016-10-27 22:11:15]

Patchset 10 (id:??) landed as
https://crrev.com/2d0dff8d82ef6b4ba129f77f542e02391b7b04fb
Cr-Commit-Position: refs/heads/master@{#428154}