Domain block and 1PES integration #10339

goodov · 2021-10-01T13:22:32Z

Add first party ephemeral storage auto-enable logic when a First Party Ephemeral Storage feature is enabled and a user proceeds a domain block interstitial.

Resolves brave/brave-browser#19099

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

components/ephemeral_storage/ephemeral_storage_service.h

browser/ephemeral_storage/ephemeral_storage_service_factory.cc

components/ephemeral_storage/BUILD.gn

iefremov · 2021-11-01T13:23:27Z

components/ephemeral_storage/ephemeral_storage_service.cc

+      FROM_HERE,
+      base::BindOnce(&UrlStorageChecker::StartCheck,
+                     base::MakeRefCounted<UrlStorageChecker>(
+                         context_->GetStoragePartitionForUrl(url, true), url,


this one is deprecated

iefremov · 2021-11-01T16:35:33Z

components/ephemeral_storage/url_storage_checker.h

+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"


iefremov · 2021-11-01T16:35:54Z

components/ephemeral_storage/url_storage_checker.h

+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
+#include "components/services/storage/public/mojom/local_storage_control.mojom.h"


can be moved to .cc ?

iefremov · 2021-11-01T17:16:44Z

components/ephemeral_storage/ephemeral_storage_service.h

+// actors.
+class EphemeralStorageService
+    : public KeyedService,
+      public base::SupportsWeakPtr<EphemeralStorageService> {


is it used anywhere?

iefremov · 2021-11-01T17:29:41Z

browser/ephemeral_storage/ephemeral_storage_1p_domain_block_browsertest.cc

+    WebContents* first_party_tab =
+        LoadURLInNewTab(a_site_ephemeral_storage_url_);
+    EXPECT_TRUE(IsShowingInterstitial(first_party_tab));
+    Click(first_party_tab, "dont-warn-again-checkbox");


why is it needed?

iefremov · 2021-11-01T17:31:05Z

browser/ephemeral_storage/ephemeral_storage_1p_domain_block_browsertest.cc

+  void NavigateToBlockedDomainAndExpectEphemeralEnabled() {
+    WebContents* first_party_tab = NavigateToBlockedDomain();
+
+    // After keepalive values should be cleared.


can you also check that they are NOT cleared while we are keepaliving?

iefremov · 2021-11-01T17:34:18Z

during manual testing i've noticed that keepalive actually doesn't work with 1PES, mb I'm doing something wrong?
wondering how we should deal with "don't ask again". If the checkbox is not checked, we still will keep the content setting for a host forever. Not a big deal, just want to check whether this is what we want.

goodov · 2021-11-02T07:26:55Z

during manual testing i've noticed that keepalive actually doesn't work with 1PES, mb I'm doing something wrong?

discussed in Slack. It works, but if a tab is closed (and no other tabs are opened with the same website), the storage is cleaned up immediately. Keep-alive works for in-tab navigations, for ex. when we navigate away and back; a use-case to remember is an SSO-like authorization (going to an auth website and back).

wondering how we should deal with "don't ask again". If the checkbox is not checked, we still will keep the content setting for a host forever. Not a big deal, just want to check whether this is what we want.

I think it doesn't matter if this options is checked or not. When it's not checked, any attempt to visit the website will still show the interstitial, and we will enable 1PES anyways (if for some reason we decide to turn it off after each visit).

iefremov · 2021-12-09T16:05:20Z

components/brave_shields/common/features.cc

-                                             base::FEATURE_DISABLED_BY_DEFAULT};
+// When enabled, Brave will attempt to enable 1PES mode in a standard blocking
+// mode when a user visists a domain that is present in currently active adblock
+// filters. 1PES will be enabled only if nor cookies nor localStorage data is


neither cookies nor localStorage are stored

iefremov · 2021-12-09T16:16:33Z

components/brave_shields/browser/brave_shields_util.h

@@ -32,6 +32,12 @@ enum ControlType {
  AGGRESSIVE
 };

+enum class DomainBlockingType {


iefremov · 2021-12-09T17:15:40Z

components/brave_shields/browser/brave_shields_util.cc

    return DomainBlockingType::kAggressive;
  }

-  if (base::FeatureList::IsEnabled(
+  // Block using 1PES if ad blocking is "standard".
+  if (cosmetic_control_type == BLOCK_THIRD_PARTY &&


not sure I understand why it is about cosmetic filters, but that's not relevant to the PR...

this is definitely a bit confusing and we need to consolidate the logic for adblock with its own control type. RIght now we're duplicating checks in BraveShieldsDataController and presumably in android for the UI setting

brave/brave-browser#39197

iefremov · 2021-12-09T17:38:24Z

components/brave_shields/common/features.cc

+// filters. 1PES will be enabled only if nor cookies nor localStorage data is
+// stored for the website.
+const base::Feature kBraveDomainBlock1PES{"BraveDomainBlock1PES",
+                                          base::FEATURE_DISABLED_BY_DEFAULT};


entry on brave://flags?

mariospr

chromium_src LGTM

bridiver · 2024-06-20T15:43:39Z

components/brave_shields/browser/brave_shields_util.cc

  // Don't block if feature is disabled
  if (!base::FeatureList::IsEnabled(brave_shields::features::kBraveDomainBlock))
-    return false;
+    return DomainBlockingType::kNone;

  // Don't block if Brave Shields is down (this also handles cases where
  // the URL is not HTTP(S))
  if (!brave_shields::GetBraveShieldsEnabled(map, url))


This goes against the established pattern of using Get for the raw value for display in the UI and I think that's a bit confusing

to be clear I'm talking about GetDomainBlockingType, just commenting here because of the shields check

maybe absl::optional<DomainBlockingType> ShouldDoDominBlocking(...) and get rid of kNone?

goodov requested review from a team as code owners October 1, 2021 13:22

goodov self-assigned this Oct 1, 2021

goodov force-pushed the debounce+1pes branch from d111307 to 4bb20ca Compare October 7, 2021 13:44

iefremov self-requested a review October 25, 2021 14:16

iefremov reviewed Oct 26, 2021

View reviewed changes

components/ephemeral_storage/ephemeral_storage_service.h Outdated Show resolved Hide resolved

iefremov reviewed Oct 26, 2021

View reviewed changes

components/ephemeral_storage/ephemeral_storage_service.h Outdated Show resolved Hide resolved

iefremov reviewed Oct 26, 2021

View reviewed changes

browser/ephemeral_storage/ephemeral_storage_service_factory.cc Show resolved Hide resolved

iefremov reviewed Oct 26, 2021

View reviewed changes

components/ephemeral_storage/BUILD.gn Outdated Show resolved Hide resolved

goodov force-pushed the debounce+1pes branch 2 times, most recently from d57d075 to f708321 Compare October 28, 2021 15:14

goodov requested a review from a team as a code owner October 28, 2021 15:14

goodov force-pushed the debounce+1pes branch 3 times, most recently from eda447d to 2678787 Compare October 29, 2021 11:29

goodov changed the title ~~WIP Debounce and 1pes integration~~ Debounce and 1pes integration Oct 29, 2021

goodov changed the title ~~Debounce and 1pes integration~~ Domain block and 1PES integration Oct 29, 2021

iefremov reviewed Nov 1, 2021

View reviewed changes

goodov force-pushed the debounce+1pes branch from 2678787 to 4752dca Compare November 3, 2021 17:34

goodov force-pushed the debounce+1pes branch 2 times, most recently from f40eb4f to ed16116 Compare November 30, 2021 11:20

iefremov self-requested a review December 1, 2021 08:50

iefremov reviewed Dec 9, 2021

View reviewed changes

iefremov approved these changes Dec 9, 2021

View reviewed changes

goodov force-pushed the debounce+1pes branch from ed16116 to 6aee03d Compare December 13, 2021 10:09

mariospr approved these changes Dec 15, 2021

View reviewed changes

goodov added 9 commits December 16, 2021 19:18

Add ephemeral storage enabler to domain block throttler.

80c33b2

Use recommended StoragePartition get method instead of a deprecated one.

a767911

Fix compilation.

7ae13e9

Support non aggressive blocking mode to auto-enable 1PES.

9e9599a

Add brave://settings strings change when 1PES feature is enabled.

4e226df

Rename feature, remove unnecessary include.

dfe7d7e

Add AdControl check into GetDomainBlockingType, add tests.

c3a211a

Keep 3p blocks when SESSION_ONLY is used without 1PES feature.

4a4d74f

Update strings, add flag, add comments.

f3c6d3e

goodov force-pushed the debounce+1pes branch from 6aee03d to f3c6d3e Compare December 16, 2021 12:18

goodov merged commit 6d7fb31 into master Dec 17, 2021

goodov deleted the debounce+1pes branch December 17, 2021 08:11

goodov added this to the 1.35.x - Nightly milestone Dec 17, 2021

bridiver reviewed Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain block and 1PES integration #10339

Domain block and 1PES integration #10339

goodov commented Oct 1, 2021 •

edited

Loading

iefremov Nov 1, 2021

iefremov Nov 1, 2021

iefremov Nov 1, 2021

iefremov Nov 1, 2021

iefremov Nov 1, 2021

iefremov Nov 1, 2021

iefremov commented Nov 1, 2021

goodov commented Nov 2, 2021

iefremov Dec 9, 2021 •

edited

Loading

iefremov Dec 9, 2021

iefremov Dec 9, 2021

bridiver Jun 20, 2024

bridiver Jun 20, 2024

iefremov Dec 9, 2021

mariospr left a comment

bridiver Jun 20, 2024 •

edited

Loading

bridiver Jun 20, 2024

bridiver Jun 20, 2024

Domain block and 1PES integration #10339

Domain block and 1PES integration #10339

Conversation

goodov commented Oct 1, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Nov 1, 2021

goodov commented Nov 2, 2021

iefremov Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariospr left a comment

Choose a reason for hiding this comment

bridiver Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goodov commented Oct 1, 2021 •

edited

Loading

iefremov Dec 9, 2021 •

edited

Loading

bridiver Jun 20, 2024 •

edited

Loading