[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}

chrome/browser/extensions/extension_sync_data.cc

@@ -87,14 +87,12 @@ ExtensionSyncData::ExtensionSyncData(const Extension& extension,
                                      int disable_reasons,
                                      bool incognito_enabled,
                                      bool remote_install,
-                                     base::Optional<bool> all_urls_enabled,
                                      bool installed_by_custodian)
     : ExtensionSyncData(extension,
                         enabled,
                         disable_reasons,
                         incognito_enabled,
                         remote_install,
-                        all_urls_enabled,
                         installed_by_custodian,
                         StringOrdinal(),
                         StringOrdinal(),
@@ -105,7 +103,6 @@ ExtensionSyncData::ExtensionSyncData(const Extension& extension,
                                      int disable_reasons,
                                      bool incognito_enabled,
                                      bool remote_install,
-                                     base::Optional<bool> all_urls_enabled,
                                      bool installed_by_custodian,
                                      const StringOrdinal& app_launch_ordinal,
                                      const StringOrdinal& page_ordinal,
@@ -118,7 +115,6 @@ ExtensionSyncData::ExtensionSyncData(const Extension& extension,
       disable_reasons_(disable_reasons),
       incognito_enabled_(incognito_enabled),
       remote_install_(remote_install),
-      all_urls_enabled_(all_urls_enabled),
       installed_by_custodian_(installed_by_custodian),
       version_(extension.from_bookmark() ? base::Version("0")
                                          : extension.version()),
@@ -196,8 +192,6 @@ void ExtensionSyncData::ToExtensionSpecifics(
     specifics->set_disable_reasons(disable_reasons_);
   specifics->set_incognito_enabled(incognito_enabled_);
   specifics->set_remote_install(remote_install_);
-  if (all_urls_enabled_.has_value())
-    specifics->set_all_urls_enabled(*all_urls_enabled_);
   specifics->set_installed_by_custodian(installed_by_custodian_);
   specifics->set_name(name_);
 }
@@ -278,13 +272,6 @@ bool ExtensionSyncData::PopulateFromExtensionSpecifics(
   supports_disable_reasons_ = specifics.has_disable_reasons();
   disable_reasons_ = specifics.disable_reasons();
   incognito_enabled_ = specifics.incognito_enabled();
-  if (specifics.has_all_urls_enabled()) {
-    all_urls_enabled_ = specifics.all_urls_enabled();
-  } else {
-    // Set this explicitly (even though it's the default) on the offchance
-    // that someone is re-using an ExtensionSyncData object.
-    all_urls_enabled_ = base::nullopt;
-  }
   remote_install_ = specifics.remote_install();
   installed_by_custodian_ = specifics.installed_by_custodian();
   name_ = specifics.name();

chrome/browser/extensions/extension_sync_data.h

@@ -49,7 +49,6 @@ class ExtensionSyncData {
                     int disable_reasons,
                     bool incognito_enabled,
                     bool remote_install,
-                    base::Optional<bool> all_urls_enabled,
                     bool installed_by_custodian);
   // App constructor.
   ExtensionSyncData(const Extension& extension,
@@ -57,7 +56,6 @@ class ExtensionSyncData {
                     int disable_reasons,
                     bool incognito_enabled,
                     bool remote_install,
-                    base::Optional<bool> all_urls_enabled,
                     bool installed_by_custodian,
                     const syncer::StringOrdinal& app_launch_ordinal,
                     const syncer::StringOrdinal& page_ordinal,
@@ -90,7 +88,6 @@ class ExtensionSyncData {
   int disable_reasons() const { return disable_reasons_; }
   bool incognito_enabled() const { return incognito_enabled_; }
   bool remote_install() const { return remote_install_; }
-  base::Optional<bool> all_urls_enabled() const { return all_urls_enabled_; }
   bool installed_by_custodian() const { return installed_by_custodian_; }
 
   // Version-dependent properties (i.e., should be used only when the
@@ -156,7 +153,6 @@ class ExtensionSyncData {
   int disable_reasons_;
   bool incognito_enabled_;
   bool remote_install_;
-  base::Optional<bool> all_urls_enabled_;
   bool installed_by_custodian_;
   base::Version version_;
   GURL update_url_;

chrome/browser/extensions/extension_sync_data_unittest.cc

@@ -59,9 +59,6 @@ void ProtobufToSyncDataEqual(const sync_pb::EntitySpecifics& entity) {
   EXPECT_EQ(input.incognito_enabled(), output.incognito_enabled());
   EXPECT_EQ(input.remote_install(), output.remote_install());
   EXPECT_EQ(input.installed_by_custodian(), output.installed_by_custodian());
-  EXPECT_EQ(input.has_all_urls_enabled(), output.has_all_urls_enabled());
-  if (input.has_all_urls_enabled())
-    EXPECT_EQ(input.all_urls_enabled(), output.all_urls_enabled());
 }
 
 // Serializes an ExtensionSyncData into a protobuf structure and back again, and
@@ -78,7 +75,6 @@ void SyncDataToProtobufEqual(const ExtensionSyncData& input) {
   EXPECT_EQ(input.incognito_enabled(), output->incognito_enabled());
   EXPECT_EQ(input.remote_install(), output->remote_install());
   EXPECT_EQ(input.installed_by_custodian(), output->installed_by_custodian());
-  EXPECT_EQ(input.all_urls_enabled(), output->all_urls_enabled());
   EXPECT_EQ(input.version(), output->version());
   EXPECT_EQ(input.update_url(), output->update_url());
   EXPECT_EQ(input.name(), output->name());
@@ -100,7 +96,6 @@ TEST_F(ExtensionSyncDataTest, ExtensionSyncDataForExtension) {
   extension_specifics->set_incognito_enabled(true);
   extension_specifics->set_remote_install(false);
   extension_specifics->set_installed_by_custodian(false);
-  extension_specifics->set_all_urls_enabled(true);
   extension_specifics->set_version(kVersion);
   extension_specifics->set_name(kName);
 
@@ -117,35 +112,21 @@ TEST_F(ExtensionSyncDataTest, ExtensionSyncDataForExtension) {
   EXPECT_FALSE(extension_sync_data.enabled());
   EXPECT_EQ(true, extension_sync_data.incognito_enabled());
   EXPECT_FALSE(extension_sync_data.remote_install());
-  EXPECT_EQ(base::Optional<bool>(true), extension_sync_data.all_urls_enabled());
   EXPECT_EQ(base::Version(kVersion), extension_sync_data.version());
   EXPECT_EQ(std::string(kName), extension_sync_data.name());
 
   // Check the serialize-deserialize process for ExtensionSyncData to proto.
   SyncDataToProtobufEqual(extension_sync_data);
 
-  // The most important thing to test is the "all urls" bit, since it is a
-  // tri-state boolean (and thus has more logic). Also flip another bit for a
-  // sanity check.
-  extension_specifics->set_all_urls_enabled(false);
+  // Flip a bit and verify the result is correct.
   extension_specifics->set_incognito_enabled(false);
   ProtobufToSyncDataEqual(entity);
 
   extension_sync_data.PopulateFromExtensionSpecifics(*extension_specifics);
-  EXPECT_EQ(base::Optional<bool>(false),
-            extension_sync_data.all_urls_enabled());
   EXPECT_FALSE(extension_sync_data.incognito_enabled());
 
   SyncDataToProtobufEqual(extension_sync_data);
-
-  extension_specifics->clear_all_urls_enabled();
   ProtobufToSyncDataEqual(entity);
-
-  extension_sync_data.PopulateFromExtensionSpecifics(*extension_specifics);
-  EXPECT_FALSE(extension_specifics->has_all_urls_enabled());
-  EXPECT_FALSE(extension_sync_data.all_urls_enabled().has_value());
-
-  SyncDataToProtobufEqual(extension_sync_data);
 }
 
 class AppSyncDataTest : public testing::Test {
@@ -162,7 +143,6 @@ class AppSyncDataTest : public testing::Test {
     extension_specifics->set_disable_reasons(kValidDisableReasons);
     extension_specifics->set_incognito_enabled(true);
     extension_specifics->set_remote_install(false);
-    extension_specifics->set_all_urls_enabled(true);
     extension_specifics->set_installed_by_custodian(false);
     extension_specifics->set_name(kName);
   }

chrome/browser/extensions/extension_sync_service.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/extensions/extension_sync_service_factory.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/launch_util.h"
-#include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
 #include "chrome/common/buildflags.h"
@@ -51,26 +50,6 @@ using extensions::SyncBundle;
 
 namespace {
 
-// Returns the pref value for "all urls enabled" for the given extension id.
-base::Optional<bool> GetAllowedOnAllUrlsValue(
-    const Extension& extension,
-    content::BrowserContext* context) {
-  extensions::ScriptingPermissionsModifier permissions_modifier(context,
-                                                                &extension);
-  bool allowed_on_all_urls = permissions_modifier.IsAllowedOnAllUrls();
-  // If the extension is not allowed on all urls (which is not the default),
-  // then we have to sync the preference.
-  if (!allowed_on_all_urls)
-    return false;
-
-  // If the user has explicitly set a value, then we sync it.
-  if (permissions_modifier.HasSetAllowedOnAllUrls())
-    return true;
-
-  // Otherwise, unset.
-  return base::nullopt;
-}
-
 // Returns true if the sync type of |extension| matches |type|.
 bool IsCorrectSyncType(const Extension& extension, syncer::ModelType type) {
   return (type == syncer::EXTENSIONS && extension.is_extension()) ||
@@ -271,23 +250,21 @@ ExtensionSyncData ExtensionSyncService::CreateSyncData(
   bool incognito_enabled = extensions::util::IsIncognitoEnabled(id, profile_);
   bool remote_install = extension_prefs->HasDisableReason(
       id, extensions::disable_reason::DISABLE_REMOTE_INSTALL);
-  base::Optional<bool> allowed_on_all_url =
-      GetAllowedOnAllUrlsValue(extension, profile_);
   bool installed_by_custodian =
       extensions::util::WasInstalledByCustodian(id, profile_);
   AppSorting* app_sorting = ExtensionSystem::Get(profile_)->app_sorting();
 
-  ExtensionSyncData result = extension.is_app()
-      ? ExtensionSyncData(
-            extension, enabled, disable_reasons, incognito_enabled,
-            remote_install, allowed_on_all_url,
-            installed_by_custodian,
-            app_sorting->GetAppLaunchOrdinal(id),
-            app_sorting->GetPageOrdinal(id),
-            extensions::GetLaunchTypePrefValue(extension_prefs, id))
-      : ExtensionSyncData(
-            extension, enabled, disable_reasons, incognito_enabled,
-            remote_install, allowed_on_all_url, installed_by_custodian);
+  ExtensionSyncData result =
+      extension.is_app()
+          ? ExtensionSyncData(
+                extension, enabled, disable_reasons, incognito_enabled,
+                remote_install, installed_by_custodian,
+                app_sorting->GetAppLaunchOrdinal(id),
+                app_sorting->GetPageOrdinal(id),
+                extensions::GetLaunchTypePrefValue(extension_prefs, id))
+          : ExtensionSyncData(extension, enabled, disable_reasons,
+                              incognito_enabled, remote_install,
+                              installed_by_custodian);
 
   // If there's a pending update, send the new version to sync instead of the
   // installed one.
@@ -481,13 +458,6 @@ void ExtensionSyncService::ApplySyncData(
       id, profile_, extension_sync_data.incognito_enabled());
   extension = nullptr;  // No longer safe to use.
 
-  // Update the all urls flag.
-  if (extension_sync_data.all_urls_enabled().has_value()) {
-    bool allowed = *extension_sync_data.all_urls_enabled();
-    extensions::ScriptingPermissionsModifier::SetAllowedOnAllUrlsForSync(
-        allowed, profile_, id);
-  }
-
   // Set app-specific data.
   if (extension_sync_data.is_app()) {
     if (extension_sync_data.app_launch_ordinal().IsValid() &&

chrome/browser/extensions/extension_sync_service.h

@@ -42,8 +42,7 @@ class ExtensionSyncService : public syncer::SyncableService,
 
   // Notifies Sync (if needed) of a newly-installed extension or a change to
   // an existing extension. Call this when you change an extension setting that
-  // is synced as part of ExtensionSyncData (e.g. incognito_enabled or
-  // all_urls_enabled).
+  // is synced as part of ExtensionSyncData (e.g. incognito_enabled).
   void SyncExtensionChangeIfNeeded(const extensions::Extension& extension);
 
   // Returns whether the extension with the given |id| will be re-enabled once

chrome/browser/sync/test/integration/two_client_apps_sync_test.cc

@@ -367,7 +367,6 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, UnexpectedLaunchType) {
       original_data.disable_reasons(),
       original_data.incognito_enabled(),
       original_data.remote_install(),
-      original_data.all_urls_enabled(),
       original_data.installed_by_custodian(),
       original_data.app_launch_ordinal(),
       original_data.page_ordinal(),

components/sync/protocol/extension_specifics.proto

@@ -42,11 +42,8 @@ message ExtensionSpecifics {
   // Whether this extension was installed by the custodian of a supervised user.
   optional bool installed_by_custodian = 8;
 
-  // Whether this extension has explicit user consent access to all urls.
-  // This is a tri-state boolean, so, unlike most fields here, it really *is*
-  // optional and may be absent. We need this for the time being because we need
-  // to know if a user has not set an explicit preference.
-  optional bool all_urls_enabled = 9;
+  // DEPRECATED. See https://crbug.com/839681.
+  optional bool all_urls_enabled = 9 [deprecated = true];
 
   // Bitmask of the set of reasons why the extension is disabled (see
   // extensions::disable_reason::DisableReason). Only relevant when enabled ==