diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 42c44165078512..f838c1e3248d8f 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -111,8 +111,7 @@ #if defined(FULL_SAFE_BROWSING) #include "chrome/browser/safe_browsing/download_feedback_service.h" #include "chrome/browser/safe_browsing/download_protection_service.h" -#include "chrome/browser/safe_browsing/safe_browsing_database.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #endif using content::BrowserContext; @@ -1090,7 +1089,7 @@ class FakeDownloadProtectionService }; class FakeSafeBrowsingService - : public safe_browsing::SafeBrowsingService, + : public safe_browsing::TestSafeBrowsingService, public safe_browsing::ServicesDelegate::ServicesCreator { public: FakeSafeBrowsingService() { @@ -1098,8 +1097,6 @@ class FakeSafeBrowsingService safe_browsing::ServicesDelegate::CreateForTest(this, this); } - std::string GetDownloadReport() const { return report_; } - protected: ~FakeSafeBrowsingService() override {} @@ -1122,13 +1119,6 @@ class FakeSafeBrowsingService return nullptr; } - // SafeBrowsingService: - void SendSerializedDownloadReport(const std::string& report) override { - report_ = report; - } - - std::string report_; - DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService); }; @@ -3385,7 +3375,7 @@ IN_PROC_BROWSER_TEST_F( safe_browsing::ClientSafeBrowsingReportRequest actual_report; actual_report.ParseFromString( test_safe_browsing_factory_->fake_safe_browsing_service() - ->GetDownloadReport()); + ->serilized_download_report()); EXPECT_EQ(safe_browsing::ClientSafeBrowsingReportRequest:: DANGEROUS_DOWNLOAD_WARNING, actual_report.type()); @@ -3418,9 +3408,9 @@ IN_PROC_BROWSER_TEST_F( DownloadItem* download = downloads[0]; DownloadCommands(download).ExecuteCommand(DownloadCommands::DISCARD); - EXPECT_TRUE( - test_safe_browsing_factory_->fake_safe_browsing_service() - ->GetDownloadReport().empty()); + EXPECT_TRUE(test_safe_browsing_factory_->fake_safe_browsing_service() + ->serilized_download_report() + .empty()); } #endif // FULL_SAFE_BROWSING diff --git a/chrome/browser/download/download_danger_prompt.cc b/chrome/browser/download/download_danger_prompt.cc index 8ed6be4ec61075..44219173e57b75 100644 --- a/chrome/browser/download/download_danger_prompt.cc +++ b/chrome/browser/download/download_danger_prompt.cc @@ -70,8 +70,8 @@ void DownloadDangerPrompt::SendSafeBrowsingDownloadRecoveryReport( case content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST: report.set_download_verdict(ClientDownloadResponse::DANGEROUS_HOST); break; - default: - break; + default: // Don't send report for any other danger types. + return; } report.set_url(download.GetURL().spec()); report.set_did_proceed(did_proceed); diff --git a/chrome/browser/download/download_danger_prompt_browsertest.cc b/chrome/browser/download/download_danger_prompt_browsertest.cc index 9986c5d6b179ad..b893f5664de71f 100644 --- a/chrome/browser/download/download_danger_prompt_browsertest.cc +++ b/chrome/browser/download/download_danger_prompt_browsertest.cc @@ -8,7 +8,7 @@ #include "build/build_config.h" #include "chrome/browser/download/download_danger_prompt.h" #include "chrome/browser/profiles/profile.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/browser_tabstrip.h" @@ -34,53 +34,14 @@ using safe_browsing::SafeBrowsingService; const char kTestDownloadUrl[] = "http://evildownload.com"; -class FakeSafeBrowsingService : public SafeBrowsingService { - public: - FakeSafeBrowsingService() {} - - void SendSerializedDownloadReport(const std::string& report) override { - report_ = report; - } - - std::string GetDownloadRecoveryReport() const { return report_; } - - protected: - ~FakeSafeBrowsingService() override {} - - private: - std::string report_; -}; - -// Factory that creates FakeSafeBrowsingService instances. -class TestSafeBrowsingServiceFactory - : public safe_browsing::SafeBrowsingServiceFactory { - public: - TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(nullptr) {} - ~TestSafeBrowsingServiceFactory() override {} - - SafeBrowsingService* CreateSafeBrowsingService() override { - if (!fake_safe_browsing_service_) { - fake_safe_browsing_service_ = new FakeSafeBrowsingService(); - } - return fake_safe_browsing_service_.get(); - } - - scoped_refptr fake_safe_browsing_service() { - return fake_safe_browsing_service_; - } - - private: - scoped_refptr fake_safe_browsing_service_; -}; - class DownloadDangerPromptTest : public InProcessBrowserTest { public: DownloadDangerPromptTest() : prompt_(nullptr), expected_action_(DownloadDangerPrompt::CANCEL), did_receive_callback_(false), - test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()), - report_sent_(false) {} + test_safe_browsing_factory_( + new safe_browsing::TestSafeBrowsingServiceFactory()) {} ~DownloadDangerPromptTest() override {} @@ -115,26 +76,31 @@ class DownloadDangerPromptTest : public InProcessBrowserTest { SetUpSafeBrowsingReportExpectations( expected_action == DownloadDangerPrompt::ACCEPT, download_verdict); CreatePrompt(); - report_sent_ = false; } - void VerifyExpectations() { + void VerifyExpectations(bool should_send_report) { content::RunAllPendingInMessageLoop(); // At the end of each test, we expect no more activity from the prompt. The // prompt shouldn't exist anymore either. EXPECT_TRUE(did_receive_callback_); EXPECT_FALSE(prompt_); - testing::Mock::VerifyAndClearExpectations(&download_); - if (report_sent_) { + + if (should_send_report) { EXPECT_EQ(expected_serialized_report_, - test_safe_browsing_factory_->fake_safe_browsing_service() - ->GetDownloadRecoveryReport()); + test_safe_browsing_factory_->test_safe_browsing_service() + ->serilized_download_report()); + } else { + EXPECT_TRUE(test_safe_browsing_factory_->test_safe_browsing_service() + ->serilized_download_report() + .empty()); } + testing::Mock::VerifyAndClearExpectations(&download_); + test_safe_browsing_factory_->test_safe_browsing_service() + ->ClearDownloadReport(); } void SimulatePromptAction(DownloadDangerPrompt::Action action) { prompt_->InvokeActionForTesting(action); - report_sent_ = true; } content::MockDownloadItem& download() { return download_; } @@ -181,9 +147,9 @@ class DownloadDangerPromptTest : public InProcessBrowserTest { DownloadDangerPrompt* prompt_; DownloadDangerPrompt::Action expected_action_; bool did_receive_callback_; - std::unique_ptr test_safe_browsing_factory_; + std::unique_ptr + test_safe_browsing_factory_; std::string expected_serialized_report_; - bool report_sent_; DISALLOW_COPY_AND_ASSIGN(DownloadDangerPromptTest); }; @@ -212,45 +178,55 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, MAYBE_TestAll) { SetUpExpectations(DownloadDangerPrompt::ACCEPT, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, ClientDownloadResponse::DANGEROUS); + EXPECT_CALL(download(), IsDangerous()).WillRepeatedly(Return(true)); SimulatePromptAction(DownloadDangerPrompt::ACCEPT); - VerifyExpectations(); + VerifyExpectations(true); // Clicking the Cancel button should invoke the CANCEL action. SetUpExpectations(DownloadDangerPrompt::CANCEL, content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT, ClientDownloadResponse::UNCOMMON); + EXPECT_CALL(download(), IsDangerous()).WillRepeatedly(Return(true)); SimulatePromptAction(DownloadDangerPrompt::CANCEL); - VerifyExpectations(); + VerifyExpectations(true); // If the download is no longer dangerous (because it was accepted), the // dialog should DISMISS itself. SetUpExpectations(DownloadDangerPrompt::DISMISS, content::DOWNLOAD_DANGER_TYPE_POTENTIALLY_UNWANTED, ClientDownloadResponse::POTENTIALLY_UNWANTED); - EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(false)); + EXPECT_CALL(download(), IsDangerous()).WillRepeatedly(Return(false)); download().NotifyObserversDownloadUpdated(); - VerifyExpectations(); + VerifyExpectations(false); // If the download is in a terminal state then the dialog should DISMISS // itself. SetUpExpectations(DownloadDangerPrompt::DISMISS, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST, ClientDownloadResponse::DANGEROUS_HOST); - EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(true)); - EXPECT_CALL(download(), IsDone()).WillOnce(Return(true)); + EXPECT_CALL(download(), IsDangerous()).WillRepeatedly(Return(true)); + EXPECT_CALL(download(), IsDone()).WillRepeatedly(Return(true)); download().NotifyObserversDownloadUpdated(); - VerifyExpectations(); + VerifyExpectations(false); // If the download is dangerous and is not in a terminal state, don't dismiss // the dialog. SetUpExpectations(DownloadDangerPrompt::ACCEPT, content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT, ClientDownloadResponse::DANGEROUS); - EXPECT_CALL(download(), IsDangerous()).WillOnce(Return(true)); - EXPECT_CALL(download(), IsDone()).WillOnce(Return(false)); + EXPECT_CALL(download(), IsDangerous()).WillRepeatedly(Return(true)); + EXPECT_CALL(download(), IsDone()).WillRepeatedly(Return(false)); download().NotifyObserversDownloadUpdated(); + EXPECT_TRUE(prompt()); + SimulatePromptAction(DownloadDangerPrompt::ACCEPT); + VerifyExpectations(true); + + // If the download is not dangerous, no report will be sent. + SetUpExpectations(DownloadDangerPrompt::ACCEPT, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + ClientDownloadResponse::SAFE); SimulatePromptAction(DownloadDangerPrompt::ACCEPT); - VerifyExpectations(); + VerifyExpectations(false); // If the containing tab is closed, the dialog should DISMISS itself. OpenNewTab(); @@ -258,5 +234,5 @@ IN_PROC_BROWSER_TEST_F(DownloadDangerPromptTest, MAYBE_TestAll) { content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, ClientDownloadResponse::DANGEROUS); chrome::CloseTab(browser()); - VerifyExpectations(); + VerifyExpectations(false); } diff --git a/chrome/browser/prerender/prerender_browsertest.cc b/chrome/browser/prerender/prerender_browsertest.cc index 87d502e8dbdb5a..a1b672c3a2471b 100644 --- a/chrome/browser/prerender/prerender_browsertest.cc +++ b/chrome/browser/prerender/prerender_browsertest.cc @@ -4,6 +4,9 @@ #include #include +#include +#include +#include #include #include @@ -50,7 +53,7 @@ #include "chrome/browser/profiles/profile_io_data.h" #include "chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h" #include "chrome/browser/safe_browsing/local_database_manager.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/task_management/mock_web_contents_task_manager.h" #include "chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h" #include "chrome/browser/task_management/task_manager_browsertest_util.h" @@ -732,20 +735,12 @@ class TestPrerenderContentsFactory : public PrerenderContents::Factory { std::deque expected_contents_queue_; }; -// TODO(nparker): Switch this to use TestSafeBrowsingDatabaseManager and run -// with SAFE_BROWSING_DB_LOCAL || SAFE_BROWSING_DB_REMOTE. -// Note: don't forget to override GetProtocolManagerDelegate and return NULL, -// because FakeSafeBrowsingDatabaseManager does not implement -// LocalSafeBrowsingDatabaseManager. -#if defined(FULL_SAFE_BROWSING) // A SafeBrowsingDatabaseManager implementation that returns a fixed result for // a given URL. class FakeSafeBrowsingDatabaseManager - : public LocalSafeBrowsingDatabaseManager { + : public safe_browsing::TestSafeBrowsingDatabaseManager { public: - explicit FakeSafeBrowsingDatabaseManager(SafeBrowsingService* service) - : LocalSafeBrowsingDatabaseManager(service), - threat_type_(safe_browsing::SB_THREAT_TYPE_SAFE) {} + FakeSafeBrowsingDatabaseManager() {} // Called on the IO thread to check if the given url is safe or not. If we // can synchronously determine that the url is safe, CheckUrl returns true. @@ -757,8 +752,10 @@ class FakeSafeBrowsingDatabaseManager // client, and false will be returned). // Overrides SafeBrowsingDatabaseManager::CheckBrowseUrl. bool CheckBrowseUrl(const GURL& gurl, Client* client) override { - if (gurl != url_ || threat_type_ == safe_browsing::SB_THREAT_TYPE_SAFE) + if (bad_urls_.find(gurl.spec()) == bad_urls_.end() || + bad_urls_[gurl.spec()] == safe_browsing::SB_THREAT_TYPE_SAFE) { return true; + } BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, @@ -768,8 +765,20 @@ class FakeSafeBrowsingDatabaseManager } void SetThreatTypeForUrl(const GURL& url, SBThreatType threat_type) { - url_ = url; - threat_type_ = threat_type; + bad_urls_[url.spec()] = threat_type; + } + + // These are called when checking URLs, so we implement them. + bool IsSupported() const override { return true; } + bool ChecksAreAlwaysAsync() const override { return false; } + bool CanCheckResourceType( + content::ResourceType /* resource_type */) const override { + return true; + } + + bool CheckExtensionIDs(const std::set& extension_ids, + Client* client) override { + return true; } private: @@ -787,61 +796,14 @@ class FakeSafeBrowsingDatabaseManager client, safe_browsing::MALWARE, expected_threats); - sb_check.url_results[0] = threat_type_; + sb_check.url_results[0] = bad_urls_[gurl.spec()]; sb_check.OnSafeBrowsingResult(); } - GURL url_; - SBThreatType threat_type_; + std::unordered_map bad_urls_; DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingDatabaseManager); }; -class FakeSafeBrowsingService : public SafeBrowsingService { - public: - FakeSafeBrowsingService() { } - - // Returned pointer has the same lifespan as the database_manager_ refcounted - // object. - FakeSafeBrowsingDatabaseManager* fake_database_manager() { - return fake_database_manager_; - } - - protected: - ~FakeSafeBrowsingService() override {} - - safe_browsing::SafeBrowsingDatabaseManager* CreateDatabaseManager() override { - fake_database_manager_ = new FakeSafeBrowsingDatabaseManager(this); - return fake_database_manager_; - } - - private: - FakeSafeBrowsingDatabaseManager* fake_database_manager_; - - DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService); -}; - -// Factory that creates FakeSafeBrowsingService instances. -class TestSafeBrowsingServiceFactory - : public safe_browsing::SafeBrowsingServiceFactory { - public: - TestSafeBrowsingServiceFactory() : - most_recent_service_(NULL) { } - ~TestSafeBrowsingServiceFactory() override {} - - SafeBrowsingService* CreateSafeBrowsingService() override { - most_recent_service_ = new FakeSafeBrowsingService(); - return most_recent_service_; - } - - FakeSafeBrowsingService* most_recent_service() const { - return most_recent_service_; - } - - private: - FakeSafeBrowsingService* most_recent_service_; -}; -#endif - class FakeDevToolsClient : public content::DevToolsAgentHostClient { public: FakeDevToolsClient() {} @@ -1121,14 +1083,12 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest { PrerenderBrowserTest() : autostart_test_server_(true), prerender_contents_factory_(NULL), -#if defined(FULL_SAFE_BROWSING) - safe_browsing_factory_(new TestSafeBrowsingServiceFactory()), -#endif + safe_browsing_factory_( + new safe_browsing::TestSafeBrowsingServiceFactory()), call_javascript_(true), check_load_events_(true), loader_path_("/prerender/prerender_loader.html"), - explicitly_set_browser_(NULL) { - } + explicitly_set_browser_(NULL) {} ~PrerenderBrowserTest() override {} @@ -1140,15 +1100,13 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest { } void SetUpInProcessBrowserTestFixture() override { -#if defined(FULL_SAFE_BROWSING) - SafeBrowsingService::RegisterFactory(safe_browsing_factory_.get()); -#endif + safe_browsing_factory_->SetTestDatabaseManager( + new FakeSafeBrowsingDatabaseManager()); + SafeBrowsingService::RegisterFactory(safe_browsing_factory_); } void TearDownInProcessBrowserTestFixture() override { -#if defined(FULL_SAFE_BROWSING) SafeBrowsingService::RegisterFactory(NULL); -#endif } void SetUpCommandLine(base::CommandLine* command_line) override { @@ -1177,6 +1135,7 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest { ASSERT_TRUE(prerender_contents_factory_ == NULL); prerender_contents_factory_ = new TestPrerenderContentsFactory; prerender_manager->SetPrerenderContentsFactory(prerender_contents_factory_); + ASSERT_TRUE(safe_browsing_factory_->test_safe_browsing_service()); } // Convenience function to get the currently active WebContents in @@ -1469,12 +1428,12 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest { return static_cast(history_list->GetSize()); } -#if defined(FULL_SAFE_BROWSING) FakeSafeBrowsingDatabaseManager* GetFakeSafeBrowsingDatabaseManager() { - return safe_browsing_factory_->most_recent_service()-> - fake_database_manager(); + return static_cast( + safe_browsing_factory_->test_safe_browsing_service() + ->database_manager() + .get()); } -#endif TestPrerenderContents* GetPrerenderContentsFor(const GURL& url) const { PrerenderManager::PrerenderData* prerender_data = @@ -1711,9 +1670,7 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest { } TestPrerenderContentsFactory* prerender_contents_factory_; -#if defined(FULL_SAFE_BROWSING) - std::unique_ptr safe_browsing_factory_; -#endif + safe_browsing::TestSafeBrowsingServiceFactory* safe_browsing_factory_; NeverRunsExternalProtocolHandlerDelegate external_protocol_handler_delegate_; GURL dest_url_; std::unique_ptr https_src_server_; @@ -2975,7 +2932,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSSLClientCertIframe) { 0); } -#if defined(FULL_SAFE_BROWSING) // Ensures that we do not prerender pages with a safe browsing // interstitial. IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSafeBrowsingTopLevel) { @@ -3041,8 +2997,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSafeBrowsingIframe) { 0); } -#endif - // Checks that a local storage read will not cause prerender to fail. IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderLocalStorageRead) { PrerenderTestURL("/prerender/prerender_localstorage_read.html", diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc index 1f3af7bcca3df4..2886fb6f90d480 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc @@ -22,7 +22,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/local_database_manager.h" #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/safe_browsing/threat_details.h" #include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/browser/ui/browser.h" @@ -136,10 +136,12 @@ class FakeSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager { }; // A SafeBrowingUIManager class that allows intercepting malware details. -class FakeSafeBrowsingUIManager : public SafeBrowsingUIManager { +class FakeSafeBrowsingUIManager : public TestSafeBrowsingUIManager { public: + FakeSafeBrowsingUIManager() + : TestSafeBrowsingUIManager(), threat_details_done_(false) {} explicit FakeSafeBrowsingUIManager(SafeBrowsingService* service) - : SafeBrowsingUIManager(service), threat_details_done_(false) {} + : TestSafeBrowsingUIManager(service), threat_details_done_(false) {} // Overrides SafeBrowsingUIManager void SendSerializedThreatDetails(const std::string& serialized) override { @@ -187,68 +189,6 @@ class FakeSafeBrowsingUIManager : public SafeBrowsingUIManager { DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingUIManager); }; -class FakeSafeBrowsingService : public SafeBrowsingService { - public: - FakeSafeBrowsingService() - : fake_database_manager_(), - fake_ui_manager_() { } - - // Returned pointer has the same lifespan as the database_manager_ refcounted - // object. - FakeSafeBrowsingDatabaseManager* fake_database_manager() { - return fake_database_manager_; - } - // Returned pointer has the same lifespan as the ui_manager_ refcounted - // object. - FakeSafeBrowsingUIManager* fake_ui_manager() { - return fake_ui_manager_; - } - - protected: - ~FakeSafeBrowsingService() override {} - - SafeBrowsingDatabaseManager* CreateDatabaseManager() override { - fake_database_manager_ = new FakeSafeBrowsingDatabaseManager(); - return fake_database_manager_; - } - - SafeBrowsingUIManager* CreateUIManager() override { - fake_ui_manager_ = new FakeSafeBrowsingUIManager(this); - return fake_ui_manager_; - } - - SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() override { - // Our SafeBrowsingDatabaseManager doesn't implement this delegate. - return NULL; - } - - private: - FakeSafeBrowsingDatabaseManager* fake_database_manager_; - FakeSafeBrowsingUIManager* fake_ui_manager_; - - DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService); -}; - -// Factory that creates FakeSafeBrowsingService instances. -class TestSafeBrowsingServiceFactory : public SafeBrowsingServiceFactory { - public: - TestSafeBrowsingServiceFactory() : - most_recent_service_(NULL) { } - ~TestSafeBrowsingServiceFactory() override {} - - SafeBrowsingService* CreateSafeBrowsingService() override { - most_recent_service_ = new FakeSafeBrowsingService(); - return most_recent_service_; - } - - FakeSafeBrowsingService* most_recent_service() const { - return most_recent_service_; - } - - private: - FakeSafeBrowsingService* most_recent_service_; -}; - } // namespace class TestThreatDetailsFactory : public ThreatDetailsFactory { @@ -342,6 +282,10 @@ class SafeBrowsingBlockingPageBrowserTest SafeBrowsingBlockingPageBrowserTest() {} void SetUp() override { + // Test UI manager and test database manager should be set before + // InProcessBrowserTest::SetUp(). + factory_.SetTestUIManager(new FakeSafeBrowsingUIManager()); + factory_.SetTestDatabaseManager(new FakeSafeBrowsingDatabaseManager()); SafeBrowsingService::RegisterFactory(&factory_); SafeBrowsingBlockingPage::RegisterFactory(&blocking_page_factory_); ThreatDetails::RegisterFactory(&details_factory_); @@ -367,12 +311,12 @@ class SafeBrowsingBlockingPageBrowserTest } void SetURLThreatType(const GURL& url, SBThreatType threat_type) { - FakeSafeBrowsingService* service = - static_cast( - g_browser_process->safe_browsing_service()); - + TestSafeBrowsingService* service = factory_.test_safe_browsing_service(); ASSERT_TRUE(service); - service->fake_database_manager()->SetURLThreatType(url, threat_type); + + static_cast( + service->database_manager().get()) + ->SetURLThreatType(url, threat_type); } // Adds a safebrowsing result of the current test threat to the fake @@ -449,13 +393,15 @@ class SafeBrowsingBlockingPageBrowserTest } void SetReportSentCallback(const base::Closure& callback) { - factory_.most_recent_service() - ->fake_ui_manager() + static_cast( + factory_.test_safe_browsing_service()->ui_manager().get()) ->set_threat_details_done_callback(callback); } std::string GetReportSent() { - return factory_.most_recent_service()->fake_ui_manager()->GetReport(); + return static_cast( + factory_.test_safe_browsing_service()->ui_manager().get()) + ->GetReport(); } void MalwareRedirectCancelAndProceed(const std::string& open_function) { diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc index da750ebcc8d4e2..5a5bc65226e40c 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc @@ -7,7 +7,7 @@ #include "base/run_loop.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/safe_browsing/threat_details.h" #include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/common/pref_names.h" @@ -53,26 +53,6 @@ class TestSafeBrowsingBlockingPage : public SafeBrowsingBlockingPage { } }; -class TestSafeBrowsingUIManager: public SafeBrowsingUIManager { - public: - explicit TestSafeBrowsingUIManager(SafeBrowsingService* service) - : SafeBrowsingUIManager(service) { - } - - void SendSerializedThreatDetails(const std::string& serialized) override { - details_.push_back(serialized); - } - - std::list* GetDetails() { - return &details_; - } - - private: - ~TestSafeBrowsingUIManager() override {} - - std::list details_; -}; - class TestSafeBrowsingBlockingPageFactory : public SafeBrowsingBlockingPageFactory { public: @@ -280,8 +260,8 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageDontProceed) { EXPECT_FALSE(controller().GetPendingEntry()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a malware page and then proceeding. @@ -313,8 +293,8 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageProceed) { ASSERT_FALSE(InterstitialPage::GetInterstitialPage(web_contents())); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a page that contains malware subresources @@ -349,8 +329,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceDontProceed) { EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->GetURL().spec()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a page that contains malware subresources @@ -381,8 +361,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceProceed) { EXPECT_EQ(kGoodURL, controller().GetActiveEntry()->GetURL().spec()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a page that contains multiple malware @@ -424,8 +404,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->GetURL().spec()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a page that contains multiple malware @@ -460,8 +440,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_EQ(OK, user_response()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); ResetUserResponse(); @@ -482,8 +462,8 @@ TEST_F(SafeBrowsingBlockingPageTest, // No report should have been sent -- we don't create a report the // second time. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a page that contains multiple malware @@ -514,8 +494,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMultipleMalwareResourceProceed) { EXPECT_EQ(OK, user_response()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); ResetUserResponse(); @@ -534,8 +514,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMultipleMalwareResourceProceed) { // No report should have been sent -- we don't create a report the // second time. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page then navigating back and forth to make sure the @@ -586,8 +566,8 @@ TEST_F(SafeBrowsingBlockingPageTest, NavigatingBackAndForth) { EXPECT_EQ(kBadURL, controller().GetActiveEntry()->GetURL().spec()); // Two reports should have been sent. - EXPECT_EQ(2u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(2u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests that calling "don't proceed" after "proceed" has been called doesn't @@ -623,8 +603,8 @@ TEST_F(SafeBrowsingBlockingPageTest, ProceedThenDontProceed) { EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // Only one report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Tests showing a blocking page for a malware page with reports disabled. @@ -658,8 +638,8 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwareReportsDisabled) { EXPECT_FALSE(controller().GetPendingEntry()); // No report should have been sent. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Test that toggling the checkbox has the anticipated effects. @@ -727,8 +707,8 @@ TEST_F(SafeBrowsingBlockingPageTest, ExtendedReportingNotShownOnSecurePage) { EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // No report should have been sent. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Test that extended reporting option is not shown on blocking an HTTPS @@ -762,8 +742,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // No report should have been sent. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Test that extended reporting option is not shown on blocking an HTTP @@ -797,8 +777,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // No report should have been sent. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Test that extended reporting option is shown on blocking an HTTPS @@ -832,8 +812,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // A report should have been sent. - EXPECT_EQ(1u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(1u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // Test that extended reporting option is not shown on blocking an HTTPS @@ -875,8 +855,8 @@ TEST_F(SafeBrowsingBlockingPageTest, EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // No report should have been sent. - EXPECT_EQ(0u, ui_manager_->GetDetails()->size()); - ui_manager_->GetDetails()->clear(); + EXPECT_EQ(0u, ui_manager_->GetThreatDetails()->size()); + ui_manager_->GetThreatDetails()->clear(); } // TODO(mattm): Add test for extended reporting not shown or sent in incognito diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h index 8fcb45b4cde142..0d8534169be2b6 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.h +++ b/chrome/browser/safe_browsing/safe_browsing_service.h @@ -190,6 +190,8 @@ class SafeBrowsingService : public base::RefCountedThreadSafe< friend class SafeBrowsingServerTest; friend class SafeBrowsingServiceTest; friend class SafeBrowsingURLRequestContextGetter; + friend class TestSafeBrowsingService; + friend class TestSafeBrowsingServiceFactory; // Called to initialize objects that are used on the io_thread. This may be // called multiple times during the life of the SafeBrowsingService. diff --git a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc index 3c23c819f47bf9..ff932b9291ea7e 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc @@ -37,6 +37,7 @@ #include "chrome/browser/safe_browsing/local_database_manager.h" #include "chrome/browser/safe_browsing/protocol_manager.h" #include "chrome/browser/safe_browsing/safe_browsing_database.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/safe_browsing/ui_manager.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_navigator_params.h" @@ -51,6 +52,7 @@ #include "components/prefs/pref_service.h" #include "components/safe_browsing_db/database_manager.h" #include "components/safe_browsing_db/metadata.pb.h" +#include "components/safe_browsing_db/test_database_manager.h" #include "components/safe_browsing_db/util.h" #include "content/public/browser/interstitial_page.h" #include "content/public/browser/navigation_entry.h" @@ -121,11 +123,23 @@ void InvokeFullHashCallback( callback.Run(result, base::TimeDelta::FromMinutes(45)); } -class FakeSafeBrowsingUIManager : public SafeBrowsingUIManager { - public: - explicit FakeSafeBrowsingUIManager(SafeBrowsingService* service) - : SafeBrowsingUIManager(service) {} +// Helper function to set up protocol config. It is used to redirects safe +// browsing queries to embeded test server. It needs to be called before +// SafeBrowsingService being created, therefore it is preferred to call this +// function before InProcessBrowserTest::SetUp(). +void SetProtocolConfigURLPrefix(const std::string& url_prefix, + TestSafeBrowsingServiceFactory* factory) { + SafeBrowsingProtocolConfig config; + config.url_prefix = url_prefix; + // Makes sure the auto update is not triggered. The tests will force the + // update when needed. + config.disable_auto_update = true; + config.client_name = "browser_tests"; + factory->SetTestProtocolConfig(config); +} +class FakeSafeBrowsingUIManager : public TestSafeBrowsingUIManager { + public: void MaybeReportSafeBrowsingHit( const safe_browsing::HitReport& hit_report) override { EXPECT_FALSE(got_hit_report_); @@ -141,48 +155,6 @@ class FakeSafeBrowsingUIManager : public SafeBrowsingUIManager { ~FakeSafeBrowsingUIManager() override {} }; -class FakeSafeBrowsingService : public SafeBrowsingService { - public: - explicit FakeSafeBrowsingService(const std::string& url_prefix) - : url_prefix_(url_prefix) {} - - SafeBrowsingProtocolConfig GetProtocolConfig() const override { - SafeBrowsingProtocolConfig config; - config.url_prefix = url_prefix_; - // Makes sure the auto update is not triggered. The tests will force the - // update when needed. - config.disable_auto_update = true; - config.client_name = "browser_tests"; - return config; - } - - protected: - SafeBrowsingUIManager* CreateUIManager() override { - return new FakeSafeBrowsingUIManager(this); - } - - private: - ~FakeSafeBrowsingService() override {} - - std::string url_prefix_; - - DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService); -}; - -// Factory that creates FakeSafeBrowsingService instances. -class TestSafeBrowsingServiceFactory : public SafeBrowsingServiceFactory { - public: - explicit TestSafeBrowsingServiceFactory(const std::string& url_prefix) - : url_prefix_(url_prefix) {} - - SafeBrowsingService* CreateSafeBrowsingService() override { - return new FakeSafeBrowsingService(url_prefix_); - } - - private: - std::string url_prefix_; -}; - // A SafeBrowingDatabase class that allows us to inject the malicious URLs. class TestSafeBrowsingDatabase : public SafeBrowsingDatabase { public: @@ -517,10 +489,13 @@ class SafeBrowsingServiceTest : public InProcessBrowserTest { } void SetUp() override { - // InProcessBrowserTest::SetUp() instantiates SafebrowsingService and - // RegisterFactory has to be called before SafeBrowsingService is created. - sb_factory_.reset(new TestSafeBrowsingServiceFactory( - "https://definatelynotarealdomain/safebrowsing")); + // InProcessBrowserTest::SetUp() instantiates SafebrowsingService. + // RegisterFactory and plugging test UI manager / protocol config have to + // be called before SafeBrowsingService is created. + sb_factory_.reset(new TestSafeBrowsingServiceFactory()); + sb_factory_->SetTestUIManager(new FakeSafeBrowsingUIManager()); + SetProtocolConfigURLPrefix("https://definatelynotarealdomain/safebrowsing", + sb_factory_.get()); SafeBrowsingService::RegisterFactory(sb_factory_.get()); SafeBrowsingDatabase::RegisterFactory(&db_factory_); SafeBrowsingProtocolManager::RegisterFactory(&pm_factory_); @@ -1580,9 +1555,9 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest { embedded_test_server()->RegisterRequestHandler( base::Bind(&SafeBrowsingDatabaseManagerCookieTest::HandleRequest)); - // Point to the testing server for all SafeBrowsing requests. - GURL url_prefix = embedded_test_server()->GetURL("/testpath"); - sb_factory_.reset(new TestSafeBrowsingServiceFactory(url_prefix.spec())); + sb_factory_.reset(new TestSafeBrowsingServiceFactory()); + SetProtocolConfigURLPrefix( + embedded_test_server()->GetURL("/testpath").spec(), sb_factory_.get()); SafeBrowsingService::RegisterFactory(sb_factory_.get()); InProcessBrowserTest::SetUp(); @@ -1636,7 +1611,6 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest { EXPECT_TRUE(false); return false; } - return InProcessBrowserTest::SetUpUserDataDirectory(); } @@ -1651,7 +1625,6 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest { sql::Statement smt( db.GetUniqueStatement("SELECT name, value FROM cookies ORDER BY name")); ASSERT_TRUE(smt.is_valid()); - ASSERT_TRUE(smt.Step()); ASSERT_EQ("a", smt.ColumnString(0)); ASSERT_EQ("b", smt.ColumnString(1)); @@ -1661,21 +1634,15 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest { EXPECT_FALSE(smt.Step()); } - void SetUpOnMainThread() override { - sb_service_ = g_browser_process->safe_browsing_service(); - ASSERT_TRUE(sb_service_.get() != NULL); - } - - void TearDownOnMainThread() override { sb_service_ = NULL; } - void ForceUpdate() { - sb_service_->protocol_manager()->ForceScheduleNextUpdate( - base::TimeDelta::FromSeconds(0)); + sb_factory_->test_safe_browsing_service() + ->protocol_manager() + ->ForceScheduleNextUpdate(base::TimeDelta::FromSeconds(0)); } - scoped_refptr sb_service_; + std::unique_ptr sb_factory_; - private: + protected: static std::unique_ptr HandleRequest( const net::test_server::HttpRequest& request) { if (!base::StartsWith(request.relative_url, "/testpath/", @@ -1713,8 +1680,6 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest { return std::move(http_response); } - std::unique_ptr sb_factory_; - DISALLOW_COPY_AND_ASSIGN(SafeBrowsingDatabaseManagerCookieTest); }; @@ -1725,7 +1690,7 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingDatabaseManagerCookieTest, content::WindowedNotificationObserver observer( chrome::NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE, content::Source( - sb_service_->database_manager().get())); + sb_factory_->test_safe_browsing_service()->database_manager().get())); BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, base::Bind(&SafeBrowsingDatabaseManagerCookieTest::ForceUpdate, this)); diff --git a/chrome/browser/safe_browsing/safe_browsing_test.cc b/chrome/browser/safe_browsing/safe_browsing_test.cc index f0c8f42b8a88ee..85c79a66190d92 100644 --- a/chrome/browser/safe_browsing/safe_browsing_test.cc +++ b/chrome/browser/safe_browsing/safe_browsing_test.cc @@ -38,7 +38,7 @@ #include "chrome/browser/safe_browsing/local_database_manager.h" #include "chrome/browser/safe_browsing/local_safebrowsing_test_server.h" #include "chrome/browser/safe_browsing/protocol_manager.h" -#include "chrome/browser/safe_browsing/safe_browsing_service.h" +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" #include "chrome/browser/ui/browser.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/url_constants.h" @@ -115,64 +115,26 @@ bool ParsePhishingUrls(const std::string& data, return true; } -class FakeSafeBrowsingService : public SafeBrowsingService { - public: - explicit FakeSafeBrowsingService(const std::string& url_prefix) - : url_prefix_(url_prefix) {} - - SafeBrowsingProtocolConfig GetProtocolConfig() const override { - SafeBrowsingProtocolConfig config; - config.url_prefix = url_prefix_; - // Makes sure the auto update is not triggered. The tests will force the - // update when needed. - config.disable_auto_update = true; - config.client_name = "browser_tests"; - return config; - } - - private: - ~FakeSafeBrowsingService() override {} - - std::string url_prefix_; - - DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService); -}; - -// Factory that creates FakeSafeBrowsingService instances. -class TestSafeBrowsingServiceFactory : public SafeBrowsingServiceFactory { - public: - explicit TestSafeBrowsingServiceFactory(const std::string& url_prefix) - : url_prefix_(url_prefix) {} - - SafeBrowsingService* CreateSafeBrowsingService() override { - return new FakeSafeBrowsingService(url_prefix_); - } - - private: - std::string url_prefix_; -}; - } // namespace // This starts the browser and keeps status of states related to SafeBrowsing. class SafeBrowsingServerTest : public InProcessBrowserTest { public: SafeBrowsingServerTest() - : safe_browsing_service_(NULL), - is_database_ready_(true), - is_update_scheduled_(false), - is_checked_url_in_db_(false), - is_checked_url_safe_(false) { - } + : is_database_ready_(true), + is_update_scheduled_(false), + is_checked_url_in_db_(false), + is_checked_url_safe_(false) {} ~SafeBrowsingServerTest() override {} void UpdateSafeBrowsingStatus() { - ASSERT_TRUE(safe_browsing_service_); + ASSERT_TRUE(sb_factory_->test_safe_browsing_service()); base::AutoLock lock(update_status_mutex_); - last_update_ = safe_browsing_service_->protocol_manager_->last_update(); - is_update_scheduled_ = - safe_browsing_service_->protocol_manager_->update_timer_.IsRunning(); + last_update_ = sb_factory_->test_safe_browsing_service() + ->protocol_manager_->last_update(); + is_update_scheduled_ = sb_factory_->test_safe_browsing_service() + ->protocol_manager_->update_timer_.IsRunning(); } void ForceUpdate() { @@ -187,9 +149,10 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { void ForceUpdateOnIOThread() { EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO)); - ASSERT_TRUE(safe_browsing_service_); - safe_browsing_service_->protocol_manager_->ForceScheduleNextUpdate( - base::TimeDelta::FromSeconds(0)); + ASSERT_TRUE(sb_factory_->test_safe_browsing_service()); + sb_factory_->test_safe_browsing_service() + ->protocol_manager_->ForceScheduleNextUpdate( + base::TimeDelta::FromSeconds(0)); } @@ -200,7 +163,7 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { } void CheckUrl(SafeBrowsingDatabaseManager::Client* helper, const GURL& url) { - ASSERT_TRUE(safe_browsing_service_); + ASSERT_TRUE(sb_factory_->test_safe_browsing_service()); base::AutoLock lock(update_status_mutex_); if (database_manager()->CheckBrowseUrl(url, helper)) { is_checked_url_in_db_ = false; @@ -214,7 +177,7 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { } SafeBrowsingDatabaseManager* database_manager() { - return safe_browsing_service_->database_manager().get(); + return sb_factory_->test_safe_browsing_service()->database_manager().get(); } // TODO(nparker): Remove the need for this by wiring in our own @@ -263,9 +226,14 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { } protected: - bool InitSafeBrowsingService() { - safe_browsing_service_ = g_browser_process->safe_browsing_service(); - return safe_browsing_service_ != NULL; + void SetProtocolConfigURLPrefix(const std::string& url_prefix) { + SafeBrowsingProtocolConfig config; + config.url_prefix = url_prefix; + // Makes sure the auto update is not triggered. The tests will force the + // update when needed. + config.disable_auto_update = true; + config.client_name = "browser_tests"; + sb_factory_->SetTestProtocolConfig(config); } void SetUp() override { @@ -280,9 +248,9 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { ASSERT_TRUE(test_server_->Start()); LOG(INFO) << "server is " << test_server_->host_port_pair().ToString(); + sb_factory_.reset(new TestSafeBrowsingServiceFactory()); // Point to the testing server for all SafeBrowsing requests. - std::string url_prefix = test_server_->GetURL("safebrowsing").spec(); - sb_factory_.reset(new TestSafeBrowsingServiceFactory(url_prefix)); + SetProtocolConfigURLPrefix(test_server_->GetURL("safebrowsing").spec()); SafeBrowsingService::RegisterFactory(sb_factory_.get()); InProcessBrowserTest::SetUp(); @@ -311,14 +279,13 @@ class SafeBrowsingServerTest : public InProcessBrowserTest { void SetTestStep(int step) { std::string test_step = base::StringPrintf("test_step=%d", step); - safe_browsing_service_->protocol_manager_->set_additional_query(test_step); + sb_factory_->test_safe_browsing_service() + ->protocol_manager_->set_additional_query(test_step); } - private: std::unique_ptr sb_factory_; - SafeBrowsingService* safe_browsing_service_; - + private: std::unique_ptr test_server_; // Protects all variables below since they are read on UI thread @@ -501,8 +468,7 @@ class SafeBrowsingServerTestHelper // TODO(shess): Disabled pending new data for third_party/safe_browsing/testing/ IN_PROC_BROWSER_TEST_F(SafeBrowsingServerTest, DISABLED_SafeBrowsingServerTest) { - LOG(INFO) << "Start test"; - ASSERT_TRUE(InitSafeBrowsingService()); + ASSERT_TRUE(sb_factory_->test_safe_browsing_service() != NULL); net::URLRequestContextGetter* request_context = browser()->profile()->GetRequestContext(); @@ -512,7 +478,7 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingServerTest, // Waits and makes sure safebrowsing update is not happening. // The wait will stop once OnWaitForStatusUpdateDone in - // safe_browsing_helper is called and status from safe_browsing_service_ + // safe_browsing_helper is called and status from test_safe_browsing_service // is checked. safe_browsing_helper->UpdateStatus(); EXPECT_TRUE(is_database_ready()); diff --git a/chrome/browser/safe_browsing/test_safe_browsing_service.cc b/chrome/browser/safe_browsing/test_safe_browsing_service.cc new file mode 100644 index 00000000000000..b836b18dec5c15 --- /dev/null +++ b/chrome/browser/safe_browsing/test_safe_browsing_service.cc @@ -0,0 +1,157 @@ +// Copyright (c) 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/safe_browsing/test_safe_browsing_service.h" + +#include "base/strings/string_util.h" +#include "chrome/browser/safe_browsing/download_protection_service.h" +#include "chrome/browser/safe_browsing/ping_manager.h" +#include "chrome/browser/safe_browsing/ui_manager.h" +#include "components/safe_browsing_db/database_manager.h" +#include "components/safe_browsing_db/test_database_manager.h" + +namespace safe_browsing { + +// TestSafeBrowsingService functions: +TestSafeBrowsingService::TestSafeBrowsingService() + : protocol_manager_delegate_disabled_(false), + serialized_download_report_(base::EmptyString()) {} + +TestSafeBrowsingService::~TestSafeBrowsingService() {} + +SafeBrowsingProtocolConfig TestSafeBrowsingService::GetProtocolConfig() const { + if (protocol_config_) + return *protocol_config_; + return SafeBrowsingService::GetProtocolConfig(); +} + +V4ProtocolConfig TestSafeBrowsingService::GetV4ProtocolConfig() const { + if (v4_protocol_config_) + return *v4_protocol_config_; + return SafeBrowsingService::GetV4ProtocolConfig(); +} + +std::string TestSafeBrowsingService::serilized_download_report() { + return serialized_download_report_; +} + +void TestSafeBrowsingService::ClearDownloadReport() { + serialized_download_report_.clear(); +} + +void TestSafeBrowsingService::SetDatabaseManager( + TestSafeBrowsingDatabaseManager* database_manager) { + database_manager_ = database_manager; + // Since TestSafeBrowsingDatabaseManager does not implement + // SafeBrowsingProtocolManagerDelegate, when it is used we need to disable + // protocol_manager_delegate. + protocol_manager_delegate_disabled_ = true; +} + +void TestSafeBrowsingService::SetUIManager( + TestSafeBrowsingUIManager* ui_manager) { + ui_manager->SetSafeBrowsingService(this); + ui_manager_ = ui_manager; +} + +SafeBrowsingDatabaseManager* TestSafeBrowsingService::CreateDatabaseManager() { + if (database_manager_) + return database_manager_.get(); + return SafeBrowsingService::CreateDatabaseManager(); +} + +SafeBrowsingUIManager* TestSafeBrowsingService::CreateUIManager() { + if (ui_manager_) + return ui_manager_.get(); + return SafeBrowsingService::CreateUIManager(); +} + +SafeBrowsingProtocolManagerDelegate* +TestSafeBrowsingService::GetProtocolManagerDelegate() { + if (protocol_manager_delegate_disabled_) + return nullptr; + return SafeBrowsingService::GetProtocolManagerDelegate(); +} + +void TestSafeBrowsingService::SendSerializedDownloadReport( + const std::string& report) { + serialized_download_report_ = report; +} + +void TestSafeBrowsingService::SetProtocolConfig( + SafeBrowsingProtocolConfig* protocol_config) { + protocol_config_.reset(protocol_config); +} + +void TestSafeBrowsingService::SetV4ProtocolConfig( + V4ProtocolConfig* v4_protocol_config) { + v4_protocol_config_.reset(v4_protocol_config); +} + +// TestSafeBrowsingServiceFactory functions: +TestSafeBrowsingServiceFactory::TestSafeBrowsingServiceFactory() + : test_safe_browsing_service_(nullptr), test_protocol_config_(nullptr) {} + +TestSafeBrowsingServiceFactory::~TestSafeBrowsingServiceFactory() {} + +SafeBrowsingService* +TestSafeBrowsingServiceFactory::CreateSafeBrowsingService() { + // Instantiate TestSafeBrowsingService. + test_safe_browsing_service_ = new TestSafeBrowsingService(); + // Plug-in test member clases accordingly. + if (test_ui_manager_) + test_safe_browsing_service_->SetUIManager(test_ui_manager_.get()); + if (test_database_manager_) { + test_safe_browsing_service_->SetDatabaseManager( + test_database_manager_.get()); + } + if (test_protocol_config_) + test_safe_browsing_service_->SetProtocolConfig(test_protocol_config_); + return test_safe_browsing_service_; +} + +TestSafeBrowsingService* +TestSafeBrowsingServiceFactory::test_safe_browsing_service() { + return test_safe_browsing_service_; +} + +void TestSafeBrowsingServiceFactory::SetTestUIManager( + TestSafeBrowsingUIManager* ui_manager) { + test_ui_manager_ = ui_manager; +} + +void TestSafeBrowsingServiceFactory::SetTestDatabaseManager( + TestSafeBrowsingDatabaseManager* database_manager) { + test_database_manager_ = database_manager; +} + +void TestSafeBrowsingServiceFactory::SetTestProtocolConfig( + const SafeBrowsingProtocolConfig& protocol_config) { + test_protocol_config_ = new SafeBrowsingProtocolConfig(protocol_config); +} + +// TestSafeBrowsingUIManager functions: +TestSafeBrowsingUIManager::TestSafeBrowsingUIManager() + : SafeBrowsingUIManager(nullptr) {} + +TestSafeBrowsingUIManager::TestSafeBrowsingUIManager( + const scoped_refptr& service) + : SafeBrowsingUIManager(service) {} + +void TestSafeBrowsingUIManager::SetSafeBrowsingService( + SafeBrowsingService* sb_service) { + sb_service_ = sb_service; +} + +void TestSafeBrowsingUIManager::SendSerializedThreatDetails( + const std::string& serialized) { + details_.push_back(serialized); +} + +std::list* TestSafeBrowsingUIManager::GetThreatDetails() { + return &details_; +} + +TestSafeBrowsingUIManager::~TestSafeBrowsingUIManager() {} +} // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/test_safe_browsing_service.h b/chrome/browser/safe_browsing/test_safe_browsing_service.h new file mode 100644 index 00000000000000..f2e377dd2fc4ca --- /dev/null +++ b/chrome/browser/safe_browsing/test_safe_browsing_service.h @@ -0,0 +1,131 @@ +// Copyright (c) 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SAFE_BROWSING_TEST_SAFE_BROWSING_SERVICE_H_ +#define CHROME_BROWSER_SAFE_BROWSING_TEST_SAFE_BROWSING_SERVICE_H_ + +#include "chrome/browser/safe_browsing/safe_browsing_service.h" + +#include "chrome/browser/safe_browsing/protocol_manager.h" +#include "chrome/browser/safe_browsing/protocol_manager_helper.h" +#include "chrome/browser/safe_browsing/ui_manager.h" +#include "components/safe_browsing_db/v4_protocol_manager_util.h" + +namespace safe_browsing { +struct SafeBrowsingProtocolConfig; +class SafeBrowsingDatabaseManager; +class SafeBrowsingPingManager; +class SafeBrowsingProtocolManager; +struct V4ProtocolConfig; +class TestSafeBrowsingDatabaseManager; +class TestSafeBrowsingUIManager; + +// TestSafeBrowsingService and its factory provides a flexible way to configure +// customized safe browsing UI manager, database manager, protocol manager, +// and etc without the need of override SafeBrowsingService in tests over and +// over again. +// +// How to configure TestSafeBrowsingService in browser tests set up? +// * When overriding SetUp(): +// (1) create an instance of TestSafeBrowsingServiceFactory ( +// e.g. test_sb_factory_), +// (2) Set up necessary test components by calling +// test_sb_factory_->SetTest[DatabaseManager/UIManager/...](...), +// (3) Register TestSafeBrowsingServiceFactory +// SafeBrowsingService::RegisterFactory(test_sb_factory_); +// (4) InProcessBrowserTest::SetUp() or other base class SetUp() function must +// be called at last. +// * When overriding TearDown(): +// Call base class TearDown() first then call +// SafeBrowsingService::RegisterFactory(nullptr) to unregister +// test_sb_factory_. +class TestSafeBrowsingService : public SafeBrowsingService { + public: + TestSafeBrowsingService(); + // SafeBrowsingService overrides + SafeBrowsingProtocolConfig GetProtocolConfig() const override; + V4ProtocolConfig GetV4ProtocolConfig() const override; + + std::string serilized_download_report(); + void ClearDownloadReport(); + + // In browser tests, the following setters must be called before + // SafeBrowsingService::Initialize(). + // The preferable way to use these setters is by calling corresponding + // TestSafeBrowsingServiceFactory::SetTest[DatabaseManager/UIManager/ + // ProtocolConfig]() before InProcessBrowserTest::SetUp() is called. Then + // inside TestSafeBrowsingServiceFactory::CreateSafeBrowsingService(), + // TestSafeBrowsingService instance is created, customised(by using the + // following setters), and then initialized. + void SetUIManager(TestSafeBrowsingUIManager* ui_manager); + void SetDatabaseManager(TestSafeBrowsingDatabaseManager* database_manager); + void SetProtocolConfig(SafeBrowsingProtocolConfig* protocol_config); + void SetV4ProtocolConfig(V4ProtocolConfig* v4_protocol_config); + + protected: + // SafeBrowsingService overrides + ~TestSafeBrowsingService() override; + SafeBrowsingDatabaseManager* CreateDatabaseManager() override; + SafeBrowsingUIManager* CreateUIManager() override; + SafeBrowsingProtocolManagerDelegate* GetProtocolManagerDelegate() override; + void SendSerializedDownloadReport(const std::string& report) override; + + private: + bool protocol_manager_delegate_disabled_; + std::unique_ptr protocol_config_; + std::unique_ptr v4_protocol_config_; + std::string serialized_download_report_; + + DISALLOW_COPY_AND_ASSIGN(TestSafeBrowsingService); +}; + +class TestSafeBrowsingServiceFactory : public SafeBrowsingServiceFactory { + public: + TestSafeBrowsingServiceFactory(); + ~TestSafeBrowsingServiceFactory() override; + + // Creates test safe browsing service, and configures test UI manager, + // database manager and so on. + SafeBrowsingService* CreateSafeBrowsingService() override; + + TestSafeBrowsingService* test_safe_browsing_service(); + + // Test UI manager, database manager and protocol config need to be set before + // SafeBrowsingService::Initialize() is called. + void SetTestUIManager(TestSafeBrowsingUIManager* ui_manager); + void SetTestDatabaseManager( + TestSafeBrowsingDatabaseManager* database_manager); + void SetTestProtocolConfig(const SafeBrowsingProtocolConfig& protocol_config); + + private: + TestSafeBrowsingService* test_safe_browsing_service_; + scoped_refptr test_database_manager_; + scoped_refptr test_ui_manager_; + SafeBrowsingProtocolConfig* test_protocol_config_; + + DISALLOW_COPY_AND_ASSIGN(TestSafeBrowsingServiceFactory); +}; + +// This is an implemenation of SafeBrowsingUIManager without actually +// sending report to safe browsing backend. Safe browsing reports are +// stored in strings for easy verification. +class TestSafeBrowsingUIManager : public SafeBrowsingUIManager { + public: + TestSafeBrowsingUIManager(); + explicit TestSafeBrowsingUIManager( + const scoped_refptr& service); + void SendSerializedThreatDetails(const std::string& serialized) override; + void SetSafeBrowsingService(SafeBrowsingService* sb_service); + std::list* GetThreatDetails(); + + protected: + ~TestSafeBrowsingUIManager() override; + std::list details_; + + DISALLOW_COPY_AND_ASSIGN(TestSafeBrowsingUIManager); +}; + +} // namespace safe_browsing + +#endif // CHROME_BROWSER_SAFE_BROWSING_TEST_SAFE_BROWSING_SERVICE_H_ diff --git a/chrome/browser/safe_browsing/ui_manager.h b/chrome/browser/safe_browsing/ui_manager.h index 8c5b8baae90155..b9b7e528776c46 100644 --- a/chrome/browser/safe_browsing/ui_manager.h +++ b/chrome/browser/safe_browsing/ui_manager.h @@ -158,6 +158,7 @@ class SafeBrowsingUIManager private: friend class base::RefCountedThreadSafe; friend class SafeBrowsingUIManagerTest; + friend class TestSafeBrowsingUIManager; // Call protocol manager on IO thread to report hits of unsafe contents. void ReportSafeBrowsingHitOnIOThread( diff --git a/chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc b/chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc index b06411f3755955..cec27721ada6b8 100644 --- a/chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc +++ b/chrome/browser/ui/cocoa/download/download_danger_prompt_impl.cc @@ -243,11 +243,15 @@ void DownloadDangerPromptImpl::RunDone(Action action) { OnDone done = done_; done_.Reset(); if (download_ != NULL) { - const bool accept = action == DownloadDangerPrompt::ACCEPT; - RecordDownloadDangerPrompt(accept, *download_); - if (!download_->GetURL().is_empty() && - !download_->GetBrowserContext()->IsOffTheRecord()) { - SendSafeBrowsingDownloadRecoveryReport(accept, *download_); + // If this download is no longer dangerous, or is already canceled or + // completed, don't send any report. + if (download_->IsDangerous() && !download_->IsDone()) { + const bool accept = action == DownloadDangerPrompt::ACCEPT; + RecordDownloadDangerPrompt(accept, *download_); + if (!download_->GetURL().is_empty() && + !download_->GetBrowserContext()->IsOffTheRecord()) { + SendSafeBrowsingDownloadRecoveryReport(accept, *download_); + } } download_->RemoveObserver(this); download_ = NULL; diff --git a/chrome/browser/ui/views/download/download_danger_prompt_views.cc b/chrome/browser/ui/views/download/download_danger_prompt_views.cc index 050d591035ada2..cb3f6ceefe6357 100644 --- a/chrome/browser/ui/views/download/download_danger_prompt_views.cc +++ b/chrome/browser/ui/views/download/download_danger_prompt_views.cc @@ -330,11 +330,15 @@ void DownloadDangerPromptViews::RunDone(Action action) { OnDone done = done_; done_.Reset(); if (download_ != NULL) { - const bool accept = action == DownloadDangerPrompt::ACCEPT; - RecordDownloadDangerPrompt(accept, *download_); - if (!download_->GetURL().is_empty() && - !download_->GetBrowserContext()->IsOffTheRecord()) { - SendSafeBrowsingDownloadRecoveryReport(accept, *download_); + // If this download is no longer dangerous, or is already canceled or + // completed, don't send any report. + if (download_->IsDangerous() && !download_->IsDone()) { + const bool accept = action == DownloadDangerPrompt::ACCEPT; + RecordDownloadDangerPrompt(accept, *download_); + if (!download_->GetURL().is_empty() && + !download_->GetBrowserContext()->IsOffTheRecord()) { + SendSafeBrowsingDownloadRecoveryReport(accept, *download_); + } } download_->RemoveObserver(this); download_ = NULL; diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index b9df5df01a77ea..9a4728e66a2f9f 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -2554,6 +2554,8 @@ 'browser/safe_browsing/safe_browsing_service.cc', 'browser/safe_browsing/safe_browsing_service.h', 'browser/safe_browsing/services_delegate.h', + 'browser/safe_browsing/test_safe_browsing_service.cc', + 'browser/safe_browsing/test_safe_browsing_service.h', 'browser/safe_browsing/threat_details.cc', 'browser/safe_browsing/threat_details.h', 'browser/safe_browsing/threat_details_cache.cc',