Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

The approach taken here is to register the schemes, origins and the process ids which are allowed to access these
origins in the ResourceDispatcherHost class.

The ResourceDispatcherHost class now provides the following functionality.
1. Register a URL scheme for access check. Please see the AddSchemeForAccessCheck method.
2. Register and unregister a URL origin for access checks. Please see the RegisterOriginForAccessChecks
   and UnregisterOriginForAccessChecks methods.
3. Register and unregister process ids which are allowed to access or commit the URL. Please see the
   AddProcessforOrigin and RemoveProcessForOrigin methods.
4. Functionality to check if a URL origin is allowed. This is provided via a new method in the
   ResourceDispatcherHostImpl class called IsIlegalOrigin().

The other changes in this patchset are to register the extension URL scheme for access check and to push all
extension URLs to ResourceDispatcherHost along with information whether they have publicly accessible resources.

BUG=598073

[ananta, 2016-07-29 23:12:05]

Description was changed from

==========
Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

The approach we are taking is to push the legal origins to
ResourceDispatcherHost.

BUG=598073
==========

to

==========
Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

The approach taken here is to push the schemes and origins needed to be checked
to ResourceDispatcherHost along with the owner processes and guests. This data
is used for origin access checks

BUG=598073
==========

[ananta, 2016-07-29 23:12:05]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2016-07-30 02:38:48]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 02:39:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-07-30 02:43:09]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-07-30 02:56:45]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-30 02:57:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-07-30 03:02:43]

The CQ bit was unchecked by ananta@chromium.org

