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

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 16c3a6b5a46fcef99b55f4de0f248a4767fc4121..389220cc36f36a752f0c9e5d00d9b85dcee7b1c5 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1679,8 +1679,12 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
 #if defined(ENABLE_SUPERVISED_USERS)
     // If a custodian-installed extension is disabled for a supervised user due
     // to a permissions increase, send a request to the custodian, since the
-    // supervised user themselves can't re-enable the extension.
-    if (extensions::util::IsExtensionSupervised(extension, profile_)) {
+    // supervised user themselves can't re-enable the extension. If it's already
+    // disabled due to a permissions increase though, there's no need to send
+    // another request.
+    if (extensions::util::IsExtensionSupervised(extension, profile_) &&
+        !extension_prefs_->HasDisableReason(
+            extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) {

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

This is really unrelated; I just discovered the duplicate requests in the new
test.

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

Should the check be on line 1672-3 instead?

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

Hmm.. maybe? Lines 1673 and 1677 are irrelevant in this case (it already has the
disable reason, and so is necessarily also disabled already), but I don't know
about the "AutoDisable" histogram. What is that even supposed to mean?

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

Ohh right. I think at this point the prefs contain the old status of the
extension, so the AutoDisable thing means that if the extension isn't already
disabled, we're about to, and that is reported. Perfectly normal.

So I think the code as you have it is fine, however, I'd change it to use
DidExtensionEscalatePermissions instead of HasDisableReason, for consistency.

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

Hm. DidExtensionEscalatePermissions checks for DISABLE_PERMISSIONS_INCREASE |
DISABLE_REMOTE_INSTALL. Now DISABLE_REMOTE_INSTALL shouldn't ever be set for a
supervised user, but still...

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

From purely an interface perspective - without looking at the code or figuring
out its implications - it seems like calling DidExtensionEscalatePermissions is
the right thing to do? From an implementation perspective, what *will* (or
should) happen if DISABLE_REMOTE_INSTALL is set for a supervised user?

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

Hm. Right now, if the extension doesn't come from the custodian
(installed_by_custodian flag), then it'll be blocked anyway, so I guess it
doesn't matter. So, done.

       SupervisedUserService* supervised_user_service =
           SupervisedUserServiceFactory::GetForProfile(profile_);
       supervised_user_service->AddExtensionUpdateRequest(
@@ -1689,11 +1693,8 @@ void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
     }
 #endif
   }
-  if (disable_reasons != Extension::DISABLE_NONE) {
-    extension_prefs_->AddDisableReason(
-        extension->id(),
-        static_cast<Extension::DisableReason>(disable_reasons));
-  }
+  if (disable_reasons != Extension::DISABLE_NONE)
+    extension_prefs_->AddDisableReasons(extension->id(), disable_reasons);
 }
 
 void ExtensionService::UpdateActiveExtensionsInCrashReporter() {