Extension white and force lists (set by policy) should have priority over auto-updated Google black…

Extension white and force lists (set by policy) should have priority over auto-updated Google blacklist.


BUG=93246
TEST=unit_tests

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

[qfel, 2012-08-07 12:06:56]

I wonder if we should use ManagementPolicy::Provider for Google blacklist -
right now it's a bit special cased. Any thoughts?

Also moved some helper static privates to anonymous namespace so they don't
pollute header files.

Please review
Ivan: chrome/browser/protector/protected_prefs_watcher.cc
Matt: chrome/browser/extensions/*

[Ivan Korotkov, 2012-08-07 13:26:40]

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/protector/p...
File chrome/browser/protector/protected_prefs_watcher.cc (left):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/protector/p...
chrome/browser/protector/protected_prefs_watcher.cc:203: // Direct access to the
extensions prefs is required becase ExtensionService
Did something change in the initialization order? Are you sure that now
ExtensionPrefs is always initialized at this moment?

[qfel, 2012-08-07 13:43:51]

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/protector/p...
File chrome/browser/protector/protected_prefs_watcher.cc (left):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/protector/p...
chrome/browser/protector/protected_prefs_watcher.cc:203: // Direct access to the
extensions prefs is required becase ExtensionService
On 2012/08/07 13:26:40, Ivan Korotkov wrote:
> Did something change in the initialization order? Are you sure that now
> ExtensionPrefs is always initialized at this moment?

I guess I should have left the part about being uninitialized there. I just
moved the code below to ExtensionPrefs::GetExtensionsFrom (which is still a
static method), so ProtectedPrefsWatcher doesn't need to mess with extension
prefs directly (and GetExtensionsFrom uses PrefService now anyway). Nothing
changed in initialization order.

One thing I forgot to mention: now when there are no extension preferences set,
an empty set is returned (and still compared to cached_extension_ids_).
Previously it would always return false, even on non-empty 
cached_extension_ids_. Is that ok? I suppose no preference set at all should be
the same as empty dictionary, but here it was treated differently.

[Ivan Korotkov, 2012-08-07 14:20:47]

Right, my mistake. protector/ LGTM.
Please leave the comment, though.
Also, please ensure that protector.py autotests pass.

[Matt Perry, 2012-08-07 19:50:04]

lgtm

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:178: static bool
IsExtensionBlacklisted(const std::string& extension_id,
static methods should all be before non-static. you'll have to update the
comment, too.

[Mattias Nissler (ping if slow), 2012-08-08 08:08:37]

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:286: const std::string& pref_key,
int* out_value) {
all parameters on separate lines

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:586: (!forcelist ||
forcelist->Find(id) == forcelist->end());
Blacklisting has recently been abstracted into
chrome/browser/extensions/management_policy.h and we should use that for this as
well.

In fact, the main caller of this in crx_installer does the blacklist check and
the policy check right immediately after each other, so I suggest we consolidate
there.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1549: DictionaryValue::key_iterator
extension_id) const {
any reason to pass an iterator here instead of the id?

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1556: if
(IsExtensionBlacklisted(*extension_id))
so you get the extension dictionary passed, and you still make another lookup?
That doesn't seem right.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1132: if
(extension_prefs_->IsExtensionBlacklisted(extension->id())) {
should call into extensions::ManagementPolicy here.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1178:
extension_prefs_->IsExtensionBlacklisted(extension->id())) {
same here.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:2808: // Make sure pref
is updated
Missing period.
Also, pref read writes don't require message loop iterations. Can you clarify
what actually does require it?

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:2865: // then
blacklist).
I'm not sure we really need to test different order here.

[qfel, 2012-08-13 16:03:01]

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:286: const std::string& pref_key,
int* out_value) {
On 2012/08/08 08:08:37, Mattias Nissler wrote:
> all parameters on separate lines

Done.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1549: DictionaryValue::key_iterator
extension_id) const {
On 2012/08/08 08:08:37, Mattias Nissler wrote:
> any reason to pass an iterator here instead of the id?

I guess the original author did it mechanically when factoring out common code,
but doesn't seem to serve a deeper purpose. Changed.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.h:178: static bool
IsExtensionBlacklisted(const std::string& extension_id,
On 2012/08/07 19:50:04, Matt Perry wrote:
> static methods should all be before non-static. you'll have to update the
> comment, too.

Done.

Is this some kind of extension-code-specific rule?

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:2808: // Make sure pref
is updated
On 2012/08/08 08:08:37, Mattias Nissler wrote:
> Missing period.
> Also, pref read writes don't require message loop iterations. Can you clarify
> what actually does require it?

Uh, I was going to ask for the same, and forgot to put it in the message.

http://codereview.chromium.org/10854009/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:2865: // then
blacklist).
On 2012/08/08 08:08:37, Mattias Nissler wrote:
> I'm not sure we really need to test different order here.

Considering you knowing the implementation, it's a bit biased statement:)
If it was more callback-based (as opposed to current read on demand) it would be
quite ease to get one ordering right and don't refresh on the other.

http://codereview.chromium.org/10854009/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/admin_policy.cc (right):

http://codereview.chromium.org/10854009/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/admin_policy.cc:65: (!blacklist ||
blacklist->Find(id_value) == blacklist->end()) &&
Checking for NULL blacklist is redundant here, but it's not very clear at the
first sight so I'm leaving it here.

http://codereview.chromium.org/10854009/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1503: return NULL;
I'm not really sure if that's needed here, but I want other changes reviewed
before I take a look at it. And it's checked in unit tests.

http://codereview.chromium.org/10854009/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1528: path =
install_directory_.Append(path).value();
That code comes from MakePathsAbsolute (which I removed as it's no longer used).

[Mattias Nissler (ping if slow), 2012-08-13 16:54:53]

I like this version much better, LGTM from my side.

Matt, can you take another look at the extensions stuff (changed significantly)?

Also adding Pam since she might have an opinion as the original author of the
black/whitelisting refactor.

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/a...
File chrome/browser/extensions/admin_policy_unittest.cc (right):

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/a...
chrome/browser/extensions/admin_policy_unittest.cc:59: // Required extensions
may load even if they're on the blacklist.
add tests here and below to make sure the admin blacklist works as expected?

[qfel, 2012-08-14 13:03:13]

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1774: result.push_back(*it);
Now it returns Google-blacklisted extensions too. IMO that should be fine (it
did include admin-blacklisted extensions, so now it's more uniform), but this
behavior makes ExtensionPrefChange unittest crash (see
protected_prefs_watcher_unittest.cc, the very end of the test). I think we
should remove that part about blacklist update triggering backup change. WDYT?

[Mattias Nissler (ping if slow), 2012-08-14 14:12:30]

Matt, can you answer the question inline?

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1774: result.push_back(*it);
Btw, indentation is wrong here. Please fix.

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1774: result.push_back(*it);
On 2012/08/14 13:03:14, qfel wrote:
> Now it returns Google-blacklisted extensions too. IMO that should be fine (it
> did include admin-blacklisted extensions, so now it's more uniform), but this
> behavior makes ExtensionPrefChange unittest crash (see
> protected_prefs_watcher_unittest.cc, the very end of the test). I think we
> should remove that part about blacklist update triggering backup change. WDYT?

I'm not so sure about this. Isn't the blacklist bit in the extension prefs a
different thing? I'm not sure when it's present and whit its semantics are.
Matt, can you clarify?

[Matt Perry, 2012-08-15 21:17:07]

lgtm

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10854009/diff/2020/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1774: result.push_back(*it);
On 2012/08/14 14:12:31, Mattias Nissler wrote:
> On 2012/08/14 13:03:14, qfel wrote:
> > Now it returns Google-blacklisted extensions too. IMO that should be fine
(it
> > did include admin-blacklisted extensions, so now it's more uniform), but
this
> > behavior makes ExtensionPrefChange unittest crash (see
> > protected_prefs_watcher_unittest.cc, the very end of the test). I think we
> > should remove that part about blacklist update triggering backup change.
WDYT?
> 
> I'm not so sure about this. Isn't the blacklist bit in the extension prefs a
> different thing? I'm not sure when it's present and whit its semantics are.
> Matt, can you clarify?

I'm not an expert here, but judging by the use case of GetExtensionsFrom, it
makes sense to return the google-blacklisted extensions too. The only caller is
the ProtectedPrefsWatcher, who wants to keep the prefs from changing externally.

[qfel, 2012-08-16 09:33:34]

On 2012/08/07 14:20:47, Ivan Korotkov wrote:
> Right, my mistake. protector/ LGTM.
> Please leave the comment, though.
> Also, please ensure that protector.py autotests pass.

It passes.

Ivan, please take a look at ExtensionPrefChange unit test (the last part,
"blacklisting the extension does update the backup and signature").
Should I remove it?
If I understand correctly, I'll also need to update MigrateOldBackupIfNeeded
(+test) so it takes care of adding blacklisted extension ids to backup.

[Ivan Korotkov, 2012-08-17 14:54:42]

As discussed in chat, let's restore the original behaviour of GetExtensionsFrom
as ProtectedPrefsWatcher is its only client.

[commit-bot: I haz the power, 2012-08-20 14:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10854009/14020

[commit-bot: I haz the power, 2012-08-20 14:40:12]

Failed to apply patch for chrome/browser/extensions/extension_prefs.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_prefs.cc
Hunk #1 succeeded at 294 with fuzz 1 (offset 14 lines).
Hunk #2 succeeded at 407 (offset 14 lines).
Hunk #3 succeeded at 417 (offset 14 lines).
Hunk #4 succeeded at 579 (offset 52 lines).
Hunk #5 succeeded at 672 (offset 52 lines).
Hunk #6 succeeded at 1579 (offset 87 lines).
Hunk #7 succeeded at 1595 (offset 87 lines).
Hunk #8 succeeded at 1626 (offset 87 lines).
Hunk #9 succeeded at 1654 (offset 87 lines).
Hunk #10 FAILED at 1758.
1 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_prefs.cc.rej

[commit-bot: I haz the power, 2012-08-21 08:05:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10854009/24

[commit-bot: I haz the power, 2012-08-21 08:58:43]

Try job failure for 10854009-24 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-21 11:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qfel@google.com/10854009/24

[commit-bot: I haz the power, 2012-08-21 12:40:44]

Change committed as 152548