Apps&Extensions for Supervised Users: send permission request on outdated re-enables

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 <iterator>
 
 #include "base/basictypes.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 21 matching lines @@
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_icon_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/image_util.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/icons_handler.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
 #include "ui/gfx/image/image_family.h"
 
+#if defined(ENABLE_SUPERVISED_USERS)
+#include "chrome/browser/supervised_user/supervised_user_service.h"
+#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#endif
+
 using extensions::AppSyncData;
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionRegistry;
 using extensions::ExtensionSyncData;
 using extensions::FeatureSwitch;
 
 namespace {
 
 void OnWebApplicationInfoLoaded(
@@ skipping 22 matching lines @@
     return ExtensionSyncData::BOOLEAN_FALSE;
 
   // If the user has explicitly set a value, then we sync it.
   if (extensions::util::HasSetAllowedScriptingOnAllUrls(extension_id, context))
     return ExtensionSyncData::BOOLEAN_TRUE;
 
   // Otherwise, unset.
   return ExtensionSyncData::BOOLEAN_UNSET;
 }
 
+#if defined(ENABLE_SUPERVISED_USERS)
+// Callback for SupervisedUserService::AddExtensionUpdateRequest.
+void ExtensionUpdateRequestSent(const std::string& id, bool success) {
+  LOG_IF(WARNING, !success) << "Failed sending update request for " << id;
+}
+#endif
+
 }  // namespace
 
 ExtensionSyncService::ExtensionSyncService(Profile* profile,
                                            ExtensionPrefs* extension_prefs,
                                            ExtensionService* extension_service)
     : profile_(profile),
       extension_prefs_(extension_prefs),
       extension_service_(extension_service),
       app_sync_bundle_(this),
       extension_sync_bundle_(this),
