Use username from cicerone when doing sshfs for crostini mount

Chrome creates the container with default username derived from
the email address of the current logged in profile.
E.g. test.user@gmail.com gets username 'testuser'.

It is possible for users to change the username inside the container.
As long as the user keeps uid=1000, this does not cause issues
except for the sshfs mount where FilesApp connects to the container
using 'sshfs://username@hostname'.

Cicerone is now updated to return the container username in
ContainerStartedSignal and in SetUpLxdContainerUser.  CrostiniManager
can now use this for the sshfs mount.

Removed crostini_util ContainerHomeDirectoryForProfile which was
using the default username.  The container homedir is now available
in CrostiniManager::GetContainerInfo.

Bug: 916297
Change-Id: I27f41647c3b8809170521d741867a8ed430e5476
Reviewed-on: https://chromium-review.googlesource.com/c/1388124
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619897}

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -74,6 +74,22 @@ enum class UninstallPackageProgressStatus {
   UNINSTALLING,  // In progress
 };
 
+struct VmInfo {
+  VmState state;
+  vm_tools::concierge::VmInfo info;
+};
+
+struct ContainerInfo {
+  ContainerInfo(std::string name, std::string username, std::string homedir);
+  ~ContainerInfo();
+  ContainerInfo(const ContainerInfo&);
+
+  std::string name;
+  std::string username;
+  base::FilePath homedir;
+  bool sshfs_mounted = false;
+};
+
 // Return type when getting app icons from within a container.
 struct Icon {
   std::string desktop_file_id;
@@ -429,12 +445,16 @@ class CrostiniManager : public KeyedService,
 
   void SetVmState(std::string vm_name, VmState vm_state);
   bool IsVmRunning(std::string vm_name);
-
   // Returns null if VM is not running.
-  base::Optional<vm_tools::concierge::VmInfo> GetVmInfo(std::string vm_name);
-  void AddRunningVmForTesting(std::string vm_name,
-                              vm_tools::concierge::VmInfo vm_info);
-  bool IsContainerRunning(std::string vm_name, std::string container_name);
+  base::Optional<VmInfo> GetVmInfo(std::string vm_name);
+  void AddRunningVmForTesting(std::string vm_name);
+
+  void SetContainerSshfsMounted(std::string vm_name,
+                                std::string container_name);
+  // Returns null if VM or container is not running.
+  base::Optional<ContainerInfo> GetContainerInfo(std::string vm_name,
+                                                 std::string container_name);
+  void AddRunningContainerForTesting(std::string vm_name, ContainerInfo info);
 
   // If the Crostini reporting policy is set, save the last app launch
   // time window and the Termina version in prefs for asynchronous reporting.
@@ -612,11 +632,10 @@ class CrostiniManager : public KeyedService,
   // start.
   std::multimap<std::string, base::OnceClosure> tremplin_started_callbacks_;
 
-  std::map<std::string, std::pair<VmState, vm_tools::concierge::VmInfo>>
-      running_vms_;
+  std::map<std::string, VmInfo> running_vms_;
 
   // Running containers as keyed by vm name.
-  std::multimap<std::string, std::string> running_containers_;
+  std::multimap<std::string, ContainerInfo> running_containers_;
 
   std::vector<RemoveCrostiniCallback> remove_crostini_callbacks_;
 

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -715,6 +715,10 @@ TEST_F(CrostiniManagerRestartTest, MountForTerminaPenguin) {
   EXPECT_TRUE(fake_concierge_client_->create_disk_image_called());
   EXPECT_TRUE(fake_concierge_client_->start_termina_vm_called());
   EXPECT_TRUE(fake_concierge_client_->get_container_ssh_keys_called());
+  EXPECT_TRUE(crostini_manager()
+                  ->GetContainerInfo(kCrostiniDefaultVmName,
+                                     kCrostiniDefaultContainerName)
+                  ->sshfs_mounted);
   EXPECT_EQ(1, restart_crostini_callback_count_);
   base::FilePath path;
   EXPECT_TRUE(
@@ -741,7 +745,7 @@ TEST_F(CrostiniManagerRestartTest, IsContainerRunningFalseIfVmNotStarted) {
   EXPECT_EQ(1, restart_crostini_callback_count_);
 
   EXPECT_TRUE(crostini_manager()->IsVmRunning(kVmName));
-  EXPECT_TRUE(crostini_manager()->IsContainerRunning(kVmName, kContainerName));
+  EXPECT_TRUE(crostini_manager()->GetContainerInfo(kVmName, kContainerName));
 
   // Now call StartTerminaVm again. The default response state is "STARTING",
   // so no container should be considered running.
@@ -754,7 +758,7 @@ TEST_F(CrostiniManagerRestartTest, IsContainerRunningFalseIfVmNotStarted) {
                      base::Unretained(this), run_loop2.QuitClosure()));
   run_loop2.Run();
   EXPECT_TRUE(crostini_manager()->IsVmRunning(kVmName));
-  EXPECT_FALSE(crostini_manager()->IsContainerRunning(kVmName, kContainerName));
+  EXPECT_FALSE(crostini_manager()->GetContainerInfo(kVmName, kContainerName));
 }
 
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_share_path.cc

@@ -48,13 +48,13 @@ void OnVmRestartedForSeneschal(
     vm_tools::seneschal::SharePathRequest request,
     crostini::CrostiniResult result) {
   auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile);
-  base::Optional<vm_tools::concierge::VmInfo> vm_info =
+  base::Optional<crostini::VmInfo> vm_info =
       crostini_manager->GetVmInfo(std::move(vm_name));
-  if (!vm_info) {
+  if (!vm_info || vm_info->state != crostini::VmState::STARTED) {
     std::move(callback).Run(false, "VM could not be started");
     return;
   }
-  request.set_handle(vm_info->seneschal_server_handle());
+  request.set_handle(vm_info->info.seneschal_server_handle());
   chromeos::DBusThreadManager::Get()->GetSeneschalClient()->SharePath(
       request,
       base::BindOnce(&OnSeneschalSharePathResponse, std::move(callback)));
@@ -240,9 +240,9 @@ void CrostiniSharePath::CallSeneschalSharePath(
 
   // Restart VM if not currently running.
   auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile_);
-  base::Optional<vm_tools::concierge::VmInfo> vm_info =
+  base::Optional<crostini::VmInfo> vm_info =
       crostini_manager->GetVmInfo(vm_name);
-  if (!vm_info) {
+  if (!vm_info || vm_info->state != crostini::VmState::STARTED) {
     crostini_manager->RestartCrostini(
         vm_name, crostini::kCrostiniDefaultContainerName,
         base::BindOnce(&OnVmRestartedForSeneschal, profile_, std::move(vm_name),
@@ -251,7 +251,7 @@ void CrostiniSharePath::CallSeneschalSharePath(
     return;
   }
 
-  request.set_handle(vm_info->seneschal_server_handle());
+  request.set_handle(vm_info->info.seneschal_server_handle());
   chromeos::DBusThreadManager::Get()->GetSeneschalClient()->SharePath(
       request,
       base::BindOnce(&OnSeneschalSharePathResponse, std::move(callback)));
@@ -263,9 +263,9 @@ void CrostiniSharePath::CallSeneschalUnsharePath(
     base::OnceCallback<void(bool, std::string)> callback) {
   // Return success if VM is not currently running.
   auto* crostini_manager = crostini::CrostiniManager::GetForProfile(profile_);
-  base::Optional<vm_tools::concierge::VmInfo> vm_info =
-      crostini_manager->GetVmInfo(vm_name);
-  if (!vm_info) {
+  base::Optional<crostini::VmInfo> vm_info =
+      crostini_manager->GetVmInfo(std::move(vm_name));
+  if (!vm_info || vm_info->state != crostini::VmState::STARTED) {
     std::move(callback).Run(true, "VM not running");
     return;
   }
@@ -292,7 +292,7 @@ void CrostiniSharePath::CallSeneschalUnsharePath(
   }
 
   vm_tools::seneschal::UnsharePathRequest request;
-  request.set_handle(vm_info->seneschal_server_handle());
+  request.set_handle(vm_info->info.seneschal_server_handle());
   request.set_path(unshare_path.value());
   chromeos::DBusThreadManager::Get()->GetSeneschalClient()->UnsharePath(
       request,

chrome/browser/chromeos/crostini/crostini_share_path_unittest.cc

@@ -208,9 +208,8 @@ class CrostiniSharePathTest : public testing::Test {
     volume_downloads_ = file_manager::Volume::CreateForDownloads(downloads_);
 
     // Create 'vm-running' VM instance which is running.
-    vm_tools::concierge::VmInfo vm_info;
     CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-        "vm-running", vm_info);
+        "vm-running");
   }
 
   void TearDown() override {
@@ -499,9 +498,8 @@ TEST_F(CrostiniSharePathTest, SharePersistedPaths) {
   features_.InitAndEnableFeature(chromeos::features::kCrostiniFiles);
   base::FilePath share_path2_ = downloads_.AppendASCII("path-to-share-2");
   ASSERT_TRUE(base::CreateDirectory(share_path2_));
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
   base::ListValue shared_paths = base::ListValue();
   shared_paths.GetList().push_back(base::Value(share_path_.value()));
   shared_paths.GetList().push_back(base::Value(share_path2_.value()));
@@ -695,9 +693,8 @@ TEST_F(CrostiniSharePathTest, GetPersistedSharedPaths) {
 
 TEST_F(CrostiniSharePathTest, ShareOnMountSuccessParentMount) {
   features_.InitAndEnableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
   crostini_share_path_->SetMountEventSeneschalCallbackForTesting(
       base::BindRepeating(&CrostiniSharePathTest::MountEventSharePathCallback,
                           base::Unretained(this), "share-on-mount",
@@ -711,9 +708,8 @@ TEST_F(CrostiniSharePathTest, ShareOnMountSuccessParentMount) {
 
 TEST_F(CrostiniSharePathTest, ShareOnMountSuccessSelfMount) {
   features_.InitAndEnableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
   auto volume_shared_path =
       file_manager::Volume::CreateForDownloads(shared_path_);
   crostini_share_path_->SetMountEventSeneschalCallbackForTesting(
@@ -729,9 +725,8 @@ TEST_F(CrostiniSharePathTest, ShareOnMountSuccessSelfMount) {
 
 TEST_F(CrostiniSharePathTest, ShareOnMountFeatureNotEnabled) {
   features_.InitAndDisableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
 
   // Test mount.
   crostini_share_path_->OnVolumeMounted(chromeos::MountError::MOUNT_ERROR_NONE,
@@ -746,9 +741,8 @@ TEST_F(CrostiniSharePathTest, ShareOnMountFeatureNotEnabled) {
 
 TEST_F(CrostiniSharePathTest, ShareOnMountMountError) {
   features_.InitAndDisableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
 
   // Test mount.
   crostini_share_path_->OnVolumeMounted(
@@ -793,9 +787,8 @@ TEST_F(CrostiniSharePathTest, ShareOnMountVolumeUnrelated) {
 
 TEST_F(CrostiniSharePathTest, UnshareOnUnmountSuccessParentMount) {
   features_.InitAndEnableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
   crostini_share_path_->SetMountEventSeneschalCallbackForTesting(
       base::BindRepeating(
           &CrostiniSharePathTest::MountEventUnsharePathCallback,
@@ -809,9 +802,8 @@ TEST_F(CrostiniSharePathTest, UnshareOnUnmountSuccessParentMount) {
 
 TEST_F(CrostiniSharePathTest, UnshareOnUnmountSuccessSelfMount) {
   features_.InitAndEnableFeature(chromeos::features::kCrostiniFiles);
-  vm_tools::concierge::VmInfo vm_info;
   CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
-      kCrostiniDefaultVmName, vm_info);
+      kCrostiniDefaultVmName);
   auto volume_shared_path =
       file_manager::Volume::CreateForDownloads(shared_path_);
   crostini_share_path_->SetMountEventSeneschalCallbackForTesting(

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -384,7 +384,7 @@ std::string CryptohomeIdForProfile(Profile* profile) {
   return id.empty() ? "test" : id;
 }
 
-std::string ContainerUserNameForProfile(Profile* profile) {
+std::string DefaultContainerUserNameForProfile(Profile* profile) {
   // Get rid of the @domain.name in the profile user name (an email address).
   std::string container_username = profile->GetProfileUserName();
   if (container_username.find('@') != std::string::npos) {
@@ -396,10 +396,6 @@ std::string ContainerUserNameForProfile(Profile* profile) {
   return container_username;
 }
 
-base::FilePath ContainerHomeDirectoryForProfile(Profile* profile) {
-  return base::FilePath("/home/" + ContainerUserNameForProfile(profile));
-}
-
 base::FilePath ContainerChromeOSBaseDirectory() {
   return base::FilePath("/mnt/chromeos");
 }

chrome/browser/chromeos/crostini/crostini_util.h

@@ -78,10 +78,7 @@ std::string CryptohomeIdForProfile(Profile* profile);
 
 // Retrieves username from profile.  This is the text until '@' in
 // profile->GetProfileUserName() email address.
-std::string ContainerUserNameForProfile(Profile* profile);
-
-// Returns the home directory within the container for a given profile.
-base::FilePath ContainerHomeDirectoryForProfile(Profile* profile);
+std::string DefaultContainerUserNameForProfile(Profile* profile);
 
 // Returns the mount directory within the container where paths from the Chrome
 // OS host such as within Downloads are shared with the container.

chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc

@@ -551,9 +551,11 @@ IN_PROC_BROWSER_TEST_F(FileManagerPrivateApiTest, Crostini) {
   crostini::CrostiniManager* crostini_manager =
       crostini::CrostiniManager::GetForProfile(browser()->profile());
   crostini_manager->set_skip_restart_for_testing();
-  vm_tools::concierge::VmInfo vm_info;
-  crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName,
-                                           std::move(vm_info));
+  crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName);
+  crostini_manager->AddRunningContainerForTesting(
+      crostini::kCrostiniDefaultVmName,
+      crostini::ContainerInfo(crostini::kCrostiniDefaultContainerName,
+                              "testuser", "/home/testuser"));
 
   ExpectCrostiniMount();
 
@@ -585,8 +587,17 @@ IN_PROC_BROWSER_TEST_F(FileManagerPrivateApiTest, Crostini) {
 IN_PROC_BROWSER_TEST_F(FileManagerPrivateApiTest, CrostiniIncognito) {
   base::test::ScopedFeatureList scoped_feature_list;
   EnableCrostiniForProfile(&scoped_feature_list);
-  crostini::CrostiniManager::GetForProfile(browser()->profile())
-      ->set_skip_restart_for_testing();
+
+  // Setup CrostiniManager for testing.
+  crostini::CrostiniManager* crostini_manager =
+      crostini::CrostiniManager::GetForProfile(browser()->profile());
+  crostini_manager->set_skip_restart_for_testing();
+  crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName);
+  crostini_manager->AddRunningContainerForTesting(
+      crostini::kCrostiniDefaultVmName,
+      crostini::ContainerInfo(crostini::kCrostiniDefaultContainerName,
+                              "testuser", "/home/testuser"));
+
   ExpectCrostiniMount();
 
   scoped_refptr<extensions::FileManagerPrivateMountCrostiniFunction> function(

chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc

@@ -1252,13 +1252,21 @@ void FileManagerBrowserTestBase::SetUpOnMainThread() {
     drive_volume_ = drive_volumes_[profile()->GetOriginalProfile()].get();
     test_util::WaitUntilDriveMountPointIsAdded(profile());
 
-    // Init crostini.  Set prefs to enable crostini and register
-    // CustomMountPointCallback. TODO(joelhockey): It would be better if the
-    // crostini interface allowed for testing without such tight coupling.
+    // Init crostini.  Set prefs to enable crostini, set VM and container
+    // running for testing, and register CustomMountPointCallback.
+    // TODO(joelhockey): It would be better if the crostini interface allowed
+    // for testing without such tight coupling.
     crostini_volume_ = std::make_unique<CrostiniTestVolume>();
     profile()->GetPrefs()->SetBoolean(crostini::prefs::kCrostiniEnabled, true);
-    crostini::CrostiniManager::GetForProfile(profile()->GetOriginalProfile())
-        ->set_skip_restart_for_testing();
+    crostini::CrostiniManager* crostini_manager =
+        crostini::CrostiniManager::GetForProfile(
+            profile()->GetOriginalProfile());
+    crostini_manager->set_skip_restart_for_testing();
+    crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName);
+    crostini_manager->AddRunningContainerForTesting(
+        crostini::kCrostiniDefaultVmName,
+        crostini::ContainerInfo(crostini::kCrostiniDefaultContainerName,
+                                "testuser", "/home/testuser"));
     chromeos::DBusThreadManager* dbus_thread_manager =
         chromeos::DBusThreadManager::Get();
     static_cast<chromeos::FakeCrosDisksClient*>(
@@ -1733,13 +1741,7 @@ base::FilePath FileManagerBrowserTestBase::MaybeMountCrostini(
   if (source_url.scheme() != "sshfs") {
     return {};
   }
-  // Mount crostini volume, and set VM now running for CrostiniManager.
   CHECK(crostini_volume_->Mount(profile()));
-  crostini::CrostiniManager* crostini_manager =
-      crostini::CrostiniManager::GetForProfile(profile()->GetOriginalProfile());
-  vm_tools::concierge::VmInfo vm_info;
-  crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName,
-                                           std::move(vm_info));
   return crostini_volume_->mount_path();
 }
 

chrome/browser/chromeos/file_manager/path_util.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h"
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h"
 #include "chrome/browser/chromeos/arc/fileapi/chrome_content_provider_url_util.h"
+#include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/drive/drive_integration_service.h"
 #include "chrome/browser/chromeos/fileapi/external_file_url_util.h"
@@ -270,7 +271,14 @@ bool ConvertFileSystemURLToPathInsideCrostini(
   base::FilePath base_to_exclude(id);
   if (id == GetCrostiniMountPointName(profile)) {
     // Crostini.
-    *inside = crostini::ContainerHomeDirectoryForProfile(profile);
+    base::Optional<crostini::ContainerInfo> container_info =
+        crostini::CrostiniManager::GetForProfile(profile)->GetContainerInfo(
+            crostini::kCrostiniDefaultVmName,
+            crostini::kCrostiniDefaultContainerName);
+    if (!container_info) {
+      return false;
+    }
+    *inside = container_info->homedir;
   } else if (id == GetDownloadsMountPointName(profile)) {
     // MyFiles or Downloads.
     if (base::FeatureList::IsEnabled(chromeos::features::kMyFilesVolume)) {

chrome/browser/chromeos/file_manager/path_util_unittest.cc

@@ -14,6 +14,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h"
 #include "chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h"
+#include "chrome/browser/chromeos/crostini/crostini_manager.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/drive/drive_integration_service.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
 #include "chrome/browser/chromeos/file_manager/fake_disk_mount_manager.h"
@@ -26,6 +28,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/account_id/account_id.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/arc_service_manager.h"
@@ -327,6 +330,15 @@ TEST(FileManagerPathUtilTest, ConvertFileSystemURLToPathInsideCrostini) {
       AccountId::FromUserEmailGaiaId(profile.GetProfileUserName(), "12345"));
   profile.GetPrefs()->SetString(drive::prefs::kDriveFsProfileSalt, "a");
 
+  // Initialize DBUS and running container.
+  chromeos::DBusThreadManager::Initialize();
+  crostini::CrostiniManager* crostini_manager =
+      crostini::CrostiniManager::GetForProfile(&profile);
+  crostini_manager->AddRunningVmForTesting(crostini::kCrostiniDefaultVmName);
+  crostini_manager->AddRunningContainerForTesting(
+      crostini::kCrostiniDefaultVmName,
+      crostini::ContainerInfo(crostini::kCrostiniDefaultContainerName,
+                              "testuser", "/home/testuser"));
   {
     base::test::ScopedFeatureList features;
     features.InitAndEnableFeature(chromeos::features::kDriveFs);
@@ -362,7 +374,7 @@ TEST(FileManagerPathUtilTest, ConvertFileSystemURLToPathInsideCrostini) {
             GURL(), "crostini_0123456789abcdef_termina_penguin",
             base::FilePath("path/in/crostini")),
         &inside));
-    EXPECT_EQ("/home/testing_profile/path/in/crostini", inside.value());
+    EXPECT_EQ("/home/testuser/path/in/crostini", inside.value());
 
     EXPECT_TRUE(ConvertFileSystemURLToPathInsideCrostini(
         &profile,