Add setting option for disabling tor component #4046

simonhong · 2019-11-21T04:47:11Z

When tor is managed by group policy, this option is disabled. (only possible on Windows now)

Submitter Checklist:

Test Plan:

yarn test brave_browser_tests --filter=BraveLocalStateBrowserTest*
yarn test brave_browser_tests --filter=TorDisabledPolicyBrowserTest*
yarn test brave_browser_tests --filter=TorEnabledPolicyBrowserTest*
yarn test brave_browser_tests --filter=NoTorPolicyBrowserTest*

Manual test steps

Launch browser with clean profile
Load brave://settings/extensions and check Tor is on by default
Turn it off and Tor menu is not visible in the app menu
Turn it on and Tor menu is visible in the app menu
Open tor window
Turn it off and tor window is still alive
Close opened tor window and check Tor utility process is not visible from task manager.

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.

browser/resources/settings/brave_default_extensions_page/brave_default_extensions_page.html

bsclifton

Changes look good and work great! As shared here, some of these blocks of settings are getting crowded and could use some organization

There was a lint problem, I just pushed a fix for. All other CI passed fine (only failure besides lint was on Publish xUnit test result report which is fine)

simonhong · 2019-11-21T21:49:30Z

@bsclifton Thanks for fixing lint error 👍

simonhong · 2019-11-21T23:01:33Z

@rebron Can you check the this options' title and position?
Also if we want to add sub label, let me know.

simonhong · 2019-11-22T23:24:25Z

Friendly ping to owner review @bridiver

bridiver · 2019-11-25T16:49:30Z

browser/ui/webui/settings/brave_default_extensions_handler.h

+  void GetTorDisabled(const base::ListValue* args);
+  void OnTorDisabledChanged();
+#endif
+  void GetEnableTorOption(const base::ListValue* args);


What is this?

This is used to determine whether Disable Tor option should be enabled or not.
If tor is managed by group policy or tor is not built, this setting option should be in disabled state.
Added comment for this method.

If the tor feature is not included we shouldn't be showing any UI for it at all and we can detect that with the buildflag so we don't need a method for it. For group policy it should be IsTorManaged

Yes, I agree with you. UI should not be visible if tor is not built.
To do this, I tried to hide settings UI by using template with dom-if.
However, toggle button can't get proper runtime value change if it is inside template.
I debugged this with @bsclifton but can't find solution at that time.
Maybe we need to dig more about polymer.
So, I left like this because we would not build brave w/o tor.
How about this as a f/u?

I'm trying to find how to hide webui with buildflags.

Moved this into tor buildflags with renamed one.
and tor settings will be hidden by adding enable_tor defines to grit defines.
Much more clear 👍

bridiver · 2019-11-25T16:49:59Z

browser/ui/webui/settings/brave_default_extensions_handler.h


  void GetRestartNeeded(const base::ListValue* args);
  void SetWebTorrentEnabled(const base::ListValue* args);
  void SetHangoutsEnabled(const base::ListValue* args);
  void SetIPFSCompanionEnabled(const base::ListValue* args);
  void SetMediaRouterEnabled(const base::ListValue* args);
  void SetBraveWalletEnabled(const base::ListValue* args);
+#if BUILDFLAG(ENABLE_TOR)
+  void SetTorDisabled(const base::ListValue* args);


please use Enabled, not Disabled. It's easier to understand and more consistent with everything else.

Agreed that negative expression makes difficult to read/understand.
How about this leave as a f/u?
Just using enabled in this class also could increase complexity because related pref name is kTorDisabled and options name is also Disable Tor. Also some related static methods are IsTorDisabled() and already used by many other places.
I think it would be good to clean this up as a separate PR. WDYT?

@bridiver, Changed to Enabled as settings text is changed to enabled meaning.

bridiver · 2019-11-25T16:50:42Z

browser/ui/webui/settings/brave_default_extensions_handler.h