@@ skipping 378 matching lines @@
 bool ExtensionSyncService::IsPendingEnable(
     const std::string& extension_id) const {
   return pending_app_enables_.Contains(extension_id) ||
       pending_extension_enables_.Contains(extension_id);
 }
 
 bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
     const ExtensionSyncData& extension_sync_data,
     syncer::ModelType type) {
   const std::string& id = extension_sync_data.id();
   const Extension* extension = extension_service_->GetInstalledExtension(id);

[not at google - send to devlin, 2015/06/22 21:03:10]

Can we save a reference to ExtensionRegistry (say "registry") up here, use
registry->GetInstalledExtension(id) instead of this, and use
registry->enabled_extensions()->Contains(id) below? Just a tiny bit cleaner.

[Marc Treib, 2015/06/23 10:22:25]

Sure, done.

 
   // TODO(bolms): we should really handle this better.  The particularly bad
   // case is where an app becomes an extension or vice versa, and we end up with
   // a zombie extension that won't go away.
   if (extension && !IsCorrectSyncType(*extension, type))
     return true;
 
   // Handle uninstalls first.
   if (extension_sync_data.uninstalled()) {
     if (!extension_service_->UninstallExtensionHelper(
             extension_service_, id, extensions::UNINSTALL_REASON_SYNC)) {
       LOG(WARNING) << "Could not uninstall extension " << id << " for sync";
     }
     return true;
   }
 
   // Extension from sync was uninstalled by the user as external extensions.
   // Honor user choice and skip installation/enabling.
   if (extensions::ExtensionPrefs::Get(profile_)
           ->IsExternalExtensionUninstalled(id)) {
     LOG(WARNING) << "Extension with id " << id
                  << " from sync was uninstalled as external extension";
     return true;
   }
 
+  int version_compare_result = extension ?
+      extension->version()->CompareTo(extension_sync_data.version()) : 0;
+
   // Set user settings.
   if (extension_sync_data.enabled()) {
+    DCHECK(!extension_sync_data.disable_reasons());
+
+#if defined(ENABLE_SUPERVISED_USERS)
+    if (extension &&
+        extensions::util::IsExtensionSupervised(extension, profile_) &&
+        !ExtensionRegistry::Get(profile_)->enabled_extensions().Contains(id)) {
+      if (version_compare_result < 0) {
+        // The installed version is outdated. Defer applying the change until
+        // it's up-to-date.
+        return false;

[Marc Treib, 2015/06/22 15:35:18]

Actually, I think the same behavior should apply to all users, not just
supervised ones: Defer the re-enabling until we're on the new version, since the
old version might have permissions that the user never approved. WDYT?
(Note that there already is an "is this outdated" check below, but it happens
after everything has been applied.)

[not at google - send to devlin, 2015/06/22 21:03:10]

I agree this isn't necessarily supervised-user specific, and it would be good to
separate that out.

However I guess it could never happen outside supervised users, because the sync
change to push the update to the extension could only ever come before a sync
change to grant it permissions? Now... ok, you're going to need to refresh my
memory. How are supervised users different?

[Marc Treib, 2015/06/23 10:22:25]

But the extension update could have arrived first, and then a Sync update from
another client that hasn't seen the new version yet, right? Not very likely, but
still.

Supervised users are different in that the Sync change to re-enable does not
come from another Chrome instance, but instead comes from their custodian, via a
web dashboard that calls a Sync RPC. That generates a Sync change that sets the
"enabled" bit to true, and the version to whatever the custodian saw and
approved. We can't guarantee that what the custodian saw is actually what we
have installed here, so we need to check that the versions match.

[not at google - send to devlin, 2015/06/24 00:25:11]

Ok, right. I think it only makes sense for supervised users. In the normal case
there is no way we could be getting a sync change to enable an extension before
it's actually been installed at that version. Could you explain that?

[Marc Treib, 2015/06/24 11:50:57]

I still think the same thing can happen for regular users in some (unlikely)
circumstances. Say another device has updated to the new version and sends Sync
updates. On this device, the web store is unreachable/slow, so we're still on
the old version.

[not at google - send to devlin, 2015/06/24 20:47:38]

Nit: it's omaha not the webstore.

Also the situation you describe is pretty common, sync is faster than omaha, but
perhaps I'm misinterpreting you. Or confused you :-)

But... ok, I think given that sync coalesces updates with the same key (in this
case the key is the extension ID I think), an update event could get squashed by
an enabled event. In fact in that case shouldn't we be updating the extension
then? ARGH.

+      }
+      if (version_compare_result > 0) {
+        // The installed version is newer than what Sync tells us to re-enable.
+        // This means another update has happened since the custodian approved
+        // the re-enable. We can't be sure if there are new permissions that the
+        // custodian hasn't seen, so ignore the re-enable. Instead send another
+        // re-enable request to the custodian.

[not at google - send to devlin, 2015/06/24 00:25:10]

Is this actually necessary? I would have expected there to be code elsewhere,
somewhere, that sees an update and makes an update request.

[Marc Treib, 2015/06/24 11:50:57]

Hm, I'd need to remove the "don't send duplicate permission requests" check that
I just added in extension_service.cc again. Then we would not need to send
requests here, at the cost of some extraneous requests. But those shouldn't
hurt, so maybe that's the better option overall.

[not at google - send to devlin, 2015/06/24 20:47:38]

Mm I don't understand why you'd need to remove that check to have this work, but
now that I read this code again, I think what you're written isn't wrong: drop
the change if it's for an older version.

I think it can happen outside supervised users though - just like you said, more
common because there are 2 users involved not 1. But I think you could simplify
this (insofar as pulling out of supervised users) but basically saying: if any
sync change happens for an older version, we should drop it. That prevents
rolling back extensions but wow if that ever needs to happen we're in serious
trouble all over the place.

[Marc Treib, 2015/06/29 09:52:49]

Wait, rolling back extensions isn't possible anyway, right? Omaha only ever
gives us the latest version AFAIK..?

[not at google - send to devlin, 2015/06/29 22:59:04]

Yes, I just meant the case when an update from sync comes for an older version.
Obviously Chrome can't do much with it because Omaha only keeps the latest
version - and nor should it anyway, because there is a never version.

My point was just that rather than trying to write any special code at all, we
should simply have a check "if (sync_change.version < current_version) return"
or the equivalent.

or does this code need to be special to handle the cross-managed-and-supervised
user flow?

[Marc Treib, 2015/06/30 08:32:37]

I'm totally happy to do this for all users, and have one less "if(supervised)"
:)
For SUs, we still need to send a permission request *somewhere*, and I'm not
sure yet if there's a better place for that than here. But having special code
for that is still way better than changing the sync behavior for SUs.

+        SupervisedUserService* supervised_user_service =
+            SupervisedUserServiceFactory::GetForProfile(profile_);
+        supervised_user_service->AddExtensionUpdateRequest(
+            id, *extension->version(),
+            base::Bind(ExtensionUpdateRequestSent, id));
+        return true;
+      }
+    }
+#endif
     // Only grant permissions if the sync data explicitly sets the disable
     // reasons to Extension::DISABLE_NONE (as opposed to the legacy (<M45) case
     // where they're not set at all), and if the version from sync matches our
     // local one. Otherwise we just enable it without granting permissions. If
     // any permissions are missing, CheckPermissionsIncrease will soon disable
     // it again.
-    DCHECK(!extension_sync_data.disable_reasons());
     bool grant_permissions =
         extension_sync_data.supports_disable_reasons() &&
-        extension &&
-        extension->version()->Equals(extension_sync_data.version());
+        extension && (version_compare_result == 0);
     if (grant_permissions)
       extension_service_->GrantPermissionsAndEnableExtension(extension);
     else
       extension_service_->EnableExtension(id);
   } else if (!IsPendingEnable(id)) {
     int disable_reasons = extension_sync_data.disable_reasons();
     if (extension_sync_data.remote_install()) {
       if (!(disable_reasons & Extension::DISABLE_REMOTE_INSTALL)) {
         // In the non-legacy case (>=M45) where disable reasons are synced at
         // all, DISABLE_REMOTE_INSTALL should be among them already.
         DCHECK(!extension_sync_data.supports_disable_reasons());
         disable_reasons |= Extension::DISABLE_REMOTE_INSTALL;
       }
     } else if (!extension_sync_data.supports_disable_reasons()) {
       // Legacy case (<M45), from before we synced disable reasons (see
       // crbug.com/484214).
       disable_reasons = Extension::DISABLE_UNKNOWN_FROM_SYNC;
     }
 
     // In the non-legacy case (>=M45), clear any existing disable reasons first.
     // Otherwise sync can't remove just some of them.
     if (extension_sync_data.supports_disable_reasons())
       extensions::ExtensionPrefs::Get(profile_)->ClearDisableReasons(id);
 
     extension_service_->DisableExtension(id, disable_reasons);
   }
 
