Factor common SMB share parameters into a SmbShareInfo class

A common set of share parameters needs to be passed around. Use a common
class instead of repeatedly passing multiple arguments to functions.

Bug: 939235
Change-Id: Idf41c6e09693c1377cc1613ff18bfe97d7ba3f6c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2049011Reviewed-by: Austin Tankiang <austinct@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740632}

chrome/browser/chromeos/BUILD.gn

@@ -2199,6 +2199,8 @@ source_set("chromeos") {
     "smb_client/smb_service_helper.h",
     "smb_client/smb_share_finder.cc",
     "smb_client/smb_share_finder.h",
+    "smb_client/smb_share_info.cc",
+    "smb_client/smb_share_info.h",
     "smb_client/smb_task_queue.cc",
     "smb_client/smb_task_queue.h",
     "smb_client/smb_url.cc",

chrome/browser/chromeos/smb_client/smb_service.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/chromeos/smb_client/smb_kerberos_credentials_updater.h"
 #include "chrome/browser/chromeos/smb_client/smb_provider.h"
 #include "chrome/browser/chromeos/smb_client/smb_service_helper.h"
+#include "chrome/browser/chromeos/smb_client/smb_share_info.h"
 #include "chrome/browser/chromeos/smb_client/smb_url.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/webui/chromeos/smb_shares/smb_credentials_dialog.h"
@@ -335,8 +336,9 @@ void SmbService::Mount(const file_system_provider::MountOptions& options,
     provider_options.file_system_id =
         CreateFileSystemIdForUser(share_path, full_username);
   }
-  MountInternal(provider_options, parsed_url, options.display_name, username,
-                workgroup, password, use_kerberos, save_credentials,
+  SmbShareInfo info(parsed_url, options.display_name, username, workgroup,
+                    use_kerberos);
+  MountInternal(provider_options, info, password, save_credentials,
                 false /* skip_connect */,
                 base::BindOnce(&SmbService::MountInternalDone,
                                base::Unretained(this), std::move(callback),
@@ -366,12 +368,8 @@ void SmbService::MountInternalDone(MountResponse callback,
 
 void SmbService::MountInternal(
     const file_system_provider::MountOptions& options,
-    const SmbUrl& share_url,
-    const std::string& display_name,
-    const std::string& username,
-    const std::string& workgroup,
+    const SmbShareInfo& info,
     const std::string& password,
-    bool use_kerberos,
     bool save_credentials,
     bool skip_connect,
     MountInternalCallback callback) {
@@ -382,11 +380,11 @@ void SmbService::MountInternal(
   if (IsSmbFsEnabled()) {
     // TODO(amistry): Pass resolved host address to smbfs.
     SmbFsShare::MountOptions smbfs_options;
-    smbfs_options.username = username;
-    smbfs_options.workgroup = workgroup;
+    smbfs_options.username = info.username();
+    smbfs_options.workgroup = info.workgroup();
     smbfs_options.password = password;
     smbfs_options.allow_ntlm = IsNTLMAuthenticationEnabled();
-    if (use_kerberos) {
+    if (info.use_kerberos()) {
       if (user->IsActiveDirectoryUser()) {
         smbfs_options.kerberos_options =
             base::make_optional<SmbFsShare::KerberosOptions>(
@@ -404,8 +402,9 @@ void SmbService::MountInternal(
       }
     }
 
-    std::unique_ptr<SmbFsShare> mount = std::make_unique<SmbFsShare>(
-        profile_, share_url.ToString(), display_name, smbfs_options);
+    std::unique_ptr<SmbFsShare> mount =
+        std::make_unique<SmbFsShare>(profile_, info.share_url().ToString(),
+                                     info.display_name(), smbfs_options);
     SmbFsShare* raw_mount = mount.get();
     const std::string mount_id = mount->mount_id();
     smbfs_shares_[mount_id] = std::move(mount);
@@ -414,15 +413,16 @@ void SmbService::MountInternal(
   } else {
     // If using kerberos, the hostname should not be resolved since kerberos
     // service tickets are keyed on hosname.
-    const SmbUrl url =
-        use_kerberos ? share_url : share_finder_->GetResolvedUrl(share_url);
+    const SmbUrl url = info.use_kerberos()
+                           ? info.share_url()
+                           : share_finder_->GetResolvedUrl(info.share_url());
 
     SmbProviderClient::MountOptions smb_mount_options;
-    smb_mount_options.original_path = share_url.ToString();
-    smb_mount_options.username = username;
-    smb_mount_options.workgroup = workgroup;
+    smb_mount_options.original_path = info.share_url().ToString();
+    smb_mount_options.username = info.username();
+    smb_mount_options.workgroup = info.workgroup();
     smb_mount_options.ntlm_enabled = IsNTLMAuthenticationEnabled();
-    smb_mount_options.save_password = save_credentials && !use_kerberos;
+    smb_mount_options.save_password = save_credentials && !info.use_kerberos();
     smb_mount_options.account_hash = user->username_hash();
     smb_mount_options.skip_connect = skip_connect;
     GetSmbProviderClient()->Mount(
@@ -652,10 +652,11 @@ void SmbService::MountPreconfiguredShare(const SmbUrl& share_url) {
   mount_options.persistent = false;
 
   // Note: Preconfigured shares are mounted without credentials.
+  SmbShareInfo info(share_url, mount_options.display_name, "" /* username */,
+                    "" /* workgroup */, false /* use_kerberos */);
   MountInternal(
-      mount_options, share_url, mount_options.display_name, "" /* username */,
-      "" /* workgroup */, "" /* password */, false /* use_kerberos */,
-      false /* save_credentials */, true /* skip_connect */,
+      mount_options, info, "" /* password */, false /* save_credentials */,
+      true /* skip_connect */,
       base::BindOnce(&SmbService::OnMountPreconfiguredShareDone, AsWeakPtr()));
 }
 

chrome/browser/chromeos/smb_client/smb_service.h

@@ -42,6 +42,7 @@ namespace smb_client {
 using file_system_provider::ProvidedFileSystemInfo;
 
 class SmbKerberosCredentialsUpdater;
+class SmbShareInfo;
 
 // Creates and manages an smb file system.
 class SmbService : public KeyedService,
@@ -133,12 +134,8 @@ class SmbService : public KeyedService,
   // based on feature flags.
   // Calls SmbProviderClient::Mount() or start the smbfs mount process.
   void MountInternal(const file_system_provider::MountOptions& options,
-                     const SmbUrl& share_url,
-                     const std::string& display_name,
-                     const std::string& username,
-                     const std::string& workgroup,
+                     const SmbShareInfo& info,
                      const std::string& password,
-                     bool use_kerberos,
                      bool save_credentials,
                      bool skip_connect,
                      MountInternalCallback callback);

chrome/browser/chromeos/smb_client/smb_service_unittest.cc

@@ -57,6 +57,7 @@ constexpr char kTestPassword[] = "my_secret_password";
 constexpr char kTestDomain[] = "EXAMPLE.COM";
 constexpr char kSharePath[] = "\\\\server\\foobar";
 constexpr char kMountPath[] = "smb://server/foobar";
+constexpr char kDisplayName[] = "My Share";
 
 void SaveMountResult(SmbMountResult* out, SmbMountResult result) {
   *out = result;
@@ -131,6 +132,8 @@ class SmbServiceTest : public testing::Test {
     testing::Mock::AllowLeak(mock_client_);
     chromeos::DBusThreadManager::GetSetterForTesting()->SetSmbProviderClient(
         base::WrapUnique(mock_client_));
+
+    mount_options_.display_name = kDisplayName;
   }
 
   ~SmbServiceTest() override {}
@@ -208,6 +211,8 @@ class SmbServiceTest : public testing::Test {
   file_system_provider::FakeRegistry* registry_;  // Owned by |fsp_service_|.
   // Extension Registry and Registry needed for fsp_service.
   std::unique_ptr<extensions::ExtensionRegistry> extension_registry_;
+
+  chromeos::file_system_provider::MountOptions mount_options_;
 };
 
 TEST_F(SmbServiceTest, InvalidUrls) {
@@ -260,7 +265,7 @@ TEST_F(SmbServiceTest, Mount) {
           })));
 
   smb_service_->Mount(
-      {}, base::FilePath(kSharePath), kTestUser, kTestPassword,
+      mount_options_, base::FilePath(kSharePath), kTestUser, kTestPassword,
       false /* use_chromad_kerberos */,
       false /* should_open_file_manager_after_mount */,
       false /* save_credentials */,
@@ -307,7 +312,7 @@ TEST_F(SmbServiceTest, MountSaveCredentials) {
           })));
 
   smb_service_->Mount(
-      {}, base::FilePath(kSharePath), kTestUser, kTestPassword,
+      mount_options_, base::FilePath(kSharePath), kTestUser, kTestPassword,
       false /* use_chromad_kerberos */,
       false /* should_open_file_manager_after_mount */,
       true /* save_credentials */,

chrome/browser/chromeos/smb_client/smb_share_info.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/chromeos/smb_client/smb_share_info.h"
+
+#include "base/logging.h"
+
+namespace chromeos {
+namespace smb_client {
+
+SmbShareInfo::SmbShareInfo(const SmbUrl& share_url,
+                           const std::string& display_name,
+                           const std::string& username,
+                           const std::string& workgroup,
+                           bool use_kerberos)
+    : share_url_(share_url),
+      display_name_(display_name),
+      username_(username),
+      workgroup_(workgroup),
+      use_kerberos_(use_kerberos) {
+  DCHECK(share_url_.IsValid());
+  DCHECK(!display_name.empty());
+}
+
+SmbShareInfo::~SmbShareInfo() = default;
+
+SmbShareInfo::SmbShareInfo(const SmbShareInfo&) = default;
+
+SmbShareInfo& SmbShareInfo::operator=(const SmbShareInfo&) = default;
+
+}  // namespace smb_client
+}  // namespace chromeos

chrome/browser/chromeos/smb_client/smb_share_info.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 CHROME_BROWSER_CHROMEOS_SMB_CLIENT_SMB_SHARE_INFO_H_
+#define CHROME_BROWSER_CHROMEOS_SMB_CLIENT_SMB_SHARE_INFO_H_
+
+#include <string>
+
+#include "chrome/browser/chromeos/smb_client/smb_url.h"
+
+namespace chromeos {
+namespace smb_client {
+
+// Common parameters for SMB shares.
+// Note: Password is explicitly excluded here. Due to being sensitive
+// information, it should be considered separate to other parameters.
+class SmbShareInfo {
+ public:
+  SmbShareInfo(const SmbUrl& share_url,
+               const std::string& display_name,
+               const std::string& username,
+               const std::string& workgroup,
+               bool use_kerberos);
+  ~SmbShareInfo();
+
+  // Allow copies.
+  SmbShareInfo(const SmbShareInfo&);
+  SmbShareInfo& operator=(const SmbShareInfo&);
+
+  // Disallow creating an empty instance.
+  SmbShareInfo() = delete;
+
+  const SmbUrl& share_url() const { return share_url_; }
+  const std::string& display_name() const { return display_name_; }
+  const std::string& username() const { return username_; }
+  const std::string& workgroup() const { return workgroup_; }
+  bool use_kerberos() const { return use_kerberos_; }
+
+ private:
+  SmbUrl share_url_;
+  std::string display_name_;
+  std::string username_;
+  std::string workgroup_;
+  bool use_kerberos_ = false;
+};
+
+}  // namespace smb_client
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_SMB_CLIENT_SMB_SHARE_INFO_H_