Update the super MAC after performing MAC validation.

Update the super MAC after performing MAC validation.

If, for any reason, the super MAC is missing or invalid but all individual MACs are valid, the super MAC would remain invalid. This could occur if the super MAC were deleted by an external process, but in the future it could also occur after the initial import of MACs from Local State to Protected Preferences. In that import we would not create a super MAC but each of the individual protected preferences would be valid. If none of the protected preferences changes during that execution we would not end up writing a new super MAC and a subsequent launch would not be able to "TrustedInitialize" newly protected values.

BUG=372547

[erikwright (departed), 2014-06-11 18:41:15]

gab: PTAL.

[erikwright (departed), 2014-06-11 18:41:35]

On 2014/06/11 18:41:15, erikwright wrote:
> gab: PTAL.

I'll add a test prior to commit.

[gab, 2014-06-11 19:44:01]

Actually do we even need this?

Previously we would have needed this when we originally shipped since we wanted
to seed a super MAC even when no MAC was modified.

But assuming we have a super MAC (which is a fine assumption for M37 -- even in
the new model since ImportHash() forces a new super MAC), then is it not
impossible that we need to generate a new super MAC yet no other hashes have
been modified (i.e., has_changed is still false)?

[gab, 2014-06-11 19:44:27]

On 2014/06/11 19:44:01, gab wrote:
> Actually do we even need this?
> 
> Previously we would have needed this when we originally shipped since we
wanted
> to seed a super MAC even when no MAC was modified.
> 
> But assuming we have a super MAC (which is a fine assumption for M37 -- even
in
> the new model since ImportHash() forces a new super MAC), then is it not
> impossible that we need to generate a new super MAC yet no other hashes have
> been modified (i.e., has_changed is still false)?

If you think we still need it, please update the CL description to explain why.

[erikwright (departed), 2014-06-11 20:20:17]

PTAL. I added two scenarios where this change is required - one affects the
current code and one affects the code once migration is implemented.

[gab, 2014-06-11 21:28:04]

lgtm w/ test and comments below

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_filter.cc:183: prefs_altered = true;
FWIW, setting |prefs_altered = true| is technically incorrect until the MACs are
stored in the prefs (which is not the case in this extracted CL).

I don't think it really matters as this will just schedule an unnecessary write
of Preferences while MACs are still in Local State and will be correct once they
move.

So I say, leave it as-is, but something to keep in mind.

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
File chrome/browser/prefs/pref_hash_store_transaction.h (right):

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_transaction.h:71: // Forces the super MAC
to be updated. Returns true if this results in a
s/updated/set if it's invalid/

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_transaction.h:72: // change to the store
contents.
s/the store/this store's/

https://codereview.chromium.org/334433002/diff/1/chrome/browser/prefs/pref_ha...
chrome/browser/prefs/pref_hash_store_transaction.h:73: virtual bool
StampSuperMac() = 0;
How about SetSuperMacIfRequired() ?

With that name if it feels more natural to me that it may or may not do work and
that it would return a boolean based on whether it did work.

[erikwright (departed), 2014-06-12 14:02:51]

I now believe this CL to be wrong.

With two pref stores sharing a hash store, the validation of the first store
will update the super MAC, leading the second store to mistakenly
TrustedInitialize new values. I'm holding off on this for now.

[gab, 2014-06-12 14:28:12]

On 2014/06/12 14:02:51, erikwright wrote:
> I now believe this CL to be wrong.
> 
> With two pref stores sharing a hash store, the validation of the first store
> will update the super MAC, leading the second store to mistakenly
> TrustedInitialize new values. I'm holding off on this for now.

Oh right of course, good catch, this only makes sense in a model where each
store is independent.

I'm happy to see this land as part of your other CL. It's small enough that it
won't really add complexity to the main CL IMO.

[gab, 2014-06-18 15:50:27]

On 2014/06/12 14:28:12, gab wrote:
> On 2014/06/12 14:02:51, erikwright wrote:
> > I now believe this CL to be wrong.
> > 
> > With two pref stores sharing a hash store, the validation of the first store
> > will update the super MAC, leading the second store to mistakenly
> > TrustedInitialize new values. I'm holding off on this for now.
> 
> Oh right of course, good catch, this only makes sense in a model where each
> store is independent.
> 
> I'm happy to see this land as part of your other CL. It's small enough that it
> won't really add complexity to the main CL IMO.

This is landing as part of https://codereview.chromium.org/324493002/ as
discussed above.

Closing.