Skip to content

Commit

Permalink
dlp: Improve DlpFilesControllerAsh unit test coverage
Browse files Browse the repository at this point in the history 
- Removes unnecessary code in dlp_files_controller_ash.cc
- Adds unit tests for uncovered edge cases

R=chromeos-dlp@google.com

Bug: b/304691713
Change-Id: I4b959368a6f81eebf7b8c026ab9f58fdc3ac3ce3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4941932
Reviewed-by: Daniel Brinkers <brinky@google.com>
Commit-Queue: Aida Zolic <aidazolic@chromium.org>
Auto-Submit: Aida Zolic <aidazolic@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1210718}
  • Loading branch information
Aida Zolic authored and Chromium LUCI CQ committed Oct 17, 2023
1 parent 27539ae commit 0c1d9ef
Show file tree
Hide file tree
Showing 3 changed files with 174 additions and 45 deletions.
31 changes: 13 additions & 18 deletions chrome/browser/ash/policy/dlp/dlp_files_controller_ash.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ constexpr base::TimeDelta kCooldownTimeout = base::Seconds(5);
constexpr size_t kEntriesLimit = 100;

// FileSystemContext instance set for testing.
storage::FileSystemContext* g_file_system_context_for_testing = nullptr;
absl::optional<storage::FileSystemContext*> g_file_system_context_for_testing =
absl::nullopt;

