[DBSC] Update TokenBindingResponseEncryptionError histograms

This change affects two histograms that record the same enum value:
- Signin.OAuth2MintToken.BoundFetchEncryptionError
- Signin.OAuthMultiloginResponseEncryptionError

To have a better metric baseline, this CL adds non-error cases to these
histograms.

Also, this CL fixes an issue with unbalanced bucket recording in
Signin.OAuthMultiloginResponseEncryptionError. All buckets are now
recorded per cookie.

The histograms have only been recorded in canary/dev, so it's still fine
to change modify recorded values.

Bug: 365492729
Change-Id: I0a3ac3dceed5b9cb7b0be7ce4b9b8c4856ac0d34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097129
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1397468}

google_apis/gaia/oauth2_mint_access_token_fetcher_adapter.cc

@@ -122,9 +122,14 @@ void OAuth2MintAccessTokenFetcherAdapter::OnMintTokenSuccess(
           GoogleServiceAuthError::FromUnexpectedServiceResponse(
               "Failed to decrypt token"));
       return;
+    } else {
+      RecordEncryptionError(
+          TokenBindingResponseEncryptionError::kSuccessfullyDecrypted);
     }
     decrypted_token = std::move(decryption_result);
   } else {
+    RecordEncryptionError(
+        TokenBindingResponseEncryptionError::kSuccessNoEncryption);
     decrypted_token = std::move(result.access_token);
   }
   // The token will expire in `time_to_live` seconds. Take a 10% error margin to

google_apis/gaia/oauth2_mint_access_token_fetcher_adapter_unittest.cc

@@ -267,6 +267,10 @@ TEST_F(OAuth2MintAccessTokenFetcherAdapterTest, SuccessWithEncryption) {
   histogram_tester().ExpectUniqueSample(kFetchAuthErrorHistogram,
                                         GoogleServiceAuthError::NONE,
                                         /*expected_bucket_count=*/1);
+  histogram_tester().ExpectUniqueSample(
+      kFetchEncryptionErrorHistogram,
+      TokenBindingResponseEncryptionError::kSuccessfullyDecrypted,
+      /*expected_bucket_count=*/1);
 }
 
 TEST_F(OAuth2MintAccessTokenFetcherAdapterTest, SuccessDecryptorUnused) {
@@ -285,6 +289,10 @@ TEST_F(OAuth2MintAccessTokenFetcherAdapterTest, SuccessDecryptorUnused) {
   histogram_tester().ExpectUniqueSample(kFetchAuthErrorHistogram,
                                         GoogleServiceAuthError::NONE,
                                         /*expected_bucket_count=*/1);
+  histogram_tester().ExpectUniqueSample(
+      kFetchEncryptionErrorHistogram,
+      TokenBindingResponseEncryptionError::kSuccessNoEncryption,
+      /*expected_bucket_count=*/1);
 }
 
 TEST_F(OAuth2MintAccessTokenFetcherAdapterTest, Failure) {

google_apis/gaia/oauth_multilogin_result.cc

@@ -124,8 +124,11 @@ void OAuthMultiloginResult::TryParseCookiesFromValue(
       json_value.Find("token_binding_directed_response") != nullptr;
   if (are_cookies_encrypted && cookie_decryptor.is_null()) {
     VLOG(1) << "The response unexpectedly contains encrypted cookies";
-    RecordMultiloginResponseEncryptionError(
-        TokenBindingResponseEncryptionError::kResponseUnexpectedlyEncrypted);
+    for ([[maybe_unused]] const auto& unused : *cookie_list) {
+      // Record the histogram once per cookie for parity with other buckets.
+      RecordMultiloginResponseEncryptionError(
+          TokenBindingResponseEncryptionError::kResponseUnexpectedlyEncrypted);
+    }
     status_ = OAuthMultiloginResponseStatus::kUnknownStatus;
     return;
   }
@@ -150,7 +153,13 @@ void OAuthMultiloginResult::TryParseCookiesFromValue(
         RecordMultiloginResponseEncryptionError(
             TokenBindingResponseEncryptionError::kDecryptionFailed);
         continue;
+      } else {
+        RecordMultiloginResponseEncryptionError(
+            TokenBindingResponseEncryptionError::kSuccessfullyDecrypted);
       }
+    } else {
+      RecordMultiloginResponseEncryptionError(
+          TokenBindingResponseEncryptionError::kSuccessNoEncryption);
     }
 
     base::Time now = base::Time::Now();

google_apis/gaia/oauth_multilogin_result_unittest.cc

@@ -89,6 +89,17 @@ constexpr char kResponseWithEncryptedCookiesFormat[] =
               "priority":"HIGH",
               "maxAge":63070000,
               "sameSite":"Lax"
+            },
+            {
+              "name":"APISID",
+              "value":"%s",
+              "host":"google.com",
+              "path":"/",
+              "isSecure":false,
+              "isHttpOnly":true,
+              "priority":"HIGH",
+              "maxAge":63070000,
+              "sameSite":"Lax"
             }
           ]
         }
