Refactor SmbCredentialsDialog::Show() to respond with a callback

Also change |mount_id| to be a string to accommodate smbfs. Oh, and add
a browser test.

Bug: 1054704

Change-Id: I7c5b8eee7886093cde6b0088de489f8f91fb8084
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076862
Commit-Queue: Anand Mistry <amistry@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746257}

chrome/browser/chromeos/smb_client/smb_service.cc

@@ -11,6 +11,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_tick_clock.h"
@@ -215,33 +216,6 @@ void SmbService::GatherSharesInNetwork(HostDiscoveryResponse discovery_callback,
           std::move(shares_callback)));
 }
 
-void SmbService::UpdateCredentials(int32_t mount_id,
-                                   const std::string& username,
-                                   const std::string& password) {
-  std::string parsed_username = username;
-  std::string workgroup;
-  ParseUserName(username, &parsed_username, &workgroup);
-
-  GetSmbProviderClient()->UpdateMountCredentials(
-      mount_id, workgroup, parsed_username, MakeFdWithContents(password),
-      base::BindOnce(&SmbService::OnUpdateCredentialsResponse, AsWeakPtr(),
-                     mount_id));
-}
-
-void SmbService::OnUpdateCredentialsResponse(int32_t mount_id,
-                                             smbprovider::ErrorType error) {
-  auto creds_reply_iter = update_credential_replies_.find(mount_id);
-  DCHECK(creds_reply_iter != update_credential_replies_.end());
-
-  if (error == smbprovider::ERROR_OK) {
-    std::move(creds_reply_iter->second).Run();
-  } else {
-    LOG(ERROR) << "Failed to update the credentials for mount id " << mount_id;
-  }
-
-  update_credential_replies_.erase(creds_reply_iter);
-}
-
 void SmbService::UpdateSharePath(int32_t mount_id,
                                  const std::string& share_path,
                                  StartReadDirIfSuccessfulCallback reply) {
@@ -874,13 +848,38 @@ std::vector<SmbUrl> SmbService::GetPreconfiguredSharePaths(
 void SmbService::RequestCredentials(const std::string& share_path,
                                     int32_t mount_id,
                                     base::OnceClosure reply) {
-  update_credential_replies_[mount_id] = std::move(reply);
-  OpenRequestCredentialsDialog(share_path, mount_id);
+  smb_dialog::SmbCredentialsDialog::Show(
+      base::NumberToString(mount_id), share_path,
+      base::BindOnce(&SmbService::OnSmbCredentialsDialogShown, AsWeakPtr(),
+                     mount_id, std::move(reply)));
 }
 
-void SmbService::OpenRequestCredentialsDialog(const std::string& share_path,
-                                              int32_t mount_id) {
-  smb_dialog::SmbCredentialsDialog::Show(mount_id, share_path);
+void SmbService::OnSmbCredentialsDialogShown(int32_t mount_id,
+                                             base::OnceClosure reply,
+                                             bool canceled,
+                                             const std::string& username,
+                                             const std::string& password) {
+  if (canceled) {
+    return;
+  }
+
+  std::string parsed_username = username;
+  std::string workgroup;
+  ParseUserName(username, &parsed_username, &workgroup);
+
+  GetSmbProviderClient()->UpdateMountCredentials(
+      mount_id, workgroup, parsed_username, MakeFdWithContents(password),
+      base::BindOnce(
+          [](int32_t mount_id, base::OnceClosure reply,
+             smbprovider::ErrorType error) {
+            if (error == smbprovider::ERROR_OK) {
+              std::move(reply).Run();
+            } else {
+              LOG(ERROR) << "Failed to update the credentials for mount id "
+                         << mount_id;
+            }
+          },
+          mount_id, std::move(reply)));
 }
 
 std::vector<SmbUrl> SmbService::GetPreconfiguredSharePathsForDropdown() const {

chrome/browser/chromeos/smb_client/smb_service.h

@@ -87,13 +87,6 @@ class SmbService : public KeyedService,
   void GatherSharesInNetwork(HostDiscoveryResponse discovery_callback,
                              GatherSharesResponse shares_callback);
 
-  // Updates the credentials for |mount_id|. If there is a stored callback in
-  // |update_credentials_replies_| for |mount_id|, it will be run upon once the
-  // credentials are successfully updated.
-  void UpdateCredentials(int32_t mount_id,
-                         const std::string& username,
-                         const std::string& password);
-
   // Updates the share path for |mount_id|.
   void UpdateSharePath(int32_t mount_id,
                        const std::string& share_path,
@@ -264,17 +257,15 @@ class SmbService : public KeyedService,
                           int32_t mount_id,
                           base::OnceClosure reply);
 
-  // Opens a request credential dialog for the share path |share_path|.
-  // When a user clicks "Update" in the dialog, UpdateCredentials is run.
-  void OpenRequestCredentialsDialog(const std::string& share_path,
-                                    int32_t mount_id);
-
-  // Handles the response from attempting to the update the credentials of an
-  // existing share. If |error| indicates success, the callback is run and
-  // removed from |update_credential_replies_|. Otherwise, the callback
-  // is removed from |update_credential_replies_| and the error is logged.
-  void OnUpdateCredentialsResponse(int32_t mount_id,
-                                   smbprovider::ErrorType error);
+  // Handles the response from showing the SMB credentials dialog. If |canceled|
+  // is true, the |reply| callback is dropped. Otherwise, |username| and
+  // |password| are passed to the smb service and |reply| is run if the service
+  // returns success.
+  void OnSmbCredentialsDialogShown(int32_t mount_id,
+                                   base::OnceClosure reply,
+                                   bool canceled,
+                                   const std::string& username,
+                                   const std::string& password);
 
   // Requests an updated share path via running
   // ShareFinder::DiscoverHostsInNetwork. |reply| is stored. Once the share path
@@ -308,8 +299,6 @@ class SmbService : public KeyedService,
   Profile* profile_;
   std::unique_ptr<base::TickClock> tick_clock_;
   std::unique_ptr<SmbShareFinder> share_finder_;
-  // |mount_id| -> |reply|. Stored callbacks to run after updating credential.
-  std::map<int32_t, base::OnceClosure> update_credential_replies_;
   // |file_system_id| -> |mount_id|
   std::unordered_map<std::string, int32_t> mount_id_map_;
   // |smbfs_mount_id| -> SmbFsShare

chrome/browser/resources/chromeos/smb_shares/smb_credentials_dialog.js

@@ -34,11 +34,8 @@ Polymer({
     /** @private {string} */
     password_: String,
 
-    /** @private {number} */
-    mountId_: {
-      type: Number,
-      value: -1,
-    },
+    /** @private {string} */
+    mountId_: String,
   },
 
   /** @private {?SmbBrowserProxy} */
@@ -56,7 +53,7 @@ Polymer({
     var args = JSON.parse(dialogArgs);
     assert(args);
     assert(args.path);
-    assert(typeof args.mid === 'number');
+    assert(args.mid);
     this.sharePath_ = args.path;
     this.mountId_ = args.mid;
 

chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog.cc

@@ -4,8 +4,10 @@
 
 #include "chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "base/json/json_writer.h"
-#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/browser_resources.h"
@@ -36,35 +38,57 @@ void AddSmbCredentialsDialogStrings(content::WebUIDataSource* html_source) {
   }
 }
 
-std::string GetDialogId(int32_t mount_id) {
-  return chrome::kChromeUISmbCredentialsURL + base::NumberToString(mount_id);
+std::string GetDialogId(const std::string& mount_id) {
+  return chrome::kChromeUISmbCredentialsURL + mount_id;
+}
+
+SmbCredentialsDialog* GetDialog(const std::string& id) {
+  return static_cast<SmbCredentialsDialog*>(
+      SystemWebDialogDelegate::FindInstance(id));
 }
 
 }  // namespace
 
 // static
-void SmbCredentialsDialog::Show(int32_t mount_id,
-                                const std::string& share_path) {
+void SmbCredentialsDialog::Show(const std::string& mount_id,
+                                const std::string& share_path,
+                                RequestCallback callback) {
   // If an SmbCredentialsDialog is already opened for |mount_id|, focus that
   // dialog rather than opening a second one.
-  auto* instance = SystemWebDialogDelegate::FindInstance(GetDialogId(mount_id));
-  if (instance) {
-    instance->Focus();
+  SmbCredentialsDialog* dialog = GetDialog(GetDialogId(mount_id));
+  if (dialog) {
+    // Replace the dialog's callback so that is responds to the most recent
+    // request.
+    dialog->callback_ = std::move(callback);
+    dialog->Focus();
     return;
   }
 
-  SmbCredentialsDialog* dialog = new SmbCredentialsDialog(mount_id, share_path);
+  dialog = new SmbCredentialsDialog(mount_id, share_path, std::move(callback));
   dialog->ShowSystemDialog();
 }
 
-SmbCredentialsDialog::SmbCredentialsDialog(int32_t mount_id,
-                                           const std::string& share_path)
+SmbCredentialsDialog::SmbCredentialsDialog(const std::string& mount_id,
+                                           const std::string& share_path,
+                                           RequestCallback callback)
     : SystemWebDialogDelegate(GURL(GetDialogId(mount_id)),
                               base::string16() /* title */),
       mount_id_(mount_id),
-      share_path_(share_path) {}
+      share_path_(share_path),
+      callback_(std::move(callback)) {}
+
+SmbCredentialsDialog::~SmbCredentialsDialog() {
+  if (callback_) {
+    std::move(callback_).Run(true /* canceled */, std::string() /* username */,
+                             std::string() /* password */);
+  }
+}
 
-SmbCredentialsDialog::~SmbCredentialsDialog() = default;
+void SmbCredentialsDialog::Respond(const std::string& username,
+                                   const std::string& password) {
+  DCHECK(callback_);
+  std::move(callback_).Run(false /* canceled */, username, password);
+}
 
 void SmbCredentialsDialog::GetDialogSize(gfx::Size* size) const {
   size->SetSize(SystemWebDialogDelegate::kDialogWidth,
@@ -92,13 +116,24 @@ SmbCredentialsDialogUI::SmbCredentialsDialogUI(content::WebUI* web_ui)
   source->AddResourcePath("smb_credentials_dialog.js",
                           IDR_SMB_CREDENTIALS_DIALOG_JS);
 
-  web_ui->AddMessageHandler(
-      std::make_unique<SmbHandler>(Profile::FromWebUI(web_ui)));
+  web_ui->AddMessageHandler(std::make_unique<SmbHandler>(
+      Profile::FromWebUI(web_ui),
+      base::BindOnce(&SmbCredentialsDialogUI::OnUpdateCredentials,
+                     base::Unretained(this))));
 
   content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), source);
 }
 
 SmbCredentialsDialogUI::~SmbCredentialsDialogUI() = default;
 
+void SmbCredentialsDialogUI::OnUpdateCredentials(const std::string& username,
+                                                 const std::string& password) {
+  SmbCredentialsDialog* dialog =
+      GetDialog(web_ui()->GetWebContents()->GetLastCommittedURL().spec());
+  if (dialog) {
+    dialog->Respond(username, password);
+  }
+}
+
 }  // namespace smb_dialog
 }  // namespace chromeos

chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog.h

@@ -5,6 +5,9 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_CHROMEOS_SMB_SHARES_SMB_CREDENTIALS_DIALOG_H_
 #define CHROME_BROWSER_UI_WEBUI_CHROMEOS_SMB_SHARES_SMB_CREDENTIALS_DIALOG_H_
 
+#include <string>
+
+#include "base/callback.h"
 #include "base/macros.h"
 #include "chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.h"
 #include "ui/web_dialogs/web_dialog_ui.h"
@@ -14,11 +17,25 @@ namespace smb_dialog {
 
 class SmbCredentialsDialog : public SystemWebDialogDelegate {
  public:
-  // Shows the dialog.
-  static void Show(int32_t mount_id, const std::string& share_path);
+  using RequestCallback = base::OnceCallback<void(bool canceled,
+                                                  const std::string& username,
+                                                  const std::string& password)>;
+
+  // Shows the dialog, and runs |callback| when the user responds with a
+  // username/password, or the dialog is closed. If a dialog is currently being
+  // shown for |mount_id|, the existing dialog will be focused and its callback
+  // will be replaced with |callback|.
+  static void Show(const std::string& mount_id,
+                   const std::string& share_path,
+                   RequestCallback callback);
+
+  // Respond to the dialog being show with a username/password.
+  void Respond(const std::string& username, const std::string& password);
 
  protected:
-  SmbCredentialsDialog(int32_t mount_id, const std::string& share_path);
+  SmbCredentialsDialog(const std::string& mount_id,
+                       const std::string& share_path,
+                       RequestCallback callback);
   ~SmbCredentialsDialog() override;
 
   // ui::WebDialogDelegate
@@ -26,8 +43,9 @@ class SmbCredentialsDialog : public SystemWebDialogDelegate {
   std::string GetDialogArgs() const override;
 
  private:
-  int32_t mount_id_;
-  std::string share_path_;
+  const std::string mount_id_;
+  const std::string share_path_;
+  RequestCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SmbCredentialsDialog);
 };
@@ -37,6 +55,10 @@ class SmbCredentialsDialogUI : public ui::WebDialogUI {
   explicit SmbCredentialsDialogUI(content::WebUI* web_ui);
   ~SmbCredentialsDialogUI() override;
 
+ private:
+  void OnUpdateCredentials(const std::string& username,
+                           const std::string& password);
+
   DISALLOW_COPY_AND_ASSIGN(SmbCredentialsDialogUI);
 };
 

chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog_browsertest.cc

+// 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.
+
+#include "chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog.h"
+
+#include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/common/webui_url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+
+namespace chromeos {
+namespace smb_dialog {
+
+const char kMountId[] = "mount-id";
+const char kSharePath[] = "//test/share";
+
+class SmbCredentialsDialogTest : public InProcessBrowserTest {};
+
+IN_PROC_BROWSER_TEST_F(SmbCredentialsDialogTest, CloseDialog) {
+  content::WebContentsAddedObserver observer;
+
+  base::RunLoop run_loop;
+  SmbCredentialsDialog::Show(
+      kMountId, kSharePath,
+      base::BindLambdaForTesting([&run_loop](bool canceled,
+                                             const std::string& username,
+                                             const std::string& password) {
+        EXPECT_TRUE(canceled);
+        run_loop.Quit();
+      }));
+
+  content::WebContents* dialog_contents = observer.GetWebContents();
+  ASSERT_TRUE(content::WaitForLoadStop(dialog_contents));
+  EXPECT_EQ(dialog_contents->GetLastCommittedURL().host(),
+            chrome::kChromeUISmbCredentialsHost);
+  ASSERT_TRUE(
+      content::ExecuteScript(dialog_contents, "chrome.send('dialogClose');"));
+
+  run_loop.Run();
+}
+
+IN_PROC_BROWSER_TEST_F(SmbCredentialsDialogTest, ShowSameMountId) {
+  content::WebContentsAddedObserver observer;
+
+  base::RunLoop run_loop;
+  SmbCredentialsDialog::Show(
+      kMountId, kSharePath,
+      // This callback is replaced in the next call to Show() below, and
+      // therefore will never be run.
+      base::BindOnce([](bool canceled, const std::string& username,
+                        const std::string& password) { FAIL(); }));
+  SmbCredentialsDialog::Show(
+      kMountId, kSharePath,
+      base::BindLambdaForTesting([&run_loop](bool canceled,
+                                             const std::string& username,
+                                             const std::string& password) {
+        EXPECT_TRUE(canceled);
+        run_loop.Quit();
+      }));
+
+  content::WebContents* dialog_contents = observer.GetWebContents();
+  ASSERT_TRUE(content::WaitForLoadStop(dialog_contents));
+  EXPECT_EQ(dialog_contents->GetLastCommittedURL().host(),
+            chrome::kChromeUISmbCredentialsHost);
+  ASSERT_TRUE(
+      content::ExecuteScript(dialog_contents, "chrome.send('dialogClose');"));
+
+  run_loop.Run();
+}
+
+IN_PROC_BROWSER_TEST_F(SmbCredentialsDialogTest, SubmitCredentials) {
+  content::WebContentsAddedObserver observer;
+
+  base::RunLoop run_loop;
+  SmbCredentialsDialog::Show(
+      kMountId, kSharePath,
+      base::BindLambdaForTesting([&run_loop](bool canceled,
+                                             const std::string& username,
+                                             const std::string& password) {
+        EXPECT_FALSE(canceled);
+        EXPECT_EQ(username, "my-username");
+        EXPECT_EQ(password, "my-password");
+        run_loop.Quit();
+      }));
+
+  content::WebContents* dialog_contents = observer.GetWebContents();
+  ASSERT_TRUE(content::WaitForLoadStop(dialog_contents));
+  EXPECT_EQ(dialog_contents->GetLastCommittedURL().host(),
+            chrome::kChromeUISmbCredentialsHost);
+  ASSERT_TRUE(content::ExecuteScript(dialog_contents,
+                                     R"xxx(
+const dialog = document.querySelector('smb-credentials-dialog');
+dialog.username_ = 'my-username';
+dialog.password_ = 'my-password';
+dialog.$$('.action-button').click();
+      )xxx"));
+
+  run_loop.Run();
+}
+
+}  // namespace smb_dialog
+}  // namespace chromeos

chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.cc

@@ -34,7 +34,10 @@ base::Value BuildShareList(const std::vector<smb_client::SmbUrl>& shares) {
 
 }  // namespace
 
-SmbHandler::SmbHandler(Profile* profile) : profile_(profile) {}
+SmbHandler::SmbHandler(Profile* profile,
+                       UpdateCredentialsCallback update_cred_callback)
+    : profile_(profile),
+      update_cred_callback_(std::move(update_cred_callback)) {}
 
 SmbHandler::~SmbHandler() = default;
 
@@ -136,20 +139,16 @@ void SmbHandler::HandleGatherSharesResponse(
 void SmbHandler::HandleUpdateCredentials(const base::ListValue* args) {
   CHECK_EQ(3U, args->GetSize());
 
-  int32_t mount_id;
+  std::string mount_id;
   std::string username;
   std::string password;
 
-  CHECK(args->GetInteger(0, &mount_id));
+  CHECK(args->GetString(0, &mount_id));
   CHECK(args->GetString(1, &username));
   CHECK(args->GetString(2, &password));
 
-  smb_client::SmbService* const service = GetSmbService(profile_);
-  if (!service) {
-    return;
-  }
-
-  service->UpdateCredentials(mount_id, username, password);
+  DCHECK(update_cred_callback_);
+  std::move(update_cred_callback_).Run(username, password);
 }
 
 }  // namespace smb_dialog

chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_CHROMEOS_SMB_SHARES_SMB_HANDLER_H_
 #define CHROME_BROWSER_UI_WEBUI_CHROMEOS_SMB_SHARES_SMB_HANDLER_H_
 
+#include <string>
+
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -20,7 +22,11 @@ using smb_client::SmbMountResult;
 
 class SmbHandler : public content::WebUIMessageHandler {
  public:
-  explicit SmbHandler(Profile* profile);
+  using UpdateCredentialsCallback =
+      base::OnceCallback<void(const std::string& username,
+                              const std::string& password)>;
+
+  SmbHandler(Profile* profile, UpdateCredentialsCallback update_cred_callback);
   ~SmbHandler() override;
 
  private:
@@ -51,6 +57,7 @@ class SmbHandler : public content::WebUIMessageHandler {
   bool host_discovery_done_ = false;
   base::OnceClosure stored_mount_call_;
   Profile* const profile_;
+  UpdateCredentialsCallback update_cred_callback_;
   base::WeakPtrFactory<SmbHandler> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(SmbHandler);

chrome/browser/ui/webui/chromeos/smb_shares/smb_share_dialog.cc

@@ -88,8 +88,8 @@ SmbShareDialogUI::SmbShareDialogUI(content::WebUI* web_ui)
   source->SetDefaultResource(IDR_SMB_SHARES_DIALOG_CONTAINER_HTML);
   source->AddResourcePath("smb_share_dialog.js", IDR_SMB_SHARES_DIALOG_JS);
 
-  web_ui->AddMessageHandler(
-      std::make_unique<SmbHandler>(Profile::FromWebUI(web_ui)));
+  web_ui->AddMessageHandler(std::make_unique<SmbHandler>(
+      Profile::FromWebUI(web_ui), base::DoNothing()));
 
   content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), source);
 }

chrome/browser/ui/webui/settings/chromeos/os_settings_ui.cc

@@ -16,6 +16,7 @@
 #include "ash/public/cpp/resources/grit/ash_public_unscaled_resources.h"
 #include "ash/public/cpp/stylus_utils.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/feature_list.h"
 #include "base/metrics/histogram_functions.h"
 #include "build/build_config.h"
@@ -326,7 +327,8 @@ void OSSettingsUI::InitOSWebUIHandlers(content::WebUIDataSource* html_source) {
       std::make_unique<chromeos::settings::InternetHandler>(profile));
   web_ui()->AddMessageHandler(std::make_unique<::settings::TtsHandler>());
   web_ui()->AddMessageHandler(
-      std::make_unique<chromeos::smb_dialog::SmbHandler>(profile));
+      std::make_unique<chromeos::smb_dialog::SmbHandler>(profile,
+                                                         base::DoNothing()));
 
   if (!profile->IsGuestSession()) {
     chromeos::android_sms::AndroidSmsService* android_sms_service =

chrome/test/BUILD.gn

@@ -2489,6 +2489,7 @@ if (!is_android) {
         "../browser/ui/webui/chromeos/login/js_calls_container_test_api.cc",
         "../browser/ui/webui/chromeos/login/js_calls_container_test_api.h",
         "../browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc",
+        "../browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog_browsertest.cc",
         "../browser/ui/webui/chromeos/system_web_dialog_browsertest.cc",
         "../browser/ui/webui/settings/chromeos/account_manager_handler_browsertest.cc",
         "../browser/ui/webui/settings/chromeos/device_power_handler_browsertest.cc",

ui/webui/resources/cr_components/chromeos/smb_shares/smb_browser_proxy.js

@@ -62,7 +62,7 @@ cr.define('smb_shares', function() {
 
     /**
      * Updates the credentials for a mounted share.
-     * @param {number} mountId
+     * @param {string} mountId
      * @param {string} username
      * @param {string} password
      */