Enable import/export of passwords into/from Password Manager

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

Index: chrome/browser/ui/webui/options/password_manager_handler_unittest.cc
diff --git a/chrome/browser/ui/webui/options/password_manager_handler_unittest.cc b/chrome/browser/ui/webui/options/password_manager_handler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..446ae13b9014ef96aab9381589a38519c6bdb121
--- /dev/null
+++ b/chrome/browser/ui/webui/options/password_manager_handler_unittest.cc
@@ -0,0 +1,210 @@
+// Copyright 2016 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.
+
+#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 "content/public/test/web_contents_tester.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/shell_dialogs/select_file_dialog.h"
+#include "ui/shell_dialogs/select_file_dialog_factory.h"
+#include "ui/shell_dialogs/select_file_policy.h"
+
+using password_manager::MockPasswordStore;
+
+class TestSelectFileDialogFactory final : public ui::SelectFileDialogFactory {

[Evan Stade, 2016/04/08 21:00:16]

anonymous namespace

[xunlu, 2016/04/12 05:31:03]

Done.

+ public:
+  TestSelectFileDialogFactory() {}
+  ~TestSelectFileDialogFactory() override {}
+  ui::SelectFileDialog* Create(ui::SelectFileDialog::Listener* listener,
+                               ui::SelectFilePolicy* policy) override {
+    return new TestSelectFileDialog(listener, new TestSelectFilePolicy);
+  }
+
+ private:
+  class TestSelectFilePolicy : public ui::SelectFilePolicy {
+   public:
+    bool CanOpenSelectFileDialog() override { return true; }
+    void SelectFileDenied() override {}
+  };
+
+  class TestSelectFileDialog : public ui::SelectFileDialog {
+   public:
+    TestSelectFileDialog(Listener* listener, ui::SelectFilePolicy* policy)
+        : ui::SelectFileDialog(listener, policy) {}
+
+   protected:
+    void SelectFileImpl(Type type,
+                        const base::string16& title,
+                        const base::FilePath& default_path,
+                        const FileTypeInfo* file_types,
+                        int file_type_index,
+                        const base::FilePath::StringType& default_extension,
+                        gfx::NativeWindow owning_window,
+                        void* params) override {}
+    bool IsRunning(gfx::NativeWindow owning_window) const override {
+      return false;
+    }
+    void ListenerDestroyed() override {}
+    bool HasMultipleFileTypeChoicesImpl() override { return false; }
+    ~TestSelectFileDialog() override {}
+  };
+};
+
+class CallbackTestWebUI : public content::TestWebUI {
+ public:
+  CallbackTestWebUI() {}
+  ~CallbackTestWebUI() override {}
+  void RegisterMessageCallback(const std::string& message,
+                               const MessageCallback& callback) override;
+  void ProcessWebUIMessage(const GURL& source_url,
+                           const std::string& message,
+                           const base::ListValue& args) override;
+
+  MOCK_CONST_METHOD0(GetWebContents, content::WebContents*());
+
+ private:
+  std::map<std::string, MessageCallback> message_callbacks_;
+};
+
+void CallbackTestWebUI::RegisterMessageCallback(
+    const std::string& message,
+    const MessageCallback& callback) {
+  message_callbacks_.insert(std::make_pair(message, callback));
+}
+
+void CallbackTestWebUI::ProcessWebUIMessage(const GURL& source_url,
+                                            const std::string& message,
+                                            const base::ListValue& args) {
+  std::map<std::string, MessageCallback>::const_iterator callback =
+      message_callbacks_.find(message);
+  if (callback != message_callbacks_.end()) {
+    callback->second.Run(&args);
+  }
+}
+
+class TestPasswordManagerHandler : public options::PasswordManagerHandler {
+ public:
+  TestPasswordManagerHandler(scoped_ptr<PasswordManagerPresenter> presenter,
+                             CallbackTestWebUI* web_ui)
+      : PasswordManagerHandler(std::move(presenter)) {
+    set_web_ui(web_ui);
+  }
+  ~TestPasswordManagerHandler() override {}
+};
+
+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:
+  DummyPasswordManagerHandler() : 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>>&) override {}
+  void SetPasswordExceptionList(
+      const std::vector<scoped_ptr<autofill::PasswordForm>>&) override {}
+
+#if !defined(OS_ANDROID)
+  gfx::NativeWindow GetNativeWindow() const override;
+#endif
+ private:
+  TestingProfile profile_;
+  PasswordManagerPresenter password_manager_presenter_;
+
+  DISALLOW_COPY_AND_ASSIGN(DummyPasswordManagerHandler);
+};
+
+#if !defined(OS_ANDROID)
+gfx::NativeWindow DummyPasswordManagerHandler::GetNativeWindow() const {
+  return NULL;
+}
+#endif
+
+Profile* DummyPasswordManagerHandler::GetProfile() {
+  return &profile_;
+}
+
+class PasswordManagerHandlerTest : public testing::Test {
+ protected:
+  PasswordManagerHandlerTest() {
+    UMA_HISTOGRAM_COUNTS("PasswordManager.ImportedPasswordsPerUserInCSV", 0);
+    UMA_HISTOGRAM_ENUMERATION("PasswordManager.ImportPasswordFromCSVResult", 0,
+                              4);
+    dummy_handler_.reset(new DummyPasswordManagerHandler());
+    presenter_raw_ = new MockPasswordManagerPresenter(dummy_handler_.get());
+    web_contents_ =
+        content::WebContentsTester::CreateTestWebContents(&profile_, NULL);
+    web_ui_.set_web_contents(web_contents_);
+    handler_.reset(new TestPasswordManagerHandler(
+        make_scoped_ptr(presenter_raw_), &web_ui_));
+    handler_->RegisterMessages();
+    ui::SelectFileDialog::SetFactory(new TestSelectFileDialogFactory);
+    handler_->InitializeHandler();
+  }
+
+  ~PasswordManagerHandlerTest() override {}
+
+  void ExportPassword() {
+    base::ListValue tmp;
+    web_ui_.ProcessWebUIMessage(GURL(), "exportPassword", tmp);
+  }
+
+  void ImportPassword() {
+    base::ListValue tmp;
+    web_ui_.ProcessWebUIMessage(GURL(), "importPassword", tmp);
+  }
+
+  PasswordManagerPresenter* presenter_raw_;
+  CallbackTestWebUI web_ui_;
+  content::WebContents* web_contents_;
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  scoped_ptr<DummyPasswordManagerHandler> dummy_handler_;
+  scoped_ptr<TestPasswordManagerHandler> handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordManagerHandlerTest);
+};
+
+TEST_F(PasswordManagerHandlerTest, PasswordImport) {
+  EXPECT_CALL(web_ui_, GetWebContents())
+      .Times(2)

[Evan Stade, 2016/04/08 21:00:15]

what is this attempting to test?

[xunlu, 2016/04/08 21:42:55]

This is to test PasswordManagerHandler::HandlePasswordImport() is called and
part of its logic executed when importPassword in JS is called.

[Evan Stade, 2016/04/08 21:46:39]

far too tightly coupled to be useful

[vabr (Chromium), 2016/04/11 08:42:59]

I share Evan's concerns here.

My suggestion:
Could you make TestSelectFileDialog call the listener as if the user chose some
file, and then override FileSelected in TestPasswordManagerHandler to check that
it was called with the expected |params|?

On that note, it is unfortunate that we have TestPasswordManagerHandler in the
test for PasswordManagerHandler, because it increases the danger that what we
end up testing is not the production code. This is usually a hint that the
tested class could be split into smaller functional blocks. In this case, my
further suggestion would be to separate
PasswordManagerHandler::HandlePasswordImport and
PasswordManagerHandler::HandlePasswordExport from PasswordManagerHandler into a
separate class to handle the dialogue creation
(PasswordImportExportDialogHandler?), which would be owned by
PasswordManagerHandler, but would not need to know about it at all. This class
could remove some code duplication between the two existing Handle* methods.
More importantly, this new class would also use another new class
(PasswordImportExportDialogListener?) to fulfil the role of the dialogue
listener (currently done by PasswordManagerHandler::FileSelected).

Once you have such separation, it should be easier to write the tests: you won't
have to set up the whole PasswordManagerHandler, in particular. But more
importantly, when testing each of the newly added classes, you can mock out the
rest, making it easier to verify that the important things are happening. For
example, when testing the *DialogHandler, you would mock out the listener, and
just check that it got the expected callback. Also, feel free to use the UMA
signals to verify behaviour, through base::HistogramTester.

[xunlu, 2016/04/12 05:31:03]

Thanks Vaclav.  Test updated.

[vabr (Chromium), 2016/04/12 08:55:39]

Thanks. I wish we had the above suggested separation of the functionality to
have proper testing, but I'm not going to torture you with that. It will remain
a technical debt which I'll have to pay later, though.

+      .WillOnce(testing::Return(web_contents_))
+      .WillOnce(testing::Return(web_contents_));
+  ImportPassword();
+}
+
+TEST_F(PasswordManagerHandlerTest, PasswordExport) {
+  EXPECT_CALL(*(static_cast<MockPasswordManagerPresenter*>(presenter_raw_)),
+              IsUserAuthenticated());
+  ExportPassword();

[Evan Stade, 2016/04/08 21:00:16]

I don't see what this is testing or what hypothetical regression it would catch

[xunlu, 2016/04/08 21:42:55]

Similar to previous one, this one verifies
PasswordManagerHandler::HandlePasswordImport() is called and part of its logic
executed when exportPassword in JS is called.

I agree these tests are pretty limited in their coverage. But it seems the best
we can do without having to friend this test class in PasswordManagerHandler. 
Any suggestions? 

[Evan Stade, 2016/04/08 21:46:39]

can you work with the primary reviewers on this CL to come up with better tests?
FRIEND_TEST is not the end of the world, and TestPasswordManagerHandler is a
common pattern as well.

[xunlu, 2016/04/08 22:26:54]

Considering that PasswordManagerHandler did not even have a unit_test before, I
think this is already a step forward (baby one though :) ). We can explore other
options if we find it necessary in the future.

+vabr

+}