Enable import/export of passwords into/from Password Manager

chrome/browser/ui/webui/options/password_manager_handler_unittest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.

[vabr (Chromium), 2016/03/10 10:53:32]

2016

[xunlu, 2016/03/16 07:23:58]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/metrics/histogram.h"
+#include "base/metrics/statistics_recorder.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
+#include "chrome/browser/ui/passwords/password_manager_presenter.h"
+#include "chrome/browser/ui/webui/options/password_manager_handler.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_web_ui.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using password_manager::MockPasswordStore;
+
+class MockPasswordManagerPresenter : public PasswordManagerPresenter {
+ public:
+  explicit MockPasswordManagerPresenter(PasswordUIView* handler)
+      : PasswordManagerPresenter(handler) {}
+  ~MockPasswordManagerPresenter() override {}
+
+  MOCK_METHOD0(IsUserAuthenticated, bool());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockPasswordManagerPresenter);
+};
+
+class DummyPasswordManagerHandler : public PasswordUIView {
+ public:
+  explicit DummyPasswordManagerHandler(Profile* profile)
+      : profile_(profile), password_manager_presenter_(this) {
+    password_manager_presenter_.Initialize();
+  }
+  ~DummyPasswordManagerHandler() override {}
+  Profile* GetProfile() override;
+
+  void ShowPassword(size_t,
+                    const std::string&,
+                    const std::string&,
+                    const base::string16&) override {}
+  void SetPasswordList(const std::vector<scoped_ptr<autofill::PasswordForm>>&,
+                       bool) override {}
+  void SetPasswordExceptionList(
+      const std::vector<scoped_ptr<autofill::PasswordForm>>&) override {}
+
+#if !defined(OS_ANDROID)
+  gfx::NativeWindow GetNativeWindow() const override;
+#endif
+ private:
+  Profile* profile_;
+  PasswordManagerPresenter password_manager_presenter_;
+  DISALLOW_COPY_AND_ASSIGN(DummyPasswordManagerHandler);

[vabr (Chromium), 2016/03/10 10:53:32]

nit: Please add a blank line above this line.

[xunlu, 2016/03/16 07:23:58]

Done.

+};
+
+#if !defined(OS_ANDROID)
+gfx::NativeWindow DummyPasswordManagerHandler::GetNativeWindow() const {
+  return NULL;
+}
+#endif
+Profile* DummyPasswordManagerHandler::GetProfile() {
+  return profile_;
+}
+
+class PasswordManagerHandlerTest : public testing::Test {
+ protected:
+  PasswordManagerHandlerTest() {}
+  ~PasswordManagerHandlerTest() override {
+    delete handler_;

[vabr (Chromium), 2016/03/10 10:53:32]

Why not use scoped_ptr instead of the manual deletions?

[xunlu, 2016/03/16 07:23:58]

Doing that would crash the test during the destruction time. Debugged it for
hours without success, so I changed it back.

[vabr (Chromium), 2016/03/16 17:48:28]

Just looking at the code, I can tell that handler_ depends on presenter_, which
depends on dummy_handler_. So the necessary order of destruction is in reverse
of the dependencies, first the handler_ (which apparently owns the presenter_,
so that's solved for us) and then the dummy_handler_. If you kept the member
variable order as it is now, the desctruction order of the scoped_ptrs was
wrong.

Therefore, please define them in this order at the end of the member variable
list:

scoped_ptr<DummyPasswordManagerHandler> dummy_handler_;
scoped_ptr<options::PasswordManagerHandler> handler_;

That is equivalent to the current destructor, so should work OK.

Please try doing that instead of the manual deletes -- does it still crash? I
really don't think it is a good idea to have manual deletions here, especially
just because the code crashes for an unknown reason otherwise (that sounds scary
by itself).

[xunlu, 2016/03/18 21:15:30]

It's working fine now. Thanks for the tip!

+    delete dummy_handler_;
+  }
+
+  void SetUp() override {
+    UMA_HISTOGRAM_COUNTS("PasswordManager.ImportedPasswordsPerUserInCSV", 0);

[vabr (Chromium), 2016/03/10 10:53:32]

Could this all happen in the constructor instead of the SetUp?
Constructor/desctructor is more enouraged than SetUp/TearDown:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/J2K-2Dmupb8/f27GD...

[xunlu, 2016/03/16 07:23:58]

Done.

+    UMA_HISTOGRAM_ENUMERATION("PasswordManager.ImportPasswordFromCSVResult", 0,
+                              3);
+    PasswordStoreFactory::GetInstance()->SetTestingFactory(
+        &profile_, password_manager::BuildPasswordStore<content::BrowserContext,
+                                                        MockPasswordStore>);
+    dummy_handler_ = new DummyPasswordManagerHandler(&profile_);
+    handler_ = new options::PasswordManagerHandler(
+        new MockPasswordManagerPresenter(dummy_handler_));
+    handler_->set_web_ui(&web_ui_);
+  }
+
+  MockPasswordManagerPresenter* GetPresenter() {
+    return static_cast<MockPasswordManagerPresenter*>(
+        handler_->password_manager_presenter_.get());

[vabr (Chromium), 2016/03/10 10:53:32]

Why do not cache this in the test instead of getting it back through the
handler?

[xunlu, 2016/03/16 07:23:58]

Done.

+  }
+
+  MockPasswordStore* GetPasswordStore() {
+    return static_cast<MockPasswordStore*>(GetPresenter()->GetPasswordStore());
+  }
+
+  void ExportPassword() { handler_->HandlePasswordExport(NULL); }
+
+  void ImportPassword() {
+    autofill::PasswordForm form(MakeForm());
+    std::vector<autofill::PasswordForm> passwordForms;
+    passwordForms.push_back(form);
+    passwordForms.push_back(form);
+    handler_->ImportPasswordFileRead(
+        password_manager::PasswordImporter::SUCCESS, passwordForms);
+  }
+
+  autofill::PasswordForm MakeForm() {
+    autofill::PasswordForm form;
+    GURL origin("http://test.com");
+    form.origin = origin;
+    form.username_element = base::ASCIIToUTF16("Email");
+    form.username_value = base::ASCIIToUTF16("test@test.com");
+    form.password_element = base::ASCIIToUTF16("Password");
+    form.password_value = base::ASCIIToUTF16("abcdef");
+    return form;
+  }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  options::PasswordManagerHandler* handler_;
+  DummyPasswordManagerHandler* dummy_handler_;
+  content::TestWebUI web_ui_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordManagerHandlerTest);
+};
+
+MATCHER_P(FormMatches, form, "") {
+  return form.signon_realm == arg.signon_realm && form.origin == arg.origin &&
+         form.action == arg.action &&
+         form.username_element == arg.username_element &&
+         form.password_element == arg.password_element &&
+         form.new_password_element == arg.new_password_element &&
+         form.submit_element == arg.submit_element;
+}
+
+TEST_F(PasswordManagerHandlerTest, PasswordImport) {
+  autofill::PasswordForm form(MakeForm());
+  base::HistogramBase* imported_passwords =
+      base::StatisticsRecorder::FindHistogram(
+          "PasswordManager.ImportedPasswordsPerUserInCSV");
+  base::HistogramBase* import_result_enum =
+      base::StatisticsRecorder::FindHistogram(
+          "PasswordManager.ImportPasswordFromCSVResult");
+  scoped_ptr<base::HistogramSamples> imported_count_samples(
+      imported_passwords->SnapshotSamples());
+  scoped_ptr<base::HistogramSamples> import_result_enum_samples(
+      import_result_enum->SnapshotSamples());
+  EXPECT_CALL(*GetPasswordStore(), AddLogin(FormMatches(form))).Times(2);
+  ImportPassword();
+  EXPECT_EQ(imported_count_samples->TotalCount(), 1);
+  EXPECT_EQ(import_result_enum_samples->GetCount(0), 1);
+}
+
+TEST_F(PasswordManagerHandlerTest, PasswordExport) {
+  EXPECT_CALL(*GetPresenter(), IsUserAuthenticated());
+  ExportPassword();
+}