Skip to content

GitLab

T
tangled
Commit c40a10c1 authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot
Browse files
Options

[Extensions Click-to-Script] Adjust context menu options 

Currently, if an extension is affected by runtime host permissions but
doesn't want to run on the current site, we still show the context menu
submenu entry for page access. The only item is the "Learn more" link.

Instead, remove the context menu submenu for these sites, and replace it
with a (disabled) menu item "Can't read or change site's data".

Bug: 909790
Change-Id: Id77efca23a96e8d5d8f683954946970b652612d4
Reviewed-on: https://chromium-review.googlesource.com/c/1351302
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarKaran Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612813}
parent ee3f412f
Hide whitespace changes
Inline Side-by-side
Showing with 65 additions and 17 deletions
chrome/app/generated_resources.grd
View file @ c40a10c1
......@@ -3893,6 +3893,9 @@ are declared in tools/grit/grit_rule.gni.
Has access to this site
</message>
<if expr="not use_titlecase">
<message name="IDS_EXTENSIONS_CONTEXT_MENU_CANT_ACCESS_PAGE" desc="The label in an extension's context menu indicating the extension cannot access the current page. (sentence case)">
Can't read or change site's data
</message>
<message name="IDS_EXTENSIONS_CONTEXT_MENU_PAGE_ACCESS" desc="The label in an extension's context menu for the submenu specifying whether or not the extension can run on the current page (sentence case).">
This can read and change site data
</message>
......@@ -3934,6 +3937,9 @@ are declared in tools/grit/grit_rule.gni.
</message>
</if>
<if expr="use_titlecase">
<message name="IDS_EXTENSIONS_CONTEXT_MENU_CANT_ACCESS_PAGE" desc="The label in an extension's context menu indicating the extension cannot access the current page. (title case)">
Can't Read or Change Site's Data
</message>
<message name="IDS_EXTENSIONS_CONTEXT_MENU_PAGE_ACCESS" desc="The label in an extension's context menu for the submenu specifying whether or not the extension can run on the current page (title case).">
This Can Read and Change Site Data
</message>
......
chrome/browser/extensions/extension_context_menu_model.cc
View file @ c40a10c1
......@@ -131,6 +131,10 @@ ExtensionContextMenuModel::ContextMenuAction CommandIdToContextMenuAction(
return ContextMenuAction::kPageAccessRunOnAllSites;
case ExtensionContextMenuModel::PAGE_ACCESS_LEARN_MORE:
return ContextMenuAction::kPageAccessLearnMore;
case ExtensionContextMenuModel::PAGE_ACCESS_CANT_ACCESS:
case ExtensionContextMenuModel::PAGE_ACCESS_SUBMENU:
NOTREACHED();
break;
default:
break;
}
......@@ -249,6 +253,9 @@ bool ExtensionContextMenuModel::IsCommandIdEnabled(int command_id) const {
}
case UNINSTALL:
return !IsExtensionRequiredByPolicy(extension, profile_);
case PAGE_ACCESS_CANT_ACCESS:
// This menu item, by design, is never enabled.
return false;
// The following, if they are present, are always enabled.
case TOGGLE_VISIBILITY:
case MANAGE_EXTENSIONS:
......@@ -441,16 +448,24 @@ void ExtensionContextMenuModel::CreatePageAccessSubmenu(
content::WebContents* web_contents = GetActiveWebContents();
if (!web_contents)
return;
ScriptingPermissionsModifier modifier(profile_, extension);
if (!modifier.CanAffectExtension())
return;
page_access_submenu_.reset(new ui::SimpleMenuModel(this));
const int kRadioGroup = 0;
const GURL& url = web_contents->GetLastCommittedURL();
ScriptingPermissionsModifier::SiteAccess site_access =
modifier.GetSiteAccess(url);
if (!site_access.has_site_access && !site_access.withheld_site_access) {
AddItemWithStringId(PAGE_ACCESS_CANT_ACCESS,
IDS_EXTENSIONS_CONTEXT_MENU_CANT_ACCESS_PAGE);
return;
}
const int kRadioGroup = 0;
page_access_submenu_ = std::make_unique<ui::SimpleMenuModel>(this);
// Only show the access controls if the extension either has or wants access
// to the site.
if (site_access.has_site_access || site_access.withheld_site_access) {
......
chrome/browser/extensions/extension_context_menu_model.h
View file @ c40a10c1
......@@ -35,6 +35,7 @@ class ExtensionContextMenuModel : public ui::SimpleMenuModel,
UNINSTALL,
MANAGE_EXTENSIONS,
INSPECT_POPUP,
PAGE_ACCESS_CANT_ACCESS,
PAGE_ACCESS_SUBMENU,
PAGE_ACCESS_RUN_ON_CLICK,
PAGE_ACCESS_RUN_ON_SITE,
......
chrome/browser/extensions/extension_context_menu_model_unittest.cc
View file @ c40a10c1
......@@ -214,6 +214,10 @@ class ExtensionContextMenuModelTest : public ExtensionServiceTestBase {
// Returns true if the |menu| has the page access submenu at all.
bool HasPageAccessSubmenu(const ExtensionContextMenuModel& menu) const;
// Returns true if the |menu| has a valid entry for the "can't access page"
// item.
bool HasCantAccessPageEntry(const ExtensionContextMenuModel& menu) const;
void SetUp() override;
void TearDown() override;
......@@ -316,6 +320,19 @@ bool ExtensionContextMenuModelTest::HasPageAccessSubmenu(
ExtensionContextMenuModel::PAGE_ACCESS_SUBMENU) != -1;
}
bool ExtensionContextMenuModelTest::HasCantAccessPageEntry(
const ExtensionContextMenuModel& menu) const {
if (menu.GetIndexOfCommandId(
ExtensionContextMenuModel::PAGE_ACCESS_CANT_ACCESS) == -1) {
return false;
}
// The "Can't access this page" entry, if present, should always be disabled.
EXPECT_FALSE(menu.IsCommandIdEnabled(
ExtensionContextMenuModel::PAGE_ACCESS_CANT_ACCESS));
return true;
}
void ExtensionContextMenuModelTest::SetUp() {
ExtensionServiceTestBase::SetUp();
content::BrowserSideNavigationSetUp();
......@@ -1074,6 +1091,23 @@ TEST_F(ExtensionContextMenuModelTest, PageAccessMenuOptions) {
ExtensionContextMenuModel menu(extension.get(), GetBrowser(),
ExtensionContextMenuModel::VISIBLE, nullptr);
EXPECT_EQ(test_case.selected_entry.has_value(),
!test_case.expected_entries.empty())
<< "If any entries are available, one should be selected.";
if (test_case.expected_entries.empty()) {
// If there are no expected entries (i.e., the extension can't run on the
// page), there should be no submenu and instead there should be a
// disabled label.
int label_index = menu.GetIndexOfCommandId(
ExtensionContextMenuModel::PAGE_ACCESS_CANT_ACCESS);
EXPECT_NE(-1, label_index);
EXPECT_FALSE(menu.IsCommandIdEnabled(
ExtensionContextMenuModel::PAGE_ACCESS_CANT_ACCESS));
EXPECT_FALSE(HasPageAccessSubmenu(menu));
continue;
}
// The learn more option should be visible whenever the menu is.
EXPECT_TRUE(HasPageAccessCommand(menu, kLearnMore));
......@@ -1085,10 +1119,6 @@ TEST_F(ExtensionContextMenuModelTest, PageAccessMenuOptions) {
EXPECT_EQ(test_case.expected_entries.count(kOnAllSites),
HasPageAccessCommand(menu, kOnAllSites));
EXPECT_EQ(test_case.selected_entry.has_value(),
!test_case.expected_entries.empty())
<< "If any entries are available, one should be selected.";
auto should_command_be_checked = [test_case](int command) {
return test_case.selected_entry && *test_case.selected_entry == command;
};
......@@ -1157,6 +1187,7 @@ TEST_F(ExtensionContextMenuModelTest,
// Without withholding host permissions, the menu should be visible on
// a.com...
EXPECT_TRUE(HasPageAccessSubmenu(menu));
EXPECT_FALSE(HasCantAccessPageEntry(menu));
EXPECT_TRUE(HasPageAccessCommand(menu, kOnClick));
EXPECT_TRUE(HasPageAccessCommand(menu, kOnSite));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnAllSites));
......@@ -1172,14 +1203,11 @@ TEST_F(ExtensionContextMenuModelTest,
web_contents_tester->NavigateAndCommit(b_com);
{
// ... but only the learn more option should be visible on b.com.
// ... but not on b.com, where it doesn't want to run.
ExtensionContextMenuModel menu(extension.get(), GetBrowser(),
ExtensionContextMenuModel::VISIBLE, nullptr);
EXPECT_TRUE(HasPageAccessSubmenu(menu));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnClick));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnSite));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnAllSites));
EXPECT_TRUE(HasPageAccessCommand(menu, kLearnMore));
EXPECT_FALSE(HasPageAccessSubmenu(menu));
EXPECT_TRUE(HasCantAccessPageEntry(menu));
}
modifier.SetWithholdHostPermissions(true);
......@@ -1193,6 +1221,7 @@ TEST_F(ExtensionContextMenuModelTest,
ExtensionContextMenuModel menu(extension.get(), GetBrowser(),
ExtensionContextMenuModel::VISIBLE, nullptr);
EXPECT_TRUE(HasPageAccessSubmenu(menu));
EXPECT_FALSE(HasCantAccessPageEntry(menu));
EXPECT_TRUE(HasPageAccessCommand(menu, kOnClick));
EXPECT_TRUE(HasPageAccessCommand(menu, kOnSite));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnAllSites));
......@@ -1216,11 +1245,8 @@ TEST_F(ExtensionContextMenuModelTest,
ExtensionContextMenuModel::VISIBLE, nullptr);
// Somewhat strangely, this also removes the access controls, because we don't
// show it for sites the extension doesn't want to run on.
EXPECT_TRUE(HasPageAccessSubmenu(menu));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnClick));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnSite));
EXPECT_FALSE(HasPageAccessCommand(menu, kOnAllSites));
EXPECT_TRUE(HasPageAccessCommand(menu, kLearnMore));
EXPECT_FALSE(HasPageAccessSubmenu(menu));
EXPECT_TRUE(HasCantAccessPageEntry(menu));
}
TEST_F(ExtensionContextMenuModelTest,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment