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.

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

Fix shields setting in private is weird #1097

Merged
merged 2 commits into from
Dec 31, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Dec 14, 2018

When content setting's IncognitoBehavior is INHERIT_IF_LESS_PERMISSIVE,
normal profiles setting is modified when incognito profile's content settings
are initialized. Because of this behavior, sometimes shields are off and shields's
default configurations are different with normal window.
With INHERIT_IN_INCOGNITO, private windows uses normal windows' configuration w/o
modification.

Fix brave/brave-browser#1373
Fix brave/brave-browser#1897

Below is the reason why some users have private window with shields is always off.
Some times ago, the way of brave's default content settings is refactored.(#622)
Before this refactoring, shields content setting is stored to prefs like this.

void BraveHostContentSettingsMap::InitializeBraveShieldsContentSetting() {	
  SetContentSettingCustomScope(	
      ContentSettingsPattern::Wildcard(),	
      ContentSettingsPattern::Wildcard(),	
      CONTENT_SETTINGS_TYPE_PLUGINS,	
      brave_shields::kBraveShields,	
      CONTENT_SETTING_ALLOW);	
}

With this, user data stores allow value for brave shields with wildcard in ./BraveSoftware/Brave-browser/Default/Preferences file. When private windows are open with this normal windows' value, private window changes it from allow to block. If prefs doesn't have that value, private window uses it w/o modification. So, user can see shields is on in private window with clean profile.

Why private window modifies when normal windows' prefs is inherited? The reason is plugins content setting's incognito behavior is INHERIT_IF_LESS_PERMISSIVE. When normal window has stored content setting value, it is modified by ProcessIncognitoInheritanceBehavior() in host_content_settings_map.cc. If there is no value in prefs, private window uses default value.
We can see brave's default plugin value by GetDefaultFromResourceIdentifier() in brave_shields_util.cc

So, we can expect below scenario for plugins type of kBraveShields.

normal window private window
empty ALLOW ALLOW
ALLOW ALLOW BLOCK
BLOCK BLOCK BLOCK

Above table can explain current behavior.

In conclusion, deleting default wildcard rule can't solve current issue because when user changes shields settings, its value is stored in user data explicitly. In this case, private window will get block setting. So, I think changing to INHERIT_IN_INCOGNITO is the solution.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

yarn test brave_browser_tests --filter=BraveContentSettingsRegistryBrowserTest.*

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

@simonhong
Copy link
Member Author

Will remove WIP tag after tests are added.

pilgrim-brave
pilgrim-brave previously approved these changes Dec 14, 2018
Copy link
Contributor

@pilgrim-brave pilgrim-brave left a comment

Choose a reason for hiding this comment

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

LGTM if tests are added

@simonhong
Copy link
Member Author

@pilgrim-brave Test cases are added. Please check again. Thanks!

@simonhong simonhong changed the title WIP: Fix shields setting in private is weird Fix shields setting in private is weird Dec 15, 2018
@simonhong simonhong force-pushed the fix_shields_settings_in_private branch from 0bd58c7 to 985a492 Compare December 15, 2018 04:53
pilgrim-brave
pilgrim-brave previously approved these changes Dec 17, 2018
When content setting's IncognitoBehavior is INHERIT_IF_LESS_PERMISSIVe,
normal profiles setting is modified when incognito profile's content settings
are initialized. Because of this behavior, sometimes shields are off and shields's
default configurations are different with normal window.
With INHERIT_IN_INCOGNITO, private windows uses normal windows' configuration w/o
modification.
@simonhong
Copy link
Member Author

Rebased on latest mater. Please check again.
Verified on mac and all new tests are passed.

@simonhong simonhong merged commit cdbbb35 into master Dec 31, 2018
@simonhong
Copy link
Member Author

simonhong commented Dec 31, 2018

master(cdbbb35)
0.60.x(e034955)

simonhong added a commit that referenced this pull request Jan 1, 2019
@bbondy bbondy added this to the 0.60.x - Dev milestone Jan 14, 2019
@bsclifton bsclifton deleted the fix_shields_settings_in_private branch January 24, 2019 21:28
@btlechowski btlechowski mentioned this pull request Oct 5, 2020
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shields disabled on private window Public content settings are not inherited by private window
3 participants