Fix SmbFileSystem::DeleteEntry() to respect |recursive| argument

DeleteEntry() was incorrectly ignoring the |recursive| argument and
always doing a recursive delete. This can cause server-side data loss in
certain rare situations.

BUG=chromium:1013901

Change-Id: Ia8eda8798b3cefc66765855d6e01608c235ecc09
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864498Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Sergei Datsenko <dats@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706719}

chrome/browser/chromeos/BUILD.gn

@@ -2847,6 +2847,7 @@ source_set("unit_tests") {
     "smb_client/discovery/network_scanner_unittest.cc",
     "smb_client/smb_errors_unittest.cc",
     "smb_client/smb_file_system_id_test.cc",
+    "smb_client/smb_file_system_unittest.cc",
     "smb_client/smb_service_helper_unittest.cc",
     "smb_client/smb_service_unittest.cc",
     "smb_client/smb_share_finder_unittest.cc",

chrome/browser/chromeos/smb_client/smb_file_system.cc

@@ -370,12 +370,21 @@ AbortCallback SmbFileSystem::DeleteEntry(
     bool recursive,
     storage::AsyncFileUtil::StatusCallback callback) {
   OperationId operation_id = task_queue_.GetNextOperationId();
-
-  auto reply = base::BindOnce(&SmbFileSystem::HandleGetDeleteListCallback,
-                              AsWeakPtr(), std::move(callback), operation_id);
-  SmbTask task = base::BindOnce(&SmbProviderClient::GetDeleteList,
-                                GetWeakSmbProviderClient(), GetMountId(),
-                                entry_path, std::move(reply));
+  SmbTask task;
+
+  if (recursive) {
+    auto reply = base::BindOnce(&SmbFileSystem::HandleGetDeleteListCallback,
+                                AsWeakPtr(), std::move(callback), operation_id);
+    task = base::BindOnce(&SmbProviderClient::GetDeleteList,
+                          GetWeakSmbProviderClient(), GetMountId(), entry_path,
+                          std::move(reply));
+  } else {
+    auto reply = base::BindOnce(&SmbFileSystem::HandleStatusCallback,
+                                AsWeakPtr(), std::move(callback));
+    task = base::BindOnce(&SmbProviderClient::DeleteEntry,
+                          GetWeakSmbProviderClient(), GetMountId(), entry_path,
+                          false /* recursive */, std::move(reply));
+  }
 
   EnqueueTask(std::move(task), operation_id);
   return CreateAbortCallback(operation_id);

chrome/browser/chromeos/smb_client/smb_file_system_unittest.cc

+// Copyright 2019 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_file_system.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_smb_provider_client.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Invoke;
+using ::testing::WithArg;
+
+namespace chromeos {
+namespace smb_client {
+namespace {
+
+const ProviderId kProviderId = ProviderId::CreateFromNativeId("smb");
+constexpr char kSharePath[] = "\\\\server\\foobar";
+constexpr int32_t kMountId = 4;
+constexpr char kDirectoryPath[] = "foo/bar";
+
+class MockSmbProviderClient : public chromeos::FakeSmbProviderClient {
+ public:
+  MockSmbProviderClient()
+      : FakeSmbProviderClient(true /* should_run_synchronously */) {}
+
+  MOCK_METHOD(void,
+              DeleteEntry,
+              (int32_t, const base::FilePath&, bool, StatusCallback),
+              (override));
+};
+
+class SmbFileSystemTest : public testing::Test {
+ protected:
+  SmbFileSystemTest()
+      : task_environment_(content::BrowserTaskEnvironment::REAL_IO_THREAD) {}
+
+  void SetUp() override {
+    ProvidedFileSystemInfo file_system_info(
+        kProviderId, {}, base::FilePath(kSharePath), false /* configurable */,
+        false /* watchable */, extensions::SOURCE_NETWORK,
+        chromeos::file_system_provider::IconSet());
+    file_system_ = std::make_unique<SmbFileSystem>(
+        file_system_info,
+        base::BindRepeating(
+            [](const ProvidedFileSystemInfo&) { return kMountId; }),
+        SmbFileSystem::UnmountCallback(),
+        SmbFileSystem::RequestCredentialsCallback(),
+        SmbFileSystem::RequestUpdatedSharePathCallback());
+
+    std::unique_ptr<MockSmbProviderClient> mock_client =
+        std::make_unique<MockSmbProviderClient>();
+    mock_client_ = mock_client.get();
+    // The mock needs to be marked as leaked because ownership is passed to
+    // DBusThreadManager.
+    testing::Mock::AllowLeak(mock_client.get());
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetSmbProviderClient(
+        std::move(mock_client));
+  }
+
+  void TearDown() override {
+    // Because the mock is potentially leaked, expectations needs to be manually
+    // verified.
+    EXPECT_TRUE(testing::Mock::VerifyAndClearExpectations(mock_client_));
+  }
+
+  content::BrowserTaskEnvironment task_environment_;
+  MockSmbProviderClient* mock_client_;  // Owned by DBusThreadManager.
+  std::unique_ptr<SmbFileSystem> file_system_;
+};
+
+TEST_F(SmbFileSystemTest, DeleteEntry_NonRecursive) {
+  base::FilePath dir(kDirectoryPath);
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_client_, DeleteEntry(kMountId, dir, false, _))
+      .WillOnce(WithArg<3>(Invoke([](SmbProviderClient::StatusCallback cb) {
+        std::move(cb).Run(smbprovider::ErrorType::ERROR_OK);
+      })));
+  file_system_->DeleteEntry(
+      dir, false /* recursive */,
+      base::BindLambdaForTesting([&run_loop](base::File::Error error) {
+        EXPECT_EQ(error, base::File::FILE_OK);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
+TEST_F(SmbFileSystemTest, DeleteEntry_NonRecursiveFailed) {
+  base::FilePath dir(kDirectoryPath);
+
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_client_, DeleteEntry(kMountId, dir, false, _))
+      .WillOnce(WithArg<3>(Invoke([](SmbProviderClient::StatusCallback cb) {
+        std::move(cb).Run(smbprovider::ErrorType::ERROR_NOT_EMPTY);
+      })));
+  file_system_->DeleteEntry(
+      dir, false /* recursive */,
+      base::BindLambdaForTesting([&run_loop](base::File::Error error) {
+        EXPECT_EQ(error, base::File::FILE_ERROR_NOT_EMPTY);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
+}  // namespace
+}  // namespace smb_client
+}  // namespace chromeos