[Extensions] Add an install warning for redundant optional permissions

Add an install warning for the case when an extension requests optional
permissions that are also listed as required permissions. Currently,
only do this for API permissions; a follow-up will handle host
permissions.

Bug: 859959
Change-Id: Ic9dc1a834e3e597aa12fe4136f4f087c6295400f
Reviewed-on: https://chromium-review.googlesource.com/c/1265118
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599379}

chrome/browser/extensions/api/permissions/permissions_apitest.cc

@@ -108,6 +108,16 @@ IN_PROC_BROWSER_TEST_F(PermissionsApiTest, OptionalPermissionsAutoConfirm) {
 
 // Test that denying the optional permissions confirmation dialog works.
 IN_PROC_BROWSER_TEST_F(PermissionsApiTest, OptionalPermissionsDeny) {
+  // Mark the management permission as already granted since we auto reject
+  // user prompts.
+  APIPermissionSet apis;
+  apis.insert(APIPermission::kManagement);
+
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(browser()->profile());
+  prefs->AddGrantedPermissions("kjmkgkdkpedkejedfhmfcenooemhbpbo",
+                               PermissionSet(apis, ManifestPermissionSet(),
+                                             URLPatternSet(), URLPatternSet()));
+
   PermissionsRequestFunction::SetAutoConfirmForTests(false);
   PermissionsRequestFunction::SetIgnoreUserGestureForTests(true);
   ASSERT_TRUE(StartEmbeddedTestServer());

chrome/common/extensions/manifest_tests/permissions_parser_unittest.cc

+// Copyright 2018 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 "extensions/common/manifest_handlers/permissions_parser.h"
+#include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
+#include "extensions/common/error_utils.h"
+#include "extensions/common/manifest_constants.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+using PermissionsParserTest = ChromeManifestTest;
+
+TEST_F(PermissionsParserTest, RemoveOverlappingAPIPermissions) {
+  scoped_refptr<Extension> extension(LoadAndExpectWarning(
+      "permissions_parser_overlapping_permissions.json",
+      ErrorUtils::FormatErrorMessage(
+          manifest_errors::kPermissionMarkedOptionalAndRequired, "tabs")));
+
+  std::set<std::string> required_api_names =
+      PermissionsParser::GetRequiredPermissions(extension.get())
+          .GetAPIsAsStrings();
+
+  std::set<std::string> optional_api_names =
+      PermissionsParser::GetOptionalPermissions(extension.get())
+          .GetAPIsAsStrings();
+
+  // Check that required api permissions have not changed while "tabs" is
+  // removed from optional permissions as it is specified as required.
+  EXPECT_THAT(required_api_names,
+              testing::UnorderedElementsAre("tabs", "storage"));
+  EXPECT_THAT(optional_api_names, testing::UnorderedElementsAre("geolocation"));
+}
+
+}  // namespace extensions

chrome/test/BUILD.gn

@@ -3775,6 +3775,7 @@ test("unit_tests") {
       "../common/extensions/manifest_tests/extension_manifests_web_accessible_resources_unittest.cc",
       "../common/extensions/manifest_tests/extension_manifests_web_unittest.cc",
       "../common/extensions/manifest_tests/extension_manifests_webview_accessible_resources_unittest.cc",
+      "../common/extensions/manifest_tests/permissions_parser_unittest.cc",
       "../common/extensions/manifest_unittest.cc",
       "../common/extensions/permissions/chrome_permission_message_provider_unittest.cc",
       "../common/extensions/permissions/chrome_permission_message_rules_unittest.cc",

chrome/test/data/extensions/api_test/permissions/optional/background.js

@@ -103,13 +103,6 @@ chrome.test.getConfig(function(config) {
       }));
     },
 
-    // Nothing should happen if we request permission we already have
-    function requestNoOp() {
-      chrome.permissions.request(
-          {permissions:['management'], origins:['http://a.com/*']},
-          pass(function(granted) { assertTrue(granted); }));
-    },
-
     // We should get an error when requesting permissions that haven't been
     // defined in "optional_permissions".
     function requestNonOptional() {
@@ -145,16 +138,6 @@ chrome.test.getConfig(function(config) {
       }));
     },
 
