Commit 81cad473 authored by Douglas Creager's avatar Douglas Creager Committed by Commit Bot

Reporting: Clearly separate IO and UI thread concerns

This patch separates ReportingPermissionsChecker class in two, making it
clearer which parts belong on the UI and IO threads.  The Profile
creates an instance of the UI class on the UI thread, and then creates a
subsidiary IO instance, which is how ChromeNetworkDelegate actually
performs permissions checks.  The IO instance as a WeakPtr to the UI
instance, since it must delegate to the IO thread to actually perform
the check.  If the Profile starts tearing down itself (and by extension,
the UI instance), the WeakPtr will be invalidated.  But importantly, the
IO instance will still be well-formed, in case the network delegate is
trying any outstanding permissions checks.

Bug: 704259
Change-Id: I2b8d437c835a00a90d59aa7e3f39a49d83f511f2
Reviewed-on: https://chromium-review.googlesource.com/1059455
Commit-Queue: Douglas Creager <dcreager@chromium.org>
Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Reviewed-by: default avatarWez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559526}
parent b4a07f8f
......@@ -809,6 +809,8 @@ jumbo_split_static_library("browser") {
"net/quota_policy_channel_id_store.h",
"net/referrer.cc",
"net/referrer.h",
"net/reporting_permissions_checker.cc",
"net/reporting_permissions_checker.h",
"net/safe_search_util.cc",
"net/safe_search_util.h",
"net/service_providers_win.cc",
......
......@@ -521,7 +521,7 @@ bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
bool ChromeNetworkDelegate::OnCanQueueReportingReport(
const url::Origin& origin) const {
if (!cookie_settings_)
return true;
return false;
return cookie_settings_->IsCookieAccessAllowed(origin.GetURL(),
origin.GetURL());
......@@ -530,28 +530,21 @@ bool ChromeNetworkDelegate::OnCanQueueReportingReport(
void ChromeNetworkDelegate::OnCanSendReportingReports(
std::set<url::Origin> origins,
base::OnceCallback<void(std::set<url::Origin>)> result_callback) const {
if (!cookie_settings_) {
if (!reporting_permissions_checker_) {
origins.clear();
std::move(result_callback).Run(std::move(origins));
return;
}
for (auto it = origins.begin(); it != origins.end();) {
const auto& origin = *it;
if (!cookie_settings_->IsCookieAccessAllowed(origin.GetURL(),
origin.GetURL())) {
origins.erase(it++);
} else {
++it;
}
}
std::move(result_callback).Run(std::move(origins));
reporting_permissions_checker_->FilterReportingOrigins(
std::move(origins), std::move(result_callback));
}
bool ChromeNetworkDelegate::OnCanSetReportingClient(
const url::Origin& origin,
const GURL& endpoint) const {
if (!cookie_settings_)
return true;
return false;
return cookie_settings_->IsCookieAccessAllowed(endpoint, origin.GetURL());
}
......@@ -560,7 +553,7 @@ bool ChromeNetworkDelegate::OnCanUseReportingClient(
const url::Origin& origin,
const GURL& endpoint) const {
if (!cookie_settings_)
return true;
return false;
return cookie_settings_->IsCookieAccessAllowed(endpoint, origin.GetURL());
}
......
......@@ -17,6 +17,7 @@
#include "base/memory/ref_counted.h"
#include "base/values.h"
#include "build/build_config.h"
#include "chrome/browser/net/reporting_permissions_checker.h"
#include "chrome/browser/net/safe_search_util.h"
#include "components/domain_reliability/monitor.h"
#include "components/prefs/pref_member.h"
......@@ -106,6 +107,16 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
return domain_reliability_monitor_.get();
}
void set_reporting_permissions_checker(
std::unique_ptr<ReportingPermissionsChecker>
reporting_permissions_checker) {
reporting_permissions_checker_ = std::move(reporting_permissions_checker);
}
ReportingPermissionsChecker* reporting_permissions_checker() {
return reporting_permissions_checker_.get();
}
void set_data_use_aggregator(
data_usage::DataUseAggregator* data_use_aggregator,
bool is_data_usage_off_the_record);
......@@ -214,6 +225,7 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
// Weak, owned by our owner.
std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
domain_reliability_monitor_;
std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker_;
bool experimental_web_platform_features_enabled_;
......
......@@ -18,7 +18,9 @@
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/content_settings/cookie_settings_factory.h"
#include "chrome/browser/net/reporting_permissions_checker.h"
#include "chrome/browser/net/safe_search_util.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
......@@ -34,6 +36,7 @@
#include "content/public/common/content_switches.h"
#include "content/public/common/previews_state.h"
#include "content/public/common/resource_type.h"
#include "content/public/test/mock_permission_manager.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "content/public/test/test_utils.h"
#include "extensions/buildflags/buildflags.h"
......@@ -155,7 +158,7 @@ class ChromeNetworkDelegateTest : public testing::Test {
ASSERT_TRUE(profile_manager_->SetUp());
}
void Initialize() {
virtual void Initialize() {
network_delegate_.reset(
new ChromeNetworkDelegate(forwarder(), &enable_referrers_));
context_->set_client_socket_factory(&socket_factory_);
......@@ -647,3 +650,75 @@ TEST(ChromeNetworkDelegateStaticTest, IsAccessAllowed) {
EXPECT_FALSE(IsAccessAllowed(external_storage_path.AsUTF8Unsafe(), ""));
#endif
}
namespace {
class TestingPermissionProfile : public TestingProfile {
public:
TestingPermissionProfile() = default;
content::MockPermissionManager* mock_permission_manager() {
return &mock_permission_manager_;
}
content::PermissionManager* GetPermissionManager() override {
return &mock_permission_manager_;
}
private:
content::MockPermissionManager mock_permission_manager_;
};
} // namespace
class ChromeNetworkDelegateReportingTest : public ChromeNetworkDelegateTest {
public:
ChromeNetworkDelegateReportingTest() : factory_(&profile_) {}
content::MockPermissionManager* mock_permission_manager() {
return profile_.mock_permission_manager();
}
void Initialize() override {
ChromeNetworkDelegateTest::Initialize();
chrome_network_delegate()->set_reporting_permissions_checker(
factory_.CreateChecker());
}
protected:
TestingPermissionProfile profile_;
ReportingPermissionsCheckerFactory factory_;
DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegateReportingTest);
};
TEST_F(ChromeNetworkDelegateReportingTest, ChecksReportingPermissions) {
using testing::_;
using testing::Return;
Initialize();
auto origin1 = url::Origin::Create(GURL("https://example.com/"));
auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
std::set<url::Origin> origins = {origin1, origin2};
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin1.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin2.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
std::set<url::Origin> allowed_origins;
chrome_network_delegate()->CanSendReportingReports(
std::move(origins),
base::BindOnce(
[](std::set<url::Origin>* dest, std::set<url::Origin> result) {
*dest = std::move(result);
},
&allowed_origins));
content::RunAllTasksUntilIdle();
ASSERT_EQ(1u, allowed_origins.size());
EXPECT_EQ(origin1, *allowed_origins.begin());
}
// Copyright 2018 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/net/reporting_permissions_checker.h"
#include "base/bind.h"
#include "chrome/browser/profiles/profile.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/permission_manager.h"
#include "content/public/browser/permission_type.h"
#include "url/gurl.h"
#include "url/origin.h"
ReportingPermissionsChecker::ReportingPermissionsChecker(
base::WeakPtr<Profile> weak_profile)
: weak_profile_(weak_profile) {}
ReportingPermissionsChecker::~ReportingPermissionsChecker() = default;
void ReportingPermissionsChecker::FilterReportingOrigins(
std::set<url::Origin> origins,
base::OnceCallback<void(std::set<url::Origin>)> result_callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
content::BrowserThread::PostTaskAndReplyWithResult(
content::BrowserThread::UI, FROM_HERE,
base::BindOnce(
&ReportingPermissionsCheckerFactory::DoFilterReportingOrigins,
weak_profile_, std::move(origins)),
std::move(result_callback));
}
ReportingPermissionsCheckerFactory::ReportingPermissionsCheckerFactory(
Profile* profile)
: weak_factory_(profile) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
}
ReportingPermissionsCheckerFactory::~ReportingPermissionsCheckerFactory() =
default;
std::unique_ptr<ReportingPermissionsChecker>
ReportingPermissionsCheckerFactory::CreateChecker() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
return std::make_unique<ReportingPermissionsChecker>(
weak_factory_.GetWeakPtr());
}
// static
std::set<url::Origin>
ReportingPermissionsCheckerFactory::DoFilterReportingOrigins(
base::WeakPtr<Profile> weak_profile,
std::set<url::Origin> origins) {
if (!weak_profile) {
return std::set<url::Origin>();
}
content::PermissionManager* permission_manager =
weak_profile->GetPermissionManager();
if (!permission_manager) {
// Default to prohibiting all Reporting uploads if we don't have a
// PermissionManager.
return std::set<url::Origin>();
}
for (auto it = origins.begin(); it != origins.end();) {
GURL origin = it->GetURL();
bool allowed = permission_manager->GetPermissionStatus(
content::PermissionType::BACKGROUND_SYNC, origin,
origin) == blink::mojom::PermissionStatus::GRANTED;
if (!allowed) {
origins.erase(it++);
} else {
++it;
}
}
return origins;
}
// Copyright 2018 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_NET_REPORTING_PERMISSIONS_CHECKER_H_
#define CHROME_BROWSER_NET_REPORTING_PERMISSIONS_CHECKER_H_
#include <memory>
#include <set>
#include "base/callback.h"
#include "base/memory/weak_ptr.h"
class Profile;
namespace url {
class Origin;
}
class ReportingPermissionsCheckerFactory;
// Used by the Reporting API to check whether the user has allowed reports to be
// uploaded for particular origins, via the BACKGROUND_SYNC permission.
//
// Instances of this class will live on and be used from the IO thread. The
// actual permission checking has to happen on the UI thread, via the current
// Profile; this class takes care of managing the thread orchestration for you.
// In particular, instances of this class are guaranteed to remain valid even
// once the Profile has started tearing itself down on the UI thread.
class ReportingPermissionsChecker {
public:
explicit ReportingPermissionsChecker(base::WeakPtr<Profile> weak_profile);
~ReportingPermissionsChecker();
// Checks whether each origin in |origins| has the BACKGROUND_SYNC permission
// set, removing any that don't. Call this from the IO thread. The filter
// will perform the check on the UI thread, and invoke |result_callback| back
// on the IO thread with the result. If the Profile has already started
// tearing itself down, the callback will never be invoked.
void FilterReportingOrigins(
std::set<url::Origin> origins,
base::OnceCallback<void(std::set<url::Origin>)> result_callback);
private:
base::WeakPtr<Profile> weak_profile_;
DISALLOW_COPY_AND_ASSIGN(ReportingPermissionsChecker);
};
// The actual BACKGROUND_SYNC permission checks need to happen on the UI thread.
// This class lives on the UI thread, and is owned by ProfileImpl.
class ReportingPermissionsCheckerFactory {
public:
// Does not take ownership of |profile|, which must outlive this instance.
// Must be called from the UI thread.
explicit ReportingPermissionsCheckerFactory(Profile* profile);
~ReportingPermissionsCheckerFactory();
// Creates a new ReportingPermissionsChecker that can safely be used from
// the IO thread, even when the owning Profile has started tearing itself
// down. Must be called from the UI thread.
std::unique_ptr<ReportingPermissionsChecker> CreateChecker();
private:
friend class ReportingPermissionsChecker;
// Checks whether each origin in |origins| has the BACKGROUND_SYNC permission
// set, removing any that don't. This must be called from the UI thread.
static std::set<url::Origin> DoFilterReportingOrigins(
base::WeakPtr<Profile> profile,
std::set<url::Origin> origins);
base::WeakPtrFactory<Profile> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ReportingPermissionsCheckerFactory);
};
#endif // CHROME_BROWSER_NET_REPORTING_PERMISSIONS_CHECKER_H_
// Copyright (c) 2018 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/net/reporting_permissions_checker.h"
#include <set>
#include "base/bind.h"
#include "chrome/test/base/testing_profile.h"
#include "content/public/test/mock_permission_manager.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "content/public/test/test_utils.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"
#include "url/origin.h"
namespace {
using testing::_;
using testing::Eq;
using testing::IsEmpty;
using testing::Return;
using testing::UnorderedElementsAre;
class TestingPermissionProfile : public TestingProfile {
public:
TestingPermissionProfile() = default;
content::MockPermissionManager* mock_permission_manager() {
return &mock_permission_manager_;
}
content::PermissionManager* GetPermissionManager() override {
return &mock_permission_manager_;
}
private:
content::MockPermissionManager mock_permission_manager_;
};
} // namespace
class ReportingPermissionsCheckerTest : public testing::Test {
public:
ReportingPermissionsCheckerTest()
: thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
reporting_permissions_checker_factory_(&profile_),
reporting_permissions_checker_(
reporting_permissions_checker_factory_.CreateChecker()) {}
content::MockPermissionManager* mock_permission_manager() {
return profile_.mock_permission_manager();
}
protected:
content::TestBrowserThreadBundle thread_bundle_;
TestingPermissionProfile profile_;
ReportingPermissionsCheckerFactory reporting_permissions_checker_factory_;
std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker_;
DISALLOW_COPY_AND_ASSIGN(ReportingPermissionsCheckerTest);
};
TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsBothGranted) {
auto origin1 = url::Origin::Create(GURL("https://example.com/"));
auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
std::set<url::Origin> origins = {origin1, origin2};
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin1.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin2.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
std::set<url::Origin> allowed_origins;
reporting_permissions_checker_->FilterReportingOrigins(
std::move(origins),
base::BindOnce(
[](std::set<url::Origin>* dest, std::set<url::Origin> result) {
*dest = std::move(result);
},
&allowed_origins));
content::RunAllTasksUntilIdle();
EXPECT_THAT(allowed_origins, UnorderedElementsAre(Eq(origin1), Eq(origin2)));
}
TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsBothDenied) {
auto origin1 = url::Origin::Create(GURL("https://example.com/"));
auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
std::set<url::Origin> origins = {origin1, origin2};
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin1.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin2.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
std::set<url::Origin> allowed_origins;
reporting_permissions_checker_->FilterReportingOrigins(
std::move(origins),
base::BindOnce(
[](std::set<url::Origin>* dest, std::set<url::Origin> result) {
*dest = std::move(result);
},
&allowed_origins));
content::RunAllTasksUntilIdle();
EXPECT_THAT(allowed_origins, IsEmpty());
}
TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsMixed) {
auto origin1 = url::Origin::Create(GURL("https://example.com/"));
auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
std::set<url::Origin> origins = {origin1, origin2};
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin1.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
EXPECT_CALL(*mock_permission_manager(),
GetPermissionStatus(_, origin2.GetURL(), _))
.WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
std::set<url::Origin> allowed_origins;
reporting_permissions_checker_->FilterReportingOrigins(
std::move(origins),
base::BindOnce(
[](std::set<url::Origin>* dest, std::set<url::Origin> result) {
*dest = std::move(result);
},
&allowed_origins));
content::RunAllTasksUntilIdle();
EXPECT_THAT(allowed_origins, UnorderedElementsAre(Eq(origin1)));
}
......@@ -412,7 +412,8 @@ ProfileImpl::ProfileImpl(
last_session_exit_type_(EXIT_NORMAL),
start_time_(base::Time::Now()),
delegate_(delegate),
predictor_(nullptr) {
predictor_(nullptr),
reporting_permissions_checker_factory_(this) {
TRACE_EVENT0("browser,startup", "ProfileImpl::ctor")
DCHECK(!path.empty()) << "Using an empty path will attempt to write "
<< "profile files to the root directory!";
......@@ -647,6 +648,7 @@ void ProfileImpl::DoFinalInit() {
io_data_.Init(media_cache_path, media_cache_max_size, extensions_cookie_path,
GetPath(), predictor_, GetSpecialStoragePolicy(),
reporting_permissions_checker_factory_.CreateChecker(),
CreateDomainReliabilityMonitor(local_state));
#if BUILDFLAG(ENABLE_PLUGINS)
......
......@@ -16,6 +16,7 @@
#include "base/memory/ref_counted.h"
#include "base/timer/timer.h"
#include "build/build_config.h"
#include "chrome/browser/net/reporting_permissions_checker.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_impl_io_data.h"
#include "chrome/common/buildflags.h"
......@@ -282,6 +283,8 @@ class ProfileImpl : public Profile {
chrome_browser_net::Predictor* predictor_;
ReportingPermissionsCheckerFactory reporting_permissions_checker_factory_;
// Used to post schedule CT policy updates
base::OneShotTimer ct_policy_update_timer_;
......
......@@ -28,6 +28,7 @@
#include "chrome/browser/net/chrome_network_delegate.h"
#include "chrome/browser/net/predictor.h"
#include "chrome/browser/net/quota_policy_channel_id_store.h"
#include "chrome/browser/net/reporting_permissions_checker.h"
#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h"
#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
......@@ -143,6 +144,7 @@ void ProfileImplIOData::Handle::Init(
const base::FilePath& profile_path,
chrome_browser_net::Predictor* predictor,
storage::SpecialStoragePolicy* special_storage_policy,
std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker,
std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
domain_reliability_monitor) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
......@@ -159,6 +161,8 @@ void ProfileImplIOData::Handle::Init(
lazy_params->persist_session_cookies =
profile_->ShouldPersistSessionCookies();
lazy_params->special_storage_policy = special_storage_policy;
lazy_params->reporting_permissions_checker =
std::move(reporting_permissions_checker);
lazy_params->domain_reliability_monitor =
std::move(domain_reliability_monitor);
......@@ -428,6 +432,11 @@ ProfileImplIOData::ConfigureNetworkDelegate(
std::move(lazy_params_->domain_reliability_monitor));
}
if (lazy_params_->reporting_permissions_checker) {
chrome_network_delegate->set_reporting_permissions_checker(
std::move(lazy_params_->reporting_permissions_checker));
}
return data_reduction_proxy_io_data()->CreateNetworkDelegate(
io_thread->globals()->data_use_ascriber->CreateNetworkDelegate(
std::move(chrome_network_delegate),
......
......@@ -14,6 +14,8 @@
#include "chrome/browser/profiles/profile_io_data.h"
#include "components/prefs/pref_store.h"
class ReportingPermissionsChecker;
namespace chrome_browser_net {
class Predictor;
} // namespace chrome_browser_net
......@@ -48,6 +50,8 @@ class ProfileImplIOData : public ProfileIOData {
const base::FilePath& profile_path,
chrome_browser_net::Predictor* predictor,
storage::SpecialStoragePolicy* special_storage_policy,
std::unique_ptr<ReportingPermissionsChecker>
reporting_permissions_checker,
std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
domain_reliability_monitor);
......@@ -130,6 +134,7 @@ class ProfileImplIOData : public ProfileIOData {
bool restore_old_session_cookies;
bool persist_session_cookies;
scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy;
std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker;
std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
domain_reliability_monitor;
};
......
......@@ -2412,6 +2412,7 @@ test("unit_tests") {
"../browser/net/predictor_unittest.cc",
"../browser/net/probe_message_unittest.cc",
"../browser/net/quota_policy_channel_id_store_unittest.cc",
"../browser/net/reporting_permissions_checker_unittest.cc",
"../browser/net/safe_search_util_unittest.cc",
"../browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc",
"../browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc",
......
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