@@ -899,7 +910,7 @@ TEST(OAuthMultiloginResultTest,
 TEST(OAuthMultiloginResultTest, ParseEncryptedCookies) {
   base::HistogramTester histogram_tester;
   std::string response = base::StringPrintf(kResponseWithEncryptedCookiesFormat,
-                                            "vAlUe1", "vAlUe2");
+                                            "vAlUe1", "vAlUe2", "vAlUe3");
   auto decryptor = [](std::string_view encrypted_cookie) {
     return base::StrCat({encrypted_cookie, ".decrypted"});
   };
@@ -910,8 +921,12 @@ TEST(OAuthMultiloginResultTest, ParseEncryptedCookies) {
   EXPECT_THAT(
       result.cookies(),
       ElementsAre(Property(&CanonicalCookie::Value, "vAlUe1.decrypted"),
-                  Property(&CanonicalCookie::Value, "vAlUe2.decrypted")));
-  histogram_tester.ExpectTotalCount(kEncryptionErrorHistogram, 0);
+                  Property(&CanonicalCookie::Value, "vAlUe2.decrypted"),
+                  Property(&CanonicalCookie::Value, "vAlUe3.decrypted")));
+  histogram_tester.ExpectUniqueSample(
+      kEncryptionErrorHistogram,
+      TokenBindingResponseEncryptionError::kSuccessfullyDecrypted,
+      /*expected_bucket_count=*/3);
 }
 
 // Decryptor is ignored if cookies in the response are not encrypted.
@@ -945,10 +960,14 @@ TEST(OAuthMultiloginResultTest, ParseCookiesIgnoresDecryptor) {
           ]
         }
       )";
+  base::HistogramTester histogram_tester;
   std::string response = base::StringPrintf(
       kResponseWithNonEncryptedCookiesFormat, "vAlUe1", "vAlUe2");
   auto decryptor = [](std::string_view encrypted_cookie) {
-    return base::StrCat({encrypted_cookie, ".decrypted"});
+    ADD_FAILURE()
+        << "Decryptor was called unexpectedly for cookie with value \""
+        << encrypted_cookie << "\"";
+    return std::string();
   };
 
   OAuthMultiloginResult result(response, net::HTTP_OK,
@@ -957,13 +976,17 @@ TEST(OAuthMultiloginResultTest, ParseCookiesIgnoresDecryptor) {
   EXPECT_THAT(result.cookies(),
               ElementsAre(Property(&CanonicalCookie::Value, "vAlUe1"),
                           Property(&CanonicalCookie::Value, "vAlUe2")));
+  histogram_tester.ExpectUniqueSample(
+      kEncryptionErrorHistogram,
+      TokenBindingResponseEncryptionError::kSuccessNoEncryption,
+      /*expected_bucket_count=*/2);
 }
 
 // Result is set to failure if cookies are encrypted but a decryptor is not set.
 TEST(OAuthMultiloginResultTest, ParseEncryptedCookiesFailsWithoutDecryptor) {
   base::HistogramTester histogram_tester;
   std::string response = base::StringPrintf(kResponseWithEncryptedCookiesFormat,
-                                            "vAlUe1", "vAlUe2");
+                                            "vAlUe1", "vAlUe2", "vAlUe3");
 
   OAuthMultiloginResult result(response, net::HTTP_OK, base::NullCallback());
 
@@ -972,16 +995,16 @@ TEST(OAuthMultiloginResultTest, ParseEncryptedCookiesFailsWithoutDecryptor) {
   histogram_tester.ExpectUniqueSample(
       kEncryptionErrorHistogram,
       TokenBindingResponseEncryptionError::kResponseUnexpectedlyEncrypted,
-      /*expected_bucket_count=*/1);
+      /*expected_bucket_count=*/3);
 }
 
 // Cookies that failed to decrypt are omitted from the result.
 TEST(OAuthMultiloginResultTest, ParseEncryptedCookiesDecryptionFails) {
   base::HistogramTester histogram_tester;
   std::string response = base::StringPrintf(kResponseWithEncryptedCookiesFormat,
-                                            "vAlUe1", "vAlUe2");
+                                            "vAlUe1", "vAlUe2", "vAlUe3");
   auto decryptor = [](std::string_view encrypted_cookie) {
-    return encrypted_cookie == "vAlUe1"
+    return encrypted_cookie != "vAlUe2"
                ? std::string()
                : base::StrCat({encrypted_cookie, ".decrypted"});
   };
