Add Telemetry to the Storage Access API

This change adds telemetry to the Storage Access API with the goal of tracking the utilization of the API and how the API is being used. This includes the following metrics: - UseCounter for document.hasStorageAccess and document.requestStorageAccess methods. - Histogram API.StorageAccess.AllowedRequests to track how often requests for storage access are allowed/blocked and specifically allowed due to the API. - Histogram API.StorageAccess.GrantIsImplicit to track how often requests generate an implicit vs explicit grant. - Histogram API.StorageAccess.RequestStorageAccess to track how the main method for the API is utilized and how frequently usage is successful or fails for various reasons. Various tests have been added where possible to validate the associated telemetry items have been fired. Bug: 989663 Change-Id: I44ecf8240ccfd50fee445e1782e9a578354ab68a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211135 Commit-Queue: Brandon Maslen <brandm@microsoft.com> Reviewed-by: Balazs Engedy <engedy@chromium.org> Reviewed-by: Christian Dullweber <dullweber@chromium.org> Reviewed-by: Brian White <bcwhite@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Maksim Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/master@{#774419}
marcosholgado · Jun 3, 2020 · 92d45aa · 92d45aa
1 parent a12e77a
commit 92d45aa
Show file tree

Hide file tree

Showing 17 changed files with 369 additions and 8 deletions.
diff --git a/chrome/browser/storage_access_api/api_browsertest.cc b/chrome/browser/storage_access_api/api_browsertest.cc
@@ -7,6 +7,7 @@
 #include "base/macros.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/content_settings/cookie_settings_factory.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -40,6 +41,8 @@ using content::BrowserThread;
 
 namespace {
 
+constexpr char kUseCounterHistogram[] = "Blink.UseCounter.Features";
+
 class StorageAccessAPIBrowserTest : public InProcessBrowserTest {
  protected:
   StorageAccessAPIBrowserTest()
@@ -139,6 +142,7 @@ class StorageAccessAPIBrowserTest : public InProcessBrowserTest {
 IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest,
                        ThirdPartyCookiesIFrameRequestsAccess) {
   SetBlockThirdPartyCookies(true);
+  base::HistogramTester histogram_tester;
 
   // Set a cookie on `b.com`.
   content::SetCookie(browser()->profile(), https_server_.GetURL("b.com", "/"),
@@ -163,11 +167,28 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIBrowserTest,
   storage::test::RequestStorageAccessForFrame(GetFrame(), true);
   storage::test::CheckStorageAccessForFrame(GetFrame(), true);
 
+  // Our use counter should not have fired yet, so we should have 0 occurrences.
+  histogram_tester.ExpectBucketCount(
+      kUseCounterHistogram,
+      /*kStorageAccessAPI_HasStorageAccess_Method=*/3310, 0);
+  histogram_tester.ExpectBucketCount(
+      kUseCounterHistogram,
+      /*kStorageAccessAPI_requestStorageAccess_Method=*/3311, 0);
+
   // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
   // the cookie is sent:
   NavigateFrameTo("b.com", "/echoheader?cookie");
   ExpectFrameContent("thirdparty=1");
   storage::test::CheckStorageAccessForFrame(GetFrame(), true);
+
+  // Since the frame has navigated we should see the use counter telem appear.
+  histogram_tester.ExpectBucketCount(
+      kUseCounterHistogram,
+      /*kStorageAccessAPI_HasStorageAccess_Method=*/3310, 1);
+  histogram_tester.ExpectBucketCount(
+      kUseCounterHistogram,
+      /*kStorageAccessAPI_requestStorageAccess_Method=*/3311, 1);
+
   // Navigate iframe to othersite.com and verify that the cookie is not sent.
   NavigateFrameTo("othersite.com", "/echoheader?cookie");
   ExpectFrameContent("None");

diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc b/chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
@@ -8,6 +8,7 @@
 #include "base/check_op.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial_params.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -157,6 +158,12 @@ void StorageAccessGrantPermissionContext::NotifyPermissionSetInternal(
     return;
   }
 
+  // Our failure cases are tracked by the prompt outcomes in the
+  // `Permissions.Action.StorageAccess` histogram. We'll only log when a grant
+  // is actually generated.
+  base::UmaHistogramBoolean("API.StorageAccess.GrantIsImplicit",
+                            implicit_result);
+
   HostContentSettingsMap* settings_map =
       HostContentSettingsMapFactory::GetForProfile(browser_context());
   DCHECK(settings_map);

diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context.h b/chrome/browser/storage_access_api/storage_access_grant_permission_context.h
@@ -14,7 +14,8 @@ extern const int kDefaultImplicitGrantLimit;
 class StorageAccessGrantPermissionContext
     : public permissions::PermissionContextBase {
  public:
-  StorageAccessGrantPermissionContext(content::BrowserContext* browser_context);
+  explicit StorageAccessGrantPermissionContext(
+      content::BrowserContext* browser_context);
 
   ~StorageAccessGrantPermissionContext() override;
 
@@ -27,6 +28,10 @@ class StorageAccessGrantPermissionContext
                            PermissionDeniedWithoutUserGesture);
   FRIEND_TEST_ALL_PREFIXES(StorageAccessGrantPermissionContextTest,
                            ImplicitGrantLimitPerRequestingOrigin);
+  FRIEND_TEST_ALL_PREFIXES(StorageAccessGrantPermissionContextTest,
+                           ExplicitGrantDenial);
+  FRIEND_TEST_ALL_PREFIXES(StorageAccessGrantPermissionContextTest,
+                           ExplicitGrantAccept);
   friend class StorageAccessGrantPermissionContextTest;
 
   StorageAccessGrantPermissionContext(

diff --git a/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc b/chrome/browser/storage_access_api/storage_access_grant_permission_context_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/storage_access_api/storage_access_grant_permission_context.h"
 
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "components/content_settings/core/common/content_settings.h"
@@ -15,6 +16,10 @@
 
 namespace {
 
+constexpr char kGrantIsImplicitHistogram[] =
+    "API.StorageAccess.GrantIsImplicit";
+constexpr char kPromptResultHistogram[] = "Permissions.Action.StorageAccess";
+
 constexpr char kInsecureURL[] = "http://www.example.com";
 constexpr char kSecureURL[] = "https://www.example.com";
 constexpr char kAlternateURL[] = "https://embedder_example.test";
@@ -210,6 +215,9 @@ TEST_F(StorageAccessGrantPermissionContextTest,
   base::test::ScopedFeatureList scoped_enable;
   scoped_enable.InitAndEnableFeature(blink::features::kStorageAccessAPI);
 
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 0);
+
   StorageAccessGrantPermissionContext permission_context(profile());
   permissions::PermissionRequestID fake_id(
       /*render_process_id=*/0, /*render_frame_id=*/0, /*request_id=*/0);
@@ -221,6 +229,9 @@ TEST_F(StorageAccessGrantPermissionContextTest,
   // Ensure all our implicit grants are taken care of for |requesting_origin_1|
   // before we proceed to validate.
   ExhaustImplicitGrants(requesting_origin_1, permission_context);
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
 
   ContentSetting result = CONTENT_SETTING_DEFAULT;
   permission_context.DecidePermission(
@@ -240,6 +251,13 @@ TEST_F(StorageAccessGrantPermissionContextTest,
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(CONTENT_SETTING_ASK, result);
 
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
+  histogram_tester.ExpectTotalCount(kPromptResultHistogram, 1);
+  histogram_tester.ExpectBucketCount(kPromptResultHistogram,
+                                     /*DISMISSED=*/2, 1);
+
   // However now if a different requesting origin makes a request we should see
   // it gets auto-granted as the limit has not been reached for it yet.
   result = CONTENT_SETTING_DEFAULT;
@@ -251,4 +269,110 @@ TEST_F(StorageAccessGrantPermissionContextTest,
   // We should have no prompts still and our latest result should be an allow.
   EXPECT_FALSE(manager->IsRequestInProgress());
   EXPECT_EQ(CONTENT_SETTING_ALLOW, result);
+
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 6);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 6);
+  histogram_tester.ExpectBucketCount(kPromptResultHistogram,
+                                     /*DISMISSED=*/2, 1);
+}
+
+TEST_F(StorageAccessGrantPermissionContextTest, ExplicitGrantDenial) {
+  base::test::ScopedFeatureList scoped_enable;
+  scoped_enable.InitAndEnableFeature(blink::features::kStorageAccessAPI);
+
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 0);
+  histogram_tester.ExpectTotalCount(kPromptResultHistogram, 0);
+
+  StorageAccessGrantPermissionContext permission_context(profile());
+  permissions::PermissionRequestID fake_id(
+      /*render_process_id=*/0, /*render_frame_id=*/0, /*request_id=*/0);
+
+  const GURL requesting_origin_1(kAlternateURL);
+  const GURL requesting_origin_2(kInsecureURL);
+  const GURL embedding_origin(kSecureURL);
+
+  // Ensure all our implicit grants are taken care of for |requesting_origin_1|
+  // before we proceed to validate.
+  ExhaustImplicitGrants(requesting_origin_1, permission_context);
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
+
+  ContentSetting result = CONTENT_SETTING_DEFAULT;
+  permission_context.DecidePermission(
+      web_contents(), fake_id, requesting_origin_1, embedding_origin,
+      /*user_gesture=*/true, base::BindOnce(&SaveResult, &result));
+  base::RunLoop().RunUntilIdle();
+
+  // We should get a prompt showing up right now.
+  permissions::PermissionRequestManager* manager =
+      permissions::PermissionRequestManager::FromWebContents(web_contents());
+  DCHECK(manager);
+  EXPECT_TRUE(manager->IsRequestInProgress());
+
+  // Deny the prompt and validate we get the expected setting back in our
+  // callback.
+  manager->Deny();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CONTENT_SETTING_BLOCK, result);
+
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
+  histogram_tester.ExpectTotalCount(kPromptResultHistogram, 1);
+  histogram_tester.ExpectBucketCount(kPromptResultHistogram,
+                                     /*DENIED=*/1, 1);
+}
+
+TEST_F(StorageAccessGrantPermissionContextTest, ExplicitGrantAccept) {
+  base::test::ScopedFeatureList scoped_enable;
+  scoped_enable.InitAndEnableFeature(blink::features::kStorageAccessAPI);
+
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 0);
+  histogram_tester.ExpectTotalCount(kPromptResultHistogram, 0);
+
+  StorageAccessGrantPermissionContext permission_context(profile());
+  permissions::PermissionRequestID fake_id(
+      /*render_process_id=*/0, /*render_frame_id=*/0, /*request_id=*/0);
+
+  const GURL requesting_origin_1(kAlternateURL);
+  const GURL requesting_origin_2(kInsecureURL);
+  const GURL embedding_origin(kSecureURL);
+
+  // Ensure all our implicit grants are taken care of for |requesting_origin_1|
+  // before we proceed to validate.
+  ExhaustImplicitGrants(requesting_origin_1, permission_context);
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
+
+  ContentSetting result = CONTENT_SETTING_DEFAULT;
+  permission_context.DecidePermission(
+      web_contents(), fake_id, requesting_origin_1, embedding_origin,
+      /*user_gesture=*/true, base::BindOnce(&SaveResult, &result));
+  base::RunLoop().RunUntilIdle();
+
+  // We should get a prompt showing up right now.
+  permissions::PermissionRequestManager* manager =
+      permissions::PermissionRequestManager::FromWebContents(web_contents());
+  DCHECK(manager);
+  EXPECT_TRUE(manager->IsRequestInProgress());
+
+  // Accept the prompt and validate we get the expected setting back in our
+  // callback.
+  manager->Accept();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(CONTENT_SETTING_ALLOW, result);
+
+  histogram_tester.ExpectTotalCount(kGrantIsImplicitHistogram, 6);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*implicit_grant=*/1, 5);
+  histogram_tester.ExpectBucketCount(kGrantIsImplicitHistogram,
+                                     /*explicit_grant=*/0, 1);
+  histogram_tester.ExpectTotalCount(kPromptResultHistogram, 1);
+  histogram_tester.ExpectBucketCount(kPromptResultHistogram,
+                                     /*GRANTED=*/0, 1);
 }
diff --git a/components/content_settings/core/browser/cookie_settings.cc b/components/content_settings/core/browser/cookie_settings.cc
@@ -208,6 +208,11 @@ void CookieSettings::GetCookieSettingInternal(
   ContentSetting setting = ValueToContentSetting(value.get());
   bool block = block_third && is_third_party_request;
 
+  if (!block) {
+    FireStorageAccessHistogram(
+        net::cookie_util::StorageAccessResult::ACCESS_ALLOWED);
+  }
+
 #if !defined(OS_IOS)
   // IOS doesn't use blink and as such cannot check our feature flag. Disabling
   // by default there should be no-op as the lack of Blink also means no grants
@@ -221,11 +226,19 @@ void CookieSettings::GetCookieSettingInternal(
         url, first_party_url, ContentSettingsType::STORAGE_ACCESS,
         std::string());
 
-    if (setting == CONTENT_SETTING_ALLOW)
+    if (setting == CONTENT_SETTING_ALLOW) {
       block = false;
+      FireStorageAccessHistogram(net::cookie_util::StorageAccessResult::
+                                     ACCESS_ALLOWED_STORAGE_ACCESS_GRANT);
+    }
   }
 #endif
 
+  if (block) {
+    FireStorageAccessHistogram(
+        net::cookie_util::StorageAccessResult::ACCESS_BLOCKED);
+  }
+
   *cookie_setting = block ? CONTENT_SETTING_BLOCK : setting;
 }
 

diff --git a/components/content_settings/core/browser/cookie_settings_unittest.cc b/components/content_settings/core/browser/cookie_settings_unittest.cc
@@ -7,6 +7,7 @@
 #include <cstddef>
 
 #include "base/scoped_observer.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/values.h"
@@ -22,11 +23,16 @@
 #include "extensions/buildflags/buildflags.h"
 #include "net/base/features.h"
 #include "net/cookies/cookie_constants.h"
+#include "net/cookies/cookie_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 #if !defined(OS_IOS)
 #include "third_party/blink/public/common/features.h"
+namespace {
+constexpr char kAllowedRequestsHistogram[] =
+    "API.StorageAccess.AllowedRequests";
+}
 #endif
 
 namespace content_settings {
@@ -425,6 +431,25 @@ TEST_F(CookieSettingsTest, CookiesBlockEverythingExceptAllowed) {
 }
 
 #if !defined(OS_IOS)
+TEST_F(CookieSettingsTest, GetCookieSettingAllowedTelemetry) {
+  const GURL top_level_url = GURL(kFirstPartySite);
+  const GURL url = GURL(kAllowedSite);
+
+  prefs_.SetBoolean(prefs::kBlockThirdPartyCookies, false);
+
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 0);
+
+  ContentSetting setting;
+  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
+  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
+  histogram_tester.ExpectBucketCount(
+      kAllowedRequestsHistogram,
+      static_cast<int>(net::cookie_util::StorageAccessResult::ACCESS_ALLOWED),
+      1);
+}
+
 // When the Storage Access API is disabled, block third party cookie setting
 // should behave like normal.
 TEST_F(CookieSettingsTest, GetCookieSettingDisabledSAA) {
@@ -453,6 +478,9 @@ TEST_F(CookieSettingsTest, GetCookieSettingDefaultSAA) {
   const GURL top_level_url = GURL(kFirstPartySite);
   const GURL url = GURL(kAllowedSite);
 
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 0);
+
   prefs_.SetBoolean(prefs::kBlockThirdPartyCookies, true);
 
   settings_map_->SetContentSettingCustomScope(
@@ -464,6 +492,11 @@ TEST_F(CookieSettingsTest, GetCookieSettingDefaultSAA) {
   ContentSetting setting;
   cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
   EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
+  histogram_tester.ExpectBucketCount(
+      kAllowedRequestsHistogram,
+      static_cast<int>(net::cookie_util::StorageAccessResult::ACCESS_BLOCKED),
+      1);
 }
 
 // When enabled, the Storage Access API should unblock storage access that would
@@ -476,6 +509,9 @@ TEST_F(CookieSettingsTest, GetCookieSettingEnabledSAA) {
   const GURL url = GURL(kAllowedSite);
   const GURL third_url = GURL(kBlockedSite);
 
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 0);
+
   prefs_.SetBoolean(prefs::kBlockThirdPartyCookies, true);
 
   settings_map_->SetContentSettingCustomScope(
@@ -490,6 +526,12 @@ TEST_F(CookieSettingsTest, GetCookieSettingEnabledSAA) {
   ContentSetting setting;
   cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
   EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
+  histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
+  histogram_tester.ExpectBucketCount(
+      kAllowedRequestsHistogram,
+      static_cast<int>(net::cookie_util::StorageAccessResult::
+                           ACCESS_ALLOWED_STORAGE_ACCESS_GRANT),
+      1);
 
   // Invalid pair the |top_level_url| granting access to |url| is now
   // being loaded under |url| as the top level url.

diff --git a/components/page_load_metrics/browser/observers/use_counter/ukm_features.cc b/components/page_load_metrics/browser/observers/use_counter/ukm_features.cc
@@ -170,6 +170,8 @@ UseCounterPageLoadMetricsObserver::GetAllowedUkmFeatures() {
           WebFeature::kSchemefulSameSiteContextDowngrade,
           WebFeature::kIdleDetectionStart,
           WebFeature::kPerformanceObserverEntryTypesAndBuffered,
+          WebFeature::kStorageAccessAPI_HasStorageAccess_Method,
+          WebFeature::kStorageAccessAPI_requestStorageAccess_Method,
       }));
   return *opt_in_features;
 }