Add the ability to check the CSD Whitelist asynchronously, for PhishGuard.

Add the ability to check the CSD Whitelist asynchronously,for PhishGuard.

This adds V4LocalDatabaseManager::CheckCsdWhitelistUrl() and tests for
it. A later CL will switch the three callers of MatchCsdWhitelistUrl()
to this new function, and remove the Match method.

BUG=714300
Review-Url: https://codereview.chromium.org/2890293004
Cr-Commit-Position: refs/heads/master@{#476372}
Committed: https://chromium.googlesource.com/chromium/src/+/56e1ea730204dbc043f58b14fec3cb6dfb6a4739

[Nathan Parker, 2017-05-19 16:29:22]

The CQ bit was checked by nparker@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 16:30:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 17:35:33]

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

[commit-bot: I haz the power, 2017-05-19 17:35:34]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-05-19 19:00:20]

nparker@chromium.org changed reviewers:
+ vakh@chromium.org

[Nathan Parker, 2017-05-19 19:00:21]

vakh -- PTAL.
I have a few tweaks to the tests remaining -- I'd be glad for your suggestions
on my TODOs there.

[vakh (use Gerrit instead), 2017-05-24 16:01:17]

lgtm

I'm sorry, I don't know how I missed this CL for 4 days.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/database_manager.h:77: virtual void
OnCheckWhitelistUrlResult(bool is_whitelisted) {}
The comment is CSD specific but the method name is not. It'd be better to make
them consistent.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/database_manager.h:173: virtual AsyncMatch
CheckCsdWhitelistUrl(const GURL& url, Client* client) = 0;
Please move this declaration above to keep it sorted among the Check* method
declarations.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:762: bool
did_match_whitelist =
would it be clearer to have it the other way:

bool did_match_whitelist = check->most_severe_threat_type ==
SB_THREAT_TYPE_CSD_WHITELIST;
DCHECK(did_match_whitelist || check->most_severe_threat_type ==
SB_THREAT_TYPE_SAFE);

