Make extensions developer mode adhere to policy

chrome/browser/policy/policy_browsertest.cc

Index: chrome/browser/policy/policy_browsertest.cc
diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
index ff9bdf7ccc147d8c4b9e143bb20ab930a92fe5b5..2a5cae6cc470cd6d5c51160eea802160f2425c60 100644
--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -1317,6 +1317,86 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabled) {
   EXPECT_FALSE(DevToolsWindow::GetInstanceForInspectedWebContents(contents));
 }
 
+namespace {
+
+void WaitForExtensionsDevModeControlsVisibility(content::WebContents* contents,
+                                                bool expected_visible) {
+  // Visibility of dev-controls is controlled through its height - wait for that
+  bool done = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents,
+      base::StringPrintf(
+          "var screenElement = document.getElementById('dev-controls');"
+          "function SendReplyIfAsExpected() {"
+          "  var is_visible = parseFloat(screenElement.style.height) > 0;"
+          "  if (is_visible != %s)"
+          "    return false;"
+          "  observer.disconnect();"
+          "  domAutomationController.send(true);"
+          "  return true;"
+          "}"
+          "var observer = new MutationObserver(SendReplyIfAsExpected);"
+          "if (!SendReplyIfAsExpected()) {"
+          "  var options = { 'attributes': true };"
+          "  observer.observe(screenElement, options);"
+          "}",
+          (expected_visible ? "true" : "false")),
+      &done));
+}
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) {

[Devlin, 2016/11/28 17:34:01]

This is a pretty fragile browser test for a number of reasons - mainly that a)
it's very breakable with any changes in the extensions page and b) it only tests
that clicking with a mouse doesn't work (see also comment in
developer_private_api.cc).  I'd prefer to remove this and instead use a new api
unittest test for the developerPrivate.updateProfileConfiguration method.

[pmarko, 2016/11/29 12:40:22]

unittest: good idea, I'll check if I can add one.

browsertest:
I should have described that better - the main purpose of the test was the
sequence:
- user enables devmode -> the dev controls appear
- policy comes in -> (1) dev controls automatically disappear and
                     (2) user can not re-enable it.
You are right in that it is only testing clicking on the toggle control -- but
it clicks when the checkbox is still enabled. After the policy comes in, I've
changed it to only check that the toggle control is disabled.

As for the fragility: The test depends on the following assumptions about the
extensions UI:
- that the control for toggling dev mode has the ID 'toggle-dev-on'
- that the dev-mode-buttons are in a DOM element with the ID 'dev-controls'
- that the implementor chose to display/hide the 'dev-controls' using the
'height' attribute.
On the other hand, it seemed to me like a good trade-off for an end-to-end test
of the feature. This way we can have a regression test seeing if the controls
are being actively hidden when the policy comes in.

I'd suggest the following:
- I will extract the IDs/assumptions into constants and comment what they are
- I will clearly document the why it's checking for height

Would you think that this makes the test better or would you still favor not
having such a test at all?

[Devlin, 2016/11/30 19:19:50]

Ah, okay, that makes more sense.  If this is deliberately just checking the UI,
I think it's fine.

[pmarko, 2016/12/02 00:20:07]

Acknowledged.

+  // Verifies that when DeveloperToolsDisabled policy is set, the "dev mode"
+  // in chrome://extensions-frame is also not accessible.
+  // Also verifies that the developer mode related buttons are actively hidden
+  // when the policy comes in.
+  // Note: We don't test the indicator as it is tested in the policy pref test
+  // for kDeveloperToolsDisabled.
+
+  const char js_toggle_dev_mode_checkbox[] =
+      "document.getElementById('toggle-dev-on')";
+
+  // Navigate to the extensions frame and enabled "Developer mode"
+  ui_test_utils::NavigateToURL(browser(),
+                               GURL(chrome::kChromeUIExtensionsFrameURL));
+
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(content::ExecuteScript(
+      contents, base::StringPrintf("domAutomationController.send(%s.click());",
+                                   js_toggle_dev_mode_checkbox)));
+
+  WaitForExtensionsDevModeControlsVisibility(contents, true);
+
+  // Disable devtools via policy.
+  PolicyMap policies;
+  policies.Set(key::kDeveloperToolsDisabled, POLICY_LEVEL_MANDATORY,
+               POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
+               base::MakeUnique<base::FundamentalValue>(true), nullptr);
+  UpdateProviderPolicy(policies);
+
+  // Expect devcontrols to be hidden now...
+  WaitForExtensionsDevModeControlsVisibility(contents, false);
+
+  // ... and checkbox state
+  bool is_toggle_dev_mode_checkbox_checked = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents, base::StringPrintf("domAutomationController.send(%s.checked);",
+                                   js_toggle_dev_mode_checkbox),
+      &is_toggle_dev_mode_checkbox_checked));
+  EXPECT_FALSE(is_toggle_dev_mode_checkbox_checked);
+
+  bool is_toggle_dev_mode_checkbox_enabled = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      contents,
+      base::StringPrintf(
+          "domAutomationController.send(!%s.hasAttribute('disabled'))",
+          js_toggle_dev_mode_checkbox),
+      &is_toggle_dev_mode_checkbox_enabled));
+  EXPECT_FALSE(is_toggle_dev_mode_checkbox_enabled);
+}
+
 // TODO(samarth): remove along with rest of NTP4 code.
 IN_PROC_BROWSER_TEST_F(PolicyTest, DISABLED_WebStoreIconHidden) {
   // Verifies that the web store icons can be hidden from the new tab page.