Reland "Add metrics to InsecureCredentialsManager"

This is a reland of d0bda2ee.
This is the same as the initial CL except logging of the password
length.

Original change's description:
> Add metrics to InsecureCredentialsManager
>
> This CL adds metrics about passwords weakness check.
>
> Bug: 1119752
> Change-Id: I20ff20dde480c84ebd5d252ce7684cecc4fc4c87
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2434336
> Commit-Queue: Irina Fedorova <irfedorova@google.com>
> Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#812625}

TBR=rkaplow

Bug: 1119752, 1134152
Change-Id: Iecfabdcfa32b687b53e00a9e147b6695e4610327
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2442734Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812810}

components/password_manager/core/browser/ui/insecure_credentials_manager.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/containers/flat_set.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/ranges/algorithm.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ -242,7 +243,7 @@ void InsecureCredentialsManager::StartWeakCheck() {
       base::BindOnce(&BulkWeakCheck,
                      ExtractPasswords(presenter_->GetSavedPasswords())),
       base::BindOnce(&InsecureCredentialsManager::OnWeakCheckDone,
-                     weak_ptr_factory_.GetWeakPtr()));
+                     weak_ptr_factory_.GetWeakPtr(), base::ElapsedTimer()));
 }
 
 void InsecureCredentialsManager::SaveCompromisedCredential(
@@ -338,12 +339,15 @@ void InsecureCredentialsManager::RemoveObserver(Observer* observer) {
 }
 
 void InsecureCredentialsManager::OnWeakCheckDone(
+    base::ElapsedTimer timer_since_weak_check_start,
     base::flat_set<base::string16> weak_passwords) {
   weak_passwords_ = std::move(weak_passwords);
 
   credentials_to_forms_ = JoinInsecureCredentialsWithSavedPasswords(
       compromised_credentials_, weak_passwords_,
       presenter_->GetSavedPasswords());
+  base::UmaHistogramTimes("PasswordManager.WeakCheck.Time",
+                          timer_since_weak_check_start.Elapsed());
   NotifyWeakCredentialsChanged();
 }
 

components/password_manager/core/browser/ui/insecure_credentials_manager.h

@@ -14,6 +14,7 @@
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "base/scoped_observer.h"
+#include "base/timer/elapsed_timer.h"
 #include "base/util/type_safety/strong_alias.h"
 #include "components/password_manager/core/browser/compromised_credentials_consumer.h"
 #include "components/password_manager/core/browser/compromised_credentials_table.h"
@@ -199,7 +200,8 @@ class InsecureCredentialsManager
 
   // Updates |weak_passwords| set and notifies observers that weak credentials
   // were changed.
-  void OnWeakCheckDone(base::flat_set<base::string16> weak_passwords);
+  void OnWeakCheckDone(base::ElapsedTimer timer_since_weak_check_start,
+                       base::flat_set<base::string16> weak_passwords);
 
   // CompromisedCredentialsReader::Observer:
   void OnCompromisedCredentialsChanged(

components/password_manager/core/browser/ui/insecure_credentials_manager_unittest.cc

@@ -7,7 +7,9 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece_forward.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
+#include "base/timer/elapsed_timer.h"
 #include "components/password_manager/core/browser/compromised_credentials_table.h"
 #include "components/password_manager/core/browser/password_form.h"
 #include "components/password_manager/core/browser/test_password_store.h"
@@ -38,6 +40,9 @@ using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::IsEmpty;
 
+// Delay in milliseconds.
+const int kDelay = 2;
+
 struct MockInsecureCredentialsManagerObserver
     : InsecureCredentialsManager::Observer {
   MOCK_METHOD(void,
@@ -136,9 +141,14 @@ class InsecureCredentialsManagerTest : public ::testing::Test {
     return std::string();
   }
 
+  base::HistogramTester& histogram_tester() { return histogram_tester_; }
+
+  void AdvanceClock(base::TimeDelta time) { task_env_.AdvanceClock(time); }
+
  private:
   base::test::TaskEnvironment task_env_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  base::HistogramTester histogram_tester_;
   scoped_refptr<TestPasswordStore> store_ =
       base::MakeRefCounted<TestPasswordStore>();
   SavedPasswordsPresenter presenter_{store_};
@@ -498,6 +508,26 @@ TEST_F(InsecureCredentialsManagerTest, MapCompromisedPasswordsToPasswords) {
               ElementsAreArray(store().stored_passwords().at(kExampleOrg)));
 }
 
+TEST_F(InsecureCredentialsManagerTest, StartWeakCheckOnEmptyPasswordsList) {
+  EXPECT_THAT(
+      histogram_tester().GetTotalCountsForPrefix("PasswordManager.WeakCheck"),
+      IsEmpty());
+
+  RunUntilIdle();
+  provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
+  RunUntilIdle();
+
+  EXPECT_THAT(provider().GetWeakCredentials(), IsEmpty());
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 0, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 0, 1);
+}
+
 TEST_F(InsecureCredentialsManagerTest, WeakCredentialsNotFound) {
   std::vector<PasswordForm> passwords = {
       MakeSavedPassword(kExampleCom, kUsername1, kStrongPassword1),
@@ -505,12 +535,25 @@ TEST_F(InsecureCredentialsManagerTest, WeakCredentialsNotFound) {
 
   store().AddLogin(passwords[0]);
   store().AddLogin(passwords[1]);
+  EXPECT_THAT(
+      histogram_tester().GetTotalCountsForPrefix("PasswordManager.WeakCheck"),
+      IsEmpty());
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(2 * kDelay));
   RunUntilIdle();
 
   EXPECT_THAT(provider().GetWeakCredentials(), IsEmpty());
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 2, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        2 * kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 0, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.PasswordScore", 4, 2);
 }
 
 TEST_F(InsecureCredentialsManagerTest, DetectedWeakCredential) {
@@ -520,9 +563,13 @@ TEST_F(InsecureCredentialsManagerTest, DetectedWeakCredential) {
 
   store().AddLogin(passwords[0]);
   store().AddLogin(passwords[1]);
+  EXPECT_THAT(
+      histogram_tester().GetTotalCountsForPrefix("PasswordManager.WeakCheck"),
+      IsEmpty());
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
   RunUntilIdle();
 
   std::vector<CredentialWithPassword> weak_credentials =
@@ -531,6 +578,15 @@ TEST_F(InsecureCredentialsManagerTest, DetectedWeakCredential) {
   ASSERT_EQ(weak_credentials.size(), 1u);
   EXPECT_EQ(base::UTF16ToUTF8(weak_credentials[0].password), kWeakPassword1);
   EXPECT_TRUE(IsWeak(weak_credentials[0].insecure_type));
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 2, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 1, 1);
+  histogram_tester().ExpectTotalCount("PasswordManager.WeakCheck.PasswordScore",
+                                      2);
 }
 
 // Tests that credentials with the same signon_realm and username, but different
@@ -546,6 +602,7 @@ TEST_F(InsecureCredentialsManagerTest,
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
   RunUntilIdle();
 
   std::vector<CredentialWithPassword> weak_credentials =
@@ -554,6 +611,15 @@ TEST_F(InsecureCredentialsManagerTest,
   ASSERT_EQ(weak_credentials.size(), 2u);
   EXPECT_TRUE(IsWeak(weak_credentials[0].insecure_type));
   EXPECT_TRUE(IsWeak(weak_credentials[1].insecure_type));
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 2, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 2, 1);
+  histogram_tester().ExpectTotalCount("PasswordManager.WeakCheck.PasswordScore",
+                                      2);
 }
 
 // Tests that credentials with the same signon_realm, username and passwords
@@ -569,6 +635,7 @@ TEST_F(InsecureCredentialsManagerTest,
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
   RunUntilIdle();
 
   std::vector<CredentialWithPassword> weak_credentials =
@@ -577,6 +644,15 @@ TEST_F(InsecureCredentialsManagerTest,
   ASSERT_EQ(weak_credentials.size(), 1u);
   EXPECT_EQ(base::UTF16ToUTF8(weak_credentials[0].password), kWeakPassword1);
   EXPECT_TRUE(IsWeak(weak_credentials[0].insecure_type));
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 1, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 1, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.PasswordScore", 0, 1);
 }
 
 TEST_F(InsecureCredentialsManagerTest, BothWeakAndCompromisedCredentialsExist) {
@@ -594,6 +670,7 @@ TEST_F(InsecureCredentialsManagerTest, BothWeakAndCompromisedCredentialsExist) {
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
   RunUntilIdle();
 
   std::vector<CredentialWithPassword> returned_weak_credentials =
@@ -609,6 +686,15 @@ TEST_F(InsecureCredentialsManagerTest, BothWeakAndCompromisedCredentialsExist) {
   ASSERT_EQ(returned_compromised_credentials.size(), 2u);
   EXPECT_TRUE(IsCompromised(returned_compromised_credentials[0].insecure_type));
   EXPECT_TRUE(IsCompromised(returned_compromised_credentials[1].insecure_type));
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 2, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 1, 1);
+  histogram_tester().ExpectTotalCount("PasswordManager.WeakCheck.PasswordScore",
+                                      2);
 }
 
 // Checks that for a credential that is both weak and compromised,
@@ -625,6 +711,7 @@ TEST_F(InsecureCredentialsManagerTest, SingleCredentialIsWeakAndCompromised) {
 
   RunUntilIdle();
   provider().StartWeakCheck();
+  AdvanceClock(base::TimeDelta::FromMilliseconds(kDelay));
   RunUntilIdle();
 
   std::vector<CredentialWithPassword> returned_weak_credentials =
@@ -646,6 +733,15 @@ TEST_F(InsecureCredentialsManagerTest, SingleCredentialIsWeakAndCompromised) {
             kWeakPassword1);
   EXPECT_TRUE(IsWeak(returned_compromised_credentials[0].insecure_type));
   EXPECT_TRUE(IsCompromised(returned_compromised_credentials[0].insecure_type));
+
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.CheckedPasswords", 1, 1);
+  histogram_tester().ExpectUniqueSample("PasswordManager.WeakCheck.Time",
+                                        kDelay, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.WeakPasswords", 1, 1);
+  histogram_tester().ExpectUniqueSample(
+      "PasswordManager.WeakCheck.PasswordScore", 0, 1);
 }
 
 // Test verifies that saving LeakCheckCredential via provider adds expected

components/password_manager/core/browser/ui/weak_check_utility.cc

@@ -4,6 +4,7 @@
 
 #include "components/password_manager/core/browser/ui/weak_check_utility.h"
 
+#include "base/metrics/histogram_functions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/zxcvbn-cpp/native-src/zxcvbn/matching.hpp"
 #include "third_party/zxcvbn-cpp/native-src/zxcvbn/scoring.hpp"
@@ -13,6 +14,17 @@ namespace password_manager {
 
 namespace {
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class PasswordWeaknessScore {
+  kTooGuessablePassword = 0,
+  kVeryGuessablePassword = 1,
+  kSomewhatGuessablePassword = 2,
+  kSafelyUnguessablePassword = 3,
+  kVeryUnguessablePassword = 4,
+  kMaxValue = kVeryUnguessablePassword,
+};
+
 // Passwords longer than this constant should not be checked for weakness using
 // the zxcvbn-cpp library. This is because the runtime grows extremely, starting
 // at a password length of 40.
@@ -33,16 +45,25 @@ int PasswordWeakCheck(base::StringPiece16 password16) {
   std::vector<zxcvbn::Match> matches = zxcvbn::omnimatch(password);
   zxcvbn::ScoringResult result =
       zxcvbn::most_guessable_match_sequence(password, matches);
-  return zxcvbn::estimate_attack_times(result.guesses).score;
+
+  int score = zxcvbn::estimate_attack_times(result.guesses).score;
+  base::UmaHistogramEnumeration("PasswordManager.WeakCheck.PasswordScore",
+                                static_cast<PasswordWeaknessScore>(score));
+  return score;
 }
 
 }  // namespace
 
 base::flat_set<base::string16> BulkWeakCheck(
     base::flat_set<base::string16> passwords) {
+  base::UmaHistogramCounts1000("PasswordManager.WeakCheck.CheckedPasswords",
+                               passwords.size());
   base::EraseIf(passwords, [](const auto& password) {
     return kLowSeverityScore < PasswordWeakCheck(password);
   });
+
+  base::UmaHistogramCounts1000("PasswordManager.WeakCheck.WeakPasswords",
+                               passwords.size());
   return passwords;
 }
 

tools/metrics/histograms/enums.xml

@@ -56287,6 +56287,14 @@ Called by update_net_trust_anchors.py.-->
   <int value="4" label="DB Error"/>
 </enum>
 
+<enum name="PasswordWeaknessScore">
+  <int value="0" label="Too guessable password"/>
+  <int value="1" label="Very guessable password"/>
+  <int value="2" label="Somewhat guessable password"/>
+  <int value="3" label="Safely unguessable password"/>
+  <int value="4" label="Very unguessable password"/>
+</enum>
+
 <enum name="PaymentRequestAbortReason">
   <int value="0" label="DismissedByUser"/>
   <int value="1" label="AbortedByMerchant"/>

tools/metrics/histograms/histograms_xml/password/histograms.xml

@@ -2156,6 +2156,40 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="PasswordManager.WeakCheck.CheckedPasswords" units="passwords"
+    expires_after="M92">
+  <owner>jdoerrie@chromium.org</owner>
+  <owner>vasilii@chromium.org</owner>
+  <summary>
+    The number of passwords analyzed during the passwords weak check.
+  </summary>
+</histogram>
+
+<histogram name="PasswordManager.WeakCheck.PasswordScore"
+    enum="PasswordWeaknessScore" expires_after="M92">
+  <owner>jdoerrie@chromium.org</owner>
+  <owner>vasilii@chromium.org</owner>
+  <summary>
+    The score of the password that was checked by the passwords weak check. The
+    score indicates how guessable the password is.
+  </summary>
+</histogram>
+
+<histogram name="PasswordManager.WeakCheck.Time" units="ms" expires_after="M92">
+  <owner>jdoerrie@chromium.org</owner>
+  <owner>vasilii@chromium.org</owner>
+  <summary>The time it took to complete the passwords weak check.</summary>
+</histogram>
+
+<histogram name="PasswordManager.WeakCheck.WeakPasswords" units="passwords"
+    expires_after="M92">
+  <owner>jdoerrie@chromium.org</owner>
+  <owner>vasilii@chromium.org</owner>
+  <summary>
+    The number of weak passwords found when the passwords weak check completed.
+  </summary>
+</histogram>
+
 <histogram
     name="PasswordManager.WellKnownChangePassword.GetChangePasswordUsage"
     enum="GetChangePasswordUrlMetric" expires_after="2021-01-31">