Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706503002:
Add enterprise policy to turn off Bluetooth. (Closed)
|
Created:
4 years, 10 months ago by Jeffrey Yasskin Modified:
4 years, 10 months ago Reviewers:
bartfab (slow), Andrew T Wilson (Slow), ortuno, jam, Thiemo Nagel, Alexei Svitkine (slow), raymes CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, jam, jochen+watch_chromium.org, juncai, kcarattini, markusheintz_, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, mlamouri+watch-permissions_chromium.org, msramek+watch_chromium.org, ortuno+watch_chromium.org, Peter Beverloo, raymes+watch_chromium.org, scheib+watch_chromium.org, tnagel+watch_chromium.org, raymes, mlamouri (slow - plz ping), Reilly Grant (use Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd enterprise policy to turn off Web Bluetooth.
Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser.
BUG=562592
TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch.
1) Run chrome with the --enable-web-bluetooth flag.
2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html
3) Type "Test" into the "Device Name Prefix" text box (the third).
4) Press the "Get Bluetooth Device Info" button.
Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it.
Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally."
Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it.
Committed: https://crrev.com/98bdd399e51a63f104421344e79a1e838d00fcdc
Cr-Commit-Position: refs/heads/master@{#377959}
Patch Set 1 : Sync #Patch Set 2 : Rename the Bluetooth content setting to "Bluetooth Guard" #Patch Set 3 : Sync #Patch Set 4 : Fix aw_permission_manager.cc #Patch Set 5 : Load the policy on all OSes #Patch Set 6 : Remove the DefaultWebBluetoothGuardSetting indicator_tests. #Patch Set 7 : Sync #
Total comments: 9
Patch Set 8 : Improve the bluetooth policy documentation. #Patch Set 9 : Rewrite to avoid content settings and permissions, and add a 2-part test #Patch Set 10 : Remove the pref_mapping from policy_test_cases.json. #Patch Set 11 : Release the BrowserMessageFilter::Internal once it has attached the Sender. #
Total comments: 13
Patch Set 12 : Read the policy through the ContentSettings system. #Patch Set 13 : Switch to a unified browsertest #
Total comments: 15
Patch Set 14 : Fix nits #Messages
Total messages: 82 (40 generated)
Description was changed from ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=TODO ========== to ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog with a "Cancel" and a grayed-out "Pair" button should appear. Click Cancel or click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. ==========
Description was changed from ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog with a "Cancel" and a grayed-out "Pair" button should appear. Click Cancel or click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. ========== to ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
Description was changed from ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS, Android, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
Description was changed from ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS, Android, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/60001
Description was changed from ========== Add enterprise policy to turn off Bluetooth. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
jyasskin@chromium.org changed reviewers: + mlamouri@chromium.org, raymes@chromium.org, reillyg@chromium.org
Hey permission folks, I need to add an enterprise policy to control Web Bluetooth, and going through the existing PermissionManager seems like the easiest way to do that. As a bonus, this gives us immediate access to Kendra's kill switch, which we'd otherwise need to re-invent. However, as discussed in https://codereview.chromium.org/1560263002/, we may or may not want a separate ChooserPermissionManager in the long run. I don't want to block the EF launch in M50 on figuring out the long-term strategy. What do you think of this temporary solution? Do you see any better ways to do it? This is also related to https://codereview.chromium.org/1427433017. I'm using an explicit "_GUARD" suffix on the block/ask content setting, to make it clear that this isn't the primary data storage for the chooser. I'm also adding the chooser-data CONTENT_SETTINGS_TYPE at the same time, even though it's unused, to keep it adjacent to the guard enum value. I could leave that until it's used, if you prefer. Thoughts? Jeffrey
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/80001
Description was changed from ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings: 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support. If it doesn't, you'll get a "" error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
Description was changed from ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support. If it doesn't, you'll get a "" error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support. If it doesn't, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ==========
Description was changed from ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support. If it doesn't, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ==========
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Jeffrey, I'll take a look. My sense for a longer term direction is that perhaps we should consider merging PermissionContext and ChooserContext. This would fit in line with the proposed changes to the JS Permissions API to make it work with bluetooth that you showed me: https://gist.github.com/beaufortfrancois/951fc384e87a786a6d35 I don't think others were on board with this idea initially but I think it makes sense. How would you feel about heading that way? If you think it's better to land something like this first, let's do it. But any help (from bluetooth/usb folks) figuring out the right longer term solution would be great :)
On 2016/02/17 05:55:36, raymes wrote: > Hey Jeffrey, I'll take a look. My sense for a longer term direction is that > perhaps we should consider merging PermissionContext and ChooserContext. This > would fit in line with the proposed changes to the JS Permissions API to make it > work with bluetooth that you showed me: > https://gist.github.com/beaufortfrancois/951fc384e87a786a6d35 > > I don't think others were on board with this idea initially but I think it makes > sense. How would you feel about heading that way? > > If you think it's better to land something like this first, let's do it. But any > help (from bluetooth/usb folks) figuring out the right longer term solution > would be great :) This patch does move in the direction of merging the two Context classes, and I think it's likely enough that we'll merge PermissionContext and ChooserContext that I asked Jun to pause work on https://codereview.chromium.org/1560263002/, but because I haven't worked out the details, I don't want to say we're definitely going that direction in the long run. If we don't, I'll have to back out the use of PermissionContext in this patch, and re-implement it using ChooserContext. I'm expecting to sit down and figure this out a bit after the M50 branch point.
Description was changed from ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ==========
jyasskin@chromium.org changed reviewers: + ortuno@chromium.org
On 2016/02/17 18:15:49, Jeffrey Yasskin wrote: > On 2016/02/17 05:55:36, raymes wrote: > > Hey Jeffrey, I'll take a look. My sense for a longer term direction is that > > perhaps we should consider merging PermissionContext and ChooserContext. This > > would fit in line with the proposed changes to the JS Permissions API to make > it > > work with bluetooth that you showed me: > > https://gist.github.com/beaufortfrancois/951fc384e87a786a6d35 > > > > I don't think others were on board with this idea initially but I think it > makes > > sense. How would you feel about heading that way? > > > > If you think it's better to land something like this first, let's do it. But > any > > help (from bluetooth/usb folks) figuring out the right longer term solution > > would be great :) > > This patch does move in the direction of merging the two Context classes, and I > think it's likely enough that we'll merge PermissionContext and ChooserContext > that I asked Jun to pause work on https://codereview.chromium.org/1560263002/, > but because I haven't worked out the details, I don't want to say we're > definitely going that direction in the long run. *Because I haven't worked out the details and because Reilly isn't convinced yet. I'm hoping to get the permissions folks to LGTM this before I send it to the rest of the owners I need.
Description was changed from ========== Add enterprise policy to turn off Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ==========
jyasskin@chromium.org changed reviewers: + tnagel@chromium.org
tnagel, could you look at the policy changes? This is the first policy I've added, so sorry if I got some obvious things wrong. One particular place I didn't follow https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add... is that I didn't add the test to policy_browsertest.cc. It's in https://codereview.chromium.org/1706503002/diff/140001/third_party/WebKit/Lay... instead, and assumes that the policy->permission code works as expected.
Jeffrey, could you please add a browser test? Customers tend to become upset when policies aren't working and the best way to ensure that the policy doesn't break is an end to end test. BTW, it seems that the code requires a rebase. https://codereview.chromium.org/1706503002/diff/140001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1706503002/diff/140001/components/policy/reso... components/policy/resources/policy_templates.json:3108: 3, 'AskWebBluetooth', allows web pages from asking users to grant access to nearby Bluetooth LE devices. If granted, the web page has full access to the device's Bluetooth GATT services. For consistency, I'd suggest to better align the description with that other content settings policies, cf. DefaultMediaStreamSetting for an example.
lgtm Jeffrey and I chatted and this is a temporary solution until we find a unified solution for PermissionContext/ChooserContext. He said that they would be looking at refactoring in the next several weeks. https://codereview.chromium.org/1706503002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1706503002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:87: case PermissionType::BLUETOOTH_GUARD: I wonder if we should just call this BLUETOOTH? https://codereview.chromium.org/1706503002/diff/140001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/1706503002/diff/140001/components/content_set... components/content_settings/core/common/content_settings.cc:56: CONTENT_SETTINGS_TYPE_BLUETOOTH, Did you mean to add this yet? https://codereview.chromium.org/1706503002/diff/140001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/1706503002/diff/140001/components/content_set... components/content_settings/core/common/content_settings_types.h:48: CONTENT_SETTINGS_TYPE_BLUETOOTH, Did you mean to add this yet?
Thanks for looking. https://codereview.chromium.org/1706503002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_manager.cc (right): https://codereview.chromium.org/1706503002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_manager.cc:87: case PermissionType::BLUETOOTH_GUARD: On 2016/02/22 22:34:34, raymes wrote: > I wonder if we should just call this BLUETOOTH? I picked BLUETOOTH_GUARD because it's not where the main BLUETOOTH data gets stored; it just guards use of the overall API. That said, I'm fine using just "BLUETOOTH" like https://codereview.chromium.org/1427433017/diff/1/components/content_settings... did. Let me know which you prefer. And maybe in the long run we can have a single PermissionType that covers both the guard and the main data storage, and then we won't have that ambiguity at all until we call down to the ContentSettings layer. https://codereview.chromium.org/1706503002/diff/140001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/1706503002/diff/140001/components/content_set... components/content_settings/core/common/content_settings.cc:56: CONTENT_SETTINGS_TYPE_BLUETOOTH, On 2016/02/22 22:34:34, raymes wrote: > Did you mean to add this yet? I did, but I can remove it if you prefer. It's there so the two Bluetooth settings are together, even though one isn't used yet. https://codereview.chromium.org/1706503002/diff/140001/components/content_set... File components/content_settings/core/common/content_settings_types.h (right): https://codereview.chromium.org/1706503002/diff/140001/components/content_set... components/content_settings/core/common/content_settings_types.h:48: CONTENT_SETTINGS_TYPE_BLUETOOTH, On 2016/02/22 22:34:34, raymes wrote: > Did you mean to add this yet? Ditto from the other addition.
On 2016/02/22 12:41:51, Thiemo Nagel wrote: > Jeffrey, could you please add a browser test? Customers tend to become upset > when policies aren't working and the best way to ensure that the policy doesn't > break is an end to end test. I have one in https://codereview.chromium.org/1720263002/, but I don't think the changes it needs to DEPS and BrowserMessageFilter are going to be acceptable to the Content API owners. I might be able to add a RenderFrameHost::ExecuteJavaScriptWithUserGestureForTests(string, callback) and use that instead of the BrowserMessageFilter change, but fundamentally, to test this policy, we have to be able to fake out the BluetoothAdapter to pretend to be present, and that requires direct access to the BluetoothDispatcherHost that lives in content/browser. I don't see any test here for geolocation or the media settings policies either. Do they live somewhere else that I could imitate them? https://codereview.chromium.org/1706503002/diff/140001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1706503002/diff/140001/components/policy/reso... components/policy/resources/policy_templates.json:3108: 3, 'AskWebBluetooth', allows web pages from asking users to grant access to nearby Bluetooth LE devices. If granted, the web page has full access to the device's Bluetooth GATT services. On 2016/02/22 12:41:51, Thiemo Nagel wrote: > For consistency, I'd suggest to better align the description with that other > content settings policies, cf. DefaultMediaStreamSetting for an example. Done. Note that http://www.chromium.org/administrators/policy-list-3#DefaultMediaStreamSetting, uses 'PromptOnAccess' without ever defining it. It also claims that the policy lets administrators allow by default, but the policy actually doesn't give that option.
I'm really unsure why the permissions layer is being used here. The fact that the permission is called "guard" and the content/ layer code checks that the value is different than ASK to reject (ie. grant is assumed to be a failure) are good hints that the permission isn't actually a permission. Wouldn't be cleaner to check on the chrome/ layer if the enterprise policy is set and not show the picker? If the call has to happen from the content/ layer, what about adding an explicit call to ContentBrowserClient that checks if the bluetooth api is enabled/disabled?
On 2016/02/23 14:39:23, Mounir Lamouri wrote: > I'm really unsure why the permissions layer is being used here. The fact that > the permission is called "guard" and the content/ layer code checks that the > value is different than ASK to reject (ie. grant is assumed to be a failure) are > good hints that the permission isn't actually a permission. > > Wouldn't be cleaner to check on the chrome/ layer if the enterprise policy is > set and not show the picker? If the call has to happen from the content/ layer, > what about adding an explicit call to ContentBrowserClient that checks if the > bluetooth api is enabled/disabled? "grant" can't happen, so it's better to fail safe than open. I could CHECK instead. But sure, if you really don't want this to go through the permission manager, I'll add a new tunnel through the Content API to express this, imitating ChromeContentBrowserClient::AllowKeygen(). This _is_ going to require the separate browser test for policy. Theimo, do you mind separate tests that 1) setting the policy sets ChromeContentBrowserClient::AllowBluetooth(), and 2) having AllowBluetooth() false blocks use of BluetoothDispatcherHost? If you really want a single test, we'll have to negotiate that with the Content API owners.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
jyasskin@chromium.org changed reviewers: + jam@chromium.org - mlamouri@chromium.org, raymes@chromium.org, reillyg@chromium.org
Here's a version that doesn't use ContentSettings or the permission system, but instead calls into the PolicyMap directly. Thiemo, how's it look? John, this CL gives the MockRenderProcessHost the ability to intercept BrowserMessageFilter messages. Is that ok?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/200001
tnagel@chromium.org changed reviewers: + bartfab@chromium.org
Bartosz, would you possibly have cycles to look into this before branch point?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ==========
bartfab@chromium.org changed reviewers: + atwilson@chromium.org
https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1993: const base::Value* policy_value = Nit 1: #include "base/values.h" Nit 2: const pointer https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1995: ->GetPolicies(policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME, Nit: #include "components/policy/core/common/policy_namespace.h" https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1997: .GetValue(policy::key::kDefaultWebBluetoothGuardSetting); You documented this policy as being per-profile, but you read it from a browser-global policy service here, which has no notion of per-profile policy. https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1999: if (policy_value && policy_value->GetAsInteger(&int_value)) { 99% of policies are implemented differently: - Define a pref (or reuse an existing one if possible) - Read the pref wherever you need to know the setting value - Have the policy take control of the pref Can you switch to this standard mechanism instead? https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.h:175: bool AllowWebBluetooth() override; We have a number of policies that control content settings. Can the Bluetooth policy not be implemented the way those other policies are? https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3668: // See BluetoothDispatcherHostTest.ContentClientCanDisableAPI for evidence We try hard to make our browser tests be truly end-to-end. Would it be possible to bring up the Bluetooth stack here and verify that it is affected by the policy? https://codereview.chromium.org/1706503002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1706503002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63400: + <int value="320" label="Default web bluetooth guard setting"/> Nit: Please sync this label with the caption in policy_templates.json.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/240001
jyasskin@chromium.org changed reviewers: + asvitkine@chromium.org, palmer@chromium.org
Chris, could you check the macro change in bluetooth_messages.h? Alexei, could you check histograms.xml? Thanks! https://codereview.chromium.org/1706503002/diff/140001/components/content_set... File components/content_settings/core/common/content_settings.cc (right): https://codereview.chromium.org/1706503002/diff/140001/components/content_set... components/content_settings/core/common/content_settings.cc:56: CONTENT_SETTINGS_TYPE_BLUETOOTH, On 2016/02/22 22:52:17, Jeffrey Yasskin wrote: > On 2016/02/22 22:34:34, raymes wrote: > > Did you mean to add this yet? > > I did, but I can remove it if you prefer. It's there so the two Bluetooth > settings are together, even though one isn't used yet. I didn't re-add the non-GUARD enumerator when I re-added the content-settings piece. https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1999: if (policy_value && policy_value->GetAsInteger(&int_value)) { On 2016/02/25 17:37:50, bartfab (slow) wrote: > 99% of policies are implemented differently: > - Define a pref (or reuse an existing one if possible) > - Read the pref wherever you need to know the setting value > - Have the policy take control of the pref > > Can you switch to this standard mechanism instead? Done, which covers the previous 3 comments too. https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.h:175: bool AllowWebBluetooth() override; On 2016/02/25 17:37:50, bartfab (slow) wrote: > We have a number of policies that control content settings. Can the Bluetooth > policy not be implemented the way those other policies are? As discussed over IM, most of the policies are either chrome-only or go through the PermissionManager tunnel through the Content API, which Mounir suggested I not use for this. I'm imitating DefaultKeygenSetting for this one. https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3668: // See BluetoothDispatcherHostTest.ContentClientCanDisableAPI for evidence On 2016/02/25 17:37:50, bartfab (slow) wrote: > We try hard to make our browser tests be truly end-to-end. Would it be possible > to bring up the Bluetooth stack here and verify that it is affected by the > policy? It turns out to be really hard. To run a bluetooth test, we have to fake the BluetoothAdapter to return true from IsPresent, for which we use content::BluetoothDispatcherHost::SetBluetoothAdapterForTesting(). The Policy code lives in chrome/ or chrome-level components, which can't call directly into the content/ layer. If we overcame that, I'd still need to drive the renderer to call into the Bluetooth API. (I could send the IPCs directly to the BluetoothDispatcherHost, and do something to intercept the responses, but jam@'s rejected similar attempts in the past.) RenderFrameHost::ExecuteJavaScriptWithUserGestureForTests offers a way to do that, but only allows getting a response from Javascript that can return synchronously. navigator.bluetooth.requestDevice() returns a Promise, so can't work with this system. As discussed over IM, I'll keep watching for a way to do this in a single test, but I'll leave the 2-part test for this CL. https://codereview.chromium.org/1706503002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1706503002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63400: + <int value="320" label="Default web bluetooth guard setting"/> On 2016/02/25 17:37:50, bartfab (slow) wrote: > Nit: Please sync this label with the caption in policy_templates.json. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/260001
Chris, I no longer have a change to bluetooth_messages.h, so you don't need to look anymore. https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3668: // See BluetoothDispatcherHostTest.ContentClientCanDisableAPI for evidence On 2016/02/25 20:38:50, Jeffrey Yasskin wrote: > On 2016/02/25 17:37:50, bartfab (slow) wrote: > > We try hard to make our browser tests be truly end-to-end. Would it be > possible > > to bring up the Bluetooth stack here and verify that it is affected by the > > policy? > > It turns out to be really hard. To run a bluetooth test, we have to fake the > BluetoothAdapter to return true from IsPresent, for which we use > content::BluetoothDispatcherHost::SetBluetoothAdapterForTesting(). The Policy > code lives in chrome/ or chrome-level components, which can't call directly into > the content/ layer. > > If we overcame that, I'd still need to drive the renderer to call into the > Bluetooth API. (I could send the IPCs directly to the BluetoothDispatcherHost, > and do something to intercept the responses, but jam@'s rejected similar > attempts in the past.) RenderFrameHost::ExecuteJavaScriptWithUserGestureForTests > offers a way to do that, but only allows getting a response from Javascript that > can return synchronously. navigator.bluetooth.requestDevice() returns a Promise, > so can't work with this system. > > As discussed over IM, I'll keep watching for a way to do this in a single test, > but I'll leave the 2-part test for this CL. Gio pointed out the device::BluetoothAdapterFactory::SetAdapterForTesting() function that doesn't go through the content/ API, and I re-discovered content::ExecuteScriptAndExtractString, which can handle asynchronous results, at which point the unified test became easy. Let me know what you think.
jyasskin@chromium.org changed reviewers: - palmer@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
policy LGTM with a couple nits - feel free to CQ after fixing these https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/220001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3668: // See BluetoothDispatcherHostTest.ContentClientCanDisableAPI for evidence On 2016/02/25 22:34:49, Jeffrey Yasskin wrote: > On 2016/02/25 20:38:50, Jeffrey Yasskin wrote: > > On 2016/02/25 17:37:50, bartfab (slow) wrote: > > > We try hard to make our browser tests be truly end-to-end. Would it be > > possible > > > to bring up the Bluetooth stack here and verify that it is affected by the > > > policy? > > > > It turns out to be really hard. To run a bluetooth test, we have to fake the > > BluetoothAdapter to return true from IsPresent, for which we use > > content::BluetoothDispatcherHost::SetBluetoothAdapterForTesting(). The Policy > > code lives in chrome/ or chrome-level components, which can't call directly > into > > the content/ layer. > > > > If we overcame that, I'd still need to drive the renderer to call into the > > Bluetooth API. (I could send the IPCs directly to the BluetoothDispatcherHost, > > and do something to intercept the responses, but jam@'s rejected similar > > attempts in the past.) > RenderFrameHost::ExecuteJavaScriptWithUserGestureForTests > > offers a way to do that, but only allows getting a response from Javascript > that > > can return synchronously. navigator.bluetooth.requestDevice() returns a > Promise, > > so can't work with this system. > > > > As discussed over IM, I'll keep watching for a way to do this in a single > test, > > but I'll leave the 2-part test for this CL. > > Gio pointed out the device::BluetoothAdapterFactory::SetAdapterForTesting() > function that doesn't go through the content/ API, and I re-discovered > content::ExecuteScriptAndExtractString, which can handle asynchronous results, > at which point the unified test became easy. Let me know what you think. Thanks for that. Looks great! https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1995: HostContentSettingsMap* content_settings = Nit: Const pointer to const. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2000: GURL(requesting_origin.Serialize()), Nit 1: #include "url/gurl.h" Nit 2: #include "url/origin.h" https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2002: CONTENT_SETTINGS_TYPE_BLUETOOTH_GUARD, Nit: #include "components/content_settings/core/common/content_settings_types.h" https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3681: content::WebContents* const web_contents = Nit: const pointer https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3683: ASSERT_THAT( Nit: Would an EXPECT not be sufficient here? https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3684: web_contents->GetMainFrame()->GetLastCommittedOrigin().Serialize(), Nit: #include "url/origin.h" https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3698: ".then(() => { domAutomationController.send('Success'); }," Nit: Indentation.
Thanks for the last-minute review! https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1995: HostContentSettingsMap* content_settings = On 2016/02/26 18:00:44, bartfab (slow) wrote: > Nit: Const pointer to const. Done. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2000: GURL(requesting_origin.Serialize()), On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit 1: #include "url/gurl.h" > Nit 2: #include "url/origin.h" Done. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2002: CONTENT_SETTINGS_TYPE_BLUETOOTH_GUARD, On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit: #include "components/content_settings/core/common/content_settings_types.h" Done. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3681: content::WebContents* const web_contents = On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit: const pointer Nope, GetMainFrame() is non-const. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3683: ASSERT_THAT( On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit: Would an EXPECT not be sufficient here? We won't get useful errors from the rest of the test if the navigation failed, which is what this is testing, but it also won't crash the test, so sure. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3684: web_contents->GetMainFrame()->GetLastCommittedOrigin().Serialize(), On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit: #include "url/origin.h" Done. https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3698: ".then(() => { domAutomationController.send('Success'); }," On 2016/02/26 18:00:45, bartfab (slow) wrote: > Nit: Indentation. Heh, I have no idea how to indent .then() calls. I've added 2 spaces.
The CQ bit was checked by jyasskin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, jam@chromium.org, bartfab@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1706503002/#ps280001 (title: "Fix nits")
lgtm https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1706503002/diff/260001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3698: ".then(() => { domAutomationController.send('Success'); }," On 2016/02/26 18:21:25, Jeffrey Yasskin wrote: > On 2016/02/26 18:00:45, bartfab (slow) wrote: > > Nit: Indentation. > > Heh, I have no idea how to indent .then() calls. I've added 2 spaces. I have no idea either, but zero spaces seemed like too little :).
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706503002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706503002/280001
Message was sent while issue was closed.
Description was changed from ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. ========== to ========== Add enterprise policy to turn off Web Bluetooth. Organizations will be able to set "DefaultWebBluetoothGuardSetting" to '2' to prevent their users from using Web Bluetooth at all, or '3' to explicitly leave the setting at the default "ask" value. There is no "allow" option, because it wouldn't specify which device to select from the chooser. BUG=562592 TEST=On ChromeOS, Android, Mac, and Windows 8.1, run the following steps with 3 different enterprise policy settings. Note that the test machine needs Bluetooth 4 support, and Bluetooth has to be turned on. If it's not, you'll get a "Bluetooth adapter not available." error, and won't have tested this patch. 1) Run chrome with the --enable-web-bluetooth flag. 2) Visit https://googlechrome.github.io/samples/web-bluetooth/device-info.html 3) Type "Test" into the "Device Name Prefix" text box (the third). 4) Press the "Get Bluetooth Device Info" button. Enterprise policy A: 'DefaultWebBluetoothGuardSetting' unset. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Enterprise policy B: 'DefaultWebBluetoothGuardSetting' set to 2. No dialog appears, and the "Live Output" box says "Argh! NotFoundError: User or their enterprise policy has disabled Web Bluetooth globally." Enterprise policy C: 'DefaultWebBluetoothGuardSetting' set to 3. A dialog saying "https://googlechrome.github.io wants to pair with" should appear. Click 'Cancel' or outside the dialog to close it. Committed: https://crrev.com/98bdd399e51a63f104421344e79a1e838d00fcdc Cr-Commit-Position: refs/heads/master@{#377959} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/98bdd399e51a63f104421344e79a1e838d00fcdc Cr-Commit-Position: refs/heads/master@{#377959} |