?

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:441:
EXPECT_EQ(AsyncMatch::ASYNC, v4_local_database_manager_->CheckCsdWhitelistUrl(
Either I am reading this incorrectly, or there's a bug in the V4 implementation,
or this test can't be passing :)

The hash prefix was added to GetUrlMalwareId() but the hash prefix still matches
in GetUrlCsdWhitelistId() ?

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:446: //
TODO(nparker): Verify full-hash-check fired?
For the TODO, you could use the TestClient to verify that the
OnCheckWhitelistUrlResult() is/isn't called, which can be considered a poor
proxy for checking whether full hash check fired.

[Nathan Parker, 2017-05-31 23:36:29]

The CQ bit was checked by nparker@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 23:37:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nathan Parker, 2017-05-31 23:40:11]

Thanks! PTAL.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/database_manager.h:77: virtual void
OnCheckWhitelistUrlResult(bool is_whitelisted) {}
On 2017/05/24 16:01:17, vakh (Varun Khaneja) wrote:
> The comment is CSD specific but the method name is not. It'd be better to make
> them consistent.

Done.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/database_manager.h:173: virtual AsyncMatch
CheckCsdWhitelistUrl(const GURL& url, Client* client) = 0;
On 2017/05/24 16:01:16, vakh (Varun Khaneja) wrote:
> Please move this declaration above to keep it sorted among the Check* method
> declarations.

Done.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager.cc:762: bool
did_match_whitelist =
On 2017/05/24 16:01:17, vakh (Varun Khaneja) wrote:
> would it be clearer to have it the other way:
> 
> bool did_match_whitelist = check->most_severe_threat_type ==
> SB_THREAT_TYPE_CSD_WHITELIST;
> DCHECK(did_match_whitelist || check->most_severe_threat_type ==
> SB_THREAT_TYPE_SAFE);
> 
> ?

yea, I think it is clearer, but I had it the other way to be more conservative
since then any non-safe verdict would be considered "whitelisted".  Actually
that seems really unlikley.  I'll do it your way.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:441:
EXPECT_EQ(AsyncMatch::ASYNC, v4_local_database_manager_->CheckCsdWhitelistUrl(
On 2017/05/24 16:01:17, vakh (Varun Khaneja) wrote:
> Either I am reading this incorrectly, or there's a bug in the V4
implementation,
> or this test can't be passing :)
> 
> The hash prefix was added to GetUrlMalwareId() but the hash prefix still
matches
> in GetUrlCsdWhitelistId() ?

Huh!  Ah -- found a missing check in the fake GetStoresMatchingFullHash().  I
fixed that and a few miss-aligned tests that that fix uncovered.

https://codereview.chromium.org/2890293004/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:446: //
TODO(nparker): Verify full-hash-check fired?
On 2017/05/24 16:01:17, vakh (Varun Khaneja) wrote:
> For the TODO, you could use the TestClient to verify that the
> OnCheckWhitelistUrlResult() is/isn't called, which can be considered a poor
> proxy for checking whether full hash check fired.

Good idea -- this also check that the callback is invoked properly (wasn't
testing that before).

[Nathan Parker, 2017-06-01 00:34:15]

nparker@chromium.org changed reviewers:
+ jialiul@chromium.org

[Nathan Parker, 2017-06-01 00:34:16]

+jialul for second reviewer

[vakh (use Gerrit instead), 2017-06-01 00:54:36]

lgtm

https://codereview.chromium.org/2890293004/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2890293004/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:165: //
TODO(nparker): This might be simpler with a mock and EXPECT calls.
Feel free to assign it to me.

[Jialiu Lin, 2017-06-01 00:56:50]

LGTM. I'll change password protection code once this CL lands.

[commit-bot: I haz the power, 2017-06-01 03:09:36]

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

[commit-bot: I haz the power, 2017-06-01 03:09:37]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-06-01 18:27:47]

Thanks!

Jialiu -- Actually I already have a CL underway for that.

[Nathan Parker, 2017-06-01 18:30:14]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:31:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 18:41:27]

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

[commit-bot: I haz the power, 2017-06-01 18:41:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nathan Parker, 2017-06-01 18:43:56]

nparker@chromium.org changed reviewers:
+ estark@chromium.org

[Nathan Parker, 2017-06-01 18:43:57]

+estark for OWNERS of 
chrome/browser/ssl/security_state_tab_helper.cc

Thanks

[estark, 2017-06-01 18:44:46]

c/b/ssl lgtm

[Nathan Parker, 2017-06-01 18:45:34]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:46:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 18:53:43]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496342734092490,
"parent_rev": "1cd2c85e634da2d19943d5615c7ab260286b5b0b", "commit_rev":
"56e1ea730204dbc043f58b14fec3cb6dfb6a4739"}

[commit-bot: I haz the power, 2017-06-01 18:53:50]

Description was changed from

==========
Add the ability to check the CSD Whitelist asynchronously,for PhishGuard.

This adds V4LocalDatabaseManager::CheckCsdWhitelistUrl() and tests for
it. A later CL will switch the three callers of MatchCsdWhitelistUrl()
to this new function, and remove the Match method.

BUG=714300
==========

to

==========
Add the ability to check the CSD Whitelist asynchronously,for PhishGuard.

This adds V4LocalDatabaseManager::CheckCsdWhitelistUrl() and tests for
it. A later CL will switch the three callers of MatchCsdWhitelistUrl()
to this new function, and remove the Match method.

BUG=714300

Review-Url: https://codereview.chromium.org/2890293004
Cr-Commit-Position: refs/heads/master@{#476372}
Committed:
https://chromium.googlesource.com/chromium/src/+/56e1ea730204dbc043f58b14fec3...
==========

[commit-bot: I haz the power, 2017-06-01 18:53:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/56e1ea730204dbc043f58b14fec3...