From f5fd1a5b830f3a772aec90dc7c222e425b26a7a3 Mon Sep 17 00:00:00 2001 From: Jocelyn Liu Date: Tue, 1 Dec 2020 12:14:45 -0800 Subject: [PATCH 1/2] Check if IPFS resolution is enabled before redirect URL requests. This commit makes per_profile as True for IPFS policy, so we could move IPFS checks to component utils to be used in the IPFS network delegate helper. --- browser/brave_local_state_prefs.cc | 8 --- browser/extensions/api/ipfs_api.cc | 5 +- .../extensions/brave_extension_management.cc | 8 +-- .../extensions/brave_extension_management.h | 2 +- browser/ipfs/content_browser_client_helper.cc | 3 +- browser/ipfs/ipfs_policy_browsertest.cc | 13 +++- browser/ipfs/ipfs_service_factory.cc | 40 +---------- browser/ipfs/ipfs_service_factory.h | 6 -- .../ipfs_redirect_network_delegate_helper.cc | 3 + ...ect_network_delegate_helper_browsertest.cc | 71 +++++++++++++++++++ ...direct_network_delegate_helper_unittest.cc | 4 ++ browser/net/url_context.cc | 3 + browser/net/url_context.h | 1 + .../webui/brave_web_ui_controller_factory.cc | 4 +- .../ipfs/ipfs_navigation_throttle_unittest.cc | 5 +- components/ipfs/ipfs_service.cc | 8 +-- components/ipfs/ipfs_service.h | 1 - components/ipfs/ipfs_utils.cc | 34 +++++++++ components/ipfs/ipfs_utils.h | 10 +++ script/policy_source_helper.py | 2 +- test/BUILD.gn | 1 + 21 files changed, 155 insertions(+), 77 deletions(-) create mode 100644 browser/net/ipfs_redirect_network_delegate_helper_browsertest.cc diff --git a/browser/brave_local_state_prefs.cc b/browser/brave_local_state_prefs.cc index 25f59e6f0b18..16404bfbe1e9 100644 --- a/browser/brave_local_state_prefs.cc +++ b/browser/brave_local_state_prefs.cc @@ -13,7 +13,6 @@ #include "brave/components/brave_referrals/buildflags/buildflags.h" #include "brave/components/brave_shields/browser/ad_block_service.h" #include "brave/components/brave_shields/browser/brave_shields_p3a.h" -#include "brave/components/ipfs/buildflags/buildflags.h" #include "brave/components/ntp_background_images/browser/ntp_background_images_service.h" #include "brave/components/ntp_background_images/browser/view_counter_service.h" #include "brave/components/p3a/brave_p3a_service.h" @@ -32,10 +31,6 @@ #include "brave/components/tor/tor_profile_service.h" #endif -#if BUILDFLAG(IPFS_ENABLED) -#include "brave/components/ipfs/ipfs_service.h" -#endif - #include "brave/browser/ui/webui/new_tab_page/brave_new_tab_message_handler.h" #if !defined(OS_ANDROID) @@ -69,9 +64,6 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { registry->SetDefaultPrefValue( metrics::prefs::kMetricsReportingEnabled, base::Value(GetDefaultPrefValueForMetricsReporting())); -#if BUILDFLAG(IPFS_ENABLED) - ipfs::IpfsService::RegisterLocalStatePrefs(registry); -#endif #if BUILDFLAG(BRAVE_P3A_ENABLED) brave::BraveP3AService::RegisterPrefs(registry, diff --git a/browser/extensions/api/ipfs_api.cc b/browser/extensions/api/ipfs_api.cc index 060a79e11ccf..bc702a3e2020 100644 --- a/browser/extensions/api/ipfs_api.cc +++ b/browser/extensions/api/ipfs_api.cc @@ -8,13 +8,12 @@ #include #include -#include "base/feature_list.h" #include "base/json/json_writer.h" #include "base/values.h" #include "brave/browser/ipfs/ipfs_service_factory.h" -#include "brave/components/ipfs/features.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_service.h" +#include "brave/components/ipfs/ipfs_utils.h" #include "brave/grit/brave_generated_resources.h" #include "ui/base/l10n/l10n_util.h" @@ -25,7 +24,7 @@ ipfs::IpfsService* GetIpfsService(content::BrowserContext* context) { } bool IsIpfsEnabled(content::BrowserContext* context) { - return ipfs::IpfsServiceFactory::IsIpfsEnabled(context); + return ipfs::IsIpfsEnabled(context); } base::Value MakeSelectValue(const base::string16& name, diff --git a/browser/extensions/brave_extension_management.cc b/browser/extensions/brave_extension_management.cc index ed20c5d8bd81..658b3b342200 100644 --- a/browser/extensions/brave_extension_management.cc +++ b/browser/extensions/brave_extension_management.cc @@ -27,8 +27,8 @@ #endif #if BUILDFLAG(IPFS_ENABLED) -#include "brave/browser/ipfs/ipfs_service_factory.h" #include "brave/components/ipfs/brave_ipfs_client_updater.h" +#include "brave/components/ipfs/ipfs_utils.h" #endif namespace extensions { @@ -45,7 +45,7 @@ BraveExtensionManagement::BraveExtensionManagement(Profile* profile) tor::prefs::kTorDisabled, base::BindRepeating(&BraveExtensionManagement::OnTorDisabledChanged, base::Unretained(this))); - Cleanup(); + Cleanup(profile); } BraveExtensionManagement::~BraveExtensionManagement() { @@ -74,14 +74,14 @@ void BraveExtensionManagement::OnTorDisabledChanged() { #endif } -void BraveExtensionManagement::Cleanup() { +void BraveExtensionManagement::Cleanup(content::BrowserContext* context) { // BrowserPolicyConnector enforce policy earlier than this constructor so we // have to manully cleanup tor executable when tor is disabled by gpo OnTorDisabledChanged(); #if BUILDFLAG(IPFS_ENABLED) // Remove ipfs executable if it is disabled by GPO. - if (ipfs::IpfsServiceFactory::IsIpfsDisabledByPolicy()) + if (ipfs::IsIpfsDisabledByPolicy(context)) g_brave_browser_process->ipfs_client_updater()->Cleanup(); #endif } diff --git a/browser/extensions/brave_extension_management.h b/browser/extensions/brave_extension_management.h index adc26232469b..f5e697166659 100644 --- a/browser/extensions/brave_extension_management.h +++ b/browser/extensions/brave_extension_management.h @@ -31,7 +31,7 @@ class BraveExtensionManagement : public ExtensionManagement, UnloadedExtensionReason reason) override; void OnTorDisabledChanged(); - void Cleanup(); + void Cleanup(content::BrowserContext* browser_context); PrefChangeRegistrar local_state_pref_change_registrar_; diff --git a/browser/ipfs/content_browser_client_helper.cc b/browser/ipfs/content_browser_client_helper.cc index d7989b46e58c..3f71df4ffcce 100644 --- a/browser/ipfs/content_browser_client_helper.cc +++ b/browser/ipfs/content_browser_client_helper.cc @@ -15,6 +15,7 @@ #include "brave/common/url_constants.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" +#include "brave/components/ipfs/ipfs_utils.h" #include "brave/components/ipfs/pref_names.h" #include "brave/components/ipfs/translate_ipfs_uri.h" #include "chrome/browser/external_protocol/external_protocol_handler.h" @@ -54,7 +55,7 @@ bool HandleIPFSURLRewrite( return true; } - if (!IpfsServiceFactory::IsIpfsResolveMethodDisabled(browser_context) && + if (!IsIpfsResolveMethodDisabled(browser_context) && // When it's not the local gateway we don't want to show a ipfs:// URL. // We instead will translate the URL later. IsIPFSLocalGateway(browser_context) && diff --git a/browser/ipfs/ipfs_policy_browsertest.cc b/browser/ipfs/ipfs_policy_browsertest.cc index 67f8b524d4e2..f89eec0a3e42 100644 --- a/browser/ipfs/ipfs_policy_browsertest.cc +++ b/browser/ipfs/ipfs_policy_browsertest.cc @@ -11,6 +11,7 @@ #include "brave/components/ipfs/features.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_navigation_throttle.h" +#include "brave/components/ipfs/ipfs_utils.h" #include "brave/components/ipfs/pref_names.h" #include "chrome/browser/infobars/infobar_service.h" #include "chrome/browser/ui/browser.h" @@ -61,7 +62,7 @@ class IpfsPolicyTest : public InProcessBrowserTest { BrowserPolicyConnector::SetPolicyProviderForTesting(&provider_); PolicyMap policies; policies.Set(key::kIPFSEnabled, POLICY_LEVEL_MANDATORY, - POLICY_SCOPE_MACHINE, POLICY_SOURCE_PLATFORM, + POLICY_SCOPE_USER, POLICY_SOURCE_PLATFORM, base::Value(enable), nullptr); provider_.UpdateChromePolicy(policies); } @@ -92,11 +93,17 @@ using IpfsEnabledPolicyTest = IpfsPolicyTest; using IpfsDisabledPolicyTest = IpfsPolicyTest; IN_PROC_BROWSER_TEST_F(IpfsEnabledPolicyTest, IsIpfsDisabledByPolicy) { - EXPECT_FALSE(ipfs::IpfsServiceFactory::IsIpfsDisabledByPolicy()); + EXPECT_FALSE(ipfs::IsIpfsDisabledByPolicy(browser_context())); + auto* prefs = user_prefs::UserPrefs::Get(browser_context()); + EXPECT_TRUE(prefs->FindPreference(kIPFSEnabled)); + EXPECT_TRUE(prefs->GetBoolean(kIPFSEnabled)); } IN_PROC_BROWSER_TEST_F(IpfsDisabledPolicyTest, IsIpfsDisabledByPolicy) { - EXPECT_TRUE(ipfs::IpfsServiceFactory::IsIpfsDisabledByPolicy()); + EXPECT_TRUE(ipfs::IsIpfsDisabledByPolicy(browser_context())); + auto* prefs = user_prefs::UserPrefs::Get(browser_context()); + EXPECT_TRUE(prefs->FindPreference(kIPFSEnabled)); + EXPECT_FALSE(prefs->GetBoolean(kIPFSEnabled)); } IN_PROC_BROWSER_TEST_F(IpfsEnabledPolicyTest, GetService) { diff --git a/browser/ipfs/ipfs_service_factory.cc b/browser/ipfs/ipfs_service_factory.cc index b78ac70294ef..62eebc3f88d4 100644 --- a/browser/ipfs/ipfs_service_factory.cc +++ b/browser/ipfs/ipfs_service_factory.cc @@ -12,6 +12,7 @@ #include "brave/components/ipfs/features.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_service.h" +#include "brave/components/ipfs/ipfs_utils.h" #include "brave/components/ipfs/pref_names.h" #include "chrome/browser/profiles/incognito_helpers.h" #include "chrome/common/channel_info.h" @@ -26,43 +27,6 @@ namespace ipfs { -// static -bool IpfsServiceFactory::IsIpfsDisabledByPolicy() { - if (!g_brave_browser_process) - return false; - - PrefService* local_state = g_brave_browser_process->local_state(); - - // Because we currently do not provide a settings switch for IPFSEnabled - // preference to be overwritten by users, the policy is configured that only - // a mandatory value can be set by admins. - return local_state->IsManagedPreference(kIPFSEnabled) && - !local_state->GetBoolean(kIPFSEnabled); -} - -// static -bool IpfsServiceFactory::IsIpfsEnabled(content::BrowserContext* context) { - if (!brave::IsRegularProfile(context) || IsIpfsDisabledByPolicy() || - !base::FeatureList::IsEnabled(ipfs::features::kIpfsFeature)) - return false; - - return true; -} - -// static -bool IpfsServiceFactory::IsIpfsResolveMethodDisabled( - content::BrowserContext* context) { - // Ignore the actual pref value if IPFS feature is disabled. - if (!IsIpfsEnabled(context)) { - return true; - } - - PrefService* user_prefs = user_prefs::UserPrefs::Get(context); - return user_prefs->FindPreference(kIPFSResolveMethod) && - user_prefs->GetInteger(kIPFSResolveMethod) == - static_cast(ipfs::IPFSResolveMethodTypes::IPFS_DISABLED); -} - // static IpfsServiceFactory* IpfsServiceFactory::GetInstance() { return base::Singleton::get(); @@ -71,7 +35,7 @@ IpfsServiceFactory* IpfsServiceFactory::GetInstance() { // static IpfsService* IpfsServiceFactory::GetForContext( content::BrowserContext* context) { - if (!IsIpfsEnabled(context)) + if (!brave::IsRegularProfile(context) || !IsIpfsEnabled(context)) return nullptr; return static_cast( diff --git a/browser/ipfs/ipfs_service_factory.h b/browser/ipfs/ipfs_service_factory.h index 8c35b5db0291..0b8da6101a2f 100644 --- a/browser/ipfs/ipfs_service_factory.h +++ b/browser/ipfs/ipfs_service_factory.h @@ -18,12 +18,6 @@ class IpfsServiceFactory : public BrowserContextKeyedServiceFactory { static IpfsService* GetForContext(content::BrowserContext* context); static IpfsServiceFactory* GetInstance(); - // IsIpfsEnabled returns false if IPFS feature is unsupported for the given - // context, disabled by IPFSEnabled policy, or the feature flag. - static bool IsIpfsEnabled(content::BrowserContext* context); - static bool IsIpfsResolveMethodDisabled(content::BrowserContext* context); - static bool IsIpfsDisabledByPolicy(); - private: friend struct base::DefaultSingletonTraits; diff --git a/browser/net/ipfs_redirect_network_delegate_helper.cc b/browser/net/ipfs_redirect_network_delegate_helper.cc index ae591c8f0b2d..12b34e5759d0 100644 --- a/browser/net/ipfs_redirect_network_delegate_helper.cc +++ b/browser/net/ipfs_redirect_network_delegate_helper.cc @@ -15,6 +15,9 @@ namespace ipfs { int OnBeforeURLRequest_IPFSRedirectWork( const brave::ResponseCallback& next_callback, std::shared_ptr ctx) { + if (!ctx->resolve_ipfs_enabled) + return net::OK; + GURL new_url; if (ipfs::TranslateIPFSURI(ctx->request_url, &new_url, ctx->ipfs_gateway_url)) { diff --git a/browser/net/ipfs_redirect_network_delegate_helper_browsertest.cc b/browser/net/ipfs_redirect_network_delegate_helper_browsertest.cc new file mode 100644 index 000000000000..a8859db9c484 --- /dev/null +++ b/browser/net/ipfs_redirect_network_delegate_helper_browsertest.cc @@ -0,0 +1,71 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * 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 "base/test/scoped_feature_list.h" +#include "brave/browser/ipfs/ipfs_service_factory.h" +#include "brave/components/ipfs/features.h" +#include "brave/components/ipfs/ipfs_constants.h" +#include "brave/components/ipfs/pref_names.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "components/prefs/pref_service.h" +#include "content/public/browser/web_contents.h" +#include "content/public/test/browser_test.h" + +namespace ipfs { + +class IpfsRedirectNetworkDelegateHelperBrowserTest + : public InProcessBrowserTest { + public: + IpfsRedirectNetworkDelegateHelperBrowserTest() { + feature_list_.InitAndEnableFeature(ipfs::features::kIpfsFeature); + } + + ~IpfsRedirectNetworkDelegateHelperBrowserTest() override = default; + + void SetUpOnMainThread() override { + ipfs_url_ = GURL("ipfs://QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR"); + gateway_url_ = GURL( + "https://dweb.link/ipfs/" + "QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR"); + } + + content::WebContents* web_contents() const { + return browser()->tab_strip_model()->GetActiveWebContents(); + } + + PrefService* GetPrefs() const { return browser()->profile()->GetPrefs(); } + const GURL& ipfs_url() { return ipfs_url_; } + const GURL& gateway_url() { return gateway_url_; } + + private: + base::test::ScopedFeatureList feature_list_; + GURL ipfs_url_; + GURL gateway_url_; +}; + +IN_PROC_BROWSER_TEST_F(IpfsRedirectNetworkDelegateHelperBrowserTest, + IPFSResolveMethodDisabledNoRedirect) { + GetPrefs()->SetInteger( + kIPFSResolveMethod, + static_cast(IPFSResolveMethodTypes::IPFS_DISABLED)); + + EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), ipfs_url())); + EXPECT_EQ(web_contents()->GetURL(), ipfs_url()); +} + +IN_PROC_BROWSER_TEST_F(IpfsRedirectNetworkDelegateHelperBrowserTest, + IPFSResolveMethodGatewayRedirect) { + GetPrefs()->SetInteger( + kIPFSResolveMethod, + static_cast(IPFSResolveMethodTypes::IPFS_GATEWAY)); + + EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), ipfs_url())); + EXPECT_EQ(web_contents()->GetURL(), gateway_url()); +} + +} // namespace ipfs diff --git a/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc b/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc index 44452962f13f..dc1ef09539d1 100644 --- a/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc +++ b/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc @@ -44,6 +44,7 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIHTTPScheme) { TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSSchemeLocal) { GURL url("ipfs://QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); auto brave_request_info = std::make_shared(url); + brave_request_info->resolve_ipfs_enabled = true; brave_request_info->ipfs_gateway_url = GetLocalGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL( initiator_cid, "", @@ -59,6 +60,7 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSSchemeLocal) { TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSScheme) { GURL url("ipfs://QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); auto brave_request_info = std::make_shared(url); + brave_request_info->resolve_ipfs_enabled = true; brave_request_info->ipfs_gateway_url = GetPublicGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL(initiator_cid, "", ipfs::GetDefaultIPFSGateway()); @@ -73,6 +75,7 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSScheme) { TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSSchemeLocal) { GURL url("ipns://QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); auto brave_request_info = std::make_shared(url); + brave_request_info->resolve_ipfs_enabled = true; brave_request_info->ipfs_gateway_url = GetLocalGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL( initiator_cid, "", @@ -88,6 +91,7 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSSchemeLocal) { TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSScheme) { GURL url("ipns://QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); auto brave_request_info = std::make_shared(url); + brave_request_info->resolve_ipfs_enabled = true; brave_request_info->ipfs_gateway_url = GetPublicGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL(initiator_cid, "", ipfs::GetDefaultIPFSGateway()); diff --git a/browser/net/url_context.cc b/browser/net/url_context.cc index 627541a0c58d..f0ff9fc3b629 100644 --- a/browser/net/url_context.cc +++ b/browser/net/url_context.cc @@ -22,6 +22,7 @@ #include "brave/components/ipfs/pref_names.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" +#include "brave/components/ipfs/ipfs_utils.h" #include "chrome/common/channel_info.h" #include "components/prefs/testing_pref_service.h" #include "components/user_prefs/user_prefs.h" @@ -138,6 +139,8 @@ std::shared_ptr BraveRequestInfo::MakeCTX( ctx->upload_data = GetUploadData(request); #if BUILDFLAG(IPFS_ENABLED) + ctx->resolve_ipfs_enabled = + !ipfs::IsIpfsResolveMethodDisabled(browser_context); auto* prefs = user_prefs::UserPrefs::Get(browser_context); bool local = static_cast( prefs->GetInteger(kIPFSResolveMethod)) == diff --git a/browser/net/url_context.h b/browser/net/url_context.h index 9fe17226d1d3..2daeca2df48a 100644 --- a/browser/net/url_context.h +++ b/browser/net/url_context.h @@ -78,6 +78,7 @@ struct BraveRequestInfo { bool allow_http_upgradable_resource = false; bool allow_referrers = false; bool is_webtorrent_disabled = false; + bool resolve_ipfs_enabled = false; int render_process_id = 0; int render_frame_id = 0; int frame_tree_node_id = 0; diff --git a/browser/ui/webui/brave_web_ui_controller_factory.cc b/browser/ui/webui/brave_web_ui_controller_factory.cc index 96a5fff95bb3..6a6105d6ffa3 100644 --- a/browser/ui/webui/brave_web_ui_controller_factory.cc +++ b/browser/ui/webui/brave_web_ui_controller_factory.cc @@ -41,8 +41,8 @@ #endif #if BUILDFLAG(IPFS_ENABLED) -#include "brave/browser/ipfs/ipfs_service_factory.h" #include "brave/browser/ui/webui/ipfs_ui.h" +#include "brave/components/ipfs/ipfs_utils.h" #endif using content::WebUI; @@ -70,7 +70,7 @@ WebUIController* NewWebUI(WebUI* web_ui, const GURL& url) { return new WebcompatReporterUI(web_ui, url.host()); #if BUILDFLAG(IPFS_ENABLED) } else if (host == kIPFSHost && - ipfs::IpfsServiceFactory::IsIpfsEnabled( + ipfs::IsIpfsEnabled( web_ui->GetWebContents()->GetBrowserContext())) { return new IPFSUI(web_ui, url.host()); #endif // BUILDFLAG(IPFS_ENABLED) diff --git a/components/ipfs/ipfs_navigation_throttle_unittest.cc b/components/ipfs/ipfs_navigation_throttle_unittest.cc index bf4960a13ddb..d23773644d43 100644 --- a/components/ipfs/ipfs_navigation_throttle_unittest.cc +++ b/components/ipfs/ipfs_navigation_throttle_unittest.cc @@ -131,11 +131,12 @@ TEST_F(IpfsNavigationThrottleUnitTest, DeferUntilIpfsProcessLaunched) { "/ip4/101.101.101.101/tcp/4001/p2p/" "QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"}; - ipfs_service(profile())->SetSkipGetConnectedPeersCallbackForTest(true); + auto* service = ipfs_service(profile()); + ASSERT_TRUE(service != nullptr); + service->SetSkipGetConnectedPeersCallbackForTest(true); content::MockNavigationHandle test_handle(web_contents()); test_handle.set_url(GetIPFSURL()); - auto* service = ipfs_service(profile()); auto throttle = IpfsNavigationThrottle::MaybeCreateThrottleFor( &test_handle, service, locale()); ASSERT_TRUE(throttle != nullptr); diff --git a/components/ipfs/ipfs_service.cc b/components/ipfs/ipfs_service.cc index e9d94e636672..356763f0e2bd 100644 --- a/components/ipfs/ipfs_service.cc +++ b/components/ipfs/ipfs_service.cc @@ -8,14 +8,12 @@ #include #include "base/command_line.h" -#include "base/feature_list.h" #include "base/files/file_util.h" #include "base/json/json_reader.h" #include "base/strings/strcat.h" #include "base/strings/string_util.h" #include "base/task/post_task.h" #include "base/task_runner_util.h" -#include "brave/components/ipfs/features.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" #include "brave/components/ipfs/ipfs_json_parser.h" @@ -111,6 +109,7 @@ IpfsService::~IpfsService() = default; // static void IpfsService::RegisterPrefs(PrefRegistrySimple* registry) { + registry->RegisterBooleanPref(kIPFSEnabled, true); registry->RegisterIntegerPref( kIPFSResolveMethod, static_cast(ipfs::IPFSResolveMethodTypes::IPFS_ASK)); @@ -119,11 +118,6 @@ void IpfsService::RegisterPrefs(PrefRegistrySimple* registry) { registry->RegisterIntegerPref(kIPFSInfobarCount, 0); } -// static -void IpfsService::RegisterLocalStatePrefs(PrefRegistrySimple* registry) { - registry->RegisterBooleanPref(kIPFSEnabled, true); -} - base::FilePath IpfsService::GetIpfsExecutablePath() { // ipfs_client_updater is not available in unit tests. return ipfs_client_updater_ ? ipfs_client_updater_->GetExecutablePath() diff --git a/components/ipfs/ipfs_service.h b/components/ipfs/ipfs_service.h index 943d8176df3f..e4c4f76eac84 100644 --- a/components/ipfs/ipfs_service.h +++ b/components/ipfs/ipfs_service.h @@ -67,7 +67,6 @@ class IpfsService : public KeyedService, bool IsDaemonLaunched() const; static void RegisterPrefs(PrefRegistrySimple* registry); - static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); bool IsIPFSExecutableAvailable() const; void RegisterIpfsClientUpdater(); IPFSResolveMethodTypes GetIPFSResolveMethodType() const; diff --git a/components/ipfs/ipfs_utils.cc b/components/ipfs/ipfs_utils.cc index 0c9aceb35fd7..e023ec9adf7e 100644 --- a/components/ipfs/ipfs_utils.cc +++ b/components/ipfs/ipfs_utils.cc @@ -7,16 +7,50 @@ #include +#include "base/feature_list.h" #include "base/strings/stringprintf.h" +#include "brave/components/ipfs/features.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" +#include "brave/components/ipfs/pref_names.h" #include "brave/components/ipfs/translate_ipfs_uri.h" +#include "components/prefs/pref_service.h" +#include "components/user_prefs/user_prefs.h" +#include "content/public/browser/browser_context.h" #include "extensions/common/url_pattern.h" #include "net/base/url_util.h" #include "url/gurl.h" namespace ipfs { +bool IsIpfsDisabledByPolicy(content::BrowserContext* context) { + PrefService* prefs = user_prefs::UserPrefs::Get(context); + return prefs->FindPreference(kIPFSEnabled) && + prefs->IsManagedPreference(kIPFSEnabled) && + !prefs->GetBoolean(kIPFSEnabled); +} + +bool IsIpfsEnabled(content::BrowserContext* context) { + if (context->IsOffTheRecord() || IsIpfsDisabledByPolicy(context) || + !base::FeatureList::IsEnabled(ipfs::features::kIpfsFeature)) { + return false; + } + + return true; +} + +bool IsIpfsResolveMethodDisabled(content::BrowserContext* context) { + // Ignore the actual pref value if IPFS feature is disabled. + if (!IsIpfsEnabled(context)) { + return true; + } + + PrefService* prefs = user_prefs::UserPrefs::Get(context); + return prefs->FindPreference(kIPFSResolveMethod) && + prefs->GetInteger(kIPFSResolveMethod) == + static_cast(ipfs::IPFSResolveMethodTypes::IPFS_DISABLED); +} + bool HasIPFSPath(const GURL& gurl) { static std::vector url_patterns( {URLPattern(URLPattern::SCHEME_ALL, "*://*/ipfs/*"), diff --git a/components/ipfs/ipfs_utils.h b/components/ipfs/ipfs_utils.h index cb6489dfd39c..4a2a787c3a9a 100644 --- a/components/ipfs/ipfs_utils.h +++ b/components/ipfs/ipfs_utils.h @@ -8,10 +8,20 @@ #include +namespace content { +class BrowserContext; +} // namespace content + class GURL; namespace ipfs { +// IsIpfsEnabled returns false if IPFS feature is unsupported for the given +// context, disabled by IPFSEnabled policy, or the feature flag. +bool IsIpfsEnabled(content::BrowserContext* context); +bool IsIpfsResolveMethodDisabled(content::BrowserContext* context); +bool IsIpfsDisabledByPolicy(content::BrowserContext* context); + bool HasIPFSPath(const GURL& url); bool IsDefaultGatewayURL(const GURL& url); bool IsLocalGatewayURL(const GURL& url); diff --git a/script/policy_source_helper.py b/script/policy_source_helper.py index 47c95f569806..41caf84c0a24 100644 --- a/script/policy_source_helper.py +++ b/script/policy_source_helper.py @@ -31,7 +31,7 @@ def AddBravePolicies(template_file_contents): 'supported_on': ['chrome.*:87-'], 'features': { 'dynamic_refresh': False, - 'per_profile': False, + 'per_profile': True, 'can_be_recommended': False, 'can_be_mandatory': True }, diff --git a/test/BUILD.gn b/test/BUILD.gn index 66cef73b382c..bb29cb902fc8 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -768,6 +768,7 @@ if (!is_android) { "//brave/browser/extensions/api/ipfs_apitest.cc", "//brave/browser/ipfs/ipfs_policy_browsertest.cc", "//brave/browser/ipfs/ipfs_tab_helper_browsertest.cc", + "//brave/browser/net/ipfs_redirect_network_delegate_helper_browsertest.cc", ] deps += [ "//brave/components/ipfs", From d9d3733b4911402da31662b38126720341130014 Mon Sep 17 00:00:00 2001 From: Jocelyn Liu Date: Thu, 3 Dec 2020 13:51:34 -0800 Subject: [PATCH 2/2] Address review comments - Save browser context in BraveRequestInfo. - Remove GetBrowserContextToUse override in IpfsServiceFactory. - Remove unused headers. --- browser/ipfs/ipfs_service_factory.cc | 12 ---- browser/ipfs/ipfs_service_factory.h | 2 - .../ipfs_redirect_network_delegate_helper.cc | 4 +- ...direct_network_delegate_helper_unittest.cc | 59 +++++++++++++++---- browser/net/url_context.cc | 4 +- browser/net/url_context.h | 2 +- .../ipfs/ipfs_navigation_throttle_unittest.cc | 2 +- test/BUILD.gn | 9 +-- 8 files changed, 60 insertions(+), 34 deletions(-) diff --git a/browser/ipfs/ipfs_service_factory.cc b/browser/ipfs/ipfs_service_factory.cc index 62eebc3f88d4..861985d70350 100644 --- a/browser/ipfs/ipfs_service_factory.cc +++ b/browser/ipfs/ipfs_service_factory.cc @@ -5,21 +5,14 @@ #include "brave/browser/ipfs/ipfs_service_factory.h" -#include "base/feature_list.h" #include "base/path_service.h" #include "brave/browser/brave_browser_process_impl.h" #include "brave/browser/profiles/profile_util.h" -#include "brave/components/ipfs/features.h" -#include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_service.h" #include "brave/components/ipfs/ipfs_utils.h" -#include "brave/components/ipfs/pref_names.h" -#include "chrome/browser/profiles/incognito_helpers.h" #include "chrome/common/channel_info.h" #include "chrome/common/chrome_paths.h" #include "components/keyed_service/content/browser_context_dependency_manager.h" -#include "components/prefs/pref_service.h" -#include "components/user_prefs/user_prefs.h" #include "extensions/browser/extension_registry_factory.h" #include "extensions/browser/extension_system.h" #include "extensions/browser/extension_system_provider.h" @@ -64,9 +57,4 @@ KeyedService* IpfsServiceFactory::BuildServiceInstanceFor( user_data_dir, chrome::GetChannel()); } -content::BrowserContext* IpfsServiceFactory::GetBrowserContextToUse( - content::BrowserContext* context) const { - return chrome::GetBrowserContextRedirectedInIncognito(context); -} - } // namespace ipfs diff --git a/browser/ipfs/ipfs_service_factory.h b/browser/ipfs/ipfs_service_factory.h index 0b8da6101a2f..adeb6e1ae300 100644 --- a/browser/ipfs/ipfs_service_factory.h +++ b/browser/ipfs/ipfs_service_factory.h @@ -27,8 +27,6 @@ class IpfsServiceFactory : public BrowserContextKeyedServiceFactory { // BrowserContextKeyedServiceFactory overrides: KeyedService* BuildServiceInstanceFor( content::BrowserContext* context) const override; - content::BrowserContext* GetBrowserContextToUse( - content::BrowserContext* context) const override; DISALLOW_COPY_AND_ASSIGN(IpfsServiceFactory); }; diff --git a/browser/net/ipfs_redirect_network_delegate_helper.cc b/browser/net/ipfs_redirect_network_delegate_helper.cc index 12b34e5759d0..6b6849cf26d8 100644 --- a/browser/net/ipfs_redirect_network_delegate_helper.cc +++ b/browser/net/ipfs_redirect_network_delegate_helper.cc @@ -15,8 +15,10 @@ namespace ipfs { int OnBeforeURLRequest_IPFSRedirectWork( const brave::ResponseCallback& next_callback, std::shared_ptr ctx) { - if (!ctx->resolve_ipfs_enabled) + if (!ctx->browser_context || + IsIpfsResolveMethodDisabled(ctx->browser_context)) { return net::OK; + } GURL new_url; if (ipfs::TranslateIPFSURI(ctx->request_url, &new_url, diff --git a/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc b/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc index dc1ef09539d1..cd1d92530c9d 100644 --- a/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc +++ b/browser/net/ipfs_redirect_network_delegate_helper_unittest.cc @@ -7,14 +7,24 @@ #include #include +#include #include +#include "base/test/scoped_feature_list.h" #include "brave/browser/net/url_context.h" #include "brave/common/translate_network_constants.h" +#include "brave/components/ipfs/features.h" +#include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" #include "brave/components/ipfs/ipfs_utils.h" +#include "brave/components/ipfs/pref_names.h" #include "chrome/common/channel_info.h" #include "chrome/test/base/chrome_render_view_host_test_harness.h" +#include "components/prefs/pref_registry_simple.h" +#include "components/prefs/testing_pref_service.h" +#include "components/user_prefs/user_prefs.h" +#include "content/public/test/browser_task_environment.h" +#include "content/public/test/test_browser_context.h" #include "net/traffic_annotation/network_traffic_annotation_test_helper.h" #include "net/url_request/url_request_test_util.h" #include "url/gurl.h" @@ -32,19 +42,48 @@ GURL GetPublicGateway() { const char initiator_cid[] = "bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq"; -TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIHTTPScheme) { +} // namespace + +namespace ipfs { + +class IPFSRedirectNetworkDelegateHelperTest : public testing::Test { + public: + IPFSRedirectNetworkDelegateHelperTest() + : browser_context_(new content::TestBrowserContext()) {} + ~IPFSRedirectNetworkDelegateHelperTest() override = default; + + void SetUp() override { + feature_list_.InitAndEnableFeature(ipfs::features::kIpfsFeature); + prefs_.registry()->RegisterIntegerPref( + kIPFSResolveMethod, static_cast(IPFSResolveMethodTypes::IPFS_ASK)); + user_prefs::UserPrefs::Set(browser_context_.get(), &prefs_); + } + + content::TestBrowserContext* browser_context() { + return browser_context_.get(); + } + + private: + content::BrowserTaskEnvironment task_environment_; + std::unique_ptr browser_context_; + TestingPrefServiceSimple prefs_; + base::test::ScopedFeatureList feature_list_; +}; + +TEST_F(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIHTTPScheme) { GURL url("http://a.com/ipfs/QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); auto brave_request_info = std::make_shared(url); + brave_request_info->browser_context = browser_context(); int rc = ipfs::OnBeforeURLRequest_IPFSRedirectWork(brave::ResponseCallback(), brave_request_info); EXPECT_EQ(rc, net::OK); EXPECT_TRUE(brave_request_info->new_url_spec.empty()); } -TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSSchemeLocal) { +TEST_F(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSSchemeLocal) { GURL url("ipfs://QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); auto brave_request_info = std::make_shared(url); - brave_request_info->resolve_ipfs_enabled = true; + brave_request_info->browser_context = browser_context(); brave_request_info->ipfs_gateway_url = GetLocalGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL( initiator_cid, "", @@ -57,10 +96,10 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSSchemeLocal) { "QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); } -TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSScheme) { +TEST_F(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSScheme) { GURL url("ipfs://QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); auto brave_request_info = std::make_shared(url); - brave_request_info->resolve_ipfs_enabled = true; + brave_request_info->browser_context = browser_context(); brave_request_info->ipfs_gateway_url = GetPublicGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL(initiator_cid, "", ipfs::GetDefaultIPFSGateway()); @@ -72,10 +111,10 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPFSScheme) { "https://dweb.link/ipfs/QmfM2r8seH2GiRaC4esTjeraXEachRt8ZsSeGaWTPLyMoG"); } -TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSSchemeLocal) { +TEST_F(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSSchemeLocal) { GURL url("ipns://QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); auto brave_request_info = std::make_shared(url); - brave_request_info->resolve_ipfs_enabled = true; + brave_request_info->browser_context = browser_context(); brave_request_info->ipfs_gateway_url = GetLocalGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL( initiator_cid, "", @@ -88,10 +127,10 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSSchemeLocal) { "QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); } -TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSScheme) { +TEST_F(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSScheme) { GURL url("ipns://QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); auto brave_request_info = std::make_shared(url); - brave_request_info->resolve_ipfs_enabled = true; + brave_request_info->browser_context = browser_context(); brave_request_info->ipfs_gateway_url = GetPublicGateway(); brave_request_info->initiator_url = ipfs::GetIPFSGatewayURL(initiator_cid, "", ipfs::GetDefaultIPFSGateway()); @@ -103,4 +142,4 @@ TEST(IPFSRedirectNetworkDelegateHelperTest, TranslateIPFSURIIPNSScheme) { "https://dweb.link/ipns/QmSrPmbaUKA3ZodhzPWZnpFgcPMFWF4QsxXbkWfEptTBJd"); } -} // namespace +} // namespace ipfs diff --git a/browser/net/url_context.cc b/browser/net/url_context.cc index f0ff9fc3b629..081f757a273f 100644 --- a/browser/net/url_context.cc +++ b/browser/net/url_context.cc @@ -22,7 +22,6 @@ #include "brave/components/ipfs/pref_names.h" #include "brave/components/ipfs/ipfs_constants.h" #include "brave/components/ipfs/ipfs_gateway.h" -#include "brave/components/ipfs/ipfs_utils.h" #include "chrome/common/channel_info.h" #include "components/prefs/testing_pref_service.h" #include "components/user_prefs/user_prefs.h" @@ -138,9 +137,8 @@ std::shared_ptr BraveRequestInfo::MakeCTX( ctx->redirect_source); ctx->upload_data = GetUploadData(request); + ctx->browser_context = browser_context; #if BUILDFLAG(IPFS_ENABLED) - ctx->resolve_ipfs_enabled = - !ipfs::IsIpfsResolveMethodDisabled(browser_context); auto* prefs = user_prefs::UserPrefs::Get(browser_context); bool local = static_cast( prefs->GetInteger(kIPFSResolveMethod)) == diff --git a/browser/net/url_context.h b/browser/net/url_context.h index 2daeca2df48a..57ca80abb617 100644 --- a/browser/net/url_context.h +++ b/browser/net/url_context.h @@ -78,13 +78,13 @@ struct BraveRequestInfo { bool allow_http_upgradable_resource = false; bool allow_referrers = false; bool is_webtorrent_disabled = false; - bool resolve_ipfs_enabled = false; int render_process_id = 0; int render_frame_id = 0; int frame_tree_node_id = 0; uint64_t request_identifier = 0; size_t next_url_request_index = 0; + content::BrowserContext* browser_context = nullptr; net::HttpRequestHeaders* headers = nullptr; // The following two sets are populated by |OnBeforeStartTransactionCallback|. // |set_headers| contains headers which values were added or modified. diff --git a/components/ipfs/ipfs_navigation_throttle_unittest.cc b/components/ipfs/ipfs_navigation_throttle_unittest.cc index d23773644d43..bef764fc3c56 100644 --- a/components/ipfs/ipfs_navigation_throttle_unittest.cc +++ b/components/ipfs/ipfs_navigation_throttle_unittest.cc @@ -132,7 +132,7 @@ TEST_F(IpfsNavigationThrottleUnitTest, DeferUntilIpfsProcessLaunched) { "QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"}; auto* service = ipfs_service(profile()); - ASSERT_TRUE(service != nullptr); + ASSERT_TRUE(service); service->SetSkipGetConnectedPeersCallbackForTest(true); content::MockNavigationHandle test_handle(web_contents()); diff --git a/test/BUILD.gn b/test/BUILD.gn index bb29cb902fc8..0617eb6ab7ff 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -171,6 +171,10 @@ test("brave_unit_tests") { "//components/bookmarks/browser", "//components/bookmarks/common", "//components/content_settings/core/test:test_support", + "//components/country_codes", + "//components/domain_reliability:domain_reliability", + "//components/language/core/browser", + "//components/os_crypt:test_support", "//components/permissions", "//components/prefs", "//components/prefs:test_support", @@ -179,10 +183,7 @@ test("brave_unit_tests") { "//components/signin/public/base", "//components/sync_preferences", "//components/translate/core/browser:test_support", - "//components/country_codes", - "//components/domain_reliability:domain_reliability", - "//components/language/core/browser", - "//components/os_crypt:test_support", + "//components/user_prefs", "//components/variations/service:buildflags", "//components/version_info", "//content/public/common",