Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't use CONTENT_SETTING_DEFAULT for FP setting #5879

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Don't use CONTENT_SETTING_DEFAULT for FP setting #5879

merged 1 commit into from
Jun 24, 2020

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jun 17, 2020 •
edited
Loading

CONTENT_SETTING_DEFAULT means deleting the current content setting.
With DEFAULT, it always use global FP setting.
But we want DEFAULT as a standard FP blocking instead of fallback to
global settings. Use secondary pattern for marking setting as balanced mode.

Resolves brave/brave-browser#10285

Submitter Checklist:

Test Plan:

See STR in the issue

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@simonhong simonhong added this to the 1.12.x - Nightly milestone Jun 17, 2020
@simonhong simonhong self-assigned this Jun 17, 2020
simonhong
simonhong commented Jun 17, 2020
View reviewed changes
components/brave_shields/browser/brave_shields_util.cc Outdated
// With DEFAULT, it always use global FP setting.
// But we want DEFAULT as a standard FP blocking instead of fallback to
// global settings.
// As a workaround, picked CONTENT_SETTING_ASK to store it persistently.
Copy link
Member Author

@simonhong simonhong Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this fix is best way.. Changed to use secondary pattern.

@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Jun 18, 2020
Closed
@simonhong simonhong added the CI/skip Do not run CI builds (except noplatform) label Jun 18, 2020
simonhong
simonhong commented Jun 18, 2020
View reviewed changes
components/brave_shields/browser/brave_shields_util.cc Outdated
if (balanced_setting == CONTENT_SETTING_BLOCK) {
if (setting == CONTENT_SETTING_BLOCK) {
// There is no way to determine whether site setting is balanced or
// aggressive(block) here.
Copy link
Member Author

@simonhong simonhong Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to determine whether site setting is balanced or aggressive in here.
If global setting is aggressive(BLOCK) and |balanced_setting| is also BLOCK, it's ambiguous to determine where BLOCK comes from global or site's balanced fp value. fixed.

@simonhong simonhong removed the CI/skip Do not run CI builds (except noplatform) label Jun 19, 2020
@simonhong simonhong force-pushed the fp_v2_settings branch 2 times, most recently from 41ed463 to 667490f Compare June 19, 2020 08:25
bridiver
bridiver reviewed Jun 19, 2020
View reviewed changes
components/brave_shields/browser/brave_shields_util.cc
primary_pattern, ContentSettingsPattern::Wildcard(),
ContentSettingsType::PLUGINS, kFingerprintingV2,
GetDefaultAllowFromControlType(type));
HostContentSettingsMapFactory::GetForProfile(profile)->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need two rules for this. The secondary pattern is either "balanced" or it's a wildcard. It should never be both.

Copy link
Member Author

@simonhong simonhong Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to have one fp rule for one url. - e2213b5817d4dc3b3c12a12da47bff69295034f4

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, it's still setting two rules here. We should never be setting both wildcard and balanced for the secondary pattern

Copy link
Member Author

@simonhong simonhong Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this PR only set one rule for one url but two patterns.
If current www.brave.com has balanced, only one rule (second - balanced) is stored.
and if user set other rule(allow), it will have only one rule (second - *) is stored.
When new rule is stored previous balanced rule is not deleted automatically because both are different rule. So, clearing step is needed.
If we don't clear here, prefs could have two rules.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. I missed the comment

@simonhong simonhong requested a review from bridiver June 22, 2020 01:13
@simonhong simonhong changed the title Fixed setting default FP always fallback to global FP setting Don't use CONTENT_SETTING_DEFAULT for FP setting Jun 22, 2020
@simonhong simonhong changed the title Don't use CONTENT_SETTING_DEFAULT for FP setting Fix regressions after introducing fingerprinting v2 Jun 22, 2020
bridiver
bridiver reviewed Jun 22, 2020
View reviewed changes
chromium_src/chrome/browser/prefs/browser_prefs.cc Outdated
@@ -54,4 +56,9 @@ void MigrateObsoleteProfilePrefs(Profile* profile) {
// Added 1/2020
brave_wallet::MigrateBraveWalletPrefs(profile);
#endif

// Added 6/2020
brave_shields::MigrateFingerprintingPrefsFromV1ToV2(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we intentionally did not migrate settings cc @pes10k @pilgrim-brave

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration commit is removed from this PR.

@simonhong simonhong changed the title Fix regressions after introducing fingerprinting v2 Don't use CONTENT_SETTING_DEFAULT for FP setting Jun 22, 2020
bridiver
bridiver reviewed Jun 23, 2020
View reviewed changes
components/brave_shields/browser/brave_shields_util.cc Outdated
ContentSettingsPattern::FromString("https://balanced/*");

if (type != ControlType::DEFAULT) {
content_setting = (type == ControlType::ALLOW) ? CONTENT_SETTING_ALLOW
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have helpers for this GetDefaultAllowFromControlType and GetDefaultBlockFromControlType

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@simonhong
Don't use CONTENT_SETTING_DEFAULT for FP setting
702a17b 
CONTENT_SETTING_DEFAULT means deleting the current content setting.
With DEFAULT, it always use global FP setting.
But we want DEFAULT as a standard FP blocking instead of fallback to
global settings. Use secondary pattern for marking setting as balanced mode.
@simonhong
Copy link
Member Author

Merged because only unrelated RewardsFlagBrowserTest.HandleFlagsWrongInput test was failed.

@simonhong simonhong merged commit 3bf621a into master Jun 24, 2020
@simonhong simonhong deleted the fp_v2_settings branch June 24, 2020 01:11
brave-builds pushed a commit that referenced this pull request Jun 24, 2020
@brave-browser-releases
Uplift of #5879 (squashed) to beta
7f27982
@brave-builds brave-builds mentioned this pull request Jun 24, 2020
Merged
4 tasks
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.12.58 Chromium: 83.0.4103.116 (Official Build) nightly (64-bit)
Revision 8f0c18b4dca9b6699eb629be0f51810c24fb6428-refs/branch-heads/4103@{# 716}
OS Linux
  • Verified able to change shields settings on site and is retained
  • Verified page reloads correctly after settings is changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@pes10k pes10k Awaiting requested review from pes10k

@mkarolin mkarolin Awaiting requested review from mkarolin

@pilgrim-brave pilgrim-brave Awaiting requested review from pilgrim-brave

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
1.12.x - Release #1
Development

Successfully merging this pull request may close these issues.

[Desktop] Site settings isn't retained
3 participants
@simonhong @srirambv @bridiver