Add integration browser tests for settings hardening.

Add integration browser tests for settings hardening.

This CL introduces the PrefHashBrowserTestBase fixture which these tests are based on.

This fixture (in conjunction with the new PREF_HASH_BROWSER_TEST macro) provides an easy way to override 3 methods in order to:
 1) Setup Chrome as desired in a PRE_ test.
 2) Attack Preferences while Chrome isn't running.
 3) Relaunch Chrome and verify reaction to attacks.

The fixture+macro also parametrizes every test such that they are ran in every SettingsEnforcement trial group.

A few pieces were resurrected from the old PrefHashBrowserTest used to test unloaded profile seeding prior to http://crrev.com/277209

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

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

[gab, 2014-07-31 13:19:14]

Erik PTAL, I'm still going to add at least one more test, but I'd like you to
take a look at the fixture and general structure of these tests.

Thanks!
Gab

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:62: EXPECT_EQ(0,
count_for_id) << "Faulty reporting_id: " << i;
Please ignore this for now.

I'm using it for debugging, but I'll most likely change how this works before
the last patch set.

[erikwright (departed), 2014-07-31 14:38:27]

structure looks good.

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:337:
JSONFileValueSerializer serializer(
Presumably this stanza will appear frequently.

What if the caller of this method passed in two
scoped_ptr<base::DictionaryValue>* for normal and secure prefs?

You could edit them, clear them, or replace them, and then the caller would
write them to disk.

[gab, 2014-07-31 19:14:16]

Thanks, done.

Adding another test now, but PTAL at the existing ones.

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/20001/chrome/browser/prefs/tra...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:337:
JSONFileValueSerializer serializer(
On 2014/07/31 14:38:26, erikwright wrote:
> Presumably this stanza will appear frequently.
> 
> What if the caller of this method passed in two
> scoped_ptr<base::DictionaryValue>* for normal and secure prefs?
> 
> You could edit them, clear them, or replace them, and then the caller would
> write them to disk.

Great idea, done (didn't use scoped_ptr*'s in the AttackPreferencesOnDisk() call
though as I don't intend to go as far as deleting/creating the file if it DNE
yet).

[gab, 2014-07-31 22:09:17]

All tests added, ready for full review!!

Thanks!
Gab

[gab, 2014-08-01 13:53:50]

CC Bernhard, FYI I intend to land this on Erik's sole review, let me know if
you'd like an explicit say.

Cheers,
Gab

[Bernhard Bauer, 2014-08-01 14:57:47]

This is very nice! I just found some nits below:

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:161:
kSettingsEnforcementGroupEnforceAlwaysWithExtensionsAndDSE) {
This doesn't fit into 80 columns anymore... Some things you could do to improve
this:
* Early-return at the beginning if the platform doesn't support tracking
* Move the whole block into a method that returns the protection level, then
early-return instead of concatenating else-ifs
* Use `using chrome_prefs::internals::...;`

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:167: // Protection levels
can't be adjusted via --force-fieldtrials in official
Indent is off

[Bernhard Bauer, 2014-08-01 15:04:19]

This is very nice! I just found some nits below:

[gab, 2014-08-01 17:52:21]

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:161:
kSettingsEnforcementGroupEnforceAlwaysWithExtensionsAndDSE) {
On 2014/08/01 14:57:46, Bernhard Bauer wrote:
> This doesn't fit into 80 columns anymore... Some things you could do to
improve
> this:
> * Early-return at the beginning if the platform doesn't support tracking
> * Move the whole block into a method that returns the protection level, then
> early-return instead of concatenating else-ifs
> * Use `using chrome_prefs::internals::...;` 

Done.

https://codereview.chromium.org/431973002/diff/120001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:167: // Protection levels
can't be adjusted via --force-fieldtrials in official
On 2014/08/01 14:57:47, Bernhard Bauer wrote:
> Indent is off

I think comments connected to ifdef's go at the same indent as the ifdef's
themselves, no?

[gab, 2014-08-01 18:25:28]

CC grt who is interested in looking into how this test fixture works :-)!

[erikwright (departed), 2014-08-02 07:16:52]

LGTM with some suggestions.

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:57: ALLOW_MANY
MANY -> ANY?

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:285: int
num_tracked_prefs_;
this should be private with an accessor. It doesn't make sense for tests to
change it.

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:287:
SettingsProtectionLevel protection_level_;
At a minimum, const. But an accessor is probably better.

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:307: // The strongest mode
is enforced on Windows in the absence of a field
Instead of this, how about ifdef'ing out the instantiations of the test with the
values that won't work in official builds?

[gab, 2014-08-04 18:53:10]

Done, thanks, Bernhard feel free to add post-commits comments if you care.

Cheers!
Gab

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:57: ALLOW_MANY
On 2014/08/02 07:16:52, erikwright (OOO until Aug 12) wrote:
> MANY -> ANY?

Done.

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:285: int
num_tracked_prefs_;
On 2014/08/02 07:16:52, erikwright (OOO until Aug 12) wrote:
> this should be private with an accessor. It doesn't make sense for tests to
> change it.

Done.

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:287:
SettingsProtectionLevel protection_level_;
On 2014/08/02 07:16:52, erikwright (OOO until Aug 12) wrote:
> At a minimum, const. But an accessor is probably better.

I like const, but I won't add an accessor because the checks based on
|protection_level_| are already convoluted enough as-is...

https://codereview.chromium.org/431973002/diff/140001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:307: // The strongest mode
is enforced on Windows in the absence of a field
On 2014/08/02 07:16:52, erikwright (OOO until Aug 12) wrote:
> Instead of this, how about ifdef'ing out the instantiations of the test with
the
> values that won't work in official builds?

No, I explicitly want to test that --force-fieldtrials doesn't work for
SettingsEnforcement in Official builds; this is a required feature of settings
hardening.

[gab, 2014-08-04 18:53:20]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:55:44]

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

[commit-bot: I haz the power, 2014-08-04 22:07:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42244)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-04 22:15:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-04 22:15:06]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[gab, 2014-08-05 19:27:13]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-08-05 19:28:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/431973002/220001

[commit-bot: I haz the power, 2014-08-05 23:31:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42870)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 00:00:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 00:00:23]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42933)

[gab, 2014-08-06 02:56:10]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-08-06 02:57:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/431973002/220001

[commit-bot: I haz the power, 2014-08-06 05:48:28]

Change committed as 287689

[Adam Rice, 2014-08-06 07:05:01]

On 2014/08/06 05:48:28, I haz the power (commit-bot) wrote:
> Change committed as 287689

Reverted in 287717.

Appears to have broken the official Windows build:

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/bu...

[gab, 2014-08-06 15:01:08]

Fixed unreachable code error in official builds.

Re-landing.

[gab, 2014-08-06 15:01:13]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-08-06 15:01:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/431973002/240001

[commit-bot: I haz the power, 2014-08-06 18:03:50]

Change committed as 287805