@@ -23,15 +25,23 @@ class BraveDefaultExtensionsHandler : public settings::SettingsPageUIHandler {
 private:
  // SettingsPageUIHandler overrides:
  void RegisterMessages() override;
-  void OnJavascriptAllowed() override;
-  void OnJavascriptDisallowed() override;
+  void OnJavascriptAllowed() override {}


why are you defining these as empty methods? What's the point of overriding them if we're not going to implement them?

It's because they are pure virtual method in base class.

bridiver · 2019-11-25T16:51:25Z

chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

@@ -149,6 +149,8 @@ void BraveAddCommonStrings(content::WebUIDataSource* html_source,
      IDS_SETTINGS_IPFS_COMPANION_ENABLED_DESC},
    {"mediaRouterEnabledDesc",
      IDS_SETTINGS_MEDIA_ROUTER_ENABLED_DESC},
+    {"torDisabledLabel",


same as above disabled -> enabled. It is almost always better to use the "positive" form for readability

rebron · 2019-11-26T02:11:02Z

cc: @TomL for final text. Instead of Disable Tor I'd go withPrivate Window with Tor and then description Tor hides your IP address from the sites you visit.

simonhong · 2019-12-03T02:17:15Z

browser/brave_browser_process_impl.cc

+
+void BraveBrowserProcessImpl::OnTorEnabledChanged() {
+  // Update all browsers' tor command status.
+  for (Browser* browser : *BrowserList::GetInstance()) {


When tor option is changed, all browsers' tor commands are updated.

does the utility process automatically shut down when all tor windows are closed?

In nightly, tor process is gone. but not in stable version.

bridiver · 2019-12-03T18:01:31Z

browser/brave_browser_process_impl.cc

@@ -57,6 +57,10 @@

 #if BUILDFLAG(ENABLE_TOR)
 #include "brave/browser/extensions/brave_tor_client_updater.h"
+#include "brave/browser/ui/brave_browser_command_controller.h"


even though these are only used when tor is enabled, I still think it's a bad idea to put them in this guard because in the future other things may use these same header files and people may not move them out of the ENABLE_TOR block when that happens. I think only tor header files should go in here.

bridiver · 2019-12-03T18:02:37Z

browser/brave_browser_process_impl.h

@@ -85,6 +89,7 @@ class BraveBrowserProcessImpl : public BrowserProcessImpl {
  brave_component_updater::LocalDataFilesService* local_data_files_service();
 #if BUILDFLAG(ENABLE_TOR)
  extensions::BraveTorClientUpdater* tor_client_updater();
+  void OnTorEnabledChanged();


this shouldn't be public

bridiver · 2019-12-03T18:04:49Z

browser/brave_browser_process_impl.h

@@ -130,6 +135,7 @@ class BraveBrowserProcessImpl : public BrowserProcessImpl {
 #endif
 #if BUILDFLAG(ENABLE_TOR)
  std::unique_ptr<extensions::BraveTorClientUpdater> tor_client_updater_;
+  PrefChangeRegistrar local_state_change_registrar_;


there is already pref_change_registrar_ in the superclass

and did some code cleanup.

Tor enabling change should only affect to browser after relaunching. To cache user's change before relaunching, new kTorDisabledAtNextLaunching pref value is introduced.

To check whether tor is enabled or not, tor component registration is used instead of pref value because user can change pref via settings. With this, we don't need to another prefs to check user changed or not.

Browsr commands will be updated after changing tor option. If user turns it off, tor component will be removed at the next launching. If there is already opened tor window, it can be used. But, user can't create new tor window anymore.

simonhong · 2019-12-04T10:51:31Z

CI on macOS has below error.
test report file(s) were found with the pattern 'src/brave_browser_tests.xml' relative to '/Users/jenkins/jenkins/workspace/pr_add_options_for_disabling_tor' for the testing framework 'GoogleTest-1.8'.
But this seems not unrelated with this PR and all test cases are passed.
So merged.

tildelowengrimm · 2019-12-05T00:20:10Z

This should also fix brave/brave-browser#4904.

simonhong added the feature/tor label Nov 21, 2019

simonhong added this to the 1.3.x - Nightly milestone Nov 21, 2019

simonhong requested review from bsclifton, rebron, darkdh and riastradh-brave November 21, 2019 04:47

simonhong requested a review from bridiver as a code owner November 21, 2019 04:47

simonhong self-assigned this Nov 21, 2019

simonhong force-pushed the add_options_for_disabling_tor branch 2 times, most recently from 0431d5a to cbdc72c Compare November 21, 2019 05:48

bsclifton mentioned this pull request Nov 21, 2019

Remote debugging can now be enabled via brave://settings/privacy #4044

Merged

32 tasks

bsclifton reviewed Nov 21, 2019

View reviewed changes

browser/resources/settings/brave_default_extensions_page/brave_default_extensions_page.html Outdated Show resolved Hide resolved

simonhong force-pushed the add_options_for_disabling_tor branch 3 times, most recently from a40dc70 to 576f1d6 Compare November 21, 2019 10:51

simonhong mentioned this pull request Nov 21, 2019

Add tor switch option in settings brave/brave-browser#6808

Closed

bsclifton approved these changes Nov 21, 2019

View reviewed changes

darkdh approved these changes Nov 22, 2019

View reviewed changes

simonhong force-pushed the add_options_for_disabling_tor branch 2 times, most recently from e55d35d to ee87251 Compare November 22, 2019 08:56

bridiver reviewed Nov 25, 2019

View reviewed changes

simonhong force-pushed the add_options_for_disabling_tor branch from ee87251 to e07a3a9 Compare November 26, 2019 00:59

simonhong force-pushed the add_options_for_disabling_tor branch from e07a3a9 to 695ebda Compare November 26, 2019 03:05

simonhong added the CI/skip Do not run CI builds (except noplatform) label Dec 3, 2019

simonhong force-pushed the add_options_for_disabling_tor branch from 227578c to d127206 Compare December 3, 2019 02:16

simonhong commented Dec 3, 2019

View reviewed changes

simonhong force-pushed the add_options_for_disabling_tor branch 2 times, most recently from 9b49d20 to 768f6ec Compare December 3, 2019 03:02

bridiver reviewed Dec 3, 2019

View reviewed changes

simonhong removed the CI/skip Do not run CI builds (except noplatform) label Dec 3, 2019

simonhong force-pushed the add_options_for_disabling_tor branch 2 times, most recently from 6606dae to 93a032d Compare December 3, 2019 23:04

bridiver approved these changes Dec 4, 2019

View reviewed changes

simonhong added CI/skip-linux labels Dec 4, 2019

simonhong added 6 commits December 4, 2019 11:18

Add setting option for disabling tor component

77b1453

and did some code cleanup.

Alphabetize the extension setting options

988d9f2

Don't show tor toggle option if tor feature is not built

84ef437

Don't change IsTorDisabled() during the runtime

7fafcb9

Tor enabling change should only affect to browser after relaunching. To cache user's change before relaunching, new kTorDisabledAtNextLaunching pref value is introduced.

Don't use pref for IsTorDisabled()

744f854

To check whether tor is enabled or not, tor component registration is used instead of pref value because user can change pref via settings. With this, we don't need to another prefs to check user changed or not.

Don't need to restart after changing tor option

3e01e1f

Browsr commands will be updated after changing tor option. If user turns it off, tor component will be removed at the next launching. If there is already opened tor window, it can be used. But, user can't create new tor window anymore.

simonhong force-pushed the add_options_for_disabling_tor branch from 93a032d to 3e01e1f Compare December 4, 2019 02:25

simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Dec 4, 2019

simonhong merged commit b1d0576 into master Dec 4, 2019

simonhong deleted the add_options_for_disabling_tor branch December 4, 2019 10:52

simonhong modified the milestones: 1.3.x - Dev, 1.4.x - Nightly Dec 4, 2019

tildelowengrimm mentioned this pull request Dec 5, 2019

Add a way to disable Tor via admin policy brave/brave-browser#454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting option for disabling tor component #4046

Add setting option for disabling tor component #4046

simonhong commented Nov 21, 2019 •

edited

Loading

bsclifton left a comment

simonhong commented Nov 21, 2019

simonhong commented Nov 21, 2019

simonhong commented Nov 22, 2019

bridiver Nov 25, 2019

simonhong Nov 26, 2019 •

edited

Loading

bridiver Nov 28, 2019 •

edited

Loading

simonhong Nov 28, 2019

simonhong Nov 28, 2019

simonhong Nov 28, 2019

bridiver Nov 25, 2019

simonhong Nov 26, 2019

simonhong Nov 26, 2019 •

edited

Loading

bridiver Nov 25, 2019

simonhong Nov 26, 2019

bridiver Nov 25, 2019 •

edited

Loading

simonhong Nov 26, 2019

rebron commented Nov 26, 2019

simonhong Dec 3, 2019

bridiver Dec 3, 2019

simonhong Dec 3, 2019

bridiver Dec 3, 2019

simonhong Dec 3, 2019 •

edited

Loading

bridiver Dec 3, 2019

simonhong Dec 3, 2019

bridiver Dec 3, 2019

simonhong Dec 3, 2019

simonhong commented Dec 4, 2019

tildelowengrimm commented Dec 5, 2019

Add setting option for disabling tor component #4046

Add setting option for disabling tor component #4046

Conversation

simonhong commented Nov 21, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

bsclifton left a comment

Choose a reason for hiding this comment

simonhong commented Nov 21, 2019

simonhong commented Nov 21, 2019

simonhong commented Nov 22, 2019

Choose a reason for hiding this comment

simonhong Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

bridiver Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebron commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Dec 4, 2019

tildelowengrimm commented Dec 5, 2019

simonhong commented Nov 21, 2019 •

edited

Loading

simonhong Nov 26, 2019 •

edited

Loading

bridiver Nov 28, 2019 •

edited

Loading

simonhong Nov 26, 2019 •

edited

Loading

bridiver Nov 25, 2019 •

edited

Loading

simonhong Dec 3, 2019 •

edited

Loading