Extensions: Switch to new permission message system, part II

chrome/common/extensions/permissions/permission_set_unittest.cc

 // Copyright (c) 2012 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/command_line.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_test_util.h"
 #include "chrome/common/extensions/features/feature_channel.h"
 #include "chrome/common/extensions/permissions/chrome_permission_message_provider.h"
 #include "chrome/grit/generated_resources.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
@@ skipping 763 matching lines @@
     if (skip.count(i->id())) {
       EXPECT_EQ(PermissionMessage::kNone, permission_info->message_id())
           << "unexpected message_id for " << permission_info->name();
     } else {
       EXPECT_NE(PermissionMessage::kNone, permission_info->message_id())
           << "missing message_id for " << permission_info->name();
     }
   }
 }
 
+PermissionIDSet MakePermissionIDSet(APIPermission::ID id1) {

[Devlin, 2015/04/02 21:00:23]

Can you move all of these to the anonymous namespace above?

[Marc Treib, 2015/04/07 11:34:47]

Done.

+  PermissionIDSet set;
+  set.insert(id1);
+  return set;
+}
+
+PermissionIDSet MakePermissionIDSet(APIPermission::ID id1,
+                                    APIPermission::ID id2) {
+  PermissionIDSet set;
+  set.insert(id1);
+  set.insert(id2);
+  return set;
+}
+
+PermissionIDSet MakePermissionIDSet(const APIPermissionSet& permissions) {
+  PermissionIDSet set;
+  for (const APIPermission* permission : permissions)
+    set.insert(permission->id());
+  return set;
+}
+
+std::string LegacyPermissionIDsToString(const PermissionMessageIDs& ids) {
+  if (ids.empty())

[Devlin, 2015/04/02 21:00:23]

nit: JoinString just returns "" if there are no strings, so you don't need the
if (empty) here or below.

[Marc Treib, 2015/04/07 11:34:47]

Okay, removed. We'll get some extra spaces in the brackets this way, but I guess
that doesn't really matter.

+    return "[]";
+  std::vector<std::string> strs;
+  for (const PermissionMessage::ID& id : ids)
+    strs.push_back(base::IntToString(id));
+  return "[ " + JoinString(strs, ", ") + " ]";

[Devlin, 2015/04/02 21:00:23]

nit: please use base::StringPrintf() (as a rule of thumb, any time it needs more
than one '+')

[Marc Treib, 2015/04/07 11:34:47]

Done.

+}
+
+std::string PermissionIDsToString(const PermissionIDSet& ids) {
+  if (ids.empty())
+    return "[]";
+  std::vector<std::string> strs;
+  for (const PermissionID& id : ids)
+    strs.push_back(base::IntToString(id.id()));
+  return "[ " + JoinString(strs, ", ") + " ]";
+}
+
+std::string CoalescedPermissionIDsToString(
+    const CoalescedPermissionMessages& msgs) {
+  std::vector<std::string> strs;
+  for (const CoalescedPermissionMessage& msg : msgs)
+    strs.push_back(PermissionIDsToString(msg.permissions()));
+  return JoinString(strs, " ");
+}
+
+testing::AssertionResult CheckPermissionIDs(

[Devlin, 2015/04/02 21:00:23]

This really only pertains to situations with Hidden/Suppressed permissions,
right? (otherwise we'd need to handle multiple permission messages.)  Should the
name of the function reflect as much?

[Devlin, 2015/04/02 21:00:23]

Add function comments.

[Marc Treib, 2015/04/07 11:34:47]

Done.

[Marc Treib, 2015/04/07 11:34:47]

I've renamed it to PermissionSetProducesMessage. Open for better suggestions :)

+    const PermissionSet* permissions,
+    Manifest::Type extension_type,
+    PermissionMessage::ID expected_legacy_id,
+    const PermissionIDSet& expected_ids) {
+  PermissionMessageIDs legacy_ids =
+      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
+          permissions, extension_type);
+  if (legacy_ids.size() != 1 || expected_legacy_id != legacy_ids[0]) {
+    return testing::AssertionFailure()
+           << "Expected single legacy permission ID " << expected_legacy_id

[Devlin, 2015/04/02 21:00:23]

indentation off? (Also at other returns)

[Marc Treib, 2015/04/07 11:34:47]

This is what "git cl format" produced (weird though it looks), and presubmit
even complains if I format it differently.

[Devlin, 2015/04/07 15:55:56]

:_( Why 8-space indentation?  Grrr.... Well, if git cl format does it.

[Marc Treib, 2015/04/07 16:15:55]

I think there's some special case thingie to line stuff up after the "return"...

+           << " but got " << LegacyPermissionIDsToString(legacy_ids);
+  }
+
+  const PermissionMessageProvider* provider = PermissionMessageProvider::Get();

[Devlin, 2015/04/02 21:00:23]

Cache this above, since you also use it on line 845

[Marc Treib, 2015/04/07 11:34:47]

Done.

+  CoalescedPermissionMessages msgs = provider->GetCoalescedPermissionMessages(
+      provider->GetAllPermissionIDs(permissions, extension_type));
+  if (msgs.size() != 1) {
+    return testing::AssertionFailure()
+           << "Expected single permission message with IDs "
+           << PermissionIDsToString(expected_ids) << " but got " << msgs.size()
+           << " messages: " << CoalescedPermissionIDsToString(msgs);
+  }
+  if (!msgs.front().permissions().Equals(expected_ids)) {
+    return testing::AssertionFailure()
+           << "Expected permission IDs " << PermissionIDsToString(expected_ids)
+           << " but got " << PermissionIDsToString(msgs.front().permissions());
+  }
+
+  return testing::AssertionSuccess();
+}
+
 TEST(PermissionsTest, FileSystemPermissionMessages) {
   APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemWrite);
   api_permissions.insert(APIPermission::kFileSystemDirectory);
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, ManifestPermissionSet(),
                         URLPatternSet(), URLPatternSet()));
