Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IPFSEnabled policy to control IPFS feature #6966

Merged
merged 2 commits into from
Oct 30, 2020
Merged

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Oct 27, 2020

Resolves brave/brave-browser#11565

Submitter Checklist:

Test Plan:

On Linux or Windows:

  1. Open brave browser and enable IPFS feature flag via brave://flags.

  2. Go to brave://settings and search IPFS, should see Method to resolve IPFS locations and IPFS public gateway fallback settings available, change Method to resolve IPFS locations to Ask if not.

  3. Visit https://dweb.link/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR, ipfs infobar should be shown to ask for enabling IPFS local node support.

  4. Accept the infobar.

  5. Visit brave://ipfs, page should be shown and daemon should be launched.

  6. Quit the browser

  7. In bash, go to profile folder, for example, ~/.config/BraveSoftware/Brave-Browser-Development on local linux build, /C/Users/yrliou/AppData/Local/BraveSoftware/Brave-Browser-Development/User Data on local windows build.
    Run find . -name *go-ipfs* should have results, for example, ./oecghfpdmkjlhnfpmmjegjacfimiafjp/1.0.2/go-ipfs_v0.6.0_linux-amd64 on linux, ./lnbclahgobmjphilkalbhebakmblnbij/1.0.2/go-ipfs_v0.6.0_windows-amd64 on windows.

  8. [Linux]
    (sudo is needed in this step) Go to /etc/brave/policies/managed (create folders if not exists already), create test_policy.json with content:

{
  "IPFSEnabled": false
}
  1. [Windows]
    Open regedit, for 64 bit, create the following keys:
    BraveSoftware\Brave under the existing key HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Policies
    (for 32 bit, you can edit HKEY_LOCAL_MACHINE\SOFTWARE\Policies)
    Create a DWORD value at this path called IPFSEnabled and set the value as 0

  2. Open brave again

  3. Go to brave://policy, should see:

Screen Shot 2020-10-27 at 9 46 56 PM

  1. Go to brave://settings and search IPFS, Method to resolve IPFS locations and IPFS public gateway fallback settings should not be available

  2. Go to brave://ipfs, should show an error page saying This site can’t be reached.

  3. Visit https://dweb.link/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR, no infobar is shown.

  4. Do step 7 again, this time it should be an empty result. (no daemon binary can be found now)

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@yrliou yrliou added this to the 1.18.x - Nightly milestone Oct 27, 2020
@yrliou yrliou self-assigned this Oct 27, 2020
@yrliou yrliou force-pushed the ipfs_admin_policy branch 7 times, most recently from 725f654 to 4247f1c Compare October 29, 2020 21:45
@yrliou yrliou marked this pull request as ready for review October 29, 2020 21:45
@yrliou yrliou requested a review from a team as a code owner October 29, 2020 21:45
&ipfs::ContentBrowserClientHelper::HandleIPFSURLRewrite,
&ipfs::ContentBrowserClientHelper::HandleIPFSURLReverseRewrite);
}
handler->AddHandlerPair(
Copy link
Member Author

@yrliou yrliou Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

condition checks are done in those static functions in content_browser_client_helper.cc now, remove the condition checks in this file.

// 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) &&
Copy link
Member Author

@yrliou yrliou Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expose a new settings switch cuz there will be too much settings and could cause confusion.
Users are still able to set the resolve method (per-profile setting) as disabled in the settings which will be respected immediately without restarting the browser as before.
This new kIPFSEnabled pref is only for admins to add a mandatory policy to disable the whole IPFS feature for the machine and will remove the binary if already downloaded.

namespace ipfs {

// Allows disabling the Ipfs client updater extension.
constexpr char kDisableIpfsClientUpdaterExtension[] =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command-line switch is removed because we already have a feature flag to control IPFS.

#if BUILDFLAG(ENABLE_TOR)
// Wrap whole array definition in build flags to avoid unused variable build
// error. It can happen if the platform doesn't support any of these features.
#if BUILDFLAG(ENABLE_TOR) || BUILDFLAG(IPFS_ENABLED)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI The only chromium_src change is this file.

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@yrliou
Copy link
Member Author

yrliou commented Oct 30, 2020

CI only had known rewards failures.
mac: RewardsContributionBrowserTest.PanelMonthlyTipAmount
windows: RewardsContributionBrowserTest.RecurringTipForUnverifiedPublisher

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src changes LGTM

@simonhong
Copy link
Member

@yrliou How about filing an issue for group policy directory path?
I think /etc/chromium/policies/managed should be /etc/brave/policies/managed.

@yrliou
Copy link
Member Author

yrliou commented Oct 30, 2020

The latest windows CI build is aborted due to node is removed.
My last rebase is just resolving a trivial merge conflict, merging as windows is already passed before it.

@yrliou yrliou merged commit f2f77a1 into master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin policy for IPFS
4 participants