New issue
Advanced search Search tips

Issue 775530 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 800176


Show other hotlists

Hotlists containing this issue:
EnamelAndFriendsFixIt


Sign in to add a comment

Make safe browsing browser tests exercise more code

Project Member Reported by csharrison@chromium.org, Oct 17 2017

Issue description

It would be nice to have test infrastructure which mock out Android APIs / V4 server to use in browsertests, to exercise metadata parsing code in e2e tests.
 
Huh, it didn't log a message but this was landed here:
https://chromium-review.googlesource.com/c/chromium/src/+/719816

In a followup I think I will try to integrate this work with the TestSafeBrowsingDatabaseHelper.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df479a79836e03a72bfef55b54d4b2c378677bfe

commit df479a79836e03a72bfef55b54d4b2c378677bfe
Author: Charles Harrison <csharrison@chromium.org>
Date: Tue Oct 17 18:55:06 2017

Introduce V4GetHashInterceptor for safe browsing end to end tests

The class is a URLRequestInterceptor which intercepts URL requests to
the safe browsing endpoint https://safebrowsing.googleapis.com/v4 and
responds with canned responses.

This allows browsertests to additionally test that V4 metadata parsing
is working as intended.

Bug:  775530 
Change-Id: I40d4504768b46d00259f13f2900be98fa6a713c1
Reviewed-on: https://chromium-review.googlesource.com/719816
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509467}
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/BUILD.gn
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc
[add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/safe_browsing/v4_get_hash_interceptor_browsertest.cc
[delete] https://crrev.com/7748cea2ab5767bef12997c2a5225b566df02d96/chrome/browser/safe_browsing/v4_test_utils.cc
[delete] https://crrev.com/7748cea2ab5767bef12997c2a5225b566df02d96/chrome/browser/safe_browsing/v4_test_utils.h
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/chrome/test/BUILD.gn
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/BUILD.gn
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_database.h
[add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_get_hash_interceptor.cc
[add] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_get_hash_interceptor.h
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_protocol_manager_util.cc
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_protocol_manager_util.h
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_test_util.cc
[modify] https://crrev.com/df479a79836e03a72bfef55b54d4b2c378677bfe/components/safe_browsing/db/v4_test_util.h

Welp, just talked to mmenke@ who mentioned URLRequestInterceptor is deprecated and will break soon with the network service.

I'll fix this by converting it into an embedded test server callback, with some associated host resolver mappings.
There is work on the Safe Browsing backend team to produce a test server that can walk through a bunch of update scenarios for a client like Chrome.  Would that be the kind of e2e test you're thinking of?  
Labels: SafeBrowsing-Triaged
Status: Assigned (was: Untriaged)
Ah, I see now you said browser tests, so never mind my last comment.
Summary: Make safe browsing browser tests exercise more code (was: End to end tests of safe browsing)
nparker: good to know! Yeah I've updated the summary to be a bit more clear. The main reason I started working on this was that I had a CL which added metadata parsing but it was only exercised in unit tests. As I am not very familiar with the safe browsing codebase, this made me a bit uncomfortable and I wanted to make sure I didn't introduce bugs.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ad8ea07f746dc869f51dd2573599c236a9b5932

commit 3ad8ea07f746dc869f51dd2573599c236a9b5932
Author: Charles Harrison <csharrison@chromium.org>
Date: Wed Oct 25 03:52:32 2017

Replace the V4 URLRequestInterceptor with a embedded test server handler

URLRequestInterceptors are deprecated and will not work in the
network service.

This CL does a few things:
1. Introduce a global g_sbv4_url_prefix_for_testing, which is used in
   production code if it is set. This is non-ideal, but it turned out
   to be quite difficult to redirect the request using only host
   resolver rules.
2. Replace the V4 URLRequestInterceptor with a EmbeddedTestServer
   request handler, which is compatible with the network service.
3. Further improve the V4 handler by
   a. Only intercepting requests to /v4/fullHashes:find.
   b. Fully support requests with only hash prefixes, i.e. if a request
      comes through that matches multiple URLs, respond with multiple
      ThreatMatches.
4. Reduces the scope of some of the browser tests because the test
   server should have the handlers registered before being started.

Bug:  775530 
Change-Id: I0eba45b72eece5791224e54082a7b442e9b7f6d0
Reviewed-on: https://chromium-review.googlesource.com/726259
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511357}
[rename] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/chrome/browser/safe_browsing/v4_embedded_test_server_browsertest.cc
[modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/chrome/test/BUILD.gn
[modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/BUILD.gn
[modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_database.h
[add] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_embedded_test_server_util.cc
[add] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_embedded_test_server_util.h
[delete] https://crrev.com/c8ca971ca4303f3d5075896d072346b7ba656b97/components/safe_browsing/db/v4_get_hash_interceptor.cc
[delete] https://crrev.com/c8ca971ca4303f3d5075896d072346b7ba656b97/components/safe_browsing/db/v4_get_hash_interceptor.h
[modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_protocol_manager_util.cc
[modify] https://crrev.com/3ad8ea07f746dc869f51dd2573599c236a9b5932/components/safe_browsing/db/v4_protocol_manager_util.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d

commit a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d
Author: Charles Harrison <csharrison@chromium.org>
Date: Mon Oct 30 19:07:44 2017

[subresource_filter] Add a full hash request intercepting browsertest

This patch does a few things:
1. Beef up the TestSafeBrowsingDatabaseHelper, so callers can optionally,
   pass in a custom TestV4GetHashProtocolManagerFactory. If this is null,
   then the helper will not mock the v4 hash protocol manager. This
   causes real API requests to go out to the network.

2. Make TestV4Store::MarkPrefixAsBad support multiple prefixes per
   prefix size.

3. Minor cleanups of names and std::make_unique

4. Add a new browser test suite for subresource_filter which uses
   the v4 embedded_test_server request handler, combined with the new
   TestSafeBrowsingDatabaseHelper, to mock out v4 API responses for
   the subresource filter list. This tests enforcement and warning
   variants.

Bug:  775530 , 737201 
Change-Id: Id26b676194fd33c28737bcb693e47ca75145f82f
Reviewed-on: https://chromium-review.googlesource.com/730405
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512567}
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/test_safe_browsing_database_helper.h
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/safe_browsing/v4_embedded_test_server_browsertest.cc
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_browsertest.cc
[add] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/subresource_filter/subresource_filter_intercepting_browsertest.cc
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_browsertest.cc
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/chrome/test/BUILD.gn
[modify] https://crrev.com/a9f882c736eb48d8cefc4e42566af4ddbb7b3b6d/components/safe_browsing/db/v4_test_util.cc

Comment 9 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt
Blocking: 800176
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18708f498d97f6c157d82ad0e6f558252791a733

commit 18708f498d97f6c157d82ad0e6f558252791a733
Author: Charles Harrison <csharrison@chromium.org>
Date: Tue Jan 09 19:27:14 2018

Safe Browsing e2e instrumentation tests on Android

This CL adds a MockSafeBrowsingApiHandler which vends fake response
JSON. It is used by a simple test harness which checks that an
interstitial is properly shown.

Bug:  775530 
Change-Id: I5a8c21edd57a6a57c1de85feed1205502799673b
Reviewed-on: https://chromium-review.googlesource.com/850339
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528066}
[modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/BUILD.gn
[modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/java_sources.gni
[modify] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/DEPS
[add] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java
[add] https://crrev.com/18708f498d97f6c157d82ad0e6f558252791a733/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java

Status: Fixed (was: Assigned)
I'm going to close this issue out, despite the fact that it's always nicer to make tests exercise *even more* code :)

To sum up the work:
1. On Android: MockSafeBrowsingApiHandler can be used to mock out SafetyNet JSON responses. This can only be used in Java tests but should be relatively easy to use once we get native browser_tests working on Android.

2. On desktop: We provide a fake safe browsing v4 server which can respond with arbitrary full hash responses, via a custom embedded test server handler. We don't currently support update requests though. This should be used in conjunction with the test v4 db for hash prefixes.

For new code that sits below ThreatMetadata, or for instrumentation tests on Android, consider using these new tools!
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd

commit 7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd
Author: Joy Ming <jming@chromium.org>
Date: Tue Jan 09 22:01:49 2018

Revert "Safe Browsing e2e instrumentation tests on Android"

This reverts commit 18708f498d97f6c157d82ad0e6f558252791a733.

Reason for revert: The test failed on KitKat Phone Tester (dbg),
See crbug.com/800514

Original change's description:
> Safe Browsing e2e instrumentation tests on Android
> 
> This CL adds a MockSafeBrowsingApiHandler which vends fake response
> JSON. It is used by a simple test harness which checks that an
> interstitial is properly shown.
> 
> Bug:  775530 
> Change-Id: I5a8c21edd57a6a57c1de85feed1205502799673b
> Reviewed-on: https://chromium-review.googlesource.com/850339
> Commit-Queue: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Varun Khaneja <vakh@chromium.org>
> Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528066}

TBR=tedchoc@chromium.org,csharrison@chromium.org,vakh@chromium.org

Change-Id: I3c5ee49892684a2a12718326b55bc32da58a7ab1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  775530 
Reviewed-on: https://chromium-review.googlesource.com/857924
Reviewed-by: Joy Ming <jming@chromium.org>
Commit-Queue: Joy Ming <jming@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528132}
[modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/BUILD.gn
[modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/java_sources.gni
[modify] https://crrev.com/7f9da00a4990a7a593aa7c27c827e5d8f7ac6cfd/chrome/android/javatests/DEPS
[delete] https://crrev.com/0acaa0b094e76002120a3e6f3e9492f2e9c64b0c/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java
[delete] https://crrev.com/0acaa0b094e76002120a3e6f3e9492f2e9c64b0c/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a675185d4ed064433611c309b85b9aad971bf0f2

commit a675185d4ed064433611c309b85b9aad971bf0f2
Author: Charles Harrison <csharrison@chromium.org>
Date: Fri Jan 12 21:02:52 2018

Reland: Safe Browsing e2e instrumentation tests on Android

This CL adds a MockSafeBrowsingApiHandler which vends fake response
JSON. It is used by a simple test harness which checks that an
interstitial is properly shown.

Previously landed here:
https://chromium-review.googlesource.com/c/chromium/src/+/850339

TBR=vakh@chromium.org

Bug:  775530 ,800514
Change-Id: I4834caef5323a2450b21b54e9307d316b44d8b5b
Reviewed-on: https://chromium-review.googlesource.com/860622
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529062}
[modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/BUILD.gn
[modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/java_sources.gni
[modify] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/DEPS
[add] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/src/org/chromium/chrome/browser/MockSafeBrowsingApiHandler.java
[add] https://crrev.com/a675185d4ed064433611c309b85b9aad971bf0f2/chrome/android/javatests/src/org/chromium/chrome/browser/SafeBrowsingTest.java

Sign in to add a comment