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

chrome/browser/extensions/extension_sync_service.cc

Index: chrome/browser/extensions/extension_sync_service.cc
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 90f679f66343d434af13c17359b6d2c6eb5381db..494af8188d1a7380a9b524c27011979631aa6db5 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -39,6 +39,11 @@
 #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;
@@ -81,6 +86,13 @@ ExtensionSyncData::OptionalBoolean GetAllowedOnAllUrlsOptionalBoolean(
   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,
@@ -505,19 +517,46 @@ bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
     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
@@ -545,12 +584,9 @@ bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
     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