Skip to content

GitLab

T
tangled
Commit bd67b8ef authored by stefanocs's avatar stefanocs Committed by Commit bot
Browse files
Options

Add SourceUI field to permission report 

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum and code enum.

BUG=613883

Review-Url: https://codereview.chromium.org/2075523002
Cr-Commit-Position: refs/heads/master@{#406769}
parent d54345e6
Hide whitespace changes
Inline Side-by-side
Showing with 89 additions and 35 deletions
chrome/browser/permissions/permission_uma_util.cc
View file @ bd67b8ef
......@@ -197,28 +197,33 @@ void PermissionUmaUtil::PermissionRequested(PermissionType permission,
void PermissionUmaUtil::PermissionGranted(PermissionType permission,
const GURL& requesting_origin,
Profile* profile) {
RecordPermissionAction(permission, GRANTED, requesting_origin, profile);
RecordPermissionAction(permission, GRANTED, PermissionSourceUI::PROMPT,
requesting_origin, profile);
}
void PermissionUmaUtil::PermissionDenied(PermissionType permission,
const GURL& requesting_origin,
Profile* profile) {
RecordPermissionAction(permission, DENIED, requesting_origin, profile);
RecordPermissionAction(permission, DENIED, PermissionSourceUI::PROMPT,
requesting_origin, profile);
}
void PermissionUmaUtil::PermissionDismissed(PermissionType permission,
const GURL& requesting_origin,
Profile* profile) {
RecordPermissionAction(permission, DISMISSED, requesting_origin, profile);
RecordPermissionAction(permission, DISMISSED, PermissionSourceUI::PROMPT,
requesting_origin, profile);
}
void PermissionUmaUtil::PermissionIgnored(PermissionType permission,
const GURL& requesting_origin,
Profile* profile) {
RecordPermissionAction(permission, IGNORED, requesting_origin, profile);
RecordPermissionAction(permission, IGNORED, PermissionSourceUI::PROMPT,
requesting_origin, profile);
}
void PermissionUmaUtil::PermissionRevoked(PermissionType permission,
PermissionSourceUI source_ui,
const GURL& revoked_origin,
Profile* profile) {
// TODO(tsergeant): Expand metrics definitions for revocation to include all
......@@ -227,7 +232,8 @@ void PermissionUmaUtil::PermissionRevoked(PermissionType permission,
permission == PermissionType::GEOLOCATION ||
permission == PermissionType::AUDIO_CAPTURE ||
permission == PermissionType::VIDEO_CAPTURE) {
RecordPermissionAction(permission, REVOKED, revoked_origin, profile);
RecordPermissionAction(permission, REVOKED, source_ui, revoked_origin,
profile);
}
}
......@@ -330,6 +336,7 @@ bool PermissionUmaUtil::IsOptedIntoPermissionActionReporting(Profile* profile) {
void PermissionUmaUtil::RecordPermissionAction(PermissionType permission,
PermissionAction action,
PermissionSourceUI source_ui,
const GURL& requesting_origin,
Profile* profile) {
if (IsOptedIntoPermissionActionReporting(profile)) {
......@@ -337,7 +344,8 @@ void PermissionUmaUtil::RecordPermissionAction(PermissionType permission,
// sent.
g_browser_process->safe_browsing_service()
->ui_manager()
->ReportPermissionAction(requesting_origin, permission, action);
->ReportPermissionAction(requesting_origin, permission, action,
source_ui);
}
bool secure_origin = content::IsOriginSecure(requesting_origin);
......
chrome/browser/permissions/permission_uma_util.h
View file @ bd67b8ef
......@@ -35,6 +35,18 @@ enum PermissionAction {
PERMISSION_ACTION_NUM,
};
// This should stay in sync with the SourceUI enum in the permission report
// protobuf (src/chrome/common/safe_browsing/permission_report.proto).
enum class PermissionSourceUI {
PROMPT = 0,
OIB = 1,
SITE_SETTINGS = 2,
PAGE_ACTION = 3,
// Always keep this at the end.
SOURCE_UI_NUM,
};
// Provides a convenient way of logging UMA for permission related operations.
class PermissionUmaUtil {
public:
......@@ -63,6 +75,7 @@ class PermissionUmaUtil {
const GURL& requesting_origin,
Profile* profile);
static void PermissionRevoked(content::PermissionType permission,
PermissionSourceUI source_ui,
const GURL& revoked_origin,
Profile* profile);
......@@ -97,6 +110,7 @@ class PermissionUmaUtil {
static void RecordPermissionAction(content::PermissionType permission,
PermissionAction action,
PermissionSourceUI source_ui,
const GURL& requesting_origin,
Profile* profile);
......
chrome/browser/permissions/permission_util.cc
View file @ bd67b8ef
......@@ -100,8 +100,11 @@ void PermissionUtil::SetContentSettingAndRecordRevocation(
final_value != CONTENT_SETTING_ALLOW) {
PermissionType permission_type;
if (PermissionUtil::GetPermissionType(content_type, &permission_type)) {
PermissionUmaUtil::PermissionRevoked(permission_type, primary_url,
profile);
// TODO(stefanocs): Report revocations from page action as PAGE_ACTION
// source UI instead of SITE_SETTINGS source UI.
PermissionUmaUtil::PermissionRevoked(permission_type,
PermissionSourceUI::SITE_SETTINGS,
primary_url, profile);
}
}
}
chrome/browser/safe_browsing/permission_reporter.cc
View file @ bd67b8ef
......@@ -80,6 +80,24 @@ PermissionReport::Action PermissionActionForReport(PermissionAction action) {
return PermissionReport::ACTION_UNSPECIFIED;
}
PermissionReport::SourceUI SourceUIForReport(PermissionSourceUI source_ui) {
switch (source_ui) {
case PermissionSourceUI::PROMPT:
return PermissionReport::PROMPT;
case PermissionSourceUI::OIB:
return PermissionReport::OIB;
case PermissionSourceUI::SITE_SETTINGS:
return PermissionReport::SITE_SETTINGS;
case PermissionSourceUI::PAGE_ACTION:
return PermissionReport::PAGE_ACTION;
case PermissionSourceUI::SOURCE_UI_NUM:
break;
}
NOTREACHED();
return PermissionReport::SOURCE_UI_UNSPECIFIED;
}
} // namespace
bool PermissionAndOrigin::operator==(const PermissionAndOrigin& other) const {
......@@ -112,11 +130,12 @@ PermissionReporter::~PermissionReporter() {}
void PermissionReporter::SendReport(const GURL& origin,
content::PermissionType permission,
PermissionAction action) {
PermissionAction action,
PermissionSourceUI source_ui) {
if (IsReportThresholdExceeded(permission, origin))
return;
std::string serialized_report;
BuildReport(origin, permission, action, &serialized_report);
BuildReport(origin, permission, action, source_ui, &serialized_report);
permission_report_sender_->Send(GURL(kPermissionActionReportingUploadUrl),
serialized_report);
}
......@@ -125,11 +144,13 @@ void PermissionReporter::SendReport(const GURL& origin,
bool PermissionReporter::BuildReport(const GURL& origin,
PermissionType permission,
PermissionAction action,
PermissionSourceUI source_ui,
std::string* output) {
PermissionReport report;
report.set_origin(origin.spec());
report.set_permission(PermissionTypeForReport(permission));
report.set_action(PermissionActionForReport(action));
report.set_source_ui(SourceUIForReport(source_ui));
// Collect platform data.
#if defined(OS_ANDROID)
......
chrome/browser/safe_browsing/permission_reporter.h
View file @ bd67b8ef
......@@ -55,7 +55,8 @@ class PermissionReporter {
// //src/chrome/common/safe_browsing/permission_report.proto
void SendReport(const GURL& origin,
content::PermissionType permission,
PermissionAction action);
PermissionAction action,
PermissionSourceUI source_ui);
private:
friend class PermissionReporterTest;
......@@ -73,6 +74,7 @@ class PermissionReporter {
static bool BuildReport(const GURL& origin,
content::PermissionType permission,
PermissionAction action,
PermissionSourceUI source_ui,
std::string* output);
// Returns false if the number of reports sent in the last one minute per
......
chrome/browser/safe_browsing/permission_reporter_unittest.cc
View file @ bd67b8ef
......@@ -33,6 +33,7 @@ const char kDummyOriginTwo[] = "http://example2.test/";
const PermissionType kDummyPermissionOne = PermissionType::GEOLOCATION;
const PermissionType kDummyPermissionTwo = PermissionType::NOTIFICATIONS;
const PermissionAction kDummyAction = GRANTED;
const PermissionSourceUI kDummySourceUI = PermissionSourceUI::PROMPT;
const char kDummyTrialOne[] = "trial one";
const char kDummyGroupOne[] = "group one";
......@@ -106,13 +107,14 @@ class PermissionReporterTest : public ::testing::Test {
// SafeBrowsing CSD servers.
TEST_F(PermissionReporterTest, SendReport) {
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
PermissionReport permission_report;
ASSERT_TRUE(
permission_report.ParseFromString(mock_report_sender_->latest_report()));
EXPECT_EQ(PermissionReport::GEOLOCATION, permission_report.permission());
EXPECT_EQ(PermissionReport::GRANTED, permission_report.action());
EXPECT_EQ(PermissionReport::PROMPT, permission_report.source_ui());
EXPECT_EQ(kDummyOriginOne, permission_report.origin());
#if defined(OS_ANDROID)
EXPECT_EQ(PermissionReport::ANDROID_PLATFORM,
......@@ -159,7 +161,7 @@ TEST_F(PermissionReporterTest, SendReportWithFieldTrials) {
EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_two->trial_name()));
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
PermissionReport permission_report;
ASSERT_TRUE(
......@@ -189,29 +191,29 @@ TEST_F(PermissionReporterTest, IsReportThresholdExceeded) {
int reports_to_send = kMaximumReportsPerOriginPerPermissionPerMinute;
while (reports_to_send--)
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(5, mock_report_sender_->GetAndResetNumberOfReportsSent());
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(0, mock_report_sender_->GetAndResetNumberOfReportsSent());
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionTwo,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(1, mock_report_sender_->GetAndResetNumberOfReportsSent());
permission_reporter_->SendReport(GURL(kDummyOriginTwo), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(1, mock_report_sender_->GetAndResetNumberOfReportsSent());
clock_->Advance(base::TimeDelta::FromMinutes(1));
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(0, mock_report_sender_->GetAndResetNumberOfReportsSent());
clock_->Advance(base::TimeDelta::FromMicroseconds(1));
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
EXPECT_EQ(1, mock_report_sender_->GetAndResetNumberOfReportsSent());
clock_->Advance(base::TimeDelta::FromMinutes(1));
......@@ -219,7 +221,7 @@ TEST_F(PermissionReporterTest, IsReportThresholdExceeded) {
while (reports_to_send--) {
clock_->Advance(base::TimeDelta::FromSeconds(5));
permission_reporter_->SendReport(GURL(kDummyOriginOne), kDummyPermissionOne,
kDummyAction);
kDummyAction, kDummySourceUI);
}
EXPECT_EQ(kMaximumReportsPerOriginPerPermissionPerMinute,
mock_report_sender_->GetAndResetNumberOfReportsSent());
......
chrome/browser/safe_browsing/ping_manager.cc
View file @ bd67b8ef
......@@ -138,8 +138,9 @@ void SafeBrowsingPingManager::SetCertificateErrorReporterForTesting(
void SafeBrowsingPingManager::ReportPermissionAction(
const GURL& origin,
content::PermissionType permission,
PermissionAction action) {
permission_reporter_->SendReport(origin, permission, action);
PermissionAction action,
PermissionSourceUI source_ui) {
permission_reporter_->SendReport(origin, permission, action, source_ui);
}
GURL SafeBrowsingPingManager::SafeBrowsingHitUrl(
......
chrome/browser/safe_browsing/ping_manager.h
View file @ bd67b8ef
......@@ -67,7 +67,8 @@ class SafeBrowsingPingManager : public net::URLFetcherDelegate {
// Report permission action to SafeBrowsing servers.
void ReportPermissionAction(const GURL& origin,
content::PermissionType permission,
PermissionAction action);
PermissionAction action,
PermissionSourceUI source_ui);
private:
FRIEND_TEST_ALL_PREFIXES(SafeBrowsingPingManagerTest,
......
chrome/browser/safe_browsing/ui_manager.cc
View file @ bd67b8ef
......@@ -283,12 +283,13 @@ void SafeBrowsingUIManager::ReportInvalidCertificateChain(
void SafeBrowsingUIManager::ReportPermissionAction(
const GURL& origin,
content::PermissionType permission,
PermissionAction action) {
PermissionAction action,
PermissionSourceUI source_ui) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&SafeBrowsingUIManager::ReportPermissionActionOnIOThread, this,
origin, permission, action));
origin, permission, action, source_ui));
}
void SafeBrowsingUIManager::AddObserver(Observer* observer) {
......@@ -316,7 +317,8 @@ void SafeBrowsingUIManager::ReportInvalidCertificateChainOnIOThread(
void SafeBrowsingUIManager::ReportPermissionActionOnIOThread(
const GURL& origin,
content::PermissionType permission,
PermissionAction action) {
PermissionAction action,
PermissionSourceUI source_ui) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// The service may delete the ping manager (i.e. when user disabling service,
......@@ -325,7 +327,7 @@ void SafeBrowsingUIManager::ReportPermissionActionOnIOThread(
return;
sb_service_->ping_manager()->ReportPermissionAction(origin, permission,
action);
action, source_ui);
}
// If the user had opted-in to send ThreatDetails, this gets called
......
chrome/browser/safe_browsing/ui_manager.h
View file @ bd67b8ef
......@@ -154,7 +154,8 @@ class SafeBrowsingUIManager
// thread.
void ReportPermissionAction(const GURL& origin,
content::PermissionType permission,
PermissionAction action);
PermissionAction action,
PermissionSourceUI source_ui);
// Add and remove observers. These methods must be invoked on the UI thread.
void AddObserver(Observer* observer);
......@@ -179,7 +180,8 @@ class SafeBrowsingUIManager
// Report permission action to SafeBrowsing servers.
void ReportPermissionActionOnIOThread(const GURL& origin,
content::PermissionType permission,
PermissionAction action);
PermissionAction action,
PermissionSourceUI source_ui);
// Updates the whitelist state. Called on the UI thread.
void AddToWhitelist(const UnsafeResource& resource);
......
chrome/browser/ui/website_settings/website_settings.cc
View file @ bd67b8ef
......@@ -293,8 +293,9 @@ void WebsiteSettings::OnSitePermissionChanged(ContentSettingsType type,
// in the permissions layer. See crbug.com/469221.
content::PermissionType permission_type;
if (PermissionUtil::GetPermissionType(type, &permission_type)) {
PermissionUmaUtil::PermissionRevoked(permission_type, this->site_url_,
this->profile_);
PermissionUmaUtil::PermissionRevoked(permission_type,
PermissionSourceUI::OIB,
this->site_url_, this->profile_);
}
}
......
chrome/common/safe_browsing/permission_report.proto
View file @ bd67b8ef
......@@ -53,8 +53,6 @@ message PermissionReport {
// User Permission Actions. This enum is intentionally different with
// the one in src/chrome/browser/permissions/permission_uma_util.h
enum Action {
// TODO(stefanocs): Add ACTION_UNSPECIFIED to the corresponding Safe
// Browsing logs proto.
ACTION_UNSPECIFIED = 0;
GRANTED = 1;
DENIED = 2;
......@@ -68,9 +66,8 @@ message PermissionReport {
SOURCE_UI_UNSPECIFIED = 0;
PROMPT = 1;
OIB = 2;
// TODO(stefanocs): Remove CONTENT_SETTINGS from the corresponding Safe
// Browsing logs proto.
SITE_SETTINGS = 3;
PAGE_ACTION = 4;
}
// The various types of permissions. This should stay in sync with the
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment