Add crostini unshare path

Unshares a path previously shared, and removes path from prefs.

Bug: 878324
Change-Id: I9655c16851eed2eccca225fc3d9642db21b51dde
Reviewed-on: https://chromium-review.googlesource.com/c/1317212
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608290}

chrome/browser/chromeos/crostini/crostini_share_path.cc

@@ -59,6 +59,17 @@ void OnVmRestartedForSeneschal(
       base::BindOnce(&OnSeneschalSharePathResponse, std::move(callback)));
 }
 
+void OnSeneschalUnsharePathResponse(
+    base::OnceCallback<void(bool, std::string)> callback,
+    base::Optional<vm_tools::seneschal::UnsharePathResponse> response) {
+  if (!response) {
+    std::move(callback).Run(false, "System error");
+    return;
+  }
+  std::move(callback).Run(response.value().success(),
+                          response.value().failure_reason());
+}
+
 // Barrier Closure that captures the first instance of error.
 class ErrorCapture {
  public:
@@ -223,6 +234,48 @@ void CrostiniSharePath::CallSeneschalSharePath(
       base::BindOnce(&OnSeneschalSharePathResponse, std::move(callback)));
 }
 
+void CrostiniSharePath::CallSeneschalUnsharePath(
+    std::string vm_name,
+    const base::FilePath& path,
+    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) {
+    std::move(callback).Run(true, "VM not running");
+    return;
+  }
+
+  // Convert path to a virtual path relative to one of the external mounts,
+  // then get it as a FilesSystemURL to convert to a path inside crostini,
+  // then remove /mnt/chromeos/ base dir prefix to get the path to unshare.
+  storage::ExternalMountPoints* mount_points =
+      storage::ExternalMountPoints::GetSystemInstance();
+  base::FilePath virtual_path;
+  base::FilePath inside;
+  bool result = mount_points->GetVirtualPath(path, &virtual_path);
+  if (result) {
+    storage::FileSystemURL url = mount_points->CreateCrackedFileSystemURL(
+        GURL(), storage::kFileSystemTypeExternal, virtual_path);
+    result = file_manager::util::ConvertFileSystemURLToPathInsideCrostini(
+        profile_, url, &inside);
+  }
+  base::FilePath unshare_path;
+  if (!result || !crostini::ContainerChromeOSBaseDirectory().AppendRelativePath(
+                     inside, &unshare_path)) {
+    std::move(callback).Run(false, "Invalid path to unshare");
+    return;
+  }
+
+  vm_tools::seneschal::UnsharePathRequest request;
+  request.set_handle(vm_info->seneschal_server_handle());
+  request.set_path(unshare_path.value());
+  chromeos::DBusThreadManager::Get()->GetSeneschalClient()->UnsharePath(
+      request,
+      base::BindOnce(&OnSeneschalUnsharePathResponse, std::move(callback)));
+}
+
 void CrostiniSharePath::SharePath(
     std::string vm_name,
     const base::FilePath& path,
@@ -246,7 +299,7 @@ void CrostiniSharePath::SharePaths(
           &ErrorCapture::Run,
           base::Owned(new ErrorCapture(paths.size(), std::move(callback))));
   for (const auto& path : paths) {
-    CallSeneschalSharePath(kCrostiniDefaultVmName, path, persist,
+    CallSeneschalSharePath(vm_name, path, persist,
                            base::BindOnce(barrier, std::move(path)));
   }
 }
@@ -258,10 +311,9 @@ void CrostiniSharePath::UnsharePath(
   PrefService* pref_service = profile_->GetPrefs();
   ListPrefUpdate update(pref_service, crostini::prefs::kCrostiniSharedPaths);
   base::ListValue* shared_paths = update.Get();
-  shared_paths->Remove(base::Value(path.value()), nullptr);
-  // TODO(joelhockey): Call to seneschal once UnsharePath is supported,
-  // and update FilesApp to watch for changes.
-  std::move(callback).Run(true, "");
+  if (!shared_paths->Remove(base::Value(path.value()), nullptr))
+    LOG(WARNING) << "Unshared path not in prefs: " << path.value();
+  CallSeneschalUnsharePath(vm_name, path, std::move(callback));
   for (Observer& observer : observers_) {
     observer.OnUnshare(path);
   }

chrome/browser/chromeos/crostini/crostini_share_path_unittest.cc

@@ -65,14 +65,17 @@ class CrostiniSharePathTest : public testing::Test {
     EXPECT_EQ(fake_seneschal_client_->share_path_called(),
               expected_seneschal_client_called == SeneschalClientCalled::YES);
     if (expected_seneschal_client_called == SeneschalClientCalled::YES) {
-      EXPECT_EQ(fake_seneschal_client_->last_request().storage_location(),
+      EXPECT_EQ(
+          fake_seneschal_client_->last_share_path_request().storage_location(),
           *expected_seneschal_storage_location);
-      EXPECT_EQ(fake_seneschal_client_->last_request().shared_path().path(),
+      EXPECT_EQ(fake_seneschal_client_->last_share_path_request()
+                    .shared_path()
+                    .path(),
                 expected_seneschal_path);
     }
     EXPECT_EQ(success, expected_success == Success::YES);
     EXPECT_EQ(failure_reason, expected_failure_reason);
-    run_loop()->QuitClosure().Run();
+    run_loop()->Quit();
   }
 
   void SharePersistedPathsCallback(bool success, std::string failure_reason) {
@@ -80,7 +83,7 @@ class CrostiniSharePathTest : public testing::Test {
     EXPECT_EQ(
         profile()->GetPrefs()->GetList(prefs::kCrostiniSharedPaths)->GetSize(),
         2U);
-    run_loop()->QuitClosure().Run();
+    run_loop()->Quit();
   }
 
   void SharePathErrorVmNotRunningCallback(base::OnceClosure closure,
@@ -92,6 +95,28 @@ class CrostiniSharePathTest : public testing::Test {
     std::move(closure).Run();
   }
 
+  void UnsharePathCallback(
+      const base::FilePath& path,
+      SeneschalClientCalled expected_seneschal_client_called,
+      std::string expected_seneschal_path,
+      Success expected_success,
+      std::string expected_failure_reason,
+      bool success,
+      std::string failure_reason) {
+    const base::ListValue* prefs =
+        profile()->GetPrefs()->GetList(prefs::kCrostiniSharedPaths);
+    EXPECT_EQ(prefs->Find(base::Value(path.value())), prefs->end());
+    EXPECT_EQ(fake_seneschal_client_->unshare_path_called(),
+              expected_seneschal_client_called == SeneschalClientCalled::YES);
+    if (expected_seneschal_client_called == SeneschalClientCalled::YES) {
+      EXPECT_EQ(fake_seneschal_client_->last_unshare_path_request().path(),
+                expected_seneschal_path);
+    }
+    EXPECT_EQ(success, expected_success == Success::YES);
+    EXPECT_EQ(failure_reason, expected_failure_reason);
+    run_loop()->Quit();
+  }
+
   CrostiniSharePathTest()
       : scoped_task_environment_(
             base::test::ScopedTaskEnvironment::MainThreadType::UI),
@@ -481,4 +506,34 @@ TEST_F(CrostiniSharePathTest, RegisterPersistedPaths) {
   EXPECT_EQ(path, "/");
 }
 
+TEST_F(CrostiniSharePathTest, UnsharePathSuccess) {
+  crostini_share_path()->UnsharePath(
+      "vm-running", shared_path_,
+      base::BindOnce(&CrostiniSharePathTest::UnsharePathCallback,
+                     base::Unretained(this), shared_path_,
+                     SeneschalClientCalled::YES,
+                     "MyFiles/Downloads/already-shared", Success::YES, ""));
+  run_loop()->Run();
+}
+
+TEST_F(CrostiniSharePathTest, UnsharePathVmNotRunning) {
+  crostini_share_path()->UnsharePath(
+      "vm-not-running", shared_path_,
+      base::BindOnce(&CrostiniSharePathTest::UnsharePathCallback,
+                     base::Unretained(this), shared_path_,
+                     SeneschalClientCalled::NO, "", Success::YES,
+                     "VM not running"));
+  run_loop()->Run();
+}
+
+TEST_F(CrostiniSharePathTest, UnsharePathInvalidPath) {
+  base::FilePath invalid("invalid/path");
+  crostini_share_path()->UnsharePath(
+      "vm-running", invalid,
+      base::BindOnce(&CrostiniSharePathTest::UnsharePathCallback,
+                     base::Unretained(this), invalid, SeneschalClientCalled::NO,
+                     "", Success::NO, "Invalid path to unshare"));
+  run_loop()->Run();
+}
+
 }  // namespace crostini

chromeos/dbus/fake_seneschal_client.cc

@@ -13,6 +13,7 @@ namespace chromeos {
 FakeSeneschalClient::FakeSeneschalClient() {
   share_path_response_.set_success(true);
   share_path_response_.set_path("foo");
+  unshare_path_response_.set_success(true);
 }
 
 FakeSeneschalClient::~FakeSeneschalClient() = default;
@@ -21,9 +22,18 @@ void FakeSeneschalClient::SharePath(
     const vm_tools::seneschal::SharePathRequest& request,
     DBusMethodCallback<vm_tools::seneschal::SharePathResponse> callback) {
   share_path_called_ = true;
-  last_request_ = request;
+  last_share_path_request_ = request;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), share_path_response_));
 }
 
+void FakeSeneschalClient::UnsharePath(
+    const vm_tools::seneschal::UnsharePathRequest& request,
+    DBusMethodCallback<vm_tools::seneschal::UnsharePathResponse> callback) {
+  unshare_path_called_ = true;
+  last_unshare_path_request_ = request;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), unshare_path_response_));
+}
+
 }  // namespace chromeos

