New issue
Advanced search Search tips

Issue 839681 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Extensions Click-to-Script: Remove automatic sync application logic

Project Member Reported by rdevlin....@chromium.org, May 4 2018

Issue description

Way back when, kalman@ and I thought it would be a good idea to have click-to-script incorporate sync logic to automatically restrict host permissions on machines, even if that machine didn't have the experiment enabled.  The motivation here was that we didn't want to display the "all_urls" warning in the installation dialog, and wanted to also ensure that users were safe (i.e., didn't install an all_urls-permissioned extension and think it wouldn't run anywhere, only to have it sync and run everywhere).

In retrospect, this was a mistake.  Experiments should always be disable-able through finch and/or chrome://flags, and need an escape hatch.  This sync logic prevented that from being the case.

Instead, we should:
- Ensure that click-to-script is only in effect if the experiment is enabled.
- Ensure users are still aware of what they're agreeing to when they install an extension (currently, this is a weird, long-text warning at the bottom of the install dialog - we'll likely change that to just display the all-urls warning).

2-page (for now) doc:
https://docs.google.com/document/d/1WxhklYOh-KFf1Jg7LOXuQTITYlZPQp3_o67kI5Lvr80/edit#
(sorry, google-only for now)
 
This gets trickier because we need to ensure that old clients still running with the sync behavior don't have any issues.  In particular, if we start syncing the pref in all cases (as we eventually want to), right now older clients will take that as a signal to withhold permissions (if set to false).
Summary: Extensions Click-to-Script: Remove automatic sync application logic (was: Extensions Click-to-Script: Remove sync application logic)
Project Member

Comment 3 by bugdroid1@chromium.org, May 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56daf3513a3dd7569d1ddbf53da4c85ca4173855

commit 56daf3513a3dd7569d1ddbf53da4c85ca4173855
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu May 10 16:25:33 2018

[Extensions Click-to-Script] Remove sync'd all_urls_enabled bit

Awhile back, we added logic to withhold all-hosts style permissions via
sync if the user had withheld hosts on any machine. This was a mistake,
since it resulted in the user being affected by an experiment across
all machines, even if they only opted into the experiment on one.

Remove the all_urls_enabled bit on the ExtensionSpecifics proto. A
replacement will be added in a subsequent patch. We can't just re-use
the existing one, because we need to prevent older machines from
seeing the preference and automatically applying experimental behavior.

Bug:  839681 
Change-Id: I735b7005ca76b9abe3b61709083bb4f552d0a47f
Reviewed-on: https://chromium-review.googlesource.com/1047850
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557548}
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_service_sync_unittest.cc
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_sync_data.cc
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_sync_data.h
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_sync_data_unittest.cc
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_sync_service.cc
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/extensions/extension_sync_service.h
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/chrome/browser/sync/test/integration/two_client_apps_sync_test.cc
[modify] https://crrev.com/56daf3513a3dd7569d1ddbf53da4c85ca4173855/components/sync/protocol/extension_specifics.proto

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34a47acf551fda25534bde4146c5f94409bd8b5b

commit 34a47acf551fda25534bde4146c5f94409bd8b5b
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Jul 12 03:08:20 2018

[Extensions Cleanup] Remove unnecessary call to sync service

The runtime host permissions preferences used to be synced, but aren't
currently. Remove the call to the sync service, since it's unnecessary.

If/when we add back syncing support, we can re-add this call.

Bug:  839681 
Change-Id: Iba45e431ad09f8ff92861c69ae8447c125af707a
Reviewed-on: https://chromium-review.googlesource.com/1134433
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574474}
[modify] https://crrev.com/34a47acf551fda25534bde4146c5f94409bd8b5b/chrome/browser/extensions/scripting_permissions_modifier.cc

Sign in to add a comment