Added unit tests for ChromePermissionMessageProvider

chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc

Index: chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
diff --git a/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc b/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..26e803824293f0d2cbf62fb4919a5c422589dae1
--- /dev/null
+++ b/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
@@ -0,0 +1,333 @@
+// 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 "chrome/common/extensions/permissions/chrome_permission_message_provider.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.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/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"

[sashab, 2014/09/23 04:52:56]

Some of these #includes violate checkdeps rules:

You added one or more #includes that violate checkdeps rules.
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/extension_service.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/extension_service_test_base.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/permissions_updater.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/profiles/profile.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "extensions/browser/extension_prefs.h"
    Because of no rule applying. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "extensions/browser/extension_registry.h"
    Because of no rule applying.

Is this file exempt from those rules because its a test? Or is there somewhere
better this test should go?

[scheib, 2014/09/23 17:10:54]

Looking into this raises a few questions:

1. Should ChromePermissionMessageProvider be refactored out of common and into
browser? 5min of code search led me to believe this is possible, and
conceptually I don't see why  ChromePermissionMessageProvider is needed anywhere
but in the browser.

2. [Predicated on ChromePermissionMessageProvider staying in common] Decide if
these tests should be for just the  ChromePermissionMessageProvider or for the
larger system. As written now this patch is testing an ecosystem all the way
from a manifest, through PermissionsParser, to the
ChromePermissionMessageProvider. I can see an argument for more 'functional /
system' testing to increase coverage. If you want to do that, perhaps it is best
to place this test in some location with visibility for all that, e.g.
chrome/browser/extensions/permission_message_unit_tests ... and comment at the
top of ChromePermissionMessageProvider the location for tests.

The alternative is to rework these tests to really just unit test 
ChromePermissionMessageProvider, remove the other dependencies. I think this
would be fine for the types of tests I see so far, because the other systems are
really just handing over data but are not involved in much logic. But perhaps
I'm overlooking something, or there's value in future extensions to these tests
that would need to rely on the other systems. 

[sashab, 2014/09/24 23:23:49]

I'm actually really glad you brought this up - it seems I've written this test
in two minds, one from the permissions perspective and one as a unit test.

I'd like to go with Option 2, treating this as a unit test just for
ChromePermissionMessageProvider. Maybe we can refactor it into browser another
time, but for now it actually helped narrow the purpose of the test and simplify
it a lot :)

Edit: went ahead and just made two sets of tests - one as a unit test for the
message provider, one as an integration test. The use cases separate nicely this
way.

