Skip to content

Commit

Permalink
Child users: Ask for parent permission or display error from launcher
Browse files Browse the repository at this point in the history 
Currently, if an app is disabled pending custodian approval, and the
supervised user tries to open it through the launcher, nothing
happens. This CL fixes this behavior by either asking for parent
permission or displaying an error dialog. If the parent has the
"Permissions for sites, apps and extensions" toggle enabled, then
launching the app will ask for parent permission. Otherwise, the user
will see an error dialog.

This CL also refactors some common logic between the ManagementAPI
and ExtensionEnableFlow into the SupervisedUserExtensionsDelegate for
determining which dialog to show when enabling extensions for
supervised users.

Bug: 1079415,1071255
Change-Id: I585238db5e9a9de96849b7a15dda120b13339b68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2238408
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dan S <danan@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Yilkal Abe <yilkal@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785958}
  • Loading branch information
Toby Huang authored and Commit Bot committed Jul 7, 2020
1 parent 2da1460 commit 349247d
Show file tree
Hide file tree
Showing 24 changed files with 603 additions and 408 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/apps/app_service/extension_apps_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class ExtensionAppsEnableFlow : public ExtensionEnableFlowDelegate {

if (!flow_) {
flow_ = std::make_unique<ExtensionEnableFlow>(profile_, app_id_, this);
flow_->StartForNativeWindow(nullptr);
flow_->Start();
}
}

Expand Down
10 changes: 9 additions & 1 deletion chrome/browser/chromeos/child_accounts/time_limits/app_activity_registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,13 @@ void AppActivityRegistry::OnAppAvailable(const AppId& app_id) {
OnAppReinstalled(app_id);
}

if (IsWebAppOrExtension(app_id) && app_id != GetChromeAppId() &&
base::Contains(activity_registry_, GetChromeAppId()) &&
GetAppState(app_id) == AppState::kBlocked) {
SetAppState(app_id, GetAppState(GetChromeAppId()));
return;
}

SetAppState(app_id, AppState::kAvailable);
}

Expand Down Expand Up @@ -241,7 +248,8 @@ void AppActivityRegistry::OnAppActive(const AppId& app_id,
return;
}

DCHECK(IsAppAvailable(app_id));
if (!IsAppAvailable(app_id))
return;

std::set<aura::Window*>& active_windows = app_details.active_windows;

Expand Down
137 changes: 50 additions & 87 deletions chrome/browser/extensions/api/management/management_api_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
#include "chrome/browser/supervised_user/supervised_user_service.h"
#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
#include "chrome/browser/supervised_user/supervised_user_test_util.h"
#include "components/web_modal/web_contents_modal_dialog_manager.h"
#include "content/public/browser/gpu_data_manager.h"
#endif

Expand Down Expand Up @@ -852,56 +851,78 @@ class TestSupervisedUserExtensionsDelegate
// SupervisedUserExtensionsDelegate:
bool IsChild(content::BrowserContext* context) const override { return true; }

bool IsSupervisedChildWhoMayInstallExtensions(
content::BrowserContext* context) const override {
return is_supervised_child_who_may_install_extensions_;
}
bool IsExtensionAllowedByParent(
const extensions::Extension& extension,
content::BrowserContext* context) const override {
return false;
SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForBrowserContext(context);
return supervised_user_service->IsExtensionAllowed(extension);
}

void PromptForParentPermissionOrShowError(
const extensions::Extension& extension,
content::BrowserContext* context,
content::WebContents* contents,
ParentPermissionDialogDoneCallback parent_permission_callback,
base::OnceClosure error_callback) override {
// Preconditions.
DCHECK(IsChild(context));
DCHECK(!IsExtensionAllowedByParent(extension, context));

if (CanInstallExtensions(context)) {
ShowParentPermissionDialogForExtension(
extension, context, contents, std::move(parent_permission_callback));
} else {
ShowExtensionEnableBlockedByParentDialogForExtension(
extension, contents, std::move(error_callback));
}
}

void set_next_parent_permission_dialog_result(
ParentPermissionDialogResult result) {
dialog_result_ = result;
}

int show_dialog_count() const { return show_dialog_count_; }
int show_block_dialog_count() const { return show_block_dialog_count_; }

