Shields settings is lost after clearing data and relaunching

[srirambv]

Description

Shields settings is lost after clearing data and relaunching

Steps to Reproduce

1. Visit brave.com and disable shields
2. Visit github.com and change site shields settings from default to custom
3. Clear browsing data from all time and select all checkboxes
4. Restart browser after data is cleared
5. Open brave.com, shields is auto enabled
6. Open github.com, site shields settings are lost and reverted back to default settings

Actual result:

Shields settings is lost after clearing data and relaunching

Expected result:

Clear data shouldn't reset shields settings

Reproduces how often:

Easy

Brave version (brave://version info)

Brave	0.56.15 Chromium: 70.0.3538.110 (Official Build) (64-bit)
Revision	ca97ba107095b2a88cf04f9135463301e685cbb0-refs/branch-heads/3538@{#1094}
OS	All

Reproducible on current release:

• Does it reproduce on brave-browser dev/beta builds?
  Yes

Website problems only:

• Does the issue resolve itself when disabling Brave Shields?
• Is the issue reproducible on the latest version of Chrome?

Additional Information

Issue reproduced by @kjozwiak on macOS and @GeetaSarvadnya on Windows
cc: @diracdeltas @tomlowenthal @bbondy this should probably be part of 0.58.x or any earlier hotfix

Designs

Add clear browsing data controls for Shields.

On the "Clear browsing data" panel, add a "Shields site-specific preferences" checkbox which is off by default on the "Advanced" and "On exit" tabs.

In the Shields section in Settings, add a row at the bottom to "Clear all site-specific Shields settings"

Clicking on it will trigger an alert dialogue. The user has to click "Reset Shields" to clear Shields site-specific preferences.

[bbondy]

Shields are stored as content settings so I think this is expected? Maybe more desirable as a separate item to clear but probably not an urgent bug since it seems reasonable currently as far as I can tell.

[diracdeltas]

yup, it was a separate clear item in b-l. i think it's more desirable to have shields be cleared with history than to have no way to clear shields at all

[diracdeltas]

discussed in security confab; if a setting is added for this we think it should be in chrome://settings/clearBrowserData under the advanced panel

[bbondy]

cc @mkarolin since you've been around that code.

[eljuno]

+1 from community https://community.brave.com/t/settings-going-to-default/41261?u=eljuno

[srirambv]

+1 from @xetorixik via #4263

[xetorixik]

@srirambv Thanks for the heads up…
As this bug is labeled in this topic as "not an urgent bug" in December 2018.
What is going to happen?

[kjozwiak]

@srirambv Thanks for the heads up…
As this bug is labeled in this topic as "not an urgent bug" in December 2018.
What is going to happen?

@xetorixik it's currently labelled as a P3 which means it will most likely get done once the P1 & P2 are completed. There's currently no timeline but it will definitely get fixed once the more important issues are addressed. If this receives more +1 from the community, we will reprioritize as needed.

[simonhong]

@btlechowski @GeetaSarvadnya I think this PR's description is outdated.
After the discussion, we don't want to separate shields settings from site settings.
Because of that reason, its name is changed to Site and Shields Settings from Site Settings.
So, I think #8813 is not an issue.

[ze0ss]

@simonhong what?!
This isn't going to be fixed?
Please untie the settings, they get cleared after auto clearing history+data.
If #2400 wasn't fixed, why was it closed?

[simonhong]

Ah,, @abh-oss01 . you're right.
This issue should not be closed. re-opened.

[bridiver]

@abh-oss01 2400 was fixed. Shield settings are site settings and we changed the wording to make this more clear. If you select all the checkboxes, your shields setting absolutely should be deleted just like all other sites settings. This is not a bug, this is the correct and expected behavior and the fix was to make sure people knew exactly what they were deleting. cc @bbondy

[bridiver]

added closed/invalid because this is the expected and correct behavior. The label for the checkbox has been updated to make it clear to users that shield settings are part of site settings so they don't delete them accidentally.

[ze0ss]

@bridiver the whole issue was opened to get the settings separated/untied from site settings
how is this site settings anyway?
this must be separate...
pls make the global settings simpler to understand and implement, and, the current ("fixed") behaviour is no different than the previous one which was a bug.

[bridiver]

@abh-oss01 the previous behavior was not a bug. These are all site settings and the label has been updated to make it clear that shields are part of site settings. Trying to split things up would make it even more confusing because nobody would know which checkbox (site settings vs shield settings) deleted which settings. For instance, where would JS settings go? On Chrome those are deleted as part of site settings, but in Brave they are one of the shield settings. You can also create site settings for cookies using either shields or chromium site settings. If I have cookie settings from both, will only some of my cookie settings be deleted with shield settings? The fact that we call some of the settings "shield settings" does not change the fact that all of them are in fact site settings. You are asking us to separate things in a way that @simonhong was not even sure how to implement because of things like JS and cookie settings. If Brave devs don't even know which settings should go with which checkbox, how are users supposed to know?

[bridiver]

@abh-oss01 also what is the use case for deleting just some of your site settings, but not others? You want to delete your site location, camera, microphone, flash, images, sound, autoplay, notifications, etc... settings, but not your cookie, httpse, js and fingerprinting settings? And that's ignoring the fact that cookie and JS are listed as part of site settings in brave://settings/content

[bridiver]

The bug here is that the checkbox was not labelled in a way that made it clear to users that shields settings are part of site settings. That has been corrected because we don't want people to unintentionally delete their settings.

[ze0ss]

@bridiver Yes, to achieve fine grain control over what gets deleted, we would've liked clearing cookies, site settings, and js leftovers, but not the shield settings.
That was the whole point of the issue.

[bridiver]

@abh-oss01 your comment reinforces the fact that nobody would understand exactly what gets deleted for each option if we split it up. Also if you want fine grain control over site settings, it is already available in brave://settings/content. The only site settings that are not currently listed there are httpse and fingerprinting and @GeetaSarvadnya has opened a ticket for that #8840