+
+namespace extensions {
+
+// Tests that ChromePermissionMessageProvider provides not only correct, but
+// meaningful permission messages.
+// There are two main types of permission messages generated that we need to
+// test:
+//  1. The full set of required permissions, displayed at install time (or when
+//     the app has been disabled automatically and needs to be re-enabled)
+//  2. The various combinations of optional permissions, displayed in a prompt
+//     when confirming the permission during runtime
+//  3. The split set of required/optional permissions, as displayed in the App
+//     Info dialog, where the optional permissions are individually revokable.
+// Some of these tests are currently 'anti-tests' - they demonstrate existing

[scheib, 2014/09/23 17:10:54]

Which ones? Can I search this file for TODO or 'AntiTest' or crbug.com to find
them?

[sashab, 2014/09/24 23:23:49]

Preceded them with 'AntiTest_'.

+// problematic functionality. These tests will be changed as the correct
+// behaviour is implemented.
+class ChromePermissionMessageProviderUnittest
+    : public ExtensionServiceTestBase {
+ public:
+  ChromePermissionMessageProviderUnittest()
+      : message_provider_(new ChromePermissionMessageProvider()) {}
+  virtual ~ChromePermissionMessageProviderUnittest() {}
+
+  // Overridden from testing::Test:
+  virtual void SetUp() OVERRIDE {
+    ExtensionServiceTestBase::SetUp();
+    InitializeExtensionService(CreateDefaultInitParams());
+    InitializeProcessManager();
+  }
+
+ protected:
+  // Overridden from views::WidgetObserver:
+  void InstallApp(Profile* profile, const Extension* app) {
+    service()->AddExtension(app);
+  }
+
+  void UninstallApp(const std::string& app_id) {
+    service()->UninstallExtension(app_id,
+                                  UninstallReason::UNINSTALL_REASON_FOR_TESTING,
+                                  base::Closure(),
+                                  NULL);
+  }
+
+  void CreateAppWithPermissions(const std::string id_seed,

[scheib, 2014/09/23 17:10:54]

Do we need an id_seed that varies between tests? Can this be simplified to just
always use a constant, or be omitted?

[sashab, 2014/09/24 23:23:48]

Now uses a constant.

+                                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(id_seed))
+               .SetLocation(Manifest::INTERNAL)
+               .Build();
+  }
+
+  void CreateExtensionWithPermissions(const std::string id_seed,

[scheib, 2014/09/23 17:10:54]

ditto for id_seed here

[sashab, 2014/09/24 23:23:49]

Done.

+                                      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(id_seed))
+               .SetLocation(Manifest::INTERNAL)
+               .Build();
+  }
+
+  scoped_refptr<PermissionSet> PermissionSetFromAPIPermissions(
+      APIPermissionSet& permissions) {
+    scoped_refptr<PermissionSet> permission_set = new PermissionSet(
+        permissions, ManifestPermissionSet(), URLPatternSet(), URLPatternSet());
+    return permission_set;
+  }
+
+  // Returns the permission messages that would display in the prompt that
+  // requests the given permissions for the current |app_|.
+  std::vector<base::string16> OptionalPermissionsRequestPromptMessages(
+      APIPermissionSet& permissions) {
+    scoped_refptr<PermissionSet> permission_set = new PermissionSet(
+        permissions, ManifestPermissionSet(), URLPatternSet(), URLPatternSet());
+    permission_set = PermissionSet::CreateDifference(
+        permission_set.get(), granted_permissions().get());
+    return message_provider_->GetWarningMessages(permission_set.get(),
+                                                 app_->GetType());
+  }
+
+  void GrantPermissions(scoped_refptr<const PermissionSet> permissions) {
+    PermissionsUpdater perms_updater(profile());
+    perms_updater.AddPermissions(app_.get(), permissions.get());
+  }
+
+  scoped_refptr<const PermissionSet> active_permissions() {
+    return app_->permissions_data()->active_permissions();
+  }
+
+  scoped_refptr<const PermissionSet> withheld_permissions() {
+    return app_->permissions_data()->withheld_permissions();
+  }
+
+  scoped_refptr<const PermissionSet> granted_permissions() {
+    return ExtensionPrefs::Get(profile())->GetGrantedPermissions(app_->id());
+  }
+
+  scoped_refptr<const PermissionSet> required_permissions() {
+    return PermissionsParser::GetRequiredPermissions(app_.get());
+  }
+
+  scoped_refptr<const PermissionSet> optional_permissions() {
+    return PermissionsParser::GetOptionalPermissions(app_.get());
+  }
+
+  std::vector<base::string16> messages(

[scheib, 2014/09/23 17:10:54]

Simplify test code by having the 'required_permissions()' and other methods
return the message strings formatted for assertions?

E.g. to read like:

ASSERT_EQ(1U, require_permissions().size());
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL),
          require_permissions()[0]));

[sashab, 2014/09/24 23:23:48]

Good idea. Done :)

