Check if IPFS resolution is enabled before redirect URL requests. #7307

yrliou · 2020-12-02T00:16:11Z

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.

Resolves brave/brave-browser#12978

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed).
Requested a security/privacy review as needed.

Reviewer Checklist:

New files have MPL-2.0 license header.
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Set IPFS resolve method to Gateway in settings.
Visit ipfs://QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR in a private window, the scheme should not load.
Visit ipfs://QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR in a guest window, the scheme should not load
Set IPFS resolve method to disabled in settings
Visit ipfs://QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR, the scheme should not load.

iefremov · 2020-12-03T08:43:17Z

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);


ASSERT_TRUE(service)

iefremov · 2020-12-03T09:06:28Z

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;


just a random thought - it seems that after all we should just save browser_context here

That was actually my first thought, I prefer that way so ipfs details could be wrapped inside ipfs's helper and not in the general url_context. I'm changing it to save browser_context directly here.

iefremov · 2020-12-03T09:45:26Z

browser/ipfs/ipfs_service_factory.cc

@@ -71,7 +35,7 @@ IpfsServiceFactory* IpfsServiceFactory::GetInstance() {
 // static
 IpfsService* IpfsServiceFactory::GetForContext(
    content::BrowserContext* context) {
-  if (!IsIpfsEnabled(context))
+  if (!brave::IsRegularProfile(context) || !IsIpfsEnabled(context))


curious, why this is needed. In theory, if you don't override GetBrowserContextToUse, default GetBrowserContextToUse will return nullptr for anything off-the-record and the service will not be instantiated. So removing this check and also removing GetBrowserContextToUse should do the trick

GetBrowserContextToUse override is not needed actually, removed.
brave::IsRegularProfile(context) checks IsGuestSession() also and is currently used by several of our service factories, we don't want to create instance for the regular guest profile (which is created when switching to guest happens, tho only the incognito profile of it will be used for browsing).

I'm wondering whether brave::IsRegularProfile is needed at all, beacuse both Tor and Guest profiles are off-the-record. IMO we should erase it from the codebase in favor of BrowserContext::IsOffTheRecord, wdyt?

This way, it seems like there is no sense in using here

iefremov · 2020-12-03T09:47:45Z

browser/ipfs/ipfs_service_factory.cc

@@ -12,6 +12,7 @@
 #include "brave/components/ipfs/features.h"


pls check for unused headers

iefremov · 2020-12-03T09:50:20Z

browser/net/url_context.cc

@@ -138,6 +139,8 @@ std::shared_ptr<brave::BraveRequestInfo> BraveRequestInfo::MakeCTX(
  ctx->upload_data = GetUploadData(request);

 #if BUILDFLAG(IPFS_ENABLED)
+  ctx->resolve_ipfs_enabled =


mb it should be resolve_ipfs_DISabled , to avoid negation here and in the helper

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.

- Save browser context in BraveRequestInfo. - Remove GetBrowserContextToUse override in IpfsServiceFactory. - Remove unused headers.

yrliou · 2020-12-04T06:45:23Z

iOS test failure is irrelevant, windows fail at known test-install failure, merging.

yrliou self-assigned this Dec 2, 2020

yrliou force-pushed the ipfs_add_redirect_check branch from 8ebd28a to b782b10 Compare December 2, 2020 01:02

yrliou marked this pull request as ready for review December 2, 2020 01:15

yrliou requested a review from iefremov as a code owner December 2, 2020 01:15

yrliou requested a review from bbondy December 2, 2020 01:16

yrliou force-pushed the ipfs_add_redirect_check branch 5 times, most recently from 0652e73 to 18e39dd Compare December 2, 2020 17:32

iefremov reviewed Dec 3, 2020

View reviewed changes

iefremov approved these changes Dec 3, 2020

View reviewed changes

yrliou force-pushed the ipfs_add_redirect_check branch 2 times, most recently from 9101151 to c49aa38 Compare December 3, 2020 21:54

bbondy approved these changes Dec 3, 2020

View reviewed changes

yrliou added 2 commits December 3, 2020 14:21

Check if IPFS resolution is enabled before redirect URL requests.

f5fd1a5

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.

Address review comments

d9d3733

- Save browser context in BraveRequestInfo. - Remove GetBrowserContextToUse override in IpfsServiceFactory. - Remove unused headers.

yrliou force-pushed the ipfs_add_redirect_check branch from c49aa38 to d9d3733 Compare December 3, 2020 22:22

yrliou merged commit 1099b24 into master Dec 4, 2020

yrliou deleted the ipfs_add_redirect_check branch December 4, 2020 06:45

yrliou added this to the 1.20.x - Nightly milestone Dec 4, 2020

brave-builds pushed a commit that referenced this pull request Jan 11, 2021

Uplift of #7307 (squashed) to beta

106f0ec

brave-builds mentioned this pull request Jan 11, 2021

Check if IPFS resolution is enabled before redirect URL requests. (uplift to 1.19.x) #7555

Merged

4 tasks

This was referenced Jan 11, 2021

Should not redirect in IPFS network delegate when resolve method is disabled or IPFS is disabled or in private/guest windows brave/brave-browser#12978

Closed

IPFS resolve type of disabled should disable brave/brave-browser#13472

Closed

yrliou mentioned this pull request Jan 11, 2021

IPFS and IPNS should not resolve in guest and incognito contexts brave/brave-browser#13463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if IPFS resolution is enabled before redirect URL requests. #7307

Check if IPFS resolution is enabled before redirect URL requests. #7307

yrliou commented Dec 2, 2020 •

edited

Loading

iefremov Dec 3, 2020

iefremov Dec 3, 2020

yrliou Dec 3, 2020 •

edited

Loading

iefremov Dec 3, 2020

yrliou Dec 3, 2020

iefremov Dec 4, 2020 •

edited

Loading

iefremov Dec 4, 2020

iefremov Dec 3, 2020

iefremov Dec 3, 2020

yrliou commented Dec 4, 2020

		@@ -12,6 +12,7 @@
		#include "brave/components/ipfs/features.h"

Check if IPFS resolution is enabled before redirect URL requests. #7307

Check if IPFS resolution is enabled before redirect URL requests. #7307

Conversation

yrliou commented Dec 2, 2020 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

iefremov Dec 3, 2020

Choose a reason for hiding this comment

iefremov Dec 3, 2020

Choose a reason for hiding this comment

yrliou Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

iefremov Dec 3, 2020

Choose a reason for hiding this comment

yrliou Dec 3, 2020

Choose a reason for hiding this comment

iefremov Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

iefremov Dec 4, 2020

Choose a reason for hiding this comment

iefremov Dec 3, 2020

Choose a reason for hiding this comment

iefremov Dec 3, 2020

Choose a reason for hiding this comment

yrliou commented Dec 4, 2020

yrliou commented Dec 2, 2020 •

edited

Loading

yrliou Dec 3, 2020 •

edited

Loading

iefremov Dec 4, 2020 •

edited

Loading