From f9bde4c57f6070dde911bb060e114e6da770f4d3 Mon Sep 17 00:00:00 2001 From: treib Date: Thu, 27 Aug 2015 09:32:24 -0700 Subject: [PATCH] Remove APIPermission::kFileSystemWriteDirectory In the new permission system, the corresponding message can be represented by kFileSystemWrite + kFileSystemDirectory, we don't need a special fake ID anymore. Skipping presubmit because of renamed histogram entry. NOPRESUBMIT=true BUG=284849 Review URL: https://codereview.chromium.org/1217773006 Cr-Commit-Position: refs/heads/master@{#345897} --- .../permissions/chrome_api_permissions.cc | 1 - .../chrome_permission_message_provider.cc | 29 ++--------------- .../chrome_permission_message_provider.h | 14 ++------ .../permissions/chrome_permission_message_rules.cc | 9 +----- .../permissions/permission_set_unittest.cc | 37 ++-------------------- .../common/manifest_handlers/permissions_parser.cc | 2 -- extensions/common/permissions/api_permission.h | 2 +- .../common/permissions/api_permission_set.cc | 11 ------- extensions/common/permissions/api_permission_set.h | 2 -- tools/metrics/histograms/histograms.xml | 2 +- 10 files changed, 11 insertions(+), 98 deletions(-) diff --git a/chrome/common/extensions/permissions/chrome_api_permissions.cc b/chrome/common/extensions/permissions/chrome_api_permissions.cc index dee698d8dddb..0c8648fa3491 100644 --- a/chrome/common/extensions/permissions/chrome_api_permissions.cc +++ b/chrome/common/extensions/permissions/chrome_api_permissions.cc @@ -238,7 +238,6 @@ std::vector ChromeAPIPermissions::GetAllPermissions() "fileSystem.requestFileSystem"}, {APIPermission::kFileSystemRetainEntries, "fileSystem.retainEntries"}, {APIPermission::kFileSystemWrite, "fileSystem.write"}, - {APIPermission::kFileSystemWriteDirectory, "fileSystem.writeDirectory"}, {APIPermission::kMediaGalleries, "mediaGalleries", APIPermissionInfo::kFlagNone, &CreateAPIPermission}, diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc index 12e843ea6730..786e65c5bfdf 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc +++ b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc @@ -105,10 +105,7 @@ bool ChromePermissionMessageProvider::IsPrivilegeIncrease( if (IsHostPrivilegeIncrease(old_permissions, new_permissions, extension_type)) return true; - if (IsAPIPrivilegeIncrease(old_permissions, new_permissions)) - return true; - - if (IsManifestPermissionPrivilegeIncrease(old_permissions, new_permissions)) + if (IsAPIOrManifestPrivilegeIncrease(old_permissions, new_permissions)) return true; return false; @@ -177,36 +174,16 @@ void ChromePermissionMessageProvider::AddHostPermissions( } } -bool ChromePermissionMessageProvider::IsAPIPrivilegeIncrease( +bool ChromePermissionMessageProvider::IsAPIOrManifestPrivilegeIncrease( const PermissionSet* old_permissions, const PermissionSet* new_permissions) const { PermissionIDSet old_ids; AddAPIPermissions(old_permissions, &old_ids); - PermissionIDSet new_ids; - AddAPIPermissions(new_permissions, &new_ids); - - // A special hack: kFileSystemWriteDirectory implies kFileSystemDirectory. - // TODO(sammc): Remove this. See http://crbug.com/284849. - if (old_ids.ContainsID(APIPermission::kFileSystemWriteDirectory)) - old_ids.insert(APIPermission::kFileSystemDirectory); - - return IsAPIOrManifestPrivilegeIncrease(old_ids, new_ids); -} - -bool ChromePermissionMessageProvider::IsManifestPermissionPrivilegeIncrease( - const PermissionSet* old_permissions, - const PermissionSet* new_permissions) const { - PermissionIDSet old_ids; AddManifestPermissions(old_permissions, &old_ids); PermissionIDSet new_ids; + AddAPIPermissions(new_permissions, &new_ids); AddManifestPermissions(new_permissions, &new_ids); - return IsAPIOrManifestPrivilegeIncrease(old_ids, new_ids); -} - -bool ChromePermissionMessageProvider::IsAPIOrManifestPrivilegeIncrease( - const PermissionIDSet& old_ids, - const PermissionIDSet& new_ids) const { // If all the IDs were already there, it's not a privilege increase. if (old_ids.Includes(new_ids)) return false; diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider.h b/chrome/common/extensions/permissions/chrome_permission_message_provider.h index e04c773d2652..18154070ccf1 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_provider.h +++ b/chrome/common/extensions/permissions/chrome_permission_message_provider.h @@ -49,20 +49,12 @@ class ChromePermissionMessageProvider : public PermissionMessageProvider { PermissionIDSet* permission_ids, Manifest::Type extension_type) const; - // Returns true if |new_permissions| has an elevated API privilege level - // compared to |old_permissions|. - bool IsAPIPrivilegeIncrease(const PermissionSet* old_permissions, - const PermissionSet* new_permissions) const; - - // Returns true if |new_permissions| has an elevated manifest permission - // privilege level compared to |old_permissions|. - bool IsManifestPermissionPrivilegeIncrease( + // Returns true if |new_permissions| has an elevated API or manifest privilege + // level compared to |old_permissions|. + bool IsAPIOrManifestPrivilegeIncrease( const PermissionSet* old_permissions, const PermissionSet* new_permissions) const; - bool IsAPIOrManifestPrivilegeIncrease(const PermissionIDSet& old_ids, - const PermissionIDSet& new_ids) const; - // Returns true if |new_permissions| has more host permissions compared to // |old_permissions|. bool IsHostPrivilegeIncrease( diff --git a/chrome/common/extensions/permissions/chrome_permission_message_rules.cc b/chrome/common/extensions/permissions/chrome_permission_message_rules.cc index 977a21a47c92..2b744e07ff67 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_rules.cc +++ b/chrome/common/extensions/permissions/chrome_permission_message_rules.cc @@ -431,13 +431,9 @@ ChromePermissionMessageRule::GetAllRules() { {APIPermission::kHistory}, {APIPermission::kFavicon, APIPermission::kProcesses, APIPermission::kTab, APIPermission::kTopSites, APIPermission::kWebNavigation}}, - // A special hack: If kFileSystemWriteDirectory would be displayed, hide - // kFileSystemDirectory as the write directory message implies it. - // TODO(sashab): Remove kFileSystemWriteDirectory; it's no longer needed - // since this rules system can represent the rule. See crbug.com/284849. {IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_WRITE_DIRECTORY, {APIPermission::kFileSystemWrite, APIPermission::kFileSystemDirectory}, - {APIPermission::kFileSystemWriteDirectory}}, + {}}, // Full access already allows DeclarativeWebRequest, reading the list of // most frequently visited sites, and tab access. // The warning message for declarativeWebRequest @@ -577,9 +573,6 @@ ChromePermissionMessageRule::GetAllRules() { {IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_DIRECTORY, {APIPermission::kFileSystemDirectory}, {}}, - {IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_WRITE_DIRECTORY, - {APIPermission::kFileSystemWriteDirectory}, - {}}, // Because warning messages for the "mediaGalleries" permission // vary based on the permissions parameters, no message ID or diff --git a/chrome/common/extensions/permissions/permission_set_unittest.cc b/chrome/common/extensions/permissions/permission_set_unittest.cc index e62a5109a902..7f8f381d9aa7 100644 --- a/chrome/common/extensions/permissions/permission_set_unittest.cc +++ b/chrome/common/extensions/permissions/permission_set_unittest.cc @@ -873,43 +873,10 @@ TEST(PermissionsTest, FileSystemPermissionMessages) { MakePermissionIDSet(api_permissions))); } -// The file system permissions have a special-case hack to show a warning for -// write and directory at the same time. -// TODO(sammc): Remove this. See http://crbug.com/284849. -TEST(PermissionsTest, FileSystemImplicitPermissions) { - APIPermissionSet apis; - apis.insert(APIPermission::kFileSystemWrite); - apis.AddImpliedPermissions(); - - EXPECT_EQ(apis.find(APIPermission::kFileSystemWrite)->id(), - APIPermission::kFileSystemWrite); - EXPECT_EQ(apis.size(), 1u); - - apis.erase(APIPermission::kFileSystemWrite); - apis.insert(APIPermission::kFileSystemDirectory); - apis.AddImpliedPermissions(); - - EXPECT_EQ(apis.find(APIPermission::kFileSystemDirectory)->id(), - APIPermission::kFileSystemDirectory); - EXPECT_EQ(apis.size(), 1u); - - apis.insert(APIPermission::kFileSystemWrite); - apis.AddImpliedPermissions(); - - EXPECT_EQ(apis.find(APIPermission::kFileSystemWrite)->id(), - APIPermission::kFileSystemWrite); - EXPECT_EQ(apis.find(APIPermission::kFileSystemDirectory)->id(), - APIPermission::kFileSystemDirectory); - EXPECT_EQ(apis.find(APIPermission::kFileSystemWriteDirectory)->id(), - APIPermission::kFileSystemWriteDirectory); - EXPECT_EQ(apis.size(), 3u); -} - TEST(PermissionsTest, HiddenFileSystemPermissionMessages) { APIPermissionSet api_permissions; api_permissions.insert(APIPermission::kFileSystemWrite); api_permissions.insert(APIPermission::kFileSystemDirectory); - api_permissions.insert(APIPermission::kFileSystemWriteDirectory); scoped_refptr permissions( new PermissionSet(api_permissions, ManifestPermissionSet(), URLPatternSet(), URLPatternSet())); @@ -1074,8 +1041,8 @@ TEST(PermissionsTest, MergedFileSystemPermissionComparison) { URLPatternSet(), URLPatternSet())); APIPermissionSet write_directory_api_permissions; - write_directory_api_permissions.insert( - APIPermission::kFileSystemWriteDirectory); + write_directory_api_permissions.insert(APIPermission::kFileSystemWrite); + write_directory_api_permissions.insert(APIPermission::kFileSystemDirectory); scoped_refptr write_directory_permissions( new PermissionSet(write_directory_api_permissions, ManifestPermissionSet(), diff --git a/extensions/common/manifest_handlers/permissions_parser.cc b/extensions/common/manifest_handlers/permissions_parser.cc index f8ef11c41fac..e221ed36e78f 100644 --- a/extensions/common/manifest_handlers/permissions_parser.cc +++ b/extensions/common/manifest_handlers/permissions_parser.cc @@ -149,8 +149,6 @@ bool ParseHelper(Extension* extension, } } - api_permissions->AddImpliedPermissions(); - // Remove permissions that are not available to this extension. for (std::vector::const_iterator iter = to_remove.begin(); iter != to_remove.end(); diff --git a/extensions/common/permissions/api_permission.h b/extensions/common/permissions/api_permission.h index b2c6d46b7e6a..e49a1531363c 100644 --- a/extensions/common/permissions/api_permission.h +++ b/extensions/common/permissions/api_permission.h @@ -118,7 +118,7 @@ class APIPermission { kFileSystemRequestFileSystem, kFileSystemRetainEntries, kFileSystemWrite, - kFileSystemWriteDirectory, + kDeleted_FileSystemWriteDirectory, kFirstRunPrivate, kFontSettings, kFullscreen, diff --git a/extensions/common/permissions/api_permission_set.cc b/extensions/common/permissions/api_permission_set.cc index 74a3e6629145..9f6df2f45f85 100644 --- a/extensions/common/permissions/api_permission_set.cc +++ b/extensions/common/permissions/api_permission_set.cc @@ -182,17 +182,6 @@ bool APIPermissionSet::ParseFromJSON( return true; } -void APIPermissionSet::AddImpliedPermissions() { - // The fileSystem.write and fileSystem.directory permissions imply - // fileSystem.writeDirectory. - // Has a corresponding rule in ChromePermissionMessageProvider. - // TODO(sammc): Remove this. See http://crbug.com/284849. - if (ContainsKey(map(), APIPermission::kFileSystemWrite) && - ContainsKey(map(), APIPermission::kFileSystemDirectory)) { - insert(APIPermission::kFileSystemWriteDirectory); - } -} - PermissionID::PermissionID(APIPermission::ID id) : std::pair(id, base::string16()) { } diff --git a/extensions/common/permissions/api_permission_set.h b/extensions/common/permissions/api_permission_set.h index 43b97ec02894..a596a0e34616 100644 --- a/extensions/common/permissions/api_permission_set.h +++ b/extensions/common/permissions/api_permission_set.h @@ -60,8 +60,6 @@ class APIPermissionSet : public BaseSetOperators { APIPermissionSet* api_permissions, base::string16* error, std::vector* unhandled_permissions); - - void AddImpliedPermissions(); }; // An ID representing a single permission that belongs to an app or extension. diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 7d33fa3262db..aafe865d69ff 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -58781,7 +58781,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. - + -- 2.11.4.GIT