[jam, 2016-08-01 20:06:02]

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1158: bool
ChromeContentBrowserClient::IsIllegalOrigin(
remove this method here and in the public interface and in
ChromeContentBrowserClientExtensionsPart

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2299:
DCHECK(content::ResourceDispatcherHost::Get());
nit: not needed by definition. if somehow someone tried to change this to call
this method before RDH was created, there would be a crash which is just as
useful of a signal

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2301:
extensions::kExtensionScheme);
The rest of the RDH initialization on the chrome side is in
BrowserProcessImpl::ResourceDispatcherHostCreated, so probably best to keep this
there with them

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:523:
void ChromeContentBrowserClientExtensionsPart::AddExtension(
these two methods are only called by extension_system_impl.cc, so they could
just be moved to it. the reason is that this class is the extension related
callbacks from content->extensions. for extension->content code, you can just
have that at the originating extensions code.

also, the extension code can just do a PostTask directly on the RDH instead of
posting a task to another method which does this

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:511: // ctors like STL
set.
nit: no need for this comment (i.e. we don't do it elsewhere)

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:576: // Delete the
origin access info maps.
(wouldn't be needed if unique_ptr was used)

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:802: DCHECK(false);
nit: NOTREACHED() is preferred to DCHECK(false)

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:367: // resources, which
processes own it, which are its guests, etc.
nit: this comment discusses chrome apps concepts, which are a layering violation
in this networking code.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:384: typedef
std::map<const ResourceContext*, OriginAccessInfoMap*>
unique_ptr?

https://codereview.chromium.org/2182633007/diff/120001/content/browser/securi...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/securi...
content/browser/security_exploit_browsertest.cc:404: class
IsolatedAppContentBrowserClient : public TestContentBrowserClient {
not needed anymore

https://codereview.chromium.org/2182633007/diff/120001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2182633007/diff/120001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:85: virtual void
AddGuestForOrigin(const ResourceContext* context,
from the loading perspective, there's no need to differentiate between owner vs
guest process right? i.e. looking at the implementation inside content/, they're
treated the same way.

in that case, it seems the API could be simplified by having one set of methods:
AddProcessForOrigin/RemoveProcessForOrigin.

Also, do we really need AddOriginAccessInformation and
RemoveOriginAccessInformation? i.e. can't RDH automatically create/remove the
map as needed based on the AddProcessForOrigin/RemoveProcessForOrigin calls
automatically?

https://codereview.chromium.org/2182633007/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1527:
guest_process_id);
nit: it would be shorter to read this code if there were two PostTask calls
above instead of these two extra methods

[ananta, 2016-08-02 00:40:50]

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1158: bool
ChromeContentBrowserClient::IsIllegalOrigin(
On 2016/08/01 20:06:01, jam wrote:
> remove this method here and in the public interface and in
> ChromeContentBrowserClientExtensionsPart

Done.

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2299:
DCHECK(content::ResourceDispatcherHost::Get());
On 2016/08/01 20:06:01, jam wrote:
> nit: not needed by definition. if somehow someone tried to change this to call
> this method before RDH was created, there would be a crash which is just as
> useful of a signal

Done.

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2301:
extensions::kExtensionScheme);
On 2016/08/01 20:06:01, jam wrote:
> The rest of the RDH initialization on the chrome side is in
> BrowserProcessImpl::ResourceDispatcherHostCreated, so probably best to keep
this
> there with them

Done.

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:523:
void ChromeContentBrowserClientExtensionsPart::AddExtension(
On 2016/08/01 20:06:01, jam wrote:
> these two methods are only called by extension_system_impl.cc, so they could
> just be moved to it. the reason is that this class is the extension related
> callbacks from content->extensions. for extension->content code, you can just
> have that at the originating extensions code.
> 
> also, the extension code can just do a PostTask directly on the RDH instead of
> posting a task to another method which does this

Done. I did not want another PostTask to the RDH on the IO thread because we
already do one post task to add the extension information to the InfoMap on the
IO thread.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:511: // ctors like STL
set.
On 2016/08/01 20:06:01, jam wrote:
> nit: no need for this comment (i.e. we don't do it elsewhere)

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:576: // Delete the
origin access info maps.
On 2016/08/01 20:06:01, jam wrote:
> (wouldn't be needed if unique_ptr was used)

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:802: DCHECK(false);
On 2016/08/01 20:06:01, jam wrote:
> nit: NOTREACHED() is preferred to DCHECK(false)

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:367: // resources, which
processes own it, which are its guests, etc.
On 2016/08/01 20:06:01, jam wrote:
> nit: this comment discusses chrome apps concepts, which are a layering
violation
> in this networking code.

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:384: typedef
std::map<const ResourceContext*, OriginAccessInfoMap*>
On 2016/08/01 20:06:01, jam wrote:
> unique_ptr?

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/browser/securi...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/content/browser/securi...
content/browser/security_exploit_browsertest.cc:404: class
IsolatedAppContentBrowserClient : public TestContentBrowserClient {
On 2016/08/01 20:06:01, jam wrote:
> not needed anymore

Done.

https://codereview.chromium.org/2182633007/diff/120001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2182633007/diff/120001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:85: virtual void
AddGuestForOrigin(const ResourceContext* context,
On 2016/08/01 20:06:01, jam wrote:
> from the loading perspective, there's no need to differentiate between owner
vs
> guest process right? i.e. looking at the implementation inside content/,
they're
> treated the same way.
> 
> in that case, it seems the API could be simplified by having one set of
methods:
> AddProcessForOrigin/RemoveProcessForOrigin.
> 
> Also, do we really need AddOriginAccessInformation and
> RemoveOriginAccessInformation? i.e. can't RDH automatically create/remove the
> map as needed based on the AddProcessForOrigin/RemoveProcessForOrigin calls
> automatically?

Done.

https://codereview.chromium.org/2182633007/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1527:
guest_process_id);
On 2016/08/01 20:06:01, jam wrote:
> nit: it would be shorter to read this code if there were two PostTask calls
> above instead of these two extra methods

Leaving this as is. I wanted to avoid two PostTasks to the IO thread.

[ananta, 2016-08-02 01:24:42]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 01:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 01:27:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 01:27:34]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2016-08-02 01:56:04]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 01:56:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 02:06:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 02:06:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2016-08-02 02:13:57]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 02:14:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 02:35:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 02:35:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2016-08-02 17:01:46]

lgtm with comments

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1158: bool
ChromeContentBrowserClient::IsIllegalOrigin(
On 2016/08/02 00:40:49, ananta wrote:
> On 2016/08/01 20:06:01, jam wrote:
> > remove this method here and in the public interface and in
> > ChromeContentBrowserClientExtensionsPart
> 
> Done.

please also remove from ContentBrowserClient

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
(right):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:95: //
Helper functions to register and unregister an extension process. Invoked
nit: since they're just static private methods, no need to declare them here.
just put them in an anonymous namespace in the cc file

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:773: GURL url(origin);
nit: in these three new methods, add a DCHECK_CURRENTLY_ON(BrowserThread::IO);
at the top.

since these are public methods and the caller is outside content, it's generally
good to have defensive checks in case calling code is changed. (and then no need
for the thread DCHECK in the methods that call these)

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:343: // be explicitly
defined in the cc file.
now that this is just one set, any reason to keep it around? i.e. why not make
ResourceContextOriginMap be a map from ResourceContext to a std::set<int>?

https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.h:326: // Helper functions
to add and remove guest processes. Invoked on the IO
ditto: just move to anonymous namespace

[Charlie Reis, 2016-08-02 20:41:41]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2016-08-02 20:41:42]

I'd like to review as well, if that's ok.  I'm not sure I understand yet how
this is equivalent, and it looks like there's some test failures.

I'm curious, what was the reason these needed to move?  At first glance, it's
unintuitive to me to put an origin registry on ResourceDispatcherHost.

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(left):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:292:
return true;
Sanity check: We'll still deny this case because the extension's origin won't
have been registered, right?

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:295:
// itself, or by one if its guests if there are accessible_resources.
Please don't lose all the comments in this method.  :)  I wanted it to be much
more strict, but we had to allow certain cases that are worth documenting.

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:638:
if (extension->is_platform_app()) {
This would be a good place for some of the previous comments.

https://codereview.chromium.org/2182633007/diff/220001/content/browser/securi...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/2182633007/diff/220001/content/browser/securi...
content/browser/security_exploit_browsertest.cc:419:
ResourceDispatcherHost::Get()->AddSchemeForAccessCheck("https");
Please add a comment about how this leads to denying
embedder_isolated_origin_msg.

https://codereview.chromium.org/2182633007/diff/220001/content/browser/securi...
content/browser/security_exploit_browsertest.cc:442: TestContentBrowserClient
app_client;
Why keep this if you're removing IsolatedAppContentBrowserClient?  I think it
was only there so we returned that the origin was illegal.

https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:56: // allowed
nit: End with period.

Also clarify what access checked means here.  I think it means that any process
claiming to have committed a URL within the scheme must have been registered by
the methods below?

[ananta, 2016-08-02 22:28:28]

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(left):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:292:
return true;
On 2016/08/02 20:41:42, Charlie Reis wrote:
> Sanity check: We'll still deny this case because the extension's origin won't
> have been registered, right?

The URL won't be found in the RDH map and hence will be treated as illegal. I
updated the RDH IsIllegalOrigin function comments to clarify this.

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:295:
// itself, or by one if its guests if there are accessible_resources.
On 2016/08/02 20:41:42, Charlie Reis wrote:
> Please don't lose all the comments in this method.  :)  I wanted it to be much
> more strict, but we had to allow certain cases that are worth documenting.

Done.

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:638:
if (extension->is_platform_app()) {
On 2016/08/02 20:41:42, Charlie Reis wrote:
> This would be a good place for some of the previous comments.

Done.

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
(right):

https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:95: //
Helper functions to register and unregister an extension process. Invoked
On 2016/08/02 17:01:46, jam wrote:
> nit: since they're just static private methods, no need to declare them here.
> just put them in an anonymous namespace in the cc file

Done.

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:773: GURL url(origin);
On 2016/08/02 17:01:46, jam wrote:
> nit: in these three new methods, add a DCHECK_CURRENTLY_ON(BrowserThread::IO);
> at the top.
> 
> since these are public methods and the caller is outside content, it's
generally
> good to have defensive checks in case calling code is changed. (and then no
need
> for the thread DCHECK in the methods that call these)

Added the DCHECK for the following methods:
1. RegisterSchemeForAccessCheck
2. UnregisterSchemeForAccessCheck
1. AddProcessForOrigin
2. RemoveProcessForOrigin

The AddSchemeForAccessCheck gets called on the main thread.

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:343: // be explicitly
defined in the cc file.
On 2016/08/02 17:01:46, jam wrote:
> now that this is just one set, any reason to keep it around? i.e. why not make
> ResourceContextOriginMap be a map from ResourceContext to a std::set<int>?

Done.

https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:56: // allowed
On 2016/08/02 20:41:42, Charlie Reis wrote:
> nit: End with period.
> 
> Also clarify what access checked means here.  I think it means that any
process
> claiming to have committed a URL within the scheme must have been registered
by
> the methods below?

Done.

https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.h:326: // Helper functions
to add and remove guest processes. Invoked on the IO
On 2016/08/02 17:01:46, jam wrote:
> ditto: just move to anonymous namespace

Can't right now. The WebViewRendererState::AddGuest function invoked by this
class is private and the reason it works today is due to a friend relationship

[ananta, 2016-08-02 22:29:24]

On 2016/08/02 22:28:28, ananta wrote:
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
> File
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
> (left):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
>
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:292:
> return true;
> On 2016/08/02 20:41:42, Charlie Reis wrote:
> > Sanity check: We'll still deny this case because the extension's origin
won't
> > have been registered, right?
> 
> The URL won't be found in the RDH map and hence will be treated as illegal. I
> updated the RDH IsIllegalOrigin function comments to clarify this.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
>
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:295:
> // itself, or by one if its guests if there are accessible_resources.
> On 2016/08/02 20:41:42, Charlie Reis wrote:
> > Please don't lose all the comments in this method.  :)  I wanted it to be
much
> > more strict, but we had to allow certain cases that are worth documenting.
> 
> Done.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
> File
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
> (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
>
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:638:
> if (extension->is_platform_app()) {
> On 2016/08/02 20:41:42, Charlie Reis wrote:
> > This would be a good place for some of the previous comments.
> 
> Done.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
> File chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
> (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensi...
> chrome/browser/extensions/chrome_content_browser_client_extensions_part.h:95:
//
> Helper functions to register and unregister an extension process. Invoked
> On 2016/08/02 17:01:46, jam wrote:
> > nit: since they're just static private methods, no need to declare them
here.
> > just put them in an anonymous namespace in the cc file
> 
> Done.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:773: GURL url(origin);
> On 2016/08/02 17:01:46, jam wrote:
> > nit: in these three new methods, add a
DCHECK_CURRENTLY_ON(BrowserThread::IO);
> > at the top.
> > 
> > since these are public methods and the caller is outside content, it's
> generally
> > good to have defensive checks in case calling code is changed. (and then no
> need
> > for the thread DCHECK in the methods that call these)
> 
> Added the DCHECK for the following methods:
> 1. RegisterSchemeForAccessCheck
> 2. UnregisterSchemeForAccessCheck
> 1. AddProcessForOrigin
> 2. RemoveProcessForOrigin
> 
> The AddSchemeForAccessCheck gets called on the main thread.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.h (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.h:343: // be explicitly
> defined in the cc file.
> On 2016/08/02 17:01:46, jam wrote:
> > now that this is just one set, any reason to keep it around? i.e. why not
make
> > ResourceContextOriginMap be a map from ResourceContext to a std::set<int>?
> 
> Done.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
> File content/public/browser/resource_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:56: // allowed
> On 2016/08/02 20:41:42, Charlie Reis wrote:
> > nit: End with period.
> > 
> > Also clarify what access checked means here.  I think it means that any
> process
> > claiming to have committed a URL within the scheme must have been registered
> by
> > the methods below?
> 
> Done.
> 
>
https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
> File extensions/browser/guest_view/web_view/web_view_guest.h (right):
> 
>
https://codereview.chromium.org/2182633007/diff/220001/extensions/browser/gue...
> extensions/browser/guest_view/web_view/web_view_guest.h:326: // Helper
functions
> to add and remove guest processes. Invoked on the IO
> On 2016/08/02 17:01:46, jam wrote:
> > ditto: just move to anonymous namespace
> 
> Can't right now. The WebViewRendererState::AddGuest function invoked by this
> class is private and the reason it works today is due to a friend relationship

I brought back the functionality to register and unregister an origin for access
check via RDH::RegisterOriginForAccessChecks and
RDH::UnregisterOriginForAccessChecks

[ananta, 2016-08-02 22:30:13]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 22:30:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-02 22:33:27]

The CQ bit was unchecked by ananta@chromium.org

[jam, 2016-08-02 23:43:58]

On 2016/08/02 22:29:24, ananta wrote:
> I brought back the functionality to register and unregister an origin for
access
> check via RDH::RegisterOriginForAccessChecks and
> RDH::UnregisterOriginForAccessChecks

Why do we need these? i.e. as we chatted about can't we default to returning
true if the origin is for the extensions scheme but isn't found?

[ananta, 2016-08-03 18:57:13]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 18:58:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-03 19:01:17]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-08-03 19:34:46]

Description was changed from

==========
Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

The approach taken here is to push the schemes and origins needed to be checked
to ResourceDispatcherHost along with the owner processes and guests. This data
is used for origin access checks

BUG=598073
==========

to

==========
Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost.

The approach taken here is to register the schemes, origins and the process ids
which are allowed to access these
origins in the ResourceDispatcherHost class.

The ResourceDispatcherHost class now provides the following functionality.
1. Register a URL scheme for access check. Please see the
AddSchemeForAccessCheck method.
2. Register and unregister a URL origin for access checks. Please see the
RegisterOriginForAccessChecks
   and UnregisterOriginForAccessChecks methods.
3. Register and unregister process ids which are allowed to access or commit the
URL. Please see the
   AddProcessforOrigin and RemoveProcessForOrigin methods.
4. Functionality to check if a URL origin is allowed. This is provided via a new
method in the
   ResourceDispatcherHostImpl class called IsIlegalOrigin().

The other changes in this patchset are to register the extension URL scheme for
access check and to push all
extension URLs to ResourceDispatcherHost along with information whether they
have publicly accessible resources.

BUG=598073
==========

[ananta, 2016-08-03 21:01:06]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 21:02:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-03 21:03:34]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-08-03 21:07:10]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 21:07:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-03 21:09:26]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-08-03 22:50:17]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 22:50:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-08-03 22:53:33]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-08-03 23:35:23]

We now have a green try run. Changes made since the last iteration include
registering all extension urls with RDH along with information about whether
they have publicly available resources and also some information about how to go
about processing the origin access check. 

I added an enum which contains the possible access masks RDH can use to handle
the origin checks.

[Charlie Reis, 2016-08-09 02:07:48]

Sorry for the delay-- I'm back from the offsite.

Thanks for working through that.  I think your new approach is probably
expressive enough to capture the same policy we had before, and we can probably
reason through it to make sure it's correct.  I'm a bit concerned about it being
pretty hard to follow, though.  Some specific questions about it below, just to
see if there are other options that might be clearer.

(This is a bit of thinking ahead, too, since I imagine this won't be the only
extensions code that needs to be factored out.  Many places could have corner
cases on par with this illegal origin check, which get difficult to talk about
with abstract terms like "owner processes.")

If we decide that this is the way to go, though, I can try to do another pass to
ensure equivalence with the old check.

https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:314: //    |origin| it is
allowed. Please see OriginAccessInfo defined above for
nit: Below?

https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:319: //    |origin| and
there are publicly available resources it is allowed.
Hmm.  This is hard to understand in the abstract without having extension
concepts to map it to, which also makes it hard to reason about whether the
policy is the same as before (or if it's correct).

Do we have a sense for whether it will be possible in general to completely
avoid having extension code in the network process?  I'm wondering if it would
be easier to have a similar ContentClient structure in that process where we can
call out to the embedder and let it answer questions like this in context.

Would that be a worse tradeoff?  I'm just trying to brainstorm ways to make this
easier to understand, since this abstract mapping is hard (and may be hard to
maintain as we eliminate or change the corner cases in the extension code
itself).

https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:35: DENY_FOR_NON_OWNERS = 0x0,
// Denied for non owner processes.
What's an owner process?  (We'll need to elaborate on that.)

https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:76: // used to grant or deny
access to the origin. By default owner processes
It's not clear what an owner process is.

https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:90: // The |owner_process|
flag indicates whether the process owns the |origin|.
We'll need more guidance on what to pass for owner process, but maybe that'll be
clearer if it's explained more above.

https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:102: bool owner_proces) = 0;
Do we need the flag on removal as well?  What happens if it differs between Add
and Remove calls?

