From 811d06033578761d8238b59912e473cc4c159cb1 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 4 Oct 2018 02:17:19 +0900 Subject: [PATCH 1/4] Add ShieldSettingsPersistTest For now, this test is failed. Need to make this test pass. --- .../api/brave_shields_api_browsertest.cc | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/browser/extensions/api/brave_shields_api_browsertest.cc b/browser/extensions/api/brave_shields_api_browsertest.cc index 8314fea569fd..21a728364ab5 100644 --- a/browser/extensions/api/brave_shields_api_browsertest.cc +++ b/browser/extensions/api/brave_shields_api_browsertest.cc @@ -6,6 +6,7 @@ #include "brave/browser/extensions/api/brave_shields_api.h" #include "brave/common/brave_paths.h" #include "brave/common/extensions/extension_constants.h" +#include "brave/components/brave_shields/common/brave_shield_constants.h" #include "chrome/browser/content_settings/host_content_settings_map_factory.h" #include "chrome/browser/extensions/api/content_settings/content_settings_api.h" #include "chrome/browser/extensions/api/content_settings/content_settings_api_constants.h" @@ -268,4 +269,31 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, content_settings::SettingSource::SETTING_SOURCE_USER); } +// Checks shields configuration is persisted across the sessions. +IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, + PRE_ShieldSettingsPersistTest) { + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + SetContentSettingCustomScope( + ContentSettingsPattern::Wildcard(), + ContentSettingsPattern::Wildcard(), + CONTENT_SETTINGS_TYPE_PLUGINS, + brave_shields::kHTTPUpgradableResources, + CONTENT_SETTING_ALLOW); + + ContentSetting setting = + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + GetContentSetting(GURL(), GURL(), CONTENT_SETTINGS_TYPE_PLUGINS, + brave_shields::kHTTPUpgradableResources); + EXPECT_EQ(setting, CONTENT_SETTING_ALLOW); +} + +IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, + ShieldSettingsPersistTest) { + ContentSetting setting = + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + GetContentSetting(GURL(), GURL(), CONTENT_SETTINGS_TYPE_PLUGINS, + brave_shields::kHTTPUpgradableResources); + EXPECT_EQ(setting, CONTENT_SETTING_ALLOW); +} + } // namespace extensions From 582f2a2b384e6276bbb4d054bbed18bbd7feca08 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 4 Oct 2018 18:22:58 +0900 Subject: [PATCH 2/4] Makes shields configuration persist across sesstions BravePrefProvider and BraveEphemeralProvider make shields configuration persistent. --- .../core/browser/host_content_settings_map.cc | 12 ++++ .../content_settings/core/browser/BUILD.gn | 4 ++ ...ave_content_settings_ephemeral_provider.cc | 48 +++++++++++++++ ...rave_content_settings_ephemeral_provider.h | 31 ++++++++++ .../brave_content_settings_pref_provider.cc | 61 +++++++++++++++++++ .../brave_content_settings_pref_provider.h | 44 +++++++++++++ ...ser-content_settings_pref_provider.h.patch | 12 ++++ 7 files changed, 212 insertions(+) create mode 100644 chromium_src/components/content_settings/core/browser/host_content_settings_map.cc create mode 100644 components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc create mode 100644 components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h create mode 100644 components/content_settings/core/browser/brave_content_settings_pref_provider.cc create mode 100644 components/content_settings/core/browser/brave_content_settings_pref_provider.h create mode 100644 patches/components-content_settings-core-browser-content_settings_pref_provider.h.patch diff --git a/chromium_src/components/content_settings/core/browser/host_content_settings_map.cc b/chromium_src/components/content_settings/core/browser/host_content_settings_map.cc new file mode 100644 index 000000000000..3c72f8e89e8b --- /dev/null +++ b/chromium_src/components/content_settings/core/browser/host_content_settings_map.cc @@ -0,0 +1,12 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h" +#include "brave/components/content_settings/core/browser/brave_content_settings_pref_provider.h" + +#define EphemeralProvider BraveEphemeralProvider +#define PrefProvider BravePrefProvider +#include "../../../../../../components/content_settings/core/browser/host_content_settings_map.cc" +#undef EphemeralProvider +#undef PrefProvider diff --git a/components/content_settings/core/browser/BUILD.gn b/components/content_settings/core/browser/BUILD.gn index 6a0918b8ed6c..91ee96a54872 100644 --- a/components/content_settings/core/browser/BUILD.gn +++ b/components/content_settings/core/browser/BUILD.gn @@ -1,5 +1,9 @@ source_set("browser") { sources = [ + "brave_content_settings_ephemeral_provider.cc", + "brave_content_settings_ephemeral_provider.h", + "brave_content_settings_pref_provider.cc", + "brave_content_settings_pref_provider.h", "brave_cookie_settings.cc", "brave_cookie_settings.h", "brave_host_content_settings_map.cc", diff --git a/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc new file mode 100644 index 000000000000..274293be2feb --- /dev/null +++ b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc @@ -0,0 +1,48 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h" + +#include "brave/components/brave_shields/common/brave_shield_constants.h" + +namespace { + +bool IsShieldsResourceID( + const content_settings::ResourceIdentifier& resource_identifier) { + if (resource_identifier == brave_shields::kAds || + resource_identifier == brave_shields::kTrackers || + resource_identifier == brave_shields::kHTTPUpgradableResources || + resource_identifier == brave_shields::kJavaScript || + resource_identifier == brave_shields::kFingerprinting || + resource_identifier == brave_shields::kBraveShields || + resource_identifier == brave_shields::kReferrers || + resource_identifier == brave_shields::kCookies) { + return true; + } else { + return false; + } +} + +} + +namespace content_settings { + +bool BraveEphemeralProvider::SetWebsiteSetting( + const ContentSettingsPattern& primary_pattern, + const ContentSettingsPattern& secondary_pattern, + ContentSettingsType content_type, + const ResourceIdentifier& resource_identifier, + base::Value* in_value) { + // Prevent this handle shields configuration. + if (content_type == CONTENT_SETTINGS_TYPE_PLUGINS && + IsShieldsResourceID(resource_identifier)) { + return false; + } + + return EphemeralProvider::SetWebsiteSetting( + primary_pattern, secondary_pattern, + content_type, resource_identifier, in_value); +} + +} // namespace content_settings diff --git a/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h new file mode 100644 index 000000000000..b9ecba8bef8b --- /dev/null +++ b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.h @@ -0,0 +1,31 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_CONTENT_SETTINGS_EPHEMERAL_PROVIDER_H_ +#define BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_CONTENT_SETTINGS_EPHEMERAL_PROVIDER_H_ + +#include "components/content_settings/core/browser/content_settings_ephemeral_provider.h" + +namespace content_settings { + +// See the comments of BravePrefProvider. +class BraveEphemeralProvider : public EphemeralProvider { + public: + using EphemeralProvider::EphemeralProvider; + ~BraveEphemeralProvider() override {} + + private: + // EphemeralProvider overrides: + bool SetWebsiteSetting(const ContentSettingsPattern& primary_pattern, + const ContentSettingsPattern& secondary_pattern, + ContentSettingsType content_type, + const ResourceIdentifier& resource_identifier, + base::Value* value) override; + + DISALLOW_COPY_AND_ASSIGN(BraveEphemeralProvider); +}; + +} // namespace content_settings + +#endif // BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_CONTENT_SETTINGS_EPHEMERAL_PROVIDER_H_ diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider.cc b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc new file mode 100644 index 000000000000..6e59f0f998d1 --- /dev/null +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc @@ -0,0 +1,61 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/components/content_settings/core/browser/brave_content_settings_pref_provider.h" + +#include "base/bind.h" +#include "components/content_settings/core/browser/content_settings_pref.h" +#include "components/content_settings/core/browser/website_settings_registry.h" + +namespace content_settings { + +BravePrefProvider::BravePrefProvider(PrefService* prefs, + bool incognito, + bool store_last_modified) + : PrefProvider(prefs, incognito, store_last_modified) { + brave_pref_change_registrar_.Init(prefs_); + + WebsiteSettingsRegistry* website_settings = + WebsiteSettingsRegistry::GetInstance(); + // Makes BravePrefProvder handle plugin type. + for (const WebsiteSettingsInfo* info : *website_settings) { + if (info->type() == CONTENT_SETTINGS_TYPE_PLUGINS) { + content_settings_prefs_.insert(std::make_pair( + info->type(), + std::make_unique( + info->type(), prefs_, &brave_pref_change_registrar_, + info->pref_name(), + is_incognito_, + base::Bind(&PrefProvider::Notify, base::Unretained(this))))); + return; + } + } +} + +void BravePrefProvider::ShutdownOnUIThread() { + brave_pref_change_registrar_.RemoveAll(); + PrefProvider::ShutdownOnUIThread(); +} + +bool BravePrefProvider::SetWebsiteSetting( + const ContentSettingsPattern& primary_pattern, + const ContentSettingsPattern& secondary_pattern, + ContentSettingsType content_type, + const ResourceIdentifier& resource_identifier, + base::Value* in_value) { + // Flash type shouldn't be handled here. Its id is empty. + // It's type is plugin and id is empty string. + // One excpetion is default setting. It can be persisted. + if (content_type == CONTENT_SETTINGS_TYPE_PLUGINS && + resource_identifier.empty()) { + DCHECK(primary_pattern == ContentSettingsPattern::Wildcard() && + secondary_pattern == ContentSettingsPattern::Wildcard()); + } + + return PrefProvider::SetWebsiteSetting( + primary_pattern, secondary_pattern, + content_type, resource_identifier, in_value); +} + +} // namespace content_settings diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider.h b/components/content_settings/core/browser/brave_content_settings_pref_provider.h new file mode 100644 index 000000000000..94d11787ee2f --- /dev/null +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider.h @@ -0,0 +1,44 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_BRAVE_CONTENT_SETTINGS_PREF_PROVIDER_H_ +#define BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_BRAVE_CONTENT_SETTINGS_PREF_PROVIDER_H_ + +#include "components/content_settings/core/browser/content_settings_pref_provider.h" +#include "components/prefs/pref_change_registrar.h" + +namespace content_settings { + +// With this subclass, shields configuration is persisted across sessions. +// Its content type is |CONTENT_SETTINGS_TYPE_PLUGIN| and its storage option is +// ephemeral because chromium want that flash configuration shouldn't be +// persisted. (Maybe chromium assumes flash is the only one of this type). +// Because of this reasion, shields configuration was also ephemeral. +// However, we want shilelds configuration persisted. To do this, we make +// EphemeralProvider ignore shields type and this class handles. +class BravePrefProvider : public PrefProvider { + public: + BravePrefProvider( + PrefService* prefs, bool incognito, bool store_last_modified); + ~BravePrefProvider() override {} + + private: + // content_settings::PrefProvider overrides: + void ShutdownOnUIThread() override; + bool SetWebsiteSetting( + const ContentSettingsPattern& primary_pattern, + const ContentSettingsPattern& secondary_pattern, + ContentSettingsType content_type, + const ResourceIdentifier& resource_identifier, + base::Value* value) override; + + // PrefProvider::pref_change_registrar_ alreay has plugin type. + PrefChangeRegistrar brave_pref_change_registrar_; + + DISALLOW_COPY_AND_ASSIGN(BravePrefProvider); +}; + +} // namespace content_settings + +#endif // BRAVE_COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_BRAVE_CONTENT_SETTINGS_PREF_PROVIDER_H_ diff --git a/patches/components-content_settings-core-browser-content_settings_pref_provider.h.patch b/patches/components-content_settings-core-browser-content_settings_pref_provider.h.patch new file mode 100644 index 000000000000..c3ca275e9177 --- /dev/null +++ b/patches/components-content_settings-core-browser-content_settings_pref_provider.h.patch @@ -0,0 +1,12 @@ +diff --git a/components/content_settings/core/browser/content_settings_pref_provider.h b/components/content_settings/core/browser/content_settings_pref_provider.h +index 2dcdfbacda452ca8519f40d9db74d202b76dc425..915591404eb06bc04696d61522c087ea1a95c729 100644 +--- a/components/content_settings/core/browser/content_settings_pref_provider.h ++++ b/components/content_settings/core/browser/content_settings_pref_provider.h +@@ -64,6 +64,7 @@ class PrefProvider : public UserModifiableProvider { + void SetClockForTesting(base::Clock* clock); + + private: ++ friend class BravePrefProvider; + friend class DeadlockCheckerObserver; // For testing. + + void Notify(const ContentSettingsPattern& primary_pattern, From b5d769bee0bb30226f63129f9584775bac0a2ec2 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 4 Oct 2018 20:09:37 +0900 Subject: [PATCH 3/4] Add more test to check flash configuration isn't persisted --- .../api/brave_shields_api_browsertest.cc | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/browser/extensions/api/brave_shields_api_browsertest.cc b/browser/extensions/api/brave_shields_api_browsertest.cc index 21a728364ab5..9203f26693f4 100644 --- a/browser/extensions/api/brave_shields_api_browsertest.cc +++ b/browser/extensions/api/brave_shields_api_browsertest.cc @@ -273,16 +273,15 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, PRE_ShieldSettingsPersistTest) { HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> - SetContentSettingCustomScope( - ContentSettingsPattern::Wildcard(), - ContentSettingsPattern::Wildcard(), + SetContentSettingDefaultScope( + kBraveURL, kBraveURL, CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kHTTPUpgradableResources, CONTENT_SETTING_ALLOW); ContentSetting setting = HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> - GetContentSetting(GURL(), GURL(), CONTENT_SETTINGS_TYPE_PLUGINS, + GetContentSetting(kBraveURL, kBraveURL, CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kHTTPUpgradableResources); EXPECT_EQ(setting, CONTENT_SETTING_ALLOW); } @@ -291,9 +290,35 @@ IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, ShieldSettingsPersistTest) { ContentSetting setting = HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> - GetContentSetting(GURL(), GURL(), CONTENT_SETTINGS_TYPE_PLUGINS, + GetContentSetting(kBraveURL, kBraveURL, CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kHTTPUpgradableResources); EXPECT_EQ(setting, CONTENT_SETTING_ALLOW); } +// Checks flash configuration isn't persisted across the sessions. +IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, + PRE_FlashPersistTest) { + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + SetContentSettingDefaultScope( + kBraveURL, kBraveURL, + CONTENT_SETTINGS_TYPE_PLUGINS, + std::string(), + CONTENT_SETTING_ALLOW); + + ContentSetting setting = + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + GetContentSetting(kBraveURL, kBraveURL, CONTENT_SETTINGS_TYPE_PLUGINS, + std::string()); + EXPECT_EQ(setting, CONTENT_SETTING_ALLOW); +} + +IN_PROC_BROWSER_TEST_F(BraveShieldsAPIBrowserTest, + FlashPersistTest) { + ContentSetting setting = + HostContentSettingsMapFactory::GetForProfile(browser()->profile())-> + GetContentSetting(kBraveURL, kBraveURL, CONTENT_SETTINGS_TYPE_PLUGINS, + std::string()); + EXPECT_EQ(setting, CONTENT_SETTING_BLOCK); +} + } // namespace extensions From e652fc61b614adc389d1296f2a9c1a49b0e7d5cd Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Thu, 4 Oct 2018 21:55:27 +0900 Subject: [PATCH 4/4] Add more DCHECKs to BraveEphemeralProvider BraveEphemeralProvider should not handle shield settings. --- .../core/browser/brave_content_settings_ephemeral_provider.cc | 3 +++ .../core/browser/brave_content_settings_pref_provider.cc | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc index 274293be2feb..f4a292817dec 100644 --- a/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc +++ b/components/content_settings/core/browser/brave_content_settings_ephemeral_provider.cc @@ -40,6 +40,9 @@ bool BraveEphemeralProvider::SetWebsiteSetting( return false; } + // Only flash plugin setting can be reached here. + DCHECK(resource_identifier.empty()); + return EphemeralProvider::SetWebsiteSetting( primary_pattern, secondary_pattern, content_type, resource_identifier, in_value); diff --git a/components/content_settings/core/browser/brave_content_settings_pref_provider.cc b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc index 6e59f0f998d1..cbeedec92b9c 100644 --- a/components/content_settings/core/browser/brave_content_settings_pref_provider.cc +++ b/components/content_settings/core/browser/brave_content_settings_pref_provider.cc @@ -44,8 +44,8 @@ bool BravePrefProvider::SetWebsiteSetting( ContentSettingsType content_type, const ResourceIdentifier& resource_identifier, base::Value* in_value) { - // Flash type shouldn't be handled here. Its id is empty. - // It's type is plugin and id is empty string. + // Flash's setting shouldn't be reached here. + // Its content type is plugin and id is empty string. // One excpetion is default setting. It can be persisted. if (content_type == CONTENT_SETTINGS_TYPE_PLUGINS && resource_identifier.empty()) {