bluetooth: Return specific error if getPrimaryServices() is called without requesting access to any…

bluetooth: Return specific error if getPrimaryServices() is called without requesting access to any UUIDs.

BUG=656458
Committed: https://crrev.com/08303d660862c7d0d4f9deb7cb753d3dc68570dc
Cr-Commit-Position: refs/heads/master@{#432186}

[fbeaufortchromium, 2016-10-27 12:25:27]

Patchset #3 (id:40001) has been deleted

[fbeaufortchromium, 2016-10-27 12:25:34]

Patchset #2 (id:20001) has been deleted

[fbeaufortchromium, 2016-10-27 12:27:14]

Patchset #1 (id:1) has been deleted

[fbeaufortchromium, 2016-10-27 13:31:20]

Patchset #1 (id:60001) has been deleted

[fbeaufortchromium, 2016-10-28 06:56:08]

Patchset #1 (id:80001) has been deleted

[François Beaufort, 2016-10-28 12:41:00]

The CQ bit was checked by beaufort.francois@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 12:41:09]

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-28 12:41:10]

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

[commit-bot: I haz the power, 2016-10-28 12:41:10]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[François Beaufort, 2016-10-28 12:41:51]

beaufort.francois@gmail.com changed reviewers:
+ ortuno@chromium.org

[François Beaufort, 2016-10-28 12:41:51]

Hey Gio,

As it has beaten many developers, here's the patch that adds a specific error if
getPrimaryServices() is called without requesting access to any services.

[François Beaufort, 2016-10-28 12:44:26]

The CQ bit was checked by beaufort.francois@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 12:44:38]

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-28 12:44:42]

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

[commit-bot: I haz the power, 2016-10-28 12:44:42]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[fbeaufortchromium, 2016-10-28 14:04:33]

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

[commit-bot: I haz the power, 2016-10-28 14:04:47]

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-28 15:38:07]

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

[commit-bot: I haz the power, 2016-10-28 15:38:08]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-10-31 00:31:16]

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/delayed-discovery-no-permission-present-service.html
(right):

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/delayed-discovery-no-permission-present-service.html:10:
filters: [{name: 'Heart Rate Device'}]}))
The purpose of this test is to make sure we don't return services for which
users don't have permission. Basically we want to make sure that
IsOriginAllowedToAccessService in [1] is being run. Please add 'battery_service'
as an optional service and add the following to the description: "... request
for present service without permission for that service."

Also please add a separate test for this new error.

[1]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission-present-service.html
(right):

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission-present-service.html:14:
new DOMException('Origin is not allowed to access any service. Tip: ' +
See comment in delayed-discovery-no-permission-present-service.html

[fbeaufortchromium, 2016-11-02 15:36:50]

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

[commit-bot: I haz the power, 2016-11-02 15:37:07]

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

[François Beaufort, 2016-11-02 16:46:17]

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/delayed-discovery-no-permission-present-service.html
(right):

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/delayed-discovery-no-permission-present-service.html:10:
filters: [{name: 'Heart Rate Device'}]}))
On 2016/10/31 00:31:16, ortuno wrote:
> The purpose of this test is to make sure we don't return services for which
> users don't have permission. Basically we want to make sure that
> IsOriginAllowedToAccessService in [1] is being run. Please add
'battery_service'
> as an optional service and add the following to the description: "... request
> for present service without permission for that service."
> 
> Also please add a separate test for this new error.
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

Done.

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission-present-service.html
(right):