private:
// Returns true if |context| represents a supervised child account who may
// install extensions with parent permission.
bool CanInstallExtensions(content::BrowserContext* context) const {
SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForBrowserContext(context);
return supervised_user_service->CanInstallExtensions();
}

// Shows a parent permission dialog for |extension| and call |done_callback|
// when it completes.
void ShowParentPermissionDialogForExtension(
const extensions::Extension& extension,
content::BrowserContext* context,
content::WebContents* contents,
ParentPermissionDialogDoneCallback done_callback) override {
ParentPermissionDialogDoneCallback done_callback) {
++show_dialog_count_;
std::move(done_callback).Run(dialog_result_);
}

// Shows a dialog indicating that |extension| has been blocked and call
// |done_callback| when it completes.
void ShowExtensionEnableBlockedByParentDialogForExtension(
const extensions::Extension* extension,
const extensions::Extension& extension,
content::WebContents* contents,
base::OnceClosure done_callback) override {
base::OnceClosure done_callback) {
show_block_dialog_count_++;
std::move(done_callback).Run();
}

void RecordExtensionEnableBlockedByParentDialogUmaMetric() override {
SupervisedUserExtensionsMetricsRecorder::RecordEnablementUmaMetrics(
SupervisedUserExtensionsMetricsRecorder::EnablementState::
kFailedToEnable);
std::move(done_callback).Run();
}

void set_next_parent_permission_dialog_result(
ParentPermissionDialogResult result) {
dialog_result_ = result;
}

void set_is_supervised_child_who_may_install_extensions(bool value) {
is_supervised_child_who_may_install_extensions_ = value;
}

int show_dialog_count() const { return show_dialog_count_; }
int show_block_dialog_count() const { return show_block_dialog_count_; }

private:
ParentPermissionDialogResult dialog_result_ =
ParentPermissionDialogResult::kParentPermissionFailed;
int show_dialog_count_ = 0;
int show_block_dialog_count_ = 0;
bool is_supervised_child_who_may_install_extensions_ = true;
};

// Tests for supervised users (child accounts). Supervised users are not allowed
Expand Down Expand Up @@ -975,11 +996,8 @@ TEST_F(ManagementApiSupervisedUserTest, SetEnabled_BlockedByParent) {

// Simulate disabling Permissions for sites, apps and extensions
// in the testing supervised user service delegate used by the Management API.
supervised_user_delegate_->set_is_supervised_child_who_may_install_extensions(
false);
// Ensure that the web contents can be used to create a modal dialog.
web_modal::WebContentsModalDialogManager::CreateForWebContents(
web_contents_.get());
GetSupervisedUserService()
->SetSupervisedUserExtensionsMayRequestPermissionsPrefForTesting(false);

// The supervised user trying to enable while Permissions for sites, apps and
// extensions is disabled should fail.
Expand Down Expand Up @@ -1008,61 +1026,6 @@ TEST_F(ManagementApiSupervisedUserTest, SetEnabled_BlockedByParent) {
SupervisedUserExtensionsMetricsRecorder::kFailedToEnableActionName));
}

