From 7831f57d5f56e9cafe77388b20a83a90d6eb7918 Mon Sep 17 00:00:00 2001 From: mlerman Date: Mon, 25 May 2015 04:40:15 -0700 Subject: [PATCH] Fix the System Profile to not load extensions. It could otherwise be forced to by GPO. A few releases ago I introduced a System Profile to back the User Manager (https://codereview.chromium.org/847733005). This profile, however, is causing some problems. I'm trying to reduce those by making clearer the contract of what it's for: A profile that doesn't take a browser window, has no extensions, writes little to disk, etc. This certainly addresses issue #4 raised within the bug, and address the other issues as well (I can't repro them). This CL generally hardens Chrome for the System Profile in a lot of the same ways we do the Guest Profile. BUG=482176 TBR=jcivelli@chromium.org (a function, called from chrome/test that lives in c/b/profiles, was renamed). Review URL: https://codereview.chromium.org/1129293002 Cr-Commit-Position: refs/heads/master@{#331276} --- chrome/browser/app_controller_mac.mm | 16 +++++++------ .../extensions/chrome_process_manager_delegate.cc | 3 ++- chrome/browser/extensions/extension_system_impl.cc | 3 ++- chrome/browser/extensions/tab_helper.cc | 1 + chrome/browser/profiles/profile_manager.cc | 26 ++++++++++++---------- chrome/browser/profiles/profile_manager.h | 5 +++-- chrome/browser/profiles/profile_window.cc | 2 +- .../ui/ash/launcher/chrome_launcher_controller.cc | 9 ++++---- chrome/browser/ui/browser.cc | 6 +++++ chrome/browser/ui/browser_command_controller.cc | 6 ++++- .../browser/ui/startup/startup_browser_creator.cc | 11 ++++----- chrome/test/base/testing_profile_manager.cc | 2 +- tools/metrics/actions/actions.xml | 9 ++++++++ 13 files changed, 64 insertions(+), 35 deletions(-) diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index 06f9e19bdfc0..40245a05184a 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -1032,9 +1032,10 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { NSInteger tag = [sender tag]; // If there are no browser windows, and we are trying to open a browser - // for a locked profile, we have to show the User Manager instead as the - // locked profile needs authentication. - if (IsProfileSignedOut(lastProfile)) { + // for a locked profile or the system profile, we have to show the User + // Manager instead as the locked profile needs authentication and the system + // profile cannot have a browser. + if (IsProfileSignedOut(lastProfile) || lastProfile->IsSystemProfile()) { UserManager::Show(base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL, profiles::USER_MANAGER_SELECT_PROFILE_NO_ACTION); @@ -1246,11 +1247,12 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { // Otherwise open a new window. // If the last profile was locked, we have to open the User Manager, as the - // profile requires authentication. Similarly, because guest mode is - // implemented as forced incognito, we can't open a new guest browser either, - // so we have to show the User Manager as well. + // profile requires authentication. Similarly, because guest mode and system + // profile are implemented as forced incognito, we can't open a new guest + // browser either, so we have to show the User Manager as well. Profile* lastProfile = [self lastProfile]; - if (lastProfile->IsGuestSession() || IsProfileSignedOut(lastProfile)) { + if (lastProfile->IsGuestSession() || IsProfileSignedOut(lastProfile) || + lastProfile->IsSystemProfile()) { UserManager::Show(base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL, profiles::USER_MANAGER_SELECT_PROFILE_NO_ACTION); diff --git a/chrome/browser/extensions/chrome_process_manager_delegate.cc b/chrome/browser/extensions/chrome_process_manager_delegate.cc index e3d1354a1b64..d141058e816c 100644 --- a/chrome/browser/extensions/chrome_process_manager_delegate.cc +++ b/chrome/browser/extensions/chrome_process_manager_delegate.cc @@ -41,7 +41,8 @@ bool ChromeProcessManagerDelegate::IsBackgroundPageAllowed( content::BrowserContext* context) const { Profile* profile = static_cast(context); - bool is_normal_session = !profile->IsGuestSession(); + bool is_normal_session = !profile->IsGuestSession() && + !profile->IsSystemProfile(); #if defined(OS_CHROMEOS) is_normal_session = is_normal_session && user_manager::UserManager::Get()->IsUserLoggedIn(); diff --git a/chrome/browser/extensions/extension_system_impl.cc b/chrome/browser/extensions/extension_system_impl.cc index 667a693bc2a8..b03d987c373d 100644 --- a/chrome/browser/extensions/extension_system_impl.cc +++ b/chrome/browser/extensions/extension_system_impl.cc @@ -306,7 +306,8 @@ void ExtensionSystemImpl::Shared::Init(bool extensions_enabled) { // ExtensionService depends on RuntimeData. runtime_data_.reset(new RuntimeData(ExtensionRegistry::Get(profile_))); - bool autoupdate_enabled = !profile_->IsGuestSession(); + bool autoupdate_enabled = !profile_->IsGuestSession() && + !profile_->IsSystemProfile(); #if defined(OS_CHROMEOS) if (!extensions_enabled) autoupdate_enabled = false; diff --git a/chrome/browser/extensions/tab_helper.cc b/chrome/browser/extensions/tab_helper.cc index 0c057b4e3cc1..e3c8c445b453 100644 --- a/chrome/browser/extensions/tab_helper.cc +++ b/chrome/browser/extensions/tab_helper.cc @@ -144,6 +144,7 @@ bool TabHelper::CanCreateApplicationShortcuts() const { bool TabHelper::CanCreateBookmarkApp() const { return !profile_->IsGuestSession() && + !profile_->IsSystemProfile() && IsValidBookmarkAppUrl(web_contents()->GetURL()); } diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc index 408917b51b3d..b4d70c8f3507 100644 --- a/chrome/browser/profiles/profile_manager.cc +++ b/chrome/browser/profiles/profile_manager.cc @@ -306,6 +306,7 @@ Profile* ProfileManager::GetLastUsedProfile() { Profile* ProfileManager::GetLastUsedProfileAllowedByPolicy() { Profile* profile = GetLastUsedProfile(); if (profile->IsGuestSession() || + profile->IsSystemProfile() || IncognitoModePrefs::GetAvailability(profile->GetPrefs()) == IncognitoModePrefs::FORCED) { return profile->GetOffTheRecordProfile(); @@ -439,8 +440,10 @@ void ProfileManager::CreateProfileAsync( if (iter != profiles_info_.end() && info->created) { Profile* profile = info->profile.get(); // If this was the guest profile, apply settings and go OffTheRecord. - if (profile->GetPath() == ProfileManager::GetGuestProfilePath()) { - SetGuestProfilePrefs(profile); + // The system profile also needs characteristics of being off the record, + // such as having no extensions, not writing to disk, etc. + if (profile->IsGuestSession() || profile->IsSystemProfile()) { + SetNonPersonalProfilePrefs(profile); profile = profile->GetOffTheRecordProfile(); } // Profile has already been created. Run callback immediately. @@ -1004,9 +1007,10 @@ void ProfileManager::OnProfileCreated(Profile* profile, } if (profile) { - // If this was the guest profile, finish setting its special status. - if (profile->GetPath() == ProfileManager::GetGuestProfilePath()) - SetGuestProfilePrefs(profile); + // If this was the guest or system profile, finish setting its special + // status. + if (profile->IsGuestSession() || profile->IsSystemProfile()) + SetNonPersonalProfilePrefs(profile); // Invoke CREATED callback for incognito profiles. if (go_off_the_record) @@ -1326,16 +1330,12 @@ void ProfileManager::AddProfileToCache(Profile* profile) { } } -void ProfileManager::SetGuestProfilePrefs(Profile* profile) { +void ProfileManager::SetNonPersonalProfilePrefs(Profile* profile) { PrefService* prefs = profile->GetPrefs(); prefs->SetBoolean(prefs::kSigninAllowed, false); prefs->SetBoolean(bookmarks::prefs::kEditBookmarksEnabled, false); prefs->SetBoolean(bookmarks::prefs::kShowBookmarkBar, false); prefs->ClearPref(DefaultSearchManager::kDefaultSearchProviderDataPrefName); - // This can be removed in the future but needs to be present through - // a release (or two) so that any existing installs get switched to - // the new state and away from the previous "forced" state. - IncognitoModePrefs::SetAvailability(prefs, IncognitoModePrefs::ENABLED); } bool ProfileManager::ShouldGoOffTheRecord(Profile* profile) { @@ -1344,7 +1344,7 @@ bool ProfileManager::ShouldGoOffTheRecord(Profile* profile) { return true; } #endif - return profile->IsGuestSession(); + return profile->IsGuestSession() || profile->IsSystemProfile(); } void ProfileManager::RunCallbacks(const std::vector& callbacks, @@ -1370,7 +1370,9 @@ void ProfileManager::UpdateLastUser(Profile* last_active) { PrefService* local_state = g_browser_process->local_state(); DCHECK(local_state); // Only keep track of profiles that we are managing; tests may create others. - if (profiles_info_.find(last_active->GetPath()) != profiles_info_.end()) { + // Also never consider the SystemProfile as "active". + if (profiles_info_.find(last_active->GetPath()) != profiles_info_.end() && + !last_active->IsSystemProfile()) { std::string profile_path_base = last_active->GetPath().BaseName().MaybeAsASCII(); if (profile_path_base != GetLastUsedProfileName()) diff --git a/chrome/browser/profiles/profile_manager.h b/chrome/browser/profiles/profile_manager.h index 4b5c8a881d7f..f502168e6f18 100644 --- a/chrome/browser/profiles/profile_manager.h +++ b/chrome/browser/profiles/profile_manager.h @@ -286,8 +286,9 @@ class ProfileManager : public base::NonThreadSafe, // Adds |profile| to the profile info cache if it hasn't been added yet. void AddProfileToCache(Profile* profile); - // Apply settings for (desktop) Guest User profile. - void SetGuestProfilePrefs(Profile* profile); + // Apply settings for profiles created by the system rather than users: The + // (desktop) Guest User profile and (desktop) System Profile. + void SetNonPersonalProfilePrefs(Profile* profile); // For ChromeOS, determines if profile should be otr. bool ShouldGoOffTheRecord(Profile* profile); diff --git a/chrome/browser/profiles/profile_window.cc b/chrome/browser/profiles/profile_window.cc index 177bb3bf4415..90b4a353a72c 100644 --- a/chrome/browser/profiles/profile_window.cc +++ b/chrome/browser/profiles/profile_window.cc @@ -384,7 +384,7 @@ bool IsLockAvailable(Profile* profile) { if (!switches::IsNewProfileManagement()) return false; - if (profile->IsGuestSession()) + if (profile->IsGuestSession() || profile->IsSystemProfile()) return false; std::string hosted_domain = profile->GetPrefs()-> diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc index 0e6b955ca7a3..295ca318799c 100644 --- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc +++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc @@ -374,9 +374,9 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile, // Use the original profile as on chromeos we may get a temporary off the // record profile, unless in guest session (where off the record profile is // the right one). - Profile* active_profile = ProfileManager::GetActiveUserProfile(); - profile_ = active_profile->IsGuestSession() ? active_profile : - active_profile->GetOriginalProfile(); + profile_ = ProfileManager::GetActiveUserProfile(); + if (!profile_->IsGuestSession() && !profile_->IsSystemProfile()) + profile_ = profile_->GetOriginalProfile(); app_sync_ui_state_ = AppSyncUIState::Get(profile_); if (app_sync_ui_state_) @@ -2052,7 +2052,8 @@ bool ChromeLauncherController::IsIncognito( const content::WebContents* web_contents) const { const Profile* profile = Profile::FromBrowserContext(web_contents->GetBrowserContext()); - return profile->IsOffTheRecord() && !profile->IsGuestSession(); + return profile->IsOffTheRecord() && !profile->IsGuestSession() && + !profile->IsSystemProfile(); } void ChromeLauncherController::CloseWindowedAppsFromRemovedExtension( diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index ff1fedc7d932..6ccb52cab2b0 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -366,6 +366,12 @@ Browser::Browser(const CreateParams& params) CHECK(IncognitoModePrefs::CanOpenBrowser(profile_)); CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord()) << "Only off the record browser may be opened in guest mode"; + DCHECK(!profile_->IsSystemProfile()) + << "The system profile should never have a real browser."; + // TODO(mlerman): After this hits stable channel, see if there are counts + // for this metric. If not, change the DCHECK above to a CHECK. + if (profile_->IsSystemProfile()) + content::RecordAction(base::UserMetricsAction("BrowserForSystemProfile")); // TODO(jeremy): Move to initializer list once flag is removed. if (IsFastTabUnloadEnabled()) diff --git a/chrome/browser/ui/browser_command_controller.cc b/chrome/browser/ui/browser_command_controller.cc index 570de3e90725..ab25a9acb600 100644 --- a/chrome/browser/ui/browser_command_controller.cc +++ b/chrome/browser/ui/browser_command_controller.cc @@ -954,7 +954,10 @@ void BrowserCommandController::InitCommandState() { command_updater_.UpdateCommandEnabled(IDC_ZOOM_MINUS, true); // Show various bits of UI - const bool guest_session = profile()->IsGuestSession(); + const bool guest_session = profile()->IsGuestSession() || + profile()->IsSystemProfile(); + DCHECK(!profile()->IsSystemProfile()) + << "Ought to never have browser for the system profile."; const bool normal_window = browser_->is_type_tabbed(); UpdateOpenFileState(&command_updater_); command_updater_.UpdateCommandEnabled(IDC_CREATE_SHORTCUTS, false); @@ -1193,6 +1196,7 @@ void BrowserCommandController::UpdateCommandsForBookmarkBar() { command_updater_.UpdateCommandEnabled( IDC_SHOW_BOOKMARK_BAR, browser_defaults::bookmarks_enabled && !profile()->IsGuestSession() && + !profile()->IsSystemProfile() && !profile()->GetPrefs()->IsManagedPreference( bookmarks::prefs::kShowBookmarkBar) && IsShowingMainUI()); diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc index 82f182bb39d5..38cc153f64b0 100644 --- a/chrome/browser/ui/startup/startup_browser_creator.cc +++ b/chrome/browser/ui/startup/startup_browser_creator.cc @@ -682,15 +682,16 @@ bool StartupBrowserCreator::ProcessCmdLineImpl( bool signin_required = profile_index != std::string::npos && profile_info.ProfileIsSigninRequiredAtIndex(profile_index); - // Guest or locked profiles cannot be re-opened on startup. The only - // exception is if there's already a Guest window open in a separate + // Guest, system or locked profiles cannot be re-opened on startup. The + // only exception is if there's already a Guest window open in a separate // process (for example, launching a new browser after clicking on a // downloaded file in Guest mode). - bool has_guest_browsers = last_used_profile->IsGuestSession() && + bool guest_or_system = last_used_profile->IsGuestSession() || + last_used_profile->IsSystemProfile(); + bool has_guest_browsers = guest_or_system && chrome::GetTotalBrowserCountForProfile( last_used_profile->GetOffTheRecordProfile()) > 0; - if (signin_required || - (last_used_profile->IsGuestSession() && !has_guest_browsers)) { + if (signin_required || (guest_or_system && !has_guest_browsers)) { profiles::UserManagerProfileSelected action = command_line.HasSwitch(switches::kShowAppList) ? profiles::USER_MANAGER_SELECT_PROFILE_APP_LAUNCHER : diff --git a/chrome/test/base/testing_profile_manager.cc b/chrome/test/base/testing_profile_manager.cc index 46ec927ee9b6..0ae9f9995f88 100644 --- a/chrome/test/base/testing_profile_manager.cc +++ b/chrome/test/base/testing_profile_manager.cc @@ -130,7 +130,7 @@ TestingProfile* TestingProfileManager::CreateGuestProfile() { TestingProfile::Builder().BuildIncognito(profile); profile_manager_->AddProfile(profile); // Takes ownership. - profile_manager_->SetGuestProfilePrefs(profile); + profile_manager_->SetNonPersonalProfilePrefs(profile); testing_profiles_.insert(std::make_pair(kGuestProfileName, profile)); diff --git a/tools/metrics/actions/actions.xml b/tools/metrics/actions/actions.xml index 457fb3b8a18a..3158004aa20a 100644 --- a/tools/metrics/actions/actions.xml +++ b/tools/metrics/actions/actions.xml @@ -1781,6 +1781,15 @@ should be able to be added at any place in this file. Please enter the description of this user action. + + mlerman@chromium.org + + A browser opened with the System Profile. Tsk, tsk. Any non-zero amount + means something is using the system profile for a browser when it likely + shouldn't be. + + + Please list the metric's owners. Add more owner tags as needed. Please enter the description of this user action. -- 2.11.4.GIT