Disable modifying extensions when in managed mode.

Disable modifying extensions when in managed mode.

Consolidate yellow banners on Extensions and Options pages. Add managed mode as an extensions::ManagementPolicy::Provider. Managed mode prohibits installing, uninstalling, enabling, or disabling extensions, both in the UI and in the back end. Also remove links to the web store, as well as optional controls (options, incognito, reload, etc.).

BUG=117987
TEST=unit tests (for managed mode methods); manual: verify that chrome://extensions UI is disabled in managed mode; manual: launch in managed mode and verify that managed mode can be disabled

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143960

[Pam (message me for reviews), 2012-06-12 12:38:47]

Bernhard: please review.
Pat: remember when I asked you how to get the banner displaying properly on the
Extensions page? Here's the result -- please take a look.

Thanks,
- Pam

[Bernhard Bauer, 2012-06-12 12:55:21]

http://codereview.chromium.org/10542023/diff/8025/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10542023/diff/8025/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4410: +      <message
name="IDS_EXTENSION_CANT_MODIFY_MANAGED_MODE" desc="Error message when a user
tries to remove or change an extension while the browser is in managed mode.">
Could you merge this string with IDS_EXTENSIONS_MANAGED_MODE?

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode.cc
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.cc:45: if (IsInManagedModeImpl()) {
Return early if we're not in managed mode?

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.cc:49:
managed_profile_)->management_policy()->RegisterProvider(this);
This looks a bit awkward. Pull out the extension system into a local variable?

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.cc:133: std::string
ManagedMode::GetPolicyProviderName(void) const {
"void" in the signature is unnecessary.

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.cc:134: return "Managed Mode";
This looks really strange to me. Is is correct that this is only used in Debug
builds? If so, could we compile it out in Release builds?

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.cc:148: if (IsInManagedModeImpl()) {
You could invert the condition to return early if we're not in managed mode.

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode.h
File chrome/browser/managed_mode.h (right):

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.h:101: // already have been set before this is
called.
We could change the signature of this method to take the managed profile when
entering managed mode and NULL when leaving it.

[Pam (message me for reviews), 2012-06-15 13:27:07]

Thanks. I've implemented all but one of your suggestions. See the one remaining
below, and please take another look at the patch.

- Pam

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode.h
File chrome/browser/managed_mode.h (right):

http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
chrome/browser/managed_mode.h:101: // already have been set before this is
called.
On 2012/06/12 12:55:21, Bernhard Bauer wrote:
> We could change the signature of this method to take the managed profile when
> entering managed mode and NULL when leaving it.

That would preclude its being called in ManagedMode::RegisterPrefs, which is
static and doesn't have access to managed_profile_.

I tried having two methods, one that takes the profile and one that uses
managed_profile_, but that was more confusing -- especially since we need to set
managed_profile_ before calling SetInManagedMode() in EnterManagedModeImpl()
anyway so it's correct in OnBrowserAdded().

[Bernhard Bauer, 2012-06-15 13:45:11]

Did you delete the old patch set? I don't see them anymore, and the comment
links return an error.

On 2012/06/15 13:27:07, Pam wrote:
> Thanks. I've implemented all but one of your suggestions. See the one
remaining
> below, and please take another look at the patch.
> 
> - Pam
> 
>
http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode.h
> File chrome/browser/managed_mode.h (right):
> 
>
http://codereview.chromium.org/10542023/diff/8025/chrome/browser/managed_mode...
> chrome/browser/managed_mode.h:101: // already have been set before this is
> called.
> On 2012/06/12 12:55:21, Bernhard Bauer wrote:
> > We could change the signature of this method to take the managed profile
when
> > entering managed mode and NULL when leaving it.
> 
> That would preclude its being called in ManagedMode::RegisterPrefs, which is
> static and doesn't have access to managed_profile_.

Ohhh, I see. But that seems wrong now anyway. RegisterPrefs() would be exactly a
case where we call SetInManagedMode() without having previously set the
|managed_profile_|, right? If we're setting |managed_profile_| in InitImpl()
anyway, we could check the command line there and adjust whether we are in
managed mode accordingly.

> I tried having two methods, one that takes the profile and one that uses
> managed_profile_, but that was more confusing -- especially since we need to
set
> managed_profile_ before calling SetInManagedMode() in EnterManagedModeImpl()
> anyway so it's correct in OnBrowserAdded().

[Pam (message me for reviews), 2012-06-15 15:48:18]

On 2012/06/15 13:45:11, Bernhard Bauer wrote:
> Did you delete the old patch set? I don't see them anymore, and the comment
> links return an error.

Not on purpose. Maybe I mis-clicked. Sorry about that!

> If we're setting |managed_profile_| in InitImpl()
> anyway, we could check the command line there and adjust whether we are in
> managed mode accordingly.

Good idea. Done.

I also changed my mind about the GetPolicyProviderName method. It's too messy to
wrap every implementation and call in #ifndef NDEBUG. I took those out, but left
the name change to GetDebugPolicyProviderName. Does that work for you?

New patch ready for review.

[Patrick Dubroy, 2012-06-18 12:13:01]

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.html:53: <div id="dev-controls">
Why aren't you using hidden anymore?

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:226: var developer_mode =
extensionsData.developerMode;
JS style is developerMode, not developer_mode.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:229:
pageDiv.classList.add('showing-banner');
Do you need both of these classes? Can't you just show the banner whenever
managed mode is enabled?

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:239: if (developer_mode) {
I'd just do this:

    if (extensionsData.developerMode && !extensionsData.managedMode)

and get rid of the variable.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.html:457: <span
id="banner-text" class="main-page-banner-text"></span>
IMO the old name was more descriptive.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
File chrome/browser/resources/shared/css/chrome_shared.css (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:110: */
It seems like a lot of this code should be in uber_shared.css, not
chrome_shared.css. Ideally, anything for the visual styling of the banner should
be in the latter, and everything for positioning and hiding/showing should be in
the former.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:112: margin-top: 45px;
This should be in uber_shared.css, for example.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:116: background-color:
white;
Is this necessary?

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:125:
.main-page-banner-gradient {
I'd move this to chrome_shared.css and rename the class to just banner-gradient.

[Bernhard Bauer, 2012-06-18 13:41:05]

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:136: return "Managed Mode";
Is the compiler smart enough to strip this out if it isn't used? I would like to
avoid having these strings in a shipping binary.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/managed_mode.h
File chrome/browser/managed_mode.h (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/managed_mod...
chrome/browser/managed_mode.h:105: Profile* newly_managed_profile);
Is there a case where we call SetInManageMode(true, NULL) or
SetInManagedMode(false, <not NULL>)? If not, would it make sense to leave out
the boolean parameter, as it's redundant?

[Pam (message me for reviews), 2012-06-18 18:26:00]

Thanks for the reviews. Everything's done, except as noted below. Please take
another look.

- Pam

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.html:53: <div id="dev-controls">
On 2012/06/18 12:13:01, dubroy wrote:
> Why aren't you using hidden anymore?

Since it already has the "dev-mode" class set when they're visible, I changed it
to use that to control the display mode in CSS. This shortens the JS and is
consistent with other similarly toggled elements.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:229:
pageDiv.classList.add('showing-banner');
On 2012/06/18 12:13:01, dubroy wrote:
> Do you need both of these classes? Can't you just show the banner whenever
> managed mode is enabled?

Showing-banner controls the page banner, which is part of the shared CSS and
thus isn't managed-mode-specific. And managed-mode controls the visibility of
the optional controls for each listed extension.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.html:457: <span
id="banner-text" class="main-page-banner-text"></span>
On 2012/06/18 12:13:01, dubroy wrote:
> IMO the old name was more descriptive.

Yep. But it's using shared CSS now, so it needs a more general shared name.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
File chrome/browser/resources/shared/css/chrome_shared.css (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:110: */
On 2012/06/18 12:13:01, dubroy wrote:
> It seems like a lot of this code should be in uber_shared.css, not
> chrome_shared.css. Ideally, anything for the visual styling of the banner
should
> be in the latter, and everything for positioning and hiding/showing should be
in
> the former.

Moved the whole block to uber_shared.css following our discussion.

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/s...
chrome/browser/resources/shared/css/chrome_shared.css:116: background-color:
white;
On 2012/06/18 12:13:01, dubroy wrote:
> Is this necessary?

I don't know -- I'm just moving this from options_page.css and am reluctant to
remove it.

[Bernhard Bauer, 2012-06-19 10:01:36]

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:252: DCHECK(!managed_profile_);
Does that work for the case where we wait for other browsers to close? I think
in that case |managed_profile_| is already set.

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
File chrome/browser/managed_mode_unittest.cc (right):

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
chrome/browser/managed_mode_unittest.cc:45: return in_managed_mode_;
We could probably save this flag now by just directly comparing
|managed_profile_| in here.

[Pam (message me for reviews), 2012-06-19 12:56:05]

Thanks, Bernhard; here's another go.

- Pam

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:252: DCHECK(!managed_profile_);
On 2012/06/19 10:01:36, Bernhard Bauer wrote:
> Does that work for the case where we wait for other browsers to close? I think
> in that case |managed_profile_| is already set.

Fixed. And the tests didn't catch it because they override this method. I fixed
the test too, and will think about the cleanest way to handle the fact that this
method has zero test coverage.

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
File chrome/browser/managed_mode_unittest.cc (right):

http://codereview.chromium.org/10542023/diff/12043/chrome/browser/managed_mod...
chrome/browser/managed_mode_unittest.cc:45: return in_managed_mode_;
On 2012/06/19 10:01:36, Bernhard Bauer wrote:
> We could probably save this flag now by just directly comparing
> |managed_profile_| in here.

We still need the flag, because while we're waiting for other browsers to close
(via EnterManagedModeForTesting, rather than SetInManagedMode) we're not yet in
managed mode.

[Pam (message me for reviews), 2012-06-19 12:58:48]

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:252: DCHECK(!managed_profile_ || managed_profile_
== newly_managed_profile);
I'll remove the extra space before the == before I commit this.

[Bernhard Bauer, 2012-06-19 13:01:04]

LGTM with one last thing for the test:

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_unittest.cc (right):

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
chrome/browser/managed_mode_unittest.cc:46: DCHECK(!managed_profile_ ||
managed_profile_ == newly_managed_profile);
Could you ASSERT instead of DCHECK so that we fail instead of crashing?

[Patrick Dubroy, 2012-06-19 13:01:52]

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.html:457: <span
id="banner-text" class="main-page-banner-text"></span>
On 2012/06/18 18:26:00, Pam wrote:
> On 2012/06/18 12:13:01, dubroy wrote:
> > IMO the old name was more descriptive.
> 
> Yep. But it's using shared CSS now, so it needs a more general shared name.

IMO "main-page-banner" is still not the best name. Is it the banner of the main
page, or the main banner of the page? I think I'd just call it a page banner.

[Pam (message me for reviews), 2012-06-19 20:37:45]

Thanks again. Pat, please take another look.

- Pam

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/internet_detail.html (right):

http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/internet_detail.html:457: <span
id="banner-text" class="main-page-banner-text"></span>
On 2012/06/19 13:01:52, dubroy wrote:
> IMO "main-page-banner" is still not the best name. Is it the banner of the
main
> page, or the main banner of the page? I think I'd just call it a page banner.

Fair enough.

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_unittest.cc (right):

http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
chrome/browser/managed_mode_unittest.cc:46: DCHECK(!managed_profile_ ||
managed_profile_ == newly_managed_profile);
On 2012/06/19 13:01:04, Bernhard Bauer wrote:
> Could you ASSERT instead of DCHECK so that we fail instead of crashing?

Doh. Of course.

[Patrick Dubroy, 2012-06-20 09:11:34]

HTML, JS, & CSS parts LGTM.

On 2012/06/19 20:37:45, Pam wrote:
> Thanks again. Pat, please take another look.
> 
> - Pam
> 
>
http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
> File chrome/browser/resources/options2/chromeos/internet_detail.html (right):
> 
>
http://codereview.chromium.org/10542023/diff/12037/chrome/browser/resources/o...
> chrome/browser/resources/options2/chromeos/internet_detail.html:457: <span
> id="banner-text" class="main-page-banner-text"></span>
> On 2012/06/19 13:01:52, dubroy wrote:
> > IMO "main-page-banner" is still not the best name. Is it the banner of the
> main
> > page, or the main banner of the page? I think I'd just call it a page
banner.
> 
> Fair enough.
> 
>
http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
> File chrome/browser/managed_mode_unittest.cc (right):
> 
>
http://codereview.chromium.org/10542023/diff/38001/chrome/browser/managed_mod...
> chrome/browser/managed_mode_unittest.cc:46: DCHECK(!managed_profile_ ||
> managed_profile_ == newly_managed_profile);
> On 2012/06/19 13:01:04, Bernhard Bauer wrote:
> > Could you ASSERT instead of DCHECK so that we fail instead of crashing?
> 
> Doh. Of course.

[Pam (message me for reviews), 2012-06-20 20:56:51]

Evan, please review for OWNERS approval. Specifically,

chrome/browser/resources/options2/*
chrome/browser/ui/webui/*
chrome/browser/resources/*

Thanks,
- Pam

[Evan Stade, 2012-06-20 21:27:35]

lgtm

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.html (right):

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.html:49:
i18n-content="extensionSettingsManagedMode"></span>
4 indent

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
File chrome/browser/resources/uber/uber_shared.css (right):

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
chrome/browser/resources/uber/uber_shared.css:122: */
move */ to previous line.

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
chrome/browser/resources/uber/uber_shared.css:153: background-image:
url('chrome://theme/IDR_MANAGED');
I'm not sure that every page installs a theme data source, meaning this might
not work if you navigated to some uber pages without first navigating to the NTP
(or any page that installs the correct data source). All the more reason we
should move theme data source registration to profile startup.

[Pam (message me for reviews), 2012-06-21 16:38:13]

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
File chrome/browser/resources/uber/uber_shared.css (right):

http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
chrome/browser/resources/uber/uber_shared.css:153: background-image:
url('chrome://theme/IDR_MANAGED');
On 2012/06/20 21:27:35, Evan Stade wrote:
> I'm not sure that every page installs a theme data source, meaning this might
> not work if you navigated to some uber pages without first navigating to the
NTP
> (or any page that installs the correct data source). All the more reason we
> should move theme data source registration to profile startup.

Currently it's only used on the Settings and Extensions pages, which appear to
install theme data. Judging from your LGTM, that's sufficient for now?

[Evan Stade, 2012-06-21 18:35:43]

On 2012/06/21 16:38:13, Pam wrote:
>
http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
> File chrome/browser/resources/uber/uber_shared.css (right):
> 
>
http://codereview.chromium.org/10542023/diff/35006/chrome/browser/resources/u...
> chrome/browser/resources/uber/uber_shared.css:153: background-image:
> url('chrome://theme/IDR_MANAGED');
> On 2012/06/20 21:27:35, Evan Stade wrote:
> > I'm not sure that every page installs a theme data source, meaning this
might
> > not work if you navigated to some uber pages without first navigating to the
> NTP
> > (or any page that installs the correct data source). All the more reason we
> > should move theme data source registration to profile startup.
> 
> Currently it's only used on the Settings and Extensions pages, which appear to
> install theme data. Judging from your LGTM, that's sufficient for now?

yea, sufficient for now. You could make uber ui install a theme data source but
I hope to find time to fix it the better way sometime soon.

[commit-bot: I haz the power, 2012-06-21 20:20:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10542023/57001

[commit-bot: I haz the power, 2012-06-21 21:06:27]

Try job failure for 10542023-57001 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-06-25 17:28:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pam@chromium.org/10542023/57001

[commit-bot: I haz the power, 2012-06-25 18:28:27]

Change committed as 143960