-  PermissionMessageIDs ids =
-      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-          permissions.get(), Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, ids.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemDirectory, ids[0]);
+  EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_PLATFORM_APP,
+                                 PermissionMessage::kFileSystemDirectory,
+                                 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();
 
@@ skipping 22 matching lines @@
 }
 
 TEST(PermissionsTest, HiddenFileSystemPermissionMessages) {
   APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemWrite);
   api_permissions.insert(APIPermission::kFileSystemDirectory);
   api_permissions.insert(APIPermission::kFileSystemWriteDirectory);
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, ManifestPermissionSet(),
                         URLPatternSet(), URLPatternSet()));
-  PermissionMessageIDs ids =
-      PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-          permissions.get(), Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, ids.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemWriteDirectory, ids[0]);
+  EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_PLATFORM_APP,
+                                 PermissionMessage::kFileSystemWriteDirectory,
+                                 MakePermissionIDSet(api_permissions)));
 }
 
 TEST(PermissionsTest, SuppressedPermissionMessages) {
   {
     // Tabs warning suppresses favicon warning.
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kTab);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI,
                                 "chrome://favicon/"));
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kTabs, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION, PermissionMessage::kTabs,
+        MakePermissionIDSet(APIPermission::kTab, APIPermission::kFavicon)));
   }
   {
     // History warning suppresses favicon warning.
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kHistory);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI,
                                 "chrome://favicon/"));
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kBrowsingHistory, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kBrowsingHistory,
+        MakePermissionIDSet(APIPermission::kHistory, APIPermission::kFavicon)));
   }
   {
     // All sites warning suppresses tabs warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kTab);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kTab);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kHostsAll,
+        MakePermissionIDSet(APIPermission::kHostsAll, APIPermission::kTab)));
   }
   {
     // All sites warning suppresses topSites warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kTopSites);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kTopSites);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(
+        CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                           PermissionMessage::kHostsAll,
+                           MakePermissionIDSet(APIPermission::kHostsAll,
+                                               APIPermission::kTopSites)));
   }
   {
     // All sites warning suppresses declarativeWebRequest warning.
     APIPermissionSet api_permissions;
+    api_permissions.insert(APIPermission::kDeclarativeWebRequest);
     URLPatternSet hosts;
     hosts.AddPattern(URLPattern(URLPattern::SCHEME_CHROMEUI, "*://*/*"));
-    api_permissions.insert(APIPermission::kDeclarativeWebRequest);
     scoped_refptr<PermissionSet> permissions(new PermissionSet(
         api_permissions, ManifestPermissionSet(), hosts, URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kHostsAll, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(
+        permissions.get(), Manifest::TYPE_EXTENSION,
+        PermissionMessage::kHostsAll,
+        MakePermissionIDSet(APIPermission::kHostsAll,
+                            APIPermission::kDeclarativeWebRequest)));
   }
   {
     // BrowsingHistory warning suppresses all history read/write warnings.
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kHistory);
     api_permissions.insert(APIPermission::kTab);
     api_permissions.insert(APIPermission::kTopSites);
     api_permissions.insert(APIPermission::kProcesses);
     api_permissions.insert(APIPermission::kWebNavigation);
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           URLPatternSet(), URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kBrowsingHistory, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                                   PermissionMessage::kBrowsingHistory,
+                                   MakePermissionIDSet(api_permissions)));
   }
   {
     // Tabs warning suppresses all read-only history warnings.
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kTab);
     api_permissions.insert(APIPermission::kTopSites);
     api_permissions.insert(APIPermission::kProcesses);
     api_permissions.insert(APIPermission::kWebNavigation);
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions, ManifestPermissionSet(),
                           URLPatternSet(), URLPatternSet()));
