From e2082a0e325cd4d19b9608cc699ee81a347537bf Mon Sep 17 00:00:00 2001 From: treib Date: Wed, 8 Apr 2015 03:15:30 -0700 Subject: [PATCH] Supervised user SafeSites: Add a field trial to turn on/off. Also combine the enable/disable flags for blacklist and online check into a single multi-value flag. BUG=417722 Review URL: https://codereview.chromium.org/698253002 Cr-Commit-Position: refs/heads/master@{#324208} --- chrome/app/generated_resources.grd | 24 +++---- chrome/browser/about_flags.cc | 39 +++++++----- .../child_accounts/child_account_service.cc | 22 ------- .../child_accounts/child_account_service.h | 2 - .../supervised_user_filtering_switches.cc | 73 ++++++++++++++++++++++ .../supervised_user_filtering_switches.h | 17 +++++ .../supervised_user/supervised_user_service.cc | 11 ++-- chrome/chrome_browser.gypi | 2 + chrome/common/chrome_switches.cc | 20 ++---- chrome/common/chrome_switches.h | 5 +- tools/metrics/histograms/histograms.xml | 1 + 11 files changed, 138 insertions(+), 78 deletions(-) create mode 100644 chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc create mode 100644 chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 48da62965b95..78c72fbef3c1 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -6584,24 +6584,12 @@ Keep your key file in a safe place. You will need it to create new versions of y Enable the experimental Chrome suggestions service. - - Enable the child account host blacklist - - - Enable the host blacklist for use by child accounts. - Enable managed bookmarks for supervised users Enable the managed bookmarks folder for supervised users. - - Enable the child account SafeSites filter - - - Enable SafeSites filtering for child accounts. - Enable App Launcher sync @@ -7051,6 +7039,18 @@ Keep your key file in a safe place. You will need it to create new versions of y Demo mode + + Child account SafeSites filtering + + + Enable or disable SafeSites filtering for child accounts. + + + Static blacklist only + + + Online check only + diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc index 1e64b8341cf3..defc7a1ff762 100644 --- a/chrome/browser/about_flags.cc +++ b/chrome/browser/about_flags.cc @@ -438,6 +438,22 @@ const Experiment::Choice kDataSaverPromptChoices[] = { }; #endif +const Experiment::Choice kSupervisedUserSafeSitesChoices[] = { + { IDS_GENERIC_EXPERIMENT_CHOICE_DEFAULT, "", "" }, + { IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, + switches::kSupervisedUserSafeSites, + "enabled" }, + { IDS_GENERIC_EXPERIMENT_CHOICE_DISABLED, + switches::kSupervisedUserSafeSites, + "disabled" }, + { IDS_SUPERVISED_USER_SAFESITES_BLACKLIST_ONLY, + switches::kSupervisedUserSafeSites, + "blacklist-only" }, + { IDS_SUPERVISED_USER_SAFESITES_ONLINE_CHECK_ONLY, + switches::kSupervisedUserSafeSites, + "online-check-only" } +}; + // RECORDING USER METRICS FOR FLAGS: // ----------------------------------------------------------------------------- // The first line of the experiment is the internal name. If you'd like to @@ -1197,28 +1213,12 @@ const Experiment kExperiments[] = { switches::kDisableSuggestionsService) }, { - "enable-supervised-user-blacklist", - IDS_FLAGS_ENABLE_SUPERVISED_USER_BLACKLIST_NAME, - IDS_FLAGS_ENABLE_SUPERVISED_USER_BLACKLIST_DESCRIPTION, - kOsAndroid | kOsMac | kOsWin | kOsLinux | kOsCrOS, - ENABLE_DISABLE_VALUE_TYPE(switches::kEnableSupervisedUserBlacklist, - switches::kDisableSupervisedUserBlacklist) - }, - { "enable-supervised-user-managed-bookmarks-folder", IDS_FLAGS_ENABLE_SUPERVISED_USER_MANAGED_BOOKMARKS_FOLDER_NAME, IDS_FLAGS_ENABLE_SUPERVISED_USER_MANAGED_BOOKMARKS_FOLDER_DESCRIPTION, kOsAndroid | kOsMac | kOsWin | kOsLinux | kOsCrOS, SINGLE_VALUE_TYPE(switches::kEnableSupervisedUserManagedBookmarksFolder) }, - { - "enable-supervised-user-safesites", - IDS_FLAGS_ENABLE_SUPERVISED_USER_SAFESITES_NAME, - IDS_FLAGS_ENABLE_SUPERVISED_USER_SAFESITES_DESCRIPTION, - kOsAndroid | kOsMac | kOsWin | kOsLinux | kOsCrOS, - ENABLE_DISABLE_VALUE_TYPE(switches::kEnableSupervisedUserSafeSites, - switches::kDisableSupervisedUserSafeSites) - }, #if defined(ENABLE_APP_LIST) { "enable-sync-app-list", @@ -2341,6 +2341,13 @@ const Experiment kExperiments[] = { MULTI_VALUE_TYPE(kDataSaverPromptChoices) }, #endif // defined(OS_CHROMEOS) + { + "supervised-user-safesites", + IDS_FLAGS_SUPERVISED_USER_SAFESITES_NAME, + IDS_FLAGS_SUPERVISED_USER_SAFESITES_DESCRIPTION, + kOsAndroid | kOsMac | kOsWin | kOsLinux | kOsCrOS, + MULTI_VALUE_TYPE(kSupervisedUserSafeSitesChoices) + }, // NOTE: Adding new command-line switches requires adding corresponding // entries to enum "LoginCustomFlags" in histograms.xml. See note in // histograms.xml and don't forget to run AboutFlagsHistogramTest unit test. diff --git a/chrome/browser/supervised_user/child_accounts/child_account_service.cc b/chrome/browser/supervised_user/child_accounts/child_account_service.cc index c192d83d6871..9c7e092e6f75 100644 --- a/chrome/browser/supervised_user/child_accounts/child_account_service.cc +++ b/chrome/browser/supervised_user/child_accounts/child_account_service.cc @@ -181,8 +181,6 @@ bool ChildAccountService::SetActive(bool active) { SupervisedUserServiceFactory::GetForProfile(profile_); service->AddPermissionRequestCreator( PermissionRequestCreatorApiary::CreateWithProfile(profile_)); - - EnableExperimentalFiltering(); } else { SupervisedUserSettingsService* settings_service = SupervisedUserSettingsServiceFactory::GetForProfile(profile_); @@ -426,23 +424,3 @@ void ChildAccountService::ClearSecondCustodianPrefs() { profile_->GetPrefs()->ClearPref( prefs::kSupervisedUserSecondCustodianProfileImageURL); } - -void ChildAccountService::EnableExperimentalFiltering() { - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); - - // Static blacklist defaults to enabled. - bool has_enable_blacklist = - command_line->HasSwitch(switches::kEnableSupervisedUserBlacklist); - bool has_disable_blacklist = - command_line->HasSwitch(switches::kDisableSupervisedUserBlacklist); - if (!has_enable_blacklist && !has_disable_blacklist) - command_line->AppendSwitch(switches::kEnableSupervisedUserBlacklist); - - // Query-based filtering also defaults to enabled. - bool has_enable_safesites = - command_line->HasSwitch(switches::kEnableSupervisedUserSafeSites); - bool has_disable_safesites = - command_line->HasSwitch(switches::kDisableSupervisedUserSafeSites); - if (!has_enable_safesites && !has_disable_safesites) - command_line->AppendSwitch(switches::kEnableSupervisedUserSafeSites); -} diff --git a/chrome/browser/supervised_user/child_accounts/child_account_service.h b/chrome/browser/supervised_user/child_accounts/child_account_service.h index dcab44263746..67539a9a7d00 100644 --- a/chrome/browser/supervised_user/child_accounts/child_account_service.h +++ b/chrome/browser/supervised_user/child_accounts/child_account_service.h @@ -103,8 +103,6 @@ class ChildAccountService : public KeyedService, void ClearFirstCustodianPrefs(); void ClearSecondCustodianPrefs(); - void EnableExperimentalFiltering(); - // Owns us via the KeyedService mechanism. Profile* profile_; diff --git a/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc b/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc new file mode 100644 index 000000000000..557580730d1e --- /dev/null +++ b/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.cc @@ -0,0 +1,73 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h" + +#include "base/command_line.h" +#include "base/metrics/field_trial.h" +#include "chrome/common/chrome_switches.h" + +namespace { + +enum class SafeSitesState { + ENABLED, + DISABLED, + BLACKLIST_ONLY, + ONLINE_CHECK_ONLY +}; + +const char kSafeSitesFieldTrialName[] = "SafeSites"; + +SafeSitesState GetState() { + // Note: It's important to query the field trial state first, to ensure that + // UMA reports the correct group. + std::string trial_group = + base::FieldTrialList::FindFullName(kSafeSitesFieldTrialName); + + std::string arg = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( + switches::kSupervisedUserSafeSites); + if (!arg.empty()) { + if (arg == "enabled") + return SafeSitesState::ENABLED; + else if (arg == "disabled") + return SafeSitesState::DISABLED; + else if (arg == "blacklist-only") + return SafeSitesState::BLACKLIST_ONLY; + else if (arg == "online-check-only") + return SafeSitesState::ONLINE_CHECK_ONLY; + + LOG(WARNING) << "Invalid value \"" << arg << "\" specified for flag \"" + << switches::kSupervisedUserSafeSites + << "\", defaulting to \"disabled\""; + return SafeSitesState::DISABLED; + } + + // If no cmdline arg is specified, evaluate the field trial. + if (trial_group == "Disabled") + return SafeSitesState::DISABLED; + else if (trial_group == "BlacklistOnly") + return SafeSitesState::BLACKLIST_ONLY; + else if (trial_group == "OnlineCheckOnly") + return SafeSitesState::ONLINE_CHECK_ONLY; + else + return SafeSitesState::ENABLED; +} + +} // namespace + +namespace supervised_users { + +bool IsSafeSitesBlacklistEnabled() { + SafeSitesState state = GetState(); + return state == SafeSitesState::ENABLED || + state == SafeSitesState::BLACKLIST_ONLY; +} + +bool IsSafeSitesOnlineCheckEnabled() { + SafeSitesState state = GetState(); + return state == SafeSitesState::ENABLED || + state == SafeSitesState::ONLINE_CHECK_ONLY; +} + +} // namespace supervised_users diff --git a/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h b/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h new file mode 100644 index 000000000000..425bb1292a3d --- /dev/null +++ b/chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h @@ -0,0 +1,17 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SUPERVISED_USER_EXPERIMENTAL_SUPERVISED_USER_FILTERING_SWITCHES_H_ +#define CHROME_BROWSER_SUPERVISED_USER_EXPERIMENTAL_SUPERVISED_USER_FILTERING_SWITCHES_H_ + +namespace supervised_users { + +// These functions are wrappers around switches::kSupervisedUserSafeSites that +// evaluate a field trial if no command line arguments are specified. +bool IsSafeSitesBlacklistEnabled(); +bool IsSafeSitesOnlineCheckEnabled(); + +} // namespace supervised_users + +#endif // CHROME_BROWSER_SUPERVISED_USER_EXPERIMENTAL_SUPERVISED_USER_FILTERING_SWITCHES_H_ diff --git a/chrome/browser/supervised_user/supervised_user_service.cc b/chrome/browser/supervised_user/supervised_user_service.cc index 440fa1cc8fd6..20c3f5a3f970 100644 --- a/chrome/browser/supervised_user/supervised_user_service.cc +++ b/chrome/browser/supervised_user/supervised_user_service.cc @@ -18,6 +18,7 @@ #include "chrome/browser/signin/profile_oauth2_token_service_factory.h" #include "chrome/browser/signin/signin_manager_factory.h" #include "chrome/browser/supervised_user/experimental/supervised_user_blacklist_downloader.h" +#include "chrome/browser/supervised_user/experimental/supervised_user_filtering_switches.h" #include "chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h" #include "chrome/browser/supervised_user/legacy/custodian_profile_downloader_service_factory.h" #include "chrome/browser/supervised_user/legacy/permission_request_creator_sync.h" @@ -797,16 +798,14 @@ void SupervisedUserService::SetActive(bool active) { whitelist_service_->Init(); UpdateManualHosts(); UpdateManualURLs(); - bool use_blacklist = base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableSupervisedUserBlacklist); - if (delegate_ && use_blacklist) { + if (profile_->IsChild() && + supervised_users::IsSafeSitesBlacklistEnabled()) { base::FilePath blacklist_path = delegate_->GetBlacklistPath(); if (!blacklist_path.empty()) LoadBlacklist(blacklist_path, delegate_->GetBlacklistURL()); } - bool use_safesites = base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableSupervisedUserSafeSites); - if (delegate_ && use_safesites) { + if (profile_->IsChild() && + supervised_users::IsSafeSitesOnlineCheckEnabled()) { const std::string& cx = delegate_->GetSafeSitesCx(); if (!cx.empty()) { url_filter_context_.InitAsyncURLChecker( diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 3490870cfa51..a8a5f9b9d1dd 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -2670,6 +2670,8 @@ 'browser/supervised_user/experimental/supervised_user_blacklist.h', 'browser/supervised_user/experimental/supervised_user_blacklist_downloader.cc', 'browser/supervised_user/experimental/supervised_user_blacklist_downloader.h', + 'browser/supervised_user/experimental/supervised_user_filtering_switches.cc', + 'browser/supervised_user/experimental/supervised_user_filtering_switches.h', 'browser/supervised_user/legacy/custodian_profile_downloader_service.cc', 'browser/supervised_user/legacy/custodian_profile_downloader_service.h', 'browser/supervised_user/legacy/custodian_profile_downloader_service_factory.cc', diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc index c452036fb5d6..61a7647298b6 100644 --- a/chrome/common/chrome_switches.cc +++ b/chrome/common/chrome_switches.cc @@ -354,14 +354,6 @@ const char kDisableSessionCrashedBubble[] = "disable-session-crashed-bubble"; // Disables the suggestions service. const char kDisableSuggestionsService[] = "disable-suggestions-service"; -// Disables the supervised user host blacklist. -const char kDisableSupervisedUserBlacklist[] = - "disable-supervised-user-blacklist"; - -// Disables SafeSites filtering for supervised users. -const char kDisableSupervisedUserSafeSites[] = - "disable-supervised-user-safesites"; - // Disables syncing browser data to a Google Account. const char kDisableSync[] = "disable-sync"; @@ -575,18 +567,10 @@ const char kEnableSpdy4[] = "enable-spdy4"; // Enables the suggestions service. const char kEnableSuggestionsService[] = "enable-suggestions-service"; -// Enables the supervised user host blacklist. -const char kEnableSupervisedUserBlacklist[] = - "enable-supervised-user-blacklist"; - // Enables the supervised user managed bookmarks folder. const char kEnableSupervisedUserManagedBookmarksFolder[] = "enable-supervised-user-managed-bookmarks-folder"; -// Enables SafeSites filtering for supervised users. -const char kEnableSupervisedUserSafeSites[] = - "enable-supervised-user-safesites"; - // Enables synced articles. const char kEnableSyncArticles[] = "enable-sync-articles"; @@ -1126,6 +1110,10 @@ const char kStartMaximized[] = "start-maximized"; // Used for testing. const char kSupervisedUserId[] = "managed-user-id"; +// Enables/disables SafeSites filtering for supervised users. Possible values +// are "enabled", "disabled", "blacklist-only", and "online-check-only". +const char kSupervisedUserSafeSites[] = "supervised-user-safesites"; + // Used to authenticate requests to the Sync service for supervised users. // Setting this switch also causes Sync to be set up for a supervised user. const char kSupervisedUserSyncToken[] = "managed-user-sync-token"; diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h index fbcdcfd968f5..1dcfabdcf602 100644 --- a/chrome/common/chrome_switches.h +++ b/chrome/common/chrome_switches.h @@ -104,8 +104,6 @@ extern const char kDisableSavePasswordBubble[]; extern const char kDisableSdchPersistence[]; extern const char kDisableSessionCrashedBubble[]; extern const char kDisableSuggestionsService[]; -extern const char kDisableSupervisedUserBlacklist[]; -extern const char kDisableSupervisedUserSafeSites[]; extern const char kDisableSync[]; extern const char kDisableSyncTypes[]; extern const char kDisableWebResources[]; @@ -167,9 +165,7 @@ extern const char kEnableSettingsWindow[]; extern const char kDisableSettingsWindow[]; extern const char kEnableSpdy4[]; extern const char kEnableSuggestionsService[]; -extern const char kEnableSupervisedUserBlacklist[]; extern const char kEnableSupervisedUserManagedBookmarksFolder[]; -extern const char kEnableSupervisedUserSafeSites[]; extern const char kEnableSyncArticles[]; extern const char kEnableTabAudioMuting[]; extern const char kEnableThumbnailRetargeting[]; @@ -313,6 +309,7 @@ extern const char kSSLVersionTLSv11[]; extern const char kSSLVersionTLSv12[]; extern const char kStartMaximized[]; extern const char kSupervisedUserId[]; +extern const char kSupervisedUserSafeSites[]; extern const char kSupervisedUserSyncToken[]; extern const char kSyncShortInitialRetryOverride[]; extern const char kSyncServiceURL[]; diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index f8ae7cfa61a6..5275953d887c 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -54288,6 +54288,7 @@ To add a new entry, add it with any value and run test to compute valid value. + -- 2.11.4.GIT