Allow re-enabling of DISABLE_NOT_VERIFIED extensions if appropriate

Allow re-enabling of DISABLE_NOT_VERIFIED extensions if appropriate

This fixes a problem where once an extension gets disabled because the
webstore verification check fails, there was no way to re-enable it. Now
we'll re-run the verification check and if it succeeds then we can
proceed with the re-enabling.

BUG=321230
R=finnur@chromium.org

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

[Finnur, 2013-12-11 17:28:20]

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:900: extension_id));
I'm worried this will slam the webstore with lots of requests from users. Is
that worry warranted? We have a large user base and almost all of the time this
is just going to waste CPU cycles on the server (I could imagine a lot of devs
are not going to bother migrating to the store). This is also not
time-sensitive, per se. Should we do something here to address this? What do you
think?

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2805:
!policy->MustRemainDisabled(extension, NULL, NULL)) {
We're guaranteed to have this check in other locations too, right? Shouldn't
there be one chokepoint to make sure they don't get out of sync?

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2807: }
nit: Single line if body, no braces.

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:708: void
EnableOnVerifySuccess(std::string extension_id, bool verify_success);
nit: const std::string&  ?

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_list.js (left):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_list.js:92:
node.classList.add('may-not-modify');
Wait... why is this necessary? Doesn't it undo what I did for the
chrome://extensions page (prevent the user from enabling)?

[asargent_no_longer_on_chrome, 2013-12-11 21:26:05]

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:900: extension_id));
On 2013/12/11 17:28:20, Finnur wrote:
> I'm worried this will slam the webstore with lots of requests from users. Is
> that worry warranted? We have a large user base and almost all of the time
this
> is just going to waste CPU cycles on the server (I could imagine a lot of devs
> are not going to bother migrating to the store). This is also not
> time-sensitive, per se. Should we do something here to address this? What do
you
> think?

I think this should only happen as a result of the user manually trying to
re-enable, since the API to do this in management_api.cc already early-returns
by checking the policy MustRemainDisabled function. 

I suppose we could rate-limit it somehow, but my guess is that most people would
try checking the "enable" checkbox a few times, and if it didn't work give up.

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2805:
!policy->MustRemainDisabled(extension, NULL, NULL)) {
On 2013/12/11 17:28:20, Finnur wrote:
> We're guaranteed to have this check in other locations too, right? Shouldn't
> there be one chokepoint to make sure they don't get out of sync?

Yeah, I agree this is a little awkward. The problem I was trying to solve is
that the code above in EnableExtension tests "MustRemainDisabled", and if that
comes back as DISABLE_NOT_VERIFIED then it kicks off the verification test that
gets us into this function. If |verify_success| is true then when we call
EnableExtension again it *shouldn't* be the case that the MustRemainDisabled
check it repeats will return DISABLE_NOT_VERIFIED again, but I was worried about
a bug creating an infinite loop of requests here. 

Does that make sense (and should I add a comment here to this effect)? Or do you
think I should be more explicit and refactor things so I have a way to call into
the same logic as EnableExtension but eliminating the possibility of generating
another verification attempt?

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2807: }
On 2013/12/11 17:28:20, Finnur wrote:
> nit: Single line if body, no braces.

Done.

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:708: void
EnableOnVerifySuccess(std::string extension_id, bool verify_success);
On 2013/12/11 17:28:20, Finnur wrote:
> nit: const std::string&  ?

Done.

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_list.js (left):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_list.js:92:
node.classList.add('may-not-modify');
On 2013/12/11 17:28:20, Finnur wrote:
> Wait... why is this necessary? Doesn't it undo what I did for the
> chrome://extensions page (prevent the user from enabling)?

The problem is that if for some reason we got the wrong answer from the
verification check (say the server was down or had a momentary hiccup) and an
extension ends up in the DISABLE_NOT_VERIFIED state, there isn't any way for the
user to ever force the system to retry. Their only option would be to uninstall
and reinstall, but this could lose saved state that they care about. 

With the changes in this CL, if they click the enable checkbox, we'll go off and
do another verification check, and if that succeeds the extension will be
re-enabled. It might be nice to eventually show some sort of spinner or
something, but for now I wanted to at least make this possible.

