[CSC] Avoid automatic embargo
At the moment, Captured Surface Control APIs permissions only persist
for the duration of the capture, so embargo must likewise not persist
across capture sessions.
Bug: 1521259, 1466247
Change-Id: I9ba01cb762014a0c8b0488f5c31e97deb1f3d6c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5233494
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1251748}
diff --git a/chrome/browser/display_capture/captured_surface_control_permission_context.cc b/chrome/browser/display_capture/captured_surface_control_permission_context.cc
index 2184dcc..8e29a52 100644
--- a/chrome/browser/display_capture/captured_surface_control_permission_context.cc
+++ b/chrome/browser/display_capture/captured_surface_control_permission_context.cc
@@ -17,6 +17,10 @@
ContentSettingsType::CAPTURED_SURFACE_CONTROL,
blink::mojom::PermissionsPolicyFeature::kCapturedSurfaceControl) {}
+bool CapturedSurfaceControlPermissionContext::UsesAutomaticEmbargo() const {
+ return false;
+}
+
void CapturedSurfaceControlPermissionContext::UpdateContentSetting(
const GURL& requesting_origin,
const GURL& embedding_origin,
diff --git a/chrome/browser/display_capture/captured_surface_control_permission_context.h b/chrome/browser/display_capture/captured_surface_control_permission_context.h
index 36a0a87..9409e94 100644
--- a/chrome/browser/display_capture/captured_surface_control_permission_context.h
+++ b/chrome/browser/display_capture/captured_surface_control_permission_context.h
@@ -21,6 +21,8 @@
CapturedSurfaceControlPermissionContext& operator=(
const CapturedSurfaceControlPermissionContext&) = delete;
+ bool UsesAutomaticEmbargo() const override;
+
protected:
void UpdateContentSetting(const GURL& requesting_origin,
const GURL& embedding_origin,
diff --git a/components/permissions/permission_context_base.cc b/components/permissions/permission_context_base.cc
index 7f94718..62fb026 100644
--- a/components/permissions/permission_context_base.cc
+++ b/components/permissions/permission_context_base.cc
@@ -258,7 +258,11 @@
base::OnceClosure delete_callback) const {
return std::make_unique<PermissionRequest>(
std::move(request_data), std::move(permission_decided_callback),
- std::move(delete_callback));
+ std::move(delete_callback), UsesAutomaticEmbargo());
+}
+
+bool PermissionContextBase::UsesAutomaticEmbargo() const {
+ return true;
}
content::PermissionResult PermissionContextBase::GetPermissionStatus(
@@ -341,13 +345,15 @@
content::PermissionStatusSource::UNSPECIFIED);
}
- absl::optional<content::PermissionResult> result =
- PermissionsClient::Get()
- ->GetPermissionDecisionAutoBlocker(browser_context_)
- ->GetEmbargoResult(requesting_origin, content_settings_type_);
- if (result) {
- DCHECK(result->status == PermissionStatus::DENIED);
- return result.value();
+ if (UsesAutomaticEmbargo()) {
+ absl::optional<content::PermissionResult> result =
+ PermissionsClient::Get()
+ ->GetPermissionDecisionAutoBlocker(browser_context_)
+ ->GetEmbargoResult(requesting_origin, content_settings_type_);
+ if (result) {
+ DCHECK(result->status == PermissionStatus::DENIED);
+ return result.value();
+ }
}
return content::PermissionResult(
PermissionStatus::ASK, content::PermissionStatusSource::UNSPECIFIED);
diff --git a/components/permissions/permission_context_base.h b/components/permissions/permission_context_base.h
index 1af620c7..b667b6d 100644
--- a/components/permissions/permission_context_base.h
+++ b/components/permissions/permission_context_base.h
@@ -204,6 +204,9 @@
PermissionRequest::PermissionDecidedCallback permission_decided_callback,
base::OnceClosure delete_callback) const;
+ // Implementors can override this method to avoid using automatic embargo.
+ virtual bool UsesAutomaticEmbargo() const;
+
base::ObserverList<permissions::Observer> permission_observers_;
// Set by subclasses to inform the base class that they will handle adding
diff --git a/components/permissions/permission_context_base_unittest.cc b/components/permissions/permission_context_base_unittest.cc
index 45cce03..5588117 100644
--- a/components/permissions/permission_context_base_unittest.cc
+++ b/components/permissions/permission_context_base_unittest.cc
@@ -122,6 +122,8 @@
respond_permission_ = std::move(callback);
}
+ void SetUsesAutomaticEmbargo(bool value) { uses_automatic_embargo_ = value; }
+
protected:
void UpdateTabContext(const PermissionRequestID& id,
const GURL& requesting_origin,
@@ -131,6 +133,8 @@
bool IsRestrictedToSecureOrigins() const override { return false; }
+ bool UsesAutomaticEmbargo() const override { return uses_automatic_embargo_; }
+
private:
std::vector<ContentSetting> decisions_;
bool tab_context_updated_;
@@ -138,6 +142,7 @@
// Callback for responding to a permission once the request has been completed
// (valid URL, kill switch disabled)
base::OnceClosure respond_permission_;
+ bool uses_automatic_embargo_ = true;
};
class TestKillSwitchPermissionContext : public TestPermissionContext {
@@ -550,6 +555,82 @@
result.source);
}
+ void TestVariationBlockOnSeveralDismissalsAutomaticEmbargoOff_TestContent() {
+ GURL url("https://www.google.com");
+ SetUpUrl(url);
+ base::HistogramTester histograms;
+
+ std::map<std::string, std::string> params;
+ params
+ [PermissionDecisionAutoBlocker::GetPromptDismissCountKeyForTesting()] =
+ "5";
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitAndEnableFeatureWithParameters(
+ features::kBlockPromptsIfDismissedOften, params);
+
+ std::map<std::string, std::string> actual_params;
+ EXPECT_TRUE(base::GetFieldTrialParamsByFeature(
+ features::kBlockPromptsIfDismissedOften, &actual_params));
+ EXPECT_EQ(params, actual_params);
+
+ EXPECT_TRUE(base::GetFieldTrialParamsByFeature(
+ features::kBlockPromptsIfDismissedOften, &actual_params));
+ EXPECT_EQ(params, actual_params);
+
+ for (uint32_t i = 0; i < 5; ++i) {
+ TestPermissionContext permission_context(browser_context(),
+ ContentSettingsType::MIDI_SYSEX);
+ permission_context.SetUsesAutomaticEmbargo(false);
+
+ const PermissionRequestID id(
+ web_contents()->GetPrimaryMainFrame()->GetGlobalId(),
+ PermissionRequestID::RequestLocalId(i + 1));
+ permission_context.SetRespondPermissionCallback(
+ base::BindOnce(&PermissionContextBaseTests::RespondToPermission,
+ base::Unretained(this), &permission_context, id, url,
+ CONTENT_SETTING_ASK));
+ permission_context.RequestPermission(
+ PermissionRequestData(&permission_context, id,
+ /*user_gesture=*/true, url),
+ base::BindOnce(&TestPermissionContext::TrackPermissionDecision,
+ base::Unretained(&permission_context)));
+
+ EXPECT_EQ(1u, permission_context.decisions().size());
+ ASSERT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);
+ EXPECT_TRUE(permission_context.tab_context_updated());
+ content::PermissionResult result = permission_context.GetPermissionStatus(
+ nullptr /* render_frame_host */, url, url);
+
+ histograms.ExpectTotalCount(
+ "Permissions.Prompt.Dismissed.PriorDismissCount2.MidiSysEx", i + 1);
+ histograms.ExpectBucketCount(
+ "Permissions.Prompt.Dismissed.PriorDismissCount2.MidiSysEx", 0,
+ i + 1);
+
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoPromptSuppression",
+ static_cast<int>(PermissionEmbargoStatus::NOT_EMBARGOED), i + 1);
+ histograms.ExpectTotalCount("Permissions.AutoBlocker.EmbargoStatus",
+ i + 1);
+
+ EXPECT_EQ(PermissionStatus::ASK, result.status);
+ EXPECT_EQ(content::PermissionStatusSource::UNSPECIFIED, result.source);
+ histograms.ExpectUniqueSample(
+ "Permissions.AutoBlocker.EmbargoStatus",
+ static_cast<int>(PermissionEmbargoStatus::NOT_EMBARGOED), i + 1);
+ }
+
+ // Ensure that we DO NOT finish in the block state (unlike when automatic
+ // embargo is enabled).
+ TestPermissionContext permission_context(browser_context(),
+ ContentSettingsType::MIDI_SYSEX);
+ permission_context.SetUsesAutomaticEmbargo(false);
+ content::PermissionResult result = permission_context.GetPermissionStatus(
+ nullptr /* render_frame_host */, url, url);
+ EXPECT_EQ(PermissionStatus::ASK, result.status);
+ EXPECT_EQ(content::PermissionStatusSource::UNSPECIFIED, result.source);
+ }
+
void TestRequestPermissionInvalidUrl(
ContentSettingsType content_settings_type) {
base::HistogramTester histograms;
@@ -778,6 +859,11 @@
TestVariationBlockOnSeveralDismissals_TestContent();
}
+// Test that contexts that disable the automatic blocker are not blocker.
+TEST_F(PermissionContextBaseTests, TestDismissVariationsWithoutEmbargo) {
+ TestVariationBlockOnSeveralDismissalsAutomaticEmbargoOff_TestContent();
+}
+
// Simulates non-valid requesting URL.
// The permission should be denied but not saved for future use.
TEST_F(PermissionContextBaseTests, TestNonValidRequestingUrl) {
diff --git a/components/permissions/permission_request.cc b/components/permissions/permission_request.cc
index 13916710..c71defe 100644
--- a/components/permissions/permission_request.cc
+++ b/components/permissions/permission_request.cc
@@ -36,10 +36,12 @@
PermissionRequest::PermissionRequest(
PermissionRequestData request_data,
PermissionDecidedCallback permission_decided_callback,
- base::OnceClosure delete_callback)
+ base::OnceClosure delete_callback,
+ bool uses_automatic_embargo)
: data_(std::move(request_data)),
permission_decided_callback_(std::move(permission_decided_callback)),
- delete_callback_(std::move(delete_callback)) {}
+ delete_callback_(std::move(delete_callback)),
+ uses_automatic_embargo_(uses_automatic_embargo) {}
PermissionRequest::~PermissionRequest() {
DCHECK(delete_callback_.is_null());
diff --git a/components/permissions/permission_request.h b/components/permissions/permission_request.h
index 31b7605..eae05c47 100644
--- a/components/permissions/permission_request.h
+++ b/components/permissions/permission_request.h
@@ -60,7 +60,8 @@
PermissionRequest(PermissionRequestData request_data,
PermissionDecidedCallback permission_decided_callback,
- base::OnceClosure delete_callback);
+ base::OnceClosure delete_callback,
+ bool uses_automatic_embargo);
PermissionRequest(const PermissionRequest&) = delete;
PermissionRequest& operator=(const PermissionRequest&) = delete;
@@ -201,6 +202,8 @@
// identify the permission being requested.
virtual std::u16string GetPermissionNameTextFragment() const;
+ bool uses_automatic_embargo() const { return uses_automatic_embargo_; }
+
protected:
// Sets whether this request is permission element initiated, for testing
// subclasses only.
@@ -217,6 +220,8 @@
// caller.
base::OnceClosure delete_callback_;
+ const bool uses_automatic_embargo_ = true;
+
base::WeakPtrFactory<PermissionRequest> weak_factory_{this};
};
diff --git a/components/permissions/permission_request_manager.cc b/components/permissions/permission_request_manager.cc
index cb42e633..12bcea2 100644
--- a/components/permissions/permission_request_manager.cc
+++ b/components/permissions/permission_request_manager.cc
@@ -1058,10 +1058,6 @@
prediction_grant_likelihood_, was_decision_held_back_, ignore_reason,
did_show_prompt_, did_click_manage_, did_click_learn_more_);
- PermissionDecisionAutoBlocker* autoblocker =
- PermissionsClient::Get()->GetPermissionDecisionAutoBlocker(
- browser_context);
-
absl::optional<QuietUiReason> quiet_ui_reason;
if (ShouldCurrentRequestUseQuietUI())
quiet_ui_reason = ReasonForUsingQuietUi();
@@ -1084,27 +1080,8 @@
request->GetGestureType(), quiet_ui_reason, time_since_shown,
web_contents());
- PermissionEmbargoStatus embargo_status =
- PermissionEmbargoStatus::NOT_EMBARGOED;
- if (permission_action == PermissionAction::DISMISSED &&
- !request->IsEmbeddedPermissionElementInitiated()) {
- if (autoblocker->RecordDismissAndEmbargo(
- request->requesting_origin(), request->GetContentSettingsType(),
- ShouldCurrentRequestUseQuietUI())) {
- embargo_status = PermissionEmbargoStatus::REPEATED_DISMISSALS;
- }
- } else if (permission_action == PermissionAction::IGNORED &&
- !request->IsEmbeddedPermissionElementInitiated()) {
- if (autoblocker->RecordIgnoreAndEmbargo(
- request->requesting_origin(), request->GetContentSettingsType(),
- ShouldCurrentRequestUseQuietUI())) {
- embargo_status = PermissionEmbargoStatus::REPEATED_IGNORES;
- }
- } else if (permission_action == PermissionAction::GRANTED_ONCE) {
- autoblocker->RemoveEmbargoAndResetCounts(
- request->requesting_origin(), request->GetContentSettingsType());
- }
- PermissionUmaUtil::RecordEmbargoStatus(embargo_status);
+ PermissionUmaUtil::RecordEmbargoStatus(RecordActionAndGetEmbargoStatus(
+ browser_context, request, permission_action));
}
// IGNORED is not a decision on the prompt and it occurs because of external
@@ -1518,6 +1495,41 @@
return view_->ShouldFinalizeRequestAfterDecided();
}
+PermissionEmbargoStatus
+PermissionRequestManager::RecordActionAndGetEmbargoStatus(
+ content::BrowserContext* browser_context,
+ PermissionRequest* request,
+ PermissionAction permission_action) {
+ if (!request->uses_automatic_embargo()) {
+ return PermissionEmbargoStatus::NOT_EMBARGOED;
+ }
+
+ PermissionDecisionAutoBlocker* const autoblocker =
+ PermissionsClient::Get()->GetPermissionDecisionAutoBlocker(
+ browser_context);
+
+ if (permission_action == PermissionAction::DISMISSED &&
+ !request->IsEmbeddedPermissionElementInitiated()) {
+ if (autoblocker->RecordDismissAndEmbargo(
+ request->requesting_origin(), request->GetContentSettingsType(),
+ ShouldCurrentRequestUseQuietUI())) {
+ return PermissionEmbargoStatus::REPEATED_DISMISSALS;
+ }
+ } else if (permission_action == PermissionAction::IGNORED &&
+ !request->IsEmbeddedPermissionElementInitiated()) {
+ if (autoblocker->RecordIgnoreAndEmbargo(request->requesting_origin(),
+ request->GetContentSettingsType(),
+ ShouldCurrentRequestUseQuietUI())) {
+ return PermissionEmbargoStatus::REPEATED_IGNORES;
+ }
+ } else if (permission_action == PermissionAction::GRANTED_ONCE) {
+ autoblocker->RemoveEmbargoAndResetCounts(request->requesting_origin(),
+ request->GetContentSettingsType());
+ }
+
+ return PermissionEmbargoStatus::NOT_EMBARGOED;
+}
+
WEB_CONTENTS_USER_DATA_KEY_IMPL(PermissionRequestManager);
} // namespace permissions
diff --git a/components/permissions/permission_request_manager.h b/components/permissions/permission_request_manager.h
index ee443d6f..40b4e4c 100644
--- a/components/permissions/permission_request_manager.h
+++ b/components/permissions/permission_request_manager.h
@@ -408,6 +408,12 @@
// permission decision.
bool ShouldFinalizeRequestAfterDecided(PermissionAction action) const;
+ // Calculate and record the PermissionEmbargoStatus.
+ PermissionEmbargoStatus RecordActionAndGetEmbargoStatus(
+ content::BrowserContext* browser_context,
+ PermissionRequest* request,
+ PermissionAction permission_action);
+
// Factory to be used to create views when needed.
PermissionPrompt::Factory view_factory_;