https://codereview.chromium.org/2459523002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission-present-service.html:14:
new DOMException('Origin is not allowed to access any service. Tip: ' +
On 2016/10/31 00:31:16, ortuno wrote:
> See comment in delayed-discovery-no-permission-present-service.html

Done.

[commit-bot: I haz the power, 2016-11-02 16:50:00]

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

[commit-bot: I haz the power, 2016-11-02 16:50:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[François Beaufort, 2016-11-02 19:58:21]

Patchset #2 (id:120001) has been deleted

[fbeaufortchromium, 2016-11-02 20:00:20]

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

[commit-bot: I haz the power, 2016-11-02 20:00:42]

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-11-02 22:55:09]

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

[commit-bot: I haz the power, 2016-11-02 22:55:10]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-03 05:30:26]

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html
(right):

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html:14:
new DOMException('Origin is not allowed to access any service. Tip: ' +
Could you use the new script-tests pattern?

You will need to create two new files service-no-permission-for-any-service.js
and service-delayed-discovery-no-permission-for-any-service.js in
bluetooth/scrip-tests/

and instead of gattServer.getPrimaryServices() you would have:

gattServer.CALLS([
  getPrimaryService('heart_rate')|
  getPrimaryServices()|
  getPrimaryServices('heart_rate')]),

Then running third_party/WebKit/LayoutTests/bluetooth/generate.py should
generate six new tests.

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html:18:
}, 'Request for present service without permission. Reject with
SecurityError.');
Please make the description more specific so that it's easier to understand how
this is different from the other test.

[fbeaufortchromium, 2016-11-03 11:01:20]

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

[commit-bot: I haz the power, 2016-11-03 11:01:36]

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

[François Beaufort, 2016-11-03 13:00:02]

Here it is!

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html
(right):

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html:14:
new DOMException('Origin is not allowed to access any service. Tip: ' +
On 2016/11/03 05:30:26, ortuno wrote:
> Could you use the new script-tests pattern?
> 
> You will need to create two new files service-no-permission-for-any-service.js
> and service-delayed-discovery-no-permission-for-any-service.js in
> bluetooth/scrip-tests/
> 
> and instead of gattServer.getPrimaryServices() you would have:
> 
> gattServer.CALLS([
>   getPrimaryService('heart_rate')|
>   getPrimaryServices()|
>   getPrimaryServices('heart_rate')]),
> 
> Then running third_party/WebKit/LayoutTests/bluetooth/generate.py should
> generate six new tests.

I had to use [UUID] to generate 6 tests (not 4).

Done.

https://codereview.chromium.org/2459523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/getPrimaryServices/no-permission.html:18:
}, 'Request for present service without permission. Reject with
SecurityError.');
On 2016/11/03 05:30:26, ortuno wrote:
> Please make the description more specific so that it's easier to understand
how
> this is different from the other test.

Done.

[commit-bot: I haz the power, 2016-11-03 13:26:28]

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

[commit-bot: I haz the power, 2016-11-03 13:26:29]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-03 21:31:21]

https://codereview.chromium.org/2459523002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right):

https://codereview.chromium.org/2459523002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.h:64: bool
IsOriginAllowedToAccessAtLeastOneService(
Ah sorry for not noticing earlier. Please add tests to
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_allo...

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js:16:
}, 'Delayed service discovery, request for present service without permission to
access any service. Reject with SecurityError.');
nit: Please break line.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js:16:
}, 'Delayed service discovery, request for present service without permission to
access any service. Reject with SecurityError.');
nit: Please break line.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:9:
getPrimaryService('heart_rate')[UUID]|
no need for [UUID] here.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:16:
}, 'Request for present service without permission to access any service. Reject
with SecurityError.');
nit: please break line.

[François Beaufort, 2016-11-04 09:57:27]

Adding new tests helped as there was a crash. It is now fixed. Thanks Gio!

https://codereview.chromium.org/2459523002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right):

https://codereview.chromium.org/2459523002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.h:64: bool
IsOriginAllowedToAccessAtLeastOneService(
On 2016/11/03 21:31:21, ortuno wrote:
> Ah sorry for not noticing earlier. Please add tests to
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_allo...

Done.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js:16:
}, 'Delayed service discovery, request for present service without permission to
access any service. Reject with SecurityError.');
On 2016/11/03 21:31:21, ortuno wrote:
> nit: Please break line.