@@ -991,8 +1014,12 @@ TEST(OAuthMultiloginResultTest, ParseEncryptedCookiesDecryptionFails) {
 
   EXPECT_THAT(result.cookies(), ElementsAre(Property(&CanonicalCookie::Value,
                                                      "vAlUe2.decrypted")));
-  histogram_tester.ExpectUniqueSample(
-      kEncryptionErrorHistogram,
-      TokenBindingResponseEncryptionError::kDecryptionFailed,
-      /*expected_bucket_count=*/1);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples(kEncryptionErrorHistogram),
+      ElementsAre(
+          base::Bucket(TokenBindingResponseEncryptionError::kDecryptionFailed,
+                       /*count=*/2),
+          base::Bucket(
+              TokenBindingResponseEncryptionError::kSuccessfullyDecrypted,
+              /*count=*/1)));
 }

google_apis/gaia/token_binding_response_encryption_error.h

@@ -13,7 +13,9 @@
 enum class TokenBindingResponseEncryptionError {
   kResponseUnexpectedlyEncrypted = 0,
   kDecryptionFailed = 1,
-  kMaxValue = kDecryptionFailed
+  kSuccessfullyDecrypted = 2,
+  kSuccessNoEncryption = 3,
+  kMaxValue = kSuccessNoEncryption
 };
 // LINT.ThenChange(//tools/metrics/histograms/metadata/signin/enums.xml:TokenBindingResponseEncryptionError)
 

tools/metrics/histograms/metadata/signin/enums.xml

@@ -1130,6 +1130,8 @@ chromium-metrics-reviews@google.com.
 <enum name="TokenBindingResponseEncryptionError">
   <int value="0" label="Response is unexpectedly encrypted"/>
   <int value="1" label="Decryption failed"/>
+  <int value="2" label="No error. Response was successfully decrypted"/>
+  <int value="3" label="No error. Response wasn't encrypted"/>
 </enum>
 
 <!-- LINT.ThenChange(//google_apis/gaia/token_binding_response_encryption_error.h:TokenBindingResponseEncryptionError) -->

tools/metrics/histograms/metadata/signin/histograms.xml

@@ -1924,9 +1924,9 @@ the browser and content area. -->
   <owner>droger@chromium.org</owner>
   <owner>chrome-signin-team@google.com</owner>
   <summary>
-    Records an encryption failure that occured during OAuth2MintToken API fetch
-    with a bound refresh token. Recorded after the response is processed and
-    only if the fetch failed due to such an error. Desktop only.
+    Records an encryption failure (or lack of any) that occured during
+    OAuth2MintToken API fetch with a bound refresh token. Recorded after the
+    response is processed. Desktop only.
   </summary>
 </histogram>
 
@@ -1960,9 +1960,9 @@ tokens and is needed for as long as Chrome is fetching access tokens. -->
   <owner>alexilin@chromium.org</owner>
   <owner>chrome-signin-team@google.com</owner>
   <summary>
-    Records an encryption failure that occured during OAuthMultilogin API fetch
-    with a bound refresh token. Recorded while the response is being processed.
-    Desktop only.
+    Records an encryption failure (or lack of any) that occured during
+    OAuthMultilogin API fetch. Recorded once per cookie while the response is
+    being processed.
   </summary>
 </histogram>