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

Manage script should not show enforced by extension #232

Closed
srirambv opened this issue May 27, 2018 · 9 comments
Closed

Manage script should not show enforced by extension #232

srirambv opened this issue May 27, 2018 · 9 comments

Comments

@srirambv
Copy link
Contributor

srirambv commented May 27, 2018

Description

Need to allow users to be able to change content settings set by shield extension.

Steps to Reproduce

  1. Build from source () and load any site in new tab
  2. Block scripts on the page
  3. Click on Block script icon in URL bar and click manage, settings page shows exception added
  4. Hover over extension icon shows This setting is enforced by the "" extension

Actual result:

image

Expected result:

Should show shields as extension

Reproduces how often:

Easy

Brave version (about:brave info)

2e9c470

@srirambv srirambv added feature/extensions feature/global-settings Settings at browser level independent of shields settings labels May 27, 2018
@bbondy bbondy changed the title Shields doesn't show up as extension Manage script should not show enforced by extension Jun 2, 2018
@bbondy bbondy self-assigned this Jun 6, 2018
@bbondy bbondy added this to the Milestone 3: June-July milestone Jun 6, 2018
@simonhong simonhong self-assigned this Aug 28, 2018
@simonhong
Copy link
Member

simonhong commented Sep 3, 2018

The enum value SETTING_SOURCE_EXTENSION of enum SettingSource in content_settings.h would have some clues.

It seems that content settings from extensions are managed separately from settings/content webui or page info bubble.
I think that the main issue is there are two different content_settings are used.(They are stored separately in Preferences and Secure Preferences file in user data)

Basically, editing isn't allowed when content settings are not set by user.
When content settings from extensions are set regardless of default or not, that value is always used instead of content settings from per-site content setting by settings/content or page info bubble.


From the high level view, there are three way to change content settings set by shield extension.

  • (1) Implement CustomExtensionProvider::SetWebsiteSetting(...)
  • (2) Makes brave shield use user preference
    • (a) via chrome.send() (not sure it is possible)
    • (b) use new shields api that set content settings to pref(not by extension content settings store)

About , I think it needs a lot of implementation or changing upstream code. We need to allow setting source is from shield extension(currently only settings from user is allowed to change). Also, we need to know whether this setting is set by shield extension in settings or page info bubble.
About 2, I'm not sure extension can change user prefs(ex, allowing/blocking javascript) by using chrome.send().

2-b would be simplest way.

@simonhong
Copy link
Member

simonhong commented Sep 3, 2018

We should also change settings from page info bubble.

screen shot 2018-09-03 at 13 21 07

@simonhong
Copy link
Member

simonhong commented Sep 14, 2018

chrome.braveShields.javascript.set({}) works well from shield with this brave/brave-core@228d872,
When shields set javascript setting with this new api, user can edit this setting from page info bubble and setting page.

All below issues are resolved.

  1. Async doesn't work. chrome.braveShields.javascript.setAsync({}) causes type error. shields uses chrome.contentSettings.javascript.setAsync({}) but there is only set() api in chrome api spec.
    => chrome.braveShields.javascript.[set|get]Async() works well. Resolved by using promisifyAll (thanks @yrliou for advice)

  2. chrome.braveShields.plugin doesn't work properly. Need to debug.
    =>Works fine with Use braveShields's contentSetting method for shield configuration brave-extension#63.

  3. BRAVE_SHIELDS and HTTP_UPGRADABLE_RESOURCES settings settings have errors
    => Invalid url error (*://www.google.com)

    => Fixed by using content_settings_helpers::ParseExtensionPattern()

  4. If shield already set some content settings, user can't edit because chrome disallow edit settings touched by extension.
    => Already set value is deleted when user changed current value. Then new value is set to user preference instead.

@simonhong
Copy link
Member

Need to check current behavior isn't affected.

  • BraveContentSettingsObserver
  • BraveContentSettingsStore

@simonhong
Copy link
Member

With current PR, user only can edit after user changes from shields because settings by extension is cleared when shields set is called.
User can't edit from page info bubble or settings page before user changes settings from shields.
Need to find a way to make user be able to edit always.

@bbondy
Copy link
Member

bbondy commented Sep 22, 2018

@simonhong please post an issue for the last comment that you made and we can move forward with landing what you have for this one. I think we can not have the second part you mention for 0.55.x.

@simonhong
Copy link
Member

@bbondy Ok, I'll create new issue about above my last comment after finishing review of brave/brave-core#439 because that will be happened when brave/brave-core#439 is merged.

@simonhong
Copy link
Member

User can edit script settings from settings or page info bubble with brave/brave-extension#63.

@srirambv
Copy link
Contributor Author

srirambv commented Oct 5, 2018

Verification passed on

Brave 0.55.11 Chromium: 70.0.3538.35 (Official Build) beta(64-bit)
Revision 28dcb499844fa40c28d5f62e337876cb936f79f5-refs/branch-heads/3538@{#678}
OS Linux

screenshot from 2018-10-05 15-47-56
screenshot from 2018-10-05 15-48-37

Verified passed with

Brave 0.55.11 Chromium: 70.0.3538.35 (Official Build) beta(64-bit)
Revision 28dcb499844fa40c28d5f62e337876cb936f79f5-refs/branch-heads/3538@{#678}
OS Mac OS X

Verification passed on

Brave 0.55.11 Chromium: 70.0.3538.35 (Official Build) beta (64-bit)
Revision 28dcb499844fa40c28d5f62e337876cb936f79f5-refs/branch-heads/3538@{#678}
OS Windows 7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants