Commit 455760fe authored by Joel Hockey's avatar Joel Hockey Committed by Commit Bot

Crostini: re-share all paths with container at startup

Read list of persisted shared paths from prefs, and re-share
with seneschal/9p when container starts.

Bug: 878324
Change-Id: Iea0ff71b79e5787d06b9d1b42a985cea2fb2631b
Reviewed-on: https://chromium-review.googlesource.com/1232793
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: default avatarNicholas Verne <nverne@chromium.org>
Reviewed-by: default avatarStuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592724}
parent 62cc38ff
......@@ -16,6 +16,7 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chromeos/crostini/crostini_manager_factory.h"
#include "chrome/browser/chromeos/crostini/crostini_remover.h"
#include "chrome/browser/chromeos/crostini/crostini_share_path.h"
#include "chrome/browser/chromeos/crostini/crostini_util.h"
#include "chrome/browser/chromeos/file_manager/path_util.h"
#include "chrome/browser/chromeos/file_manager/volume_manager.h"
......@@ -271,7 +272,8 @@ class CrostiniManager::CrostiniRestarter
return;
}
// If default termina/penguin, then do sshfs mount, else we are finished.
// If default termina/penguin, then do sshfs mount and reshare folders,
// else we are finished.
if (vm_name_ == kCrostiniDefaultVmName &&
container_name_ == kCrostiniDefaultContainerName) {
crostini_manager_->GetContainerSshKeys(
......@@ -348,11 +350,15 @@ class CrostiniManager::CrostiniRestarter
vmgr->AddSshfsCrostiniVolume(mount_path);
}
// Abort not checked until end of function. On abort, do not call
// FinishRestart, but still remove observer and add volume as per above.
// Abort not checked until end of function. On abort, do not continue,
// but still remove observer and add volume as per above.
if (is_aborted_)
return;
FinishRestart(ConciergeClientResult::SUCCESS);
// Share folders from Downloads, etc with container.
ShareAllPaths(profile_,
base::BindOnce(&CrostiniRestarter::FinishRestart, this,
ConciergeClientResult::SUCCESS));
}
Profile* profile_;
......
......@@ -4,67 +4,110 @@
#include "chrome/browser/chromeos/crostini/crostini_share_path.h"
#include "base/barrier_closure.h"
#include "base/bind.h"
#include "base/files/file_util.h"
#include "base/optional.h"
#include "chrome/browser/chromeos/crostini/crostini_manager.h"
#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
#include "chrome/browser/chromeos/crostini/crostini_util.h"
#include "chrome/browser/chromeos/file_manager/path_util.h"
#include "chrome/browser/profiles/profile.h"
#include "chromeos/dbus/concierge/service.pb.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/seneschal_client.h"
#include "components/prefs/pref_service.h"
#include "content/public/browser/browser_thread.h"
namespace crostini {
namespace {
// static
CrostiniSharePath* CrostiniSharePath::GetInstance() {
return base::Singleton<CrostiniSharePath>::get();
void OnSeneschalSharePathResponse(
base::FilePath path,
base::OnceCallback<void(bool, std::string)> callback,
base::Optional<vm_tools::seneschal::SharePathResponse> response) {
if (!response) {
std::move(callback).Run(false, "System error");
return;
}
std::move(callback).Run(response.value().success(),
response.value().failure_reason());
}
CrostiniSharePath::CrostiniSharePath() : weak_ptr_factory_(this) {}
CrostiniSharePath::~CrostiniSharePath() {}
void CrostiniSharePath::SharePath(
void CallSeneschalSharePath(
Profile* profile,
std::string vm_name,
std::string path,
const base::FilePath path,
base::OnceCallback<void(bool, std::string)> callback) {
// TODO(joelhockey): Save new path into prefs once management UI is ready.
// Verify path is in one of the allowable mount points.
// This logic is similar to DownloadPrefs::SanitizeDownloadTargetPath().
// TODO(joelhockey): Seneschal currently only support sharing directories
// under Downloads.
if (!path.IsAbsolute()) {
std::move(callback).Run(false, "Path must be absolute");
return;
}
base::FilePath downloads =
file_manager::util::GetDownloadsFolderForProfile(profile);
base::FilePath relative_path;
if (!downloads.AppendRelativePath(path, &relative_path)) {
std::move(callback).Run(false, "Path must be under Downloads");
return;
}
// VM must be running.
base::Optional<vm_tools::concierge::VmInfo> vm_info =
crostini::CrostiniManager::GetForProfile(profile)->GetVmInfo(
std::move(vm_name));
if (!vm_info) {
std::move(callback).Run(false, "Cannot share, VM not running");
std::move(callback).Run(false, "VM not running");
return;
}
// Path must be a valid directory.
if (!base::DirectoryExists(path)) {
std::move(callback).Run(false, "Path is not a valid directory");
return;
}
vm_tools::seneschal::SharePathRequest request;
request.set_handle(vm_info->seneschal_server_handle());
request.mutable_shared_path()->set_path(path);
request.mutable_shared_path()->set_path(relative_path.value());
request.mutable_shared_path()->set_writable(true);
request.set_storage_location(
vm_tools::seneschal::SharePathRequest::DOWNLOADS);
request.set_owner_id(CryptohomeIdForProfile(profile));
chromeos::DBusThreadManager::Get()->GetSeneschalClient()->SharePath(
request, base::BindOnce(&CrostiniSharePath::OnSharePathResponse,
weak_ptr_factory_.GetWeakPtr(), std::move(path),
request, base::BindOnce(&OnSeneschalSharePathResponse, std::move(path),
std::move(callback)));
}
void CrostiniSharePath::OnSharePathResponse(
std::string path,
base::OnceCallback<void(bool, std::string)> callback,
base::Optional<vm_tools::seneschal::SharePathResponse> response) const {
if (!response.has_value()) {
std::move(callback).Run(false, "Error sharing " + path);
return;
void SharePathLogErrorCallback(std::string path,
base::RepeatingClosure barrier,
bool success,
std::string failure_reason) {
barrier.Run();
if (!success) {
LOG(ERROR) << "Error SharePath=" << path
<< ", FailureReason=" << failure_reason;
}
std::move(callback).Run(response.value().success(),
response.value().failure_reason());
}
std::vector<std::string> CrostiniSharePath::GetSharedPaths(Profile* profile) {
} // namespace
namespace crostini {
void SharePath(Profile* profile,
std::string vm_name,
const base::FilePath& path,
base::OnceCallback<void(bool, std::string)> callback) {
DCHECK(profile);
DCHECK(callback);
// TODO(joelhockey): Save new path into prefs once management UI is ready.
CallSeneschalSharePath(profile, vm_name, path, std::move(callback));
}
std::vector<std::string> GetSharedPaths(Profile* profile) {
DCHECK(profile);
std::vector<std::string> result;
const base::ListValue* shared_paths =
profile->GetPrefs()->GetList(prefs::kCrostiniSharedPaths);
......@@ -74,4 +117,16 @@ std::vector<std::string> CrostiniSharePath::GetSharedPaths(Profile* profile) {
return result;
}
void ShareAllPaths(Profile* profile, base::OnceCallback<void()> callback) {
DCHECK(profile);
std::vector<std::string> paths = GetSharedPaths(profile);
base::RepeatingClosure barrier =
base::BarrierClosure(paths.size(), std::move(callback));
for (const auto& path : paths) {
CallSeneschalSharePath(
profile, kCrostiniDefaultVmName, base::FilePath(path),
base::BindOnce(&SharePathLogErrorCallback, std::move(path), barrier));
}
}
} // namespace crostini
......@@ -7,44 +7,27 @@
#include <vector>
#include "base/macros.h"
#include "base/memory/singleton.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/callback.h"
#include "base/files/file_path.h"
#include "chromeos/dbus/seneschal/seneschal_service.pb.h"
class Profile;
namespace crostini {
class CrostiniSharePath {
public:
// Returns the singleton instance of CrostiniSharePath.
static CrostiniSharePath* GetInstance();
// Share specified absolute path with vm.
// Callback receives success bool and failure reason string.
void SharePath(Profile* profile,
std::string vm_name,
const base::FilePath& path,
base::OnceCallback<void(bool, std::string)> callback);
// Share specified path with vm.
void SharePath(Profile* profile,
std::string vm_name,
std::string path,
base::OnceCallback<void(bool, std::string)> callback);
void OnSharePathResponse(
std::string path,
base::OnceCallback<void(bool, std::string)> callback,
base::Optional<vm_tools::seneschal::SharePathResponse> response) const;
// Get list of all shared paths for the default crostini container.
std::vector<std::string> GetSharedPaths(Profile* profile);
// Get list of all shared paths for the default crostini container.
std::vector<std::string> GetSharedPaths(Profile* profile);
private:
friend struct base::DefaultSingletonTraits<CrostiniSharePath>;
CrostiniSharePath();
~CrostiniSharePath();
base::WeakPtrFactory<CrostiniSharePath> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(CrostiniSharePath);
};
// Share all paths configured in prefs for the default crostini container.
// Called at container startup. Callback is invoked once complete.
void ShareAllPaths(Profile* profile, base::OnceCallback<void()> callback);
} // namespace crostini
......
......@@ -4,27 +4,38 @@
#include "chrome/browser/chromeos/crostini/crostini_share_path.h"
#include "base/bind.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/run_loop.h"
#include "base/sys_info.h"
#include "base/test/scoped_task_environment.h"
#include "chrome/browser/chromeos/crostini/crostini_manager.h"
#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
#include "chrome/browser/chromeos/crostini/crostini_util.h"
#include "chrome/browser/chromeos/file_manager/path_util.h"
#include "chrome/test/base/testing_profile.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/fake_cicerone_client.h"
#include "chromeos/dbus/fake_concierge_client.h"
#include "chromeos/dbus/fake_seneschal_client.h"
#include "components/prefs/pref_service.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace crostini {
const char kLsbRelease[] =
"CHROMEOS_RELEASE_NAME=Chrome OS\n"
"CHROMEOS_RELEASE_VERSION=1.2.3.4\n";
class CrostiniSharePathTest : public testing::Test {
public:
void SharePathSuccessStartTerminaVmCallback(ConciergeClientResult result) {
EXPECT_TRUE(fake_concierge_client_->start_termina_vm_called());
EXPECT_EQ(result, ConciergeClientResult::SUCCESS);
CrostiniSharePath::GetInstance()->SharePath(
profile(), "vm-running-success", "path",
SharePath(
profile(), "success", share_path_,
base::BindOnce(&CrostiniSharePathTest::SharePathSuccessCallback,
base::Unretained(this), run_loop()->QuitClosure()));
}
......@@ -38,22 +49,26 @@ class CrostiniSharePathTest : public testing::Test {
std::move(closure).Run();
}
void SharePathErrorStartTerminaVmCallback(ConciergeClientResult result) {
void SharePathErrorSeneschalStartTerminaVmCallback(
ConciergeClientResult result) {
EXPECT_TRUE(fake_concierge_client_->start_termina_vm_called());
EXPECT_EQ(result, ConciergeClientResult::SUCCESS);
CrostiniSharePath::GetInstance()->SharePath(
profile(), "vm-running-error", "path",
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), run_loop()->QuitClosure()));
SharePath(profile(), "error-seneschal", share_path_,
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), true, "test failure",
run_loop()->QuitClosure()));
}
void SharePathErrorCallback(base::OnceClosure closure,
void SharePathErrorCallback(bool expected_seneschal_client_called,
std::string expected_failure_reason,
base::OnceClosure closure,
bool success,
std::string failure_reason) {
EXPECT_TRUE(fake_seneschal_client_->share_path_called());
EXPECT_EQ(fake_seneschal_client_->share_path_called(),
expected_seneschal_client_called);
EXPECT_EQ(success, false);
EXPECT_EQ(failure_reason, "test failure");
EXPECT_EQ(failure_reason, expected_failure_reason);
std::move(closure).Run();
}
......@@ -83,16 +98,38 @@ class CrostiniSharePathTest : public testing::Test {
void SetUp() override {
run_loop_ = std::make_unique<base::RunLoop>();
profile_ = std::make_unique<TestingProfile>();
// Fake that this is a real ChromeOS system in order to use TestingProfile
// /tmp path for Downloads rather than the current Linux user $HOME.
// SetChromeOSVersionInfoForTest() must not be called until D-Bus is
// initialized with fake clients, and then it must be cleared in TearDown()
// before D-Bus is re-initialized for the next test.
base::SysInfo::SetChromeOSVersionInfoForTest(kLsbRelease, base::Time());
// Setup Downloads and path to share.
downloads_ = file_manager::util::GetDownloadsFolderForProfile(profile());
share_path_ = downloads_.AppendASCII("path");
ASSERT_TRUE(base::CreateDirectory(share_path_));
// Create 'vm-running' VM instance which is running.
vm_tools::concierge::VmInfo vm_info;
CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
"vm-running", vm_info);
}
void TearDown() override {
run_loop_.reset();
profile_.reset();
// Clear SetChromeOSVersionInfoForTest() so D-Bus will use fake clients
// again in the next test.
base::SysInfo::SetChromeOSVersionInfoForTest("", base::Time());
}
protected:
base::RunLoop* run_loop() { return run_loop_.get(); }
Profile* profile() { return profile_.get(); }
base::FilePath downloads_;
base::FilePath share_path_;
// Owned by chromeos::DBusThreadManager
chromeos::FakeSeneschalClient* fake_seneschal_client_;
......@@ -101,7 +138,6 @@ class CrostiniSharePathTest : public testing::Test {
std::unique_ptr<base::RunLoop>
run_loop_; // run_loop_ must be created on the UI thread.
std::unique_ptr<TestingProfile> profile_;
private:
base::test::ScopedTaskEnvironment scoped_task_environment_;
content::TestBrowserThreadBundle test_browser_thread_bundle_;
......@@ -115,14 +151,14 @@ TEST_F(CrostiniSharePathTest, Success) {
fake_concierge_client_->set_start_vm_response(start_vm_response);
CrostiniManager::GetForProfile(profile())->StartTerminaVm(
"vm-running-success", base::FilePath("path"),
"success", share_path_,
base::BindOnce(
&CrostiniSharePathTest::SharePathSuccessStartTerminaVmCallback,
base::Unretained(this)));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, SharePathError) {
TEST_F(CrostiniSharePathTest, SharePathErrorSeneschal) {
vm_tools::concierge::StartVmResponse start_vm_response;
start_vm_response.set_success(true);
start_vm_response.mutable_vm_info()->set_seneschal_server_handle(123);
......@@ -134,18 +170,61 @@ TEST_F(CrostiniSharePathTest, SharePathError) {
fake_seneschal_client_->set_share_path_response(share_path_response);
CrostiniManager::GetForProfile(profile())->StartTerminaVm(
"vm-running-error", base::FilePath("path"),
"error-seneschal", share_path_,
base::BindOnce(
&CrostiniSharePathTest::SharePathErrorStartTerminaVmCallback,
&CrostiniSharePathTest::SharePathErrorSeneschalStartTerminaVmCallback,
base::Unretained(this)));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, SharePathErrorPathNotAbsolute) {
const base::FilePath path("not/absolute/dir");
SharePath(profile(), "vm-running", path,
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), false,
"Path must be absolute", run_loop()->QuitClosure()));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, SharePathErrorNotUnderDownloads) {
const base::FilePath path("/not/under/downloads");
SharePath(profile(), "vm-running", path,
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), false,
"Path must be under Downloads",
run_loop()->QuitClosure()));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, SharePathErrorVmNotRunning) {
CrostiniSharePath::GetInstance()->SharePath(
profile(), "vm-not-running", "path",
base::BindOnce(&CrostiniSharePathTest::SharePathErrorVmNotRunningCallback,
base::Unretained(this), run_loop()->QuitClosure()));
SharePath(profile(), "error-vm-not-running", share_path_,
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), false, "VM not running",
run_loop()->QuitClosure()));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, SharePathErrorNotValidDirectory) {
const base::FilePath path = share_path_.AppendASCII("not-exists");
SharePath(profile(), "vm-running", path,
base::BindOnce(&CrostiniSharePathTest::SharePathErrorCallback,
base::Unretained(this), false,
"Path is not a valid directory",
run_loop()->QuitClosure()));
run_loop()->Run();
}
TEST_F(CrostiniSharePathTest, ShareAllPaths) {
base::FilePath share_path2_ = downloads_.AppendASCII("path");
ASSERT_TRUE(base::CreateDirectory(share_path2_));
vm_tools::concierge::VmInfo vm_info;
CrostiniManager::GetForProfile(profile())->AddRunningVmForTesting(
kCrostiniDefaultVmName, vm_info);
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()));
profile()->GetPrefs()->Set(prefs::kCrostiniSharedPaths, shared_paths);
ShareAllPaths(profile(), run_loop()->QuitClosure());
run_loop()->Run();
}
......
......@@ -692,20 +692,8 @@ FileManagerPrivateInternalSharePathWithCrostiniFunction::Run() {
storage::FileSystemURL cracked =
file_system_context->CrackURL(GURL(params->url));
// TODO(joelhockey): Seneschal currently only supports sharing
// directories under Downloads.
std::string downloads_mount_name =
file_manager::util::GetDownloadsMountPointName(profile);
if (cracked.filesystem_id() != downloads_mount_name) {
return RespondNow(Error(
"Share with Linux only allowed for directories within Downloads."));
}
// Path must be relative under Downloads/
std::string share_path =
cracked.virtual_path().value().substr(downloads_mount_name.size() + 1);
crostini::CrostiniSharePath::GetInstance()->SharePath(
profile, kCrostiniDefaultVmName, share_path,
crostini::SharePath(
profile, kCrostiniDefaultVmName, cracked.path(),
base::BindOnce(&FileManagerPrivateInternalSharePathWithCrostiniFunction::
SharePathCallback,
this));
......@@ -723,14 +711,13 @@ ExtensionFunction::ResponseAction
FileManagerPrivateInternalGetCrostiniSharedPathsFunction::Run() {
Profile* profile = Profile::FromBrowserContext(browser_context());
file_manager::util::FileDefinitionList file_definition_list;
auto shared_paths =
crostini::CrostiniSharePath::GetInstance()->GetSharedPaths(profile);
auto shared_paths = crostini::GetSharedPaths(profile);
for (const std::string& path : shared_paths) {
file_manager::util::FileDefinition file_definition;
// All shared paths should be directories. Even if this is not true, it
// is fine for foreground/js/crostini.js class to think so.
// We verify that the paths are in fact valid directories before calling
// seneschal/9p.
// seneschal/9p in CrostiniSharePath::CallSeneschalSharePath().
file_definition.is_directory = true;
if (file_manager::util::ConvertAbsoluteFilePathToRelativeFileSystemPath(
profile, extension_id(), base::FilePath(path),
......
......@@ -28,6 +28,7 @@
#include "content/public/browser/browser_thread.h"
#include "net/base/escape.h"
#include "net/base/filename_util.h"
#include "storage/browser/fileapi/external_mount_points.h"
#include "ui/base/l10n/l10n_util.h"
#include "url/gurl.h"
......@@ -87,6 +88,15 @@ const base::FilePath::CharType kAndroidFilesPath[] =
FILE_PATH_LITERAL("/run/arc/sdcard/write/emulated/0");
base::FilePath GetDownloadsFolderForProfile(Profile* profile) {
// Check if FilesApp has a registered path already. This happens for tests.
const std::string mount_point_name =
util::GetDownloadsMountPointName(profile);
storage::ExternalMountPoints* const mount_points =
storage::ExternalMountPoints::GetSystemInstance();
base::FilePath path;
if (mount_points->GetRegisteredPath(mount_point_name, &path))
return path;
// On non-ChromeOS system (test+development), the primary profile uses
// $HOME/Downloads for ease for accessing local files for debugging.
if (!base::SysInfo::IsRunningOnChromeOS() &&
......@@ -99,6 +109,8 @@ base::FilePath GetDownloadsFolderForProfile(Profile* profile) {
if (user == primary_user)
return DownloadPrefs::GetDefaultDownloadDirectory();
}
// Return <cryptohome>/Downloads.
return profile->GetPath().AppendASCII(kDownloadsFolderName);
}
......
......@@ -47,9 +47,7 @@ chrome.test.runTests([
function testSharePathWithCrostiniNotDownloads() {
getEntry('testing', 'test_dir').then((entry) => {
chrome.fileManagerPrivate.sharePathWithCrostini(
entry,
chrome.test.callbackFail(
'Share with Linux only allowed for directories within Downloads.'));
entry, chrome.test.callbackFail('Path must be under Downloads'));
});
},
function testGetCrostiniSharedPaths() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment