Add support for goog-csdinclusionwhite-sha256 in SafeBrowsingDatabase.

Add support for goog-csdinclusionwhite-sha256 in SafeBrowsingDatabase.

It has the same format as goog-downloadwhite-digest256.

Tests have been generalized to cover all whitelists without code duplication
using a technique similar to the one used in the
BrowseAndUnwantedDatabasesAndPrefixSets test in the same test file.

BUG=412468
TBR=mpearson@chromium.org

Committed: https://crrev.com/5db59789e1cda0585a8fc9ba44df25b165be4d94
Cr-Commit-Position: refs/heads/master@{#310971}

[gab, 2014-12-31 03:52:20]

Patchset #1 (id:1) has been deleted

[gab, 2014-12-31 03:52:27]

Patchset #1 (id:20001) has been deleted

[gab, 2014-12-31 04:03:04]

gab@chromium.org changed reviewers:
+ noelutz@chromium.org

[gab, 2014-12-31 04:03:04]

Noe PTAL.

Thanks!
Gab

[gab, 2015-01-07 20:17:08]

gab@chromium.org changed reviewers:
+ mattm@chromium.org
- noelutz@chromium.org

[gab, 2015-01-07 20:17:09]

Hey Matt, was trying to spare you this one but looks like Noe is OOO for another
week...

PTAL, thanks!
Gab

[mattm, 2015-01-08 22:07:27]

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1725:
RecordFailure(FAILURE_DATABASE_STORE_DELETE);
These don't seem to be sorted anyway, so why not just put the new one at the end
for a cleaner diff? 

Or maybe just change the implementation to avoid this problem..
maybe something like:


bool ok = true;

{
  if (!DeleteFile(...) {
    RecordFailure(..)
    ok = false;
  }
}

...

return ok;

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.h:65: // The browse list and
off-domain inclusion whitelist are always on;
Doesn't seem to actually be the case (default constructor doesn't make one, and
the code has conditions to handle there not being one).

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1353: // Add two
simple chunks to the csd whitelist.
this is more than two chunks :)

[gab, 2015-01-09 13:51:30]

Thanks, small patch update, see below for replies to other comments.

Cheers,
Gab

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1725:
RecordFailure(FAILURE_DATABASE_STORE_DELETE);
On 2015/01/08 22:07:27, mattm wrote:
> These don't seem to be sorted anyway, so why not just put the new one at the
end
> for a cleaner diff? 

I inserted the |inclusion_whitelist_store_| code right after
|csd_whitelist_store_| and |download_whitelist_store_| everywhere since it's
exactly the same code as the other whitelists, having it together makes this
easier to read IMO (in this specific method all stores are handled the same way,
but I find it easier if all methods at least handle all the stores/lists in the
same order).


> 
> Or maybe just change the implementation to avoid this problem..
> maybe something like:
> 
> 
> bool ok = true;
> 
> {
>   if (!DeleteFile(...) {
>     RecordFailure(..)
>     ok = false;
>   }
> }
> 
> ...
> 
> return ok;

Yea, considered this but it would make for a bigger diff and cleaning up is not
really the point of this CL... I totally agree that the multiple bools are
overkill though (besides perhaps when debugging, but I don't think anyone would
have used this property in a while).

Happy to make a follow-up CL if you feel strongly.

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.h:65: // The browse list and
off-domain inclusion whitelist are always on;
On 2015/01/08 22:07:27, mattm wrote:
> Doesn't seem to actually be the case (default constructor doesn't make one,
and
> the code has conditions to handle there not being one).

SafeBrowsingDatabase::Create() doesn't use the default constructor (i.e.
SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew()).

In fact it's explicitly marked as a legacy interface and looking in code search
it looks like no one is using it anymore, should look into removing it
altogether.

Good point about not needing the code to handle the absence of an
inclusion_whitelist though; but looking into changing that it feels more natural
to have this code look the same as the code next to it even though it makes for
slightly superfluous checks... it would also allow for a very simple disabling
change if need be I guess...

