Add some UMA stats to in_session_password_change_manager

Bug: 930109 Change-Id: I142b1754a8fc3fa52d0d9c1994da513c01c6ff67 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730162 Commit-Queue: A Olsen <olsen@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org> Cr-Commit-Position: refs/heads/master@{#685142}
chromium · Aug 8, 2019 · 7929307 · 7929307
1 parent 7311f5f
commit 7929307
Show file tree

Hide file tree

Showing 5 changed files with 146 additions and 14 deletions.
diff --git a/chrome/browser/chromeos/login/saml/in_session_password_change_manager.cc b/chrome/browser/chromeos/login/saml/in_session_password_change_manager.cc
@@ -7,6 +7,7 @@
 #include "ash/public/cpp/session/session_activation_observer.h"
 #include "ash/public/cpp/session/session_controller.h"
 #include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "chrome/browser/browser_process.h"
@@ -28,6 +29,75 @@ namespace chromeos {
 
 namespace {
 
+using PasswordSource = InSessionPasswordChangeManager::PasswordSource;
+
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused. This must be kept in sync with
+// SamlInSessionPasswordChangeEvent in tools/metrics/histogram/enums.xml
+enum class InSessionPasswordChangeEvent {
+  kManagerCreated = 0,
+  kNotified = 1,
+  kUrgentNotified = 2,
+  kNotifiedAlreadyExpired = 3,
+  kStartPasswordChange = 4,
+  kSamlPasswordChanged = 5,
+  kPasswordScrapeSuccess = 6,
+  kPasswordScrapePartial = 7,
+  kPasswordScrapeFailure = 8,
+  kCryptohomePasswordChangeSuccessScraped = 9,
+  kCryptohomePasswordChangeSuccessRetyped = 10,
+  kCryptohomePasswordChangeFailureScraped = 11,
+  kCryptohomePasswordChangeFailureRetyped = 12,
+  kFinishPasswordChange = 13,
+  kMaxValue = kFinishPasswordChange,
+};
+
+void RecordEvent(InSessionPasswordChangeEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("ChromeOS.SAML.InSessionPasswordChangeEvent",
+                            event);
+}
+
+void RecordNotificationShown(bool is_expired, bool show_as_urgent) {
+  if (is_expired) {
+    RecordEvent(InSessionPasswordChangeEvent::kNotifiedAlreadyExpired);
+  } else if (show_as_urgent) {
+    RecordEvent(InSessionPasswordChangeEvent::kUrgentNotified);
+  } else {
+    RecordEvent(InSessionPasswordChangeEvent::kNotified);
+  }
+}
+
+void RecordScrapingResult(bool old_password_scraped,
+                          bool new_password_scraped) {
+  if (old_password_scraped && new_password_scraped) {
+    RecordEvent(InSessionPasswordChangeEvent::kPasswordScrapeSuccess);
+  } else if (old_password_scraped || new_password_scraped) {
+    RecordEvent(InSessionPasswordChangeEvent::kPasswordScrapePartial);
+  } else {
+    RecordEvent(InSessionPasswordChangeEvent::kPasswordScrapeFailure);
+  }
+}
+
+void RecordCryptohomePasswordChangeSuccess(PasswordSource password_source) {
+  if (password_source == PasswordSource::PASSWORDS_SCRAPED) {
+    RecordEvent(
+        InSessionPasswordChangeEvent::kCryptohomePasswordChangeSuccessScraped);
+  } else {
+    RecordEvent(
+        InSessionPasswordChangeEvent::kCryptohomePasswordChangeSuccessRetyped);
+  }
+}
+
+void RecordCryptohomePasswordChangeFailure(PasswordSource password_source) {
+  if (password_source == PasswordSource::PASSWORDS_SCRAPED) {
+    RecordEvent(
+        InSessionPasswordChangeEvent::kCryptohomePasswordChangeFailureScraped);
+  } else {
+    RecordEvent(
+        InSessionPasswordChangeEvent::kCryptohomePasswordChangeFailureRetyped);
+  }
+}
+
 InSessionPasswordChangeManager* g_test_instance = nullptr;
 
 // Traits for running RecheckPasswordExpiryTask.
@@ -94,6 +164,7 @@ InSessionPasswordChangeManager::CreateIfEnabled(Profile* primary_profile) {
     std::unique_ptr<InSessionPasswordChangeManager> manager =
         std::make_unique<InSessionPasswordChangeManager>(primary_profile);
     manager->MaybeShowExpiryNotification();
+    RecordEvent(InSessionPasswordChangeEvent::kManagerCreated);
     return manager;
   } else {
     // If the policy is disabled, clear the SAML password attributes.
@@ -181,6 +252,7 @@ void InSessionPasswordChangeManager::MaybeShowExpiryNotification() {
     } else {
       ShowStandardExpiryNotification(time_until_expiry);
     }
+    RecordNotificationShown(is_expired, show_as_urgent);
 
     // We check again whether to reshow / update the notification after one day:
     recheck_task_.RecheckAfter(kOneDay);
@@ -230,7 +302,8 @@ void InSessionPasswordChangeManager::OnScreenUnlocked() {
 }
 
 void InSessionPasswordChangeManager::StartInSessionPasswordChange() {
-  NotifyObservers(START_SAML_IDP_PASSWORD_CHANGE);
+  RecordEvent(InSessionPasswordChangeEvent::kStartPasswordChange);
+  NotifyObservers(Event::START_SAML_IDP_PASSWORD_CHANGE);
   DismissExpiryNotification();
   PasswordChangeDialog::Show();
   ConfirmPasswordChangeDialog::Dismiss();
@@ -239,13 +312,17 @@ void InSessionPasswordChangeManager::StartInSessionPasswordChange() {
 void InSessionPasswordChangeManager::OnSamlPasswordChanged(
     const std::string& scraped_old_password,
     const std::string& scraped_new_password) {
-  NotifyObservers(START_SAML_IDP_PASSWORD_CHANGE);
+  RecordEvent(InSessionPasswordChangeEvent::kSamlPasswordChanged);
+  NotifyObservers(Event::SAML_IDP_PASSWORD_CHANGED);
 
   user_manager::UserManager::Get()->SaveForceOnlineSignin(
       primary_user_->GetAccountId(), true);
   DismissExpiryNotification();
   PasswordChangeDialog::Dismiss();
 
+  RecordScrapingResult(!scraped_old_password.empty(),
+                       !scraped_new_password.empty());
+
   const bool both_passwords_scraped =
       !scraped_old_password.empty() && !scraped_new_password.empty();
   if (both_passwords_scraped) {
@@ -256,7 +333,8 @@ void InSessionPasswordChangeManager::OnSamlPasswordChanged(
     ConfirmPasswordChangeDialog::Show(scraped_old_password,
                                       scraped_new_password,
                                       /*show_spinner_initially=*/true);
-    ChangePassword(scraped_old_password, scraped_new_password);
+    ChangePassword(scraped_old_password, scraped_new_password,
+                   PasswordSource::PASSWORDS_SCRAPED);
   } else {
     // Failed to scrape passwords - prompt for passwords immediately.
     ConfirmPasswordChangeDialog::Show(scraped_old_password,
@@ -267,8 +345,10 @@ void InSessionPasswordChangeManager::OnSamlPasswordChanged(
 
 void InSessionPasswordChangeManager::ChangePassword(
     const std::string& old_password,
-    const std::string& new_password) {
-  NotifyObservers(START_CRYPTOHOME_PASSWORD_CHANGE);
+    const std::string& new_password,
+    PasswordSource password_source) {
+  password_source_ = password_source;
+  NotifyObservers(Event::START_CRYPTOHOME_PASSWORD_CHANGE);
   UserContext user_context(*primary_user_);
   user_context.SetKey(Key(new_password));
   authenticator_->MigrateKey(user_context, old_password);
@@ -284,18 +364,21 @@ void InSessionPasswordChangeManager::RemoveObserver(Observer* observer) {
 
 void InSessionPasswordChangeManager::OnAuthFailure(const AuthFailure& error) {
   VLOG(1) << "Failed to change cryptohome password: " << error.GetErrorString();
-  NotifyObservers(CRYPTOHOME_PASSWORD_CHANGE_FAILURE);
+  RecordCryptohomePasswordChangeFailure(password_source_);
+  NotifyObservers(Event::CRYPTOHOME_PASSWORD_CHANGE_FAILURE);
 }
 
 void InSessionPasswordChangeManager::OnPasswordChangeDetected() {
   VLOG(1) << "Failed to change cryptohome password: PasswordChangeDetected";
-  NotifyObservers(CRYPTOHOME_PASSWORD_CHANGE_FAILURE);
+  RecordCryptohomePasswordChangeFailure(password_source_);
+  NotifyObservers(Event::CRYPTOHOME_PASSWORD_CHANGE_FAILURE);
 }
 
 void InSessionPasswordChangeManager::OnAuthSuccess(
     const UserContext& user_context) {
   VLOG(3) << "Cryptohome password is changed.";
-  NotifyObservers(CRYPTOHOME_PASSWORD_CHANGED);
+  RecordCryptohomePasswordChangeSuccess(password_source_);
+  NotifyObservers(Event::CRYPTOHOME_PASSWORD_CHANGED);
 
   user_manager::UserManager::Get()->SaveForceOnlineSignin(
       user_context.GetAccountId(), false);
@@ -312,6 +395,7 @@ void InSessionPasswordChangeManager::OnAuthSuccess(
   DismissExpiryNotification();
   PasswordChangeDialog::Dismiss();
   ConfirmPasswordChangeDialog::Dismiss();
+  RecordEvent(InSessionPasswordChangeEvent::kFinishPasswordChange);
 }
 
 void InSessionPasswordChangeManager::OnSessionActivated(bool activated) {

diff --git a/chrome/browser/chromeos/login/saml/in_session_password_change_manager.h b/chrome/browser/chromeos/login/saml/in_session_password_change_manager.h
@@ -62,7 +62,7 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer,
                                        public ash::SessionActivationObserver {
  public:
   // Events in the in-session SAML password change flow.
-  enum Event {
+  enum class Event {
     // Dialog is open showing third-party IdP SAML password change page:
     START_SAML_IDP_PASSWORD_CHANGE,
     // Third party IdP SAML password is changed (but not cryptohome yet):
@@ -75,6 +75,12 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer,
     CRYPTOHOME_PASSWORD_CHANGED,
   };
 
+  // How the passwords were able to be obtained.
+  enum class PasswordSource {
+    PASSWORDS_SCRAPED,  // Passwords were scraped during SAML password change.
+    PASSWORDS_RETYPED,  // Passwords had to be manually confirmed by user.
+  };
+
   // Observers of InSessionPasswordChangeManager are notified of certain events.
   class Observer : public base::CheckedObserver {
    public:
@@ -134,7 +140,8 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer,
 
   // Change cryptohome password for primary user.
   void ChangePassword(const std::string& old_password,
-                      const std::string& new_password);
+                      const std::string& new_password,
+                      PasswordSource password_source);
 
   // Handle a failure to scrape the passwords during in-session password change,
   // by showing a dialog for the user to confirm their old + new password.
@@ -168,6 +175,7 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer,
   scoped_refptr<CryptohomeAuthenticator> authenticator_;
   int urgent_warning_days_;
   bool renotify_on_unlock_ = false;
+  PasswordSource password_source_ = PasswordSource::PASSWORDS_SCRAPED;
 
   friend class InSessionPasswordChangeManagerTest;
 

diff --git a/...e/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.cc b/...e/browser/ui/webui/chromeos/in_session_password_change/confirm_password_change_handler.cc
@@ -19,6 +19,16 @@
 
 namespace chromeos {
 
+namespace {
+
+const InSessionPasswordChangeManager::Event kIncorrectPasswordEvent =
+    InSessionPasswordChangeManager::Event::CRYPTOHOME_PASSWORD_CHANGE_FAILURE;
+
+const InSessionPasswordChangeManager::PasswordSource kPasswordSource =
+    InSessionPasswordChangeManager::PasswordSource::PASSWORDS_RETYPED;
+
+}  // namespace
+
 ConfirmPasswordChangeHandler::ConfirmPasswordChangeHandler() {
   if (InSessionPasswordChangeManager::IsInitialized()) {
     InSessionPasswordChangeManager::Get()->AddObserver(this);
@@ -33,8 +43,7 @@ ConfirmPasswordChangeHandler::~ConfirmPasswordChangeHandler() {
 
 void ConfirmPasswordChangeHandler::OnEvent(
     InSessionPasswordChangeManager::Event event) {
-  if (event ==
-      InSessionPasswordChangeManager::CRYPTOHOME_PASSWORD_CHANGE_FAILURE) {
+  if (event == kIncorrectPasswordEvent) {
     AllowJavascript();
     FireWebUIListener("incorrect-old-password");
   }
@@ -44,8 +53,8 @@ void ConfirmPasswordChangeHandler::HandleChangePassword(
     const base::ListValue* params) {
   const std::string old_password = params->GetList()[0].GetString();
   const std::string new_password = params->GetList()[1].GetString();
-  InSessionPasswordChangeManager::Get()->ChangePassword(old_password,
-                                                        new_password);
+  InSessionPasswordChangeManager::Get()->ChangePassword(
+      old_password, new_password, kPasswordSource);
 }
 
 void ConfirmPasswordChangeHandler::RegisterMessages() {

diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -51838,6 +51838,27 @@ Called by update_net_trust_anchors.py.-->
   <int value="4" label="NOT_SUPPORTED"/>
 </enum>
 
+<enum name="SamlInSessionPasswordChangeEvent">
+  <summary>
+    Track how often we try to guide users through the in-session flow to change
+    their SAML password, and how often they complete the flow.
+  </summary>
+  <int value="0" label="In-session PW-change manager enabled and created."/>
+  <int value="1" label="User notified that password will soon expire."/>
+  <int value="2" label="User notified urgently: password will soon expire."/>
+  <int value="3" label="User notified urgently: password already expired."/>
+  <int value="4" label="User acknowledges and begins change password flow."/>
+  <int value="5" label="SAML password is changed during in-session flow."/>
+  <int value="6" label="Password scraping succeeds during in-session change."/>
+  <int value="7" label="Password scraping partial success - 1 password only."/>
+  <int value="8" label="Password scraping fails - no passwords scraped."/>
+  <int value="9" label="Cryptohome password changed using scraped passwords."/>
+  <int value="10" label="Cryptohome password changed using retyped passwords."/>
+  <int value="11" label="Cryptohome change failed: wrong password scraped."/>
+  <int value="12" label="Cryptohome change failed: wrong password retyped."/>
+  <int value="13" label="In-session change password flow complete."/>
+</enum>
+
 <enum name="SamplingProfilerUnwindResult">
   <summary>Track reason for unwind failures in sampling profiler.</summary>
   <int value="0" label="Futex wait for signal handler failed."/>

diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
@@ -19286,6 +19286,16 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="ChromeOS.SAML.InSessionPasswordChangeEvent"
+    enum="SamlInSessionPasswordChangeEvent" expires_after="M83">
+  <owner>olsen@chromium.org</owner>
+  <owner>rsorokin@chromium.org</owner>
+  <summary>
+    Records how often users are guided through the SAML in-session password
+    change flow, and how often it is completed succesfully.
+  </summary>
+</histogram>
+
 <histogram name="ChromeOS.SAML.Scraping.PasswordCount" expires_after="M77">
   <owner>bartfab@chromium.org</owner>
   <summary>