Skip to content

Commit

Permalink
Show the review button when some custom dialog setting is set
Browse files Browse the repository at this point in the history 
Currently the review button is only shown when more that one file is
blocked or warned. With this CL, the review button will also be shown
for single files if the admin set up custom dialog settings.

Bug: b/304457184
Change-Id: I942b56137300608c2e46a00bc9a0f448b0497637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4931308
Reviewed-by: Aya Elsayed <ayaelattar@chromium.org>
Reviewed-by: Aida Zolic <aidazolic@chromium.org>
Commit-Queue: Luca Accorsi <accorsi@google.com>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209292}
  • Loading branch information
Luca Accorsi authored and Chromium LUCI CQ committed Oct 13, 2023
1 parent b1d8eb2 commit 0ff12e5
Show file tree
Hide file tree
Showing 12 changed files with 310 additions and 88 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/ash/extensions/file_manager/event_router.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1327,6 +1327,8 @@ void EventRouter::OnIOTaskStatus(const io_task::ProgressStatus& status) {
event_status.policy_error->policy_file_count =
status.policy_error->blocked_files;
event_status.policy_error->file_name = status.policy_error->file_name;
event_status.policy_error->always_show_review =
status.policy_error->always_show_review;
}
event_status.sources_scanned = status.sources_scanned;
event_status.destination_volume_id = status.GetDestinationVolumeId();
Expand Down Expand Up @@ -1407,6 +1409,8 @@ void EventRouter::OnIOTaskStatus(const io_task::ProgressStatus& status) {
status.pause_params.policy_params->warning_files_count;
pause_params.policy_params->file_name =
status.pause_params.policy_params->file_name;
pause_params.policy_params->always_show_review =
status.pause_params.policy_params->always_show_review;
}
event_status.pause_params = std::move(pause_params);
}
Expand Down
40 changes: 30 additions & 10 deletions chrome/browser/ash/extensions/file_manager/event_router_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,11 @@ MATCHER_P3(ExpectEventArgString, index, field, expected_value, "") {
// extract the "conflictParams" and "pauseParams" keys. It expects
// "conflictParams" to be empty, and then matches the "policyParams" values
// against the expected ones.
MATCHER_P3(ExpectEventArgPauseParams,
MATCHER_P4(ExpectEventArgPauseParams,
expected_type,
expected_count,
expected_file_name,
expected_always_show_review,
"") {
EXPECT_GE(arg.size(), 1u);
const base::Value::Dict* pause_params =
Expand All @@ -221,21 +222,29 @@ MATCHER_P3(ExpectEventArgPauseParams,
policy_pause_params->FindString("fileName");
EXPECT_TRUE(actual_file_name)
<< "Could not find the string with key: fileName";
const absl::optional<bool> actual_always_show_review =
policy_pause_params->FindBool("alwaysShowReview");
EXPECT_TRUE(actual_always_show_review.has_value())
<< "Could not find the string with key: alwaysShowReview";
return testing::ExplainMatchResult(expected_type, *actual_type,
result_listener) &&
testing::ExplainMatchResult(expected_count, actual_count.value(),
result_listener) &&
testing::ExplainMatchResult(expected_file_name, *actual_file_name,
result_listener) &&
testing::ExplainMatchResult(expected_always_show_review,
actual_always_show_review.value(),
result_listener);
}

// A matcher that matches an `extensions::Event::event_args` and attempts to
// extract the "policyError" key. It then matches the "policyError" values
// against the expected ones.
MATCHER_P3(ExpectEventArgPolicyError,
MATCHER_P4(ExpectEventArgPolicyError,
expected_type,
expected_count,
expected_file_name,
expected_always_show_review,
"") {
EXPECT_GE(arg.size(), 1u);
const base::Value::Dict* policy_error =
Expand All @@ -252,11 +261,18 @@ MATCHER_P3(ExpectEventArgPolicyError,
const std::string* actual_file_name = policy_error->FindString("fileName");
EXPECT_TRUE(actual_file_name)
<< "Could not find the string with key: fileName";
const absl::optional<bool> actual_always_show_review =
policy_error->FindBool("alwaysShowReview");
EXPECT_TRUE(actual_always_show_review.has_value())
<< "Could not find the string with key: alwaysShowReview";
return testing::ExplainMatchResult(expected_type, *actual_type,
result_listener) &&
testing::ExplainMatchResult(expected_count, actual_count.value(),
result_listener) &&
testing::ExplainMatchResult(expected_file_name, *actual_file_name,
result_listener) &&
testing::ExplainMatchResult(expected_always_show_review,
actual_always_show_review.value(),
result_listener);
}

Expand Down Expand Up @@ -321,13 +337,15 @@ TEST_F(FileManagerEventRouterTest, OnIOTaskStatusForCopyPause) {
status.state = file_manager::io_task::State::kPaused;
status.sources = std::move(source_entries);
status.pause_params.policy_params = io_task::PolicyPauseParams(
policy::Policy::kDlp, /*warning_files_count*/ 2u, "foo.txt");
policy::Policy::kDlp, /*warning_files_count*/ 2u, "foo.txt",
/*always_show_review=*/false);

// Expect the event to have dlp as policy pause params.
base::RunLoop run_loop;
EXPECT_CALL(observer, OnBroadcastEvent(Field(&extensions::Event::event_args,
AllOf(ExpectEventArgPauseParams(
"dlp", 2, "foo.txt")))))
EXPECT_CALL(observer,
OnBroadcastEvent(Field(&extensions::Event::event_args,
AllOf(ExpectEventArgPauseParams(
"dlp", 2, "foo.txt", false)))))
.WillOnce(RunClosure(run_loop.QuitClosure()));

event_router->OnIOTaskStatus(status);
Expand All @@ -354,13 +372,15 @@ TEST_F(FileManagerEventRouterTest, OnIOTaskStatusForPolicyError) {
status.state = file_manager::io_task::State::kError;
status.sources = std::move(source_entries);
status.policy_error.emplace(io_task::PolicyErrorType::kDlp,
/*blocked_files=*/1, "foo.txt");
/*blocked_files=*/1, "foo.txt",
/*always_show_review=*/true);

// Expect the event to have dlp as policy error.
base::RunLoop run_loop;
EXPECT_CALL(observer, OnBroadcastEvent(Field(&extensions::Event::event_args,
AllOf(ExpectEventArgPolicyError(
"dlp", 1, "foo.txt")))))
EXPECT_CALL(observer,
OnBroadcastEvent(Field(
&extensions::Event::event_args,
AllOf(ExpectEventArgPolicyError("dlp", 1, "foo.txt", true)))))
.WillOnce(RunClosure(run_loop.QuitClosure()));

event_router->OnIOTaskStatus(status);
Expand Down
88 changes: 55 additions & 33 deletions chrome/browser/ash/file_manager/copy_or_move_io_task_policy_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,43 @@ void StartReportOnlyScanning(
DoReportOnlyScanning(nullptr, 0, std::move(settings), std::move(sources),
std::move(outputs), profile, file_system_context);
}

// Notify FilesPolicyNotificationManager of files that were blocked by
// enterprise connectors to show proper blocked dialog.
// This is not done if the new UI for enterprise connectors is disabled.
void MaybeSendConnectorsBlockedFilesNotification(
Profile* profile,
std::map<policy::FilesPolicyDialog::BlockReason,
policy::FilesPolicyDialog::Info> dialog_info_map,
IOTaskId task_id,
OperationType type) {
if (dialog_info_map.empty()) {
return;
}

// Blocked files are only added if kFileTransferEnterpriseConnectorUI is
// enabled.
CHECK(base::FeatureList::IsEnabled(
features::kFileTransferEnterpriseConnectorUI));

auto* files_policy_manager =
policy::FilesPolicyNotificationManagerFactory::GetForBrowserContext(
profile);
if (!files_policy_manager) {
LOG(ERROR) << "Couldn't find FilesPolicyNotificationManager";
return;
}

for (const auto& [block_reason, dialog_info] : dialog_info_map) {
files_policy_manager->SetConnectorsBlockedFiles(
task_id,
type == file_manager::io_task::OperationType::kMove
? policy::dlp::FileAction::kMove
: policy::dlp::FileAction::kCopy,
block_reason, std::move(dialog_info));
}
}

} // namespace

CopyOrMoveIOTaskPolicyImpl::CopyOrMoveIOTaskPolicyImpl(
Expand Down Expand Up @@ -186,37 +223,6 @@ void CopyOrMoveIOTaskPolicyImpl::Resume(ResumeParams params) {
}
}

void CopyOrMoveIOTaskPolicyImpl::MaybeSendConnectorsBlockedFilesNotification() {
if (connectors_blocked_files_.empty()) {
return;
}

// Blocked files are only added if kFileTransferEnterpriseConnectorUI is
// enabled.
CHECK(base::FeatureList::IsEnabled(
features::kFileTransferEnterpriseConnectorUI));

auto* files_policy_manager =
policy::FilesPolicyNotificationManagerFactory::GetForBrowserContext(
profile_);
if (!files_policy_manager) {
LOG(ERROR) << "Couldn't find FilesPolicyNotificationManager";
return;
}

for (const auto& [block_reason, paths] : connectors_blocked_files_) {
auto dialog_settings =
policy::GetDialogInfoForEnterpriseConnectorsBlockReason(
block_reason, paths, file_transfer_analysis_delegates_);
files_policy_manager->SetConnectorsBlockedFiles(
progress_->task_id,
progress_->type == file_manager::io_task::OperationType::kMove
? policy::dlp::FileAction::kMove
: policy::dlp::FileAction::kCopy,
block_reason, std::move(dialog_settings));
}
}

void CopyOrMoveIOTaskPolicyImpl::Complete(State state) {
bool has_dlp_errors = !dlp_blocked_files_.empty();
bool has_connector_errors = !connectors_blocked_files_.empty();
Expand All @@ -236,14 +242,30 @@ void CopyOrMoveIOTaskPolicyImpl::Complete(State state) {
.value_or(base::FilePath())
.BaseName()
.value();
bool always_show_review = false;

std::map<policy::FilesPolicyDialog::BlockReason,
policy::FilesPolicyDialog::Info>
dialog_info_map;
for (const auto& [reason, paths] : connectors_blocked_files_) {
if (paths.empty()) {
continue;
}
auto dialog_info =
policy::GetDialogInfoForEnterpriseConnectorsBlockReason(
reason, paths, file_transfer_analysis_delegates_);
always_show_review |= dialog_info.HasCustomDetails();
dialog_info_map.insert({reason, std::move(dialog_info)});
}

progress_->policy_error.emplace(
error_type,
(dlp_blocked_files_.size() + GetConnectorsBlockedFilesNum()),
blocked_file_name);
blocked_file_name, always_show_review);
state = State::kError;

MaybeSendConnectorsBlockedFilesNotification();
MaybeSendConnectorsBlockedFilesNotification(
profile_, dialog_info_map, progress_->task_id, progress_->type);
}

CopyOrMoveIOTaskImpl::Complete(state);
Expand Down
5 changes: 0 additions & 5 deletions chrome/browser/ash/file_manager/copy_or_move_io_task_policy_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ class CopyOrMoveIOTaskPolicyImpl : public CopyOrMoveIOTaskImpl {
// Returns whether the warning was shown.
bool MaybeShowConnectorsWarning();

// Notify FilesPolicyNotificationManager of files that were blocked by
// enterprise connectors to show proper blocked dialog.
// This is not done if the new UI for enterprise connectors is disabled.
void MaybeSendConnectorsBlockedFilesNotification();

// Called after the warning dialog is proceeded or cancelled.
// This resumes the transfer and allows for the warned files to be transferred
// if the warning is proceeded.
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/ash/file_manager/io_task.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ struct PolicyError {
// The name of the first file among those under block restriction. Used for
// notifications.
std::string file_name;
// Normally the review button is only shown when `blocked_files` is >1, this
// option allows to force the display of the review button irrespective of
// other conditions.
bool always_show_review = false;

bool operator==(const PolicyError& other) const;
bool operator!=(const PolicyError& other) const;
Expand Down Expand Up @@ -129,6 +133,10 @@ struct PolicyPauseParams {
// The name of the first file among those under warning restriction. Used for
// notifications.
std::string file_name;
// Normally the review button is only shown when `warning_files_count` is >1,
// this option allows to force the display of the review button irrespective
// of other conditions.
bool always_show_review = false;

bool operator==(const PolicyPauseParams& other) const;
};
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/ash/policy/dlp/dialogs/files_policy_dialog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ void FilesPolicyDialog::Info::SetLearnMoreURL(const absl::optional<GURL>& url) {
}
}

bool FilesPolicyDialog::Info::HasCustomDetails() const {
return DoesBypassRequireJustification() || HasCustomMessage() ||
is_custom_learn_more_url_;
}

FilesPolicyDialog::FilesPolicyDialog(size_t file_count,
dlp::FileAction action,
gfx::NativeWindow modal_parent)
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/ash/policy/dlp/dialogs/files_policy_dialog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ class FilesPolicyDialog : public PolicyDialogBase {
// Overrides the default learn more URL.
void SetLearnMoreURL(const absl::optional<GURL>& url);

// Returns whether at least one of the default values (e.g., message, learn
// more URL, etc...) has been overridden with a custom value.
bool HasCustomDetails() const;

private:
Info();

Expand All @@ -142,6 +146,9 @@ class FilesPolicyDialog : public PolicyDialogBase {
// Whether `message_` is a custom message.
bool is_custom_message_ = false;

// Whether `learn_more_url_` is a custom url.
bool is_custom_learn_more_url_ = false;

// Default, admin defined learn more URL, or none of them.
absl::optional<GURL> learn_more_url_;
};
Expand Down

0 comments on commit 0ff12e5

Please sign in to comment.