bluetooth: Test Web Bluetooth getCharacteristic calls against blacklist.

bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398
Committed: https://crrev.com/c14fbe2c02a55350c75927383c28e9afb660b391
Cr-Commit-Position: refs/heads/master@{#377478}

[scheib, 2016-02-23 21:57:16]

Description was changed from

==========
bluetooth: Test Web Bluetooth getCharacteristic calls against blacklist.

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

[scheib, 2016-02-23 21:57:16]

scheib@chromium.org changed reviewers:
+ ortuno@chromium.org

[scheib, 2016-02-23 22:03:55]

Description was changed from

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398
==========

[ortuno, 2016-02-23 22:37:59]

BTW I'm ok with this patch being only about access characteristics and
read/writting is done in a follow up patch.

https://codereview.chromium.org/1697873002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1697873002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:796: if
(BluetoothBlacklist::Get().IsExcluded(
You probably want to crash the renderer in QueryCacheForCharacteristic if the
UUID is blacklisted.

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(left):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:69:
const char kHeartRateMeasurementUUID[] = "2a37";
Unsure if alphabetical is the right order. I think ordering them by UUIDs makes
it easier to see if a service has it's corresponding characteristics and makes
it easier to see which Characteristic corresponds to which Service:

kHeartRateServiceUUID
kHeartRateMeasurementUUID
kBodySensorLocationUUID

What do you think?

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:65:
const char kDeviceInformationServiceUUID[] = "180A";
Use lower-case i.e. 180a

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:72:
const char kSerialNumberStringUUID[] = "2A25";
Same here use lower-case. 2a25

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:216:
//         - Device Information Service - Characteristics as described in
The services in the device should match the UUIDs above.

https://codereview.chromium.org/1697873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1697873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:64:
MAP_ERROR(GetCharacteristicWithBlacklistedUUID, SecurityError,
"getCharacteristic() called with blacklisted UUID. https://goo.gl/4NeimX");
I would call the error BlacklistedCharacteristicUUID and use
getCharacteristic(s) that way we can use the error for both getCharacteristic()
and getCharacteristics().

[scheib, 2016-02-24 19:51:27]

scheib@chromium.org changed reviewers:
+ isherman@chromium.org

[scheib, 2016-02-24 19:51:30]

isherman, PTAL at histograms.xml 

Thanks ortuno, see below:

https://codereview.chromium.org/1697873002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1697873002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:796: if
(BluetoothBlacklist::Get().IsExcluded(
On 2016/02/23 22:37:58, ortuno wrote:
> You probably want to crash the renderer in QueryCacheForCharacteristic if the
> UUID is blacklisted.

We discussed in person, and concluded that the initial blacklist checks will
result in service and characteristics not being found in respective lists and
also resulting in renderer crashes in QueryCache* methods if a blacklisted
resource is attempted to be accessed.

No code change made.

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(left):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:69:
const char kHeartRateMeasurementUUID[] = "2a37";
On 2016/02/23 22:37:58, ortuno wrote:
> Unsure if alphabetical is the right order. I think ordering them by UUIDs
makes
> it easier to see if a service has it's corresponding characteristics and makes
> it easier to see which Characteristic corresponds to which Service:
> 
> kHeartRateServiceUUID
> kHeartRateMeasurementUUID
> kBodySensorLocationUUID
> 
> What do you think?

Splitting the Services and Characteristics SGTM.

I don't think sorting by UUID value helps much. Doing that sort it mostly just
looks jumbled to me. Here in comment are both, and in code patch I'm uploading
alphabetical by name.

// sorted by name:

// Bluetooth UUIDs suitable to pass to BluetoothUUID(): 
// Services: 
const char kBatteryServiceUUID[] = "180f";
const char kDeviceInformationServiceUUID[] = "180a";
const char kGenericAccessServiceUUID[] = "1800";
const char kGlucoseServiceUUID[] = "1808";
const char kHeartRateServiceUUID[] = "180d";
const char kHumanInterfaceDeviceServiceUUID[] = "1812";
const char kTxPowerServiceUUID[] = "1804";
// Characteristics:
const char kBodySensorLocation[] = "2a38";
const char kDeviceNameUUID[] = "2a00";
const char kHeartRateMeasurementUUID[] = "2a37";
const char kSerialNumberStringUUID[] = "2a25";

// or sorted by UUID:
  
// Services: 
const char kGenericAccessServiceUUID[] = "1800";
const char kTxPowerServiceUUID[] = "1804";
const char kGlucoseServiceUUID[] = "1808";
const char kDeviceInformationServiceUUID[] = "180a";
const char kHeartRateServiceUUID[] = "180d";
const char kBatteryServiceUUID[] = "180f";
const char kHumanInterfaceDeviceServiceUUID[] = "1812";
// Characteristics:
const char kDeviceNameUUID[] = "2a00";
const char kSerialNumberStringUUID[] = "2a25";
const char kHeartRateMeasurementUUID[] = "2a37";
const char kBodySensorLocation[] = "2a38";

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:65:
const char kDeviceInformationServiceUUID[] = "180A";
On 2016/02/23 22:37:58, ortuno wrote:
> Use lower-case i.e. 180a

Done.

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:72:
const char kSerialNumberStringUUID[] = "2A25";
On 2016/02/23 22:37:59, ortuno wrote:
> Same here use lower-case. 2a25

Done.

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1697873002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:216:
//         - Device Information Service - Characteristics as described in
On 2016/02/23 22:37:59, ortuno wrote:
> The services in the device should match the UUIDs above.

Done.

https://codereview.chromium.org/1697873002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1697873002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:64:
MAP_ERROR(GetCharacteristicWithBlacklistedUUID, SecurityError,
"getCharacteristic() called with blacklisted UUID. https://goo.gl/4NeimX");
On 2016/02/23 22:37:59, ortuno wrote:
> I would call the error BlacklistedCharacteristicUUID and use
> getCharacteristic(s) that way we can use the error for both
getCharacteristic()
> and getCharacteristics().

Done.

[Ilya Sherman, 2016-02-24 22:31:15]

histograms.xml lgtm

[ortuno, 2016-02-24 23:08:17]

lgtm

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:723:
EXPECT_CALL(*serial_number_string, ReadRemoteCharacteristic(_, _)).Times(0);
I think you should just CHECK. I've had cases in which EXPECT just logs some
stuff but doesn't actually fail a test.

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:221:
//         - Device Information Service - Characteristics as described in
Ah. I ordered these by UUID, but it looks alphabetical. Could you fix
GlucoseDevice to be in alphabetical order? Or order by UUID ;)

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html
(right):

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html:16:
optionalServices: ['device_information', 'human_interface_device']
I think you can get away with requesting only device_information. I don't think
you use the other ones.

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html:24:
'https://goo.gl/4NeimX',
nit: I think the "'"s in these two lines should line up.

[scheib, 2016-02-24 23:38:40]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-02-24 23:38:41]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1697873002/#ps60001
(title: "addressed ortuno")

[scheib, 2016-02-24 23:38:52]

Thanks

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:723:
EXPECT_CALL(*serial_number_string, ReadRemoteCharacteristic(_, _)).Times(0);
On 2016/02/24 23:08:17, ortuno wrote:
> I think you should just CHECK. I've had cases in which EXPECT just logs some
> stuff but doesn't actually fail a test.

Done.

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1697873002/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:221:
//         - Device Information Service - Characteristics as described in
On 2016/02/24 23:08:17, ortuno wrote:
> Ah. I ordered these by UUID, but it looks alphabetical. Could you fix
> GlucoseDevice to be in alphabetical order? Or order by UUID ;)

Done.

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html
(right):

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html:16:
optionalServices: ['device_information', 'human_interface_device']
On 2016/02/24 23:08:17, ortuno wrote:
> I think you can get away with requesting only device_information. I don't
think
> you use the other ones.

Done.

https://codereview.chromium.org/1697873002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getCharacteristic-blacklist.html:24:
'https://goo.gl/4NeimX',
On 2016/02/24 23:08:17, ortuno wrote:
> nit: I think the "'"s in these two lines should line up.

Done.

[scheib, 2016-02-24 23:40:59]

The CQ bit was unchecked by scheib@chromium.org

[commit-bot: I haz the power, 2016-02-24 23:41:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1697873002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1697873002/60001

[scheib, 2016-02-24 23:42:52]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-02-24 23:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1697873002/#ps80001
(title: "move glucose UUID in code as well")

[commit-bot: I haz the power, 2016-02-24 23:45:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1697873002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1697873002/80001

[commit-bot: I haz the power, 2016-02-25 02:57:50]

Description was changed from

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398
==========

[commit-bot: I haz the power, 2016-02-25 02:57:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-25 02:59:20]

Description was changed from

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to getCharacteristic.

Disallow access to blacklisted characteristic UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

Test data specification will be updated to match this change.
Issue https://github.com/WebBluetoothCG/web-bluetooth/issues/214
has been updated with changes in this patch.

BUG=584398

Committed: https://crrev.com/c14fbe2c02a55350c75927383c28e9afb660b391
Cr-Commit-Position: refs/heads/master@{#377478}
==========

[commit-bot: I haz the power, 2016-02-25 02:59:21]

Patchset 5 (id:??) landed as
https://crrev.com/c14fbe2c02a55350c75927383c28e9afb660b391
Cr-Commit-Position: refs/heads/master@{#377478}