chromeos/dbus/fake_seneschal_client.h

@@ -21,16 +21,30 @@ class CHROMEOS_EXPORT FakeSeneschalClient : public SeneschalClient {
   void SharePath(const vm_tools::seneschal::SharePathRequest& request,
                  DBusMethodCallback<vm_tools::seneschal::SharePathResponse>
                      callback) override;
+  void UnsharePath(const vm_tools::seneschal::UnsharePathRequest& request,
+                   DBusMethodCallback<vm_tools::seneschal::UnsharePathResponse>
+                       callback) override;
 
   bool share_path_called() const { return share_path_called_; }
+  bool unshare_path_called() const { return unshare_path_called_; }
 
   void set_share_path_response(
       const vm_tools::seneschal::SharePathResponse& share_path_response) {
     share_path_response_ = share_path_response;
   }
 
-  const vm_tools::seneschal::SharePathRequest& last_request() const {
-    return last_request_;
+  void set_unshare_path_response(
+      const vm_tools::seneschal::UnsharePathResponse& unshare_path_response) {
+    unshare_path_response_ = unshare_path_response;
+  }
+
+  const vm_tools::seneschal::SharePathRequest& last_share_path_request() const {
+    return last_share_path_request_;
+  }
+
+  const vm_tools::seneschal::UnsharePathRequest& last_unshare_path_request()
+      const {
+    return last_unshare_path_request_;
   }
 
  protected:
@@ -40,9 +54,12 @@ class CHROMEOS_EXPORT FakeSeneschalClient : public SeneschalClient {
   void InitializeProtoResponses();
 
   bool share_path_called_ = false;
+  bool unshare_path_called_ = false;
 
-  vm_tools::seneschal::SharePathRequest last_request_;
+  vm_tools::seneschal::SharePathRequest last_share_path_request_;
+  vm_tools::seneschal::UnsharePathRequest last_unshare_path_request_;
   vm_tools::seneschal::SharePathResponse share_path_response_;
+  vm_tools::seneschal::UnsharePathResponse unshare_path_response_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSeneschalClient);
 };

chromeos/dbus/seneschal_client.cc

@@ -44,6 +44,27 @@ class SeneschalClientImpl : public SeneschalClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
+  void UnsharePath(const vm_tools::seneschal::UnsharePathRequest& request,
+                   DBusMethodCallback<vm_tools::seneschal::UnsharePathResponse>
+                       callback) override {
+    dbus::MethodCall method_call(vm_tools::seneschal::kSeneschalInterface,
+                                 vm_tools::seneschal::kUnsharePathMethod);
+    dbus::MessageWriter writer(&method_call);
+
+    if (!writer.AppendProtoAsArrayOfBytes(request)) {
+      LOG(ERROR) << "Failed to encode UnsharePath protobuf";
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::BindOnce(std::move(callback), base::nullopt));
+      return;
+    }
+
+    seneschal_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&SeneschalClientImpl::OnDBusProtoResponse<
+                           vm_tools::seneschal::UnsharePathResponse>,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
+
  protected:
   void Init(dbus::Bus* bus) override {
     seneschal_proxy_ = bus->GetObjectProxy(

chromeos/dbus/seneschal_client.h

@@ -28,6 +28,13 @@ class CHROMEOS_EXPORT SeneschalClient : public DBusClient {
       const vm_tools::seneschal::SharePathRequest& request,
       DBusMethodCallback<vm_tools::seneschal::SharePathResponse> callback) = 0;
 
+  // Unshares a path in the Chrome OS host with the container.
+  // |callback| is called after the method call finishes.
+  virtual void UnsharePath(
+      const vm_tools::seneschal::UnsharePathRequest& request,
+      DBusMethodCallback<vm_tools::seneschal::UnsharePathResponse>
+          callback) = 0;
+
  protected:
   // Create() should be used instead.
   SeneschalClient();