Supervised users: When an extension requires new permissions, send request to custodian

Supervised users: When an extension requires new permissions, send request to custodian

BUG=397951
Committed: https://crrev.com/8ecc1eb5c39ec46c20e410195ef454d5116239c5
Cr-Commit-Position: refs/heads/master@{#319086}

[Marc Treib, 2015-03-02 16:04:00]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-03-02 16:04:01]

PTAL!
(I'll add an extensions owner once you're done.)

[Bernhard Bauer, 2015-03-02 16:41:56]

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:93: #if
defined(ENABLE_SUPERVISED_USERS)
Move this before the OS_CHROMEOS block, so they are sorted alphabetically?

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/permission_request_creator.h (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/permission_request_creator.h:33: virtual void
CreateExtensionUpdateRequest(
This isn't technically true, I think -- we update the extension unconditionally,
and the request is to re-enable it, no?

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:568: // Find a
permission request creator that is enabled.
Urr... it's not particularly nice to duplicate this code.

An alternative would be to have a single SendRequest method that can take
different kinds of requests. A request could either have polymorphism via
subclasses (more OO-like), or be a flat struct (less overhead).

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:260: const
SuccessCallback& callback) override {
Add a FAIL()?

[Marc Treib, 2015-03-02 18:26:00]

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:93: #if
defined(ENABLE_SUPERVISED_USERS)
On 2015/03/02 16:41:55, Bernhard Bauer wrote:
> Move this before the OS_CHROMEOS block, so they are sorted alphabetically?

Done.

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/permission_request_creator.h (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/permission_request_creator.h:33: virtual void
CreateExtensionUpdateRequest(
On 2015/03/02 16:41:56, Bernhard Bauer wrote:
> This isn't technically true, I think -- we update the extension
unconditionally,
> and the request is to re-enable it, no?

Hm, right. I guess "ReenableRequest" would be more accurate. But we're calling
this an update request everywhere else (all the docs, and the server-side event
type (PERMISSION_CHROME_CWS_ITEM_UPDATE)), so I'd prefer to keep it as is. I've
update the comment though.

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:568: // Find a
permission request creator that is enabled.
On 2015/03/02 16:41:56, Bernhard Bauer wrote:
> Urr... it's not particularly nice to duplicate this code.
> 
> An alternative would be to have a single SendRequest method that can take
> different kinds of requests. A request could either have polymorphism via
> subclasses (more OO-like), or be a flat struct (less overhead).

Done.

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right):

https://codereview.chromium.org/971733003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:260: const
SuccessCallback& callback) override {
On 2015/03/02 16:41:56, Bernhard Bauer wrote:
> Add a FAIL()?

Done.

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:584:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&helper),
This is ugly: The base::Passed nulls the helper, but apparently the
helper->SendPermissionRequest is resolved first. Don't know if that's
well-defined though - do you know? I guess the alternative would be to grab a
raw pointer to the helper before the Bind...

[Bernhard Bauer, 2015-03-02 23:09:59]

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:232: virtual void
SendPermissionRequest(PermissionRequestCreator* creator,
Nice! Now that I see this, I'm wondering though whether this would be even
cleaner with callbacks? That would remove a lot of the boilerplate here.

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:584:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&helper),
On 2015/03/02 18:26:00, Marc Treib wrote:
> This is ugly: The base::Passed nulls the helper, but apparently the
> helper->SendPermissionRequest is resolved first. Don't know if that's
> well-defined though - do you know? I guess the alternative would be to grab a
> raw pointer to the helper before the Bind...

Wait, how would we call the method before its arguments are fully resolved? Or
do we dereference the scoped_ptr, *then* resolve the arguments, then call it?
That seems indeed kind of scary to me, and yes, I would in that case get a raw
pointer (which really just makes the order explicit in which things happen).

[Marc Treib, 2015-03-03 10:04:50]

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:232: virtual void
SendPermissionRequest(PermissionRequestCreator* creator,
On 2015/03/02 23:09:59, Bernhard Bauer wrote:
> Nice! Now that I see this, I'm wondering though whether this would be even
> cleaner with callbacks? That would remove a lot of the boilerplate here.

You're right, it's much less boilerplate with callbacks. Done, thanks! (How
didn't I think of that?!)

https://codereview.chromium.org/971733003/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.cc:584:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&helper),
On 2015/03/02 23:09:59, Bernhard Bauer wrote:
> On 2015/03/02 18:26:00, Marc Treib wrote:
> > This is ugly: The base::Passed nulls the helper, but apparently the
> > helper->SendPermissionRequest is resolved first. Don't know if that's
> > well-defined though - do you know? I guess the alternative would be to grab
a
> > raw pointer to the helper before the Bind...
> 
> Wait, how would we call the method before its arguments are fully resolved? Or
> do we dereference the scoped_ptr, *then* resolve the arguments, then call it?
> That seems indeed kind of scary to me, and yes, I would in that case get a raw
> pointer (which really just makes the order explicit in which things happen).

Thinking about this again, it probably depends on the order of evaluation of
method arguments: "helper->" is basically the first argument, and apparently
that's evaluated before the "base::Passed(&helper)". That's not well-defined
though.
Luckily, the version with callbacks makes this unnecessary :)

[Bernhard Bauer, 2015-03-03 11:14:01]

LGTM

https://codereview.chromium.org/971733003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/971733003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:192: typedef
base::Callback<void(PermissionRequestCreator*,
Optional: you could use `using` instead of typedef.

[Marc Treib, 2015-03-03 11:32:20]

https://codereview.chromium.org/971733003/diff/60001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/971733003/diff/60001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_service.h:192: typedef
base::Callback<void(PermissionRequestCreator*,
On 2015/03/03 11:14:01, Bernhard Bauer wrote:
> Optional: you could use `using` instead of typedef.

Done. I've also updated the existing typedefs to using.

[Marc Treib, 2015-03-03 11:42:28]

treib@chromium.org changed reviewers:
+ kalman@chromium.org

[Marc Treib, 2015-03-03 11:42:29]

+kalman: Benjamin, can I get an OWNERS review for extension_service.cc please?
Thanks!

[not at google - send to devlin, 2015-03-03 17:51:15]

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1674: #if
defined(ENABLE_SUPERVISED_USERS)
What is stopping the existing logic - wherever that is - for kicking in and
showing the install dialogue?

Also I'd feel more comfortable with a test.

[Marc Treib, 2015-03-04 11:15:38]

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1674: #if
defined(ENABLE_SUPERVISED_USERS)
On 2015/03/03 17:51:15, kalman wrote:
> What is stopping the existing logic - wherever that is - for kicking in and
> showing the install dialogue?

I assume you meant the re-enable dialog? All the UI entry points to do that are
disabled (except for any ones that I missed), see
https://codereview.chromium.org/925563002/.

If you're really talking about install, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sup....

> Also I'd feel more comfortable with a test.

There already was an existing test for extension updates with increased
permissions in a supervised profile. I've updated it to also check that a
request is created.

[not at google - send to devlin, 2015-03-04 17:43:36]

lgtm

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/971733003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1674: #if
defined(ENABLE_SUPERVISED_USERS)
> I assume you meant the re-enable dialog? All the UI entry points to do that
are
> disabled (except for any ones that I missed), see
> https://codereview.chromium.org/925563002/.

Yeah sorry, I meant the re-enable.

It would be so very nice if all of this permissions flow were in a single place
(not in ExtensionService) so that it was more obvious that the dialogue won't
show up. We already have PermissionsUpdater, seems like a logical sort of place
to have it.

Oh well.

> There already was an existing test for extension updates with increased
> permissions in a supervised profile. I've updated it to also check that a
> request is created.

Thanks.

[Marc Treib, 2015-03-04 17:57:36]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-04 17:57:36]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/971733003/#ps100001
(title: "test")

[commit-bot: I haz the power, 2015-03-04 17:59:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/971733003/100001

[commit-bot: I haz the power, 2015-03-04 18:29:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-04 18:30:17]

Patchset 6 (id:??) landed as
https://crrev.com/8ecc1eb5c39ec46c20e410195ef454d5116239c5
Cr-Commit-Position: refs/heads/master@{#319086}