Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696283007:
DevTools: disable the Chrome tab debugging warning for force-installed extensions. (Closed)
|
Created:
3 years, 10 months ago by pfeldman Modified:
3 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, asvitkine+watch_chromium.org, devtools-reviews_chromium.org, chromium-apps-reviews_chromium.org, tnagel+watch_chromium.org, bartfab (slow) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: disable the Chrome tab debugging warning for force-installed extensions.
BUG=693621
Review-Url: https://codereview.chromium.org/2696283007
Cr-Commit-Position: refs/heads/master@{#452706}
Committed: https://chromium.googlesource.com/chromium/src/+/c64e1a693b3207b89733909793721b9af2ca5cd2
Patch Set 1 #
Total comments: 2
Patch Set 2 : review comment addressed #
Total comments: 19
Patch Set 3 : review comments addressed #
Total comments: 4
Patch Set 4 : addressed comments. #Patch Set 5 : removed the policy #
Total comments: 1
Patch Set 6 : comment added #Patch Set 7 : illegal access in the tests. #Messages
Total messages: 43 (25 generated)
The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pfeldman@chromium.org changed reviewers: + bartfab@chromium.org, rdevlin.cronin@chromium.org, tnagel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
extensions lgtm https://codereview.chromium.org/2696283007/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2696283007/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1611: // A boolean linked with the kSilentDebuggerExtensionAPI command line switch for nit: I'd rephrase this as just: "A boolean to allow policy-installed extensions to use the debugging API silently." (To save the reader from needing to look up kSilentDebuggerExtensionAPI and because it's possible that switch may not always be around)
https://codereview.chromium.org/2696283007/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2696283007/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1611: // A boolean linked with the kSilentDebuggerExtensionAPI command line switch for On 2017/02/17 20:51:31, Devlin wrote: > nit: I'd rephrase this as just: > "A boolean to allow policy-installed extensions to use the debugging API > silently." > > (To save the reader from needing to look up kSilentDebuggerExtensionAPI and > because it's possible that switch may not always be around) Done.
The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Let enterprise policy disable the Chrome tab debugging warning. BUG=693621 ========== to ========== Let enterprise policy disable the Chrome tab debugging warning. BUG=693621 ==========
pfeldman@chromium.org changed reviewers: + atwilson@chromium.org
tnagel@chromium.org changed reviewers: - atwilson@chromium.org, bartfab@chromium.org
Imho it would be best to only include one reviewer per topic. Specifying overlapping reviewers leads to diffusion of responsibility and potentially duplication of effort. Moving atwilson and bartfab to cc.
https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:1611: // A boolean to allow policy-installed extensions to use the debugging API Nit: I think a more common term is "force-installed". https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9601: 'name': 'DeveloperToolsSilentManagedDebuggerAPIEnabled', Nit: Imho the "Managed" part of the name is hard to parse since it doesn't refer to any of the terms in the name. (Iiuc it refers to "managed extension" which however is not part of the name.) I'd suggest to drop it: DeveloperToolsSilentDebuggerAPIEnabled would seem more lucid to me. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9611: 'caption': '''Enable Developer Tools silent chrome.debugger extensions API for managed extensions''', Nit: Please tag "chrome.debugger" as non-translatable (cf. instructions at the top of the file). Nit: Instead of "managed extension" I think a more common term is "force-installed". https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9612: 'tags': [], Nit: "system-security" would seem appropriate here https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9613: 'desc': '''Enables enterprise-managed extensions to instrument browser without the user-visible warning. Nit: instrument *the* browser https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9615: If you enable this setting, enterprise-managed browser extensions will be able to attach to the browser and instrument it without the user-visible warning.''' Nit: It's preferred to not address the audience directly ("you") and rather use passive voice. Nit: the user-visible warning --> *a* user-visible warning https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9616: }, Nit: Please explicitly spell out what happens when the policy is disabled or unset.
atwilson@chromium.org changed reviewers: + atwilson@chromium.org
https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/debugger/debugger_api.cc:352: prefs::kDevToolsSilentManagedDebuggerAPIEnabled)) { IMO, this new policy is unnecessary and we should just hide this UI for all policy-installed extensions. But let's see what privacy folks think. Also, the tests check to see that we are setting a pref based on the policy, but don't actually check that the UI is shown/hidden. I'm not dogmatic about this, but would be nice if we had a test that actually set the policy and verified the UI didn't show up.
https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/debugger/debugger_api.cc:352: prefs::kDevToolsSilentManagedDebuggerAPIEnabled)) { On 2017/02/22 11:13:42, Andrew T Wilson (Slow) wrote: > IMO, this new policy is unnecessary and we should just hide this UI for all > policy-installed extensions. But let's see what privacy folks think. That was Pavel's original suggestion, but I pushed back because I and others were uncomfortable with effectively opting-in every enterprise to this new behavior. Some enterprises may desire, or even depend, on this UI to keep users informed.
Thank you for the review, PTAL! https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/debugger/debugger_api.cc:352: prefs::kDevToolsSilentManagedDebuggerAPIEnabled)) { I don't have a strong opinion on this one, as Devlin mentions that was my original intention. But in policy is safer and not expensive to maintain, we can have it. https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:1611: // A boolean to allow policy-installed extensions to use the debugging API On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: I think a more common term is "force-installed". Done. This term comes from Devlin, also Andrew is using the same one in the comments. The check I'm making is Manifest::IsPolicyLocation(extension->location()), so as I understand, it can be uninstalled without violating the policy. I'm fine with either of the options, but I'd like to get a recommendation that both extensions and policy people agree upon. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9601: 'name': 'DeveloperToolsSilentManagedDebuggerAPIEnabled', On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: Imho the "Managed" part of the name is hard to parse since it doesn't refer > to any of the terms in the name. (Iiuc it refers to "managed extension" which > however is not part of the name.) I'd suggest to drop it: > DeveloperToolsSilentDebuggerAPIEnabled would seem more lucid to me. There is also--silent-debugger-extension-api command line flag and it might be confusing why those two are not wired together should we drop the Managed. The difference is that the policy applies to forced / policy-installed extensions only. I'm thinking that it might be Ok to have a non-readable name that forces investigation upon stumbling upon when making a change. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9611: 'caption': '''Enable Developer Tools silent chrome.debugger extensions API for managed extensions''', On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: Please tag "chrome.debugger" as non-translatable (cf. instructions at the > top of the file). > > Nit: Instead of "managed extension" I think a more common term is > "force-installed". Done. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9612: 'tags': [], On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: "system-security" would seem appropriate here Done. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9613: 'desc': '''Enables enterprise-managed extensions to instrument browser without the user-visible warning. On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: instrument *the* browser Done. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9615: If you enable this setting, enterprise-managed browser extensions will be able to attach to the browser and instrument it without the user-visible warning.''' On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: It's preferred to not address the audience directly ("you") and rather use > passive voice. > Done, but this file has 134 "If you" mentions. > Nit: the user-visible warning --> *a* user-visible warning Done. https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9616: }, On 2017/02/22 11:13:29, Thiemo Nagel wrote: > Nit: Please explicitly spell out what happens when the policy is disabled or > unset. Done.
l-g-t-m % nits from my side, but please wait for ok from Drew. https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2696283007/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:1611: // A boolean to allow policy-installed extensions to use the debugging API Maybe "force-installed" is just what we use inside our team then? I guess both is fine then. > The check I'm making is Manifest::IsPolicyLocation(extension->location()), so as > I understand, it can be uninstalled without violating the policy. Not as I read [1]: "Specifies a list of apps and extensions [...] which cannot be uninstalled by the user." [1] http://www.chromium.org/administrators/policy-list-3#ExtensionInstallForcelist https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2696283007/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9615: If you enable this setting, enterprise-managed browser extensions will be able to attach to the browser and instrument it without the user-visible warning.''' > Done, but this file has 134 "If you" mentions. I'm not surprised. We have to start somewhere, I guess though. https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9615: If this policy is set, force-installed browser extensions will be able to attach to the browser and instrument it without a user-visible warning. Nit: set --> set to true https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9617: If this policy is unset (default), a user-visible warning shows up upon establishing new debugging sessions.''' Nit: unset (default) --> false or unset
https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9615: If this policy is set, force-installed browser extensions will be able to attach to the browser and instrument it without a user-visible warning. On 2017/02/22 17:15:10, Thiemo Nagel wrote: > Nit: set --> set to true Done. https://codereview.chromium.org/2696283007/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:9617: If this policy is unset (default), a user-visible warning shows up upon establishing new debugging sessions.''' On 2017/02/22 17:15:10, Thiemo Nagel wrote: > Nit: unset (default) --> false or unset Done.
The CQ bit was checked by pfeldman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@atwilson, over to you for the stamp.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/22 18:18:51, pfeldman wrote: > @atwilson, over to you for the stamp. Still discussing the privacy impact on the bug - just want to make sure I understand why this behavior merits a policy.
Description was changed from ========== Let enterprise policy disable the Chrome tab debugging warning. BUG=693621 ========== to ========== DevTools: disable the Chrome tab debugging warning for force-installed extensions. BUG=693621 ==========
Devlin, could you ptal?
lgtm https://codereview.chromium.org/2696283007/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/debugger/debugger_api.cc (right): https://codereview.chromium.org/2696283007/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/debugger/debugger_api.cc:351: if (Manifest::IsPolicyLocation(extension->location())) nit: Can we add a comment here like: // We allow policy-installed extensions to circumvent the normal // infobar warning. See crbug.com/693621.
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2696283007/#ps100001 (title: "comment added")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by pfeldman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2696283007/#ps120001 (title: "illegal access in the tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487894041051480,
"parent_rev": "9bed660668c954ffd18eee419fba8c5cfaac8789", "commit_rev":
"c64e1a693b3207b89733909793721b9af2ca5cd2"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: disable the Chrome tab debugging warning for force-installed extensions. BUG=693621 ========== to ========== DevTools: disable the Chrome tab debugging warning for force-installed extensions. BUG=693621 Review-Url: https://codereview.chromium.org/2696283007 Cr-Commit-Position: refs/heads/master@{#452706} Committed: https://chromium.googlesource.com/chromium/src/+/c64e1a693b3207b8973390979372... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c64e1a693b3207b8973390979372... |
