Fix for broken "Load unpacked extension" popup

chrome/browser/chromeos/extensions/permissions_updater_delegate_chromeos_unittest.cc

 // Copyright 2017 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/browser/chromeos/extensions/permissions_updater_delegate_chromeos.h"
 
 #include <string>
 
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "chromeos/login/login_state.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/permissions/api_permission.h"
+#include "extensions/common/permissions/api_permission_set.h"
+#include "extensions/common/permissions/manifest_permission.h"
+#include "extensions/common/permissions/manifest_permission_set.h"
 #include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/url_pattern.h"
+#include "extensions/common/url_pattern_set.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
 namespace {
 
 const char kWhitelistedId[] = "cbkkbcmdlboombapidmoeolnmdacpkch";
 const char kBogusId[] = "bogus";
 
+class MockManifestPermission : public ManifestPermission {

[Ivan Šandrk, 2017/04/26 13:25:02]

I've c/p-ed this from manifest_permission_set_unittest. Maybe I should extract
it from there into mock_manifest_permission.cc/h so it's easier to reuse?

[Devlin, 2017/04/26 14:58:26]

That would work.  Another option would be to just use a "real" manifest
permission, e.g. SocketsManifestPermission.  I'm fine with either.

(And if you go with the extraction route, feel free to defer that to a separate
CL if you add a TODO)

[Ivan Šandrk, 2017/04/26 17:04:43]

TODO it is.

+ public:
+  MockManifestPermission(const std::string& name)
+      : name_(name) {
+  }
+
+  std::string name() const override { return name_; }
+
+  std::string id() const override { return name(); }
+
+  PermissionIDSet GetPermissions() const override { return PermissionIDSet(); }
+
+  bool FromValue(const base::Value* value) override { return true; }
+
+  std::unique_ptr<base::Value> ToValue() const override {
+    return base::MakeUnique<base::Value>();
+  }
+
+  ManifestPermission* Diff(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return NULL;
+  }
+
+  ManifestPermission* Union(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return new MockManifestPermission(name_);
+  }
+
+  ManifestPermission* Intersect(const ManifestPermission* rhs) const override {
+    const MockManifestPermission* other =
+        static_cast<const MockManifestPermission*>(rhs);
+    EXPECT_EQ(name_, other->name_);
+    return new MockManifestPermission(name_);
+  }
+
+ private:
+  std::string name_;
+};
+
 scoped_refptr<Extension> CreateExtension(const std::string& id) {
   std::string error;
   base::DictionaryValue manifest;
   manifest.SetString(manifest_keys::kName, "test");
   manifest.SetString(manifest_keys::kVersion, "0.1");
   scoped_refptr<Extension> extension = Extension::Create(
       base::FilePath(),
       Manifest::INTERNAL,
       manifest,
       Extension::NO_FLAGS,
       id,
       &error);
   return extension;
 }
 
 std::unique_ptr<const PermissionSet> CreatePermissions() {
   APIPermissionSet apis;
   apis.insert(APIPermission::kAudio);
   apis.insert(APIPermission::kClipboardRead);
   apis.insert(APIPermission::kFullscreen);
+  ManifestPermissionSet manifest;
+  manifest.insert(new MockManifestPermission("author"));
+  manifest.insert(new MockManifestPermission("background"));
+  URLPatternSet explicit_hosts({
+      URLPattern(URLPattern::SCHEME_ALL, "http://www.google.com/*"),
+      URLPattern(URLPattern::SCHEME_ALL, "<all_urls>")});
+  URLPatternSet scriptable_hosts({
+    URLPattern(URLPattern::SCHEME_ALL, "http://www.wikipedia.com/*")});
   auto permissions = base::MakeUnique<const PermissionSet>(
-      apis, ManifestPermissionSet(),
-      URLPatternSet(), URLPatternSet());
+      apis, manifest, explicit_hosts, scriptable_hosts);
   return permissions;
 }
 
 }  // namespace
 
 TEST(PermissionsUpdaterDelegateChromeOSTest, NoFilteringOutsidePublicSession) {
   PermissionsUpdaterDelegateChromeOS delegate;
   ASSERT_FALSE(chromeos::LoginState::IsInitialized());
 
   // Whitelisted extension outside PS, nothing filtered.
@@ skipping 17 matching lines @@
   chromeos::LoginState::Get()->SetLoggedInState(
     chromeos::LoginState::LOGGED_IN_ACTIVE,
     chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
 
   // Whitelisted extension, nothing gets filtered.
   auto extension = CreateExtension(kWhitelistedId);
   auto granted_permissions = CreatePermissions();
   delegate.InitializePermissions(extension.get(), &granted_permissions);
   EXPECT_EQ(*CreatePermissions(), *granted_permissions);
 
-  // Bogus extension ID (never whitelisted), ClipboardRead filtered out.
+  // Bogus extension ID (never whitelisted), ClipboardRead filtered out,
+  // everything else stays.
   extension = CreateExtension(kBogusId);
   granted_permissions = CreatePermissions();
   delegate.InitializePermissions(extension.get(), &granted_permissions);
-  EXPECT_FALSE(granted_permissions->HasAPIPermission(
-                   APIPermission::kClipboardRead));
-  EXPECT_EQ(2u, granted_permissions->apis().size());
+  APIPermissionSet apis;
+  apis.insert(APIPermission::kClipboardRead);
+  auto expected_permissions = PermissionSet::CreateDifference(

[Devlin, 2017/04/26 14:58:26]

nitty nit: This mirrors the code a bit too much. :)  I think this would be
cleaner when we construct each set individually and omit clipboard in one of
them, e.g. (pseudo-code):

PermissionSet original = {clipboard, other1, other2};
PermissionSet expected = {other1, other2};

so that it's clear what the difference is.

Given creating the set is more lines of code than we'd want to dupe, I'd suggest
having CreatePermissions take a bool include_clipboard, so that then we do:
PermissionSet original = CreatePermissions(true);
...
EXPECT_EQ(CreatePermissions(false), *granted);

WDYT?

(I realize that fundamentally it's the same, but IMHO this makes it a bit more
clear what exactly the expected difference is)

[Ivan Šandrk, 2017/04/26 17:04:44]

Good point about the mirroring, I actually had a weird feeling about it.

Good suggestion, done.

I'm using a default parameter on CreatePermissions, do you think that's ok or
maybe bad for some reason?

[Devlin, 2017/04/26 18:34:52]

We *used* to have strong rules about not allowing default values, but they've
since been lifted and these are now allowed, but with scrutiny.  Given this
function is local to this file, this would be fine.

My personal preference would be to require the parameter, since it's used in
relatively few places, but that's entirely subjective, so feel free to do
whichever you prefer. :)

+      *CreatePermissions(),
+      PermissionSet(apis, ManifestPermissionSet(),
+                    URLPatternSet(), URLPatternSet()));
+  EXPECT_EQ(*expected_permissions, *granted_permissions);
 
   // Reset state at the end of test.
   chromeos::LoginState::Shutdown();
 }
 
 }  // namespace extensions