Skip to content

Commit

Permalink
Fix clipboard duplicate events when proceeding warning
Browse files Browse the repository at this point in the history 
Bug: 1373387
Test: tools/autotest.py -C out/Default ./chrome/browser/chromeos/policy/dlp/
Change-Id: I5b3d7bbf830aa50ab81427861bd0d4b7af831294
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3974615
Reviewed-by: Aya Elsayed <ayaelattar@chromium.org>
Commit-Queue: Luca Accorsi <accorsi@google.com>
Cr-Commit-Position: refs/heads/main@{#1069031}
  • Loading branch information
Luca Accorsi authored and Chromium LUCI CQ committed Nov 9, 2022
1 parent 51e7250 commit 035ff24
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 61 deletions.
143 changes: 83 additions & 60 deletions chrome/browser/chromeos/policy/dlp/data_transfer_dlp_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@ namespace policy {

namespace {

// Set |kSkipReportingTimeout| to 50 ms because:
// Set |kSkipReportingTimeout| to 75 ms because:
// - at 5 ms DataTransferDlpBlinkBrowserTest.Reporting test starts to be flaky
// - 100 ms is approximately the time a human needs to press a key.
// See DataTransferDlpController::LastReportedEndpoints struct for details.
const base::TimeDelta kSkipReportingTimeout = base::Milliseconds(50);
// TODO(crbug.com/1381095): Improve timeout tuning since duplicate warning
// proceed events delay is very variable.
const base::TimeDelta kSkipReportingTimeout = base::Milliseconds(75);

// In case the clipboard data is in warning mode, it will be allowed to
// be shared with Arc, Crostini, and Plugin VM without waiting for the
Expand All @@ -57,22 +59,6 @@ bool IsNullEndpoint(const absl::optional<ui::DataTransferEndpoint>& endpoint) {
endpoint->type() == ui::EndpointType::kDefault;
}

#if BUILDFLAG(IS_CHROMEOS_ASH)
// Map ui::EndpointType to DlpRulesManager::Component for VMs.
DlpRulesManager::Component GetComponent(ui::EndpointType endpoint_type) {
switch (endpoint_type) {
case ui::EndpointType::kCrostini:
return DlpRulesManager::Component::kCrostini;
case ui::EndpointType::kPluginVm:
return DlpRulesManager::Component::kPluginVm;
case ui::EndpointType::kArc:
return DlpRulesManager::Component::kArc;
default:
return DlpRulesManager::Component::kUnknownComponent;
}
}
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

bool IsFilesApp(const ui::DataTransferEndpoint* const data_dst) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
if (!data_dst || !data_dst->IsUrlType())
Expand Down Expand Up @@ -198,37 +184,19 @@ DlpRulesManager::Level IsDataTransferAllowed(
return level;
}

void ReportWarningProceededEvent(const ui::DataTransferEndpoint* const data_src,
const ui::DataTransferEndpoint* const data_dst,
const std::string& src_pattern,
const std::string& dst_pattern,
DlpReportingManager* reporting_manager) {
if (!reporting_manager)
return;

if (data_dst && IsVM(data_dst->type())) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
reporting_manager->ReportWarningProceededEvent(
src_pattern, GetComponent(data_dst->type()),
DlpRulesManager::Restriction::kClipboard);
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
} else {
reporting_manager->ReportWarningProceededEvent(
src_pattern, dst_pattern, DlpRulesManager::Restriction::kClipboard);
// Reports warning proceeded events and paste the copied text according to
// `should_proceed`. It is used as a callback in `PasteIfAllowed` to handle
// warning proceeded clipboard events.
void MaybeReportWarningProceededEventAndPaste(
base::OnceCallback<void(void)> reporting_cb,
base::OnceCallback<void(bool)> paste_cb,
bool should_proceed) {
if (should_proceed) {
std::move(reporting_cb).Run();
}
std::move(paste_cb).Run(should_proceed);
}

bool MaybeReportWarningProceededEvent(const ui::DataTransferEndpoint data_src,
const ui::DataTransferEndpoint data_dst,
std::string src_pattern,
std::string dst_pattern,
DlpReportingManager* reporting_manager,
bool should_proceed) {
if (should_proceed)
ReportWarningProceededEvent(&data_src, &data_dst, src_pattern, dst_pattern,
reporting_manager);
return should_proceed;
}
} // namespace

// static
Expand Down Expand Up @@ -293,13 +261,13 @@ void DataTransferDlpController::PasteIfAllowed(
const ui::DataTransferEndpoint* const data_dst,
const absl::optional<size_t> size,
content::RenderFrameHost* rfh,
base::OnceCallback<void(bool)> callback) {
base::OnceCallback<void(bool)> paste_cb) {
DCHECK(data_dst);
DCHECK(data_dst->IsUrlType());

auto* web_contents = content::WebContents::FromRenderFrameHost(rfh);
if (!web_contents) {
std::move(callback).Run(false);
std::move(paste_cb).Run(false);
return;
}

Expand All @@ -316,31 +284,41 @@ void DataTransferDlpController::PasteIfAllowed(

if (level == DlpRulesManager::Level::kAllow ||
level == DlpRulesManager::Level::kReport) {
std::move(callback).Run(true);
std::move(paste_cb).Run(true);
return;
}

DCHECK_EQ(level, DlpRulesManager::Level::kWarn);

if (ShouldPasteOnWarn(data_dst)) {
if (ShouldNotifyOnPaste(data_dst)) {
ReportWarningProceededEvent(data_src, data_dst, src_pattern, dst_pattern,
dlp_rules_manager_.GetReportingManager());
ReportWarningProceededEvent(base::OptionalFromPtr(data_src),
base::OptionalFromPtr(data_dst), src_pattern,
dst_pattern,
/*is_clipboard_event=*/true);
}
std::move(callback).Run(true);
std::move(paste_cb).Run(true);
} else if (ShouldCancelOnWarn(data_dst)) {
std::move(callback).Run(false);
std::move(paste_cb).Run(false);
} else {
if (ShouldNotifyOnPaste(data_dst)) {
auto reporting_callback = base::BindOnce(
&MaybeReportWarningProceededEvent, *data_src, *data_dst, src_pattern,
dst_pattern, dlp_rules_manager_.GetReportingManager());
ReportEvent(data_src, data_dst, src_pattern, dst_pattern,
DlpRulesManager::Level::kWarn, /*is_clipboard_event=*/true);

auto reporting_cb = base::BindOnce(
&DataTransferDlpController::ReportWarningProceededEvent,
weak_ptr_factory_.GetWeakPtr(), base::OptionalFromPtr(data_src),
base::OptionalFromPtr(data_dst), src_pattern, dst_pattern,
/*is_clipboard_event=*/true);

auto report_and_paste_cb =
base::BindOnce(&MaybeReportWarningProceededEventAndPaste,
std::move(reporting_cb), std::move(paste_cb));

WarnOnBlinkPaste(data_src, data_dst, web_contents,
std::move(reporting_callback).Then(std::move(callback)));
std::move(report_and_paste_cb));
} else {
std::move(callback).Run(true);
std::move(paste_cb).Run(true);
}
}
}
Expand Down Expand Up @@ -440,6 +418,7 @@ void DataTransferDlpController::WarnOnDrop(
bool DataTransferDlpController::ShouldSkipReporting(
const ui::DataTransferEndpoint* const data_src,
const ui::DataTransferEndpoint* const data_dst,
bool is_warning_proceeded,
base::TimeTicks curr_time) {
// Skip reporting for destination endpoints which don't notify the user
// because it's not originating from a user action.
Expand All @@ -452,11 +431,16 @@ bool DataTransferDlpController::ShouldSkipReporting(
: IsNullEndpoint(last_reported_.data_src);
bool is_same_dst = data_dst ? *data_dst == last_reported_.data_dst
: IsNullEndpoint(last_reported_.data_dst);
if (is_same_src && is_same_dst) {
bool is_same_mode =
last_reported_.is_warning_proceeded.has_value() &&
is_warning_proceeded == last_reported_.is_warning_proceeded.value();

if (is_same_src && is_same_dst && is_same_mode) {
base::TimeDelta time_diff = curr_time - last_reported_.time;
base::UmaHistogramTimes(
GetDlpHistogramPrefix() + dlp::kDataTransferReportingTimeDiffUMA,
time_diff);

return time_diff < GetSkipReportingTimeout();
}
return false;
Expand All @@ -475,13 +459,15 @@ void DataTransferDlpController::ReportEvent(

if (is_clipboard_event) {
base::TimeTicks curr_time = base::TimeTicks::Now();
if (ShouldSkipReporting(data_src, data_dst, curr_time))
if (ShouldSkipReporting(data_src, data_dst, /*is_warning_proceeded=*/false,
curr_time))
return;
last_reported_.data_src =
base::OptionalFromPtr<ui::DataTransferEndpoint>(data_src);
last_reported_.data_dst =
base::OptionalFromPtr<ui::DataTransferEndpoint>(data_dst);
last_reported_.time = curr_time;
last_reported_.is_warning_proceeded = false;
}

ui::EndpointType dst_type =
Expand Down Expand Up @@ -528,6 +514,43 @@ void DataTransferDlpController::MaybeReportEvent(
}
}

void DataTransferDlpController::ReportWarningProceededEvent(
const absl::optional<ui::DataTransferEndpoint> maybe_data_src,
const absl::optional<ui::DataTransferEndpoint> maybe_data_dst,
const std::string& src_pattern,
const std::string& dst_pattern,
bool is_clipboard_event) {
auto* reporting_manager = dlp_rules_manager_.GetReportingManager();

if (!reporting_manager)
return;

const ui::DataTransferEndpoint* data_dst =
maybe_data_dst.has_value() ? &maybe_data_dst.value() : nullptr;

if (is_clipboard_event) {
base::TimeTicks curr_time = base::TimeTicks::Now();

const ui::DataTransferEndpoint* data_src =
maybe_data_src.has_value() ? &maybe_data_src.value() : nullptr;

if (ShouldSkipReporting(data_src, data_dst, /*is_warning_proceeded=*/true,
curr_time))
return;
last_reported_.data_src = maybe_data_src;
last_reported_.data_dst = maybe_data_dst;
last_reported_.time = curr_time;
last_reported_.is_warning_proceeded = true;
}

if (data_dst && IsVM(data_dst->type())) {
NOTREACHED();
} else {
reporting_manager->ReportWarningProceededEvent(
src_pattern, dst_pattern, DlpRulesManager::Restriction::kClipboard);
}
}

DataTransferDlpController::LastReportedEndpoints::LastReportedEndpoints() =
default;

Expand Down
14 changes: 13 additions & 1 deletion chrome/browser/chromeos/policy/dlp/data_transfer_dlp_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define CHROME_BROWSER_CHROMEOS_POLICY_DLP_DATA_TRANSFER_DLP_CONTROLLER_H_

#include "base/callback.h"
#include "base/memory/weak_ptr.h"
#include "base/time/time.h"
#include "chrome/browser/chromeos/policy/dlp/dlp_clipboard_notifier.h"
#include "chrome/browser/chromeos/policy/dlp/dlp_drag_drop_notifier.h"
Expand Down Expand Up @@ -42,7 +43,7 @@ class DataTransferDlpController : public ui::DataTransferPolicyController {
const ui::DataTransferEndpoint* const data_dst,
const absl::optional<size_t> size,
content::RenderFrameHost* rfh,
base::OnceCallback<void(bool)> callback) override;
base::OnceCallback<void(bool)> paste_cb) override;
void DropIfAllowed(const ui::DataTransferEndpoint* data_src,
const ui::DataTransferEndpoint* data_dst,
base::OnceClosure drop_cb) override;
Expand Down Expand Up @@ -85,6 +86,7 @@ class DataTransferDlpController : public ui::DataTransferPolicyController {

bool ShouldSkipReporting(const ui::DataTransferEndpoint* const data_src,
const ui::DataTransferEndpoint* const data_dst,
bool is_warning_proceeded,
base::TimeTicks curr_time);

void ReportEvent(const ui::DataTransferEndpoint* const data_src,
Expand All @@ -101,6 +103,13 @@ class DataTransferDlpController : public ui::DataTransferPolicyController {
DlpRulesManager::Level level,
bool is_clipboard_event);

void ReportWarningProceededEvent(
const absl::optional<ui::DataTransferEndpoint> maybe_data_src,
const absl::optional<ui::DataTransferEndpoint> maybe_data_dst,
const std::string& src_pattern,
const std::string& dst_pattern,
bool is_clipboard_event);

// The solution for the issue of sending multiple reporting events for a
// single user action. When a user triggers a paste (for instance by pressing
// ctrl+V) clipboard API receives multiple mojo calls. For each call we check
Expand All @@ -112,12 +121,15 @@ class DataTransferDlpController : public ui::DataTransferPolicyController {
~LastReportedEndpoints();
absl::optional<ui::DataTransferEndpoint> data_src;
absl::optional<ui::DataTransferEndpoint> data_dst;
absl::optional<bool> is_warning_proceeded;
base::TimeTicks time;
} last_reported_;

const DlpRulesManager& dlp_rules_manager_;
DlpClipboardNotifier clipboard_notifier_;
DlpDragDropNotifier drag_drop_notifier_;

base::WeakPtrFactory<DataTransferDlpController> weak_ptr_factory_{this};
};

} // namespace policy
Expand Down
1 change: 1 addition & 0 deletions ui/base/data_transfer_policy/data_transfer_policy_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ void DataTransferPolicyController::DeleteInstance() {
return;

delete g_data_transfer_policy_controller_;
g_data_transfer_policy_controller_ = nullptr;
}

DataTransferPolicyController::DataTransferPolicyController() {
Expand Down

0 comments on commit 035ff24

Please sign in to comment.