Added unit tests for ChromePermissionMessageProvider

chrome/browser/extensions/permission_messages_unittest.cc

+// Copyright 2014 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 "base/memory/scoped_ptr.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_service_test_base.h"
+#include "chrome/browser/extensions/permissions_updater.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/permissions/chrome_permission_message_provider.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/crx_file/id_util.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/manifest_handlers/permissions_parser.h"
+#include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace extensions {
+
+// Tests that ChromePermissionMessageProvider provides not only correct, but
+// meaningful permission messages that coalesce correctly where appropriate.
+// There are 3 types of permission messages that need to be tested:
+//  1. The combined list of active permissions, displayed at install time (or
+//     when the app has been disabled automatically and needs to be re-enabled)
+//  2. The split list of active permissions, displayed in the App Info dialog,
+//     where the optional permissions are individually revokable
+//  3. The list of requested optional permissions, displayed in a prompt to the
+//     user when the app requests these during runtime
+// Some of these tests are currently 'anti-tests' - they demonstrate existing

[scheib, 2014/09/25 17:55:44]

Call out the naming convention:
"Some of these tests are prefixed "AntiTest_", they demonstrate ...

[sashab, 2014/09/29 08:03:35]

Done.

+// problematic functionality. These tests are prefixed with AntiTest_ and will
+// be changed as the correct behaviour is implemented. TODOs in the test explain
+// the currently problematic behaviour.
+class PermissionMessagesUnittest : public ExtensionServiceTestBase {

[scheib, 2014/09/25 17:55:44]

Cite the existence of this test suite at the top of the chrome permission
message provider class, so that people know to go looking in a far off place for
it.

[sashab, 2014/09/29 08:03:36]

Good idea. Done.

+ public:
+  PermissionMessagesUnittest()
+      : message_provider_(new ChromePermissionMessageProvider()) {}
+  virtual ~PermissionMessagesUnittest() {}
+
+  // Overridden from testing::Test:
+  virtual void SetUp() OVERRIDE {
+    ExtensionServiceTestBase::SetUp();
+    InitializeExtensionService(CreateDefaultInitParams());
+    InitializeProcessManager();
+  }
+
+ protected:
+  void CreateAndInstallAppWithPermissions(ListBuilder& required_permissions,
+                                          ListBuilder& optional_permissions) {
+    app_ = test_util::BuildApp(ExtensionBuilder().Pass())
+               .MergeManifest(
+                    DictionaryBuilder()
+                        .Set("permissions", required_permissions)
+                        .Set("optional_permissions", optional_permissions))
+               .SetID(crx_file::id_util::GenerateId("app"))
+               .SetLocation(Manifest::INTERNAL)
+               .Build();
+    service()->AddExtension(app_.get());
+  }
+
+  void CreateAndInstallExtensionWithPermissions(
+      ListBuilder& required_permissions,
+      ListBuilder& optional_permissions) {
+    app_ = test_util::BuildExtension(ExtensionBuilder().Pass())
+               .MergeManifest(
+                    DictionaryBuilder()
+                        .Set("permissions", required_permissions)
+                        .Set("optional_permissions", optional_permissions))
+               .SetID(crx_file::id_util::GenerateId("extension"))
+               .SetLocation(Manifest::INTERNAL)
+               .Build();
+    service()->AddExtension(app_.get());
+  }
+
+  // Returns the permission messages that would display in the prompt that
+  // requests all the optional permissions for the current |app_|.
+  std::vector<base::string16> RequestOptionalPermissions() {

[scheib, 2014/09/25 17:55:44]

This name seems a tad confusing to me still. Perhaps RequestedPermissions? 

[sashab, 2014/09/29 08:03:35]

Changed to GetOptionalPermissionMessages() :)

+    scoped_refptr<const PermissionSet> granted_permissions =
+        ExtensionPrefs::Get(profile())->GetGrantedPermissions(app_->id());
+    scoped_refptr<const PermissionSet> optional_permissions =
+        PermissionsParser::GetOptionalPermissions(app_.get());
+    scoped_refptr<const PermissionSet> requested_permissions =
+        PermissionSet::CreateDifference(optional_permissions.get(),
+                                        granted_permissions.get());
+    return GetMessages(requested_permissions);
+  }
+
+  void GrantOptionalPermissions() {
+    PermissionsUpdater perms_updater(profile());
+    perms_updater.AddPermissions(
+        app_.get(),
+        PermissionsParser::GetOptionalPermissions(app_.get()).get());
+  }
+
+  std::vector<base::string16> active_permissions() {
+    return GetMessages(app_->permissions_data()->active_permissions());
+  }
+
+  std::vector<base::string16> withheld_permissions() {
+    return GetMessages(app_->permissions_data()->withheld_permissions());
+  }
+
+  std::vector<base::string16> granted_permissions() {
+    return GetMessages(
+        ExtensionPrefs::Get(profile())->GetGrantedPermissions(app_->id()));
+  }
+
+  std::vector<base::string16> required_permissions() {
+    return GetMessages(PermissionsParser::GetRequiredPermissions(app_.get()));
+  }
+
+  std::vector<base::string16> optional_permissions() {
+    return GetMessages(PermissionsParser::GetOptionalPermissions(app_.get()));
+  }
+
+ private:
+  std::vector<base::string16> GetMessages(
+      scoped_refptr<const PermissionSet> permissions) {
+    return message_provider_->GetWarningMessages(permissions.get(),
+                                                 app_->GetType());
+  }
+
+  scoped_ptr<ChromePermissionMessageProvider> message_provider_;
+  scoped_refptr<const Extension> app_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionMessagesUnittest);
+};
+
+// If an app has both the 'serial' and 'USB' permission, they should coalesce
+// into a single permission message.
+TEST_F(PermissionMessagesUnittest, RequiredPermissionMessagesCoalesce) {
+  CreateAndInstallAppWithPermissions(
+      ListBuilder().Append("serial").Append("usb").Pass(),
+      ListBuilder().Pass());
+
+  ASSERT_EQ(1U, required_permissions().size());
+  EXPECT_EQ(l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL),
+            required_permissions()[0]);
+
+  ASSERT_EQ(0U, optional_permissions().size());
+}
+
+// If an app has both the 'history' and 'tabs' permission, one should hide the
+// other (the 'history' permission has superset permissions).
+TEST_F(PermissionMessagesUnittest, HistoryHidesTabsMessage) {
+  CreateAndInstallExtensionWithPermissions(
+      ListBuilder().Append("tabs").Append("history").Pass(),
+      ListBuilder().Pass());
+
+  ASSERT_EQ(1U, required_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      required_permissions()[0]);
+
+  ASSERT_EQ(0U, optional_permissions().size());
+}
+
+// If an app requests the 'history' permission, but already has the 'tabs'
+// permission, only the new coalesced message is displayed.
+TEST_F(PermissionMessagesUnittest, MixedPermissionMessagesCoalesceOnceGranted) {
+  CreateAndInstallExtensionWithPermissions(
+      ListBuilder().Append("tabs").Pass(),
+      ListBuilder().Append("history").Pass());
+
+  ASSERT_EQ(1U, required_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      required_permissions()[0]);
+
+  ASSERT_EQ(1U, optional_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      optional_permissions()[0]);
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      active_permissions()[0]);
+
+  ASSERT_EQ(1U, RequestOptionalPermissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      RequestOptionalPermissions()[0]);
+
+  GrantOptionalPermissions();
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      active_permissions()[0]);
+}
+
+// If an app requests the 'tabs' permission but already has the 'history'
+// permission, a prompt is displayed to request this permission, but it doesn't
+// appear in the final list of permissions since it is a subset of an already
+// granted permission.