[ananta, 2016-08-10 01:06:58]

On 2016/08/09 02:07:48, Charlie Reis wrote:
> Sorry for the delay-- I'm back from the offsite.
> 
> Thanks for working through that.  I think your new approach is probably
> expressive enough to capture the same policy we had before, and we can
probably
> reason through it to make sure it's correct.  I'm a bit concerned about it
being
> pretty hard to follow, though.  Some specific questions about it below, just
to
> see if there are other options that might be clearer.
> 
> (This is a bit of thinking ahead, too, since I imagine this won't be the only
> extensions code that needs to be factored out.  Many places could have corner
> cases on par with this illegal origin check, which get difficult to talk about
> with abstract terms like "owner processes.")
> 
> If we decide that this is the way to go, though, I can try to do another pass
to
> ensure equivalence with the old check.
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.h (right):
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.h:314: //    |origin| it
is
> allowed. Please see OriginAccessInfo defined above for
> nit: Below?
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.h:319: //    |origin| and
> there are publicly available resources it is allowed.
> Hmm.  This is hard to understand in the abstract without having extension
> concepts to map it to, which also makes it hard to reason about whether the
> policy is the same as before (or if it's correct).
> 
> Do we have a sense for whether it will be possible in general to completely
> avoid having extension code in the network process?  I'm wondering if it would
> be easier to have a similar ContentClient structure in that process where we
can
> call out to the embedder and let it answer questions like this in context.
> 
> Would that be a worse tradeoff?  I'm just trying to brainstorm ways to make
this
> easier to understand, since this abstract mapping is hard (and may be hard to
> maintain as we eliminate or change the corner cases in the extension code
> itself).
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
> File content/public/browser/resource_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:35: DENY_FOR_NON_OWNERS =
0x0,
> // Denied for non owner processes.
> What's an owner process?  (We'll need to elaborate on that.)
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:76: // used to grant or deny
> access to the origin. By default owner processes
> It's not clear what an owner process is.
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:90: // The |owner_process|
> flag indicates whether the process owns the |origin|.
> We'll need more guidance on what to pass for owner process, but maybe that'll
be
> clearer if it's explained more above.
> 
>
https://codereview.chromium.org/2182633007/diff/390001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:102: bool owner_proces) = 0;
> Do we need the flag on removal as well?  What happens if it differs between
Add
> and Remove calls?

We are abandoning this patch and trying out another approach which adds an
interceptor to resource dispatcher host. That patch
is here https://codereview.chromium.org/2222723002/

[Charlie Reis, 2016-08-10 20:46:38]

On 2016/08/10 01:06:58, ananta wrote:
> We are abandoning this patch and trying out another approach which adds an
> interceptor to resource dispatcher host. That patch
> is here https://codereview.chromium.org/2222723002/

Great-- I like the way that one is shaping up.