Add connection info to Downloads UKM

This adds the DownloadConnectionSecurity of the download and a boolean
for whether the final download URL is a different host than the
originating URL to the existing Download.Started UKM.

This also refactors out the logic for checking the
DownloadConnectionSecurity into a new public function
(CheckDownloadConnectionSecurity) in download_stats.h and exposes the
underlying DownloadConnectionSecurity enum.

This will help us identify sites that have a lot of downloads over
HTTP (or go over HTTP in their redirect chain) for outreach efforts.

Bug: 839986
Change-Id: I9a1bb78b8c62547e322ec1b85b5acfaa7acc813a
Reviewed-on: https://chromium-review.googlesource.com/1195854
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Robert Kaplow (slow) <rkaplow@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587737}

components/download/internal/common/download_item_impl.cc

@@ -36,6 +36,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/stl_util.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -1470,9 +1471,14 @@ void DownloadItemImpl::Start(
     DownloadContent file_type = DownloadContentFromMimeType(mime_type_, false);
     auto in_progress_entry = delegate_->GetInProgressEntry(this);
     if (in_progress_entry) {
+      bool is_same_host_download =
+          base::StringPiece(new_create_info.url().host())
+              .ends_with(new_create_info.site_url.host());
+      DownloadConnectionSecurity state = CheckDownloadConnectionSecurity(
+          new_create_info.url(), new_create_info.url_chain);
       DownloadUkmHelper::RecordDownloadStarted(
           in_progress_entry->ukm_download_id, new_create_info.ukm_source_id,
-          file_type, download_source_);
+          file_type, download_source_, state, is_same_host_download);
     }
 
     if (!delegate_->IsOffTheRecord()) {

components/download/internal/common/download_stats.cc

@@ -932,36 +932,9 @@ void RecordSavePackageEvent(SavePackageEvent event) {
                             SAVE_PACKAGE_LAST_ENTRY);
 }
 
-namespace {
-
-// Enumeration for histogramming purposes.
-// These values are written to logs.  New enum values can be added, but existing
-// enums must never be renumbered or deleted and reused.
-enum DownloadConnectionSecurity {
-  DOWNLOAD_SECURE = 0,  // Final download url and its redirects all use https
-  DOWNLOAD_TARGET_INSECURE =
-      1,  // Final download url uses http, redirects are all
-          // https
-  DOWNLOAD_REDIRECT_INSECURE =
-      2,  // Final download url uses https, but at least
-          // one redirect uses http
-  DOWNLOAD_REDIRECT_TARGET_INSECURE =
-      3,                      // Final download url uses http, and at
-                              // least one redirect uses http
-  DOWNLOAD_TARGET_OTHER = 4,  // Final download url uses a scheme not present in
-                              // this enumeration
-  DOWNLOAD_TARGET_BLOB = 5,   // Final download url uses blob scheme
-  DOWNLOAD_TARGET_DATA = 6,   //  Final download url uses data scheme
-  DOWNLOAD_TARGET_FILE = 7,   //  Final download url uses file scheme
-  DOWNLOAD_TARGET_FILESYSTEM = 8,  //  Final download url uses filesystem scheme
-  DOWNLOAD_TARGET_FTP = 9,         // Final download url uses ftp scheme
-  DOWNLOAD_CONNECTION_SECURITY_MAX
-};
-
-}  // namespace
-
-void RecordDownloadConnectionSecurity(const GURL& download_url,
-                                      const std::vector<GURL>& url_chain) {
+DownloadConnectionSecurity CheckDownloadConnectionSecurity(
+    const GURL& download_url,
+    const std::vector<GURL>& url_chain) {
   DownloadConnectionSecurity state = DOWNLOAD_TARGET_OTHER;
   if (download_url.SchemeIsHTTPOrHTTPS()) {
     bool is_final_download_secure = download_url.SchemeIsCryptographic();
@@ -990,9 +963,15 @@ void RecordDownloadConnectionSecurity(const GURL& download_url,
   } else if (download_url.SchemeIs(url::kFtpScheme)) {
     state = DOWNLOAD_TARGET_FTP;
   }
+  return state;
+}
 
-  UMA_HISTOGRAM_ENUMERATION("Download.TargetConnectionSecurity", state,
-                            DOWNLOAD_CONNECTION_SECURITY_MAX);
+void RecordDownloadConnectionSecurity(const GURL& download_url,
+                                      const std::vector<GURL>& url_chain) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Download.TargetConnectionSecurity",
+      CheckDownloadConnectionSecurity(download_url, url_chain),
+      DOWNLOAD_CONNECTION_SECURITY_MAX);
 }
 
 void RecordDownloadContentTypeSecurity(

components/download/internal/common/download_ukm_helper.cc

@@ -27,11 +27,15 @@ int DownloadUkmHelper::CalcNearestKB(int num_bytes) {
 void DownloadUkmHelper::RecordDownloadStarted(int download_id,
                                               ukm::SourceId source_id,
                                               DownloadContent file_type,
-                                              DownloadSource download_source) {
+                                              DownloadSource download_source,
+                                              DownloadConnectionSecurity state,
+                                              bool is_same_host_download) {
   ukm::builders::Download_Started(source_id)
       .SetDownloadId(download_id)
       .SetFileType(static_cast<int>(file_type))
       .SetDownloadSource(static_cast<int>(download_source))
+      .SetDownloadConnectionSecurity(static_cast<int>(state))
+      .SetIsSameHostDownload(is_same_host_download)
       .Record(ukm::UkmRecorder::Get());
 }
 

components/download/internal/common/download_ukm_helper_unittest.cc

@@ -56,15 +56,22 @@ TEST_F(DownloadUkmHelperTest, TestBasicReporting) {
   ukm::SourceId source_id = ukm::UkmRecorder::GetNewSourceID();
   DownloadContent file_type = DownloadContent::AUDIO;
   DownloadSource download_source = DownloadSource::UNKNOWN;
+  DownloadConnectionSecurity state =
+      DownloadConnectionSecurity::DOWNLOAD_SECURE;
+  bool is_same_host_download = true;
   DownloadUkmHelper::RecordDownloadStarted(download_id_, source_id, file_type,
-                                           download_source);
+                                           download_source, state,
+                                           is_same_host_download);
 
   ExpectUkmMetrics(
       UkmDownloadStarted::kEntryName,
       {UkmDownloadStarted::kDownloadIdName, UkmDownloadStarted::kFileTypeName,
-       UkmDownloadStarted::kDownloadSourceName},
+       UkmDownloadStarted::kDownloadSourceName,
+       UkmDownloadStarted::kDownloadConnectionSecurityName,
+       UkmDownloadStarted::kIsSameHostDownloadName},
       {download_id_, static_cast<int>(file_type),
-       static_cast<int>(download_source)});
+       static_cast<int>(download_source), static_cast<int>(state),
+       is_same_host_download});
 
   // RecordDownloadInterrupted, has change in file size.
   int change_in_file_size = 1000;