[scheib, 2014/09/25 17:55:44]

Call out more explicitly what is 'AntiTest' about this. Why is it wrong, or what
should it do? Possibly prefix with something encouraging it to be fixed:
"""
AntiTest: This behavior should be changed and improved.
If an app requests 'tabs' permission, but already has 'history', a prompt is
displayed. Something should have happened instead. It should be like this other
way.
"""

[sashab, 2014/09/29 08:03:36]

Thanks. Updated both.

+TEST_F(PermissionMessagesUnittest,
+       AntiTest_PromptCanRequestSubsetOfAlreadyGrantedPermissions) {
+  CreateAndInstallExtensionWithPermissions(
+      ListBuilder().Append("history").Pass(),
+      ListBuilder().Append("tabs").Pass());
+
+  ASSERT_EQ(1U, required_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      required_permissions()[0]);
+
+  ASSERT_EQ(1U, optional_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      optional_permissions()[0]);
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      active_permissions()[0]);
+
+  // TODO(sashab): This prompt should display no permissions, since READ is a
+  // subset permission of WRITE.
+  ASSERT_EQ(1U, RequestOptionalPermissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      RequestOptionalPermissions()[0]);
+
+  GrantOptionalPermissions();
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      active_permissions()[0]);
+}
+
+// If an app requests the 'sessions' permission, nothing is displayed in the
+// permission request prompt, but the required permissions for the app are
+// actually modified.
+TEST_F(PermissionMessagesUnittest,
+       AntiTest_PromptCanBeEmptyButCausesChangeInPermissions) {
+  CreateAndInstallExtensionWithPermissions(
+      ListBuilder().Append("tabs").Pass(),
+      ListBuilder().Append("sessions").Pass());
+
+  ASSERT_EQ(1U, required_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      required_permissions()[0]);
+
+  ASSERT_EQ(0U, optional_permissions().size());
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+      active_permissions()[0]);
+
+  // TODO(sashab): This prompt should display the sessions permission message,
+  // as well as warn the user that it can affect the existing 'tab' permission.
+  ASSERT_EQ(0U, RequestOptionalPermissions().size());
+
+  GrantOptionalPermissions();
+
+  ASSERT_EQ(1U, active_permissions().size());
+  EXPECT_EQ(l10n_util::GetStringUTF16(
+                IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS),
+            active_permissions()[0]);
+}
+
+}  // namespace extensions