Enable import/export of passwords into/from Password Manager

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

Index: chrome/browser/ui/webui/options/password_manager_handler.cc
diff --git a/chrome/browser/ui/webui/options/password_manager_handler.cc b/chrome/browser/ui/webui/options/password_manager_handler.cc
index 374e6e58cbb0ef2d542f7ce6e16df26bc80b1d5d..88bde3e0e3afdeb0e7a93e2807a89c701f9b6e9c 100644
--- a/chrome/browser/ui/webui/options/password_manager_handler.cc
+++ b/chrome/browser/ui/webui/options/password_manager_handler.cc
@@ -5,8 +5,12 @@
 #include "chrome/browser/ui/webui/options/password_manager_handler.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
@@ -15,18 +19,25 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
+#if defined(OS_WIN) && defined(USE_ASH)
+#include "chrome/browser/ui/ash/ash_util.h"

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

nit: Please move this #ifdefed #include into the section below line 50.

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

Done.

+#endif
+#include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
+#include "components/password_manager/core/browser/export/password_exporter.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/password_manager_constants.h"
+#include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/browser/password_ui_utils.h"
 #include "components/password_manager/core/common/experiments.h"
 #include "components/prefs/pref_service.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/url_formatter/url_formatter.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/user_metrics.h"
@@ -70,10 +81,23 @@ void CopyOriginInfoOfPasswordForm(const autofill::PasswordForm& form,
   entry->SetBoolean(kIsSecureField, content::IsOriginSecure(form.origin));
 }
 
+// Enumeration of different callers of SelectFile. Starting count at 1 so
+// accidental call of SelectFile with params=NULL will error out.
+enum FileSelectorCaller {
+  IMPORT_FILE_SELECTED = 1,
+  EXPORT_FILE_SELECTED,
+};
+
 }  // namespace
 
-PasswordManagerHandler::PasswordManagerHandler()
-    : password_manager_presenter_(this) {}
+PasswordManagerHandler::PasswordManagerHandler() {
+  password_manager_presenter_.reset(new PasswordManagerPresenter(this));
+}
+
+PasswordManagerHandler::PasswordManagerHandler(
+    PasswordManagerPresenter* presenter) {
+  password_manager_presenter_.reset(presenter);
+}
 
 PasswordManagerHandler::~PasswordManagerHandler() {}
 
@@ -103,6 +127,10 @@ void PasswordManagerHandler::GetLocalizedValues(
        IDS_PASSWORDS_PAGE_VIEW_NO_PASSWORDS_DESCRIPTION},
       {"passwordsNoExceptionsDescription",
        IDS_PASSWORDS_PAGE_VIEW_NO_EXCEPTIONS_DESCRIPTION},
+      {"passwordManagerImportPasswordButtonText",
+       IDS_PASSWORD_MANAGER_IMPORT_BUTTON},
+      {"passwordManagerExportPasswordButtonText",
+       IDS_PASSWORD_MANAGER_EXPORT_BUTTON},
   };
 
   RegisterStrings(localized_strings, resources, arraysize(resources));
