[Extensions Sync] Don't apply sync data for unsyncable extensions

chrome/browser/extensions/extension_service_sync_unittest.cc

Index: chrome/browser/extensions/extension_service_sync_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_sync_unittest.cc b/chrome/browser/extensions/extension_service_sync_unittest.cc
index 1b810fa91a142b85b2d532312fa24bc3fa3cfcb9..1e8551da1528c1059719bf21ce4a98f7e1eeda37 100644
--- a/chrome/browser/extensions/extension_service_sync_unittest.cc
+++ b/chrome/browser/extensions/extension_service_sync_unittest.cc
@@ -19,6 +19,7 @@
 #include "base/test/mock_entropy_provider.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/api/webstore_private/webstore_private_api.h"
+#include "chrome/browser/extensions/chrome_test_extension_loader.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_with_install.h"
@@ -337,6 +338,160 @@ TEST_F(ExtensionServiceSyncTest, DisableExtensionFromSync) {
   ASSERT_FALSE(service()->IsExtensionEnabled(good0));
 }
 
+// Test that sync can enable and disable installed extensions.
+TEST_F(ExtensionServiceSyncTest, ReenableDisabledExtensionFromSync) {
+  InitializeEmptyExtensionService();
+
+  // Enable sync.
+  browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile());
+  sync_service->SetFirstSetupComplete();
+
+  service()->Init();
+
+  // Load up a simple extension.
+  extensions::ChromeTestExtensionLoader extension_loader(profile());
+  extension_loader.set_pack_extension(true);
+  scoped_refptr<const Extension> extension = extension_loader.LoadExtension(
+      data_dir().AppendASCII("simple_with_file"));
+  ASSERT_TRUE(extension);
+  const std::string kExtensionId = extension->id();
+  EXPECT_EQ(kExtensionId, extension->id());

[karandeepb, 2017/06/09 22:53:36]

This seems redundant. Same below.

[Devlin, 2017/06/13 16:14:44]

Whoops!  This expect was from an earlier version.  Definitely unnecessary now.
:)  Thanks for catching this!

+  ASSERT_TRUE(registry()->enabled_extensions().GetByID(kExtensionId));
+
+  syncer::FakeSyncChangeProcessor* processor =
+      new syncer::FakeSyncChangeProcessor;

[karandeepb, 2017/06/09 22:53:36]

optional nit: Create a unique pointer and then take a weak reference for use in
this function. Also same below.

[Devlin, 2017/06/13 16:14:44]

Done.

+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, syncer::SyncDataList(), base::WrapUnique(processor),
+      base::MakeUnique<syncer::SyncErrorFactoryMock>());
+
+  processor->changes().clear();
+
+  // Sync says to disable the extension.
+  ExtensionSyncData disable_extension(
+      *extension, false, Extension::DISABLE_USER_ACTION, false, false,
+      ExtensionSyncData::BOOLEAN_UNSET, false);
+  SyncChangeList enable_list(
+      1, disable_extension.GetSyncChange(SyncChange::ACTION_UPDATE));
+  extension_sync_service()->ProcessSyncChanges(FROM_HERE, enable_list);
+
+  // The extension should be disabled.
+  EXPECT_TRUE(registry()->disabled_extensions().GetByID(kExtensionId));
+  EXPECT_EQ(Extension::DISABLE_USER_ACTION,
+            ExtensionPrefs::Get(profile())->GetDisableReasons(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+
+  // Enable the extension. Sync should push the new state.
+  service()->EnableExtension(kExtensionId);
+  {
+    ASSERT_EQ(1u, processor->changes().size());
+    const SyncChange& change = processor->changes()[0];
+    EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type());
+    std::unique_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(change.sync_data());
+    EXPECT_EQ(kExtensionId, data->id());
+    EXPECT_EQ(0, data->disable_reasons());
+    EXPECT_TRUE(data->enabled());
+  }
+
+  // Disable the extension again. Sync should push the new state.
+  processor->changes().clear();
+  service()->DisableExtension(kExtensionId, Extension::DISABLE_USER_ACTION);
+  EXPECT_TRUE(registry()->disabled_extensions().GetByID(kExtensionId));
+  {
+    ASSERT_EQ(1u, processor->changes().size());
+    const SyncChange& change = processor->changes()[0];
+    EXPECT_EQ(SyncChange::ACTION_UPDATE, change.change_type());
+    std::unique_ptr<ExtensionSyncData> data =
+        ExtensionSyncData::CreateFromSyncData(change.sync_data());
+    EXPECT_EQ(kExtensionId, data->id());
+    EXPECT_EQ(Extension::DISABLE_USER_ACTION, data->disable_reasons());
+    EXPECT_FALSE(data->enabled());
+  }
+  processor->changes().clear();
+
+  // Enable the extension via sync.
+  ExtensionSyncData enable_extension(*extension, true, 0, false, false,
+                                     ExtensionSyncData::BOOLEAN_UNSET, false);
+  SyncChangeList disable_list(
+      1, enable_extension.GetSyncChange(SyncChange::ACTION_UPDATE));
+  extension_sync_service()->ProcessSyncChanges(FROM_HERE, disable_list);
+
+  // The extension should be enabled.
+  EXPECT_TRUE(registry()->enabled_extensions().GetByID(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+}
+
+// Tests that default-installed extensions won't be affected by incoming sync
+// data. (It's feasible to have a sync entry for an extension that could be
+// default installed, since one installation may be default-installed while
+// another may not be).
+TEST_F(ExtensionServiceSyncTest,
+       DefaultInstalledExtensionsAreNotReenabledOrDisabledBySync) {
+  InitializeEmptyExtensionService();
+
+  // Enable sync.
+  browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile());
+  sync_service->SetFirstSetupComplete();
+
+  service()->Init();
+
+  // Load up an extension that's considered default installed.
+  extensions::ChromeTestExtensionLoader extension_loader(profile());
+  extension_loader.set_pack_extension(true);
+  extension_loader.add_creation_flag(Extension::WAS_INSTALLED_BY_DEFAULT);
+  scoped_refptr<const Extension> extension = extension_loader.LoadExtension(
+      data_dir().AppendASCII("simple_with_file"));
+  ASSERT_TRUE(extension);
+
+  // The extension shouldn't sync.
+  EXPECT_FALSE(extensions::util::ShouldSync(extension.get(), profile()));
+  const std::string kExtensionId = extension->id();
+  EXPECT_EQ(kExtensionId, extension->id());
+  ASSERT_TRUE(registry()->enabled_extensions().GetByID(kExtensionId));
+
+  syncer::FakeSyncChangeProcessor* processor =
+      new syncer::FakeSyncChangeProcessor;
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS, syncer::SyncDataList(), base::WrapUnique(processor),
+      base::MakeUnique<syncer::SyncErrorFactoryMock>());
+  processor->changes().clear();
+
+  // Sync state says the extension is disabled (e.g. on another machine).
+  ExtensionSyncData disable_extension(
+      *extension, false, Extension::DISABLE_USER_ACTION, false, false,
+      ExtensionSyncData::BOOLEAN_UNSET, false);
+  SyncChangeList disable_list(
+      1, disable_extension.GetSyncChange(SyncChange::ACTION_UPDATE));
+  extension_sync_service()->ProcessSyncChanges(FROM_HERE, disable_list);
+
+  // The extension should still be enabled, since it's default-installed.
+  EXPECT_TRUE(registry()->enabled_extensions().GetByID(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+
+  // Now disable the extension locally. Sync should *not* push new state.
+  service()->DisableExtension(kExtensionId, Extension::DISABLE_USER_ACTION);
+  EXPECT_TRUE(registry()->disabled_extensions().GetByID(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+
+  // Sync state says the extension is enabled.
+  ExtensionSyncData enable_extension(*extension, true, 0, false, false,
+                                     ExtensionSyncData::BOOLEAN_UNSET, false);
+  SyncChangeList enable_list(
+      1, enable_extension.GetSyncChange(SyncChange::ACTION_UPDATE));
+  extension_sync_service()->ProcessSyncChanges(FROM_HERE, enable_list);
+
+  // As above, the extension should not have been affected by sync.
+  EXPECT_TRUE(registry()->disabled_extensions().GetByID(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+
+  // And re-enabling the extension should not push new state to sync.
+  service()->EnableExtension(kExtensionId);
+  EXPECT_TRUE(registry()->enabled_extensions().GetByID(kExtensionId));
+  EXPECT_TRUE(processor->changes().empty());
+}
+
 TEST_F(ExtensionServiceSyncTest, IgnoreSyncChangesWhenLocalStateIsMoreRecent) {
   // Start the extension service with three extensions already installed.
   base::FilePath source_install_dir =
@@ -1359,6 +1514,10 @@ TEST_F(ExtensionServiceSyncTest, ProcessSyncDataEnableDisable) {
 }
 
 TEST_F(ExtensionServiceSyncTest, ProcessSyncDataDeferredEnable) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kAppsGalleryUpdateURL,
+      "http://localhost/autoupdate/updates.xml");

[karandeepb, 2017/06/09 22:53:36]

This seems unrelated. I am assuming you also plan to remove this?

[Devlin, 2017/06/13 16:14:44]

Actually, this one's important.  We only sync extensions that update from the
webstore, so we need to pretend that this one does.  I've added a comment
explaining this.

+
   InitializeEmptyExtensionService();
   extension_sync_service()->MergeDataAndStartSyncing(
       syncer::EXTENSIONS, syncer::SyncDataList(),
@@ -1391,12 +1550,10 @@ TEST_F(ExtensionServiceSyncTest, ProcessSyncDataDeferredEnable) {
 
   SyncChangeList list =
       MakeSyncChangeList(good_crx, specifics, SyncChange::ACTION_UPDATE);
-

[Devlin, 2017/06/09 20:16:35]

not sure how these crept in, will remove the changes on the next upload.

   extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
 
   // Since the version didn't match, the extension should still be disabled.
   EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
-
   // After we update to the matching version, the extension should get enabled.
   path = base_path.AppendASCII("v3");
   PackCRXAndUpdateExtension(id, path, pem_path, ENABLED);