-    // These permissions should be on the granted list because they're on the
-    // extension's default permission list.
-    function requestGrantedPermission() {
-      chrome.permissions.request(
-          {permissions: ['management'], origins: ['http://a.com/*']},
-          pass(function(granted) {
-        assertTrue(granted);
-      }));
-    },
-
     // You can't remove required permissions.
     function removeRequired() {
       chrome.permissions.remove(
@@ -308,7 +291,7 @@ chrome.test.getConfig(function(config) {
       });
 
       chrome.permissions.request(
-          {permissions: ['bookmarks', 'management']}, pass(function(granted) {
+          {permissions: ['bookmarks']}, pass(function(granted) {
         assertTrue(granted);
         chrome.permissions.remove(
             {permissions: ['bookmarks']}, pass(function() {

chrome/test/data/extensions/api_test/permissions/optional/manifest.json

@@ -7,16 +7,14 @@
    "background": {
      "scripts": ["background.js"]
    },
-   "permissions": [ 
+   "permissions": [
      "management",
      "http://a.com/*"
    ],
    "optional_permissions": [
      "bookmarks",
      "cookies",
-     "management",
      "background",
-     "http://a.com/*",
      "http://*.c.com/*"
    ]
 }

chrome/test/data/extensions/api_test/permissions/optional_deny/manifest.json

 {
    "name": "permissions/optional_deny",
    "description": "permissions/optional_deny",
+   "key": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDkprt3BRSqoikAhSygI6VUzDLt18cKODYmkaa/dwPp4dboyz93RSB+v76grbqsNWrJjkrEwRD3QFeBYBq7h27XAMV4X5XvWjmWQBkRTBQyQI8cZd7M9dgfKrI3EqX9OJvd/wTJkC0dgF47nwWRa/Tvwl7Y66GwEEUjpn2MTv4klwIDAQAB",
    "version": "0.1",
    "manifest_version": 2,
    "background": {
      "scripts": ["background.js"]
    },
-   "permissions": ["http://a.com/*", "management"],
+   "permissions": ["http://a.com/*"],
    "optional_permissions": ["bookmarks", "management", "http://*.c.com/*"]
 }

chrome/test/data/extensions/manifest_tests/permissions_parser_overlapping_permissions.json

+{
+  "name": "Tabs both required and optional",
+  "version": "0.0.1.33",
+  "manifest_version": 2,
+  "description": "Extension with overlapping required and optional permissions",
+  "permissions": [
+    "tabs",
+    "storage"
+  ],
+  "optional_permissions": [
+    "tabs",
+    "geolocation"
+  ]
+}

extensions/common/manifest_constants.cc

@@ -715,10 +715,13 @@ const char kMissingFile[] =
 const char kMultipleOverrides[] =
     "An extension cannot override more than one page.";
 const char kNoWildCardsInPaths[] =
-  "Wildcards are not allowed in extent URL pattern paths.";
+    "Wildcards are not allowed in extent URL pattern paths.";
 const char kNPAPIPluginsNotSupported[] = "NPAPI plugins are not supported.";
 const char kOneUISurfaceOnly[] =
     "Only one of 'browser_action', 'page_action', and 'app' can be specified.";
+const char kPermissionMarkedOptionalAndRequired[] =
+    "Permission '*' cannot be specified as both required and optional in the "
+    "manifest; this permission will be treated as required.";
 const char kPermissionMustBeOptional[] =
     "Permission '*' must be specified in the optional section of the manifest.";
 const char kPermissionNotAllowed[] =

extensions/common/manifest_constants.h

@@ -487,6 +487,7 @@ extern const char kMultipleOverrides[];
 extern const char kNoWildCardsInPaths[];
 extern const char kNPAPIPluginsNotSupported[];
 extern const char kOneUISurfaceOnly[];
+extern const char kPermissionMarkedOptionalAndRequired[];
 extern const char kPermissionMustBeOptional[];
 extern const char kPermissionNotAllowed[];
 extern const char kPermissionNotAllowedInManifest[];

extensions/common/manifest_handlers/permissions_parser.cc

@@ -277,6 +277,39 @@ bool PermissionsParser::Parse(Extension* extension, base::string16* error) {
     return false;
   }
 
+  // If permissions are specified as both required and optional
+  // add an install warning for each permission and remove them from the
+  // optional set while keeping them in the required set.
+  APIPermissionSet overlap_permissions;
+  APIPermissionSet::Intersection(initial_required_permissions_->api_permissions,
+                                 initial_optional_permissions_->api_permissions,
+                                 &overlap_permissions);
+
+  if (!overlap_permissions.empty()) {
+    std::vector<InstallWarning> install_warnings;
+    install_warnings.reserve(overlap_permissions.size());
+
+    for (const auto* api_permission : overlap_permissions) {
+      install_warnings.emplace_back(
+          ErrorUtils::FormatErrorMessage(
+              manifest_errors::kPermissionMarkedOptionalAndRequired,
+              api_permission->name()),
+          keys::kOptionalPermissions, api_permission->name());
+    }
+
+    extension->AddInstallWarnings(std::move(install_warnings));
+
+    APIPermissionSet new_optional_api_permissions;
+    APIPermissionSet::Difference(initial_optional_permissions_->api_permissions,
+                                 initial_required_permissions_->api_permissions,
+                                 &new_optional_api_permissions);
+
+    initial_optional_permissions_->api_permissions =
+        new_optional_api_permissions;
+  }
+
+  // TODO(kelvinjiang): Use URLPatternSet to check for cases where a url pattern
+  // in required permissions contains another pattern in optional permissions.
   return true;
 }