@@ -169,10 +197,23 @@ void PasswordManagerHandler::RegisterMessages() {
       "requestShowPassword",
       base::Bind(&PasswordManagerHandler::HandleRequestShowPassword,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "importPassword",
+      base::Bind(&PasswordManagerHandler::HandlePasswordImport,
+                 base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "exportPassword",
+      base::Bind(&PasswordManagerHandler::HandlePasswordExport,
+                 base::Unretained(this)));
 }
 
 void PasswordManagerHandler::InitializeHandler() {
-  password_manager_presenter_.Initialize();
+  password_manager_presenter_->Initialize();
+}
+
+void PasswordManagerHandler::InitializePage() {
+  if (password_manager::ImportExportExperimentEnabled())
+    web_ui()->CallJavascriptFunction("PasswordManager.showImportExportButton");
 }
 
 void PasswordManagerHandler::HandleRemoveSavedPassword(
@@ -180,7 +221,8 @@ void PasswordManagerHandler::HandleRemoveSavedPassword(
   std::string string_value = base::UTF16ToUTF8(ExtractStringValue(args));
   int index;
   if (base::StringToInt(string_value, &index) && index >= 0) {
-    password_manager_presenter_.RemoveSavedPassword(static_cast<size_t>(index));
+    password_manager_presenter_->RemoveSavedPassword(
+        static_cast<size_t>(index));
   }
 }
 
@@ -189,7 +231,7 @@ void PasswordManagerHandler::HandleRemovePasswordException(
   std::string string_value = base::UTF16ToUTF8(ExtractStringValue(args));
   int index;
   if (base::StringToInt(string_value, &index) && index >= 0) {
-    password_manager_presenter_.RemovePasswordException(
+    password_manager_presenter_->RemovePasswordException(
         static_cast<size_t>(index));
   }
 }
@@ -200,7 +242,7 @@ void PasswordManagerHandler::HandleRequestShowPassword(
   if (!ExtractIntegerValue(args, &index))
     NOTREACHED();
 
-  password_manager_presenter_.RequestShowPassword(static_cast<size_t>(index));
+  password_manager_presenter_->RequestShowPassword(static_cast<size_t>(index));
 }
 
 void PasswordManagerHandler::ShowPassword(
@@ -217,7 +259,7 @@ void PasswordManagerHandler::ShowPassword(
 
 void PasswordManagerHandler::HandleUpdatePasswordLists(
     const base::ListValue* args) {
-  password_manager_presenter_.UpdatePasswordLists();
+  password_manager_presenter_->UpdatePasswordLists();
 }
 
 void PasswordManagerHandler::SetPasswordList(
@@ -268,4 +310,105 @@ void PasswordManagerHandler::SetPasswordExceptionList(
                                    entries);
 }
 
+void PasswordManagerHandler::FileSelected(const base::FilePath& path,
+                                          int index,
+                                          void* params) {
+  switch (static_cast<FileSelectorCaller>(reinterpret_cast<intptr_t>(params))) {
+    case IMPORT_FILE_SELECTED:
+      ImportPasswordFileSelected(path);
+      break;
+    case EXPORT_FILE_SELECTED:
+      ExportPasswordFileSelected(path);
+      break;
+  }
+}
+
+void PasswordManagerHandler::HandlePasswordImport(const base::ListValue* args) {
+#if !defined(OS_ANDROID)  // This is never called on Android.
+  ui::SelectFileDialog::FileTypeInfo file_type_info;
+  file_type_info.extensions =
+      password_manager::PasswordImporter::GetSupportedFileExtensions();
+  DCHECK(!file_type_info.extensions.empty() &&
+         !file_type_info.extensions[0].empty());
+  file_type_info.include_all_files = true;
+  selected_file_dialog_ = ui::SelectFileDialog::Create(
+      this, new ChromeSelectFilePolicy(web_ui()->GetWebContents()));
+  selected_file_dialog_->SelectFile(
+      ui::SelectFileDialog::SELECT_OPEN_FILE,
+      l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_IMPORT_DIALOG_TITLE),
+      base::FilePath(), &file_type_info, 1, file_type_info.extensions[0][0],
+      web_ui()->GetWebContents()->GetTopLevelNativeWindow(),
+      reinterpret_cast<void*>(IMPORT_FILE_SELECTED));
+#endif
+}
+
+void PasswordManagerHandler::ImportPasswordFileSelected(
+    const base::FilePath& path) {
+  password_manager::PasswordImporter::Import(
+      path, content::BrowserThread::GetMessageLoopProxyForThread(
+                content::BrowserThread::FILE)
+                .get(),
+      base::Bind(&PasswordManagerHandler::ImportPasswordFileRead,

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

What happens when the user closes the settings page while the import is in
progress? Is it really safe to have Unretained here?

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

Maybe not, but the other options
here(https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures#TOC-base::Callback-and-base::Bind-)
don't seem to apply well either. Any suggestion?

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

My suggestion is to split the functionality: Instead of PasswordManagerHandler
taking care of executing ImportPasswordFileRead directly, this functionality
should be transferred to a separate object (called, e.g.,
ImportPasswordResultConsumer, or some similar name). This object should be
refcounted, created by PasswordManagerHandler in a local scoped_refptr within
ImportPasswordFileSelected, and then just passed into the Bind call. The bind
call will increase the refcount, making sure that the object survives the
destruction of PasswordManagerHandler, and finishes the import.

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

There is a problem:
ImportPasswordFileRead needs to use PasswordStore. And getting the PasswordStore
requires the Profile(e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...).
So unless the Profile object can outlive the handler object, splitting the
functionality is not going to help. I'm not sure about the life cycle of
Profile, can you confirm? 

[vabr (Chromium), 2016/03/21 14:01:30]

Yes, Profile definitely outlives the handler object. It represents the whole
Chrome profile, and outlives all Chrome windows for that profile.

The worst which can happen is that the helper object will ask for the password
store and will get a null pointer, because the user just closed the very last
tab, and Chrome is shutting down its services (including PasswordStore). In that
case the import will fail, but the code should not crash.

[xunlu, 2016/03/22 07:17:15]

Done.

+                 base::Unretained(this)));
+}
+
+void PasswordManagerHandler::ImportPasswordFileRead(
+    password_manager::PasswordImporter::Result result,
+    const std::vector<autofill::PasswordForm>& forms) {
+  UMA_HISTOGRAM_ENUMERATION("PasswordManager.ImportPasswordFromCSVResult",
+                            result,
+                            password_manager::PasswordImporter::SEMANTIC_ERROR);

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

Please do not hard-code the fact that SEMANTIC_ERROR is last here. Also, the
boundary value should be strictly greater than the highest possible value.

To fix both, please define an additional enum value (NUM_IMPORT_RESULTS), make
it the last enum value of Result and use NUM_IMPORT_RESULTS as the boundary
value here. See
components/password_manager/core/browser/password_manager_metrics_util.h for
similar cases.

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

Done.

+  if (result != password_manager::PasswordImporter::SUCCESS)
+    return;
+
+  UMA_HISTOGRAM_COUNTS("PasswordManager.ImportedPasswordsPerUserInCSV",
+                       forms.size());
+
+  password_manager::PasswordStore* store =
+      password_manager_presenter_->GetPasswordStore();

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

Could you use PasswordStoreFactory::GetForProfile directly? That seems to make
more sense that using the presenter for it (the presenter should take care of
presenting the settings page, not providing the Chrome services). If you
PasswordStoreFactory::GetForProfile, please also restore the presenter's
GetPasswordStore as private again.

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

Done.

+  if (store) {
+    for (const autofill::PasswordForm& form : forms) {
+      store->AddLogin(form);
+    }
+  } else {
+    UMA_HISTOGRAM_COUNTS("PasswordManager.FailToStorePasswordImportedFromCSV",
+                         1);
+  }
+}
+
+void PasswordManagerHandler::HandlePasswordExport(const base::ListValue* args) {
+#if !defined(OS_ANDROID)  // This is never called on Android.
+  if (!password_manager_presenter_->IsUserAuthenticated()) {
+    return;
+  }
+  ui::SelectFileDialog::FileTypeInfo file_type_info;
+  file_type_info.extensions =
+      password_manager::PasswordExporter::GetSupportedFileExtensions();
+  DCHECK(!file_type_info.extensions.empty() &&
+         !file_type_info.extensions[0].empty());
+  file_type_info.include_all_files = true;
+  selected_file_dialog_ = ui::SelectFileDialog::Create(
+      this, new ChromeSelectFilePolicy(web_ui()->GetWebContents()));
+  selected_file_dialog_->SelectFile(
+      ui::SelectFileDialog::SELECT_SAVEAS_FILE,
+      l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_EXPORT_DIALOG_TITLE),
+      base::FilePath(), &file_type_info, 1, file_type_info.extensions[0][0],
+      GetNativeWindow(), reinterpret_cast<void*>(EXPORT_FILE_SELECTED));
+#endif
+}
+
+void PasswordManagerHandler::ExportPasswordFileSelected(
+    const base::FilePath& path) {
+  std::vector<scoped_ptr<autofill::PasswordForm>> password_list =
+      password_manager_presenter_->GetAllPasswords();
+  UMA_HISTOGRAM_COUNTS("PasswordManager.ExportedPasswordsPerUserInCSV",
+                       password_list.size());
+  password_manager::PasswordExporter::Export(
+      path, std::move(password_list),
+      content::BrowserThread::GetMessageLoopProxyForThread(
+          content::BrowserThread::FILE)
+          .get(),
+      base::Closure());
+}
+
 }  // namespace options