Almost all actions in Declarative Web Request require all_urls host permissions

chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc

+// Copyright (c) 2013 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/extensions/api/declarative_webrequest/webrequest_rules_checker.h"
+
+#include "base/bind.h"
+#include "chrome/common/extensions/extension.h"
+#include "extensions/common/error_utils.h"
+
+namespace {
+
+const char kActionCannotBeExecuted[] = "An action can never be executed "
+    "because there are is no time in the request life-cycle during which the "
+    "conditions can be checked and the action can possibly be executed.";
+
+const char kAllURLsPermissionNeeded[] =
+    "To execute the action '*', host permissions for all URLs are needed";

[Matt Perry, 2013/04/25 19:59:21]

nit: "To execute the action '*', you need to request host permission for all
URLs."

[vabr (Chromium), 2013/04/26 09:58:52]

Done, but should we actually say "for all hosts" instead of "for all URLs"? The
latter is not technically true.

[Matt Perry, 2013/04/26 20:37:44]

OK, how about "To execute the action '*', you need to request permission for all
hosts."

[vabr (Chromium), 2013/04/26 23:47:21]

SGTM, done.

+
+}  // namespace
+
+namespace extensions {
+
+WebRequestRulesChecker::WebRequestRulesChecker(const Extension* extension)
+    : extension_(extension) {}
+
+WebRequestRule::ConsistencyChecker WebRequestRulesChecker::GetChecker() {
+  return base::Bind(&WebRequestRulesChecker::Checker, base::Unretained(this));

[Matt Perry, 2013/04/25 19:59:21]

Making this a class seems like overkill. Checker and the other methods can just
be static methods (or globals in this file) if you bind Extension instead of
|this|. Something like:

static GetChecker(Extension* ext) {
  return base::Bind(&Checker, base::Unretained(ext));
}

[vabr (Chromium), 2013/04/26 09:58:52]

Done.

+}
+
+bool WebRequestRulesChecker::Checker(const WebRequestConditionSet* conditions,
+                                     const WebRequestActionSet* actions,
+                                     std::string* error) {
+  return (StageChecker(conditions, actions, error) &&
+          HostPermissionsChecker(actions, error));
+}
+
+bool WebRequestRulesChecker::HostPermissionsChecker(
+    const WebRequestActionSet* actions,
+    std::string* error) {
+  if (extension_->HasEffectiveAccessToAllHosts())
+    return true;
+
+  // Without the permission for all URLs, only actions with the following host
+  // strategies make sense to be registered:
+  // STRATEGY_NONE:  because not host permissions are needed,
+  // STRATEGY_HOST:  only host permissions for the host in the request's URL are
+  //                 required.
+  const int allowed_strategies =
+      WebRequestAction::STRATEGY_NONE | WebRequestAction::STRATEGY_HOST;
+  for (WebRequestActionSet::Actions::const_iterator action_iter =
+           actions->actions().begin();
+       action_iter != actions->actions().end();
+       ++action_iter) {
+    // Test the intersection of bit masks, this is intentionally & and not &&.
+    if ((*action_iter)->host_permissions_strategy() & allowed_strategies)

[Matt Perry, 2013/04/25 19:59:21]

Oh, I see why you made them bitmasks now. I'd still prefer to keep them singular
values, at the expense of making this code a little more verbose, to make it
clear you can only have one strategy at a time.

[vabr (Chromium), 2013/04/26 09:58:52]

Done.

+      continue;
+    // We only get here if this is a disallowed action.
+    *error = ErrorUtils::FormatErrorMessage(kAllURLsPermissionNeeded,
+                                            (*action_iter)->GetName());
+    return false;
+  }
+  return true;
+}
+
+// static
+bool WebRequestRulesChecker::StageChecker(
+    const WebRequestConditionSet* conditions,
+    const WebRequestActionSet* actions,
+    std::string* error) {
+  // Actions and conditions can be checked and executed in specific stages
+  // of each web request. A rule is inconsistent if there is an action that
+  // can only be triggered in stages in which no condition can be evaluated.
+
+  // In which stages there are conditions to evaluate.
+  int condition_stages = 0;
+  for (WebRequestConditionSet::Conditions::const_iterator condition_iter =
+           conditions->conditions().begin();
+       condition_iter != conditions->conditions().end();
+       ++condition_iter) {
+    condition_stages |= (*condition_iter)->stages();
+  }
+
+  for (WebRequestActionSet::Actions::const_iterator action_iter =
+           actions->actions().begin();
+       action_iter != actions->actions().end();
+       ++action_iter) {
+    // Test the intersection of bit masks, this is intentionally & and not &&.
+    if ((*action_iter)->GetStages() & condition_stages)
+      continue;
+    // We only get here if no matching condition was found.
+    *error = kActionCannotBeExecuted;

[Matt Perry, 2013/04/25 19:59:21]

Add the action's name in the error message.

[vabr (Chromium), 2013/04/26 09:58:52]

Done.

+    return false;
+  }
+  return true;
+}
+
+}  // namespace extensions