Hook up extension apps notification permission, take two

Hook up extension apps notification permission, take two

This is the chromium side of a change which will wait to land on the webkit side landing. (https://bugs.webkit.org/show_bug.cgi?id=36625)

It changes the NotificationPresenter to pass the sourceURL, rather than the SecurityOrigin in checking permission.

The full URL is required to match the app extent.

BUG=32361, 31024
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43162

[Aaron Boodman, 2010-03-26 00:29:30]

http://codereview.chromium.org/1383001/diff/1/3
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1383001/diff/1/3#newcode749
chrome/browser/net/chrome_url_request_context.cc:749: bool
ChromeURLRequestContext::CheckURLAccessToExtensionPermission(
The way permission checking for chrome-extension:// URLs currently works
basically duplicates what is happening here.

It would be nice if you could rip all that out and just check that case here
too.

[rafaelw, 2010-03-29 04:44:45]

(no new snapshot yet).

http://codereview.chromium.org/1383001/diff/1/3
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1383001/diff/1/3#newcode749
chrome/browser/net/chrome_url_request_context.cc:749: bool
ChromeURLRequestContext::CheckURLAccessToExtensionPermission(
Ok, so I ripped all that out and added code here to handle both cases.

As we discussed, the problem then becomes that there are multiple
ChromeURLRequestContexts, in particular one for chrome-extension: urls and one
for web urls.

Both need to updated when extensions load & unload. Access to them is achieved
via ChromeURLRequestContextGetters which are owned by the profile and can be
lazily created when accessed by the getter. The Getters are created on the UI
thread but are largely live on the IO thread.

I see three options:

1) Do the naive thing: Just PostTask to each getter directly from
ExtensionsService in ExtensionLoaded/Unloaded.
2) Create a funnel method on Profile, which just does exactly the same thing as
(1).
3) Make ChromeURLRequestContextGetter a NotificationObserver which listens on
two new notifications: EXTENSION_WILL_LOAD, EXTENSION_WILL_UNLOAD. The Observe()
method on the UI thread just does a PostTask to itself on the IO thread to the
existing OnNewExtension/OnExtensionUnloaded.

None of these options are great. (1) is brittle. (2) seems like window dressing.
I prefer (3), but it's still kinda gross.

What do you think? Other ideas?

R

On 2010/03/26 00:29:30, Aaron Boodman wrote:
> The way permission checking for chrome-extension:// URLs currently works
> basically duplicates what is happening here.
> 
> It would be nice if you could rip all that out and just check that case here
> too.
>

[Aaron Boodman, 2010-03-29 05:04:04]

On Sun, Mar 28, 2010 at 9:44 PM,  <rafaelw@chromium.org> wrote:
> (no new snapshot yet).
>
>
> http://codereview.chromium.org/1383001/diff/1/3
> File chrome/browser/net/chrome_url_request_context.cc (right):
>
> http://codereview.chromium.org/1383001/diff/1/3#newcode749
> chrome/browser/net/chrome_url_request_context.cc:749: bool
> ChromeURLRequestContext::CheckURLAccessToExtensionPermission(
> Ok, so I ripped all that out and added code here to handle both cases.
>
> As we discussed, the problem then becomes that there are multiple
> ChromeURLRequestContexts, in particular one for chrome-extension: urls
> and one for web urls.
>
> Both need to updated when extensions load & unload. Access to them is
> achieved via ChromeURLRequestContextGetters which are owned by the
> profile and can be lazily created when accessed by the getter. The
> Getters are created on the UI thread but are largely live on the IO
> thread.
>
> I see three options:
>
> 1) Do the naive thing: Just PostTask to each getter directly from
> ExtensionsService in ExtensionLoaded/Unloaded.
> 2) Create a funnel method on Profile, which just does exactly the same
> thing as (1).
> 3) Make ChromeURLRequestContextGetter a NotificationObserver which
> listens on two new notifications: EXTENSION_WILL_LOAD,
> EXTENSION_WILL_UNLOAD. The Observe() method on the UI thread just does a
> PostTask to itself on the IO thread to the existing
> OnNewExtension/OnExtensionUnloaded.
>
> None of these options are great. (1) is brittle. (2) seems like window
> dressing. I prefer (3), but it's still kinda gross.
>
> What do you think? Other ideas?