-  // We need to cache some version information here because setting the
-  // incognito flag invalidates the |extension| pointer (it reloads the
-  // extension).
+  // We need to cache some information here because setting the incognito flag
+  // invalidates the |extension| pointer (it reloads the extension).
   bool extension_installed = (extension != NULL);
-  int version_compare_result = extension ?
-      extension->version()->CompareTo(extension_sync_data.version()) : 0;
 
   // If the target extension has already been installed ephemerally, it can
   // be promoted to a regular installed extension and downloading from the Web
   // Store is not necessary.
   if (extension && extensions::util::IsEphemeralApp(id, profile_))
     extension_service_->PromoteEphemeralApp(extension, true);
 
   // Update the incognito flag.
   extensions::util::SetIsIncognitoEnabled(
       id, profile_, extension_sync_data.incognito_enabled());
   extension = NULL;  // No longer safe to use.
 
   // Update the all urls flag.
   if (extension_sync_data.all_urls_enabled() !=
           ExtensionSyncData::BOOLEAN_UNSET) {
     bool allowed = extension_sync_data.all_urls_enabled() ==
         ExtensionSyncData::BOOLEAN_TRUE;
     extensions::util::SetAllowedScriptingOnAllUrls(id, profile_, allowed);
   }
 
   if (extension_installed) {
     // If the extension is already installed, check if it's outdated.
     if (version_compare_result < 0) {
       // Extension is outdated.
       return false;

[Marc Treib, 2015/06/22 15:35:18]

This is a bit weird: Returning "false" here causes the ExtensionSyncData to be
stored in pending_sync_data_ (in App/ExtensionSyncBundle), but before this CL,
it would never be used again if Sync is already running. I guess things worked
only because all the changes have been applied already at this point?

[not at google - send to devlin, 2015/06/22 21:03:10]

Bleh. This is too complicated.

[Marc Treib, 2015/06/23 10:22:25]

No argument here...

     }
   } else {
     CHECK(type == syncer::EXTENSIONS || type == syncer::APPS);
     extensions::PendingExtensionInfo::ShouldAllowInstallPredicate filter =
         (type == syncer::APPS) ? extensions::sync_helper::IsSyncableApp :
                                  extensions::sync_helper::IsSyncableExtension;
 
     if (!extension_service_->pending_extension_manager()->AddFromSync(
             id,
             extension_sync_data.update_url(),
@@ skipping 20 matching lines @@
       app_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::APPS);
   } else if (extensions::util::ShouldSyncExtension(&extension, profile_)) {
     if (extension_sync_bundle_.IsSyncing())
       extension_sync_bundle_.SyncChangeIfNeeded(extension);
     else if (extension_service_->is_ready() && !flare_.is_null())
       flare_.Run(syncer::EXTENSIONS);
   }
 }