From d4b8df2654ef88e0905a55365704db2a5f769eb2 Mon Sep 17 00:00:00 2001 From: treib Date: Thu, 12 Mar 2015 02:39:18 -0700 Subject: [PATCH] ChromePermissionMessageProvider: make sure to coalesce messages and their details together BUG=398257 Review URL: https://codereview.chromium.org/996193002 Cr-Commit-Position: refs/heads/master@{#320254} --- .../chrome_permission_message_provider.cc | 232 +++++++++++---------- .../chrome_permission_message_provider.h | 10 + .../chrome_permission_message_provider_unittest.cc | 57 ++++- 3 files changed, 189 insertions(+), 110 deletions(-) diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc index 8605b578ca24..d5ceeea6ba28 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_provider.cc +++ b/chrome/common/extensions/permissions/chrome_permission_message_provider.cc @@ -189,105 +189,11 @@ std::vector ChromePermissionMessageProvider::GetWarningMessages( const PermissionSet* permissions, Manifest::Type extension_type) const { std::vector message_strings; - PermissionMessages messages = - GetPermissionMessages(permissions, extension_type); - - // WARNING: When modifying a coalescing rule in this list, be sure to also - // modify the corresponding rule in - // ChromePermissionMessageProvider::GetCoalescedPermissionMessages(). - // TODO(sashab): Deprecate this function, and remove this list. - for (PermissionMessages::const_iterator i = messages.begin(); - i != messages.end(); ++i) { - int id = i->id(); - // Access to users' devices should provide a single warning message - // specifying the transport method used; serial and/or Bluetooth. - if (id == PermissionMessage::kBluetooth || - id == PermissionMessage::kSerial) { - if (ContainsMessages(messages, - PermissionMessage::kBluetooth, - PermissionMessage::kSerial)) { - if (id == PermissionMessage::kBluetooth) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_BLUETOOTH_SERIAL)); - } - continue; - } - } - if (id == PermissionMessage::kAccessibilityFeaturesModify || - id == PermissionMessage::kAccessibilityFeaturesRead) { - if (ContainsMessages(messages, - PermissionMessage::kAccessibilityFeaturesModify, - PermissionMessage::kAccessibilityFeaturesRead)) { - if (id == PermissionMessage::kAccessibilityFeaturesModify) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY)); - } - continue; - } - } - if (id == PermissionMessage::kAudioCapture || - id == PermissionMessage::kVideoCapture) { - if (ContainsMessages(messages, - PermissionMessage::kAudioCapture, - PermissionMessage::kVideoCapture)) { - if (id == PermissionMessage::kAudioCapture) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_AUDIO_AND_VIDEO_CAPTURE)); - } - continue; - } - } - if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo || - id == PermissionMessage::kMediaGalleriesAllGalleriesDelete || - id == PermissionMessage::kMediaGalleriesAllGalleriesRead) { - if (ContainsMessages( - messages, - PermissionMessage::kMediaGalleriesAllGalleriesCopyTo, - PermissionMessage::kMediaGalleriesAllGalleriesDelete, - PermissionMessage::kMediaGalleriesAllGalleriesRead)) { - if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE_DELETE)); - } - continue; - } - if (ContainsMessages( - messages, - PermissionMessage::kMediaGalleriesAllGalleriesCopyTo, - PermissionMessage::kMediaGalleriesAllGalleriesRead)) { - if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE)); - } - continue; - } - if (ContainsMessages( - messages, - PermissionMessage::kMediaGalleriesAllGalleriesDelete, - PermissionMessage::kMediaGalleriesAllGalleriesRead)) { - if (id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_DELETE)); - } - continue; - } - } - if (permissions->HasAPIPermission(APIPermission::kSessions) && - id == PermissionMessage::kTabs) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS)); - continue; - } - if (permissions->HasAPIPermission(APIPermission::kSessions) && - id == PermissionMessage::kBrowsingHistory) { - message_strings.push_back(l10n_util::GetStringUTF16( - IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE_AND_SESSIONS)); - continue; - } - - message_strings.push_back(i->message()); - } - + std::vector message_details_strings; + CoalesceWarningMessages(permissions, + extension_type, + &message_strings, + &message_details_strings); return message_strings; } @@ -296,14 +202,12 @@ ChromePermissionMessageProvider::GetWarningMessagesDetails( const PermissionSet* permissions, Manifest::Type extension_type) const { std::vector message_strings; - PermissionMessages messages = - GetPermissionMessages(permissions, extension_type); - - for (PermissionMessages::const_iterator i = messages.begin(); - i != messages.end(); ++i) - message_strings.push_back(i->details()); - - return message_strings; + std::vector message_details_strings; + CoalesceWarningMessages(permissions, + extension_type, + &message_strings, + &message_details_strings); + return message_details_strings; } bool ChromePermissionMessageProvider::IsPrivilegeIncrease( @@ -432,6 +336,120 @@ ChromePermissionMessageProvider::GetHostPermissionMessages( return messages; } +void ChromePermissionMessageProvider::CoalesceWarningMessages( + const PermissionSet* permissions, + Manifest::Type extension_type, + std::vector* message_strings, + std::vector* message_details_strings) const { + PermissionMessages messages = + GetPermissionMessages(permissions, extension_type); + + // WARNING: When modifying a coalescing rule in this list, be sure to also + // modify the corresponding rule in + // ChromePermissionMessageProvider::GetCoalescedPermissionMessages(). + // TODO(sashab): Deprecate this function, and remove this list. + for (PermissionMessages::const_iterator i = messages.begin(); + i != messages.end(); ++i) { + int id = i->id(); + // Access to users' devices should provide a single warning message + // specifying the transport method used; serial and/or Bluetooth. + if (id == PermissionMessage::kBluetooth || + id == PermissionMessage::kSerial) { + if (ContainsMessages(messages, + PermissionMessage::kBluetooth, + PermissionMessage::kSerial)) { + if (id == PermissionMessage::kBluetooth) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_BLUETOOTH_SERIAL)); + message_details_strings->push_back(base::string16()); + } + continue; + } + } + if (id == PermissionMessage::kAccessibilityFeaturesModify || + id == PermissionMessage::kAccessibilityFeaturesRead) { + if (ContainsMessages(messages, + PermissionMessage::kAccessibilityFeaturesModify, + PermissionMessage::kAccessibilityFeaturesRead)) { + if (id == PermissionMessage::kAccessibilityFeaturesModify) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_ACCESSIBILITY_FEATURES_READ_MODIFY)); + message_details_strings->push_back(base::string16()); + } + continue; + } + } + if (id == PermissionMessage::kAudioCapture || + id == PermissionMessage::kVideoCapture) { + if (ContainsMessages(messages, + PermissionMessage::kAudioCapture, + PermissionMessage::kVideoCapture)) { + if (id == PermissionMessage::kAudioCapture) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_AUDIO_AND_VIDEO_CAPTURE)); + message_details_strings->push_back(base::string16()); + } + continue; + } + } + if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo || + id == PermissionMessage::kMediaGalleriesAllGalleriesDelete || + id == PermissionMessage::kMediaGalleriesAllGalleriesRead) { + if (ContainsMessages( + messages, + PermissionMessage::kMediaGalleriesAllGalleriesCopyTo, + PermissionMessage::kMediaGalleriesAllGalleriesDelete, + PermissionMessage::kMediaGalleriesAllGalleriesRead)) { + if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE_DELETE)); + message_details_strings->push_back(base::string16()); + } + continue; + } + if (ContainsMessages( + messages, + PermissionMessage::kMediaGalleriesAllGalleriesCopyTo, + PermissionMessage::kMediaGalleriesAllGalleriesRead)) { + if (id == PermissionMessage::kMediaGalleriesAllGalleriesCopyTo) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_WRITE)); + message_details_strings->push_back(base::string16()); + } + continue; + } + if (ContainsMessages( + messages, + PermissionMessage::kMediaGalleriesAllGalleriesDelete, + PermissionMessage::kMediaGalleriesAllGalleriesRead)) { + if (id == PermissionMessage::kMediaGalleriesAllGalleriesDelete) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_MEDIA_GALLERIES_READ_DELETE)); + message_details_strings->push_back(base::string16()); + } + continue; + } + } + if (permissions->HasAPIPermission(APIPermission::kSessions) && + id == PermissionMessage::kTabs) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS)); + message_details_strings->push_back(base::string16()); + continue; + } + if (permissions->HasAPIPermission(APIPermission::kSessions) && + id == PermissionMessage::kBrowsingHistory) { + message_strings->push_back(l10n_util::GetStringUTF16( + IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE_AND_SESSIONS)); + message_details_strings->push_back(base::string16()); + continue; + } + + message_strings->push_back(i->message()); + message_details_strings->push_back(i->details()); + } +} + bool ChromePermissionMessageProvider::IsAPIPrivilegeIncrease( const PermissionSet* old_permissions, const PermissionSet* new_permissions) const { diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider.h b/chrome/common/extensions/permissions/chrome_permission_message_provider.h index d0c4614f3557..068d8bd295a6 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_provider.h +++ b/chrome/common/extensions/permissions/chrome_permission_message_provider.h @@ -70,6 +70,16 @@ class ChromePermissionMessageProvider : public PermissionMessageProvider { PermissionIDSet* permission_ids, Manifest::Type extension_type) const; + // Applies coalescing rules and writes the resulting messages and their + // details into |message_strings| and |message_details_strings|. + // TODO(treib): Remove this method as soon as we've fully switched to the + // new system. + void CoalesceWarningMessages( + const PermissionSet* permissions, + Manifest::Type extension_type, + std::vector* message_strings, + std::vector* message_details_strings) const; + // Returns true if |new_permissions| has an elevated API privilege level // compared to |old_permissions|. bool IsAPIPrivilegeIncrease( diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc b/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc index dbdced049f4a..e36e4bc8425a 100644 --- a/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc +++ b/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc @@ -4,9 +4,16 @@ #include "chrome/common/extensions/permissions/chrome_permission_message_provider.h" +#include + #include "base/memory/scoped_ptr.h" +#include "base/strings/string16.h" +#include "base/values.h" #include "chrome/grit/generated_resources.h" -#include "extensions/common/permissions/permissions_data.h" +#include "extensions/common/permissions/permission_set.h" +#include "extensions/common/permissions/permissions_info.h" +#include "extensions/common/permissions/usb_device_permission.h" +#include "extensions/common/url_pattern_set.h" #include "extensions/strings/grit/extensions_strings.h" #include "testing/gtest/include/gtest/gtest.h" #include "ui/base/l10n/l10n_util.h" @@ -25,13 +32,21 @@ class ChromePermissionMessageProviderUnittest : public testing::Test { ~ChromePermissionMessageProviderUnittest() override {} protected: - std::vector GetMessages(APIPermissionSet& permissions, + std::vector GetMessages(const APIPermissionSet& permissions, Manifest::Type type) { scoped_refptr permission_set = new PermissionSet( permissions, ManifestPermissionSet(), URLPatternSet(), URLPatternSet()); return message_provider_->GetWarningMessages(permission_set.get(), type); } + std::vector GetDetails(const APIPermissionSet& permissions, + Manifest::Type type) { + scoped_refptr permission_set = new PermissionSet( + permissions, ManifestPermissionSet(), URLPatternSet(), URLPatternSet()); + return message_provider_->GetWarningMessagesDetails(permission_set.get(), + type); + } + private: scoped_ptr message_provider_; @@ -39,7 +54,7 @@ class ChromePermissionMessageProviderUnittest : public testing::Test { }; // Checks that if an app has a superset and a subset permission, only the -// superset permission message is displayed if thye are both present. +// superset permission message is displayed if they are both present. TEST_F(ChromePermissionMessageProviderUnittest, SupersetOverridesSubsetPermission) { APIPermissionSet permissions; @@ -70,4 +85,40 @@ TEST_F(ChromePermissionMessageProviderUnittest, messages[0]); } +// Checks that when permissions are merged into a single message, their details +// are merged as well. +TEST_F(ChromePermissionMessageProviderUnittest, + WarningsAndDetailsCoalesceTogether) { + // kTab and kTopSites should be merged into a single message. + APIPermissionSet permissions; + permissions.insert(APIPermission::kTab); + permissions.insert(APIPermission::kTopSites); + // The USB device permission message has a non-empty details string. + scoped_ptr usb(new UsbDevicePermission( + PermissionsInfo::GetInstance()->GetByID(APIPermission::kUsbDevice))); + scoped_ptr devices_list(new base::ListValue()); + devices_list->Append( + UsbDevicePermissionData(0x02ad, 0x138c, -1).ToValue().release()); + devices_list->Append( + UsbDevicePermissionData(0x02ad, 0x138d, -1).ToValue().release()); + ASSERT_TRUE(usb->FromValue(devices_list.get(), nullptr, nullptr)); + permissions.insert(usb.release()); + + std::vector messages = + GetMessages(permissions, Manifest::TYPE_EXTENSION); + std::vector details = + GetDetails(permissions, Manifest::TYPE_EXTENSION); + + ASSERT_EQ(2U, messages.size()); + ASSERT_EQ(messages.size(), details.size()); + EXPECT_EQ( + l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ), + messages[0]); + EXPECT_TRUE(details[0].empty()); + EXPECT_EQ( + l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_USB_DEVICE_LIST), + messages[1]); + EXPECT_FALSE(details[1].empty()); +} + } // namespace extensions -- 2.11.4.GIT