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

Tor profile rework #3319

Merged
merged 16 commits into from
Oct 3, 2019
Merged

Tor profile rework #3319

merged 16 commits into from
Oct 3, 2019

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Aug 31, 2019

Fix brave/brave-browser#2307

It would be easier to review this PR per commit.
What this PR does:

  • Tor profile is not based on guest profiles anymore, when SwitchToTorProfile is called, we will create a dummy regular Tor profile and use its off-the-record profile to browse.
  • Service factories are hooked to inherit bookmarks(3418ed7), preferences(0979ed1) from its parent profile (the one that is used before switching to Tor profile). Our behavior should be the same with private window for these two.
  • Navigation requests to pages like brave://settings will be redirected back to its parent profile.
    (601b6f1)
  • unit and browser tests

Submitter Checklist:

Test Plan:

  1. Open the browser and go to brave://settings and disable Show bookmarks bar.
  2. Visit https://check.torproject.org and add the page into bookmarks.
  3. Open a tor window, the new tab page bookmark bar should show the one we added in step 2.
  4. Visit https://check.torproject.org in Tor window, it should show we're using Tor network.
  5. Go back to the normal window and go to brave://settings and enable Show bookmarks bar
  6. Go back to the https://check.torproject.org in Tor window, it should now display the bookmark bar with the bookmark we added in step 2.
  7. Add a bookmark again in the normal window, it should show up in the Tor window too.
  8. Delete a bookmark from the Tor window, it should disappear in the normal window too.
  9. Delete a bookmark from the normal window, it should disappear in the Tor window too.
  10. Open brave://settings in Tor window, it should open or go to the existing settings page in the normal window.

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 0.71.x - Nightly milestone Aug 31, 2019
@yrliou yrliou self-assigned this Aug 31, 2019
@yrliou yrliou force-pushed the tor_profile_rework_take2_rebase branch 2 times, most recently from 6d3a6ea to bb3440b Compare August 31, 2019 02:23
@yrliou yrliou modified the milestones: 0.71.x - Dev, 0.72.x - Nightly Sep 4, 2019
@yrliou yrliou force-pushed the tor_profile_rework_take2_rebase branch 6 times, most recently from d30e5cf to 8a2daba Compare September 10, 2019 04:44
@yrliou yrliou marked this pull request as ready for review September 10, 2019 04:45
@yrliou yrliou force-pushed the tor_profile_rework_take2_rebase branch 7 times, most recently from 3d457c2 to 1fdaa0c Compare September 10, 2019 23:40

// Return the parent profile which used to create Tor profile. This method
// returns this if the profile is not Tor profile.
Profile* Profile::GetParentProfile() {
Copy link
Member Author

@yrliou yrliou Sep 10, 2019

Choose a reason for hiding this comment

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

We cannot always use ProfileManager directly to get Tor parent profile by its path because we need to access this during shutdown when ProfileManager is already going away and g_browser_process->profile_manager() returns null in this case, so I end up putting it in Profile class (it is needed by both ProfileImpl and TestingProfile).

@yrliou
Copy link
Member Author

yrliou commented Sep 11, 2019

For the TODO to fix SearchEngineProviderServiceTest.CheckDefaultTorProfileSearchProviderTest breakage, I'm thinking of disabling the search engine pref retention test part in SearchEngineProviderServiceTest.CheckDefaultTorProfileSearchProviderTest because we shouldn't retain the pref in Tor profile but should be in its parent profile.
We could introduce the setting UI later in the normal profile's settings page to address brave/brave-browser#2158.

@yrliou yrliou changed the title WIP: Tor profile rework Tor profile rework Sep 11, 2019
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

overall looks good

@@ -162,7 +162,8 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderServiceTest,
service->SetUserSelectedDefaultSearchProvider(&other_url);
}

// Check changed provider in tor profile is retained across the sessions.
// Check changed provider in tor profile should not be retained across the
// sessions.
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 test was expecting the search engine pref in Tor profile to be retained across the sessions which is not true after this PR. Since we don't have UI exposed to users at the moment (Settings page in Tor profile is an empty page for release builds at the moment.) and we have an open issue to change the search engine in the normal profile (the parent profile) brave/brave-browser#2158, I think it's better to readdress the functionality of changing search engines for Tor profile while fixing brave/brave-browser#2158.

@yrliou yrliou force-pushed the tor_profile_rework_take2_rebase branch from 7063372 to 2861828 Compare October 2, 2019 20:27
@yrliou yrliou force-pushed the tor_profile_rework_take2_rebase branch from 5b584cf to 1db5505 Compare October 2, 2019 20:40
@yrliou
Copy link
Member Author

yrliou commented Oct 3, 2019

Only CI failure is a known intermittent error in linux network-audit, but it was passing in previous tries, should be safe to ignore this one.

17:00:00  undefined:18078
17:00:00  {"phase":2,"source":{"id":84,"type":8}]}
17:00:00                                        ^
17:00:00  
17:00:00  SyntaxError: Unexpected token ] in JSON at position 78216892
17:00:00      at JSON.parse (<anonymous>)
17:00:00      at start (/home/ubuntu/jenkins/workspace/_tor_profile_rework_take2_rebase/lib/start.js:144:23)
17:00:00      at Command.listener (/home/ubuntu/jenkins/workspace/_tor_profile_rework_take2_rebase/node_modules/commander/index.js:315:8)
17:00:00      at Command.emit (events.js:203:13)
17:00:00      at Command.parseArgs (/home/ubuntu/jenkins/workspace/_tor_profile_rework_take2_rebase/node_modules/commander/index.js:654:12)
17:00:00      at Command.parse (/home/ubuntu/jenkins/workspace/_tor_profile_rework_take2_rebase/node_modules/commander/index.js:474:21)
17:00:00      at Object.<anonymous> (/home/ubuntu/jenkins/workspace/_tor_profile_rework_take2_rebase/scripts/commands.js:155:4)
17:00:00      at Module._compile (internal/modules/cjs/loader.js:868:30)
17:00:00      at Object.Module._extensions..js (internal/modules/cjs/loader.js:879:10)
17:00:00      at Module.load (internal/modules/cjs/loader.js:731:32)
17:00:00  npm ERR! code ELIFECYCLE
17:00:00  npm ERR! errno 1
17:00:00  npm ERR! brave@0.72.66 network-audit: `node ./scripts/commands.js start --enable_brave_update --network_log --user_data_dir_name=brave-network-test "--output_path=src/out/Release/brave"`
17:00:00  npm ERR! Exit status 1
17:00:00  npm ERR! 
17:00:00  npm ERR! Failed at the brave@0.72.66 network-audit script.
17:00:00  npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
17:00:00  
17:00:00  npm ERR! A complete log of this run can be found in:
17:00:00  npm ERR!     /home/ubuntu/.npm/_logs/2019-10-02T23_59_49_713Z-debug.log

@rvklein
Copy link

rvklein commented May 14, 2020

Was forwarded here from this issue thread:
brave/brave-browser#2761

Please consider adding the ability to use browser extensions when running in Tor mode. This is incredibly important for a11y features such as user stylesheets, machine translation, colour inversion, forced high contrast, screen readers, and more.

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.

Refactor Tor implementation to retain consistent profile
5 participants