[Extensions Click-to-Script] Refactor parsing in permissions API

Refactor the permissions parsing in the chrome.permissions API in order
to separate out requested permissions into different fields, including
required permissions, optional permissions, and permissions that were
not specified in the manifest.

Add more robust testing for the permissions parsing code.

This CL is prework for subsequently adding support for requesting
withheld content script permissions through the permissions API. There
should be no behavior change as a result of this CL.

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

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

@@ -25,6 +25,7 @@
 namespace extensions {
 
 using api::permissions::Permissions;
+using permissions_api_helpers::UnpackPermissionSetResult;
 
 namespace {
 
@@ -34,7 +35,7 @@ const char kCantRemoveRequiredPermissionsError[] =
     "You cannot remove required permissions.";
 const char kNotInManifestPermissionsError[] =
     "Only permissions specified in the manifest may be requested.";
-const char kNotWhitelistedError[] =
+const char kCannotBeOptionalError[] =
     "The optional permissions API does not support '*'.";
 const char kUserGestureRequiredError[] =
     "This function must be called during a user gesture";
@@ -54,19 +55,45 @@ ExtensionFunction::ResponseAction PermissionsContainsFunction::Run() {
       api::permissions::Contains::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
-  // NOTE: |permissions| is not used to make any security decisions. Therefore,
+  // NOTE: The result is not used to make any security decisions. Therefore,
   // it is entirely fine to set |allow_file_access| to true below. This will
   // avoid throwing error when extension() doesn't have access to file://.
+  constexpr bool kAllowFileAccess = true;
+
   std::string error;
-  std::unique_ptr<const PermissionSet> permissions =
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
       permissions_api_helpers::UnpackPermissionSet(
-          params->permissions, true /* allow_file_access */, &error);
-  if (!permissions.get())
+          params->permissions,
+          PermissionsParser::GetRequiredPermissions(extension()),
+          PermissionsParser::GetOptionalPermissions(extension()),
+          kAllowFileAccess, &error);
+
+  if (!unpack_result)
     return RespondNow(Error(error));
 
-  return RespondNow(ArgumentList(api::permissions::Contains::Results::Create(
-      extension()->permissions_data()->active_permissions().Contains(
-          *permissions))));
+  const PermissionSet& active_permissions =
+      extension()->permissions_data()->active_permissions();
+
+  bool has_all_permissions =
+      // An extension can never have an active permission that wasn't listed in
+      // the manifest, so we know it won't contain all permissions in
+      // |unpack_result| if there are any unlisted.
+      unpack_result->unlisted_apis.empty() &&
+      unpack_result->unlisted_hosts.is_empty() &&
+      // Unsupported optional permissions can never be granted, so we know if
+      // there are any specified, the extension doesn't actively have them.
+      unpack_result->unsupported_optional_apis.empty() &&
+      // Otherwise, check each expected location for whether it contains the
+      // relevant permissions.
+      active_permissions.apis().Contains(unpack_result->optional_apis) &&
+      active_permissions.apis().Contains(unpack_result->required_apis) &&
+      active_permissions.explicit_hosts().Contains(
+          unpack_result->optional_explicit_hosts) &&
+      active_permissions.explicit_hosts().Contains(
+          unpack_result->required_explicit_hosts);
+
+  return RespondNow(ArgumentList(
+      api::permissions::Contains::Results::Create(has_all_permissions)));
 }
 
 ExtensionFunction::ResponseAction PermissionsGetAllFunction::Run() {
@@ -83,22 +110,28 @@ ExtensionFunction::ResponseAction PermissionsRemoveFunction::Run() {
   EXTENSION_FUNCTION_VALIDATE(params);
 
   std::string error;
-  std::unique_ptr<const PermissionSet> permissions =
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
       permissions_api_helpers::UnpackPermissionSet(
           params->permissions,
+          PermissionsParser::GetRequiredPermissions(extension()),
+          PermissionsParser::GetOptionalPermissions(extension()),
           ExtensionPrefs::Get(browser_context())
               ->AllowFileAccess(extension_->id()),
           &error);
 
-  if (!permissions.get())
+  if (!unpack_result)
     return RespondNow(Error(error));
 
-  // Make sure the extension is only trying to remove permissions supported by
-  // this API.
-  APIPermissionSet apis = permissions->apis();
-  for (const APIPermission* permission : apis) {
-    if (!permission->info()->supports_optional())
-      return RespondNow(Error(kNotWhitelistedError, permission->name()));
+  // We can't remove any permissions that weren't specified in the manifest.
+  if (!unpack_result->unlisted_apis.empty() ||
+      !unpack_result->unlisted_hosts.is_empty()) {
+    return RespondNow(Error(kNotInManifestPermissionsError));
+  }
+
+  if (!unpack_result->unsupported_optional_apis.empty()) {
+    return RespondNow(
+        Error(kCannotBeOptionalError,
+              (*unpack_result->unsupported_optional_apis.begin())->name()));
   }
 
   // Make sure we only remove optional permissions, and not required
@@ -109,26 +142,25 @@ ExtensionFunction::ResponseAction PermissionsRemoveFunction::Run() {
   // NOTE(devlin): This won't support removal of required permissions that can
   // withheld. I don't think that will be a common use case, and so is probably
   // fine.
-  const PermissionSet& optional =
-      PermissionsParser::GetOptionalPermissions(extension());
-  const PermissionSet& required =
-      PermissionsParser::GetRequiredPermissions(extension());
-  if (!optional.Contains(*permissions) ||
-      !std::unique_ptr<const PermissionSet>(
-           PermissionSet::CreateIntersection(*permissions, required))
-           ->IsEmpty()) {
+  if (!unpack_result->required_apis.empty() ||
+      !unpack_result->required_explicit_hosts.is_empty()) {
     return RespondNow(Error(kCantRemoveRequiredPermissionsError));
   }
 
+  PermissionSet permissions(
+      unpack_result->optional_apis, ManifestPermissionSet(),
+      unpack_result->optional_explicit_hosts, URLPatternSet());
+
   // Only try and remove those permissions that are active on the extension.
   // For backwards compatability with behavior before this check was added, just
   // silently remove any that aren't present.
-  permissions = PermissionSet::CreateIntersection(
-      *permissions, extension()->permissions_data()->active_permissions());
+  std::unique_ptr<const PermissionSet> permissions_to_revoke =
+      PermissionSet::CreateIntersection(
+          permissions, extension()->permissions_data()->active_permissions());
 
   PermissionsUpdater(browser_context())
       .RevokeOptionalPermissions(
-          *extension(), *permissions, PermissionsUpdater::REMOVE_SOFT,
+          *extension(), *permissions_to_revoke, PermissionsUpdater::REMOVE_SOFT,
           base::BindOnce(
               &PermissionsRemoveFunction::Respond, base::RetainedRef(this),
               ArgumentList(api::permissions::Remove::Results::Create(true))));
@@ -171,31 +203,56 @@ ExtensionFunction::ResponseAction PermissionsRequestFunction::Run() {
   EXTENSION_FUNCTION_VALIDATE(params);
 
   std::string error;
-  std::unique_ptr<const PermissionSet> requested_permissions =
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
       permissions_api_helpers::UnpackPermissionSet(
           params->permissions,
+          PermissionsParser::GetRequiredPermissions(extension()),
+          PermissionsParser::GetOptionalPermissions(extension()),
           ExtensionPrefs::Get(browser_context())
               ->AllowFileAccess(extension_->id()),
           &error);
-  if (!requested_permissions.get())
+
+  if (!unpack_result)
     return RespondNow(Error(error));
 
-  // Make sure they're only requesting permissions supported by this API.
-  for (const auto* api_permission : requested_permissions->apis()) {
-    if (!api_permission->info()->supports_optional()) {
-      return RespondNow(Error(ErrorUtils::FormatErrorMessage(
-          kNotWhitelistedError, api_permission->name())));
-    }
+  // Don't allow the extension to request any permissions that weren't specified
+  // in the manifest.
+  if (!unpack_result->unlisted_apis.empty() ||
+      !unpack_result->unlisted_hosts.is_empty()) {
+    return RespondNow(Error(kNotInManifestPermissionsError));
   }
 
-  const PermissionsData* permissions_data = extension()->permissions_data();
-  // Subtract out any permissions already active on the extension.
-  std::unique_ptr<const PermissionSet> new_permissions =
-      PermissionSet::CreateDifference(*requested_permissions,
-                                      permissions_data->active_permissions());
+  if (!unpack_result->unsupported_optional_apis.empty()) {
+    return RespondNow(
+        Error(kCannotBeOptionalError,
+              (*unpack_result->unsupported_optional_apis.begin())->name()));
+  }
+
+  const PermissionSet& active_permissions =
+      extension()->permissions_data()->active_permissions();
+
+  // Determine which of the requested permissions are optional permissions that
+  // are "new", i.e. aren't already active on the extension.
+  requested_optional_ = std::make_unique<const PermissionSet>(
+      unpack_result->optional_apis, ManifestPermissionSet(),
+      unpack_result->optional_explicit_hosts, URLPatternSet());
+  requested_optional_ =
+      PermissionSet::CreateDifference(*requested_optional_, active_permissions);
+
+  // Do the same for withheld permissions.
+  requested_withheld_ = std::make_unique<const PermissionSet>(
+      APIPermissionSet(), ManifestPermissionSet(),
+      unpack_result->required_explicit_hosts, URLPatternSet());
+  requested_withheld_ =
+      PermissionSet::CreateDifference(*requested_withheld_, active_permissions);
+
+  // Determine the total "new" permissions; this is the set of all permissions
+  // that aren't currently active on the extension.
+  std::unique_ptr<const PermissionSet> total_new_permissions =
+      PermissionSet::CreateUnion(*requested_withheld_, *requested_optional_);
 
   // If all permissions are already active, nothing left to do.
-  if (new_permissions->IsEmpty()) {
+  if (total_new_permissions->IsEmpty()) {
     constexpr bool granted = true;
     return RespondNow(OneArgument(std::make_unique<base::Value>(granted)));
   }
@@ -203,42 +260,27 @@ ExtensionFunction::ResponseAction PermissionsRequestFunction::Run() {
   // Automatically declines api permissions requests, which are blocked by
   // enterprise policy.
   if (!ExtensionManagementFactory::GetForBrowserContext(browser_context())
-           ->IsPermissionSetAllowed(extension(), *new_permissions)) {
+           ->IsPermissionSetAllowed(extension(), *total_new_permissions)) {
     return RespondNow(Error(kBlockedByEnterprisePolicy));
   }
 
-  // Look for any permissions that were withheld. These previously would have
-  // been auto-granted to the extension, but are now waiting for deliberate
-  // user approval.
-  requested_withheld_ = PermissionSet::CreateIntersection(
-      *new_permissions, permissions_data->withheld_permissions());
-  // Assume any permissions that weren't withheld are being requested as
-  // optional permissions.
-  requested_optional_ =
-      PermissionSet::CreateDifference(*new_permissions, *requested_withheld_);
-
-  // Check that the remaining permissions (assumed to be optional) are specified
-  // in the manifest.
-  if (!PermissionsParser::GetOptionalPermissions(extension())
-           .Contains(*requested_optional_)) {
-    return RespondNow(Error(kNotInManifestPermissionsError));
-  }
+  // At this point, all permissions in |requested_withheld_| should be within
+  // the |withheld_permissions| section of the PermissionsData.
+  DCHECK(extension()->permissions_data()->withheld_permissions().Contains(
+      *requested_withheld_));
 
-  // Find the permissions to prompt the user for.
-  // We prompt for |requested_withheld_| +
-  // (|requested_optional_| - |already_granted_permissions|).
-  // We prompt for |requested_withheld_| since these were deliberately revoked
-  // by the user. We don't prompt for |already_granted_permissions|
-  // since these were either granted to an earlier extension version or removed
-  // by the extension itself (using the permissions.remove() method).
+  // Prompt the user for any new permissions that aren't contained within the
+  // already-granted permissions. We don't prompt for already-granted
+  // permissions since these were either granted to an earlier extension version
+  // or removed by the extension itself (using the permissions.remove() method).
   std::unique_ptr<const PermissionSet> granted_permissions =
       ExtensionPrefs::Get(browser_context())
           ->GetRuntimeGrantedPermissions(extension()->id());
   std::unique_ptr<const PermissionSet> already_granted_permissions =
       PermissionSet::CreateIntersection(*granted_permissions,
                                         *requested_optional_);
-  new_permissions = PermissionSet::CreateDifference(
-      *new_permissions, *already_granted_permissions);
+  total_new_permissions = PermissionSet::CreateDifference(
+      *total_new_permissions, *already_granted_permissions);
 
   // We don't need to show the prompt if there are no new warnings, or if
   // we're skipping the confirmation UI. COMPONENT extensions are allowed to
@@ -251,8 +293,7 @@ ExtensionFunction::ResponseAction PermissionsRequestFunction::Run() {
   bool has_no_warnings =
       message_provider
           ->GetPermissionMessages(message_provider->GetAllPermissionIDs(
-              // |new_permissions| all permissions that need user consent.
-              *new_permissions, extension()->GetType()))
+              *total_new_permissions, extension()->GetType()))
           .empty();
   if (has_no_warnings || extension_->location() == Manifest::COMPONENT) {
     OnInstallPromptDone(ExtensionInstallPrompt::Result::ACCEPTED);
@@ -262,7 +303,7 @@ ExtensionFunction::ResponseAction PermissionsRequestFunction::Run() {
   // Otherwise, we have to prompt the user (though we might "autoconfirm" for a
   // test.
   if (auto_confirm_for_tests != DO_NOT_SKIP) {
-    prompted_permissions_for_testing_ = new_permissions->Clone();
+    prompted_permissions_for_testing_ = total_new_permissions->Clone();
     if (auto_confirm_for_tests == PROCEED)
       OnInstallPromptDone(ExtensionInstallPrompt::Result::ACCEPTED);
     else if (auto_confirm_for_tests == ABORT)
@@ -278,9 +319,7 @@ ExtensionFunction::ResponseAction PermissionsRequestFunction::Run() {
       extension(), nullptr,
       std::make_unique<ExtensionInstallPrompt::Prompt>(
           ExtensionInstallPrompt::PERMISSIONS_PROMPT),
-      // |new_permissions| includes both |requested_optional_| and
-      // |requested_withheld_|.
-      std::move(new_permissions),
+      std::move(total_new_permissions),
       ExtensionInstallPrompt::GetDefaultShowDialogCallback());
 
   // ExtensionInstallPrompt::ShowDialog() can call the response synchronously.

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

@@ -35,8 +35,146 @@ const char kUnknownPermissionError[] =
 const char kUnsupportedPermissionId[] =
     "Only the usbDevices permission supports arguments.";
 
+// Extracts an API permission that supports arguments. In practice, this is
+// restricted to the UsbDevicePermission.
+std::unique_ptr<APIPermission> UnpackPermissionWithArguments(
+    base::StringPiece permission_name,
+    base::StringPiece permission_arg,
+    const std::string& permission_str,
+    std::string* error) {
+  std::unique_ptr<base::Value> permission_json =
+      base::JSONReader::Read(permission_arg);
+  if (!permission_json.get()) {
+    *error = ErrorUtils::FormatErrorMessage(kInvalidParameter, permission_str);
+    return nullptr;
+  }
+
+  std::unique_ptr<APIPermission> permission;
+
+  // Explicitly check the permissions that accept arguments until
+  // https://crbug.com/162042 is fixed.
+  const APIPermissionInfo* usb_device_permission_info =
+      PermissionsInfo::GetInstance()->GetByID(APIPermission::kUsbDevice);
+  if (permission_name == usb_device_permission_info->name()) {
+    permission =
+        std::make_unique<UsbDevicePermission>(usb_device_permission_info);
+  } else {
+    *error = kUnsupportedPermissionId;
+    return nullptr;
+  }
+
+  CHECK(permission);
+  if (!permission->FromValue(permission_json.get(), nullptr, nullptr)) {
+    *error = ErrorUtils::FormatErrorMessage(kInvalidParameter, permission_str);
+    return nullptr;
+  }
+
+  return permission;
+}
+
+// A helper method to unpack API permissions from the list in
+// |permissions_input|, and populate the appropriate fields of |result|.
+// Returns true on success; on failure, returns false and populates |error|.
+bool UnpackAPIPermissions(const std::vector<std::string>& permissions_input,
+                          const PermissionSet& required_permissions,
+                          const PermissionSet& optional_permissions,
+                          UnpackPermissionSetResult* result,
+                          std::string* error) {
+  PermissionsInfo* info = PermissionsInfo::GetInstance();
+  APIPermissionSet apis;
+  // Iterate over the inputs and find the corresponding API permissions.
+  for (const auto& permission_str : permissions_input) {
+    // This is a compromise: we currently can't switch to a blend of
+    // objects/strings all the way through the API. Until then, put this
+    // processing here.
+    // http://code.google.com/p/chromium/issues/detail?id=162042
+    size_t delimiter = permission_str.find(kDelimiter);
+    if (delimiter != std::string::npos) {
+      base::StringPiece permission_piece(permission_str);
+      std::unique_ptr<APIPermission> permission = UnpackPermissionWithArguments(
+          permission_piece.substr(0, delimiter),
+          permission_piece.substr(delimiter + 1), permission_str, error);
+      if (!permission)
+        return false;
+
+      apis.insert(std::move(permission));
+    } else {
+      const APIPermissionInfo* permission_info =
+          info->GetByName(permission_str);
+      if (!permission_info) {
+        *error = ErrorUtils::FormatErrorMessage(kUnknownPermissionError,
+                                                permission_str);
+        return false;
+      }
+      apis.insert(permission_info->id());
+    }
+  }
+
+  // Validate and partition the parsed APIs.
+  for (const auto* api_permission : apis) {
+    if (required_permissions.apis().count(api_permission->id())) {
+      result->required_apis.insert(api_permission->id());
+      continue;
+    }
+
+    if (!optional_permissions.apis().count(api_permission->id())) {
+      result->unlisted_apis.insert(api_permission->id());
+      continue;
+    }
+
+    if (!api_permission->info()->supports_optional()) {
+      result->unsupported_optional_apis.insert(api_permission->id());
+      continue;
+    }
+
+    result->optional_apis.insert(api_permission->id());
+  }
+
+  return true;
+}
+
+// A helper method to unpack host permissions from the list in
+// |permissions_input|, and populate the appropriate fields of |result|.
+// Returns true on success; on failure, returns false and populates |error|.
+bool UnpackOriginPermissions(const std::vector<std::string>& origins_input,
+                             const PermissionSet& required_permissions,
+                             const PermissionSet& optional_permissions,
+                             bool allow_file_access,
+                             UnpackPermissionSetResult* result,
+                             std::string* error) {
+  int explicit_schemes = Extension::kValidHostPermissionSchemes;
+  if (!allow_file_access)
+    explicit_schemes &= ~URLPattern::SCHEME_FILE;
+
+  for (const auto& origin_str : origins_input) {
+    URLPattern explicit_origin(explicit_schemes);
+    URLPattern::ParseResult parse_result = explicit_origin.Parse(origin_str);
+    if (URLPattern::ParseResult::kSuccess != parse_result) {
+      *error = ErrorUtils::FormatErrorMessage(
+          kInvalidOrigin, origin_str,
+          URLPattern::GetParseResultString(parse_result));
+      return false;
+    }
+
+    if (required_permissions.explicit_hosts().ContainsPattern(
+            explicit_origin)) {
+      result->required_explicit_hosts.AddPattern(explicit_origin);
+    } else if (optional_permissions.explicit_hosts().ContainsPattern(
+                   explicit_origin)) {
+      result->optional_explicit_hosts.AddPattern(explicit_origin);
+    } else {
+      result->unlisted_hosts.AddPattern(explicit_origin);
+    }
+  }
+
+  return true;
+}
+
 }  // namespace
 
+UnpackPermissionSetResult::UnpackPermissionSetResult() = default;
+UnpackPermissionSetResult::~UnpackPermissionSetResult() = default;
+
 std::unique_ptr<Permissions> PackPermissionSet(const PermissionSet& set) {
   std::unique_ptr<Permissions> permissions(new Permissions());
 
@@ -63,91 +201,34 @@ std::unique_ptr<Permissions> PackPermissionSet(const PermissionSet& set) {
   return permissions;
 }
 
-std::unique_ptr<const PermissionSet> UnpackPermissionSet(
-    const Permissions& permissions,
+std::unique_ptr<UnpackPermissionSetResult> UnpackPermissionSet(
+    const Permissions& permissions_input,
+    const PermissionSet& required_permissions,
+    const PermissionSet& optional_permissions,
     bool allow_file_access,
     std::string* error) {
   DCHECK(error);
-  APIPermissionSet apis;
-  std::vector<std::string>* permissions_list = permissions.permissions.get();
-  if (permissions_list) {
-    PermissionsInfo* info = PermissionsInfo::GetInstance();
-    for (auto it = permissions_list->begin(); it != permissions_list->end();
-         ++it) {
-      // This is a compromise: we currently can't switch to a blend of
-      // objects/strings all the way through the API. Until then, put this
-      // processing here.
-      // http://code.google.com/p/chromium/issues/detail?id=162042
-      if (it->find(kDelimiter) != std::string::npos) {
-        size_t delimiter = it->find(kDelimiter);
-        std::string permission_name = it->substr(0, delimiter);
-        std::string permission_arg = it->substr(delimiter + 1);
-
-        std::unique_ptr<base::Value> permission_json =
-            base::JSONReader::Read(permission_arg);
-        if (!permission_json.get()) {
-          *error = ErrorUtils::FormatErrorMessage(kInvalidParameter, *it);
-          return NULL;
-        }
-
-        std::unique_ptr<APIPermission> permission;
-
-        // Explicitly check the permissions that accept arguments until the bug
-        // referenced above is fixed.
-        const APIPermissionInfo* usb_device_permission_info =
-            info->GetByID(APIPermission::kUsbDevice);
-        if (permission_name == usb_device_permission_info->name()) {
-          permission =
-              std::make_unique<UsbDevicePermission>(usb_device_permission_info);
-        } else {
-          *error = kUnsupportedPermissionId;
-          return NULL;
-        }
-
-        CHECK(permission);
-        if (!permission->FromValue(permission_json.get(), NULL, NULL)) {
-          *error = ErrorUtils::FormatErrorMessage(kInvalidParameter, *it);
-          return NULL;
-        }
-        apis.insert(std::move(permission));
-      } else {
-        const APIPermissionInfo* permission_info = info->GetByName(*it);
-        if (!permission_info) {
-          *error = ErrorUtils::FormatErrorMessage(
-              kUnknownPermissionError, *it);
-          return NULL;
-        }
-        apis.insert(permission_info->id());
-      }
-    }
-  }
 
   // TODO(rpaquay): We currently don't expose manifest permissions
   // to apps/extensions via the permissions API.
-  ManifestPermissionSet manifest_permissions;
-
-  URLPatternSet origins;
-  if (permissions.origins.get()) {
-    for (auto it = permissions.origins->begin();
-         it != permissions.origins->end(); ++it) {
-      int allowed_schemes = Extension::kValidHostPermissionSchemes;
-      if (!allow_file_access)
-        allowed_schemes &= ~URLPattern::SCHEME_FILE;
-      URLPattern origin(allowed_schemes);
-      URLPattern::ParseResult parse_result = origin.Parse(*it);
-      if (URLPattern::ParseResult::kSuccess != parse_result) {
-        *error = ErrorUtils::FormatErrorMessage(
-            kInvalidOrigin,
-            *it,
-            URLPattern::GetParseResultString(parse_result));
-        return NULL;
-      }
-      origins.AddPattern(origin);
-    }
+
+  auto result = std::make_unique<UnpackPermissionSetResult>();
+
+  if (permissions_input.permissions &&
+      !UnpackAPIPermissions(*permissions_input.permissions,
+                            required_permissions, optional_permissions,
+                            result.get(), error)) {
+    return nullptr;
+  }
+
+  if (permissions_input.origins &&
+      !UnpackOriginPermissions(*permissions_input.origins, required_permissions,
+                               optional_permissions, allow_file_access,
+                               result.get(), error)) {
+    return nullptr;
   }
 
-  return std::make_unique<PermissionSet>(apis, manifest_permissions, origins,
-                                         URLPatternSet());
+  return result;
 }
 
 }  // namespace permissions_api_helpers

chrome/browser/extensions/api/permissions/permissions_api_helpers.h

@@ -9,6 +9,8 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
+#include "extensions/common/permissions/api_permission_set.h"
+#include "extensions/common/url_pattern_set.h"
 
 namespace extensions {
 
@@ -26,10 +28,43 @@ namespace permissions_api_helpers {
 std::unique_ptr<api::permissions::Permissions> PackPermissionSet(
     const PermissionSet& set);
 
-// Creates a permission set from |permissions|. Returns NULL if the permissions
-// cannot be converted to a permission set, in which case |error| will be set.
-std::unique_ptr<const PermissionSet> UnpackPermissionSet(
-    const api::permissions::Permissions& permissions,
+// The result of unpacking the API permissions object.
+struct UnpackPermissionSetResult {
+  UnpackPermissionSetResult();
+  ~UnpackPermissionSetResult();
+
+  // API permissions that are in the extension's "required" permission set.
+  APIPermissionSet required_apis;
+  // Explicit hosts that are in the extension's "required" permission set.
+  URLPatternSet required_explicit_hosts;
+  // TODO(devlin): Add scriptable host support.
+  // https://crbug.com/889654.
+
+  // API permissions that are in the extension's "optional" permission set.
+  APIPermissionSet optional_apis;
+  // API permissions that are in the extension's "optional" permission set,
+  // but don't support the optional permissions API.
+  APIPermissionSet unsupported_optional_apis;
+  // Explicit hosts that are in the extension's "optional" permission set.
+  URLPatternSet optional_explicit_hosts;
+
+  // API permissions that were not listed in the extension's permissions.
+  APIPermissionSet unlisted_apis;
+  // Host permissions that were not listed in the extension's permissions.
+  URLPatternSet unlisted_hosts;
+};
+
+// Parses the |permissions_input| object, and partitions permissions into the
+// result. |required_permissions| and |optional_permissions| are the required
+// and optional permissions specified in the extension's manifest, used for
+// separating permissions. |allow_file_access| is used to determine whether the
+// file:-scheme is valid for host permissions. If an error is detected (e.g.,
+// an unknown API permission, invalid URL pattern, or API that doesn't support
+// being optional), |error| is populated and null is returned.
+std::unique_ptr<UnpackPermissionSetResult> UnpackPermissionSet(
+    const api::permissions::Permissions& permissions_input,
+    const PermissionSet& required_permissions,
+    const PermissionSet& optional_permissions,
     bool allow_file_access,
     std::string* error);
 

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

@@ -11,15 +11,20 @@
 
 #include "base/macros.h"
 #include "base/values.h"
+#include "chrome/browser/extensions/permissions_test_util.h"
 #include "chrome/common/extensions/api/permissions.h"
+#include "extensions/common/extension.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/url_pattern_set.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 using extensions::api::permissions::Permissions;
 using extensions::permissions_api_helpers::PackPermissionSet;
 using extensions::permissions_api_helpers::UnpackPermissionSet;
+using extensions::permissions_api_helpers::UnpackPermissionSetResult;
+using extensions::permissions_test_util::GetPatternsAsStrings;
 
 namespace extensions {
 
@@ -32,7 +37,7 @@ static void AddPattern(URLPatternSet* extent, const std::string& pattern) {
 
 }  // namespace
 
-// Tests that we can convert PermissionSets to and from values.
+// Tests that we can convert PermissionSets to the generated types.
 TEST(ExtensionPermissionsAPIHelpers, Pack) {
   APIPermissionSet apis;
   apis.insert(APIPermission::kTab);
@@ -42,48 +47,23 @@ TEST(ExtensionPermissionsAPIHelpers, Pack) {
   AddPattern(&hosts, "http://a.com/*");
   AddPattern(&hosts, "http://b.com/*");
 
-  PermissionSet permission_set(apis, ManifestPermissionSet(), hosts,
-                               URLPatternSet());
-
   // Pack the permission set to value and verify its contents.
-  std::unique_ptr<Permissions> permissions(PackPermissionSet(permission_set));
-  std::unique_ptr<base::DictionaryValue> value(permissions->ToValue());
-  base::ListValue* api_list = NULL;
-  base::ListValue* origin_list = NULL;
-  EXPECT_TRUE(value->GetList("permissions", &api_list));
-  EXPECT_TRUE(value->GetList("origins", &origin_list));
-
-  EXPECT_EQ(3u, api_list->GetSize());
-  EXPECT_EQ(2u, origin_list->GetSize());
-
-  std::string expected_apis[] = {"tabs", "fileBrowserHandler",
-                                 "fileBrowserHandlerInternal"};
-  for (size_t i = 0; i < arraysize(expected_apis); ++i) {
-    std::unique_ptr<base::Value> value(new base::Value(expected_apis[i]));
-    EXPECT_NE(api_list->end(), api_list->Find(*value));
-  }
-
-  std::string expected_origins[] = { "http://a.com/*", "http://b.com/*" };
-  for (size_t i = 0; i < arraysize(expected_origins); ++i) {
-    std::unique_ptr<base::Value> value(new base::Value(expected_origins[i]));
-    EXPECT_NE(origin_list->end(), origin_list->Find(*value));
-  }
-
-  // Unpack the value back to a permission set and make sure its equal to the
-  // original one.
-  std::string error;
-  Permissions permissions_object;
-  EXPECT_TRUE(Permissions::Populate(*value, &permissions_object));
-  std::unique_ptr<const PermissionSet> from_value =
-      UnpackPermissionSet(permissions_object, true, &error);
-  EXPECT_TRUE(error.empty());
+  std::unique_ptr<Permissions> pack_result(PackPermissionSet(
+      PermissionSet(apis, ManifestPermissionSet(), hosts, URLPatternSet())));
+  ASSERT_TRUE(pack_result);
+  ASSERT_TRUE(pack_result->permissions);
+  EXPECT_THAT(*pack_result->permissions,
+              testing::UnorderedElementsAre("tabs", "fileBrowserHandler",
+                                            "fileBrowserHandlerInternal"));
 
-  EXPECT_EQ(permission_set, *from_value);
+  ASSERT_TRUE(pack_result->origins);
+  EXPECT_THAT(*pack_result->origins, testing::UnorderedElementsAre(
+                                         "http://a.com/*", "http://b.com/*"));
 }
 
 // Tests various error conditions and edge cases when unpacking values
 // into PermissionSets.
-TEST(ExtensionPermissionsAPIHelpers, Unpack) {
+TEST(ExtensionPermissionsAPIHelpers, Unpack_Basic) {
   std::unique_ptr<base::ListValue> apis(new base::ListValue());
   apis->AppendString("tabs");
   std::unique_ptr<base::ListValue> origins(new base::ListValue());
@@ -93,15 +73,27 @@ TEST(ExtensionPermissionsAPIHelpers, Unpack) {
   std::unique_ptr<const PermissionSet> permissions;
   std::string error;
 
+  APIPermissionSet optional_apis;
+  optional_apis.insert(APIPermission::kTab);
+  URLPatternSet optional_explicit_hosts(
+      {URLPattern(Extension::kValidHostPermissionSchemes, "http://a.com/*")});
+  PermissionSet optional_permissions(optional_apis, ManifestPermissionSet(),
+                                     optional_explicit_hosts, URLPatternSet());
+
   // Origins shouldn't have to be present.
   {
     Permissions permissions_object;
     value->Set("permissions", apis->CreateDeepCopy());
     EXPECT_TRUE(Permissions::Populate(*value, &permissions_object));
-    permissions = UnpackPermissionSet(permissions_object, true, &error);
-    EXPECT_TRUE(permissions->HasAPIPermission(APIPermission::kTab));
-    EXPECT_TRUE(permissions.get());
+
+    std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+        UnpackPermissionSet(permissions_object, PermissionSet(),
+                            optional_permissions, true, &error);
+
+    ASSERT_TRUE(unpack_result);
     EXPECT_TRUE(error.empty());
+    EXPECT_EQ(1u, unpack_result->optional_apis.size());
+    EXPECT_TRUE(unpack_result->optional_apis.count(APIPermission::kTab));
   }
 
   // The api permissions don't need to be present either.
@@ -110,10 +102,15 @@ TEST(ExtensionPermissionsAPIHelpers, Unpack) {
     value->Clear();
     value->Set("origins", origins->CreateDeepCopy());
     EXPECT_TRUE(Permissions::Populate(*value, &permissions_object));
-    permissions = UnpackPermissionSet(permissions_object, true, &error);
-    EXPECT_TRUE(permissions.get());
+
+    std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+        UnpackPermissionSet(permissions_object, PermissionSet(),
+                            optional_permissions, true, &error);
+    ASSERT_TRUE(unpack_result);
     EXPECT_TRUE(error.empty());
-    EXPECT_TRUE(permissions->HasExplicitAccessToOrigin(GURL("http://a.com/")));
+    EXPECT_THAT(permissions_test_util::GetPatternsAsStrings(
+                    unpack_result->optional_explicit_hosts),
+                testing::UnorderedElementsAre("http://a.com/*"));
   }
 
   // Throw errors for non-string API permissions.
@@ -159,10 +156,15 @@ TEST(ExtensionPermissionsAPIHelpers, Unpack) {
     value->Set("origins", origins->CreateDeepCopy());
     value->Set("random", std::make_unique<base::Value>(3));
     EXPECT_TRUE(Permissions::Populate(*value, &permissions_object));
-    permissions = UnpackPermissionSet(permissions_object, true, &error);
-    EXPECT_TRUE(permissions.get());
+
+    std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+        UnpackPermissionSet(permissions_object, PermissionSet(),
+                            optional_permissions, true, &error);
+    ASSERT_TRUE(unpack_result);
     EXPECT_TRUE(error.empty());
-    EXPECT_TRUE(permissions->HasExplicitAccessToOrigin(GURL("http://a.com/")));
+    EXPECT_THAT(permissions_test_util::GetPatternsAsStrings(
+                    unpack_result->optional_explicit_hosts),
+                testing::UnorderedElementsAre("http://a.com/*"));
   }
 
   // Unknown permissions should throw an error.
@@ -173,11 +175,124 @@ TEST(ExtensionPermissionsAPIHelpers, Unpack) {
     invalid_apis->AppendString("unknown_permission");
     value->Set("permissions", std::move(invalid_apis));
     EXPECT_TRUE(Permissions::Populate(*value, &permissions_object));
-    permissions = UnpackPermissionSet(permissions_object, true, &error);
-    EXPECT_FALSE(permissions.get());
-    EXPECT_FALSE(error.empty());
+
+    EXPECT_FALSE(UnpackPermissionSet(permissions_object, PermissionSet(),
+                                     optional_permissions, true, &error));
     EXPECT_EQ(error, "'unknown_permission' is not a recognized permission.");
   }
 }
 
+// Tests that host permissions are properly partitioned according to the
+// required/optional permission sets.
+TEST(ExtensionPermissionsAPIHelpers, Unpack_HostSeparation) {
+  auto explicit_url_pattern = [](const char* pattern) {
+    return URLPattern(Extension::kValidHostPermissionSchemes, pattern);
+  };
+
+  constexpr char kRequiredExplicit1[] = "https://required_explicit1.com/*";
+  constexpr char kRequiredExplicit2[] = "https://required_explicit2.com/*";
+  constexpr char kOptionalExplicit1[] = "https://optional_explicit1.com/*";
+  constexpr char kOptionalExplicit2[] = "https://optional_explicit2.com/*";
+  constexpr char kUnlisted1[] = "https://unlisted1.com/*";
+
+  URLPatternSet required_explicit_hosts({
+      explicit_url_pattern(kRequiredExplicit1),
+      explicit_url_pattern(kRequiredExplicit2),
+  });
+  URLPatternSet optional_explicit_hosts({
+      explicit_url_pattern(kOptionalExplicit1),
+      explicit_url_pattern(kOptionalExplicit2),
+  });
+
+  PermissionSet required_permissions(APIPermissionSet(),
+                                     ManifestPermissionSet(),
+                                     required_explicit_hosts, URLPatternSet());
+  PermissionSet optional_permissions(APIPermissionSet(),
+                                     ManifestPermissionSet(),
+                                     optional_explicit_hosts, URLPatternSet());
+
+  Permissions permissions_object;
+  permissions_object.origins =
+      std::make_unique<std::vector<std::string>>(std::vector<std::string>(
+          {kRequiredExplicit1, kOptionalExplicit1, kUnlisted1}));
+
+  std::string error;
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+      UnpackPermissionSet(permissions_object, required_permissions,
+                          optional_permissions, true, &error);
+  ASSERT_TRUE(unpack_result);
+  EXPECT_TRUE(error.empty()) << error;
+
+  EXPECT_THAT(GetPatternsAsStrings(unpack_result->required_explicit_hosts),
+              testing::UnorderedElementsAre(kRequiredExplicit1));
+  EXPECT_THAT(GetPatternsAsStrings(unpack_result->optional_explicit_hosts),
+              testing::UnorderedElementsAre(kOptionalExplicit1));
+  EXPECT_THAT(GetPatternsAsStrings(unpack_result->unlisted_hosts),
+              testing::UnorderedElementsAre(kUnlisted1));
+}
+
+// Tests that host permissions are properly partitioned according to the
+// required/optional permission sets.
+TEST(ExtensionPermissionsAPIHelpers, Unpack_APISeparation) {
+  constexpr APIPermission::ID kRequired1 = APIPermission::kTab;
+  constexpr APIPermission::ID kRequired2 = APIPermission::kStorage;
+  constexpr APIPermission::ID kOptional1 = APIPermission::kCookie;
+  constexpr APIPermission::ID kOptional2 = APIPermission::kAlarms;
+  constexpr APIPermission::ID kUnlisted1 = APIPermission::kIdle;
+
+  APIPermissionSet required_apis;
+  required_apis.insert(kRequired1);
+  required_apis.insert(kRequired2);
+
+  APIPermissionSet optional_apis;
+  optional_apis.insert(kOptional1);
+  optional_apis.insert(kOptional2);
+
+  PermissionSet required_permissions(required_apis, ManifestPermissionSet(),
+                                     URLPatternSet(), URLPatternSet());
+  PermissionSet optional_permissions(optional_apis, ManifestPermissionSet(),
+                                     URLPatternSet(), URLPatternSet());
+
+  Permissions permissions_object;
+  permissions_object.permissions = std::make_unique<std::vector<std::string>>(
+      std::vector<std::string>({"tabs", "cookies", "idle"}));
+
+  std::string error;
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+      UnpackPermissionSet(permissions_object, required_permissions,
+                          optional_permissions, true, &error);
+  ASSERT_TRUE(unpack_result);
+  EXPECT_TRUE(error.empty()) << error;
+
+  EXPECT_EQ(1u, unpack_result->required_apis.size());
+  EXPECT_TRUE(unpack_result->required_apis.count(kRequired1));
+  EXPECT_EQ(1u, unpack_result->optional_apis.size());
+  EXPECT_TRUE(unpack_result->optional_apis.count(kOptional1));
+  EXPECT_EQ(1u, unpack_result->unlisted_apis.size());
+  EXPECT_TRUE(unpack_result->unlisted_apis.count(kUnlisted1));
+}
+
+// Tests that an error is thrown for permissions that cannot be optional, when
+// requested as an optional permission.
+TEST(ExtensionPermissionsAPIHelpers, Unpack_UnsupportedAPIPermission) {
+  APIPermissionSet optional_apis;
+  optional_apis.insert(APIPermission::kWallpaper);
+  EXPECT_FALSE((*optional_apis.begin())->info()->supports_optional());
+  PermissionSet optional_permissions(optional_apis, ManifestPermissionSet(),
+                                     URLPatternSet(), URLPatternSet());
+
+  Permissions permissions_object;
+  permissions_object.permissions = std::make_unique<std::vector<std::string>>(
+      std::vector<std::string>({"wallpaper"}));
+
+  std::string error;
+  std::unique_ptr<UnpackPermissionSetResult> unpack_result =
+      UnpackPermissionSet(permissions_object, PermissionSet(),
+                          optional_permissions, true, &error);
+  ASSERT_TRUE(unpack_result) << error;
+  EXPECT_EQ(1u, unpack_result->unsupported_optional_apis.size());
+  EXPECT_TRUE(unpack_result->unsupported_optional_apis.count(
+      APIPermission::kWallpaper));
+}
+
 }  // namespace extensions

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

@@ -16,7 +16,7 @@ var NOT_OPTIONAL_ERROR =
 var REQUIRED_ERROR =
     "You cannot remove required permissions.";
 
-var NOT_WHITE_LISTED_ERROR =
+var NOT_ALLOWLISTED_ERROR =
     "The optional permissions API does not support '*'.";
 
 var UNKNOWN_PERMISSIONS_ERROR =
@@ -204,13 +204,14 @@ chrome.test.getConfig(function(config) {
       }));
     },
 
-    // Make sure you can only access the white listed permissions.
-    function whitelist() {
-      var error_msg = NOT_WHITE_LISTED_ERROR.replace('*', 'cloudPrintPrivate');
+    // Make sure you can only access the allowlisted permissions.
+    function allowlist() {
+      const kPermission = 'fontSettings';
+      var error_msg = NOT_ALLOWLISTED_ERROR.replace('*', kPermission);
       chrome.permissions.request(
-          {permissions: ['cloudPrintPrivate']}, fail(error_msg));
+          {permissions: [kPermission]}, fail(error_msg));
       chrome.permissions.remove(
-          {permissions: ['cloudPrintPrivate']}, fail(error_msg));
+          {permissions: [kPermission]}, fail(error_msg));
     },
 
     function unknownPermission() {

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

@@ -15,6 +15,7 @@
      "bookmarks",
      "cookies",
      "background",
+     "fontSettings",
      "http://*.c.com/*"
    ]
 }