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

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 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_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 1656 matching lines @@
   // Extension has changed permissions significantly. Disable it. A
   // notification should be sent by the caller. If the extension is already
   // disabled because it was installed remotely, don't add another disable
   // reason.
   if (is_privilege_increase &&
       disable_reasons != Extension::DISABLE_REMOTE_INSTALL) {
     disable_reasons |= Extension::DISABLE_PERMISSIONS_INCREASE;
     if (!extension_prefs_->DidExtensionEscalatePermissions(extension->id())) {
       RecordPermissionMessagesHistogram(extension, "AutoDisable");
     }
     extension_prefs_->SetExtensionState(extension->id(), Extension::DISABLED);

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

I wonder if this should actually go on line 1697,

if (disable_reasons != Extension::DISABLE_NONE) {
  prefs_->AddDisableReasons();
  prefs_->SetExtensionState();
}

seems odd to be doing very similar work in 2 different places. But, that
basically defines this entire file and all the sync stuff.

We can deal with this later.

 
 #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(
           extension->id(), *extension->version(),
           base::Bind(ExtensionUpdateRequestSent, extension->id()));
     }
 #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() {
   std::set<std::string> extension_ids;
   const ExtensionSet& extensions = registry_->enabled_extensions();
   for (ExtensionSet::const_iterator iter = extensions.begin();
        iter != extensions.end(); ++iter) {
     const Extension* extension = iter->get();
     if (!extension->is_theme() && extension->location() != Manifest::COMPONENT)
       extension_ids.insert(extension->id());
@@ skipping 881 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }