Commit 4555b491 authored by Carlos IL's avatar Carlos IL Committed by Commit Bot

Removed extended reporting checkbox if unified consent is enabled.

This CL removes the checkbox for certificate reporting from SSL
interstitials, and for SBER from safe browsing interstitials if unified
consent is enabled.

Bug: 861747
Change-Id: I751c31cd70e7ba2a1e6358586652b5ae0b0ae18e
Reviewed-on: https://chromium-review.googlesource.com/1189058Reviewed-by: default avatarMustafa Emre Acer <meacer@chromium.org>
Reviewed-by: default avatarThomas Tangl <tangltom@chromium.org>
Reviewed-by: default avatarJialiu Lin <jialiul@chromium.org>
Reviewed-by: default avatarChangwan Ryu <changwan@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587893}
parent d0a1ab99
......@@ -101,6 +101,7 @@ void AwSafeBrowsingBlockingPage::ShowBlockingPage(
IsMainPageLoadBlocked(unsafe_resources),
safe_browsing::IsExtendedReportingOptInAllowed(*pref_service),
false, // is_off_the_record
false, // is_unified_consent_enabled
safe_browsing::IsExtendedReportingEnabled(*pref_service),
safe_browsing::IsScout(*pref_service),
safe_browsing::IsExtendedReportingPolicyManaged(*pref_service),
......
......@@ -17,12 +17,15 @@
#include "chrome/browser/renderer_preferences_util.h"
#include "chrome/browser/safe_browsing/safe_browsing_controller_client.h"
#include "chrome/browser/safe_browsing/safe_browsing_service.h"
#include "chrome/browser/signin/unified_consent_helper.h"
#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
#include "chrome/common/pref_names.h"
#include "components/prefs/pref_service.h"
#include "components/safe_browsing/browser/threat_details.h"
#include "components/safe_browsing/common/safe_browsing_prefs.h"
#include "components/safe_browsing/triggers/trigger_manager.h"
#include "components/security_interstitials/content/security_interstitial_controller_client.h"
#include "components/unified_consent/unified_consent_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/interstitial_page.h"
#include "content/public/browser/navigation_entry.h"
......@@ -56,13 +59,18 @@ class SafeBrowsingBlockingPageFactoryImpl
const SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources)
override {
// Create appropriate display options for this blocking page.
PrefService* prefs =
Profile::FromBrowserContext(web_contents->GetBrowserContext())
->GetPrefs();
Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
PrefService* prefs = profile->GetPrefs();
bool is_extended_reporting_opt_in_allowed =
IsExtendedReportingOptInAllowed(*prefs);
bool is_proceed_anyway_disabled =
prefs->GetBoolean(prefs::kSafeBrowsingProceedAnywayDisabled);
bool is_unified_consent_enabled = IsUnifiedConsentFeatureEnabled(profile);
unified_consent::UnifiedConsentService* consent_service =
UnifiedConsentServiceFactory::GetForProfile(profile);
bool is_unified_consent_given =
consent_service && consent_service->IsUnifiedConsentGiven();
// Determine if any prefs need to be updated prior to showing the security
// interstitial. This must happen before querying IsScout to populate the
......@@ -73,6 +81,7 @@ class SafeBrowsingBlockingPageFactoryImpl
BaseBlockingPage::IsMainPageLoadBlocked(unsafe_resources),
is_extended_reporting_opt_in_allowed,
web_contents->GetBrowserContext()->IsOffTheRecord(),
is_unified_consent_enabled && is_unified_consent_given,
IsExtendedReportingEnabled(*prefs), IsScout(*prefs),
IsExtendedReportingPolicyManaged(*prefs), is_proceed_anyway_disabled,
true, // should_open_links_in_new_tab
......
......@@ -36,6 +36,7 @@
#include "base/macros.h"
#include "components/safe_browsing/base_blocking_page.h"
#include "components/safe_browsing/base_ui_manager.h"
#include "components/signin/core/browser/signin_buildflags.h"
namespace safe_browsing {
......@@ -98,6 +99,13 @@ class SafeBrowsingBlockingPage : public BaseBlockingPage {
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTest,
ExtendedReportingNotShownNotAllowExtendedReporting);
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTest, BillingPage);
#if BUILDFLAG(ENABLE_DICE_SUPPORT)
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTestDiceEnabled,
ExtendedReportingNotShownUnifiedConsent);
#else
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingBlockingPageTest,
ExtendedReportingNotShownUnifiedConsent);
#endif
void UpdateReportingPref(); // Used for the transition from old to new pref.
......
......@@ -28,12 +28,14 @@
#include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
#include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
#include "chrome/browser/safe_browsing/ui_manager.h"
#include "chrome/browser/signin/unified_consent_helper.h"
#include "chrome/browser/ssl/cert_verifier_browser_test.h"
#include "chrome/browser/ssl/security_state_tab_helper.h"
#include "chrome/browser/ssl/ssl_blocking_page.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
#include "chrome/common/pref_names.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
......@@ -56,6 +58,7 @@
#include "components/security_interstitials/core/urls.h"
#include "components/security_state/core/security_state.h"
#include "components/strings/grit/components_strings.h"
#include "components/unified_consent/unified_consent_service.h"
#include "content/public/browser/interstitial_page.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/navigation_entry.h"
......@@ -315,17 +318,24 @@ class TestSafeBrowsingBlockingPageFactory
const GURL& main_frame_url,
const SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources)
override {
PrefService* prefs =
Profile::FromBrowserContext(web_contents->GetBrowserContext())
->GetPrefs();
Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
PrefService* prefs = profile->GetPrefs();
bool is_extended_reporting_opt_in_allowed =
prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
bool is_proceed_anyway_disabled =
prefs->GetBoolean(prefs::kSafeBrowsingProceedAnywayDisabled);
bool is_unified_consent_enabled = IsUnifiedConsentFeatureEnabled(profile);
unified_consent::UnifiedConsentService* consent_service =
UnifiedConsentServiceFactory::GetForProfile(profile);
bool is_unified_consent_given =
consent_service && consent_service->IsUnifiedConsentGiven();
BaseSafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
BaseBlockingPage::IsMainPageLoadBlocked(unsafe_resources),
is_extended_reporting_opt_in_allowed,
web_contents->GetBrowserContext()->IsOffTheRecord(),
is_unified_consent_enabled && is_unified_consent_given,
IsExtendedReportingEnabled(*prefs), IsScout(*prefs),
IsExtendedReportingPolicyManaged(*prefs), is_proceed_anyway_disabled,
true, // should_open_links_in_new_tab
......
......@@ -9,16 +9,27 @@
#include "chrome/browser/safe_browsing/safe_browsing_blocking_page.h"
#include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
#include "chrome/browser/safe_browsing/ui_manager.h"
#include "chrome/browser/signin/scoped_account_consistency.h"
#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/browser/signin/unified_consent_helper.h"
#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
#include "components/metrics/metrics_pref_names.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
#include "components/prefs/testing_pref_service.h"
#include "components/safe_browsing/browser/threat_details.h"
#include "components/safe_browsing/common/safe_browsing_prefs.h"
#include "components/security_interstitials/content/security_interstitial_controller_client.h"
#include "components/security_interstitials/core/safe_browsing_quiet_error_ui.h"
#include "components/signin/core/browser/signin_buildflags.h"
#include "components/signin/core/browser/signin_manager.h"
#include "components/strings/grit/components_strings.h"
#include "components/unified_consent/scoped_unified_consent.h"
#include "components/unified_consent/unified_consent_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/interstitial_page.h"
#include "content/public/browser/navigation_entry.h"
......@@ -98,17 +109,23 @@ class TestSafeBrowsingBlockingPageFactory
const GURL& main_frame_url,
const SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources)
override {
PrefService* prefs =
Profile::FromBrowserContext(web_contents->GetBrowserContext())
->GetPrefs();
Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
PrefService* prefs = profile->GetPrefs();
bool is_extended_reporting_opt_in_allowed =
prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
bool is_proceed_anyway_disabled =
prefs->GetBoolean(prefs::kSafeBrowsingProceedAnywayDisabled);
bool is_unified_consent_enabled = IsUnifiedConsentFeatureEnabled(profile);
unified_consent::UnifiedConsentService* consent_service =
UnifiedConsentServiceFactory::GetForProfile(profile);
bool is_unified_consent_given =
consent_service && consent_service->IsUnifiedConsentGiven();
BaseSafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
BaseBlockingPage::IsMainPageLoadBlocked(unsafe_resources),
is_extended_reporting_opt_in_allowed,
web_contents->GetBrowserContext()->IsOffTheRecord(),
is_unified_consent_enabled && is_unified_consent_given,
IsExtendedReportingEnabled(*prefs), IsScout(*prefs),
IsExtendedReportingPolicyManaged(*prefs), is_proceed_anyway_disabled,
true, // should_open_links_in_new_tab
......@@ -181,17 +198,23 @@ class TestSafeBrowsingBlockingQuietPageFactory
const GURL& main_frame_url,
const SafeBrowsingBlockingPage::UnsafeResourceList& unsafe_resources)
override {
PrefService* prefs =
Profile::FromBrowserContext(web_contents->GetBrowserContext())
->GetPrefs();
Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
PrefService* prefs = profile->GetPrefs();
bool is_extended_reporting_opt_in_allowed =
prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingOptInAllowed);
bool is_proceed_anyway_disabled =
prefs->GetBoolean(prefs::kSafeBrowsingProceedAnywayDisabled);
bool is_unified_consent_enabled = IsUnifiedConsentFeatureEnabled(profile);
unified_consent::UnifiedConsentService* consent_service =
UnifiedConsentServiceFactory::GetForProfile(profile);
bool is_unified_consent_given =
consent_service && consent_service->IsUnifiedConsentGiven();
BaseSafeBrowsingErrorUI::SBErrorDisplayOptions display_options(
BaseBlockingPage::IsMainPageLoadBlocked(unsafe_resources),
is_extended_reporting_opt_in_allowed,
web_contents->GetBrowserContext()->IsOffTheRecord(),
is_unified_consent_enabled && is_unified_consent_given,
IsExtendedReportingEnabled(*prefs), IsScout(*prefs),
IsExtendedReportingPolicyManaged(*prefs), is_proceed_anyway_disabled,
true, // should_open_links_in_new_tab
......@@ -372,6 +395,11 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness {
TestSafeBrowsingBlockingPageFactory factory_;
};
class SafeBrowsingBlockingPageTestDiceEnabled
: public SafeBrowsingBlockingPageTest {
private:
ScopedAccountConsistencyDice scoped_dice_;
};
// Tests showing a blocking page for a malware page and not proceeding.
TEST_F(SafeBrowsingBlockingPageTest, MalwarePageDontProceed) {
......@@ -1164,4 +1192,50 @@ TEST_F(SafeBrowsingBlockingQuietPageTest, GiantWebView) {
EXPECT_TRUE(is_giant);
}
// Test that extended reporting option is not shown if Unified Consent is
// enabled.
#if BUILDFLAG(ENABLE_DICE_SUPPORT)
TEST_F(SafeBrowsingBlockingPageTestDiceEnabled,
ExtendedReportingNotShownUnifiedConsent) {
#else
TEST_F(SafeBrowsingBlockingPageTest, ExtendedReportingNotShownUnifiedConsent) {
#endif
// Enable unified consent.
unified_consent::ScopedUnifiedConsent scoped_unified_consent(
unified_consent::UnifiedConsentFeatureState::kEnabledWithBump);
Profile* profile =
Profile::FromBrowserContext(web_contents()->GetBrowserContext());
// Fake sign in so unified consent can be given.
SigninManagerFactory::GetForProfile(profile)->SetAuthenticatedAccountInfo(
"gaia_id", "user");
TestingBrowserProcess::GetGlobal()->SetLocalState(profile->GetPrefs());
// Give unified consent.
UnifiedConsentServiceFactory::GetForProfile(profile)->SetUnifiedConsentGiven(
true);
// Start a load.
auto navigation = NavigationSimulator::CreateBrowserInitiated(GURL(kBadURL),
web_contents());
navigation->Start();
// Simulate the load causing a safe browsing interstitial to be shown.
ShowInterstitial(false, kBadURL);
SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
ASSERT_TRUE(sb_interstitial);
EXPECT_FALSE(
sb_interstitial->sb_error_ui()->CanShowExtendedReportingOption());
base::RunLoop().RunUntilIdle();
// Simulate the user clicking "don't proceed".
DontProceedThroughInterstitial(sb_interstitial);
// The interstitial should be gone.
EXPECT_EQ(CANCEL, user_response());
EXPECT_FALSE(GetSafeBrowsingBlockingPage());
TestingBrowserProcess::GetGlobal()->SetLocalState(nullptr);
}
} // namespace safe_browsing
......@@ -445,6 +445,7 @@ class TestSafeBrowsingBlockingPage : public SafeBrowsingBlockingPage {
BaseBlockingPage::IsMainPageLoadBlocked(unsafe_resources),
false, // is_extended_reporting_opt_in_allowed
false, // is_off_the_record
false, // is_unified_consent_enabled
false, // is_extended_reporting_enabled
false, // is_scout_reporting_enabled
false, // is_extended_reporting_policy_managed
......
......@@ -15,7 +15,9 @@
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/signin/unified_consent_helper.h"
#include "chrome/browser/ssl/ssl_cert_reporter.h"
#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
#include "chrome/common/channel_info.h"
#include "chrome/common/pref_names.h"
#include "components/prefs/pref_service.h"
......@@ -23,6 +25,7 @@
#include "components/security_interstitials/core/controller_client.h"
#include "components/security_interstitials/core/metrics_helper.h"
#include "components/strings/grit/components_strings.h"
#include "components/unified_consent/unified_consent_service.h"
#include "components/variations/variations_associated_data.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/web_contents.h"
......@@ -142,9 +145,6 @@ void CertReportHelper::HandleReportingCommands(
}
void CertReportHelper::FinishCertCollection() {
if (!ShouldShowCertificateReporterCheckbox())
return;
if (!safe_browsing::IsExtendedReportingEnabled(
*GetProfile(web_contents_)->GetPrefs()))
return;
......@@ -186,12 +186,14 @@ void CertReportHelper::FinishCertCollection() {
ssl_cert_reporter_->ReportInvalidCertificateChain(serialized_report);
}
bool CertReportHelper::ShouldShowCertificateReporterCheckbox() {
// Only show the checkbox iff the user is part of the respective Finch group
// and the window is not incognito and the feature is not disabled by policy.
bool CertReportHelper::IsShowingReportingCheckboxOrReportingAllowed() {
// Only show the checkbox or send reports iff the user is part of the
// respective Finch group and the window is not incognito and the feature is
// not disabled by policy.
const bool in_incognito =
web_contents_->GetBrowserContext()->IsOffTheRecord();
const PrefService* pref_service = GetProfile(web_contents_)->GetPrefs();
Profile* profile = GetProfile(web_contents_);
const PrefService* pref_service = profile->GetPrefs();
bool can_show_checkbox =
safe_browsing::IsExtendedReportingOptInAllowed(*pref_service) &&
!safe_browsing::IsExtendedReportingPolicyManaged(*pref_service);
......@@ -201,8 +203,22 @@ bool CertReportHelper::ShouldShowCertificateReporterCheckbox() {
!in_incognito && can_show_checkbox;
}
bool CertReportHelper::ShouldShowCertificateReporterCheckbox() {
if (!IsShowingReportingCheckboxOrReportingAllowed())
return false;
Profile* profile = GetProfile(web_contents_);
bool is_unified_consent_enabled = IsUnifiedConsentFeatureEnabled(profile);
unified_consent::UnifiedConsentService* consent_service =
UnifiedConsentServiceFactory::GetForProfile(profile);
bool is_unified_consent_given =
consent_service && consent_service->IsUnifiedConsentGiven();
return !(is_unified_consent_enabled && is_unified_consent_given);
}
bool CertReportHelper::ShouldReportCertificateError() {
DCHECK(ShouldShowCertificateReporterCheckbox());
if (!IsShowingReportingCheckboxOrReportingAllowed())
return false;
bool is_official_build = g_is_fake_official_build_for_cert_report_testing;
#if defined(OFFICIAL_BUILD) && defined(GOOGLE_CHROME_BUILD)
......
......@@ -79,12 +79,21 @@ class CertReportHelper {
void FinishCertCollection();
private:
// Returns false if we shouldn't show the checkbox nor send reports. Holds
// checks that are common to showing the checbox and reporting (Incognito
// mode, policy, and whether the user is in the correct Finch group).
bool IsShowingReportingCheckboxOrReportingAllowed();
// Checks whether a checkbox should be shown on the page that allows
// the user to opt in to Safe Browsing extended reporting.
// the user to opt in to Safe Browsing extended reporting. There are cases
// (e.g., when Unified Consent is enabled) where reports might be sent, but
// the checkbox will not be shown.
bool ShouldShowCertificateReporterCheckbox();
// Returns true if a certificate report should be sent for the SSL
// error for this page.
// error for this page. There are cases (e.g., certain variations
// configurations) where the checkbox will be shown but reports will not be
// sent.
bool ShouldReportCertificateError();
// Handles reports of invalid SSL certificates.
......
......@@ -48,6 +48,8 @@ ChromeUnifiedConsentServiceClient::GetServiceState(Service service) {
enabled = pref_service_->GetBoolean(prefs::kAlternateErrorPagesEnabled);
break;
case Service::kMetricsReporting:
if (!g_browser_process->metrics_service())
return ServiceState::kNotSupported;
// Uploads are disabled for non-official builds, but UnifiedConsentService
// only cares whether the user has manually disabled metrics reporting.
enabled = g_browser_process->local_state()->GetBoolean(
......
......@@ -77,6 +77,7 @@ BaseBlockingPage::CreateDefaultDisplayOptions(
IsMainPageLoadBlocked(unsafe_resources),
false, // kSafeBrowsingExtendedReportingOptInAllowed
false, // is_off_the_record
false, // is_unified_consent_enabled
false, // is_extended_reporting
false, // is_scout
false, // is_sber_policy_managed
......
......@@ -102,6 +102,7 @@ SBErrorOptions TriggerManager::GetSBErrorDisplayOptions(
return SBErrorOptions(/*is_main_frame_load_blocked=*/false,
IsExtendedReportingOptInAllowed(pref_service),
web_contents.GetBrowserContext()->IsOffTheRecord(),
/*is_unified_consent_enabled=*/false,
IsExtendedReportingEnabled(pref_service),
IsScout(pref_service),
IsExtendedReportingPolicyManaged(pref_service),
......
......@@ -28,6 +28,7 @@ BaseSafeBrowsingErrorUI::SBErrorDisplayOptions::SBErrorDisplayOptions(
bool is_main_frame_load_blocked,
bool is_extended_reporting_opt_in_allowed,
bool is_off_the_record,
bool is_unified_consent_enabled,
bool is_extended_reporting_enabled,
bool is_scout_reporting_enabled,
bool is_extended_reporting_policy_managed,
......@@ -39,6 +40,7 @@ BaseSafeBrowsingErrorUI::SBErrorDisplayOptions::SBErrorDisplayOptions(
is_extended_reporting_opt_in_allowed(
is_extended_reporting_opt_in_allowed),
is_off_the_record(is_off_the_record),
is_unified_consent_enabled(is_unified_consent_enabled),
is_extended_reporting_enabled(is_extended_reporting_enabled),
is_scout_reporting_enabled(is_scout_reporting_enabled),
is_extended_reporting_policy_managed(
......@@ -54,6 +56,7 @@ BaseSafeBrowsingErrorUI::SBErrorDisplayOptions::SBErrorDisplayOptions(
is_extended_reporting_opt_in_allowed(
other.is_extended_reporting_opt_in_allowed),
is_off_the_record(other.is_off_the_record),
is_unified_consent_enabled(other.is_unified_consent_enabled),
is_extended_reporting_enabled(other.is_extended_reporting_enabled),
is_scout_reporting_enabled(other.is_scout_reporting_enabled),
is_extended_reporting_policy_managed(
......
......@@ -30,6 +30,7 @@ class BaseSafeBrowsingErrorUI {
SBErrorDisplayOptions(bool is_main_frame_load_blocked,
bool is_extended_reporting_opt_in_allowed,
bool is_off_the_record,
bool is_unified_consent_enabled,
bool is_extended_reporting_enabled,
bool is_scout_reporting_enabled,
bool is_extended_reporting_policy_managed,
......@@ -49,6 +50,10 @@ class BaseSafeBrowsingErrorUI {
// Indicates if user is in incognito mode.
bool is_off_the_record;
// Indicates if user is enrolled in unified consent, if so, checkbox is not
// shown in SB interstitials.
bool is_unified_consent_enabled;
// Indicates if user opted in for SB extended reporting. This contains only
// the value of the pref that is currently active for the user (either the
// legacy SBER pref, or the Scout pref). Use |is_scout_reporting_enabled| to
......@@ -102,6 +107,10 @@ class BaseSafeBrowsingErrorUI {
bool is_off_the_record() const { return display_options_.is_off_the_record; }
bool is_unified_consent_enabled() const {
return display_options_.is_unified_consent_enabled;
}
bool is_extended_reporting_enabled() const {
return display_options_.is_extended_reporting_enabled;
}
......@@ -145,7 +154,8 @@ class BaseSafeBrowsingErrorUI {
// - if kSafeBrowsingExtendedReporting is managed by enterprise policy.
bool CanShowExtendedReportingOption() {
return !is_off_the_record() && is_extended_reporting_opt_in_allowed() &&
!is_extended_reporting_policy_managed();
!is_extended_reporting_policy_managed() &&
!is_unified_consent_enabled();
}
SBInterstitialReason interstitial_reason() const {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment