Replace the WAS_INSTALLED_BY_CUSTODIAN creation flag with a pref

chrome/browser/extensions/extension_sync_service.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_sync_service.h"
 
 #include <utility>
 
 #include "base/auto_reset.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 16 matching lines @@
 #include "extensions/browser/uninstall_reason.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/image_util.h"
 #include "extensions/common/permissions/permission_message_provider.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
 
 #if defined(ENABLE_SUPERVISED_USERS)
+#include "chrome/browser/extensions/extension_util.h"

[Marc Treib, 2016/06/17 08:56:54]

This needs to go out of the #ifdef

[mamir, 2016/06/17 16:12:54]

Done.

 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #endif
 
 using extensions::AppSorting;
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSet;
 using extensions::ExtensionSyncData;
@@ skipping 226 matching lines @@
   // for older Chrome versions (<M48).
   bool enabled = (disable_reasons == Extension::DISABLE_NONE);
   enabled = enabled &&
       extension_prefs->GetExtensionBlacklistState(extension.id()) ==
           extensions::NOT_BLACKLISTED;
   bool incognito_enabled = extensions::util::IsIncognitoEnabled(id, profile_);
   bool remote_install =
       extension_prefs->HasDisableReason(id, Extension::DISABLE_REMOTE_INSTALL);
   ExtensionSyncData::OptionalBoolean allowed_on_all_url =
       GetAllowedOnAllUrlsOptionalBoolean(id, 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);
+            remote_install, allowed_on_all_url, installed_by_custodian);
 
   // If there's a pending update, send the new version to sync instead of the
   // installed one.
   auto it = pending_updates_.find(id);
   if (it != pending_updates_.end()) {
     const base::Version& version = it->second.version;
     // If we have a pending version, it should be newer than the installed one.
     DCHECK_EQ(-1, extension.version()->CompareTo(version));
     result.set_version(version);
     // If we'll re-enable the extension once it's updated, also send that back
     // to sync.
     if (it->second.grant_permissions_and_reenable)
       result.set_enabled(true);
   }
   return result;
 }
 
 void ExtensionSyncService::ApplySyncData(
     const ExtensionSyncData& extension_sync_data) {
   // Ignore any pref change notifications etc. while we're applying incoming
   // sync data, so that we don't end up notifying ourselves.
   base::AutoReset<bool> ignore_updates(&ignore_updates_, true);
 
+  if (extension_sync_data.installed_by_custodian()) {

[Marc Treib, 2016/06/17 08:56:54]

Hm. Now that I see this, I kinda think we should trust the data coming from
Sync, and clear the pref if necessary. Otherwise, it's impossible to ever remove
the flag from Sync, because the Chrome client will always add it back.

Consider:
- Extension is custodian-installed.
- Custodian uninstalls it.
- Another machine gets the uninstall.
- On that other machine, SU initiates an install of the same extension.
- Back on this machine, we get a Sync update saying that it's now not installed
by custodian anymore, but Chrome ignores that, and adds the flag back to Sync.

So, would it break anything to clear the pref here if Sync says so?

[mamir, 2016/06/17 16:12:55]

I agree.
No thing should break because this flag is always set to true on the server not
the client.
Done!

+    extensions::util::SetWasInstalledByCustodian(extension_sync_data.id(),
+                                                 profile_);
+  }
+
   syncer::ModelType type = extension_sync_data.is_app() ? syncer::APPS
                                                         : syncer::EXTENSIONS;
   const std::string& id = extension_sync_data.id();
   SyncBundle* bundle = GetSyncBundle(type);
   DCHECK(bundle->IsSyncing());
   // Note: |extension| may be null if it hasn't been installed yet.
   const Extension* extension =
       ExtensionRegistry::Get(profile_)->GetInstalledExtension(id);
   if (extension && !IsCorrectSyncType(*extension, type)) {
     // The installed item isn't the same type as the sync data item, so we need
@@ skipping 168 matching lines @@
     // If the extension is installed but outdated, store the new version.
     pending_updates_[id] =
         PendingUpdate(extension_sync_data.version(), reenable_after_update);
     check_for_updates = true;
   } else if (state == NOT_INSTALLED) {
     if (!extension_service()->pending_extension_manager()->AddFromSync(
             id,
             extension_sync_data.update_url(),
             extension_sync_data.version(),
             ShouldAllowInstall,
-            extension_sync_data.remote_install(),
-            extension_sync_data.installed_by_custodian())) {
+            extension_sync_data.remote_install())) {
       LOG(WARNING) << "Could not add pending extension for " << id;
       // This means that the extension is already pending installation, with a
       // non-INTERNAL location.  Add to pending_sync_data, even though it will
       // never be removed (we'll never install a syncable version of the
       // extension), so that GetAllSyncData() continues to send it.
     }
     // Track pending extensions so that we can return them in GetAllSyncData().
     bundle->AddPendingExtensionData(extension_sync_data);
     check_for_updates = true;
   }
@@ skipping 185 matching lines @@
       sync_data_list->push_back(CreateSyncData(*extension));
     }
   }
 }
 
 bool ExtensionSyncService::ShouldSync(const Extension& extension) const {
   // Themes are handled by the ThemeSyncableService.
   return extensions::util::ShouldSync(&extension, profile_) &&
          !extension.is_theme();
 }