diff options
author | felt <felt@chromium.org> | 2015-07-29 09:00:00 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-07-29 16:00:39 +0000 |
commit | aafecea4940c28a2bc7af3d94f51a0c068582cd2 (patch) | |
tree | 6b715e36ab73aa839a409e6044f690eca99872d8 | |
parent | 56f81e32556d513af513eac0710f2e85877ffa3b (diff) | |
download | chromium_src-aafecea4940c28a2bc7af3d94f51a0c068582cd2.zip chromium_src-aafecea4940c28a2bc7af3d94f51a0c068582cd2.tar.gz chromium_src-aafecea4940c28a2bc7af3d94f51a0c068582cd2.tar.bz2 |
Move the security interstitial metrics helper into a new component
(1) Create components/security_interstitials/
(2) Move the non-Chrome-specific portions of SecurityInterstitialMetricsHelper
into the component, as security_interstitials::MetricsHelper
(3) Keep the Chrome-specific parts in ChromeSecurityInterstitialMetricsHelper
as a subclass
(4) Add browser tests to check that UMA histograms are still being recorded
BUG=488673,454945
Review URL: https://codereview.chromium.org/1256753002
Cr-Commit-Position: refs/heads/master@{#340894}
24 files changed, 685 insertions, 364 deletions
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn index 1343cb6..76d9702 100644 --- a/chrome/browser/BUILD.gn +++ b/chrome/browser/BUILD.gn @@ -137,6 +137,7 @@ source_set("browser") { "//components/search", "//components/search_engines", "//components/search_provider_logos", + "//components/security_interstitials", "//components/signin/core/browser", "//components/startup_metric_utils", "//components/strings", diff --git a/chrome/browser/DEPS b/chrome/browser/DEPS index 0889438..476f564 100644 --- a/chrome/browser/DEPS +++ b/chrome/browser/DEPS @@ -78,6 +78,7 @@ include_rules = [ "+components/search", "+components/search_engines", "+components/search_provider_logos", + "+components/security_interstitials", "+components/service_tab_launcher", "+components/session_manager", "+components/sessions", diff --git a/chrome/browser/interstitials/chrome_metrics_helper.cc b/chrome/browser/interstitials/chrome_metrics_helper.cc new file mode 100644 index 0000000..c886e44 --- /dev/null +++ b/chrome/browser/interstitials/chrome_metrics_helper.cc @@ -0,0 +1,93 @@ +// 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/interstitials/chrome_metrics_helper.h" + +#include "chrome/browser/browser_process.h" +#include "chrome/browser/history/history_service_factory.h" +#include "chrome/browser/profiles/profile.h" +#include "components/history/core/browser/history_service.h" +#include "components/rappor/rappor_service.h" +#include "content/public/browser/web_contents.h" + +#if defined(ENABLE_EXTENSIONS) +#include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h" +#endif + +ChromeMetricsHelper::ChromeMetricsHelper( + content::WebContents* web_contents, + const GURL& request_url, + const security_interstitials::MetricsHelper::ReportDetails settings, + const std::string& sampling_event_name) + : security_interstitials::MetricsHelper( + request_url, + settings, + HistoryServiceFactory::GetForProfile( + Profile::FromBrowserContext(web_contents->GetBrowserContext()), + ServiceAccessType::EXPLICIT_ACCESS), + g_browser_process->rappor_service()), + web_contents_(web_contents), + request_url_(request_url), + sampling_event_name_(sampling_event_name) { + DCHECK(!sampling_event_name_.empty()); +} + +ChromeMetricsHelper::~ChromeMetricsHelper() {} + +void ChromeMetricsHelper::RecordExtraUserDecisionMetrics( + security_interstitials::MetricsHelper::Decision decision) { +#if defined(ENABLE_EXTENSIONS) + if (!sampling_event_.get()) { + sampling_event_.reset(new extensions::ExperienceSamplingEvent( + sampling_event_name_, request_url_, + web_contents_->GetLastCommittedURL(), + web_contents_->GetBrowserContext())); + } + switch (decision) { + case PROCEED: + sampling_event_->CreateUserDecisionEvent( + extensions::ExperienceSamplingEvent::kProceed); + break; + case DONT_PROCEED: + sampling_event_->CreateUserDecisionEvent( + extensions::ExperienceSamplingEvent::kDeny); + break; + case SHOW: + case PROCEEDING_DISABLED: + case MAX_DECISION: + break; + } +#endif +} + +void ChromeMetricsHelper::RecordExtraUserInteractionMetrics( + security_interstitials::MetricsHelper::Interaction interaction) { +#if defined(ENABLE_EXTENSIONS) + if (!sampling_event_.get()) { + sampling_event_.reset(new extensions::ExperienceSamplingEvent( + sampling_event_name_, request_url_, + web_contents_->GetLastCommittedURL(), + web_contents_->GetBrowserContext())); + } + switch (interaction) { + case SHOW_LEARN_MORE: + sampling_event_->set_has_viewed_learn_more(true); + break; + case SHOW_ADVANCED: + sampling_event_->set_has_viewed_details(true); + break; + case SHOW_PRIVACY_POLICY: + case SHOW_DIAGNOSTIC: + case RELOAD: + case OPEN_TIME_SETTINGS: + case TOTAL_VISITS: + case SET_EXTENDED_REPORTING_ENABLED: + case SET_EXTENDED_REPORTING_DISABLED: + case EXTENDED_REPORTING_IS_ENABLED: + case REPORT_PHISHING_ERROR: + case MAX_INTERACTION: + break; + } +#endif +} diff --git a/chrome/browser/interstitials/chrome_metrics_helper.h b/chrome/browser/interstitials/chrome_metrics_helper.h new file mode 100644 index 0000000..d49ea4f --- /dev/null +++ b/chrome/browser/interstitials/chrome_metrics_helper.h @@ -0,0 +1,51 @@ +// 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_INTERSTITIALS_CHROME_METRICS_HELPER_H_ +#define CHROME_BROWSER_INTERSTITIALS_CHROME_METRICS_HELPER_H_ + +#include <string> + +#include "components/security_interstitials/metrics_helper.h" +#include "url/gurl.h" + +namespace content { +class WebContents; +} + +namespace extensions { +class ExperienceSamplingEvent; +} + +// This class adds desktop-Chrome-specific metrics (extension experience +// sampling) to the security_interstitials::MetricsHelper. Together, they +// record UMA, Rappor, and experience sampling metrics. +class ChromeMetricsHelper : public security_interstitials::MetricsHelper { + public: + ChromeMetricsHelper( + content::WebContents* web_contents, + const GURL& url, + const security_interstitials::MetricsHelper::ReportDetails settings, + const std::string& sampling_event_name); + ~ChromeMetricsHelper() override; + + protected: + // security_interstitials::MetricsHelper methods: + void RecordExtraUserDecisionMetrics( + security_interstitials::MetricsHelper::Decision decision) override; + void RecordExtraUserInteractionMetrics( + security_interstitials::MetricsHelper::Interaction interaction) override; + + private: + content::WebContents* web_contents_; + const GURL request_url_; + const std::string sampling_event_name_; +#if defined(ENABLE_EXTENSIONS) + scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_; +#endif + + DISALLOW_COPY_AND_ASSIGN(ChromeMetricsHelper); +}; + +#endif // CHROME_BROWSER_INTERSTITIALS_CHROME_METRICS_HELPER_H_ diff --git a/chrome/browser/interstitials/security_interstitial_metrics_helper.cc b/chrome/browser/interstitials/security_interstitial_metrics_helper.cc deleted file mode 100644 index 1c5bd85..0000000 --- a/chrome/browser/interstitials/security_interstitial_metrics_helper.cc +++ /dev/null @@ -1,197 +0,0 @@ -// Copyright (c) 2014 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/interstitials/security_interstitial_metrics_helper.h" - -#include <string> - -#include "base/metrics/histogram.h" -#include "chrome/browser/browser_process.h" -#include "chrome/browser/history/history_service_factory.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/web_data_service_factory.h" -#include "components/history/core/browser/history_service.h" -#include "components/rappor/rappor_service.h" -#include "components/rappor/rappor_utils.h" -#include "content/public/browser/web_contents.h" -#include "net/base/registry_controlled_domains/registry_controlled_domain.h" - -#if defined(ENABLE_EXTENSIONS) -#include "chrome/browser/extensions/api/experience_sampling_private/experience_sampling.h" -#endif - -namespace { -// Used for setting bits in Rappor's "interstitial.*.flags" -enum InterstitialFlagBits { - DID_PROCEED = 0, - IS_REPEAT_VISIT = 1, - HIGHEST_USED_BIT = 1 -}; -} // namespace - -SecurityInterstitialMetricsHelper::SecurityInterstitialMetricsHelper( - content::WebContents* web_contents, - const GURL& request_url, - const std::string& uma_prefix, - const std::string& rappor_prefix, - RapporReporting rappor_reporting, - const std::string& sampling_event_name) - : web_contents_(web_contents), - request_url_(request_url), - uma_prefix_(uma_prefix), - rappor_prefix_(rappor_prefix), - rappor_reporting_(rappor_reporting), - sampling_event_name_(sampling_event_name), - num_visits_(-1) { - DCHECK(!uma_prefix_.empty()); - DCHECK(!rappor_prefix_.empty()); - DCHECK(!sampling_event_name_.empty()); - history::HistoryService* history_service = - HistoryServiceFactory::GetForProfile( - Profile::FromBrowserContext(web_contents->GetBrowserContext()), - ServiceAccessType::EXPLICIT_ACCESS); - if (history_service) { - history_service->GetVisibleVisitCountToHost( - request_url_, - base::Bind(&SecurityInterstitialMetricsHelper::OnGotHistoryCount, - base::Unretained(this)), - &request_tracker_); - } -} - -SecurityInterstitialMetricsHelper::~SecurityInterstitialMetricsHelper() { -} - -// Directly adds to the UMA histograms, using the same properties as -// UMA_HISTOGRAM_ENUMERATION, because the macro doesn't allow non-constant -// histogram names. Reports to Rappor for certain decisions. -void SecurityInterstitialMetricsHelper::RecordUserDecision( - SecurityInterstitialDecision decision) { - // UMA - std::string decision_histogram_name( - "interstitial." + uma_prefix_ + ".decision"); - base::HistogramBase* decision_histogram = base::LinearHistogram::FactoryGet( - decision_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1, - base::HistogramBase::kUmaTargetedHistogramFlag); - decision_histogram->Add(decision); - - // Rappor - rappor::RapporService* rappor_service = g_browser_process->rappor_service(); - if (rappor_service && - (rappor_reporting_ == REPORT_RAPPOR || - rappor_reporting_ == REPORT_RAPPOR_FOR_SAFE_BROWSING) && - (decision == PROCEED || decision == DONT_PROCEED)) { - const rappor::RapporType rappor_type = - rappor_reporting_ == REPORT_RAPPOR_FOR_SAFE_BROWSING - ? rappor::SAFEBROWSING_RAPPOR_TYPE - : rappor::UMA_RAPPOR_TYPE; - - scoped_ptr<rappor::Sample> sample = - rappor_service->CreateSample(rappor_type); - - // This will populate, for example, "intersitial.malware.domain" or - // "interstitial.ssl2.domain". |domain| will be empty for hosts w/o TLDs. - const std::string domain = - rappor::GetDomainAndRegistrySampleFromGURL(request_url_); - sample->SetStringField("domain", domain); - - // Only report history and decision if we have history data. - if (num_visits_ >= 0) { - int flags = 0; - if (decision == PROCEED) - flags |= 1 << InterstitialFlagBits::DID_PROCEED; - if (num_visits_ > 0) - flags |= 1 << InterstitialFlagBits::IS_REPEAT_VISIT; - // e.g. "interstitial.malware.flags" - sample->SetFlagsField("flags", flags, - InterstitialFlagBits::HIGHEST_USED_BIT + 1); - } - rappor_service->RecordSampleObj("interstitial." + rappor_prefix_, - sample.Pass()); - } - -#if defined(ENABLE_EXTENSIONS) - if (!sampling_event_.get()) { - sampling_event_.reset(new extensions::ExperienceSamplingEvent( - sampling_event_name_, - request_url_, - web_contents_->GetLastCommittedURL(), - web_contents_->GetBrowserContext())); - } - switch (decision) { - case PROCEED: - sampling_event_->CreateUserDecisionEvent( - extensions::ExperienceSamplingEvent::kProceed); - break; - case DONT_PROCEED: - sampling_event_->CreateUserDecisionEvent( - extensions::ExperienceSamplingEvent::kDeny); - break; - case SHOW: - case PROCEEDING_DISABLED: - case MAX_DECISION: - break; - } -#endif - - // Record additional information about sites that users have - // visited before. - if (num_visits_ < 1 || (decision != PROCEED && decision != DONT_PROCEED)) - return; - std::string history_histogram_name( - "interstitial." + uma_prefix_ + ".decision.repeat_visit"); - base::HistogramBase* history_histogram = base::LinearHistogram::FactoryGet( - history_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1, - base::HistogramBase::kUmaTargetedHistogramFlag); - history_histogram->Add(SHOW); - history_histogram->Add(decision); -} - -void SecurityInterstitialMetricsHelper::RecordUserInteraction( - SecurityInterstitialInteraction interaction) { - std::string interaction_histogram_name( - "interstitial." + uma_prefix_ + ".interaction"); - base::HistogramBase* interaction_histogram = - base::LinearHistogram::FactoryGet( - interaction_histogram_name, 1, MAX_INTERACTION, MAX_INTERACTION + 1, - base::HistogramBase::kUmaTargetedHistogramFlag); - interaction_histogram->Add(interaction); - -#if defined(ENABLE_EXTENSIONS) - if (!sampling_event_.get()) { - sampling_event_.reset(new extensions::ExperienceSamplingEvent( - sampling_event_name_, - request_url_, - web_contents_->GetLastCommittedURL(), - web_contents_->GetBrowserContext())); - } - switch (interaction) { - case SHOW_LEARN_MORE: - sampling_event_->set_has_viewed_learn_more(true); - break; - case SHOW_ADVANCED: - sampling_event_->set_has_viewed_details(true); - break; - case SHOW_PRIVACY_POLICY: - case SHOW_DIAGNOSTIC: - case RELOAD: - case OPEN_TIME_SETTINGS: - case TOTAL_VISITS: - case SET_EXTENDED_REPORTING_ENABLED: - case SET_EXTENDED_REPORTING_DISABLED: - case EXTENDED_REPORTING_IS_ENABLED: - case REPORT_PHISHING_ERROR: - case MAX_INTERACTION: - break; - } -#endif -} - -void SecurityInterstitialMetricsHelper::OnGotHistoryCount( - bool success, - int num_visits, - base::Time first_visit) { - if (success) - num_visits_ = num_visits; -} diff --git a/chrome/browser/interstitials/security_interstitial_metrics_helper.h b/chrome/browser/interstitials/security_interstitial_metrics_helper.h deleted file mode 100644 index ffbe9d1..0000000 --- a/chrome/browser/interstitials/security_interstitial_metrics_helper.h +++ /dev/null @@ -1,100 +0,0 @@ -// Copyright (c) 2014 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_INTERSTITIALS_SECURITY_INTERSTITIAL_METRICS_HELPER_H_ -#define CHROME_BROWSER_INTERSTITIALS_SECURITY_INTERSTITIAL_METRICS_HELPER_H_ - -#include <string> - -#include "base/task/cancelable_task_tracker.h" -#include "base/time/time.h" -#include "url/gurl.h" - -namespace content { -class WebContents; -} - -namespace extensions { -class ExperienceSamplingEvent; -} - -// Most of the security interstitials share a common layout and set of -// choices. SecurityInterstitialMetricsHelper is intended to help the security -// interstitials record user choices in a common way via METRICS histograms -// and RAPPOR metrics. -class SecurityInterstitialMetricsHelper { - public: - // These enums are used for histograms. Don't reorder, delete, or insert - // elements. New elements should be added at the end (right before the max). - enum SecurityInterstitialDecision { - SHOW, - PROCEED, - DONT_PROCEED, - PROCEEDING_DISABLED, - MAX_DECISION - }; - enum SecurityInterstitialInteraction { - TOTAL_VISITS, - SHOW_ADVANCED, - SHOW_PRIVACY_POLICY, - SHOW_DIAGNOSTIC, - SHOW_LEARN_MORE, - RELOAD, - OPEN_TIME_SETTINGS, - SET_EXTENDED_REPORTING_ENABLED, - SET_EXTENDED_REPORTING_DISABLED, - EXTENDED_REPORTING_IS_ENABLED, - REPORT_PHISHING_ERROR, - MAX_INTERACTION - }; - - enum RapporReporting { - REPORT_RAPPOR, - // Only use this for safe-browsing interstitials. - REPORT_RAPPOR_FOR_SAFE_BROWSING, - SKIP_RAPPOR, - }; - - // Args: - // url: URL of page that triggered the interstitial. Only origin is used. - // uma_prefix: Histogram prefix for UMA. - // examples: "phishing", "ssl_overridable" - // rappor_prefix: Metric prefix for Rappor. - // examples: "phishing", "ssl2" - // rappor_reporting: Used to skip rappor rapporting if desired. - // sampling_event_name: Event name for Experience Sampling. - // e.g. "phishing_interstitial_" - SecurityInterstitialMetricsHelper(content::WebContents* web_contents, - const GURL& url, - const std::string& uma_prefix, - const std::string& rappor_prefix, - RapporReporting rappor_reporting, - const std::string& sampling_event_name); - ~SecurityInterstitialMetricsHelper(); - - // Record a user decision or interaction to the appropriate UMA histogram - // and potentially in a RAPPOR metric. - void RecordUserDecision(SecurityInterstitialDecision decision); - void RecordUserInteraction(SecurityInterstitialInteraction interaction); - - private: - // Used to query the HistoryService to see if the URL is in history. - void OnGotHistoryCount(bool success, int num_visits, base::Time first_visit); - - content::WebContents* web_contents_; - const GURL request_url_; - const std::string uma_prefix_; - const std::string rappor_prefix_; - const RapporReporting rappor_reporting_; - const std::string sampling_event_name_; - int num_visits_; - base::CancelableTaskTracker request_tracker_; -#if defined(ENABLE_EXTENSIONS) - scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_; -#endif - - DISALLOW_COPY_AND_ASSIGN(SecurityInterstitialMetricsHelper); -}; - -#endif // CHROME_BROWSER_INTERSTITIALS_SECURITY_INTERSTITIAL_METRICS_HELPER_H_ diff --git a/chrome/browser/interstitials/security_interstitial_page.cc b/chrome/browser/interstitials/security_interstitial_page.cc index 3551d12..9b5bc42 100644 --- a/chrome/browser/interstitials/security_interstitial_page.cc +++ b/chrome/browser/interstitials/security_interstitial_page.cc @@ -11,13 +11,13 @@ #include "base/strings/utf_string_conversions.h" #include "base/values.h" #include "chrome/browser/browser_process.h" -#include "chrome/browser/interstitials/security_interstitial_metrics_helper.h" #include "chrome/browser/net/referrer.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/pref_names.h" #include "chrome/grit/browser_resources.h" #include "chrome/grit/generated_resources.h" #include "components/google/core/browser/google_util.h" +#include "components/security_interstitials/metrics_helper.h" #include "content/public/browser/interstitial_page.h" #include "content/public/browser/page_navigator.h" #include "content/public/browser/web_contents.h" @@ -84,9 +84,10 @@ void SecurityInterstitialPage::SetReportingPreference(bool report) { PrefService* pref = profile->GetPrefs(); pref->SetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled, report); metrics_helper()->RecordUserInteraction( - report - ? SecurityInterstitialMetricsHelper::SET_EXTENDED_REPORTING_ENABLED - : SecurityInterstitialMetricsHelper::SET_EXTENDED_REPORTING_DISABLED); + report ? security_interstitials::MetricsHelper:: + SET_EXTENDED_REPORTING_ENABLED + : security_interstitials::MetricsHelper:: + SET_EXTENDED_REPORTING_DISABLED); } bool SecurityInterstitialPage::IsPrefEnabled(const char* pref) { @@ -97,7 +98,7 @@ bool SecurityInterstitialPage::IsPrefEnabled(const char* pref) { void SecurityInterstitialPage::OpenExtendedReportingPrivacyPolicy() { metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_PRIVACY_POLICY); + security_interstitials::MetricsHelper::SHOW_PRIVACY_POLICY); GURL privacy_url( l10n_util::GetStringUTF8(IDS_SAFE_BROWSING_PRIVACY_POLICY_URL)); privacy_url = google_util::AppendGoogleLocaleParam( @@ -107,12 +108,13 @@ void SecurityInterstitialPage::OpenExtendedReportingPrivacyPolicy() { web_contents()->OpenURL(params); } -SecurityInterstitialMetricsHelper* SecurityInterstitialPage::metrics_helper() { +security_interstitials::MetricsHelper* +SecurityInterstitialPage::metrics_helper() const { return metrics_helper_.get(); } void SecurityInterstitialPage::set_metrics_helper( - SecurityInterstitialMetricsHelper* metrics_helper) { + security_interstitials::MetricsHelper* metrics_helper) { metrics_helper_.reset(metrics_helper); } diff --git a/chrome/browser/interstitials/security_interstitial_page.h b/chrome/browser/interstitials/security_interstitial_page.h index 6a07dfa..0bd632b 100644 --- a/chrome/browser/interstitials/security_interstitial_page.h +++ b/chrome/browser/interstitials/security_interstitial_page.h @@ -26,7 +26,9 @@ extern const char kOptInLink[]; extern const char kPrivacyLinkHtml[]; } -class SecurityInterstitialMetricsHelper; +namespace security_interstitials { +class MetricsHelper; +} class SecurityInterstitialPage : public content::InterstitialPageDelegate { public: @@ -95,11 +97,12 @@ class SecurityInterstitialPage : public content::InterstitialPageDelegate { void OpenExtendedReportingPrivacyPolicy(); - SecurityInterstitialMetricsHelper* metrics_helper(); - void set_metrics_helper(SecurityInterstitialMetricsHelper* metrics_helper); + security_interstitials::MetricsHelper* metrics_helper() const; + void set_metrics_helper( + security_interstitials::MetricsHelper* metrics_helper); private: - scoped_ptr<SecurityInterstitialMetricsHelper> metrics_helper_; + scoped_ptr<security_interstitials::MetricsHelper> metrics_helper_; // The WebContents with which this interstitial page is // associated. Not available in ~SecurityInterstitialPage, since it // can be destroyed before this class is destroyed. diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc index 90d5004..40f0e82 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc @@ -167,17 +167,19 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( interstitial_reason_ = SB_REASON_PHISHING; // This must be done after calculating |interstitial_reason_| above. - // Use same prefix for UMA as for Rappor. - set_metrics_helper(new SecurityInterstitialMetricsHelper( - web_contents, request_url(), GetMetricPrefix(), GetRapporPrefix(), - SecurityInterstitialMetricsHelper::REPORT_RAPPOR_FOR_SAFE_BROWSING, - GetSamplingEventName())); - metrics_helper()->RecordUserDecision(SecurityInterstitialMetricsHelper::SHOW); + security_interstitials::MetricsHelper::ReportDetails reporting_info; + reporting_info.metric_prefix = GetMetricPrefix(); + reporting_info.rappor_prefix = GetRapporPrefix(); + reporting_info.rappor_report_type = rappor::SAFEBROWSING_RAPPOR_TYPE; + set_metrics_helper(new ChromeMetricsHelper( + web_contents, request_url(), reporting_info, GetSamplingEventName())); + metrics_helper()->RecordUserDecision( + security_interstitials::MetricsHelper::SHOW); metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::TOTAL_VISITS); + security_interstitials::MetricsHelper::TOTAL_VISITS); if (IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) { metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::PROCEEDING_DISABLED); + security_interstitials::MetricsHelper::PROCEEDING_DISABLED); } if (!is_main_frame_load_blocked_) { @@ -234,7 +236,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) { case CMD_OPEN_HELP_CENTER: { // User pressed "Learn more". metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_LEARN_MORE); + security_interstitials::MetricsHelper::SHOW_LEARN_MORE); GURL learn_more_url( interstitial_reason_ == SB_REASON_PHISHING ? kLearnMorePhishingUrlV2 : kLearnMoreMalwareUrlV2); @@ -257,7 +259,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) { // User pressed on the button to proceed. if (!IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) { metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::PROCEED); + security_interstitials::MetricsHelper::PROCEED); interstitial_page()->Proceed(); // |this| has been deleted after Proceed() returns. break; @@ -294,7 +296,7 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) { const UnsafeResource& unsafe_resource = unsafe_resources_[0]; std::string bad_url_spec = unsafe_resource.url.spec(); metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_DIAGNOSTIC); + security_interstitials::MetricsHelper::SHOW_DIAGNOSTIC); std::string diagnostic = base::StringPrintf(kSbDiagnosticUrl, net::EscapeQueryParamValue(bad_url_spec, true).c_str()); @@ -314,13 +316,13 @@ void SafeBrowsingBlockingPage::CommandReceived(const std::string& page_cmd) { case CMD_SHOW_MORE_SECTION: { // User has opened up the hidden text. metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_ADVANCED); + security_interstitials::MetricsHelper::SHOW_ADVANCED); break; } case CMD_REPORT_PHISHING_ERROR: { // User wants to report a phishing error. metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::REPORT_PHISHING_ERROR); + security_interstitials::MetricsHelper::REPORT_PHISHING_ERROR); GURL phishing_error_url(kReportPhishingErrorUrl); phishing_error_url = google_util::AppendGoogleLocaleParam( phishing_error_url, g_browser_process->GetApplicationLocale()); @@ -384,7 +386,7 @@ void SafeBrowsingBlockingPage::OnDontProceed() { if (!IsPrefEnabled(prefs::kSafeBrowsingProceedAnywayDisabled)) { metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::DONT_PROCEED); + security_interstitials::MetricsHelper::DONT_PROCEED); } // Send the malware details, if we opted to. @@ -429,7 +431,7 @@ void SafeBrowsingBlockingPage::FinishMalwareDetails(int64 delay_ms) { return; metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED); + security_interstitials::MetricsHelper::EXTENDED_REPORTING_IS_ENABLED); // Finish the malware details collection, send it over. BrowserThread::PostDelayedTask( BrowserThread::IO, FROM_HERE, diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.h b/chrome/browser/safe_browsing/safe_browsing_blocking_page.h index 80e0642..30af59a 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.h +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.h @@ -34,7 +34,7 @@ #include "base/gtest_prod_util.h" #include "base/task/cancelable_task_tracker.h" -#include "chrome/browser/interstitials/security_interstitial_metrics_helper.h" +#include "chrome/browser/interstitials/chrome_metrics_helper.h" #include "chrome/browser/interstitials/security_interstitial_page.h" #include "chrome/browser/safe_browsing/ui_manager.h" #include "content/public/browser/interstitial_page_delegate.h" diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc index 19b1715..965f7ca 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc @@ -11,6 +11,7 @@ #include "base/prefs/pref_service.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/histogram_tester.h" #include "base/values.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h" @@ -33,6 +34,7 @@ #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/test_switches.h" #include "chrome/test/base/ui_test_utils.h" +#include "components/security_interstitials/metrics_helper.h" #include "content/public/browser/interstitial_page.h" #include "content/public/browser/navigation_controller.h" #include "content/public/browser/notification_types.h" @@ -613,8 +615,8 @@ class SafeBrowsingBlockingPageBrowserTest DISALLOW_COPY_AND_ASSIGN(SafeBrowsingBlockingPageBrowserTest); }; -// TODO(linux_aura) http://crbug.com/163931 -// TODO(win_aura) http://crbug.com/154081 +// TODO(linux_aura) https://crbug.com/163931 +// TODO(win_aura) https://crbug.com/154081 #if defined(USE_AURA) && !defined(OS_CHROMEOS) #define MAYBE_RedirectInIFrameCanceled DISABLED_RedirectInIFrameCanceled #else @@ -639,10 +641,11 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, RedirectCanceled) { IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, DontProceed) { #if defined(OS_WIN) && defined(USE_ASH) - // Disable this test in Metro+Ash for now (http://crbug.com/262796). + // Disable this test in Metro+Ash for now (https://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kAshBrowserTests)) + switches::kAshBrowserTests)) { return; + } #endif SetupWarningAndNavigate(); @@ -673,10 +676,11 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, Proceed) { IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, IframeDontProceed) { #if defined(OS_WIN) && defined(USE_ASH) - // Disable this test in Metro+Ash for now (http://crbug.com/262796). + // Disable this test in Metro+Ash for now (https://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kAshBrowserTests)) + switches::kAshBrowserTests)) { return; + } #endif SetupThreatIframeWarningAndNavigate(); @@ -754,10 +758,11 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, // command anyway doesn't advance to the malware site. IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ProceedDisabled) { #if defined(OS_WIN) && defined(USE_ASH) - // Disable this test in Metro+Ash for now (http://crbug.com/262796). + // Disable this test in Metro+Ash for now (https://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kAshBrowserTests)) + switches::kAshBrowserTests)) { return; + } #endif // Simulate a policy disabling the "proceed anyway" link. @@ -786,10 +791,11 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ProceedDisabled) { // good way to do that in the current design. IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ReportingDisabled) { #if defined(OS_WIN) && defined(USE_ASH) - // Disable this test in Metro+Ash for now (http://crbug.com/262796). + // Disable this test in Metro+Ash for now (https://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kAshBrowserTests)) + switches::kAshBrowserTests)) { return; + } #endif browser()->profile()->GetPrefs()->SetBoolean( @@ -804,10 +810,11 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ReportingDisabled) { IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ReportingDisabledByPolicy) { #if defined(OS_WIN) && defined(USE_ASH) - // Disable this test in Metro+Ash for now (http://crbug.com/262796). + // Disable this test in Metro+Ash for now (https://crbug.com/262796). if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kAshBrowserTests)) + switches::kAshBrowserTests)) { return; + } #endif browser()->profile()->GetPrefs()->SetBoolean( @@ -832,6 +839,99 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, LearnMore) { browser()->tab_strip_model()->GetActiveWebContents()->GetURL().path()); } +IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, + Histograms_DontProceed) { +#if defined(OS_WIN) && defined(USE_ASH) + // Disable this test in Metro+Ash for now (https://crbug.com/262796). + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kAshBrowserTests)) { + return; + } +#endif + + base::HistogramTester histograms; + std::string prefix; + if (GetParam() == SB_THREAT_TYPE_URL_MALWARE) + prefix = "malware"; + else if (GetParam() == SB_THREAT_TYPE_URL_PHISHING) + prefix = "phishing"; + else if (GetParam() == SB_THREAT_TYPE_URL_UNWANTED) + prefix = "harmful"; + else + NOTREACHED(); + const std::string decision_histogram = "interstitial." + prefix + ".decision"; + const std::string interaction_histogram = + "interstitial." + prefix + ".interaction"; + + // Histograms should start off empty. + histograms.ExpectTotalCount(decision_histogram, 0); + histograms.ExpectTotalCount(interaction_histogram, 0); + + // After navigating to the page, the totals should be set. + SetupWarningAndNavigate(); + histograms.ExpectTotalCount(decision_histogram, 1); + histograms.ExpectBucketCount(decision_histogram, + security_interstitials::MetricsHelper::SHOW, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); + + // Decision should be recorded. + EXPECT_TRUE(ClickAndWaitForDetach("primary-button")); + AssertNoInterstitial(false); // Assert the interstitial is gone + histograms.ExpectTotalCount(decision_histogram, 2); + histograms.ExpectBucketCount( + decision_histogram, security_interstitials::MetricsHelper::DONT_PROCEED, + 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); +} + +IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, + Histograms_Proceed) { + base::HistogramTester histograms; + std::string prefix; + if (GetParam() == SB_THREAT_TYPE_URL_MALWARE) + prefix = "malware"; + else if (GetParam() == SB_THREAT_TYPE_URL_PHISHING) + prefix = "phishing"; + else if (GetParam() == SB_THREAT_TYPE_URL_UNWANTED) + prefix = "harmful"; + else + NOTREACHED(); + const std::string decision_histogram = "interstitial." + prefix + ".decision"; + const std::string interaction_histogram = + "interstitial." + prefix + ".interaction"; + + // Histograms should start off empty. + histograms.ExpectTotalCount(decision_histogram, 0); + histograms.ExpectTotalCount(interaction_histogram, 0); + + // After navigating to the page, the totals should be set. + GURL url = SetupWarningAndNavigate(); + histograms.ExpectTotalCount(decision_histogram, 1); + histograms.ExpectBucketCount(decision_histogram, + security_interstitials::MetricsHelper::SHOW, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); + + // Decision should be recorded. + EXPECT_TRUE(ClickAndWaitForDetach("proceed-link")); + AssertNoInterstitial(true); // Assert the interstitial is gone. + histograms.ExpectTotalCount(decision_histogram, 2); + histograms.ExpectBucketCount( + decision_histogram, security_interstitials::MetricsHelper::PROCEED, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); +} + INSTANTIATE_TEST_CASE_P(SafeBrowsingBlockingPageBrowserTestWithThreatType, SafeBrowsingBlockingPageBrowserTest, testing::Values(SB_THREAT_TYPE_URL_MALWARE, diff --git a/chrome/browser/ssl/cert_report_helper.cc b/chrome/browser/ssl/cert_report_helper.cc index 5d99b5f..80d0981 100644 --- a/chrome/browser/ssl/cert_report_helper.cc +++ b/chrome/browser/ssl/cert_report_helper.cc @@ -11,11 +11,11 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" -#include "chrome/browser/interstitials/security_interstitial_metrics_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ssl/ssl_cert_reporter.h" #include "chrome/common/pref_names.h" #include "chrome/grit/generated_resources.h" +#include "components/security_interstitials/metrics_helper.h" #include "components/variations/variations_associated_data.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/web_contents.h" @@ -36,15 +36,14 @@ CertReportHelper::CertReportHelper( const net::SSLInfo& ssl_info, CertificateErrorReport::InterstitialReason interstitial_reason, bool overridable, - SecurityInterstitialMetricsHelper* metrics_helper) + security_interstitials::MetricsHelper* metrics_helper) : ssl_cert_reporter_(ssl_cert_reporter.Pass()), web_contents_(web_contents), request_url_(request_url), ssl_info_(ssl_info), interstitial_reason_(interstitial_reason), overridable_(overridable), - metrics_helper_(metrics_helper) { -} + metrics_helper_(metrics_helper) {} CertReportHelper::~CertReportHelper() { } @@ -85,7 +84,7 @@ void CertReportHelper::FinishCertCollection( if (metrics_helper_) { metrics_helper_->RecordUserInteraction( - SecurityInterstitialMetricsHelper::EXTENDED_REPORTING_IS_ENABLED); + security_interstitials::MetricsHelper::EXTENDED_REPORTING_IS_ENABLED); } if (!ShouldReportCertificateError()) diff --git a/chrome/browser/ssl/cert_report_helper.h b/chrome/browser/ssl/cert_report_helper.h index 81ac135..6bebaff 100644 --- a/chrome/browser/ssl/cert_report_helper.h +++ b/chrome/browser/ssl/cert_report_helper.h @@ -20,7 +20,10 @@ namespace content { class WebContents; } -class SecurityInterstitialMetricsHelper; +namespace security_interstitials { +class MetricsHelper; +} + class SSLCertReporter; // CertReportHelper helps SSL interstitials report invalid certificate @@ -40,7 +43,7 @@ class CertReportHelper { const net::SSLInfo& ssl_info, CertificateErrorReport::InterstitialReason interstitial_reason, bool overridable, - SecurityInterstitialMetricsHelper* metrics_helper); + security_interstitials::MetricsHelper* metrics_helper); virtual ~CertReportHelper(); @@ -85,7 +88,7 @@ class CertReportHelper { // certificate chain error being reported. bool overridable_; // Helpful for recording metrics about cert reports. - SecurityInterstitialMetricsHelper* metrics_helper_; + security_interstitials::MetricsHelper* metrics_helper_; DISALLOW_COPY_AND_ASSIGN(CertReportHelper); }; diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc index 327fbad..55dfb48 100644 --- a/chrome/browser/ssl/ssl_blocking_page.cc +++ b/chrome/browser/ssl/ssl_blocking_page.cc @@ -24,7 +24,7 @@ #include "base/values.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/chrome_notification_types.h" -#include "chrome/browser/interstitials/security_interstitial_metrics_helper.h" +#include "chrome/browser/interstitials/chrome_metrics_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/renderer_preferences_util.h" #include "chrome/browser/ssl/cert_report_helper.h" @@ -256,18 +256,19 @@ SSLBlockingPage::SSLBlockingPage(content::WebContents* web_contents, SSL_REASON_BAD_CLOCK : SSL_REASON_SSL; // We collapse the Rappor metric name to just "ssl" so we don't leak - // the "overridable" bit. We skip Rappor altogether for bad clocks. + // the "overridable" bit. We skip Rappor altogether for bad clocks. // This must be done after calculating |interstitial_reason_| above. - set_metrics_helper(new SecurityInterstitialMetricsHelper( - web_contents, request_url, GetUmaHistogramPrefix(), kSSLRapporPrefix, - (interstitial_reason_ == SSL_REASON_BAD_CLOCK - ? SecurityInterstitialMetricsHelper::SKIP_RAPPOR - : SecurityInterstitialMetricsHelper::REPORT_RAPPOR), - GetSamplingEventName())); - - metrics_helper()->RecordUserDecision(SecurityInterstitialMetricsHelper::SHOW); + security_interstitials::MetricsHelper::ReportDetails reporting_info; + reporting_info.metric_prefix = GetUmaHistogramPrefix(); + reporting_info.rappor_prefix = kSSLRapporPrefix; + if (interstitial_reason_ != SSL_REASON_BAD_CLOCK) + reporting_info.rappor_report_type = rappor::UMA_RAPPOR_TYPE; + set_metrics_helper(new ChromeMetricsHelper( + web_contents, request_url, reporting_info, GetSamplingEventName())); + metrics_helper()->RecordUserDecision( + security_interstitials::MetricsHelper::SHOW); metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::TOTAL_VISITS); + security_interstitials::MetricsHelper::TOTAL_VISITS); cert_report_helper_.reset(new CertReportHelper( ssl_cert_reporter.Pass(), web_contents, request_url, ssl_info, @@ -303,7 +304,7 @@ SSLBlockingPage::~SSLBlockingPage() { // The page is closed without the user having chosen what to do, default to // deny. metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::DONT_PROCEED); + security_interstitials::MetricsHelper::DONT_PROCEED); RecordSSLExpirationPageEventState( expired_but_previously_allowed_, false, overridable_); NotifyDenyCertificate(); @@ -510,12 +511,12 @@ void SSLBlockingPage::CommandReceived(const std::string& command) { } case CMD_SHOW_MORE_SECTION: { metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_ADVANCED); + security_interstitials::MetricsHelper::SHOW_ADVANCED); break; } case CMD_OPEN_HELP_CENTER: { metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::SHOW_LEARN_MORE); + security_interstitials::MetricsHelper::SHOW_LEARN_MORE); content::NavigationController::LoadURLParams help_page_params( google_util::AppendGoogleLocaleParam( GURL(kHelpURL), g_browser_process->GetApplicationLocale())); @@ -524,14 +525,14 @@ void SSLBlockingPage::CommandReceived(const std::string& command) { } case CMD_RELOAD: { metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::RELOAD); + security_interstitials::MetricsHelper::RELOAD); // The interstitial can't refresh itself. web_contents()->GetController().Reload(true); break; } case CMD_OPEN_DATE_SETTINGS: { metrics_helper()->RecordUserInteraction( - SecurityInterstitialMetricsHelper::OPEN_TIME_SETTINGS); + security_interstitials::MetricsHelper::OPEN_TIME_SETTINGS); content::BrowserThread::PostTask(content::BrowserThread::FILE, FROM_HERE, base::Bind(&LaunchDateAndTimeSettings)); break; @@ -555,7 +556,7 @@ void SSLBlockingPage::OverrideRendererPrefs( void SSLBlockingPage::OnProceed() { metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::PROCEED); + security_interstitials::MetricsHelper::PROCEED); // Finish collecting information about invalid certificates, if the // user opted in to. @@ -570,7 +571,7 @@ void SSLBlockingPage::OnProceed() { void SSLBlockingPage::OnDontProceed() { metrics_helper()->RecordUserDecision( - SecurityInterstitialMetricsHelper::DONT_PROCEED); + security_interstitials::MetricsHelper::DONT_PROCEED); // Finish collecting information about invalid certificates, if the // user opted in to. diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc index 78ae5a0..9e634348 100644 --- a/chrome/browser/ssl/ssl_browser_tests.cc +++ b/chrome/browser/ssl/ssl_browser_tests.cc @@ -13,6 +13,7 @@ #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" +#include "base/test/histogram_tester.h" #include "base/thread_task_runner_handle.h" #include "base/time/time.h" #include "chrome/app/chrome_command_ids.h" @@ -38,6 +39,7 @@ #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/ui_test_utils.h" #include "components/content_settings/core/browser/host_content_settings_map.h" +#include "components/security_interstitials/metrics_helper.h" #include "components/variations/variations_associated_data.h" #include "components/web_modal/web_contents_modal_dialog_manager.h" #include "content/public/browser/browser_context.h" @@ -524,6 +526,84 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, TestBrokenHTTPSWithInsecureContent) { AuthState::DISPLAYED_INSECURE_CONTENT); } +IN_PROC_BROWSER_TEST_F(SSLUITest, TestBrokenHTTPSMetricsReporting_Proceed) { + ASSERT_TRUE(https_server_expired_.Start()); + ASSERT_NO_FATAL_FAILURE(SetUpMockReporter()); + base::HistogramTester histograms; + const std::string decision_histogram = + "interstitial.ssl_overridable.decision"; + const std::string interaction_histogram = + "interstitial.ssl_overridable.interaction"; + + // Histograms should start off empty. + histograms.ExpectTotalCount(decision_histogram, 0); + histograms.ExpectTotalCount(interaction_histogram, 0); + + // After navigating to the page, the totals should be set. + ui_test_utils::NavigateToURL(browser(), https_server_expired_.GetURL("/")); + content::WaitForInterstitialAttach( + browser()->tab_strip_model()->GetActiveWebContents()); + histograms.ExpectTotalCount(decision_histogram, 1); + histograms.ExpectBucketCount(decision_histogram, + security_interstitials::MetricsHelper::SHOW, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); + + // Decision should be recorded. + ProceedThroughInterstitial( + browser()->tab_strip_model()->GetActiveWebContents()); + histograms.ExpectTotalCount(decision_histogram, 2); + histograms.ExpectBucketCount( + decision_histogram, security_interstitials::MetricsHelper::PROCEED, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); +} + +IN_PROC_BROWSER_TEST_F(SSLUITest, TestBrokenHTTPSMetricsReporting_DontProceed) { + ASSERT_TRUE(https_server_expired_.Start()); + ASSERT_NO_FATAL_FAILURE(SetUpMockReporter()); + base::HistogramTester histograms; + const std::string decision_histogram = + "interstitial.ssl_overridable.decision"; + const std::string interaction_histogram = + "interstitial.ssl_overridable.interaction"; + + // Histograms should start off empty. + histograms.ExpectTotalCount(decision_histogram, 0); + histograms.ExpectTotalCount(interaction_histogram, 0); + + // After navigating to the page, the totals should be set. + ui_test_utils::NavigateToURL(browser(), https_server_expired_.GetURL("/")); + content::WaitForInterstitialAttach( + browser()->tab_strip_model()->GetActiveWebContents()); + histograms.ExpectTotalCount(decision_histogram, 1); + histograms.ExpectBucketCount(decision_histogram, + security_interstitials::MetricsHelper::SHOW, 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); + + // Decision should be recorded. + InterstitialPage* interstitial_page = browser() + ->tab_strip_model() + ->GetActiveWebContents() + ->GetInterstitialPage(); + interstitial_page->DontProceed(); + histograms.ExpectTotalCount(decision_histogram, 2); + histograms.ExpectBucketCount( + decision_histogram, security_interstitials::MetricsHelper::DONT_PROCEED, + 1); + histograms.ExpectTotalCount(interaction_histogram, 1); + histograms.ExpectBucketCount( + interaction_histogram, + security_interstitials::MetricsHelper::TOTAL_VISITS, 1); +} + // http://crbug.com/91745 #if defined(OS_CHROMEOS) #define MAYBE_TestOKHTTPS DISABLED_TestOKHTTPS diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 2364f2a..1adf392 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -531,8 +531,8 @@ 'browser/install_verification/win/module_verification_common.h', 'browser/internal_auth.cc', 'browser/internal_auth.h', - 'browser/interstitials/security_interstitial_metrics_helper.cc', - 'browser/interstitials/security_interstitial_metrics_helper.h', + 'browser/interstitials/chrome_metrics_helper.cc', + 'browser/interstitials/chrome_metrics_helper.h', 'browser/interstitials/security_interstitial_page.cc', 'browser/interstitials/security_interstitial_page.h', 'browser/intranet_redirect_detector.cc', @@ -3186,6 +3186,7 @@ '../components/components.gyp:search', '../components/components.gyp:search_engines', '../components/components.gyp:search_provider_logos', + '../components/components.gyp:security_interstitials', '../components/components.gyp:suggestions', '../components/components.gyp:signin_core_browser', '../components/components.gyp:startup_metric_utils', diff --git a/components/BUILD.gn b/components/BUILD.gn index dc82d95..c22576c 100644 --- a/components/BUILD.gn +++ b/components/BUILD.gn @@ -93,6 +93,7 @@ group("all_components") { "//components/search_engines", "//components/search_provider_logos", "//components/secure_display", + "//components/security_interstitials", "//components/sessions", "//components/signin/core/browser", "//components/startup_metric_utils", diff --git a/components/components.gyp b/components/components.gyp index ff7bd7e0..464dfad 100644 --- a/components/components.gyp +++ b/components/components.gyp @@ -63,6 +63,7 @@ 'search_engines.gypi', 'search_provider_logos.gypi', 'secure_display.gypi', + 'security_interstitials.gypi', 'sessions.gypi', 'signin.gypi', 'startup_metric_utils.gypi', diff --git a/components/security_interstitials.gypi b/components/security_interstitials.gypi new file mode 100644 index 0000000..e93c58f --- /dev/null +++ b/components/security_interstitials.gypi @@ -0,0 +1,26 @@ +# 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. + +{ + 'targets': [ + { + 'target_name': 'security_interstitials', + 'type': 'static_library', + 'dependencies': [ + '../base/base.gyp:base', + '../net/net.gyp:net', + 'history_core_browser', + 'metrics', + 'rappor', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'security_interstitials/metrics_helper.cc', + 'security_interstitials/metrics_helper.h', + ] + } + ] +}
\ No newline at end of file diff --git a/components/security_interstitials/BUILD.gn b/components/security_interstitials/BUILD.gn new file mode 100644 index 0000000..30d2eda --- /dev/null +++ b/components/security_interstitials/BUILD.gn @@ -0,0 +1,14 @@ +source_set("security_interstitials") { + sources = [ + "metrics_helper.cc", + "metrics_helper.h", + ] + + deps = [ + "//base", + "//components/history/core/browser", + "//components/metrics", + "//components/rappor", + "//net", + ] +} diff --git a/components/security_interstitials/DEPS b/components/security_interstitials/DEPS new file mode 100644 index 0000000..a32a580 --- /dev/null +++ b/components/security_interstitials/DEPS @@ -0,0 +1,6 @@ +include_rules = [ + "+components/history/core/browser", + "+components/metrics", + "+components/rappor", + "+net/base" +] diff --git a/components/security_interstitials/OWNERS b/components/security_interstitials/OWNERS new file mode 100644 index 0000000..d46fc3f --- /dev/null +++ b/components/security_interstitials/OWNERS @@ -0,0 +1,6 @@ +estark@chromium.org +felt@chromium.org +mattm@chromium.org +meacer@chromium.org +nparker@chromium.org +palmer@chromium.org
\ No newline at end of file diff --git a/components/security_interstitials/metrics_helper.cc b/components/security_interstitials/metrics_helper.cc new file mode 100644 index 0000000..ae2444d --- /dev/null +++ b/components/security_interstitials/metrics_helper.cc @@ -0,0 +1,116 @@ +// 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 "components/security_interstitials/metrics_helper.h" + +#include "base/metrics/histogram.h" +#include "components/history/core/browser/history_service.h" +#include "components/rappor/rappor_service.h" +#include "components/rappor/rappor_utils.h" +#include "net/base/registry_controlled_domains/registry_controlled_domain.h" + +namespace { +// Used for setting bits in Rappor's "interstitial.*.flags" +enum InterstitialFlagBits { + DID_PROCEED = 0, + IS_REPEAT_VISIT = 1, + HIGHEST_USED_BIT = 1 +}; +} // namespace + +namespace security_interstitials { + +MetricsHelper::MetricsHelper(const GURL& request_url, + const ReportDetails settings, + history::HistoryService* history_service, + rappor::RapporService* rappor_service) + : request_url_(request_url), + settings_(settings), + rappor_service_(rappor_service), + num_visits_(-1) { + DCHECK(!settings_.metric_prefix.empty()); + if (settings_.rappor_report_type == rappor::NUM_RAPPOR_TYPES) // Default. + rappor_service = nullptr; + DCHECK(!rappor_service || !settings_.rappor_prefix.empty()); + if (history_service) { + history_service->GetVisibleVisitCountToHost( + request_url_, + base::Bind(&MetricsHelper::OnGotHistoryCount, base::Unretained(this)), + &request_tracker_); + } +} + +// Directly adds to the UMA histograms, using the same properties as +// UMA_HISTOGRAM_ENUMERATION, because the macro doesn't allow non-constant +// histogram names. Reports to Rappor for certain decisions. +void MetricsHelper::RecordUserDecision(Decision decision) { + // UMA + const std::string decision_histogram_name( + "interstitial." + settings_.metric_prefix + ".decision"); + base::HistogramBase* decision_histogram = base::LinearHistogram::FactoryGet( + decision_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1, + base::HistogramBase::kUmaTargetedHistogramFlag); + decision_histogram->Add(decision); + + // Rappor + if (rappor_service_ && (decision == PROCEED || decision == DONT_PROCEED)) { + scoped_ptr<rappor::Sample> sample = + rappor_service_->CreateSample(settings_.rappor_report_type); + + // This will populate, for example, "intersitial.malware.domain" or + // "interstitial.ssl2.domain". |domain| will be empty for hosts w/o TLDs. + const std::string domain = + rappor::GetDomainAndRegistrySampleFromGURL(request_url_); + sample->SetStringField("domain", domain); + + // Only report history and decision if we have history data. + if (num_visits_ >= 0) { + int flags = 0; + if (decision == PROCEED) + flags |= 1 << InterstitialFlagBits::DID_PROCEED; + if (num_visits_ > 0) + flags |= 1 << InterstitialFlagBits::IS_REPEAT_VISIT; + // e.g. "interstitial.malware.flags" + sample->SetFlagsField("flags", flags, + InterstitialFlagBits::HIGHEST_USED_BIT + 1); + } + rappor_service_->RecordSampleObj("interstitial." + settings_.rappor_prefix, + sample.Pass()); + } + + // Record additional information about sites that users have + // visited before. + if (num_visits_ < 1) + return; + std::string history_histogram_name("interstitial." + settings_.metric_prefix + + ".decision.repeat_visit"); + base::HistogramBase* history_histogram = base::LinearHistogram::FactoryGet( + history_histogram_name, 1, MAX_DECISION, MAX_DECISION + 1, + base::HistogramBase::kUmaTargetedHistogramFlag); + history_histogram->Add(SHOW); + history_histogram->Add(decision); + + RecordExtraUserDecisionMetrics(decision); +} + +void MetricsHelper::RecordUserInteraction(Interaction interaction) { + const std::string interaction_histogram_name( + "interstitial." + settings_.metric_prefix + ".interaction"); + base::HistogramBase* interaction_histogram = + base::LinearHistogram::FactoryGet( + interaction_histogram_name, 1, MAX_INTERACTION, MAX_INTERACTION + 1, + base::HistogramBase::kUmaTargetedHistogramFlag); + interaction_histogram->Add(interaction); + + RecordExtraUserInteractionMetrics(interaction); +} + +void MetricsHelper::OnGotHistoryCount(bool success, + int num_visits, + base::Time /*first_visit*/) { + if (success) + num_visits_ = num_visits; +} + +} // namespace security_interstitials diff --git a/components/security_interstitials/metrics_helper.h b/components/security_interstitials/metrics_helper.h new file mode 100644 index 0000000..2e403bc --- /dev/null +++ b/components/security_interstitials/metrics_helper.h @@ -0,0 +1,111 @@ +// 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 COMPONENTS_SECURITY_INTERSTITIALS_METRICS_HELPER_H_ +#define COMPONENTS_SECURITY_INTERSTITIALS_METRICS_HELPER_H_ + +#include <string> + +#include "base/task/cancelable_task_tracker.h" +#include "base/time/time.h" +#include "components/rappor/rappor_service.h" +#include "url/gurl.h" + +namespace history { +class HistoryService; +} + +namespace security_interstitials { + +// MetricsHelper records user warning interactions in a +// common way via METRICS histograms and, optionally, RAPPOR metrics. The +// class will generate the following histograms: +// METRICS: interstitial.uma_prefix.decision +// METRICS: interstitial.uma_prefix.decision.repeat_visit +// METRICS: interstitial.uma_prefix.interaction +// METRICS: interstitial.uma_prefix.interaction.repeat_visit +// RAPPOR: interstitial.rappor_prefix +// wherein |uma_prefix| and |rappor_prefix| are specified via ReportDetails. +class MetricsHelper { + public: + // These enums are used for histograms. Don't reorder, delete, or insert + // elements. New elements should be added at the end (right before the max). + enum Decision { + SHOW, + PROCEED, + DONT_PROCEED, + PROCEEDING_DISABLED, + MAX_DECISION + }; + enum Interaction { + TOTAL_VISITS, + SHOW_ADVANCED, + SHOW_PRIVACY_POLICY, + SHOW_DIAGNOSTIC, + SHOW_LEARN_MORE, + RELOAD, + OPEN_TIME_SETTINGS, + SET_EXTENDED_REPORTING_ENABLED, + SET_EXTENDED_REPORTING_DISABLED, + EXTENDED_REPORTING_IS_ENABLED, + REPORT_PHISHING_ERROR, + MAX_INTERACTION + }; + + // uma_prefix: Histogram prefix for UMA. + // examples: "phishing", "ssl_overridable" + // rappor_prefix: Metric prefix for Rappor. + // examples: "phishing", "ssl2" + // rappor_report_type: Used to differentiate UMA and Safe Browsing statistics. + // The rappor preferences can be left blank if rappor_service is not set. + struct ReportDetails { + ReportDetails() : rappor_report_type(rappor::NUM_RAPPOR_TYPES) {} + std::string metric_prefix; + std::string rappor_prefix; + rappor::RapporType rappor_report_type; + }; + + // Args: + // url: URL of page that triggered the interstitial. Only origin is used. + // history_service: Set this to record metrics based on whether the user + // has visited this hostname before. + // rappor_service: If you want RAPPOR statistics, provide a service, + // settings.rappor_prefix, and settings.rappor_report_type. + // settings: Specify reporting details (prefixes and report types). + // sampling_event_name: Event name for Experience Sampling. + // e.g. "phishing_interstitial_" + MetricsHelper(const GURL& url, + const ReportDetails settings, + history::HistoryService* history_service, + rappor::RapporService* rappor_service); + virtual ~MetricsHelper() {} + + // Records a user decision or interaction to the appropriate UMA histogram + // and potentially in a RAPPOR metric. + void RecordUserDecision(Decision decision); + void RecordUserInteraction(Interaction interaction); + + protected: + // Subclasses should implement any embedder-specific recording logic in these + // methods. They'll be invoked from RecordUserDecision/Interaction. + virtual void RecordExtraUserDecisionMetrics(Decision decision) = 0; + virtual void RecordExtraUserInteractionMetrics(Interaction interaction) = 0; + + private: + // Used to query the HistoryService to see if the URL is in history. + // It will only be invoked if the constructor received |history_service|. + void OnGotHistoryCount(bool success, int num_visits, base::Time first_visit); + + const GURL request_url_; + const ReportDetails settings_; + rappor::RapporService* rappor_service_; + int num_visits_; + base::CancelableTaskTracker request_tracker_; + + DISALLOW_COPY_AND_ASSIGN(MetricsHelper); +}; + +} // namespace security_interstitials + +#endif // COMPONENTS_SECURITY_INTERSTITIALS_METRICS_HELPER_H_ |