Really I think we should look to remove most flags, etc. controlling each lists
as most of them appear to have been used in prior releases and are now always on
I assume -- it probably makes more sense to revisit all of these superfluous
checks for NULL when we do that?

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:1353: // Add two
simple chunks to the csd whitelist.
On 2015/01/08 22:07:27, mattm wrote:
> this is more than two chunks :)

:-), updated comment.

[mattm, 2015-01-10 01:10:54]

lgtm

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1725:
RecordFailure(FAILURE_DATABASE_STORE_DELETE);
On 2015/01/09 13:51:29, gab wrote:
> On 2015/01/08 22:07:27, mattm wrote:
> > These don't seem to be sorted anyway, so why not just put the new one at the
> end
> > for a cleaner diff? 
> 
> I inserted the |inclusion_whitelist_store_| code right after
> |csd_whitelist_store_| and |download_whitelist_store_| everywhere since it's
> exactly the same code as the other whitelists, having it together makes this
> easier to read IMO (in this specific method all stores are handled the same
way,
> but I find it easier if all methods at least handle all the stores/lists in
the
> same order).
> 
> 
> > 
> > Or maybe just change the implementation to avoid this problem..
> > maybe something like:
> > 
> > 
> > bool ok = true;
> > 
> > {
> >   if (!DeleteFile(...) {
> >     RecordFailure(..)
> >     ok = false;
> >   }
> > }
> > 
> > ...
> > 
> > return ok;
> 
> Yea, considered this but it would make for a bigger diff and cleaning up is
not
> really the point of this CL... I totally agree that the multiple bools are
> overkill though (besides perhaps when debugging, but I don't think anyone
would
> have used this property in a while).
> 
> Happy to make a follow-up CL if you feel strongly.

Since new lists keep getting added, would be nice. Though a more extensive
refactoring would also be nice, dunno how likely that is to happen any time
soon.

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.h:65: // The browse list and
off-domain inclusion whitelist are always on;
On 2015/01/09 13:51:30, gab wrote:
> On 2015/01/08 22:07:27, mattm wrote:
> > Doesn't seem to actually be the case (default constructor doesn't make one,
> and
> > the code has conditions to handle there not being one).
> 
> SafeBrowsingDatabase::Create() doesn't use the default constructor (i.e.
> SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew()).
> 
> In fact it's explicitly marked as a legacy interface and looking in code
search
> it looks like no one is using it anymore, should look into removing it
> altogether.
> 
> Good point about not needing the code to handle the absence of an
> inclusion_whitelist though; but looking into changing that it feels more
natural
> to have this code look the same as the code next to it even though it makes
for
> slightly superfluous checks... it would also allow for a very simple disabling
> change if need be I guess...
> 
> Really I think we should look to remove most flags, etc. controlling each
lists
> as most of them appear to have been used in prior releases and are now always
on
> I assume -- it probably makes more sense to revisit all of these superfluous
> checks for NULL when we do that?

sounds good.

