Enable WebContents elevation for managed users.

Enable the new elevation for managed users which is tied to a specific WebContents. This replaces the old elevation which was tied to a profile.

BUG=222364
TEST=unit_tests,browser_tests


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

[Adrian Kuegel, 2013-03-27 15:21:25]

Bernhard, can you please review this changelist?

[Bernhard Bauer, 2013-03-27 15:48:14]

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_uninstall_dialog.cc:28: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
Include this unconditionally.

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

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:592:
service->AddElevationForExtension(extension->id());
Often when we have the need to balance calls like this, we add a stack-allocated
object that takes care of that when it is destroyed. In this case, it would be
something like  "ScopedExtensionElevation", and it would drop elevation when it
is destroyed.

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:46:
switches::kEnableManagedUsers)) {
Could you do some cleanup here and early-return if the condition is false?

[Adrian Kuegel, 2013-03-27 16:17:43]

I uploaded a new patch. I added the ScopedExtensionElevation class in an
anonymous namespace, since I am not sure if it can be used somewhere else, too.
What do you think?

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_uninstall_dialog.cc:28: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
On 2013/03/27 15:48:14, Bernhard Bauer wrote:
> Include this unconditionally.

Done.

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

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:592:
service->AddElevationForExtension(extension->id());
On 2013/03/27 15:48:14, Bernhard Bauer wrote:
> Often when we have the need to balance calls like this, we add a
stack-allocated
> object that takes care of that when it is destroyed. In this case, it would be
> something like  "ScopedExtensionElevation", and it would drop elevation when
it
> is destroyed.

Done.

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/managed_user_settings_handler.cc (right):

https://codereview.chromium.org/13119011/diff/2001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/managed_user_settings_handler.cc:46:
switches::kEnableManagedUsers)) {
On 2013/03/27 15:48:14, Bernhard Bauer wrote:
> Could you do some cleanup here and early-return if the condition is false?

Done.

[Bernhard Bauer, 2013-03-27 16:23:13]

On 2013/03/27 16:17:43, Adrian Kuegel wrote:
> I uploaded a new patch. I added the ScopedExtensionElevation class in an
> anonymous namespace, since I am not sure if it can be used somewhere else,
too.
> What do you think?

How about in managed_user_service_unittest.cc?

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

https://codereview.chromium.org/13119011/diff/2002/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:86: namespace {
Nit: newline

https://codereview.chromium.org/13119011/diff/2002/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:97: std::string
extension_id_;
You could have a vector of extension IDs and a method that adds elevation for
one extension. I think that would make the interface slightly nicer.

[Adrian Kuegel, 2013-03-27 16:52:25]

Ok, I extracted the new class in its own file, now it could be used somewhere
else as well. Currently it wouldn't really help, because the unit test does not
need a reset of the elevation (before it also just stayed in elevated mode until
the end). Of course I could change it, but since a reset is not tested, it would
probably only make sense if I add something to test that as well. But I guess
this could go in a separate unit test, which I can write once I finished the
unpacked_installer stuff.

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

https://codereview.chromium.org/13119011/diff/2002/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:97: std::string
extension_id_;
On 2013/03/27 16:23:13, Bernhard Bauer wrote:
> You could have a vector of extension IDs and a method that adds elevation for
> one extension. I think that would make the interface slightly nicer.

Done.

[Adrian Kuegel, 2013-03-28 15:29:11]

I added a check in the IsElevatedForWebContents method if the
ManagedModeNavigationObserver is NULL. In the other cases where I use it, I
think there should always be a ManagedModeNavigationObserver there (History
page, Extensions page and Managed user Settings page). Should I use DCHECKS
there?

[Bernhard Bauer, 2013-03-28 15:48:40]

On 2013/03/28 15:29:11, Adrian Kuegel wrote:
> I added a check in the IsElevatedForWebContents method if the
> ManagedModeNavigationObserver is NULL.

Do we even expect to call this on a WebContents that doesn't have a
ManagedModeNavigationObserver?

> In the other cases where I use it, I
> think there should always be a ManagedModeNavigationObserver there (History
> page, Extensions page and Managed user Settings page). Should I use DCHECKS
> there?

Meh, you'll crash in any case if it's NULL.

[Adrian Kuegel, 2013-03-28 15:51:49]

On 2013/03/28 15:48:40, Bernhard Bauer wrote:
> Do we even expect to call this on a WebContents that doesn't have a
> ManagedModeNavigationObserver?

Probably not, but at least in that case it is not obvious when reading the code
what kind of WebContents are used.

> > In the other cases where I use it, I
> > think there should always be a ManagedModeNavigationObserver there (History
> > page, Extensions page and Managed user Settings page). Should I use DCHECKS
> > there?
> 
> Meh, you'll crash in any case if it's NULL.

Ok, then I will leave it as it is.

[Bernhard Bauer, 2013-03-28 15:52:01]

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
File chrome/browser/managed_mode/scoped_extension_elevation.h (right):

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
chrome/browser/managed_mode/scoped_extension_elevation.h:21: void
AddExtension(std::string extension_id);
Pass |extension_id| by const reference.

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
chrome/browser/managed_mode/scoped_extension_elevation.h:25:
std::vector<std::string> elevated_extensions_;
DISALLOW_COPY_AND_ASSIGN please!

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

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:791:
scoped_ptr<ScopedExtensionElevation> elevation;
The nice thing is that now you can construct the object in any case, and only
add an extension if you need elevation for it.

[Adrian Kuegel, 2013-03-28 16:05:53]

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
File chrome/browser/managed_mode/scoped_extension_elevation.h (right):

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
chrome/browser/managed_mode/scoped_extension_elevation.h:21: void
AddExtension(std::string extension_id);
On 2013/03/28 15:52:01, Bernhard Bauer wrote:
> Pass |extension_id| by const reference.

Done.

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/managed_mo...
chrome/browser/managed_mode/scoped_extension_elevation.h:25:
std::vector<std::string> elevated_extensions_;
On 2013/03/28 15:52:01, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN please!

Done.

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

https://codereview.chromium.org/13119011/diff/10017/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:791:
scoped_ptr<ScopedExtensionElevation> elevation;
On 2013/03/28 15:52:01, Bernhard Bauer wrote:
> The nice thing is that now you can construct the object in any case, and only
> add an extension if you need elevation for it.

Done.

[Bernhard Bauer, 2013-03-28 16:09:59]

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

https://codereview.chromium.org/13119011/diff/26001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:589:
scoped_ptr<ScopedExtensionElevation> elevation(
Aaand now you can stack-allocate it.

[Adrian Kuegel, 2013-03-28 16:16:12]

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

https://codereview.chromium.org/13119011/diff/26001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:589:
scoped_ptr<ScopedExtensionElevation> elevation(
On 2013/03/28 16:09:59, Bernhard Bauer wrote:
> Aaand now you can stack-allocate it.

Done.

[Bernhard Bauer, 2013-04-02 11:35:51]

lgtm

[Adrian Kuegel, 2013-04-02 12:25:04]

@Benjamin: Can you please review my changes to the file
chrome/browser/extensions/extension_uninstall_dialog.cc, and the files in
chrome/browser/ui/webui/extensions?

@James: Can you please review the files in chrome/browser/ui/webui/*?

[James Hawkins, 2013-04-02 16:03:39]

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

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:788:
ManagedUserService* service = ManagedUserServiceFactory::GetForProfile(
This looks identical to the code block starting on line 585. Can you refactor
into a function?

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:140: // Called
to verify that the management policy allows the user to enable,
// Verifies that the...

[Adrian Kuegel, 2013-04-02 16:44:29]

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

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:788:
ManagedUserService* service = ManagedUserServiceFactory::GetForProfile(
On 2013/04/02 16:03:39, James Hawkins wrote:
> This looks identical to the code block starting on line 585. Can you refactor
> into a function?

Done.

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

https://codereview.chromium.org/13119011/diff/33001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:140: // Called
to verify that the management policy allows the user to enable,
On 2013/04/02 16:03:39, James Hawkins wrote:
> // Verifies that the...

Done.

[James Hawkins, 2013-04-02 16:47:46]

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

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:599:
scoped_ptr<ScopedExtensionElevation> elevation =
|elevation| is unused.

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:795:
scoped_ptr<ScopedExtensionElevation> elevation =
elevation is unused.

[not at google - send to devlin, 2013-04-02 21:55:25]

extensions lgtm

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/extension_uninstall_dialog.cc:164:
service->AddElevationForExtension(extension_->id());
It seems like this should be part of the interface to service, i.e.
RequestAuthorizationForExtension(...) rather than RequestAuthorization(...) then
not need this line, but not my call.

[Adrian Kuegel, 2013-04-03 08:30:25]

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

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:599:
scoped_ptr<ScopedExtensionElevation> elevation =
It is used indirectly. ScopedExtensionElevation gives elevation for the managed
user to modify or load extensions with certain ids. When it goes out of scope,
the elevation is removed automatically. Bernhard suggested a pattern like this,
because we need to do balanced actions here: first give elevation, and then
remove it after the call to the policy function (either UserMayLoad or
UserMayModifySettings) has happened. I guess I should put a comment in the code.

[Bernhard Bauer, 2013-04-03 09:00:17]

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

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:585:
ExtensionSettingsHandler::GetScopedElevation(std::string extension_id) {
Pass string parameters via const ref.

[Adrian Kuegel, 2013-04-03 09:42:40]

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

https://codereview.chromium.org/13119011/diff/41001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:585:
ExtensionSettingsHandler::GetScopedElevation(std::string extension_id) {
On 2013/04/03 09:00:17, Bernhard Bauer wrote:
> Pass string parameters via const ref.

Done.

[Adrian Kuegel, 2013-04-04 12:21:51]

James, could you please take another look?

[James Hawkins, 2013-04-04 17:13:32]

lgtm

[commit-bot: I haz the power, 2013-04-05 07:49:35]

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

[commit-bot: I haz the power, 2013-04-05 08:47:32]

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

[commit-bot: I haz the power, 2013-04-05 13:04:53]

Change committed as 192549