+      scoped_refptr<const PermissionSet> permissions) {
+    return message_provider_->GetWarningMessages(permissions.get(),
+                                                 app_->GetType());
+  }
+
+  scoped_ptr<ChromePermissionMessageProvider> message_provider_;
+  scoped_refptr<const Extension> app_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromePermissionMessageProviderUnittest);
+};
+
+// If an app has both the 'serial' and 'USB' permission, they should coalesce
+// into a single permission message.
+TEST_F(ChromePermissionMessageProviderUnittest,
+       RequiredPermissionMessagesCoalesce) {
+  CreateAppWithPermissions("app",
+                           ListBuilder().Append("serial").Append("usb").Pass(),
+                           ListBuilder().Pass());
+  service()->AddExtension(app_.get());
+
+  std::vector<base::string16> required_permission_messages =
+      messages(required_permissions());
+  ASSERT_EQ(1U, required_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL),
+            base::UTF16ToUTF8(required_permission_messages[0]));
+
+  std::vector<base::string16> optional_permission_messages =
+      messages(optional_permissions());
+  ASSERT_EQ(0U, optional_permission_messages.size());
+}
+
+// If an app has both the 'history' and 'tabs' permission, one should hide the
+// other (the 'history' permission has superset permissions).
+TEST_F(ChromePermissionMessageProviderUnittest, HistoryHidesTabsMessage) {
+  CreateExtensionWithPermissions(
+      "app",
+      ListBuilder().Append("tabs").Append("history").Pass(),
+      ListBuilder().Pass());
+  service()->AddExtension(app_.get());
+
+  std::vector<base::string16> required_permission_messages =
+      messages(required_permissions());
+  ASSERT_EQ(1U, required_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(required_permission_messages[0]));
+
+  std::vector<base::string16> optional_permission_messages =
+      messages(optional_permissions());
+  ASSERT_EQ(0U, optional_permission_messages.size());
+}
+
+// If an app requests the 'history' permission, but already has the 'tabs'
+// permission, only the new coalesced message is displayed.
+TEST_F(ChromePermissionMessageProviderUnittest,
+       MixedPermissionMessagesCoalesceOnceGranted) {
+  CreateExtensionWithPermissions("app",
+                                 ListBuilder().Append("tabs").Pass(),
+                                 ListBuilder().Append("history").Pass());
+  service()->AddExtension(app_.get());
+
+  std::vector<base::string16> required_permission_messages =
+      messages(required_permissions());
+  ASSERT_EQ(1U, required_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(required_permission_messages[0]));
+
+  std::vector<base::string16> optional_permission_messages =
+      messages(optional_permissions());
+  ASSERT_EQ(1U, optional_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(optional_permission_messages[0]));
+
+  std::vector<base::string16> active_permission_messages =
+      messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(active_permission_messages[0]));
+
+  APIPermissionSet requested_optional_permissions;
+  requested_optional_permissions.insert(APIPermission::kHistory);
+  std::vector<base::string16> requested_optional_permission_messages =
+      OptionalPermissionsRequestPromptMessages(requested_optional_permissions);
+  ASSERT_EQ(1U, requested_optional_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(requested_optional_permission_messages[0]));
+
+  GrantPermissions(
+      PermissionSetFromAPIPermissions(requested_optional_permissions));
+
+  active_permission_messages = messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(active_permission_messages[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.
+TEST_F(ChromePermissionMessageProviderUnittest,
+       PromptCanRequestSubsetOfAlreadyGrantedPermissions) {
+  CreateExtensionWithPermissions("app",
+                                 ListBuilder().Append("history").Pass(),
+                                 ListBuilder().Append("tabs").Pass());
+  service()->AddExtension(app_.get());
+
+  std::vector<base::string16> required_permission_messages =
+      messages(required_permissions());
+  ASSERT_EQ(1U, required_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(required_permission_messages[0]));
+
+  std::vector<base::string16> optional_permission_messages =
+      messages(optional_permissions());
+  ASSERT_EQ(1U, optional_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(optional_permission_messages[0]));
+
+  std::vector<base::string16> active_permission_messages =
+      messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(active_permission_messages[0]));
+
+  APIPermissionSet requested_optional_permissions;
+  requested_optional_permissions.insert(APIPermission::kTab);
+  std::vector<base::string16> requested_optional_permission_messages =
+      OptionalPermissionsRequestPromptMessages(requested_optional_permissions);
+  // TODO(sashab): This prompt should display no permissions, since READ is a
+  // subset permission of WRITE.
+  ASSERT_EQ(1U, requested_optional_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(requested_optional_permission_messages[0]));
+
+  GrantPermissions(
+      PermissionSetFromAPIPermissions(requested_optional_permissions));
+
+  active_permission_messages = messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(
+      l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE),
+      base::UTF16ToUTF8(active_permission_messages[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(ChromePermissionMessageProviderUnittest,
+       PromptCanBeEmptyButCausesChangeInPermissions) {
+  CreateExtensionWithPermissions("app",
+                                 ListBuilder().Append("tabs").Pass(),
+                                 ListBuilder().Append("sessions").Pass());
+  service()->AddExtension(app_.get());
+
+  std::vector<base::string16> required_permission_messages =
+      messages(required_permissions());
+  ASSERT_EQ(1U, required_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(required_permission_messages[0]));
+
+  std::vector<base::string16> optional_permission_messages =
+      messages(optional_permissions());
+  ASSERT_EQ(0U, optional_permission_messages.size());
+
+  std::vector<base::string16> active_permission_messages =
+      messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ),
+            base::UTF16ToUTF8(active_permission_messages[0]));
+
+  APIPermissionSet requested_optional_permissions;
+  requested_optional_permissions.insert(APIPermission::kSessions);
+  std::vector<base::string16> requested_optional_permission_messages =
+      OptionalPermissionsRequestPromptMessages(requested_optional_permissions);
+  // 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, requested_optional_permission_messages.size());
+
+  GrantPermissions(
+      PermissionSetFromAPIPermissions(requested_optional_permissions));
+
+  active_permission_messages = messages(active_permissions());
+  ASSERT_EQ(1U, active_permission_messages.size());
+  EXPECT_EQ(l10n_util::GetStringUTF8(
+                IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ_AND_SESSIONS),
+            base::UTF16ToUTF8(active_permission_messages[0]));
+}
+
+}  // namespace extensions