Add managed user lock/unlock button for the extensions page.

Add managed user lock/unlock button for the extensions page. When a managed user opens the extensions page, he is not allowed to change any extension settings. The unlock button can be used to make the extensions editable. The lock button is used to revert back to the locked state.
Screenshots can be found here:
https://docs.google.com/a/google.com/folder/d/0BzMCIwDgeNezQjdabHVUUWtjRUU/edit?usp=sharing

BUG=171370
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183907

[Adrian Kuegel, 2013-02-19 17:29:51]

Bernhard, can you please review the files in chrome/browser/managed_mode?
Chris, can you please review the other files?

I have put a TODO in the code for adding a passphrase dialog. This will be done
in a follow-up changelist as soon as the code for the passphrase dialog has
landed.

[Bernhard Bauer, 2013-02-19 18:19:16]

managed_mode LGTM, one CSS nit:

https://codereview.chromium.org/12288052/diff/1/chrome/browser/resources/exte...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/12288052/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extensions.css:312: .profile-is-managed
.extension-commands-config {
You can combine all of these selectors for display: none.

[csilv, 2013-02-20 01:28:44]

lgtm for webui style.

I strongly urge you get an extensions owner review in addition to mine.  See:
chrome/browser/resources/extensions/OWNERS

[Adrian Kuegel, 2013-02-20 07:09:21]

Benjamin, could I please get OWNERS approval for the files in
chrome/browser/resources/extensions?

[not at google - send to devlin, 2013-02-21 01:29:18]

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.css:302: .profile-is-managed
.extension-commands-config,
This confused me for a while... it's orthogonal?

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.css:305: .profile-is-managed
#lock-button {
This last line doesn't make sense to me, and it's probably to do with the class
naming thing I said in the JS... generally I can't follow how you decide to show
lock or unlock.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:90: $('unlock-button').onclick
= function() {
.addEventListener('click', function(){...})

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:94: $('lock-button').onclick =
function() {
ditto

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:266: }
I find this more confusing than it needs to be, I think because the
'profile-is-managed' class is now sometimes not present when the profile is
managed.

So that class needs to be forked, like developer-mode-allowed or something. Then
you can use profile-is-managed class instead of adding the
no-managed-user-controls class.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:382:
base::Unretained(this)));
Can we combine these into a single message like setElevated or something?

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:619:
results.SetBoolean("profileIsElevated", service->IsElevated());
is_elevated?

[not at google - send to devlin, 2013-02-21 03:37:46]

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.css:302: .profile-is-managed
.extension-commands-config,
On 2013/02/21 01:29:18, kalman wrote:
> This confused me for a while... it's orthogonal?

Besides which, why hide the commands config in managed mode?

[Adrian Kuegel, 2013-02-21 10:10:54]

I uploaded a new patch which addresses the review comments.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.css:302: .profile-is-managed
.extension-commands-config,
Yes, you are right. Sorry, I didn't notice it before. I guess the commands
config does not really have to be hidden, since it only allows to configure how
to use an extension, so I removed this line now.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.css:305: .profile-is-managed
#lock-button {
The logic is as follows: The lock button is visible, if the managed user is in
elevated state (which means the manager of the account authenticated from within
the managed account). The unlock button is visible if the managed user is not in
elevated state. And there should be no button at all if the profile does not
belong to a managed user.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:90: $('unlock-button').onclick
= function() {
On 2013/02/21 01:29:18, kalman wrote:
> .addEventListener('click', function(){...})

Done.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:94: $('lock-button').onclick =
function() {
On 2013/02/21 01:29:18, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:266: }
I agree, using profile-is-managed instead of no-managed-user-controls and
another class for managed user, not elevated is certainly clearer. I used
managed-user-locked as class name now, I hope that is ok.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:382:
base::Unretained(this)));
Yes, true. I changed it like that.

https://codereview.chromium.org/12288052/diff/7001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:619:
results.SetBoolean("profileIsElevated", service->IsElevated());
The logic in the IsElevated method was not final yet. After the rebase to ToT
you can see now that it is not just returning the is_elevated variable, so I
think naming the method IsElevated should be ok.

[not at google - send to devlin, 2013-02-21 16:25:24]

