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

Make Tor profile an OTR profile associated with the original profile #7069

Merged
merged 12 commits into from
Nov 19, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Nov 6, 2020

Resolves brave/brave-browser#12429
Resolves brave/brave-browser#12470
Resolves brave/brave-browser#12650

This PR migrates Tor session profile to off the record Tor profile created from the regular profile used to create Tor window. Bookmarks, preferences, content settings are shared between regular and tor OTR, just like normal OTR. Extensions are still blocked like what we have in Tor session profile, extensions support will be addressed in different PR. Legacy Tor session profile will be removed during shutdown after upgrade.
It also fixes the issue that multiple Tor profiles opened from different regular profiles don't have correct proxy config

Submitter Checklist:

Test Plan:

Multiple profiles issue

  1. Test plan in TOR not working with Multiple Profiles Active brave-browser#12650
  2. Test plan in Click on the Open in Tor link from Guest window doesn't connect to TOR network brave-browser#12470

Tor profile migration

  1. Have two regular profiles, default and profile 1
  2. Open a tor window separately for both profiles from old Brave browser and Quit Brave
  3. Launch new Brave from the same profile
  4. Open a tor window and go to https://check.torproject.org/
  5. It should be connected to Tor network
  6. Quit Brave
  7. [user_data_dir]/Default/session_profiles/Tor Profile and [user_data_dir]/Profile 1/session_profiles/Tor Profile should be removed

Tor Window should act like Private Window with Tor connectivity

  1. Add/Remove/Modified Bookmarks would be synced with regular profile
  2. Shield setting inherited from regular profile when Tor window opened, changes will not reflect to regular profile
  3. Preference like "Show home button" would be synced with regular profile

Non component extensions blocked

  1. Install an extension and turn on Allow in private in brave://extensions/
  2. Open Tor window, the extension should not be accessible but shield(component extension) should still work
  3. Open Private window, the extension should be accessible

Different circuit for different tor profiles

  1. Have two regular profiles, default and profile 1
  2. Open Tor windows for them separately
  3. Visit https://check.torproject.org on both Tor windows
  4. IPs should be different
  5. Click "New Tor connections for this site" on both Tor windows
  6. IPs should be different

Tor process cleanup

  1. Have two regular profiles, default and profile 1
  2. Open Tor windows for them separately
  3. Open task manager in Brave, there should be a "Tor Launcher" process
  4. Close one Tor window
  5. Check "Tor Launcher" is still there
  6. Close the last Tor window
  7. "Tor Launcher" should be gone 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.

@darkdh darkdh self-assigned this Nov 6, 2020
@darkdh darkdh force-pushed the tor-otr-profile branch 2 times, most recently from 510c3dc to ffe04bb Compare November 11, 2020 01:33
@darkdh darkdh marked this pull request as ready for review November 13, 2020 18:56
@darkdh darkdh requested a review from a team as a code owner November 13, 2020 18:56
@darkdh darkdh requested review from yrliou and bbondy November 13, 2020 18:56
@darkdh darkdh force-pushed the tor-otr-profile branch 7 times, most recently from 2e5097e to b774945 Compare November 17, 2020 18:13
@darkdh darkdh requested a review from bridiver November 17, 2020 18:14
}