TEST_F(ManagementApiSupervisedUserTest,
SetEnabled_BlockedByParentNoDialogWhenNoDialogManagerAvailable) {
// Preconditions.
ASSERT_TRUE(profile()->IsChild());

base::HistogramTester histogram_tester;
base::UserActionTester user_action_tester;

base::FilePath base_path = data_dir().AppendASCII("permissions_increase");
base::FilePath pem_path = base_path.AppendASCII("permissions.pem");

base::FilePath path = base_path.AppendASCII("v1");
const Extension* extension =
PackAndInstallCRX(path, pem_path, INSTALL_WITHOUT_LOAD);
ASSERT_TRUE(extension);
// The extension should be installed but disabled.
EXPECT_TRUE(registry()->disabled_extensions().Contains(extension->id()));
const std::string extension_id = extension->id();
ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
EXPECT_TRUE(prefs->HasDisableReason(
extension_id, disable_reason::DISABLE_CUSTODIAN_APPROVAL_REQUIRED));

// Simulate disabling Permissions for sites, apps and extensions
// in the testing supervised user service delegate used by the Management API.
supervised_user_delegate_->set_is_supervised_child_who_may_install_extensions(
false);

// The supervised user trying to enable while Permissions for sites, apps and
// extensions is disabled should fail.
{
std::string error;

bool success = RunSetEnabledFunction(web_contents_.get(), extension_id,
/*use_user_gesture=*/true,
/*accept_dialog=*/true, &error);
EXPECT_FALSE(success);
EXPECT_FALSE(error.empty());
EXPECT_TRUE(registry()->disabled_extensions().Contains(extension_id));

// The block dialog should not have been shown.
EXPECT_EQ(supervised_user_delegate_->show_block_dialog_count(), 0);
}

histogram_tester.ExpectUniqueSample(
SupervisedUserExtensionsMetricsRecorder::kEnablementHistogramName,
SupervisedUserExtensionsMetricsRecorder::EnablementState::kFailedToEnable,
1);
histogram_tester.ExpectTotalCount(
SupervisedUserExtensionsMetricsRecorder::kEnablementHistogramName, 1);
EXPECT_EQ(
1,
user_action_tester.GetActionCount(
SupervisedUserExtensionsMetricsRecorder::kFailedToEnableActionName));
}

// Tests enabling an extension via management API after it was disabled due to
// permission increase for supervised users.
// Prevents a regression to crbug/1068660.
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ bool WebstorePrivateBeginInstallWithManifest3Function::

parent_permission_dialog_ =
ParentPermissionDialog::CreateParentPermissionDialogForExtension(
profile, web_contents, web_contents->GetTopLevelNativeWindow(),
profile, web_contents->GetTopLevelNativeWindow(),
gfx::ImageSkia::CreateFrom1xBitmap(icon_), dummy_extension_.get(),
std::move(done_callback));
parent_permission_dialog_->ShowDialog();
Expand Down
66 changes: 43 additions & 23 deletions chrome/browser/supervised_user/supervised_user_extensions_delegate_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
#include <memory>
#include <utility>

#include "base/threading/thread_task_runner_handle.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/supervised_user/supervised_user_extensions_metrics_recorder.h"
#include "chrome/browser/supervised_user/supervised_user_service.h"
#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
#include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
#include "content/public/browser/web_contents.h"
#include "extensions/browser/extension_dialog_auto_confirm.h"