lgtm

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
File chrome/browser/resources/extensions/extensions.css (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
chrome/browser/resources/extensions/extensions.css:304: .managed-user-locked
#lock-button {
cool, that is much clearer. thanks!

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
chrome/browser/resources/extensions/extensions.js:256:
pageDiv.classList.add('profile-is-managed');
does this need an else: classList.remove('profile-is-managed')?

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
args->GetBoolean(0, &elevated);
CHECK(args->GetBoolean(..))

[commit-bot: I haz the power, 2013-02-21 16:38:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12288052/20001

[Adrian Kuegel, 2013-02-21 16:39:00]

Thanks for the reviews.

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
File chrome/browser/resources/extensions/extensions.js (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/resources/...
chrome/browser/resources/extensions/extensions.js:256:
pageDiv.classList.add('profile-is-managed');
Currently I think the profile is put in managed state at creation time and it
cannot be changed later into a non-managed profile. Anyway, I will add it, it
doesn't hurt and is possibly less confusing.

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
args->GetBoolean(0, &elevated);
On 2013/02/21 16:25:24, kalman wrote:
> CHECK(args->GetBoolean(..))

Done.

[Bernhard Bauer, 2013-02-21 17:35:40]

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
args->GetBoolean(0, &elevated);
On 2013/02/21 16:25:24, kalman wrote:
> CHECK(args->GetBoolean(..))

Do we really want a CHECK for untrusted input? It would mean that a compromised
renderer could crash the browser (cleanly, but it would still be a DOS attack).

[not at google - send to devlin, 2013-02-21 17:37:18]

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
args->GetBoolean(0, &elevated);
On 2013/02/21 17:35:40, Bernhard Bauer wrote:
> On 2013/02/21 16:25:24, kalman wrote:
> > CHECK(args->GetBoolean(..))
> 
> Do we really want a CHECK for untrusted input? It would mean that a
compromised
> renderer could crash the browser (cleanly, but it would still be a DOS
attack).

it's chrome.send, that's not untrusted.

[Bernhard Bauer, 2013-02-21 17:56:20]

On 2013/02/21 17:37:18, kalman wrote:
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):
> 
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
> args->GetBoolean(0, &elevated);
> On 2013/02/21 17:35:40, Bernhard Bauer wrote:
> > On 2013/02/21 16:25:24, kalman wrote:
> > > CHECK(args->GetBoolean(..))
> > 
> > Do we really want a CHECK for untrusted input? It would mean that a
> compromised
> > renderer could crash the browser (cleanly, but it would still be a DOS
> attack).
> 
> it's chrome.send, that's not untrusted.

I'm not so sure about that. There have multiple examples in the past where
malicious code was run in a WebUI renderer (usually via a UXSS, I think). Now
granted, a malicious WebUI JS context can already do many Bad Things, but I
think for additional hardening it's still a good idea not to trust messages from
*any* renderer.

[not at google - send to devlin, 2013-02-21 17:59:55]

On 2013/02/21 17:56:20, Bernhard Bauer wrote:
> On 2013/02/21 17:37:18, kalman wrote:
> >
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> > File chrome/browser/ui/webui/extensions/extension_settings_handler.cc
(right):
> > 
> >
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> > chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
> > args->GetBoolean(0, &elevated);
> > On 2013/02/21 17:35:40, Bernhard Bauer wrote:
> > > On 2013/02/21 16:25:24, kalman wrote:
> > > > CHECK(args->GetBoolean(..))
> > > 
> > > Do we really want a CHECK for untrusted input? It would mean that a
> > compromised
> > > renderer could crash the browser (cleanly, but it would still be a DOS
> > attack).
> > 
> > it's chrome.send, that's not untrusted.
> 
> I'm not so sure about that. There have multiple examples in the past where
> malicious code was run in a WebUI renderer (usually via a UXSS, I think). Now
> granted, a malicious WebUI JS context can already do many Bad Things, but I
> think for additional hardening it's still a good idea not to trust messages
from
> *any* renderer.

Agreed, we can't/shouldn't trust webui. However, that ship has sailed. If you
look at the other functions here, they all CHECK - and CHECKing is much better
than the browser being in an unknown state. We could guard against it I suppose,
but, I think that CHECKing is still better.

Incidentally extensions solve this problem by killing the renderer which made
the bad call, I don't know whether webui has an idiom for the same thing. It
seems like not.

[Bernhard Bauer, 2013-02-21 18:13:35]

On 2013/02/21 17:59:55, kalman wrote:
> On 2013/02/21 17:56:20, Bernhard Bauer wrote:
> > On 2013/02/21 17:37:18, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> > > File chrome/browser/ui/webui/extensions/extension_settings_handler.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/12288052/diff/11002/chrome/browser/ui/webui/e...
> > > chrome/browser/ui/webui/extensions/extension_settings_handler.cc:551:
> > > args->GetBoolean(0, &elevated);
> > > On 2013/02/21 17:35:40, Bernhard Bauer wrote:
> > > > On 2013/02/21 16:25:24, kalman wrote:
> > > > > CHECK(args->GetBoolean(..))
> > > > 
> > > > Do we really want a CHECK for untrusted input? It would mean that a
> > > compromised
> > > > renderer could crash the browser (cleanly, but it would still be a DOS
> > > attack).
> > > 
> > > it's chrome.send, that's not untrusted.
> > 
> > I'm not so sure about that. There have multiple examples in the past where
> > malicious code was run in a WebUI renderer (usually via a UXSS, I think).
Now
> > granted, a malicious WebUI JS context can already do many Bad Things, but I
> > think for additional hardening it's still a good idea not to trust messages
> from
> > *any* renderer.
> 
> Agreed, we can't/shouldn't trust webui. However, that ship has sailed. If you
> look at the other functions here, they all CHECK - and CHECKing is much better
> than the browser being in an unknown state. We could guard against it I
suppose,
> but, I think that CHECKing is still better.

Hm, ok. For the sake of consistency, I'm willing to accept the CHECK :)

> Incidentally extensions solve this problem by killing the renderer which made
> the bad call, I don't know whether webui has an idiom for the same thing. It
> seems like not.

Yup, that would be my preferred solution as well, but like you said, we don't
have that for WebUI.

[commit-bot: I haz the power, 2013-02-21 22:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/12288052/20001

[commit-bot: I haz the power, 2013-02-21 23:54:38]

Change committed as 183907