bool Profile::IsIncognitoProfile() const {
+ BRAVE_IS_INCOGNITO_PROFILE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this one can be done via a chromium_src override.

Copy link
Member Author

@darkdh darkdh Nov 17, 2020

Choose a reason for hiding this comment

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

fixed in d38e8f5db1429052e1130c193f7c3562bbfbc1e1

@@ -6,7 +6,11 @@
#ifndef BRAVE_CHROMIUM_SRC_CONTENT_PUBLIC_BROWSER_BROWSER_CONTEXT_H_
#define BRAVE_CHROMIUM_SRC_CONTENT_PUBLIC_BROWSER_BROWSER_CONTEXT_H_

#define BRAVE_BROWSER_CONTEXT_H_ \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use IsOffTheRecord to also define IsTor to avoid the patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I already tried that, it doesn't work with virtual function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Builds ok for me. I tried:

#define IsOffTheRecord IsTor() const; \
  virtual bool IsOffTheRecord

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it builds ok, there was a crash when I use define method. But I just tried it now, it doesn't crash anymore so I guess there must be something else fix the issue with the following commits.
Anyway, I will use define instead of patch

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in d38e8f5db1429052e1130c193f7c3562bbfbc1e1

@darkdh darkdh force-pushed the tor-otr-profile branch 3 times, most recently from 4f8b1a0 to 1f00ae3 Compare November 17, 2020 23:50

GetLastUsedProfileName) {
g_browser_process->local_state()->SetString(
prefs::kProfileLastUsed, base::FilePath(tor::kTorProfileDir).value());
Copy link
Collaborator

Choose a reason for hiding this comment

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

base::FilePath::value() will return a wstring on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will use AsUTF8Unsafe()

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 1acf6fe166dfd865f8df5ebc59c4d67de0513e85

browser/profiles/brave_profile_manager.cc Outdated Show resolved Hide resolved
browser/tor/tor_profile_manager.cc Show resolved Hide resolved
browser/tor/tor_profile_manager.cc Outdated Show resolved Hide resolved
browser/tor/tor_profile_manager.cc Show resolved Hide resolved
browser/tor/tor_profile_manager.cc Show resolved Hide resolved
chromium_src/extensions/browser/extension_util.cc Outdated Show resolved Hide resolved
@darkdh darkdh force-pushed the tor-otr-profile branch 3 times, most recently from 5e1909f to 3ace81e Compare November 18, 2020 07:59
@darkdh darkdh force-pushed the tor-otr-profile branch 2 times, most recently from 1cf2607 to 6ebf9ec Compare November 18, 2020 23:25
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.

browser/themes/brave_theme_service.cc : LGTM

for (Profile* profile : profile_manager->GetLoadedProfiles()) {
const base::FilePath tor_legacy_session_path =
profile->GetPath()
.AppendASCII("session_profiles")
Copy link
Member

Choose a reason for hiding this comment

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

How about making session_profiles as a constant instead of hard-cording?
It looks like some places used hard coded string.

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 is only used for migration

Copy link
Member

Choose a reason for hiding this comment

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

It also hard coded below.

bool IsSessionProfilePath(const base::FilePath& path) {
  return path.DirName().BaseName() ==
         base::FilePath(FILE_PATH_LITERAL("session_profiles"));
}

Copy link
Member Author

@darkdh darkdh Nov 19, 2020

Choose a reason for hiding this comment

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

fixed in c4ad0bd

@darkdh
Copy link
Member Author

darkdh commented Nov 19, 2020

Unrelated browser tests failure

17:54:09  1 test failed:
17:54:09      RewardsPromotionBrowserTest.PromotionRemovedFromEndpoint (../../brave/components/brave_rewards/browser/test/rewards_promotion_browsertest.cc:230)
17:54:09  1 test timed out:
17:54:09      RewardsBrowserTest.BackupRestoreModalHasNotice (../../brave/components/brave_rewards/browser/test/rewards_browsertest.cc:303)

https://ci.brave.com/job/pr-brave-browser-tor-otr-profile-windows/33/execution/node/247/log/

@@ -23,7 +23,7 @@ const ThemeHelper& GetBraveThemeHelper(Profile* profile) {
#endif
// Because the helper is created as a NoDestructor static, we need separate
// instances for regular vs tor/guest profiles.
if (brave::IsTorProfile(profile) || brave::IsGuestProfile(profile)) {
if (profile->IsTor() || brave::IsGuestProfile(profile)) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think I gave a wrong review here.
This returns always false even if current window is tor because ThemeService uses redirected profile.
Fortunately, this will not make any regression to theme service becaue tor profile is also incognito now and incognito and tor uses same theme.
However, I'm not sure other areas have this issue or not.

But my another PR(#6912) needs more work because I need to use different theme colors for private and tor window. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants