Skip to content

Commit

Permalink
[Extensions] Start making chrome://extensions use extensions APIs
Browse files Browse the repository at this point in the history
Allow chrome://extensions to use the developerPrivate and
management apis, and make it do so for enabling/disabling
extensions.
Also, shave yaks:
- Make management api check requirements for extensions.
- Make feature provider aware of complex parents.
And bonus:
- Remove WeakPtr interface from RequirementsChecker

BUG=461039

Review URL: https://codereview.chromium.org/951633002

Cr-Commit-Position: refs/heads/master@{#317871}
  • Loading branch information
rdcronin authored and Commit bot committed Feb 24, 2015
1 parent 1d7ceee commit 69bf753
Show file tree
Hide file tree
Showing 26 changed files with 353 additions and 185 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "chrome/browser/extensions/api/developer_private/entry_picker.h"
#include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
#include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
#include "chrome/browser/extensions/chrome_requirements_checker.h"
#include "chrome/browser/extensions/devtools_util.h"
#include "chrome/browser/extensions/extension_disabled_ui.h"
#include "chrome/browser/extensions/extension_error_reporter.h"
Expand Down Expand Up @@ -802,7 +803,7 @@ bool DeveloperPrivateEnableFunction::RunSync() {
scoped_ptr<Enable::Params> params(Enable::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());

std::string extension_id = params->item_id;
const std::string& extension_id = params->item_id;

const Extension* extension =
ExtensionRegistry::Get(GetProfile())->GetExtensionById(
Expand All @@ -814,57 +815,49 @@ bool DeveloperPrivateEnableFunction::RunSync() {
ExtensionSystem* system = ExtensionSystem::Get(GetProfile());
ManagementPolicy* policy = system->management_policy();
bool enable = params->enable;
if (!policy->UserMayModifySettings(extension, NULL) ||
(!enable && policy->MustRemainEnabled(extension, NULL)) ||
(enable && policy->MustRemainDisabled(extension, NULL, NULL))) {
if (!policy->UserMayModifySettings(extension, nullptr) ||
(!enable && policy->MustRemainEnabled(extension, nullptr)) ||
(enable && policy->MustRemainDisabled(extension, nullptr, nullptr))) {
LOG(ERROR) << "Attempt to change enable state denied by management policy. "
<< "Extension id: " << extension_id.c_str();
<< "Extension id: " << extension_id;
return false;
}

ExtensionService* service = system->extension_service();
if (enable) {
ExtensionPrefs* prefs = ExtensionPrefs::Get(GetProfile());
if (prefs->DidExtensionEscalatePermissions(extension_id)) {
AppWindowRegistry* registry = AppWindowRegistry::Get(GetProfile());
CHECK(registry);
AppWindow* app_window =
registry->GetAppWindowForRenderViewHost(render_view_host());
if (!app_window) {
// If the extension escalated permissions, we have to show a dialog.
content::WebContents* web_contents = render_view_host() ?
content::WebContents::FromRenderViewHost(render_view_host()) :
nullptr;
if (!web_contents)
return false;
}

ShowExtensionDisabledDialog(
service, app_window->web_contents(), extension);
ShowExtensionDisabledDialog(service, web_contents, extension);
} else if ((prefs->GetDisableReasons(extension_id) &
Extension::DISABLE_UNSUPPORTED_REQUIREMENT) &&
!requirements_checker_.get()) {
Extension::DISABLE_UNSUPPORTED_REQUIREMENT)) {
// Recheck the requirements.
scoped_refptr<const Extension> extension =
service->GetExtensionById(extension_id, true);
requirements_checker_.reset(new RequirementsChecker);
// Released by OnRequirementsChecked.
AddRef();
requirements_checker_.reset(new ChromeRequirementsChecker());
AddRef(); // Released in OnRequirementsChecked.
// TODO(devlin): Uh... asynchronous code in a sync extension function?
requirements_checker_->Check(
extension,
make_scoped_refptr(extension),
base::Bind(&DeveloperPrivateEnableFunction::OnRequirementsChecked,
this, extension_id));
} else {
// Otherwise, we're good to re-enable the extension.
service->EnableExtension(extension_id);

// Make sure any browser action contained within it is not hidden.
ExtensionActionAPI::SetBrowserActionVisibility(
prefs, extension->id(), true);
}
} else {
} else { // !enable (i.e., disable)
service->DisableExtension(extension_id, Extension::DISABLE_USER_ACTION);
}
return true;
}

void DeveloperPrivateEnableFunction::OnRequirementsChecked(
const std::string& extension_id,
std::vector<std::string> requirements_errors) {
const std::vector<std::string>& requirements_errors) {
if (requirements_errors.empty()) {
GetExtensionService(GetProfile())->EnableExtension(extension_id);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "chrome/browser/extensions/extension_install_prompt.h"
#include "chrome/browser/extensions/extension_uninstall_dialog.h"
#include "chrome/browser/extensions/pack_extension_job.h"
#include "chrome/browser/extensions/requirements_checker.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/render_view_host.h"
Expand All @@ -35,6 +34,7 @@ class ExtensionError;
class ExtensionRegistry;
class ExtensionSystem;
class ManagementPolicy;
class RequirementsChecker;

namespace api {

Expand Down Expand Up @@ -300,8 +300,9 @@ class DeveloperPrivateEnableFunction
~DeveloperPrivateEnableFunction() override;

// Callback for requirements checker.
void OnRequirementsChecked(const std::string& extension_id,
std::vector<std::string> requirements_errors);
void OnRequirementsChecked(
const std::string& extension_id,
const std::vector<std::string>& requirements_errors);
// ExtensionFunction:
bool RunSync() override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/extensions/bookmark_app_helper.h"
#include "chrome/browser/extensions/chrome_extension_function_details.h"
#include "chrome/browser/extensions/chrome_requirements_checker.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_util.h"
#include "chrome/browser/extensions/launch_util.h"
Expand Down Expand Up @@ -207,6 +208,11 @@ ChromeManagementAPIDelegate::SetEnabledFunctionDelegate(
extension));
}

scoped_ptr<extensions::RequirementsChecker>
ChromeManagementAPIDelegate::CreateRequirementsChecker() const {
return make_scoped_ptr(new extensions::ChromeRequirementsChecker());
}

scoped_ptr<extensions::UninstallDialogDelegate>
ChromeManagementAPIDelegate::UninstallFunctionDelegate(
extensions::ManagementUninstallFunctionBase* function,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class ChromeManagementAPIDelegate : public extensions::ManagementAPIDelegate {
scoped_ptr<extensions::InstallPromptDelegate> SetEnabledFunctionDelegate(
extensions::ManagementSetEnabledFunction* function,
const extensions::Extension* extension) const override;
scoped_ptr<extensions::RequirementsChecker> CreateRequirementsChecker()
const override;
scoped_ptr<extensions::UninstallDialogDelegate> UninstallFunctionDelegate(
extensions::ManagementUninstallFunctionBase* function,
const std::string& target_extension_id) const override;
Expand Down
140 changes: 140 additions & 0 deletions chrome/browser/extensions/api/management/management_api_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// Copyright 2015 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_function_test_utils.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_service_test_base.h"
#include "chrome/browser/extensions/test_extension_system.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/host_desktop.h"
#include "chrome/test/base/test_browser_window.h"
#include "extensions/browser/api/management/management_api.h"
#include "extensions/browser/api/management/management_api_constants.h"
#include "extensions/browser/extension_prefs.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/management_policy.h"
#include "extensions/browser/test_management_policy.h"
#include "extensions/common/error_utils.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_set.h"
#include "extensions/common/test_util.h"

namespace extensions {

namespace {

KeyedService* BuildManagementApi(content::BrowserContext* context) {
return new ManagementAPI(context);
}

} // namespace

namespace constants = extension_management_api_constants;

// TODO(devlin): Unittests are awesome. Test more with unittests and less with
// heavy api/browser tests.
class ManagementApiUnitTest : public ExtensionServiceTestBase {
protected:
ManagementApiUnitTest() {}
~ManagementApiUnitTest() override {}

// A wrapper around extension_function_test_utils::RunFunction that runs with
// the associated browser, no flags, and can take stack-allocated arguments.
bool RunFunction(const scoped_refptr<UIThreadExtensionFunction>& function,
const base::ListValue& args);

Browser* browser() { return browser_.get(); }

private:
// ExtensionServiceTestBase:
void SetUp() override;
void TearDown() override;

// The browser (and accompanying window).
scoped_ptr<TestBrowserWindow> browser_window_;
scoped_ptr<Browser> browser_;

DISALLOW_COPY_AND_ASSIGN(ManagementApiUnitTest);
};

bool ManagementApiUnitTest::RunFunction(
const scoped_refptr<UIThreadExtensionFunction>& function,
const base::ListValue& args) {
return extension_function_test_utils::RunFunction(
function.get(),
make_scoped_ptr(args.DeepCopy()),
browser(),
extension_function_test_utils::NONE);
}

void ManagementApiUnitTest::SetUp() {
ExtensionServiceTestBase::SetUp();
InitializeEmptyExtensionService();
ManagementAPI::GetFactoryInstance()->SetTestingFactory(profile(),
&BuildManagementApi);
static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile()))->
SetEventRouter(make_scoped_ptr(
new EventRouter(profile(), ExtensionPrefs::Get(profile()))));

browser_window_.reset(new TestBrowserWindow());
Browser::CreateParams params(profile(), chrome::HOST_DESKTOP_TYPE_NATIVE);
params.type = Browser::TYPE_TABBED;
params.window = browser_window_.get();
browser_.reset(new Browser(params));
}

void ManagementApiUnitTest::TearDown() {
browser_.reset();
browser_window_.reset();
ExtensionServiceTestBase::TearDown();
}

// Test the basic parts of management.setEnabled.
TEST_F(ManagementApiUnitTest, ManagementSetEnabled) {
scoped_refptr<const Extension> extension = test_util::CreateEmptyExtension();
service()->AddExtension(extension.get());
std::string extension_id = extension->id();
scoped_refptr<ManagementSetEnabledFunction> function(
new ManagementSetEnabledFunction());

base::ListValue disable_args;
disable_args.AppendString(extension_id);
disable_args.AppendBoolean(false);

// Test disabling an (enabled) extension.
EXPECT_TRUE(registry()->enabled_extensions().Contains(extension_id));
EXPECT_TRUE(RunFunction(function, disable_args));
EXPECT_TRUE(registry()->disabled_extensions().Contains(extension_id));

base::ListValue enable_args;
enable_args.AppendString(extension_id);
enable_args.AppendBoolean(true);

// Test re-enabling it.
function = new ManagementSetEnabledFunction();
EXPECT_TRUE(RunFunction(function, enable_args));
EXPECT_TRUE(registry()->enabled_extensions().Contains(extension_id));

// Test that the enable function checks management policy, so that we can't
// disable an extension that is required.
TestManagementPolicyProvider provider(
TestManagementPolicyProvider::PROHIBIT_MODIFY_STATUS);
ManagementPolicy* policy =
ExtensionSystem::Get(profile())->management_policy();
policy->RegisterProvider(&provider);

function = new ManagementSetEnabledFunction();
EXPECT_FALSE(RunFunction(function, disable_args));
EXPECT_EQ(ErrorUtils::FormatErrorMessage(constants::kUserCantModifyError,
extension_id),
function->GetError());
policy->UnregisterProvider(&provider);

// TODO(devlin): We should also test enabling an extenion that has escalated
// permissions, but that needs a web contents (which is a bit of a pain in a
// unit test).
}

} // namespace extensions
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/extensions/requirements_checker.h"
#include "chrome/browser/extensions/chrome_requirements_checker.h"

#include "base/bind.h"
#include "chrome/browser/gpu/gpu_feature_checker.h"
Expand All @@ -20,15 +20,16 @@

namespace extensions {

RequirementsChecker::RequirementsChecker()
: pending_requirement_checks_(0) {
ChromeRequirementsChecker::ChromeRequirementsChecker()
: pending_requirement_checks_(0), weak_ptr_factory_(this) {
}

RequirementsChecker::~RequirementsChecker() {
ChromeRequirementsChecker::~ChromeRequirementsChecker() {
}

void RequirementsChecker::Check(scoped_refptr<const Extension> extension,
base::Callback<void(std::vector<std::string> errors)> callback) {
void ChromeRequirementsChecker::Check(
const scoped_refptr<const Extension>& extension,
const RequirementsCheckedCallback& callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

callback_ = callback;
Expand Down Expand Up @@ -58,9 +59,9 @@ void RequirementsChecker::Check(scoped_refptr<const Extension> extension,
if (requirements.webgl) {
++pending_requirement_checks_;
webgl_checker_ = new GPUFeatureChecker(
gpu::GPU_FEATURE_TYPE_WEBGL,
base::Bind(&RequirementsChecker::SetWebGLAvailability,
AsWeakPtr()));
gpu::GPU_FEATURE_TYPE_WEBGL,
base::Bind(&ChromeRequirementsChecker::SetWebGLAvailability,
weak_ptr_factory_.GetWeakPtr()));
}

if (pending_requirement_checks_ == 0) {
Expand All @@ -76,15 +77,15 @@ void RequirementsChecker::Check(scoped_refptr<const Extension> extension,
webgl_checker_->CheckGPUFeatureAvailability();
}

void RequirementsChecker::SetWebGLAvailability(bool available) {
void ChromeRequirementsChecker::SetWebGLAvailability(bool available) {
if (!available) {
errors_.push_back(
l10n_util::GetStringUTF8(IDS_EXTENSION_WEBGL_NOT_SUPPORTED));
}
MaybeRunCallback();
}

void RequirementsChecker::MaybeRunCallback() {
void ChromeRequirementsChecker::MaybeRunCallback() {
if (--pending_requirement_checks_ == 0) {
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
base::Bind(callback_, errors_));
Expand Down
Loading

0 comments on commit 69bf753

Please sign in to comment.