Create withheld permissions

extensions/renderer/script_injection.cc

 // Copyright 2014 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/renderer/script_injection.h"
 
 #include <map>
 
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram.h"
@@ skipping 22 matching lines @@
 
 typedef std::map<std::string, int> IsolatedWorldMap;
 base::LazyInstance<IsolatedWorldMap> g_isolated_worlds =
     LAZY_INSTANCE_INITIALIZER;
 
 const int kInvalidRequestId = -1;
 
 // The id of the next pending injection.
 int64 g_next_pending_id = 0;
 
-bool ShouldDelayForPermission() {
-  return FeatureSwitch::scripts_require_action()->IsEnabled();
+bool NotifyBrowserOfInjections() {

[not at google - send to devlin, 2014/06/27 23:24:34]

this should start with Should otherwise it sounds like it's actually notifying.

[Devlin, 2014/06/30 17:06:11]

Done.

+  return !FeatureSwitch::scripts_require_action()->IsEnabled();
 }
 
 // Append all the child frames of |parent_frame| to |frames_vector|.
 void AppendAllChildFrames(blink::WebFrame* parent_frame,
                           std::vector<blink::WebFrame*>* frames_vector) {
   DCHECK(parent_frame);
   for (blink::WebFrame* child_frame = parent_frame->firstChild(); child_frame;
        child_frame = child_frame->nextSibling()) {
     frames_vector->push_back(child_frame);
     AppendAllChildFrames(child_frame, frames_vector);
@@ skipping 83 matching lines @@
   if (request_id_ != -1)
     return false;  // We're waiting for permission right now, try again later.
 
   if (!extension) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
     return true;  // We're done.
   }
 
   switch (injector_->CanExecuteOnFrame(
       extension, web_frame_, tab_id_, web_frame_->top()->document().url())) {
-    case ScriptInjector::DENY_ACCESS:
+    case PermissionsData::DENY_ACCESS:
       NotifyWillNotInject(ScriptInjector::NOT_ALLOWED);
       return true;  // We're done.
-    case ScriptInjector::REQUEST_ACCESS:
+    case PermissionsData::REQUEST_ACCESS:
       RequestPermission();
-      if (ShouldDelayForPermission())
-        return false;  // Wait around for permission.
-      // else fall through
-    case ScriptInjector::ALLOW_ACCESS:
+      return false;  // Wait around for permission.
+    case PermissionsData::ALLOW_ACCESS:

[not at google - send to devlin, 2014/06/27 23:24:34]

I think it's fine if this class just interprets what it means for a permission
to be withheld or whatever, as in,

case PermissionsData::kAccessAllowed:
  ...
case PermissionsData::kAccessDenied:
  ...
case PermissionsData::kAccessWithheld:
  ...

looks good.

[Devlin, 2014/06/30 17:06:11]

Done.

       Inject(extension, scripts_run_info);
       return true;  // We're done!
   }
 
   // Some compilers don't realize that we always return from the switch() above.
   // Make them happy.
   return false;
 }
 
 bool ScriptInjection::OnPermissionGranted(const Extension* extension,
                                           ScriptsRunInfo* scripts_run_info) {
   if (!extension) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
     return false;
   }
 
   Inject(extension, scripts_run_info);
   return true;
 }
 
 void ScriptInjection::RequestPermission() {
   content::RenderView* render_view =
       content::RenderView::FromWebView(web_frame()->top()->view());
 
-  // If the feature to delay for permission isn't enabled, then just send an
+  // If we are just notifying the browser of the injection, then send an
   // invalid request (which is treated like a notification).
-  request_id_ =
-      ShouldDelayForPermission() ? g_next_pending_id++ : kInvalidRequestId;
+  request_id_ = NotifyBrowserOfInjections() ? kInvalidRequestId
+                                            : g_next_pending_id++;
   render_view->Send(new ExtensionHostMsg_RequestScriptInjectionPermission(
       render_view->GetRoutingID(),
       extension_id_,
+      injector_->script_type(),

[not at google - send to devlin, 2014/06/27 23:24:34]

I think that exposing the "script type" isn't quite the right idiom. this is
eventually just "the type of permission check to make" which amounts to either
CanAccessPage vs CanRunContentScriptOnPage... so I think that's what it should
be. have an enum like PermissionData::AccessCheckType with values "can access
page" vs "can run content script on page. remove that script type from
extension_misc.

[Devlin, 2014/06/30 17:06:11]

My thought was that it's not unlikely (and perhaps even likely) that we'll want
to UMA this stuff at some point, in which case ScriptType will be pretty
useful... Are you sure you want it gone?

[not at google - send to devlin, 2014/07/01 00:28:35]

maybe. but we don't need it now, and like I said it's the wrong idiom for the
check.

[not at google - send to devlin, 2014/07/01 02:56:50]

I should explain more.

My "wrong idiom" comment is because it would be repeating the logic of what to
do given the script type. The renderer has already chosen - either check hosts
or scripts - so why does the browser also need to decide? I think it's better if
the browser's job is to simply call the right method, not decide.

Of course, right now all it's really just a rename. However, when the
declarative content scripts come into play it won't be. We would need to add
another script type.

If the enum was somewhere better than constants.h I'd object less. Can you think
of anything? It's not obvious, because these script injection things are an API
concern, whereas permissions checks are a platform concern.

Your point about UMA is good though. I'd definitely be ok with it if not for it
being in constants.h.

[Devlin, 2014/07/01 16:27:05]

Yeah, constants.h is kind of a "I give up" location.

What would actually make sense is a specific "script_constants" file, because we
have items that relate to multiple scripts that we kinda just throw anywhere. 
Case in point: UserScript::RunLocation.  It applies to UserScripts and
tabs.executeScript(), but we chucked it in UserScript.  We could grab that and
the script type and make a new constants file for them (so that it's at least
more clear where it's coming from, and, of course, namespace it to be clear what
file it's in - script_constants::RunLocation, script_constants::ScriptType).

This has two added advantages:
- Not everything has to include user_script.h only for its enums (though we
still replace it with a [smaller] constants file).
- We can make the world a better place by renaming it from RunLocation.
(If we go this route, I'd wanna move/rename RunLocation in a separate patch, but
it'd be an immediate follow-up.)

Or we could just throw it in UserScript because it's easy.

[not at google - send to devlin, 2014/07/01 17:02:11]

ok, let's throw it in UserScript.

then maybe pull it out into its own file later.

[Devlin, 2014/07/01 18:34:08]

Done.

       render_view->GetPageId(),
       request_id_));
 }
 
 void ScriptInjection::NotifyWillNotInject(
     ScriptInjector::InjectFailureReason reason) {
   complete_ = true;
   injector_->OnWillNotInject(reason);
 }
 
 void ScriptInjection::Inject(const Extension* extension,
                              ScriptsRunInfo* scripts_run_info) {
   DCHECK(extension);
   DCHECK(scripts_run_info);
   DCHECK(!complete_);
 
+  if (NotifyBrowserOfInjections())
+    RequestPermission();
+
   std::vector<blink::WebFrame*> frame_vector;
   frame_vector.push_back(web_frame_);
   if (injector_->ShouldExecuteInChildFrames())
     AppendAllChildFrames(web_frame_, &frame_vector);
 
   scoped_ptr<blink::WebScopedUserGesture> gesture;
   if (injector_->IsUserGesture())
     gesture.reset(new blink::WebScopedUserGesture());
 
   bool inject_js = injector_->ShouldInjectJs(run_location_);
   bool inject_css = injector_->ShouldInjectCss(run_location_);
   DCHECK(inject_js || inject_css);
 
   scoped_ptr<base::ListValue> execution_results(new base::ListValue());
   GURL top_url = web_frame_->top()->document().url();
   for (std::vector<blink::WebFrame*>::iterator iter = frame_vector.begin();
        iter != frame_vector.end();
        ++iter) {
     blink::WebFrame* frame = *iter;
 
     // We recheck access here in the renderer for extra safety against races
     // with navigation, but different frames can have different URLs, and the
     // extension might only have access to a subset of them.
     // For child frames, we just skip ones the extension doesn't have access
     // to and carry on.
     // Note: we don't consider REQUEST_ACCESS because there is nowhere to
     // surface a request for a child frame.
     // TODO(rdevlin.cronin): We should ask for permission somehow.
     if (injector_->CanExecuteOnFrame(extension, frame, tab_id_, top_url) ==
-        ScriptInjector::DENY_ACCESS) {
+        PermissionsData::DENY_ACCESS) {
       DCHECK(frame->parent());
       continue;
     }
     if (inject_js)
       InjectJs(extension, frame, execution_results.get());
     if (inject_css)
       InjectCss(frame);
   }
 
   complete_ = true;
@@ skipping 63 matching lines @@
   std::vector<std::string> css_sources =
       injector_->GetCssSources(run_location_);
   for (std::vector<std::string>::const_iterator iter = css_sources.begin();
        iter != css_sources.end();
        ++iter) {
     frame->document().insertStyleSheet(blink::WebString::fromUTF8(*iter));
   }
 }
 
 }  // namespace extensions