Update extension install prompt to reflect withheld permissions

chrome/browser/extensions/permissions_updater.cc

 // Copyright (c) 2012 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/permissions_updater.h"
 
 #include "base/json/json_writer.h"
 #include "base/memory/ref_counted.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 146 matching lines @@
   if (!Manifest::IsUnpackedLocation(extension->location()) &&
       extension->location() != Manifest::INTERNAL)
     return;
 
   ExtensionPrefs::Get(browser_context_)->AddGrantedPermissions(
       extension->id(),
       extension->permissions_data()->active_permissions().get());
 }
 
 void PermissionsUpdater::InitializePermissions(const Extension* extension) {
-  scoped_refptr<const PermissionSet> active_permissions =
-      ExtensionPrefs::Get(browser_context_)
-          ->GetActivePermissions(extension->id());
-  scoped_refptr<const PermissionSet> bounded_active =
-      GetBoundedActivePermissions(extension, active_permissions);
+  InitializePermissions(extension, INIT_FLAG_NONE);
+}
+
+void PermissionsUpdater::InitializePermissions(const Extension* extension,
+                                               InitFlag init_flag) {
+  scoped_refptr<const PermissionSet> active_permissions(NULL);
+  scoped_refptr<const PermissionSet> bounded_active(NULL);
+  // If |extension| is a transient dummy extension, we do not want to look for
+  // it in preferences.
+  if (init_flag & INIT_FLAG_TRANSIENT) {
+    bounded_active = active_permissions =
+        extension->permissions_data()->active_permissions();
+  } else {
+    active_permissions = ExtensionPrefs::Get(browser_context_)
+                             ->GetActivePermissions(extension->id());
+    bounded_active = GetBoundedActivePermissions(extension, active_permissions);
+  }
 
   // Withhold permissions only if the switch applies to this extension and the
   // extension doesn't have the preference to allow scripting on all urls.
   bool should_withhold_permissions =
       util::ScriptsMayRequireActionForExtension(extension) &&
       !util::AllowedScriptingOnAllUrls(extension->id(), browser_context_);
 
   URLPatternSet granted_explicit_hosts;
   URLPatternSet withheld_explicit_hosts;
   SegregateUrlPermissions(bounded_active->explicit_hosts(),
@@ skipping 97 matching lines @@
   // TODO(rdevlin.cronin) We should notify the observers/renderer.
 }
 
 void PermissionsUpdater::SetPermissions(
     const Extension* extension,
     const scoped_refptr<const PermissionSet>& active,
     scoped_refptr<const PermissionSet> withheld) {
   withheld = withheld.get() ? withheld
                  : extension->permissions_data()->withheld_permissions();
   extension->permissions_data()->SetPermissions(active, withheld);
   ExtensionPrefs::Get(browser_context_)->SetActivePermissions(

[Devlin, 2014/09/08 16:09:22]

I'm worried about the fact that SetPermissions (which is called from
InitializePermissions()) sets them in the prefs.  I think we should pull out the
inner logic of InitializePermissions (~185 - 228) into a separate (maybe
static?) method, which we then use a) in InitializePermissions and b) when we
want to initialize transiently.

[gpdavis, 2014/09/08 20:07:05]

If we don't make the SetPermissions call for a transient extension, the
withholding won't actually happen, and the install prompt won't reflect withheld
permissions.  We already addressed looking in prefs above (using the init flag),
so maybe we could pass that on to SetPermissions? Or how about adding
SetPermissionsWithoutPrefs, calling it conditionally based on the init flag in
InitializePermissions?

[Devlin, 2014/09/09 15:53:21]

Right, so I would suggest either
a) make two methods, one of which is static and does all the work to initialize
permissions but doesn't save them in prefs and one which calls the first and
then saves them, or
b) pass the init flag into the PermissionsUpdater and check it in
SetPermissions().

I think I have a slight preference towards a), because it ensures that any time
we try to do something like AddPermission(), we must use a real
PermissionsUpdater, and if we want to initialize for install, it's guaranteed to
be a one-off.

[gpdavis, 2014/09/09 17:32:41]

How does my implementation in patch set 12 look?  I figure it makes the most
sense to keep the init flag logic in InitializePermissions.  That way it's
obvious what the flag does, and it doesn't become tangled in a bunch of other
code.  Is there any particular reason why you'd prefer one of these two
suggestions over the way I did it there?

[Devlin, 2014/09/09 17:46:22]

I don't feel super-strongly either way, but my biggest concern is just that each
time you add an "if/else" based on a passed in condition to a function, it gets
harder to read, harder to determine which case we want, harder to maintain,
harder to test, more fragile, etc.  It's better if a function like this could
say "Here's what I do to the input permissions" and then callers determine which
permissions to input and what to do with the output.  It makes the distinction
clearer and more deliberate.

That said, it's not terribly complicated, so, assuming it doesn't get much
worse, it'll probably be fine.  But who knows what else we'll need to add to it
at some point?

       extension->id(), active.get());
 }
 
 void PermissionsUpdater::DispatchEvent(
     const std::string& extension_id,
     const char* event_name,
     const PermissionSet* changed_permissions) {
   EventRouter* event_router = EventRouter::Get(browser_context_);
   if (!event_router)
     return;
@@ skipping 50 matching lines @@
             Profile::FromBrowserContext(host->GetBrowserContext()))) {
       host->Send(new ExtensionMsg_UpdatePermissions(params));
     }
   }
 
   // Trigger the onAdded and onRemoved events in the extension.
   DispatchEvent(extension->id(), event_name, changed);
 }
 
 }  // namespace extensions