namespace {

Expand Down Expand Up @@ -54,27 +56,44 @@ bool SupervisedUserExtensionsDelegateImpl::IsChild(
content::BrowserContext* context) const {
SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForBrowserContext(context);

return supervised_user_service->IsChild();
}

bool SupervisedUserExtensionsDelegateImpl::
IsSupervisedChildWhoMayInstallExtensions(
content::BrowserContext* context) const {
bool SupervisedUserExtensionsDelegateImpl::IsExtensionAllowedByParent(
const extensions::Extension& extension,
content::BrowserContext* context) const {
SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForBrowserContext(context);

return supervised_user_service->IsChild() &&
supervised_user_service->CanInstallExtensions();
return supervised_user_service->IsExtensionAllowed(extension);
}

bool SupervisedUserExtensionsDelegateImpl::IsExtensionAllowedByParent(
void SupervisedUserExtensionsDelegateImpl::PromptForParentPermissionOrShowError(
const extensions::Extension& extension,
content::BrowserContext* browser_context,
content::WebContents* web_contents,
ParentPermissionDialogDoneCallback parent_permission_callback,
base::OnceClosure error_callback) {
DCHECK(IsChild(browser_context));
DCHECK(!IsExtensionAllowedByParent(extension, browser_context));

// Supervised users who can install extensions still require parent permission
// for installation or enablement. If the user isn't allowed to install
// extensions at all, then we will just show a "blocked" dialog.
if (CanInstallExtensions(browser_context)) {
ShowParentPermissionDialogForExtension(
extension, browser_context, web_contents,
std::move(parent_permission_callback));
} else {
ShowExtensionEnableBlockedByParentDialogForExtension(
extension, web_contents, std::move(error_callback));
}
}

bool SupervisedUserExtensionsDelegateImpl::CanInstallExtensions(
content::BrowserContext* context) const {
SupervisedUserService* supervised_user_service =
SupervisedUserServiceFactory::GetForBrowserContext(context);
return IsSupervisedChildWhoMayInstallExtensions(context) &&
supervised_user_service->IsExtensionAllowed(extension);
return supervised_user_service->CanInstallExtensions();
}

void SupervisedUserExtensionsDelegateImpl::
Expand All @@ -86,31 +105,32 @@ void SupervisedUserExtensionsDelegateImpl::
ParentPermissionDialog::DoneCallback inner_done_callback = base::BindOnce(
&::OnParentPermissionDialogComplete, std::move(done_callback));

gfx::NativeWindow parent_window =
contents ? contents->GetTopLevelNativeWindow() : nullptr;
parent_permission_dialog_ =
ParentPermissionDialog::CreateParentPermissionDialogForExtension(
Profile::FromBrowserContext(context), contents,
contents->GetTopLevelNativeWindow(), gfx::ImageSkia(), &extension,
std::move(inner_done_callback));
Profile::FromBrowserContext(context), parent_window, gfx::ImageSkia(),
&extension, std::move(inner_done_callback));
parent_permission_dialog_->ShowDialog();
}

void SupervisedUserExtensionsDelegateImpl::
ShowExtensionEnableBlockedByParentDialogForExtension(
const extensions::Extension* extension,
const extensions::Extension& extension,
content::WebContents* contents,
base::OnceClosure done_callback) {
DCHECK(contents);

chrome::ShowExtensionInstallBlockedByParentDialog(
chrome::ExtensionInstalledBlockedByParentDialogAction::kEnable, extension,
contents, std::move(done_callback));
}

void SupervisedUserExtensionsDelegateImpl::
RecordExtensionEnableBlockedByParentDialogUmaMetric() {
SupervisedUserExtensionsMetricsRecorder::RecordEnablementUmaMetrics(
SupervisedUserExtensionsMetricsRecorder::EnablementState::
kFailedToEnable);
if (ScopedTestDialogAutoConfirm::GetAutoConfirmValue() !=
ScopedTestDialogAutoConfirm::NONE) {
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
std::move(done_callback));
return;
}
chrome::ShowExtensionInstallBlockedByParentDialog(
chrome::ExtensionInstalledBlockedByParentDialogAction::kEnable,
&extension, contents, std::move(done_callback));
}

} // namespace extensions
28 changes: 18 additions & 10 deletions chrome/browser/supervised_user/supervised_user_extensions_delegate_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,37 @@ class SupervisedUserExtensionsDelegateImpl

// extensions::SupervisedUserExtensionsDelegate overrides
bool IsChild(content::BrowserContext* context) const override;

bool IsSupervisedChildWhoMayInstallExtensions(
content::BrowserContext* context) const override;

bool IsExtensionAllowedByParent(
const extensions::Extension& extension,
content::BrowserContext* context) const override;
void PromptForParentPermissionOrShowError(
const extensions::Extension& extension,
content::BrowserContext* browser_context,
content::WebContents* web_contents,
ParentPermissionDialogDoneCallback parent_permission_callback,
base::OnceClosure error_callback) override;

private:
// Returns true if |context| represents a supervised child account who may
// install extensions with parent permission.
bool CanInstallExtensions(content::BrowserContext* context) const;

// Shows a parent permission dialog for |extension| and call |done_callback|
// when it completes.
void ShowParentPermissionDialogForExtension(
const extensions::Extension& extension,
content::BrowserContext* context,
content::WebContents* contents,
extensions::SupervisedUserExtensionsDelegate::
ParentPermissionDialogDoneCallback done_callback) override;
ParentPermissionDialogDoneCallback done_callback);

// Shows a dialog indicating that |extension| has been blocked and call
// |done_callback| when it completes.
void ShowExtensionEnableBlockedByParentDialogForExtension(
const extensions::Extension* extension,
const extensions::Extension& extension,
content::WebContents* contents,
base::OnceClosure done_callback) override;
base::OnceClosure done_callback);

void RecordExtensionEnableBlockedByParentDialogUmaMetric() override;

private:
std::unique_ptr<ParentPermissionDialog> parent_permission_dialog_;
};

Expand Down
Loading

0 comments on commit 349247d

Please sign in to comment.