[Extensions] CHECK() optional permissions when granting/removing

When granting and removing optional permissions from an extension in
PermissionsUpdater::GrantOptionalPermissions() and
PermissionsUpdater::RevokeOptionalPermissions(), CHECK() that the
permissions we're adding or removing are contained within the
optional permissions specified in the extension's manifest.

This required updating three unit tests that were exercising
improper behavior (trying to add or remove permissions that weren't
listed in the manifest).

Bug: None

Change-Id: I325ef2fb8c053b4ae7463aca36e6b4aba0ff4838
Reviewed-on: https://chromium-review.googlesource.com/c/1354651Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612818}

chrome/browser/extensions/permissions_updater.cc

@@ -237,12 +237,10 @@ void PermissionsUpdater::GrantOptionalPermissions(
     const Extension& extension,
     const PermissionSet& permissions,
     base::OnceClosure completion_callback) {
-  // TODO(devlin): Ideally, we'd have this CHECK in place, but unit tests are
-  // currently violating it.
-  // CHECK(PermissionsParser::GetOptionalPermissions(&extension).Contains(
-  //     permissions))
-  //     << "Cannot add optional permissions that are not "
-  //     << "specified in the manifest.";
+  CHECK(PermissionsParser::GetOptionalPermissions(&extension)
+            .Contains(permissions))
+      << "Cannot add optional permissions that are not "
+      << "specified in the manifest.";
 
   // Granted optional permissions are stored in both the granted permissions (so
   // we don't later disable the extension when we check the active permissions
@@ -304,10 +302,10 @@ void PermissionsUpdater::RevokeOptionalPermissions(
     base::OnceClosure completion_callback) {
   // TODO(devlin): Ideally, we'd have this CHECK in place, but unit tests are
   // currently violating it.
-  // CHECK(PermissionsParser::GetOptionalPermissions(&extension).Contains(
-  //     permissions))
-  //     << "Cannot remove optional permissions that are not "
-  //     << "specified in the manifest.";
+  CHECK(PermissionsParser::GetOptionalPermissions(&extension)
+            .Contains(permissions))
+      << "Cannot remove optional permissions that are not "
+      << "specified in the manifest.";
 
   // Revoked optional permissions are removed from granted and runtime-granted
   // permissions only if the user, and not the extension, removed them. This

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -159,7 +159,10 @@ TEST_F(PermissionsUpdaterTest, GrantAndRevokeOptionalPermissions) {
       ExtensionBuilder("permissions")
           .AddPermissions({"management", "http://a.com/*"})
           .SetManifestKey("optional_permissions",
-                          ListBuilder().Append("http://*.c.com/*").Build())
+                          ListBuilder()
+                              .Append("http://*.c.com/*")
+                              .Append("notifications")
+                              .Build())
           .Build();
 
   APIPermissionSet default_apis;
@@ -184,12 +187,10 @@ TEST_F(PermissionsUpdaterTest, GrantAndRevokeOptionalPermissions) {
   apis.insert(APIPermission::kNotifications);
   URLPatternSet hosts;
   AddPattern(&hosts, "http://*.c.com/*");
-  URLPatternSet scriptable_hosts;
-  AddPattern(&scriptable_hosts, "http://*.example.com/*");
 
   {
     PermissionSet delta(apis, empty_manifest_permissions, hosts,
-                        scriptable_hosts);
+                        URLPatternSet());
 
     PermissionsUpdaterListener listener;
     PermissionsUpdater(profile_.get())
@@ -223,7 +224,7 @@ TEST_F(PermissionsUpdaterTest, GrantAndRevokeOptionalPermissions) {
     // just added except for 'notifications'.
     apis.erase(APIPermission::kNotifications);
     PermissionSet delta(apis, empty_manifest_permissions, hosts,
-                        scriptable_hosts);
+                        URLPatternSet());
 
     PermissionsUpdaterListener listener;
     PermissionsUpdater(profile_.get())
@@ -631,8 +632,8 @@ TEST_F(PermissionsUpdaterTest, RevokingPermissionsWithRuntimeHostPermissions) {
 
     // Revoke the test site permission. The extension should no longer have
     // access to test site, and the revokable permissions should be empty.
-    permissions_test_util::RevokeOptionalPermissionsAndWaitForCompletion(
-        profile(), *extension, permission_set, PermissionsUpdater::REMOVE_HARD);
+    permissions_test_util::RevokeRuntimePermissionsAndWaitForCompletion(
+        profile(), *extension, permission_set);
     EXPECT_FALSE(extension->permissions_data()
                      ->active_permissions()
                      .HasExplicitAccessToOrigin(kOrigin));

chrome/browser/extensions/scripting_permissions_modifier_unittest.cc

@@ -454,13 +454,9 @@ TEST_F(ScriptingPermissionsModifierUnitTest,
 
   scoped_refptr<const Extension> extension =
       ExtensionBuilder("test")
-          .AddPermission("<all_urls>")
           .SetManifestKey("optional_permissions",
                           ListBuilder().Append("https://example.com/*").Build())
           .Build();
-  ScriptingPermissionsModifier modifier(profile(), extension.get());
-
-  modifier.SetWithholdHostPermissions(true);
 
   EXPECT_THAT(GetEffectivePatternsAsStrings(*extension), testing::IsEmpty());
 
@@ -478,6 +474,7 @@ TEST_F(ScriptingPermissionsModifierUnitTest,
   EXPECT_THAT(GetEffectivePatternsAsStrings(*extension),
               testing::UnorderedElementsAre("https://example.com/*"));
 
+  ScriptingPermissionsModifier modifier(profile(), extension.get());
   modifier.RemoveAllGrantedHostPermissions();
   EXPECT_THAT(GetEffectivePatternsAsStrings(*extension), testing::IsEmpty());
 }