One thing to watch out for is SafeBrowsingServerTest, which has issues when new
lists are added, generally it ends up needing any new features disabled until
the test data gets updated to handle it (but it's currently disabled anyway due
to the SB3.0 change, so I wouldn't worry about it too much).

[gab, 2015-01-10 06:34:13]

Thanks!

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1725:
RecordFailure(FAILURE_DATABASE_STORE_DELETE);
On 2015/01/10 01:10:54, mattm wrote:
> On 2015/01/09 13:51:29, gab wrote:
> > On 2015/01/08 22:07:27, mattm wrote:
> > > These don't seem to be sorted anyway, so why not just put the new one at
the
> > end
> > > for a cleaner diff? 
> > 
> > I inserted the |inclusion_whitelist_store_| code right after
> > |csd_whitelist_store_| and |download_whitelist_store_| everywhere since it's
> > exactly the same code as the other whitelists, having it together makes this
> > easier to read IMO (in this specific method all stores are handled the same
> way,
> > but I find it easier if all methods at least handle all the stores/lists in
> the
> > same order).
> > 
> > 
> > > 
> > > Or maybe just change the implementation to avoid this problem..
> > > maybe something like:
> > > 
> > > 
> > > bool ok = true;
> > > 
> > > {
> > >   if (!DeleteFile(...) {
> > >     RecordFailure(..)
> > >     ok = false;
> > >   }
> > > }
> > > 
> > > ...
> > > 
> > > return ok;
> > 
> > Yea, considered this but it would make for a bigger diff and cleaning up is
> not
> > really the point of this CL... I totally agree that the multiple bools are
> > overkill though (besides perhaps when debugging, but I don't think anyone
> would
> > have used this property in a while).
> > 
> > Happy to make a follow-up CL if you feel strongly.
> 
> Since new lists keep getting added, would be nice. Though a more extensive
> refactoring would also be nice, dunno how likely that is to happen any time
> soon.

Agreed, I think the most likely refactoring would be one towards complementizing
safe_browsing under components/, but yea dunno how likely that is to happen any
time soon..

[gab, 2015-01-10 06:36:53]

gab@chromium.org changed reviewers:
+ mpearson@chromium.org

[gab, 2015-01-10 06:38:57]

TBR: mpearson@ for trivial histograms side-effect for an extra suffix identical
to the ones just reviewed in https://codereview.chromium.org/815863002/

Thanks,
Gab

[gab, 2015-01-10 06:39:45]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-01-10 06:40:59]

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

[gab, 2015-01-10 06:41:35]

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/835523002/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_database.cc:1725:
RecordFailure(FAILURE_DATABASE_STORE_DELETE);
On 2015/01/10 06:34:12, gab wrote:
> On 2015/01/10 01:10:54, mattm wrote:
> > On 2015/01/09 13:51:29, gab wrote:
> > > On 2015/01/08 22:07:27, mattm wrote:
> > > > These don't seem to be sorted anyway, so why not just put the new one at
> the
> > > end
> > > > for a cleaner diff? 
> > > 
> > > I inserted the |inclusion_whitelist_store_| code right after
> > > |csd_whitelist_store_| and |download_whitelist_store_| everywhere since
it's
> > > exactly the same code as the other whitelists, having it together makes
this
> > > easier to read IMO (in this specific method all stores are handled the
same
> > way,
> > > but I find it easier if all methods at least handle all the stores/lists
in
> > the
> > > same order).
> > > 
> > > 
> > > > 
> > > > Or maybe just change the implementation to avoid this problem..
> > > > maybe something like:
> > > > 
> > > > 
> > > > bool ok = true;
> > > > 
> > > > {
> > > >   if (!DeleteFile(...) {
> > > >     RecordFailure(..)
> > > >     ok = false;
> > > >   }
> > > > }
> > > > 
> > > > ...
> > > > 
> > > > return ok;
> > > 
> > > Yea, considered this but it would make for a bigger diff and cleaning up
is
> > not
> > > really the point of this CL... I totally agree that the multiple bools are
> > > overkill though (besides perhaps when debugging, but I don't think anyone
> > would
> > > have used this property in a while).
> > > 
> > > Happy to make a follow-up CL if you feel strongly.
> > 
> > Since new lists keep getting added, would be nice. Though a more extensive
> > refactoring would also be nice, dunno how likely that is to happen any time
> > soon.
> 
> Agreed, I think the most likely refactoring would be one towards
complementizing
> safe_browsing under components/, but yea dunno how likely that is to happen
any
> time soon..

Err... Swipe typo on phone.. s/complementizing/componentizing

[commit-bot: I haz the power, 2015-01-10 07:08:03]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-01-10 07:09:05]

Patchset 5 (id:??) landed as
https://crrev.com/5db59789e1cda0585a8fc9ba44df25b165be4d94
Cr-Commit-Position: refs/heads/master@{#310971}

[Mark P, 2015-01-12 18:37:43]

lgtm