[Finnur, 2013-12-11 21:46:31]

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:900: extension_id));
Does the only codepath through this function involve the Enable checkbox on the
Extension Settings page?

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2805:
!policy->MustRemainDisabled(extension, NULL, NULL)) {
As discussed in person I had a concern that we have the same if checks in
multiple locations and that they might get out of sync.

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_list.js (left):

https://codereview.chromium.org/93513006/diff/40001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_list.js:92:
node.classList.add('may-not-modify');
OK. For anyone reading at home we had a hangout where we discussed asking Sriram
if he has any good ideas on how to improve the UX of failing to disable an
extension (because it hasn't been migrated).

[asargent_no_longer_on_chrome, 2013-12-14 00:42:38]

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/install_verifier.cc (left):

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.cc:356: not_allowed.size());
note: I removed this block of code because it was not correct and the histogram
would be misleading - it's possible the operation could have been to remove
operation->ids() from the signature, not add them.

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.cc:359:
operation->callback.Run(not_allowed.empty());
This got updated to return the success of the network request instead of whether
we thought we properly signed all of the ids that may have been requested to be
added. 

It turns out callers are more interested in the network request success/failure.

[asargent_no_longer_on_chrome, 2013-12-14 00:44:06]

Forgot to mention that I also added the code we discussed to avoid immediately
disabling on first run in verification mode.

[asargent_no_longer_on_chrome, 2013-12-16 06:54:53]

Switching reviewer to rockot. Ken, can you review since Finnur is in transit
now?

[Finnur, 2013-12-16 14:21:51]

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

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:628: // interface for
DisableReason's changing.
Log a bug for this?

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.cc:183: (signature_.get() == NULL ||
IsVerified(extension->id())) &&
Wait... it is verified if there's no signature? Where you testing something or
is this actually intended? If so, document why this makes sense...

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.h:48: // Do we need to be
bootstrapped? (Ie do we have a signature already)
nit: s/Ie/i.e.
End with period.

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

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:740: }
Does this result in HandleRequestExtensionsData getting called again? Or how
does the status change of an extension get reflected? Does the user have to
refresh the page to see the change?

[asargent_no_longer_on_chrome, 2013-12-16 18:47:09]

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

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:628: // interface for
DisableReason's changing.
On 2013/12/16 14:21:51, Finnur wrote:
> Log a bug for this?

Done.

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.cc:183: (signature_.get() == NULL ||
IsVerified(extension->id())) &&
On 2013/12/16 14:21:51, Finnur wrote:
> Wait... it is verified if there's no signature? Where you testing something or
> is this actually intended? If so, document why this makes sense...

It's temporarily allowed - the idea is that if someone gets thrown into
enforcement mode but has never done a check to the server before, we want to
hold off on disabling their extensions until we can successfully contact the
server once. That's what the "NeedsBootstrap" method above is for. 

Anyhow, I'll add a comment. (We may eventually need to make this more strict to
prevent abuse since there is an obvious attack vector here, but in the short
term we want to avoid the 100% false positive rate for existing profiles)

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/install_verifier.h:48: // Do we need to be
bootstrapped? (Ie do we have a signature already)
On 2013/12/16 14:21:51, Finnur wrote:
> nit: s/Ie/i.e.
> End with period.

Done.

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

https://codereview.chromium.org/93513006/diff/80001/chrome/browser/ui/webui/e...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:740: }
On 2013/12/16 14:21:51, Finnur wrote:
> Does this result in HandleRequestExtensionsData getting called again? Or how
> does the status change of an extension get reflected? Does the user have to
> refresh the page to see the change?

VerifyAllExtensions will post a task to talk to the server, and that comes back
asynchronously to be handled in ExtensionService::FinishVerifyAllExtensions.
Inside there, if any extensions that are currently disabled with
DISABLE_NOT_VERIFIED actually could be verified, then I send the
NOTIFICATION_EXTENSION_UNLOADED notification which will cause this object to
notice the change and make it possible for the user to enable the item again.
(without them having to manually refresh the page)

[Finnur, 2013-12-16 21:42:07]

LGTM, with nits.

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:130: // Returns the Extension of
an extension from a given url or NULL if the url
The Extension of an extension? We must go deeper! :)

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
chrome/browser/extensions/install_verifier.cc:31: ENFORCE = 2     // Request
install signatures, and enforce them.
Usually =0 is specified but we leave out the rest.
Also use:
ENFORCE, (for easier diffing later)

[asargent_no_longer_on_chrome, 2013-12-16 22:01:58]

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:130: // Returns the Extension of
an extension from a given url or NULL if the url
On 2013/12/16 21:42:08, Finnur wrote:
> The Extension of an extension? We must go deeper! :)

I actually picked up this change from someone else's CL since I did a merge from
trunk (sorry, I should have waited until end of review to do this). But I fixed
it anyway.

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/93513006/diff/140001/chrome/browser/extension...
chrome/browser/extensions/install_verifier.cc:31: ENFORCE = 2     // Request
install signatures, and enforce them.
On 2013/12/16 21:42:08, Finnur wrote:
> Usually =0 is specified but we leave out the rest.
> Also use:
> ENFORCE, (for easier diffing later)

Done.

[commit-bot: I haz the power, 2013-12-16 22:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/93513006/160001

[commit-bot: I haz the power, 2013-12-17 00:24:53]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 01:55:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/93513006/160001

[asargent_no_longer_on_chrome, 2013-12-17 02:41:58]

Committed patchset #6 manually as r241119 (presubmit successful).