-    PermissionMessageIDs ids =
-        PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
-            permissions.get(), Manifest::TYPE_EXTENSION);
-    EXPECT_EQ(1u, ids.size());
-    EXPECT_EQ(PermissionMessage::kTabs, ids[0]);
+    EXPECT_TRUE(CheckPermissionIDs(permissions.get(), Manifest::TYPE_EXTENSION,
+                                   PermissionMessage::kTabs,
+                                   MakePermissionIDSet(api_permissions)));
   }
 }
 
 TEST(PermissionsTest, AccessToDevicesMessages) {
   {
     APIPermissionSet api_permissions;
     api_permissions.insert(APIPermission::kSerial);
     scoped_refptr<PermissionSet> permissions(
         new PermissionSet(api_permissions,
                           ManifestPermissionSet(),
@@ skipping 742 matching lines @@
       URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/"));
   allowed_hosts.AddPattern(
       URLPattern(URLPattern::SCHEME_ALL, "chrome://favicon/"));
   allowed_hosts.AddPattern(
       URLPattern(URLPattern::SCHEME_ALL, "chrome://thumb/"));
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(APIPermissionSet(), ManifestPermissionSet(),
                         allowed_hosts, URLPatternSet()));
   PermissionMessageProvider::Get()->GetLegacyPermissionMessageIDs(
       permissions.get(), Manifest::TYPE_EXTENSION);
+  PermissionMessageProvider::Get()->GetCoalescedPermissionMessages(
+      PermissionMessageProvider::Get()->GetAllPermissionIDs(
+          permissions.get(), Manifest::TYPE_EXTENSION));
 }
 
 TEST(PermissionsTest, IsPrivilegeIncrease_DeclarativeWebRequest) {
   scoped_refptr<Extension> extension(
       LoadManifest("permissions", "permissions_all_urls.json"));
   scoped_refptr<const PermissionSet> permissions(
       extension->permissions_data()->active_permissions());
 
   scoped_refptr<Extension> extension_dwr(
       LoadManifest("permissions", "web_request_all_host_permissions.json"));
@@ skipping 13 matching lines @@
   apis.insert(APIPermission::kPrinterProvider);
   EXPECT_TRUE(extension->is_platform_app());
   EXPECT_TRUE(extension->permissions_data()->HasAPIPermission(
       APIPermission::kPrinterProvider));
   EXPECT_TRUE(VerifyOnePermissionMessage(
       extension->permissions_data(),
       "Add printing destinations and access any jobs you submit to them"));
 }
 
 }  // namespace extensions