// Returns true if `file_path` is in My Files directory.
bool IsInLocalFileSystem(Profile* profile, const base::FilePath& file_path) {
Expand Down Expand Up @@ -129,11 +130,8 @@ absl::optional<DlpFileDestination> GetFileDestinationForApp(
// Returns |g_file_system_context_for_testing| if set, otherwise
// it returns FileSystemContext* for the primary profile.
storage::FileSystemContext* GetFileSystemContext(Profile* profile) {
if (g_file_system_context_for_testing) {
return g_file_system_context_for_testing;
}

return file_manager::util::GetFileManagerFileSystemContext(profile);
return g_file_system_context_for_testing.value_or(
file_manager::util::GetFileManagerFileSystemContext(profile));
}

// Gets all files inside |root| recursively and runs |callback_| with the
Expand Down Expand Up @@ -533,6 +531,12 @@ void DlpFilesControllerAsh::CheckIfDownloadAllowed(
const DlpFileDestination& download_src,
const base::FilePath& file_path,
CheckIfDlpAllowedCallback result_callback) {
if (!download_src.url().has_value()) {
// Currently we only support urls as sources.
std::move(result_callback).Run(true);
return;
}

auto dst_component =
MapFilePathToPolicyComponent(profile_, base::FilePath(file_path));
if (!dst_component.has_value()) {
Expand All @@ -542,20 +546,11 @@ void DlpFilesControllerAsh::CheckIfDownloadAllowed(
return;
}

if (!download_src.url().has_value()) {
// Currently we only support urls as sources.
std::move(result_callback).Run(true);
return;
}

DlpFileDestination dlp_destination = DlpFileDestination(*dst_component);
// TODO(b/290200170): Check whether referrer_url could be set too.
FileDaemonInfo file_info({}, {}, file_path, download_src.url()->spec(),
FileDaemonInfo file_info(/*inode=*/{}, /*crtime=*/{}, file_path,
download_src.url()->spec(),
/*referrer_url=*/"");

absl::optional<data_controls::Component> component =
MapFilePathToPolicyComponent(profile_, file_path);
DlpFileDestination dlp_destination =
component ? DlpFileDestination(*component) : DlpFileDestination();
IsFilesTransferRestricted(
absl::nullopt, {std::move(file_info)}, dlp_destination,
dlp::FileAction::kDownload,
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/ash/policy/dlp/dlp_files_controller_ash.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include "ui/base/clipboard/file_info.h"
#include "ui/base/data_transfer_policy/data_transfer_endpoint.h"
#include "ui/shell_dialogs/selected_file_info.h"
#include "url/gurl.h"

namespace storage {
class FileSystemURL;
Expand Down
187 changes: 161 additions & 26 deletions chrome/browser/ash/policy/dlp/dlp_files_controller_ash_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,37 @@ TEST_F(DlpFilesControllerAshTest, FilterDisallowedUploads_MixedFiles) {
EXPECT_FALSE(request.has_io_task_id());
}

TEST_F(DlpFilesControllerAshTest, CheckIfTransferAllowed_NoFileSystemContext) {
std::vector<FileDaemonInfo> files{
FileDaemonInfo(kInode1, kCrtime1, my_files_dir_.AppendASCII(kFilePath1),
kExampleUrl1, kReferrerUrl1),
FileDaemonInfo(kInode2, kCrtime2, my_files_dir_.AppendASCII(kFilePath2),
kExampleUrl2, kReferrerUrl2),
FileDaemonInfo(kInode3, kCrtime3, my_files_dir_.AppendASCII(kFilePath3),
kExampleUrl3, kReferrerUrl3)};
std::vector<FileSystemURL> files_urls;
AddFilesToDlpClient(std::move(files), files_urls);

std::vector<storage::FileSystemURL> transferred_files(
{files_urls[0], files_urls[1], files_urls[2]});

mount_points_->RegisterFileSystem(
ash::kSystemMountNameArchive, storage::kFileSystemTypeLocal,
storage::FileSystemMountOption(),
base::FilePath(file_manager::util::kArchiveMountPath));
auto dst_url = mount_points_->CreateExternalFileSystemURL(
blink::StorageKey(), "archive",
base::FilePath("file.rar/path/in/archive"));

base::test::TestFuture<std::vector<storage::FileSystemURL>> future;
ASSERT_TRUE(files_controller_);
files_controller_->SetFileSystemContextForTesting(nullptr);
files_controller_->CheckIfTransferAllowed(
/*task_id=*/absl::nullopt, transferred_files, dst_url, /*is_move=*/true,
future.GetCallback());
EXPECT_EQ(0u, future.Get().size());
}

TEST_F(DlpFilesControllerAshTest, FilterDisallowedUploads_ErrorResponse) {
ASSERT_TRUE(mount_points_->RegisterFileSystem(
"c", storage::kFileSystemTypeLocal, storage::FileSystemMountOption(),
Expand Down Expand Up @@ -726,6 +757,37 @@ TEST_F(DlpFilesControllerAshTest, FilterDisallowedUploads_MultiFolder) {
EXPECT_FALSE(request.has_io_task_id());
}

TEST_F(DlpFilesControllerAshTest, FilterDisallowedUploads_NoFileSystemContext) {
ASSERT_TRUE(mount_points_->RegisterFileSystem(
"c", storage::kFileSystemTypeLocal, storage::FileSystemMountOption(),
my_files_dir_));

std::vector<FileDaemonInfo> files{
FileDaemonInfo(kInode1, kCrtime1, my_files_dir_.AppendASCII(kFilePath1),
kExampleUrl1, kReferrerUrl1),
FileDaemonInfo(kInode2, kCrtime2, my_files_dir_.AppendASCII(kFilePath2),
kExampleUrl2, kReferrerUrl2),
FileDaemonInfo(kInode3, kCrtime3, my_files_dir_.AppendASCII(kFilePath3),
kExampleUrl3, kReferrerUrl3)};
std::vector<FileSystemURL> files_urls;
AddFilesToDlpClient(std::move(files), files_urls);

std::vector<ui::SelectedFileInfo> selected_files;
selected_files.emplace_back(files_urls[0].path(), files_urls[0].path());
selected_files.emplace_back(files_urls[1].path(), files_urls[1].path());
selected_files.emplace_back(files_urls[2].path(), files_urls[2].path());

base::test::TestFuture<std::vector<ui::SelectedFileInfo>> future;
ASSERT_TRUE(files_controller_);
files_controller_->SetFileSystemContextForTesting(nullptr);
files_controller_->FilterDisallowedUploads(
selected_files, DlpFileDestination(GURL(kExampleUrl1)),
future.GetCallback());

ASSERT_EQ(3u, future.Get().size());
EXPECT_EQ(selected_files, future.Take());
}

TEST_F(DlpFilesControllerAshTest, GetDlpMetadata) {
std::vector<FileDaemonInfo> files{
FileDaemonInfo(kInode1, kCrtime1, my_files_dir_.AppendASCII(kFilePath1),
Expand Down Expand Up @@ -763,7 +825,6 @@ TEST_F(DlpFilesControllerAshTest, GetDlpMetadata) {
ASSERT_TRUE(files_controller_);
files_controller_->GetDlpMetadata(files_to_check, absl::nullopt,
future.GetCallback());
EXPECT_TRUE(future.Wait());
EXPECT_EQ(dlp_metadata, future.Take());
}

Expand Down Expand Up @@ -809,7 +870,6 @@ TEST_F(DlpFilesControllerAshTest, GetDlpMetadata_WithComponent) {
files_controller_->GetDlpMetadata(
files_to_check, DlpFileDestination(data_controls::Component::kUsb),
future.GetCallback());
EXPECT_TRUE(future.Wait());
EXPECT_EQ(dlp_metadata, future.Take());
}

Expand Down Expand Up @@ -855,7 +915,6 @@ TEST_F(DlpFilesControllerAshTest, GetDlpMetadata_WithDestination) {
files_controller_->GetDlpMetadata(files_to_check,
DlpFileDestination(GURL(kExampleUrl1)),
future.GetCallback());
EXPECT_TRUE(future.Wait());
EXPECT_EQ(dlp_metadata, future.Take());
}

Expand All @@ -880,10 +939,33 @@ TEST_F(DlpFilesControllerAshTest, GetDlpMetadata_FileNotAvailable) {
ASSERT_TRUE(files_controller_);
files_controller_->GetDlpMetadata(files_to_check, absl::nullopt,
future.GetCallback());
EXPECT_TRUE(future.Wait());
EXPECT_EQ(dlp_metadata, future.Take());
}

TEST_F(DlpFilesControllerAshTest, GetDlpMetadata_ClientNotRunning) {
std::vector<FileDaemonInfo> files{
FileDaemonInfo(kInode1, kCrtime1, my_files_dir_.AppendASCII(kFilePath1),
kExampleUrl1, kReferrerUrl1),
FileDaemonInfo(kInode2, kCrtime2, my_files_dir_.AppendASCII(kFilePath2),
kExampleUrl2, kReferrerUrl2),
FileDaemonInfo(kInode3, kCrtime3, my_files_dir_.AppendASCII(kFilePath3),
kExampleUrl3, kReferrerUrl3)};
std::vector<FileSystemURL> files_urls;
AddFilesToDlpClient(std::move(files), files_urls);

std::vector<storage::FileSystemURL> files_to_check(
{files_urls[0], files_urls[1], files_urls[2]});

base::test::TestFuture<std::vector<DlpFilesControllerAsh::DlpFileMetadata>>
future;
chromeos::DlpClient::Get()->GetTestInterface()->SetIsAlive(false);
ASSERT_TRUE(files_controller_);
files_controller_->GetDlpMetadata(files_to_check, absl::nullopt,
future.GetCallback());
EXPECT_EQ(std::vector<DlpFilesControllerAsh::DlpFileMetadata>(),
future.Take());
}

TEST_F(DlpFilesControllerAshTest, GetDlpRestrictionDetails_Mixed) {
DlpRulesManager::AggregatedDestinations destinations;
destinations[DlpRulesManager::Level::kBlock].insert(kExampleUrl2);
Expand Down Expand Up @@ -983,14 +1065,37 @@ TEST_F(DlpFilesControllerAshTest, GetBlockedComponents) {
}

TEST_F(DlpFilesControllerAshTest, DownloadToLocalAllowed) {
MockCheckIfDlpAllowedCallback cb;
EXPECT_CALL(cb, Run(/*is_allowed=*/true)).Times(1);

base::test::TestFuture<bool> future;
ASSERT_TRUE(files_controller_);
files_controller_->CheckIfDownloadAllowed(
DlpFileDestination(GURL(kExampleUrl1)),
base::FilePath(
"/home/chronos/u-0123456789abcdef/MyFiles/Downloads/img.jpg"),
cb.Get());
future.GetCallback());
EXPECT_TRUE(future.Take());
}

TEST_F(DlpFilesControllerAshTest, DownloadFromComponentAllowed) {
base::test::TestFuture<bool> future;
ASSERT_TRUE(files_controller_);
files_controller_->CheckIfDownloadAllowed(
DlpFileDestination(data_controls::Component::kArc),
base::FilePath("path/in/android/filename"), future.GetCallback());
EXPECT_TRUE(future.Take());
}

TEST_F(DlpFilesControllerAshTest, FilePromptForDownloadLocal) {
ASSERT_TRUE(files_controller_);
EXPECT_FALSE(files_controller_->ShouldPromptBeforeDownload(
DlpFileDestination(GURL(kExampleUrl1)),
base::FilePath(
"/home/chronos/u-0123456789abcdef/MyFiles/Downloads/img.jpg")));
}

TEST_F(DlpFilesControllerAshTest, FilePromptForDownloadNoSource) {
ASSERT_TRUE(files_controller_);
EXPECT_FALSE(files_controller_->ShouldPromptBeforeDownload(
DlpFileDestination(), base::FilePath("path/in/android/filename")));
}

TEST_F(DlpFilesControllerAshTest, CheckReportingOnIsDlpPolicyMatched) {
Expand Down Expand Up @@ -1316,7 +1421,7 @@ TEST_F(DlpFilesControllerAshTest, CheckIfDropAllowed_ErrorResponse) {
files_controller_->CheckIfDropAllowed(std::move(dropped_files), &data_dst,
future.GetCallback());

ASSERT_EQ(true, future.Get());
ASSERT_TRUE(future.Take());

// Validate the request sent to the daemon.
::dlp::CheckFilesTransferRequest request =
Expand Down Expand Up @@ -1369,8 +1474,7 @@ TEST_F(DlpFilesControllerAshTest, CheckIfDropAllowed) {
ASSERT_TRUE(files_controller_);
files_controller_->CheckIfDropAllowed(dropped_files, &data_dst,
future.GetCallback());
EXPECT_TRUE(future.Wait());
EXPECT_EQ(false, future.Take());
EXPECT_FALSE(future.Take());

// Validate that only the local file was sent to the daemon.
::dlp::CheckFilesTransferRequest request =
Expand All @@ -1384,6 +1488,26 @@ TEST_F(DlpFilesControllerAshTest, CheckIfDropAllowed) {
EXPECT_FALSE(request.has_io_task_id());
}

TEST_F(DlpFilesControllerAshTest, CheckIfDropAllowed_NoFileSystemContext) {
ASSERT_TRUE(mount_points_->RegisterFileSystem(
"c", storage::kFileSystemTypeLocal, storage::FileSystemMountOption(),
my_files_dir_));

base::FilePath file_path1 = my_files_dir_.AppendASCII(kFilePath1);
ASSERT_TRUE(CreateDummyFile(file_path1));
auto file_url1 = CreateFileSystemURL(kTestStorageKey, file_path1.value());

std::vector<ui::FileInfo> dropped_files{ui::FileInfo(file_path1, file_path1)};
const ui::DataTransferEndpoint data_dst((GURL(kExampleUrl1)));

base::test::TestFuture<bool> future;
ASSERT_TRUE(files_controller_);
files_controller_->SetFileSystemContextForTesting(nullptr);
files_controller_->CheckIfDropAllowed(std::move(dropped_files), &data_dst,
future.GetCallback());
ASSERT_TRUE(future.Take());
}

TEST_F(DlpFilesControllerAshTest, IsFilesTransferRestricted_MyFiles) {
const auto histogram_tester = base::HistogramTester();

Expand Down Expand Up @@ -2188,20 +2312,6 @@ TEST_P(DlpFilesAppLaunchTest_ExtensionApp, CheckIfAppLaunchAllowed) {
testing::UnorderedElementsAreArray(expected_requested_files));
}

class DlpFilesAppLaunchTest_WebApp
: public DlpFilesAppLaunchTest,
public ::testing::WithParamInterface<
std::tuple<apps::AppType, std::string, std::string>> {
protected:
void SetUp() override {
DlpFilesAppLaunchTest::SetUp();

CreateAndStoreFakeApp(kWebAppId, apps::AppType::kWeb, kExampleUrl1);
CreateAndStoreFakeApp(kSystemWebAppId, apps::AppType::kSystemWeb,
kExampleUrl2);
}
};

TEST_P(DlpFilesAppLaunchTest_ExtensionApp, IsLaunchBlocked) {
auto [app_type, app_id] = GetParam();

Expand All @@ -2218,6 +2328,20 @@ TEST_P(DlpFilesAppLaunchTest_ExtensionApp, IsLaunchBlocked) {
}));
}

class DlpFilesAppLaunchTest_WebApp
: public DlpFilesAppLaunchTest,
public ::testing::WithParamInterface<
std::tuple<apps::AppType, std::string, std::string>> {
protected:
void SetUp() override {
DlpFilesAppLaunchTest::SetUp();

CreateAndStoreFakeApp(kWebAppId, apps::AppType::kWeb, kExampleUrl1);
CreateAndStoreFakeApp(kSystemWebAppId, apps::AppType::kSystemWeb,
kExampleUrl2);
}
};

INSTANTIATE_TEST_SUITE_P(
DlpFiles,
DlpFilesAppLaunchTest_WebApp,
Expand Down Expand Up @@ -2490,7 +2614,18 @@ INSTANTIATE_TEST_SUITE_P(
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kCrostini),
::dlp::DlpComponent::CROSTINI),
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kPluginVm),
::dlp::DlpComponent::PLUGIN_VM)));
::dlp::DlpComponent::PLUGIN_VM),
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kLacros),
::dlp::DlpComponent::UNKNOWN_COMPONENT),
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kDefault),
::dlp::DlpComponent::UNKNOWN_COMPONENT),
std::make_tuple(
ui::DataTransferEndpoint(ui::EndpointType::kClipboardHistory),
::dlp::DlpComponent::UNKNOWN_COMPONENT),
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kBorealis),
::dlp::DlpComponent::UNKNOWN_COMPONENT),
std::make_tuple(ui::DataTransferEndpoint(ui::EndpointType::kUnknownVm),
::dlp::DlpComponent::UNKNOWN_COMPONENT)));

// Tests dropping a mix of an external file and a local directory.
TEST_P(DlpFilesDnDTest, CheckIfDropAllowed) {
Expand Down

0 comments on commit 0c1d9ef

Please sign in to comment.