Done.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:9:
getPrimaryService('heart_rate')[UUID]|
On 2016/11/03 21:31:21, ortuno wrote:
> no need for [UUID] here.

Done.

https://codereview.chromium.org/2459523002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:16:
}, 'Request for present service without permission to access any service. Reject
with SecurityError.');
On 2016/11/03 21:31:21, ortuno wrote:
> nit: please break line.

Done.

[fbeaufortchromium, 2016-11-04 09:58:42]

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

[commit-bot: I haz the power, 2016-11-04 09:58:53]

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-11-04 11:20:58]

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

[commit-bot: I haz the power, 2016-11-04 11:20:59]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-06 22:26:00]

lgtm bar some indentation issues

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js:12:
new DOMException('Origin is not allowed to access any service. Tip: ' +
nit: I think you want to align this with gattServer.

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:12:
new DOMException('Origin is not allowed to access any service. Tip: ' +
nit: I think you want to line up this with gattServer

[François Beaufort, 2016-11-07 09:35:47]

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-delayed-discovery-no-permission-for-any-service.js:12:
new DOMException('Origin is not allowed to access any service. Tip: ' +
On 2016/11/06 22:25:59, ortuno wrote:
> nit: I think you want to align this with gattServer.

Done.

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js
(right):

https://codereview.chromium.org/2459523002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/service-no-permission-for-any-service.js:12:
new DOMException('Origin is not allowed to access any service. Tip: ' +
On 2016/11/06 22:26:00, ortuno wrote:
> nit: I think you want to line up this with gattServer

Done.

[François Beaufort, 2016-11-07 09:47:25]

beaufort.francois@gmail.com changed reviewers:
+ cpalmer@chromium.org

[François Beaufort, 2016-11-07 09:47:26]

Hello Chris,

Do you mind reviewing changes in:

third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

Thank you in advance,
Francois.

[François Beaufort, 2016-11-11 07:31:22]

beaufort.francois@gmail.com changed reviewers:
+ dcheng@chromium.org
- cpalmer@chromium.org

[François Beaufort, 2016-11-11 07:31:23]

dcheng@chromium.org: Please review changes in
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

Thank you!

[dcheng, 2016-11-14 22:42:08]

mojom lgtm but please fix the style guide violations (I know there's existing
violations, those should be fixed in a followup as well)

https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:22: const
std::string kDeviceName = "foo";
This violates the style guide:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[ortuno, 2016-11-14 22:57:27]

https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:22: const
std::string kDeviceName = "foo";
nit: Please use a better name than "foo" e.g. "TestName"

[François Beaufort, 2016-11-15 14:00:24]

On 2016/11/14 22:57:27, ortuno wrote:
>
https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
> File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2459523002/diff/200001/content/browser/blueto...
> content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:22: const
> std::string kDeviceName = "foo";
> nit: Please use a better name than "foo" e.g. "TestName"

"foo" has been replaced by "TestName".
Thank you for catching this.

[François Beaufort, 2016-11-15 14:00:35]

The CQ bit was checked by beaufort.francois@gmail.com

[François Beaufort, 2016-11-15 14:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2459523002/#ps220001
(title: "s/foo/TestName/")

[commit-bot: I haz the power, 2016-11-15 14:00:55]

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-11-15 15:53:49]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2016-11-15 15:56:13]

Description was changed from

==========
bluetooth: Return specific error if getPrimaryServices() is called without
requesting access to any UUIDs.

BUG=656458
==========

to

==========
bluetooth: Return specific error if getPrimaryServices() is called without
requesting access to any UUIDs.

BUG=656458

Committed: https://crrev.com/08303d660862c7d0d4f9deb7cb753d3dc68570dc
Cr-Commit-Position: refs/heads/master@{#432186}
==========

[commit-bot: I haz the power, 2016-11-15 15:56:15]

Patchset 6 (id:??) landed as
https://crrev.com/08303d660862c7d0d4f9deb7cb753d3dc68570dc
Cr-Commit-Position: refs/heads/master@{#432186}