Using ACCESS_PASSWORD_COPIED metrics

Used AccessPasswordInSettingsEvent::ACCESS_PASSWORD_COPIED metrics to
count number of times user clicked Copy password button in the password
settings. Added related check to unittests.

Bug: 917337
Change-Id: I4b6172acfaffc0f8b840632cfc320187e242ad7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2064549
Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743913}

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl.cc

@@ -25,6 +25,7 @@
 #include "components/password_manager/core/browser/password_list_sorter.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/password_ui_utils.h"
+#include "components/password_manager/core/browser/ui/plaintext_reason.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/web_contents.h"
@@ -223,25 +224,39 @@ void PasswordsPrivateDelegateImpl::RequestPlaintextPassword(
     return;
   }
 
-  if (reason == api::passwords_private::PLAINTEXT_REASON_COPY) {
-    // In case of copy we don't need to give password back to UI. callback
-    // will receive either empty string in case of success or null otherwise.
-    // Copying occurs here so javascript doesn't need plaintext password.
-    callback = base::BindOnce(
-        [](PlaintextPasswordCallback callback,
-           base::Optional<base::string16> password) {
-          if (!password) {
-            std::move(callback).Run(base::nullopt);
-            return;
-          }
-          ui::ScopedClipboardWriter(ui::ClipboardBuffer::kCopyPaste)
-              .WriteText(*password);
-          std::move(callback).Run(base::string16());
-        },
-        std::move(callback));
+  password_manager::PlaintextReason presenter_reason =
+      password_manager::PlaintextReason::kView;
+
+  switch (reason) {
+    case api::passwords_private::PLAINTEXT_REASON_VIEW:
+      break;
+    case api::passwords_private::PLAINTEXT_REASON_COPY:
+      presenter_reason = password_manager::PlaintextReason::kCopy;
+      // In case of copy we don't need to give password back to UI. callback
+      // will receive either empty string in case of success or null otherwise.
+      // Copying occurs here so javascript doesn't need plaintext password.
+      callback = base::BindOnce(
+          [](PlaintextPasswordCallback callback,
+             base::Optional<base::string16> password) {
+            if (!password) {
+              std::move(callback).Run(base::nullopt);
+              return;
+            }
+            ui::ScopedClipboardWriter(ui::ClipboardBuffer::kCopyPaste)
+                .WriteText(*password);
+            std::move(callback).Run(base::string16());
+          },
+          std::move(callback));
+      break;
+    case api::passwords_private::PLAINTEXT_REASON_EDIT:
+      presenter_reason = password_manager::PlaintextReason::kEdit;
+      break;
+    case api::passwords_private::PLAINTEXT_REASON_NONE:
+      NOTREACHED();
+      break;
   }
-  password_manager_presenter_->RequestPlaintextPassword(*sort_key,
-                                                        std::move(callback));
+  password_manager_presenter_->RequestPlaintextPassword(
+      *sort_key, presenter_reason, std::move(callback));
 }
 
 bool PasswordsPrivateDelegateImpl::OsReauthCall(

chrome/browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/passwords_private/passwords_private_delegate.h"
@@ -25,6 +26,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/password_list_sorter.h"
+#include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/reauth_purpose.h"
 #include "components/password_manager/core/browser/test_password_store.h"
@@ -41,11 +43,12 @@ using ::testing::Eq;
 using ::testing::Ne;
 using ::testing::Return;
 using ::testing::StrictMock;
-
 namespace extensions {
 
 namespace {
 
+constexpr char kHistogramName[] = "PasswordManager.AccessPasswordInSettings";
+
 using MockPlaintextPasswordCallback =
     base::MockCallback<PasswordsPrivateDelegate::PlaintextPasswordCallback>;
 
@@ -117,6 +120,8 @@ class PasswordsPrivateDelegateImplTest : public testing::Test {
   // PasswordsPrivateEventRouter.
   void SetUpRouters();
 
+  base::HistogramTester& histogram_tester() { return histogram_tester_; }
+
  protected:
   content::BrowserTaskEnvironment task_environment_;
   TestingProfile profile_;
@@ -125,6 +130,7 @@ class PasswordsPrivateDelegateImplTest : public testing::Test {
       ui::TestClipboard::CreateForCurrentThread();
 
  private:
+  base::HistogramTester histogram_tester_;
   DISALLOW_COPY_AND_ASSIGN(PasswordsPrivateDelegateImplTest);
 };
 
@@ -266,6 +272,10 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResult) {
   base::string16 result;
   test_clipboard_->ReadText(ui::ClipboardBuffer::kCopyPaste, &result);
   EXPECT_EQ(form.password_value, result);
+
+  histogram_tester().ExpectUniqueSample(
+      kHistogramName, password_manager::metrics_util::ACCESS_PASSWORD_COPIED,
+      1);
 }
 
 TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResultFail) {
@@ -292,6 +302,9 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestCopyPasswordCallbackResultFail) {
   test_clipboard_->ReadText(ui::ClipboardBuffer::kCopyPaste, &result);
   EXPECT_EQ(base::string16(), result);
   EXPECT_EQ(before_call, test_clipboard_->GetLastModifiedTime());
+
+  // Since Reauth had failed password was not copied and metric wasn't recorded
+  histogram_tester().ExpectTotalCount(kHistogramName, 0);
 }
 
 TEST_F(PasswordsPrivateDelegateImplTest, TestPassedReauthOnView) {
@@ -313,6 +326,10 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestPassedReauthOnView) {
   delegate.RequestPlaintextPassword(
       0, api::passwords_private::PLAINTEXT_REASON_VIEW, password_callback.Get(),
       nullptr);
+
+  histogram_tester().ExpectUniqueSample(
+      kHistogramName, password_manager::metrics_util::ACCESS_PASSWORD_VIEWED,
+      1);
 }
 
 TEST_F(PasswordsPrivateDelegateImplTest, TestFailedReauthOnView) {
@@ -334,6 +351,9 @@ TEST_F(PasswordsPrivateDelegateImplTest, TestFailedReauthOnView) {
   delegate.RequestPlaintextPassword(
       0, api::passwords_private::PLAINTEXT_REASON_VIEW, password_callback.Get(),
       nullptr);
+
+  // Since Reauth had failed password was not viewed and metric wasn't recorded
+  histogram_tester().ExpectTotalCount(kHistogramName, 0);
 }
 
 TEST_F(PasswordsPrivateDelegateImplTest, TestReauthOnExport) {

chrome/browser/ui/passwords/settings/password_manager_presenter.cc

@@ -345,6 +345,7 @@ void PasswordManagerPresenter::UndoRemoveSavedPasswordOrException() {
 #if !defined(OS_ANDROID)  // This is never called on Android.
 void PasswordManagerPresenter::RequestPlaintextPassword(
     const std::string& sort_key,
+    password_manager::PlaintextReason reason,
     base::OnceCallback<void(base::Optional<base::string16>)> callback) const {
   auto it = password_map_.find(sort_key);
   if (it == password_map_.end()) {
@@ -369,9 +370,12 @@ void PasswordManagerPresenter::RequestPlaintextPassword(
 
   // Call back the front end to reveal the password.
   std::move(callback).Run(form.password_value);
+  auto metric_type =
+      reason == password_manager::PlaintextReason::kCopy
+          ? password_manager::metrics_util::ACCESS_PASSWORD_COPIED
+          : password_manager::metrics_util::ACCESS_PASSWORD_VIEWED;
   UMA_HISTOGRAM_ENUMERATION(
-      "PasswordManager.AccessPasswordInSettings",
-      password_manager::metrics_util::ACCESS_PASSWORD_VIEWED,
+      "PasswordManager.AccessPasswordInSettings", metric_type,
       password_manager::metrics_util::ACCESS_PASSWORD_COUNT);
 }
 #endif

chrome/browser/ui/passwords/settings/password_manager_presenter.h

@@ -11,14 +11,15 @@
 #include <memory>
 #include <string>
 #include <vector>
-
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
+#include "build/build_config.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/ui/credential_provider_interface.h"
+#include "components/password_manager/core/browser/ui/plaintext_reason.h"
 #include "components/prefs/pref_member.h"
 #include "components/undo/undo_manager.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
@@ -91,6 +92,7 @@ class PasswordManagerPresenter
   // Undoes the last saved password or exception removal.
   void UndoRemoveSavedPasswordOrException();
 
+#if !defined(OS_ANDROID)
   // Requests to reveal the plain text password corresponding to |sort_key|. If
   // |sort_key| is a valid key into |password_map_|, runs |callback| with the
   // corresponding value, or nullopt otherwise.
@@ -98,7 +100,9 @@ class PasswordManagerPresenter
   // instead.
   void RequestPlaintextPassword(
       const std::string& sort_key,
+      password_manager::PlaintextReason reason,
       base::OnceCallback<void(base::Optional<base::string16>)> callback) const;
+#endif
 
   // Wrapper around |PasswordStore::AddLogin| that adds the corresponding undo
   // action to |undo_manager_|.

chrome/browser/ui/passwords/settings/password_manager_presenter_unittest.cc

@@ -15,14 +15,21 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/bind_test_util.h"
+#if !defined(OS_ANDROID)
+#include "base/test/metrics/histogram_tester.h"
+#endif
+#include "base/test/mock_callback.h"
 #include "build/build_config.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/ui/passwords/settings/password_ui_view.h"
 #include "chrome/browser/ui/passwords/settings/password_ui_view_mock.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/password_manager/core/browser/password_list_sorter.h"
+#include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
 #include "components/password_manager/core/browser/test_password_store.h"
+#include "components/password_manager/core/browser/ui/plaintext_reason.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -45,7 +52,9 @@ constexpr char kPassword[] = "pass";
 constexpr char kPassword2[] = "pass2";
 constexpr char kUsername[] = "user";
 constexpr char kUsername2[] = "user2";
-
+#if !defined(OS_ANDROID)
+constexpr char kHistogramName[] = "PasswordManager.AccessPasswordInSettings";
+#endif
 MATCHER(IsNotBlacklisted, "") {
   return !arg->blacklisted_by_user;
 }
@@ -82,9 +91,9 @@ class PasswordManagerPresenterTest : public testing::Test {
     task_environment_.RunUntilIdle();
   }
 
-  void AddPasswordEntry(const GURL& origin,
-                        base::StringPiece username,
-                        base::StringPiece password) {
+  autofill::PasswordForm AddPasswordEntry(const GURL& origin,
+                                          base::StringPiece username,
+                                          base::StringPiece password) {
     autofill::PasswordForm form;
     form.origin = origin;
     form.signon_realm = origin.GetOrigin().spec();
@@ -93,6 +102,7 @@ class PasswordManagerPresenterTest : public testing::Test {
     form.password_element = base::ASCIIToUTF16("Passwd");
     form.password_value = base::ASCIIToUTF16(password);
     store_->AddLogin(form);
+    return form;
   }
 
   void AddPasswordException(const GURL& origin) {
@@ -372,4 +382,28 @@ TEST_F(PasswordManagerPresenterTest, BlacklistDoesNotPreventExporting) {
   EXPECT_EQ(kSameOrigin, passwords_for_export[0]->origin);
 }
 
+#if !defined(OS_ANDROID)
+TEST_F(PasswordManagerPresenterTest, TestRequestPlaintextPassword) {
+  base::HistogramTester histogram_tester_;
+  autofill::PasswordForm form =
+      AddPasswordEntry(GURL(kExampleCom), kUsername, kPassword);
+
+  EXPECT_CALL(GetUIController(), SetPasswordList(SizeIs(1)));
+  EXPECT_CALL(GetUIController(), SetPasswordExceptionList(IsEmpty()));
+  UpdatePasswordLists();
+  base::MockOnceCallback<void(base::Optional<base::string16>)>
+      password_callback;
+  EXPECT_CALL(password_callback,
+              Run(testing::Eq(base::ASCIIToUTF16(kPassword))));
+  std::string sort_key = password_manager::CreateSortKey(form);
+  GetUIController().GetPasswordManagerPresenter()->RequestPlaintextPassword(
+      sort_key, password_manager::PlaintextReason::kView,
+      password_callback.Get());
+
+  histogram_tester_.ExpectUniqueSample(
+      kHistogramName, password_manager::metrics_util::ACCESS_PASSWORD_VIEWED,
+      1);
+}
+#endif
+
 }  // namespace

components/password_manager/core/browser/BUILD.gn

@@ -224,6 +224,7 @@ jumbo_static_library("browser") {
     "ui/export_flow.h",
     "ui/export_progress_status.h",
     "ui/import_flow.h",
+    "ui/plaintext_reason.h",
     "ui/saved_passwords_presenter.cc",
     "ui/saved_passwords_presenter.h",
     "votes_uploader.cc",

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

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_UI_PLAINTEXT_REASON_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_UI_PLAINTEXT_REASON_H_
+
+namespace password_manager {
+
+// Possible reasons why a plaintext password was requested.
+enum class PlaintextReason {
+  kView,
+  kCopy,
+  kEdit,
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_UI_PLAINTEXT_REASON_H_