components/download/public/common/download_stats.h

@@ -367,6 +367,34 @@ enum OriginStateOnResumption {
   ORIGIN_STATE_ON_RESUMPTION_MAX = 1 << 3
 };
 
+// Enumeration for histogramming purposes.
+// These values are written to logs.  New enum values can be added, but existing
+// enums must never be renumbered or deleted and reused.
+enum DownloadConnectionSecurity {
+  DOWNLOAD_SECURE = 0,  // Final download url and its redirects all use https
+  DOWNLOAD_TARGET_INSECURE =
+      1,  // Final download url uses http, redirects are all
+          // https
+  DOWNLOAD_REDIRECT_INSECURE =
+      2,  // Final download url uses https, but at least
+          // one redirect uses http
+  DOWNLOAD_REDIRECT_TARGET_INSECURE =
+      3,                      // Final download url uses http, and at
+                              // least one redirect uses http
+  DOWNLOAD_TARGET_OTHER = 4,  // Final download url uses a scheme not present in
+                              // this enumeration
+  DOWNLOAD_TARGET_BLOB = 5,   // Final download url uses blob scheme
+  DOWNLOAD_TARGET_DATA = 6,   //  Final download url uses data scheme
+  DOWNLOAD_TARGET_FILE = 7,   //  Final download url uses file scheme
+  DOWNLOAD_TARGET_FILESYSTEM = 8,  //  Final download url uses filesystem scheme
+  DOWNLOAD_TARGET_FTP = 9,         // Final download url uses ftp scheme
+  DOWNLOAD_CONNECTION_SECURITY_MAX
+};
+
+COMPONENTS_DOWNLOAD_EXPORT DownloadConnectionSecurity
+CheckDownloadConnectionSecurity(const GURL& download_url,
+                                const std::vector<GURL>& url_chain);
+
 COMPONENTS_DOWNLOAD_EXPORT void RecordDownloadConnectionSecurity(
     const GURL& download_url,
     const std::vector<GURL>& url_chain);

components/download/public/common/download_ukm_helper.h

@@ -10,6 +10,7 @@
 #include "components/download/public/common/download_content.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_source.h"
+#include "components/download/public/common/download_stats.h"
 #include "components/download/public/common/resume_mode.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 
@@ -29,7 +30,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadUkmHelper {
   static void RecordDownloadStarted(int download_id,
                                     ukm::SourceId source_id,
                                     DownloadContent file_type,
-                                    DownloadSource download_source);
+                                    DownloadSource download_source,
+                                    DownloadConnectionSecurity state,
+                                    bool is_same_host_download);
 
   // Record when the download is interrupted.
   static void RecordDownloadInterrupted(int download_id,

content/browser/download/save_package.cc

@@ -263,7 +263,11 @@ void SavePackage::InternalInit() {
   ukm_download_id_ = download::GetUniqueDownloadId();
   download::DownloadUkmHelper::RecordDownloadStarted(
       ukm_download_id_, ukm_source_id_, download::DownloadContent::TEXT,
-      download::DownloadSource::UNKNOWN);
+      download::DownloadSource::UNKNOWN,
+      download::CheckDownloadConnectionSecurity(
+          web_contents()->GetLastCommittedURL(),
+          std::vector<GURL>{web_contents()->GetLastCommittedURL()}),
+      true /* is_same_host_download */);
 }
 
 bool SavePackage::Init(

tools/metrics/ukm/ukm.xml

@@ -1078,6 +1078,12 @@ be describing additional metrics about the same event.
     Metrics taken when a download begins. It has one Download.Ended and none to
     multiple Download.Interrupted/Download.Resumed events associated with it.
   </summary>
+  <metric name="DownloadConnectionSecurity">
+    <summary>
+      The state of the security of the final download URL and all the redirects
+      leading to it. Expressed as an enum defined in DownloadConnectionSecurity.
+    </summary>
+  </metric>
   <metric name="DownloadId">
     <summary>
       The id of the download that is used to associate separate events.
@@ -1094,6 +1100,13 @@ be describing additional metrics about the same event.
       DownloadContentType.
     </summary>
   </metric>
+  <metric name="IsSameHostDownload">
+    <summary>
+      A boolean denoting if the final download URL is the same host as the
+      initiating frame (i.e., whether the initiating site likely controls the
+      download itself).
+    </summary>
+  </metric>
 </event>
 
 <event name="Download.Completed">