Register granting of previously withheld permissions as a permissions

chrome/browser/extensions/crx_installer.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/crx_installer.h"
 
 #include <map>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 441 matching lines @@
 
   if (original_manifest)
     original_manifest_.reset(new Manifest(
         Manifest::INVALID_LOCATION,
         scoped_ptr<base::DictionaryValue>(original_manifest->DeepCopy())));
 
   // We don't have to delete the unpack dir explicity since it is a child of
   // the temp dir.
   unpacked_extension_root_ = extension_dir;
 
+  if (!BrowserThread::PostTask(
+           BrowserThread::UI,
+           FROM_HERE,
+           base::Bind(&CrxInstaller::InitializeExtensionPermissions, this)))

[Devlin, 2014/09/09 16:04:26]

Ew.  We already bounce around a ton in extension installation, but I'd love to
avoid any extra.  Can we instead move the permissions update to
OnInstallChecksComplete()?  InstallChecker doesn't check anything with
permissions, and, if you're worried, we can put in a comment mentioning that the
permissions in the extension in installchecker might not reflect the total
permissions.

[gpdavis, 2014/09/09 17:48:53]

Well, the goal here is to call InitializePermissions before IsPrivilegeIncrease
is called in AllowInstall, because that's what's catching the permissions
elevation.  This is why I was having threading trouble; the timing is quite
sensitive.  If we move it to OnInstallChecksComplete, we'd have to make another
call to IsPrivilegeIncrease (or move the call) to catch it with the appropriate
permissions, which is certainly possible, but I'm not sure if it would have any
unintended consequences.

I certainly agree that the thread jumping is ugly, though.  It just seemed like
the least impactful change.  What do you think?

[Devlin, 2014/09/09 17:57:44]

Hmm... that makes me wonder if this is actually the right place for this.

The goal is to have this trigger a permissions increase via
ExtensionService::CheckPermissionsIncrease().  This actually looks like all it
will do is not install the extension (and throw an error saying the manifest is
invalid).  That's not quite right, right?

+    NOTREACHED();
+}
+
+void CrxInstaller::InitializeExtensionPermissions() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  PermissionsUpdater(profile()).InitializePermissions(
+      install_checker_.extension().get());
+  if (!installer_task_runner_->PostTask(
+           FROM_HERE,
+           base::Bind(&CrxInstaller::ContinueUnpackSuccess, this)))
+    NOTREACHED();
+}
+
+void CrxInstaller::ContinueUnpackSuccess() {
+  DCHECK(installer_task_runner_->RunsTasksOnCurrentThread());
+  const Extension* extension = install_checker_.extension().get();
   CrxInstallerError error = AllowInstall(extension);
   if (error.type() != CrxInstallerError::ERROR_NONE) {
     ReportFailureFromFileThread(error);
     return;
   }
 
   if (!BrowserThread::PostTask(BrowserThread::UI,
                                FROM_HERE,
                                base::Bind(&CrxInstaller::CheckInstall, this)))
     NOTREACHED();
@@ skipping 418 matching lines @@
   if (!prefs->DidExtensionEscalatePermissions(extension()->id()))
     return;
 
   if (client_) {
     AddRef();  // Balanced in InstallUIProceed() and InstallUIAbort().
     client_->ConfirmReEnable(this, extension());
   }
 }
 
 }  // namespace extensions