Thanks for taking the time to understand this.

The right thing is of course (4) -- refactor to make this less lame.
However, I don't think it's a good time to do that, and I don't have a
concrete plan either.

So I would say my favorite is (2). I don't think it's window dressing
at all. Profile is the owner of the various URLRequestContexts, it
knows how many there are and manages them. So it seems right to me
that it should act as the funnel and hide this information from
ExtensionsService.

I don't like (3) because it seems like a hack -- we're creating a new
notification to ensure things happen in a certain order. If we want
things to happen in a certain order we should just do so procedurally.

- a

[rafaelw, 2010-03-29 22:31:37]

Ok. Done. New snapshot for review.

[Aaron Boodman, 2010-03-29 22:50:48]

Nice.

http://codereview.chromium.org/1383001/diff/7001/8001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1383001/diff/7001/8001#newcode583
chrome/browser/extensions/extensions_service.cc:583: if (profile_ &&
!profile_->IsOffTheRecord())
I don't think the OTR part of this check is correct anymore, post matt's
changes. Get his opinion?

http://codereview.chromium.org/1383001/diff/7001/8002
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1383001/diff/7001/8002#newcode772
chrome/browser/net/chrome_url_request_context.cc:772: std::vector<std::string>&
permissions = info->second->api_permissions;
Kind of a shame to have the permission check code duplicated. Is there a way to
combine them?

http://codereview.chromium.org/1383001/diff/7001/8006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/1383001/diff/7001/8006#newcode1010
chrome/browser/profile.cc:1010: // Notify the default, extension and media
contexts on the IO thread.
Comment is out of date (there are three url request contexts).

http://codereview.chromium.org/1383001/diff/7001/8006#newcode1037
chrome/browser/profile.cc:1037: void ProfileImpl::OnExtensionUnloaded(Extension*
extension) {
Undo the database quota change here?

http://codereview.chromium.org/1383001/diff/7001/8007
File chrome/browser/profile.h (right):

http://codereview.chromium.org/1383001/diff/7001/8007#newcode298
chrome/browser/profile.h:298: virtual void OnExtensionLoaded(Extension*
extension) {}
Nit: For clarity, I would just name these more specifically until they have more
general uses. Something like UpdateExtensionsInRequestContexts() (or something
shorter if you can come up with it). You can pull the database code back into
ExtensionsSerivce::OnLoaded.

http://codereview.chromium.org/1383001/diff/7001/8008
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/1383001/diff/7001/8008#newcode938
chrome/browser/renderer_host/resource_message_filter.cc:938: if
(!source_url.SchemeIs(chrome::kExtensionScheme))
Nice catch.

[dumi, 2010-03-30 00:29:48]

the changes to database_tracker LGTM. thanks for taking the time to understand
this code and do the right thing!

[Aaron Boodman, 2010-03-30 00:51:54]

http://codereview.chromium.org/1383001/diff/24001/25001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1383001/diff/24001/25001#newcode586
chrome/browser/extensions/extensions_service.cc:586:
profile_->UpdateExtensionLoadedInRequestContexts(extension);
Sorry for naming nitpickery, but how about: profile_->UpdateRequestContexts().

http://codereview.chromium.org/1383001/diff/24001/25002
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1383001/diff/24001/25002#newcode754
chrome/browser/net/chrome_url_request_context.cc:754: // id and check its
permission.
I should have been more specific in my last comment. I was thinking something
like:

ExtensionInfo* info = NULL;
if (url.SchemeIs(extension)) {
  if (extension_info_.find(url.host()) {
    info = ...
  }
} else {
  // look for an extension with a matching extent
  info = ...
}

if (info && info.permissions.contains(permission_name)) {
  return true;
} else {
  return false;
}

Extension::HasApiPermission() seems a bit weird to me since all it does is
encapsulate a vector search.

[Aaron Boodman, 2010-03-30 03:30:48]

perfect!

lgtm