Restore username when remounting a saved SMB share.

This change adds the restore path, but usernames are not currently
saved.

BUG=891462

Change-Id: I045fa9d1624a84dd95892e7b3c45d8b303992ef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1644321
Commit-Queue: Anand Mistry <amistry@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666198}

chrome/browser/chromeos/smb_client/smb_file_system_id.cc

@@ -4,13 +4,17 @@
 
 #include "chrome/browser/chromeos/smb_client/smb_file_system_id.h"
 
+#include <string.h>
+
 #include <string>
 
 #include "base/files/file_path.h"
+#include "base/logging.h"
 #include "base/rand_util.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 
 namespace chromeos {
 namespace smb_client {
@@ -18,12 +22,12 @@ namespace {
 
 constexpr char kDelimiter[] = "@@";
 constexpr char kKerberosSymbol[] = "kerberos_chromad";
+constexpr char kUserPrefix[] = "user=";
 constexpr int kRandomIdBytes = 8;
 
 std::vector<std::string> GetComponents(const std::string& file_system_id) {
-  const std::vector<std::string> components =
-      SplitString(file_system_id, kDelimiter, base::TRIM_WHITESPACE,
-                  base::SPLIT_WANT_NONEMPTY);
+  std::vector<std::string> components = SplitStringUsingSubstr(
+      file_system_id, kDelimiter, base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
 
   DCHECK_GE(components.size(), 2u);
   DCHECK_LE(components.size(), 3u);
@@ -51,6 +55,18 @@ std::string CreateFileSystemId(const base::FilePath& share_path,
   return file_system_id;
 }
 
+std::string CreateFileSystemIdForUser(const base::FilePath& share_path,
+                                      const std::string& username) {
+  // Disallow down-level logon names.
+  CHECK_EQ(username.find('\\'), std::string::npos);
+  const std::string base_file_system_id =
+      base::StrCat({GenerateRandomId(), kDelimiter, share_path.value()});
+  if (username.empty()) {
+    return base_file_system_id;
+  }
+  return base::StrCat({base_file_system_id, kDelimiter, kUserPrefix, username});
+}
+
 base::FilePath GetSharePathFromFileSystemId(const std::string& file_system_id) {
   const std::vector<std::string> components = GetComponents(file_system_id);
   DCHECK_GE(components.size(), 1u);
@@ -64,5 +80,16 @@ bool IsKerberosChromadFileSystemId(const std::string& file_system_id) {
   return components.size() >= 3 && components[2] == kKerberosSymbol;
 }
 
+base::Optional<std::string> GetUserFromFileSystemId(
+    const std::string& file_system_id) {
+  const std::vector<std::string> components = GetComponents(file_system_id);
+  if (components.size() < 3 ||
+      !base::StartsWith(components[2], kUserPrefix,
+                        base::CompareCase::SENSITIVE)) {
+    return {};
+  }
+  return components[2].substr(strlen(kUserPrefix));
+}
+
 }  // namespace smb_client
 }  // namespace chromeos

chrome/browser/chromeos/smb_client/smb_file_system_id.h

@@ -7,7 +7,8 @@
 
 #include <string>
 
-#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/optional.h"
 
 namespace chromeos {
 namespace smb_client {
@@ -19,6 +20,14 @@ namespace smb_client {
 std::string CreateFileSystemId(const base::FilePath& share_path,
                                bool is_kerberos_chromad);
 
+// Creates a FileSystemId by concatenating a random filesystem identifier,
+// |share_path|, and |username| with a delimiter. The random ID is used so that
+// the same share path can be mounted multiple times. |username| must be either
+// a name without a domain/workgroup, or in the "user@domain.com" format
+// parsable by ParseUserPrincipalName().
+std::string CreateFileSystemIdForUser(const base::FilePath& share_path,
+                                      const std::string& username);
+
 // Returns the SharePath component of a |file_system_id|. |file_system_id| must
 // be well-formed (e.g. 2@@smb://192.168.1.1/testShare).
 base::FilePath GetSharePathFromFileSystemId(const std::string& file_system_id);
@@ -27,6 +36,12 @@ base::FilePath GetSharePathFromFileSystemId(const std::string& file_system_id);
 // using ChromAD Kerberos.
 bool IsKerberosChromadFileSystemId(const std::string& file_system_id);
 
+// Returns the username if |file_system_id| was constructed with
+// CreateFileSystemIdForUser(). Returns nullopt if |file_system_id| does not
+// store the username.
+base::Optional<std::string> GetUserFromFileSystemId(
+    const std::string& file_system_id);
+
 }  // namespace smb_client
 }  // namespace chromeos
 

chrome/browser/chromeos/smb_client/smb_file_system_id_test.cc

@@ -7,6 +7,7 @@
 #include <string>
 
 #include "base/files/file_path.h"
+#include "base/strings/strcat.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -23,6 +24,9 @@ constexpr char kRandomIdRegex[] =
     "^\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w\\w";
 }  // namespace
 
+constexpr char kTestUsername[] = "foobar";
+constexpr char kTestWorkgroup[] = "accounts.baz.com";
+
 class SmbFileSystemIdTest : public testing::Test {
  public:
   SmbFileSystemIdTest() = default;
@@ -43,6 +47,20 @@ TEST_F(SmbFileSystemIdTest, ShouldCreateFileSystemIdCorrectly) {
                            "@@smb://192.168.0.0/test@@kerberos_chromad"));
 }
 
+TEST_F(SmbFileSystemIdTest, CreateFileSystemIdForUser) {
+  const base::FilePath share_path("smb://192.168.0.0/test");
+
+  EXPECT_THAT(CreateFileSystemIdForUser(share_path, kTestUsername),
+              MatchesRegex(std::string(kRandomIdRegex) +
+                           "@@smb://192.168.0.0/test@@user=" + kTestUsername));
+
+  std::string user_workgroup =
+      base::StrCat({kTestUsername, "@", kTestWorkgroup});
+  EXPECT_THAT(CreateFileSystemIdForUser(share_path, user_workgroup),
+              MatchesRegex(std::string(kRandomIdRegex) +
+                           "@@smb://192.168.0.0/test@@user=" + user_workgroup));
+}
+
 TEST_F(SmbFileSystemIdTest, ShouldParseSharePathCorrectly) {
   // Note: These are still valid because existing users might have saved shares.
   const std::string file_system_id_1 = "12@@smb://192.168.0.0/test";
@@ -72,5 +90,36 @@ TEST_F(SmbFileSystemIdTest, IsKerberosChromadReturnsCorrectly) {
   EXPECT_FALSE(IsKerberosChromadFileSystemId(non_kerberos_file_system_id));
 }
 
+TEST_F(SmbFileSystemIdTest, GetUserFromFileSystemId) {
+  const std::string file_system_id_1 = base::StrCat(
+      {"EFAFF3864D0FE389@@smb://192.168.0.1/test@@user=", kTestUsername});
+  const std::string user_workgroup =
+      base::StrCat({kTestUsername, "@", kTestWorkgroup});
+  const std::string file_system_id_2 = base::StrCat(
+      {"EFAFF3864D0FE389@@smb://192.168.0.1/test@@user=", user_workgroup});
+
+  base::Optional<std::string> actual_user =
+      GetUserFromFileSystemId(file_system_id_1);
+  ASSERT_TRUE(actual_user);
+  EXPECT_EQ(kTestUsername, *actual_user);
+
+  actual_user = GetUserFromFileSystemId(file_system_id_2);
+  ASSERT_TRUE(actual_user);
+  EXPECT_EQ(user_workgroup, *actual_user);
+}
+
+TEST_F(SmbFileSystemIdTest, GetUserFromFileSystemId_NoUser) {
+  const std::string file_system_id_1 =
+      "EFAFF3864D0FE389@@smb://192.168.0.1/test";
+  const std::string file_system_id_2 =
+      "EFAFF3864D0FE389@@smb://192.168.0.1/test@@kerberos_chromad";
+  const std::string file_system_id_3 =
+      "EFAFF3864D0FE389@@smb://192.168.0.1/test@@unrecognised_field";
+
+  EXPECT_FALSE(GetUserFromFileSystemId(file_system_id_1));
+  EXPECT_FALSE(GetUserFromFileSystemId(file_system_id_2));
+  EXPECT_FALSE(GetUserFromFileSystemId(file_system_id_3));
+}
+
 }  // namespace smb_client
 }  // namespace chromeos

chrome/browser/chromeos/smb_client/smb_service.cc

@@ -417,6 +417,13 @@ void SmbService::Remount(const ProvidedFileSystemInfo& file_system_info) {
     DCHECK(user->IsActiveDirectoryUser());
 
     ParseUserPrincipalName(user->GetDisplayEmail(), &username, &workgroup);
+  } else {
+    base::Optional<std::string> user_workgroup =
+        GetUserFromFileSystemId(file_system_info.file_system_id());
+    if (user_workgroup &&
+        !ParseUserName(*user_workgroup, &username, &workgroup)) {
+      LOG(ERROR) << "Failed to parse username/workgroup from file system ID";
+    }
   }
 
   SmbUrl parsed_url(share_path.value());

chrome/browser/chromeos/smb_client/smb_service_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/run_loop.h"
+#include "base/strings/strcat.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h"
@@ -43,7 +44,10 @@ namespace smb_client {
 namespace {
 
 const ProviderId kProviderId = ProviderId::CreateFromNativeId("smb");
-const char kTestDomain[] = "EXAMPLE.COM";
+constexpr char kTestUser[] = "foobar";
+constexpr char kTestDomain[] = "EXAMPLE.COM";
+constexpr char kSharePath[] = "\\\\server\\foobar";
+constexpr char kMountPath[] = "smb://server/foobar";
 
 void SaveMountResult(SmbMountResult* out, SmbMountResult result) {
   *out = result;
@@ -181,9 +185,6 @@ TEST_F(SmbServiceTest, InvalidSsoUrls) {
 }
 
 TEST_F(SmbServiceTest, Remount) {
-  const char* kSharePath = "\\\\server\\foobar";
-  const char* kRemountPath = "smb://server/foobar";
-
   file_system_provider::MountOptions mount_options(
       CreateFileSystemId(base::FilePath(kSharePath),
                          false /* is_kerberos_chromad */),
@@ -196,7 +197,7 @@ TEST_F(SmbServiceTest, Remount) {
   registry_->RememberFileSystem(file_system_info, {});
 
   base::RunLoop run_loop;
-  EXPECT_CALL(*mock_client_, Mount(base::FilePath(kRemountPath), _, "", "", _,
+  EXPECT_CALL(*mock_client_, Mount(base::FilePath(kMountPath), _, "", "", _,
                                    true /* skip_connect */, _))
       .WillOnce(WithArg<6>(
           Invoke([&run_loop](SmbProviderClient::MountCallback callback) {
@@ -213,9 +214,6 @@ TEST_F(SmbServiceTest, Remount) {
 }
 
 TEST_F(SmbServiceTest, Remount_ActiveDirectory) {
-  const char* kSharePath = "\\\\krbserver\\foobar";
-  const char* kRemountPath = "smb://krbserver/foobar";
-
   file_system_provider::MountOptions mount_options(
       CreateFileSystemId(base::FilePath(kSharePath),
                          true /* is_kerberos_chromad */),
@@ -236,8 +234,8 @@ TEST_F(SmbServiceTest, Remount_ActiveDirectory) {
                 FROM_HERE, base::BindOnce(std::move(callback), true));
           })));
   EXPECT_CALL(*mock_client_,
-              Mount(base::FilePath(kRemountPath), _, kTestDomain,
-                    "ad-test-user", _, true /* skip_connect */, _))
+              Mount(base::FilePath(kMountPath), _, kTestDomain, "ad-test-user",
+                    _, true /* skip_connect */, _))
       .WillOnce(WithArg<6>(
           Invoke([&run_loop](SmbProviderClient::MountCallback callback) {
             std::move(callback).Run(smbprovider::ErrorType::ERROR_OK, 7);
@@ -252,6 +250,65 @@ TEST_F(SmbServiceTest, Remount_ActiveDirectory) {
   EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(mock_client_));
 }
 
+TEST_F(SmbServiceTest, Remount_SavedUser) {
+  file_system_provider::MountOptions mount_options(
+      CreateFileSystemIdForUser(base::FilePath(kSharePath),
+                                base::StrCat({kTestUser, "@", kTestDomain})),
+      "Foo");
+  ProvidedFileSystemInfo file_system_info(
+      kProviderId, mount_options, base::FilePath(kSharePath),
+      false /* configurable */, false /* watchable */,
+      extensions::SOURCE_NETWORK, chromeos::file_system_provider::IconSet());
+  CreateFspRegistry(profile_);
+  registry_->RememberFileSystem(file_system_info, {});
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_client_, Mount(base::FilePath(kMountPath), _, kTestDomain,
+                                   kTestUser, _, true /* skip_connect */, _))
+      .WillOnce(WithArg<6>(
+          Invoke([&run_loop](SmbProviderClient::MountCallback callback) {
+            std::move(callback).Run(smbprovider::ErrorType::ERROR_OK, 7);
+            run_loop.Quit();
+          })));
+
+  CreateService(profile_);
+  run_loop.Run();
+
+  // Because the mock is potentially leaked, expectations needs to be manually
+  // verified.
+  EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(mock_client_));
+}
+
+TEST_F(SmbServiceTest, Remount_SavedInvalidUser) {
+  file_system_provider::MountOptions mount_options(
+      CreateFileSystemIdForUser(
+          base::FilePath(kSharePath),
+          base::StrCat({kTestUser, "@", kTestDomain, "@", kTestDomain})),
+      "Foo");
+  ProvidedFileSystemInfo file_system_info(
+      kProviderId, mount_options, base::FilePath(kSharePath),
+      false /* configurable */, false /* watchable */,
+      extensions::SOURCE_NETWORK, chromeos::file_system_provider::IconSet());
+  CreateFspRegistry(profile_);
+  registry_->RememberFileSystem(file_system_info, {});
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_client_, Mount(base::FilePath(kMountPath), _, "", "", _,
+                                   true /* skip_connect */, _))
+      .WillOnce(WithArg<6>(
+          Invoke([&run_loop](SmbProviderClient::MountCallback callback) {
+            std::move(callback).Run(smbprovider::ErrorType::ERROR_OK, 7);
+            run_loop.Quit();
+          })));
+
+  CreateService(profile_);
+  run_loop.Run();
+
+  // Because the mock is potentially leaked, expectations needs to be manually
+  // verified.
+  EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(mock_client_));
+}
+
 TEST_F(SmbServiceTest, Premount) {
   const char kPremountPath[] = "smb://server/foobar";
   const char kPreconfiguredShares[] =