Commit 59d5b2b8 authored by Nathan Parker's avatar Nathan Parker Committed by Commit Bot

Switch to async whitelist checking in PasswordProtectionRequest

This adds a new WhitelistCheckerClient class that lets the caller
provide a callback rather than extending
SafeBrowsingDatabaseManager::Client, and makes ownership a bit simpler.

Bug: 714300
Change-Id: I1005debdbd9afe6314962537b48ce5f25c521614
Reviewed-on: https://chromium-review.googlesource.com/524694
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: default avatarVarun Khaneja <vakh@chromium.org>
Reviewed-by: default avatarJialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480271}
parent db5d4906
......@@ -24,6 +24,7 @@ source_set("password_protection") {
"//components/safe_browsing:csd_proto",
"//components/safe_browsing_db:database_manager",
"//components/safe_browsing_db:v4_protocol_manager_util",
"//components/safe_browsing_db:whitelist_checker_client",
"//content/public/browser:browser",
"//net:net",
"//third_party/protobuf:protobuf_lite",
......
// Copyright 2017 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 "components/safe_browsing/password_protection/password_protection_request.h"
#include "base/memory/ptr_util.h"
......@@ -8,6 +9,7 @@
#include "base/metrics/histogram_macros.h"
#include "components/data_use_measurement/core/data_use_user_data.h"
#include "components/password_manager/core/browser/password_reuse_detector.h"
#include "components/safe_browsing_db/whitelist_checker_client.h"
#include "content/public/browser/web_contents.h"
#include "net/base/escape.h"
#include "net/base/load_flags.h"
......@@ -50,26 +52,37 @@ PasswordProtectionRequest::~PasswordProtectionRequest() {
void PasswordProtectionRequest::Start() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
CheckWhitelistOnUIThread();
CheckWhitelist();
}
void PasswordProtectionRequest::CheckWhitelistOnUIThread() {
void PasswordProtectionRequest::CheckWhitelist() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
bool* match_whitelist = new bool(false);
tracker_.PostTaskAndReply(
// Start a task on the IO thread to check the whitelist. It may
// callback immediately on the IO thread or take some time if a full-hash-
// check is required.
auto result_callback = base::Bind(&OnWhitelistCheckDoneOnIO, GetWeakPtr());
tracker_.PostTask(
BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get(), FROM_HERE,
base::Bind(&PasswordProtectionService::CheckCsdWhitelistOnIOThread,
base::Unretained(password_protection_service_),
main_frame_url_, match_whitelist),
base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, this,
base::Owned(match_whitelist)));
base::Bind(&WhitelistCheckerClient::StartCheckCsdWhitelist,
password_protection_service_->database_manager(),
main_frame_url_, result_callback));
}
// static
void PasswordProtectionRequest::OnWhitelistCheckDoneOnIO(
base::WeakPtr<PasswordProtectionRequest> weak_request,
bool match_whitelist) {
// Don't access weak_request on IO thread. Move it back to UI thread first.
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, weak_request,
match_whitelist));
}
void PasswordProtectionRequest::OnWhitelistCheckDone(
const bool* match_whitelist) {
void PasswordProtectionRequest::OnWhitelistCheckDone(bool match_whitelist) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (*match_whitelist)
if (match_whitelist)
Finish(PasswordProtectionService::MATCHED_WHITELIST, nullptr);
else
CheckCachedVerdicts();
......
......@@ -85,16 +85,16 @@ class PasswordProtectionRequest : public base::RefCountedThreadSafe<
friend class base::DeleteHelper<PasswordProtectionRequest>;
~PasswordProtectionRequest() override;
void CheckWhitelistOnUIThread();
// Start checking the whitelist.
void CheckWhitelist();
static void OnWhitelistCheckDoneOnIO(
base::WeakPtr<PasswordProtectionRequest> weak_request,
bool match_whitelist);
// If |main_frame_url_| matches whitelist, call Finish() immediately;
// otherwise call CheckCachedVerdicts(). It is the task posted back to UI
// thread by the PostTaskAndReply() in CheckWhitelistOnUIThread().
// |match_whitelist| boolean pointer is used to pass whitelist checking result
// between UI and IO thread. The object it points to will be deleted at the
// end of OnWhitelistCheckDone(), since base::Owned() transfers its ownership
// to this callback function.
void OnWhitelistCheckDone(const bool* match_whitelist);
// otherwise call CheckCachedVerdicts().
void OnWhitelistCheckDone(bool match_whitelist);
// Looks up cached verdicts. If verdict is already cached, call SendRequest();
// otherwise call Finish().
......
......@@ -18,6 +18,7 @@
#include "components/safe_browsing/password_protection/password_protection_request.h"
#include "components/safe_browsing_db/database_manager.h"
#include "components/safe_browsing_db/v4_protocol_manager_util.h"
#include "components/safe_browsing_db/whitelist_checker_client.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_contents.h"
#include "google_apis/google_api_keys.h"
......@@ -105,14 +106,6 @@ PasswordProtectionService::~PasswordProtectionService() {
weak_factory_.InvalidateWeakPtrs();
}
void PasswordProtectionService::CheckCsdWhitelistOnIOThread(
const GURL& url,
bool* check_result) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
*check_result =
url.is_valid() ? database_manager_->MatchCsdWhitelistUrl(url) : true;
}
bool PasswordProtectionService::CanGetReputationOfURL(const GURL& url) {
if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS())
return false;
......
......@@ -19,6 +19,10 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using testing::_;
using testing::ElementsAre;
using testing::Return;
namespace {
const char kFormActionUrl[] = "https://form_action.com/";
......@@ -36,7 +40,8 @@ class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
public:
MockSafeBrowsingDatabaseManager() {}
MOCK_METHOD1(MatchCsdWhitelistUrl, bool(const GURL&));
MOCK_METHOD2(CheckCsdWhitelistUrl,
AsyncMatch(const GURL&, SafeBrowsingDatabaseManager::Client*));
protected:
~MockSafeBrowsingDatabaseManager() override {}
......@@ -167,8 +172,9 @@ class PasswordProtectionServiceTest : public testing::Test {
void InitializeAndStartPasswordOnFocusRequest(bool match_whitelist,
int timeout_in_ms) {
GURL target_url(kTargetUrl);
EXPECT_CALL(*database_manager_.get(), MatchCsdWhitelistUrl(target_url))
.WillRepeatedly(testing::Return(match_whitelist));
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url, _))
.WillRepeatedly(
Return(match_whitelist ? AsyncMatch::MATCH : AsyncMatch::NO_MATCH));
request_ = new PasswordProtectionRequest(
nullptr, target_url, GURL(kFormActionUrl), GURL(kPasswordFrameUrl),
......@@ -181,8 +187,9 @@ class PasswordProtectionServiceTest : public testing::Test {
bool match_whitelist,
int timeout_in_ms) {
GURL target_url(kTargetUrl);
EXPECT_CALL(*database_manager_.get(), MatchCsdWhitelistUrl(target_url))
.WillRepeatedly(testing::Return(match_whitelist));
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url, _))
.WillRepeatedly(
Return(match_whitelist ? AsyncMatch::MATCH : AsyncMatch::NO_MATCH));
request_ = new PasswordProtectionRequest(
nullptr, target_url, GURL(), GURL(), saved_domain,
......@@ -449,7 +456,7 @@ TEST_F(PasswordProtectionServiceTest, TestNoRequestSentForWhitelistedURL) {
EXPECT_EQ(nullptr, password_protection_service_->latest_response());
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(4 /* MATCHED_WHITELIST */, 1)));
ElementsAre(base::Bucket(4 /* MATCHED_WHITELIST */, 1)));
}
TEST_F(PasswordProtectionServiceTest, TestNoRequestSentIfVerdictAlreadyCached) {
......@@ -461,7 +468,7 @@ TEST_F(PasswordProtectionServiceTest, TestNoRequestSentIfVerdictAlreadyCached) {
base::RunLoop().RunUntilIdle();
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(5 /* RESPONSE_ALREADY_CACHED */, 1)));
ElementsAre(base::Bucket(5 /* RESPONSE_ALREADY_CACHED */, 1)));
EXPECT_EQ(LoginReputationClientResponse::LOW_REPUTATION,
password_protection_service_->latest_response()->verdict_type());
}
......@@ -480,7 +487,7 @@ TEST_F(PasswordProtectionServiceTest, TestResponseFetchFailed) {
EXPECT_EQ(nullptr, password_protection_service_->latest_response());
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(9 /* FETCH_FAILED */, 1)));
ElementsAre(base::Bucket(9 /* FETCH_FAILED */, 1)));
}
TEST_F(PasswordProtectionServiceTest, TestMalformedResponse) {
......@@ -499,7 +506,7 @@ TEST_F(PasswordProtectionServiceTest, TestMalformedResponse) {
EXPECT_EQ(nullptr, password_protection_service_->latest_response());
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(10 /* RESPONSE_MALFORMED */, 1)));
ElementsAre(base::Bucket(10 /* RESPONSE_MALFORMED */, 1)));
}
TEST_F(PasswordProtectionServiceTest, TestRequestTimedout) {
......@@ -510,7 +517,7 @@ TEST_F(PasswordProtectionServiceTest, TestRequestTimedout) {
EXPECT_EQ(nullptr, password_protection_service_->latest_response());
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(3 /* TIMEDOUT */, 1)));
ElementsAre(base::Bucket(3 /* TIMEDOUT */, 1)));
}
TEST_F(PasswordProtectionServiceTest, TestRequestAndResponseSuccessfull) {
......@@ -530,9 +537,9 @@ TEST_F(PasswordProtectionServiceTest, TestRequestAndResponseSuccessfull) {
base::RunLoop().RunUntilIdle();
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(1 /* SUCCEEDED */, 1)));
ElementsAre(base::Bucket(1 /* SUCCEEDED */, 1)));
EXPECT_THAT(histograms_.GetAllSamples(kVerdictHistogramName),
testing::ElementsAre(base::Bucket(3 /* PHISHING */, 1)));
ElementsAre(base::Bucket(3 /* PHISHING */, 1)));
LoginReputationClientResponse* actual_response =
password_protection_service_->latest_response();
EXPECT_EQ(expected_response.verdict_type(), actual_response->verdict_type());
......@@ -545,8 +552,8 @@ TEST_F(PasswordProtectionServiceTest, TestRequestAndResponseSuccessfull) {
TEST_F(PasswordProtectionServiceTest, TestTearDownWithPendingRequests) {
histograms_.ExpectTotalCount(kPasswordOnFocusRequestOutcomeHistogramName, 0);
GURL target_url(kTargetUrl);
EXPECT_CALL(*database_manager_.get(), MatchCsdWhitelistUrl(target_url))
.WillRepeatedly(testing::Return(false));
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url, _))
.WillRepeatedly(Return(AsyncMatch::NO_MATCH));
password_protection_service_->StartRequest(
nullptr, target_url, GURL("http://foo.com/submit"),
GURL("http://foo.com/frame"), std::string(),
......@@ -558,7 +565,7 @@ TEST_F(PasswordProtectionServiceTest, TestTearDownWithPendingRequests) {
EXPECT_THAT(
histograms_.GetAllSamples(kPasswordOnFocusRequestOutcomeHistogramName),
testing::ElementsAre(base::Bucket(2 /* CANCELED */, 1)));
ElementsAre(base::Bucket(2 /* CANCELED */, 1)));
}
TEST_F(PasswordProtectionServiceTest, TestCleanUpExpiredVerdict) {
......
......@@ -72,6 +72,10 @@ static_library("database_manager") {
"//net",
"//url",
]
public_deps = [
":safebrowsing_proto",
]
}
static_library("hit_report") {
......@@ -406,6 +410,7 @@ source_set("unit_tests_shared") {
"util_unittest.cc",
"v4_get_hash_protocol_manager_unittest.cc",
"v4_protocol_manager_util_unittest.cc",
"whitelist_checker_client_unittest.cc",
]
deps = [
":database_manager",
......@@ -416,6 +421,7 @@ source_set("unit_tests_shared") {
":v4_get_hash_protocol_manager",
":v4_protocol_manager_util",
":v4_test_util",
":whitelist_checker_client",
"//base",
"//content/public/browser",
"//content/test:test_support",
......@@ -489,3 +495,30 @@ source_set("unit_tests_mobile") {
cflags = [ "/wd4267" ] # Conversion from size_t to 'type'.
}
}
static_library("whitelist_checker_client") {
sources = [
"whitelist_checker_client.cc",
"whitelist_checker_client.h",
]
deps = [
":database_manager",
"//base:base",
]
}
source_set("whitelist_checker_client_unittest") {
testonly = true
sources = [
"whitelist_checker_client_unittest.cc",
]
deps = [
":database_manager",
":test_database_manager",
":whitelist_checker_client",
"//base:base",
"//base/test:test_support",
"//testing/gmock:gmock",
"//testing/gtest:gtest",
]
}
// Copyright 2017 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 "components/safe_browsing_db/whitelist_checker_client.h"
#include "base/bind.h"
namespace safe_browsing {
// Static
void WhitelistCheckerClient::StartCheckCsdWhitelist(
scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
const GURL& url,
base::Callback<void(bool)> callback_for_result) {
// TODO(nparker): Maybe also call SafeBrowsingDatabaseManager::CanCheckUrl()
if (!url.is_valid()) {
callback_for_result.Run(true /* is_whitelisted */);
return;
}
// Make a client for each request. The caller could have several in
// flight at once.
std::unique_ptr<WhitelistCheckerClient> client =
base::MakeUnique<WhitelistCheckerClient>(callback_for_result);
AsyncMatch match = database_manager->CheckCsdWhitelistUrl(url, client.get());
switch (match) {
case AsyncMatch::MATCH:
callback_for_result.Run(true /* is_whitelisted */);
break;
case AsyncMatch::NO_MATCH:
callback_for_result.Run(false /* is_whitelisted */);
break;
case AsyncMatch::ASYNC:
// Client is now self-owned. When it gets called back with the result,
// it will delete itself.
client.release();
break;
}
}
WhitelistCheckerClient::WhitelistCheckerClient(
base::Callback<void(bool)> callback_for_result)
: callback_for_result_(callback_for_result), weak_factory_(this) {
// Set a timer to fail open, i.e. call it "whitelisted", if the full
// check takes too long.
auto timeout_callback =
base::Bind(&WhitelistCheckerClient::OnCheckWhitelistUrlResult,
weak_factory_.GetWeakPtr(), true /* is_whitelisted */);
timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(kTimeoutMsec),
timeout_callback);
}
WhitelistCheckerClient::~WhitelistCheckerClient() {}
// SafeBrowsingDatabaseMananger::Client impl
void WhitelistCheckerClient::OnCheckWhitelistUrlResult(bool is_whitelisted) {
// This method is invoked only if we're already self-owned.
callback_for_result_.Run(is_whitelisted);
delete this;
}
} // namespace safe_browsing
// Copyright 2017 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 COMPONENTS_SAFE_BROWSING_DB_WHITELIST_CHECKER_CLIENT_H_
#define COMPONENTS_SAFE_BROWSING_DB_WHITELIST_CHECKER_CLIENT_H_
#include "base/memory/ptr_util.h"
#include "base/memory/weak_ptr.h"
#include "base/timer/timer.h"
#include "components/safe_browsing_db/database_manager.h"
namespace safe_browsing {
// This provides a simpler interface to
// SafeBrowsingDatabaseManager::CheckCsdWhitelistUrl() for callers that
// don't want to track their own clients.
class WhitelistCheckerClient : public SafeBrowsingDatabaseManager::Client {
public:
using BoolCallback = base::Callback<void(bool /* is_whitelisted */)>;
// Static method to instantiate and start a check. The callback will
// be invoked when it's done, times out, or if database_manager gets
// shut down. Must be called on IO thread.
static void StartCheckCsdWhitelist(
scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
const GURL& url,
BoolCallback callback_for_result);
WhitelistCheckerClient(BoolCallback callback_for_result);
~WhitelistCheckerClient() override;
// SafeBrowsingDatabaseMananger::Client impl
void OnCheckWhitelistUrlResult(bool is_whitelisted) override;
protected:
static const int kTimeoutMsec = 5000;
base::OneShotTimer timer_;
BoolCallback callback_for_result_;
base::WeakPtrFactory<WhitelistCheckerClient> weak_factory_;
private:
WhitelistCheckerClient();
};
} // namespace safe_browsing
#endif // COMPONENTS_SAFE_BROWSING_DB_WHITELIST_CHECKER_CLIENT_H_
// Copyright 2017 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 "components/safe_browsing_db/whitelist_checker_client.h"
#include "base/bind.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/test/mock_callback.h"
#include "base/test/test_mock_time_task_runner.h"
#include "base/threading/thread_task_runner_handle.h"
#include "components/safe_browsing_db/test_database_manager.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace safe_browsing {
using base::TimeDelta;
using testing::_;
using testing::Return;
using testing::SaveArg;
using BoolCallback = base::Callback<void(bool /* is_whitelisted */)>;
using MockBoolCallback = testing::StrictMock<base::MockCallback<BoolCallback>>;
class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
public:
MockSafeBrowsingDatabaseManager(){};
MOCK_METHOD2(CheckCsdWhitelistUrl,
AsyncMatch(const GURL&, SafeBrowsingDatabaseManager::Client*));
protected:
~MockSafeBrowsingDatabaseManager() override {}
private:
DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager);
};
class WhitelistCheckerClientTest : public testing::Test {
public:
WhitelistCheckerClientTest() : target_url_("http://foo.bar"){};
void SetUp() override {
database_manager_ = new MockSafeBrowsingDatabaseManager;
task_runner_ = new base::TestMockTimeTaskRunner(base::Time::Now(),
base::TimeTicks::Now());
message_loop_.reset(new base::MessageLoop);
message_loop_->SetTaskRunner(task_runner_);
}
void TearDown() override {
// Verify no callback is remaining.
// TODO(nparker): We should somehow EXPECT that no entry is remaining,
// rather than just invoking it.
task_runner_->FastForwardUntilNoTasksRemain();
}
protected:
GURL target_url_;
scoped_refptr<MockSafeBrowsingDatabaseManager> database_manager_;
std::unique_ptr<base::MessageLoop> message_loop_;
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
};
TEST_F(WhitelistCheckerClientTest, TestMatch) {
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url_, _))
.WillOnce(Return(AsyncMatch::MATCH));
MockBoolCallback callback;
EXPECT_CALL(callback, Run(true /* is_whitelisted */));
WhitelistCheckerClient::StartCheckCsdWhitelist(database_manager_, target_url_,
callback.Get());
}
TEST_F(WhitelistCheckerClientTest, TestNoMatch) {
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url_, _))
.WillOnce(Return(AsyncMatch::NO_MATCH));
MockBoolCallback callback;
EXPECT_CALL(callback, Run(false /* is_whitelisted */));
WhitelistCheckerClient::StartCheckCsdWhitelist(database_manager_, target_url_,
callback.Get());
}
TEST_F(WhitelistCheckerClientTest, TestAsyncNoMatch) {
SafeBrowsingDatabaseManager::Client* client;
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url_, _))
.WillOnce(DoAll(SaveArg<1>(&client), Return(AsyncMatch::ASYNC)));
MockBoolCallback callback;
WhitelistCheckerClient::StartCheckCsdWhitelist(database_manager_, target_url_,
callback.Get());
// Callback should not be called yet.
EXPECT_CALL(callback, Run(false /* is_whitelisted */));
// The self-owned client deletes itself here.
client->OnCheckWhitelistUrlResult(false);
}
TEST_F(WhitelistCheckerClientTest, TestAsyncTimeout) {
EXPECT_CALL(*database_manager_.get(), CheckCsdWhitelistUrl(target_url_, _))
.WillOnce(Return(AsyncMatch::ASYNC));
MockBoolCallback callback;
WhitelistCheckerClient::StartCheckCsdWhitelist(database_manager_, target_url_,
callback.Get());
task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(1));
// No callback yet.
EXPECT_CALL(callback, Run(true /* is_whitelisted */));
task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(5));
}
} // namespace safe_browsing
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment