Respect content settings for Service Worker registration

Respect content settings for Service Worker registration

Add an AllowServiceWorker setting and use it when registering/unregistering
a Service Worker.

BUG=419277
Committed: https://crrev.com/3dbc36e7379336e40083eca8340e0d9471bc93ef
Cr-Commit-Position: refs/heads/master@{#299127}

[falken, 2014-10-02 09:51:41]

falken@chromium.org changed reviewers:
+ michaeln@chromium.org

[falken, 2014-10-02 09:51:42]

PTAL

I verified this works manually for the linked bug. I'd like to add unit tests if
possible (maybe using SetBrowserClientForTesting?)

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:46: //
controlling whether Service Worker is allowed.
Doing the check here would kill the renderer if the content settings disabled
service worker, which isn't appropriate here.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:61: //
controlling whether Service Worker is allowed.
Even if cookies are blocked/service worker is disabled, GetRegistration probably
should return no service workers (or a service worker that was registered before
the blocking and still exists), so it doesn't check content settings below.

[michaeln, 2014-10-02 20:32:51]

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1470: return
io_data->GetCookieSettings()->IsSettingCookieAllowed(scope,
nit: maybe use wrapping style consistent with ln 1461 and 1484 (when in rome)

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:46: //
controlling whether Service Worker is allowed.
On 2014/10/02 09:51:42, falken wrote:
> Doing the check here would kill the renderer if the content settings disabled
> service worker, which isn't appropriate here.

i remember when these methods were originally added, they were intended to be
place holders for these 'allow' browser policy checks as opposed to origin
checking for spec compliance... c'est le vie :)

CanDoThisOrThat is a very cryptic way of saying IsSameOrigin.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:61: //
controlling whether Service Worker is allowed.
On 2014/10/02 09:51:42, falken wrote:
> Even if cookies are blocked/service worker is disabled, GetRegistration
probably
> should return no service workers (or a service worker that was registered
before
> the blocking and still exists), so it doesn't check content settings below.

That's not consistent with how cookies or other things work. When blocking is
enabled, no reading/writing (no soup for) you. Pre-existing values are not
accessible. To be consistent everywhere, when blocked is turned on, content
should see no signs of sw's anywhere.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:5: #include
"content/browser/service_worker/service_worker_dispatcher_host.h"
This class covers the script accessible reg/unreg/getreg methods. We also need
to poke at where a registered sw is identified during navigation, fired up, and
used.

See ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource,
at that point we have the docurl and the reg.scope which is enough info to make
the Allow query.

[michaeln, 2014-10-02 20:46:29]

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1471: document_url);
The docurl is not a good value for the 2nd param to IsSettingCookieAllowed. That
param is there to support the 'Block third-party cookies and site data' checkbox
in the settings. It should be the url of the topmost frame in the frame
hierarchy of the page poking at sw stuff.

See appcache_host.cc for how the first_party_url value can be determined and
stored.

[falken, 2014-10-03 08:22:35]

Patchset #3 (id:40001) has been deleted

[falken, 2014-10-03 08:30:42]

Thanks! PTAL

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1470: return
io_data->GetCookieSettings()->IsSettingCookieAllowed(scope,
On 2014/10/02 20:32:51, michaeln wrote:
> nit: maybe use wrapping style consistent with ln 1461 and 1484 (when in rome)

Done (I'd just run git-cl format and didn't check closely)

https://codereview.chromium.org/625533002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1471: document_url);
On 2014/10/02 20:46:28, michaeln wrote:
> The docurl is not a good value for the 2nd param to IsSettingCookieAllowed.
That
> param is there to support the 'Block third-party cookies and site data'
checkbox
> in the settings. It should be the url of the topmost frame in the frame
> hierarchy of the page poking at sw stuff.
> 
> See appcache_host.cc for how the first_party_url value can be determined and
> stored.

Done.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (left):

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:46: //
controlling whether Service Worker is allowed.
On 2014/10/02 20:32:51, michaeln wrote:
> On 2014/10/02 09:51:42, falken wrote:
> > Doing the check here would kill the renderer if the content settings
disabled
> > service worker, which isn't appropriate here.
> 
> i remember when these methods were originally added, they were intended to be
> place holders for these 'allow' browser policy checks as opposed to origin
> checking for spec compliance... c'est le vie :)
> 
> CanDoThisOrThat is a very cryptic way of saying IsSameOrigin.

Acknowledged. Now it's doing a bit more, checking same and secure origin. I
can't think of a good name now, so leaving it as is.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:61: //
controlling whether Service Worker is allowed.
On 2014/10/02 20:32:51, michaeln wrote:
> On 2014/10/02 09:51:42, falken wrote:
> > Even if cookies are blocked/service worker is disabled, GetRegistration
> probably
> > should return no service workers (or a service worker that was registered
> before
> > the blocking and still exists), so it doesn't check content settings below.
> 
> That's not consistent with how cookies or other things work. When blocking is
> enabled, no reading/writing (no soup for) you. Pre-existing values are not
> accessible. To be consistent everywhere, when blocked is turned on, content
> should see no signs of sw's anywhere.

OK, makes sense. Done.

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/625533002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:5: #include
"content/browser/service_worker/service_worker_dispatcher_host.h"
On 2014/10/02 20:32:51, michaeln wrote:
> This class covers the script accessible reg/unreg/getreg methods. We also need
> to poke at where a registered sw is identified during navigation, fired up,
and
> used.
> 
> See
ServiceWorkerControlleeRequestHandler::DidLookupRegistrationForMainResource,
> at that point we have the docurl and the reg.scope which is enough info to
make
> the Allow query.

Right, I'll try to handle that in a next patch. The bug for this patch is about
poking at SW registrations in JavaScript.

[falken, 2014-10-06 01:20:21]

ping!

[michaeln, 2014-10-06 21:36:34]

lgtm!

[falken, 2014-10-06 23:16:18]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:20:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625533002/60001

[commit-bot: I haz the power, 2014-10-06 23:58:09]

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

[commit-bot: I haz the power, 2014-10-06 23:58:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[falken, 2014-10-07 00:33:38]

falken@chromium.org changed reviewers:
+ jochen@chromium.org

[falken, 2014-10-07 00:33:38]

jochen, can you please review the files:
    content/browser/renderer_host/render_process_host_impl.cc
    content/public/browser/content_browser_client.cc
    content/public/browser/content_browser_client.h

[jochen (gone - plz use gerrit), 2014-10-07 11:49:32]

jochen@chromium.org changed reviewers:
+ markusheintz@chromium.org

[jochen (gone - plz use gerrit), 2014-10-07 11:50:05]

+markus

It's a bit unclear to me how the interaction with content settings should be.

[michaeln, 2014-10-08 00:34:08]

There's something else missing from this CL (in addition to blocking on
navigation), the showing of the 'content blocked' ui for the tab. The renderer
side can probably trigger that upon receipt of the ErrorTypeDisabled result
code.

[falken, 2014-10-08 01:02:02]

jochen, any specific concern about the interaction with content settings?

michaeln, I think that's http://crbug.com/419284 which is P2 so I plan to leave
it for now (though it is probably easy to fix after this CL)

[michaeln, 2014-10-08 01:08:26]

> michaeln, I think that's http://crbug.com/419284 which is P2 so I plan to
leave
> it for now (though it is probably easy to fix after this CL)

yup, doing the additional things as seperate cls sgtm

[jochen (gone - plz use gerrit), 2014-10-08 14:55:16]

On 2014/10/08 at 01:02:02, falken wrote:
> jochen, any specific concern about the interaction with content settings?

hooking this up to cookie settings feels difficult to explain to users.

i'd rather first get a decision from the privacy team what kind of ux they want
before proceeding with this

[falken, 2014-10-08 15:19:58]

On 2014/10/08 14:55:16, jochen wrote:
> On 2014/10/08 at 01:02:02, falken wrote:
> > jochen, any specific concern about the interaction with content settings?
> 
> hooking this up to cookie settings feels difficult to explain to users.
> 
> i'd rather first get a decision from the privacy team what kind of ux they
want
> before proceeding with this

Yep, hooking up to cookie settings is the guidance from the privacy team. See
the bug https://code.google.com/p/chromium/issues/detail?id=419277 and
go/sw-privacy-review.

[michaeln, 2014-10-08 19:39:24]

> Yep, hooking up to cookie settings is the guidance from the privacy team. See
> the bug https://code.google.com/p/chromium/issues/detail?id=419277 and
> go/sw-privacy-review.

Also, this CL for serviceworkers corresponds with how the data for the other
storage apis are handled.

[kenjibaheux, 2014-10-09 08:13:31]

On 2014/10/08 19:39:24, michaeln wrote:
> > Yep, hooking up to cookie settings is the guidance from the privacy team.
See
> > the bug https://code.google.com/p/chromium/issues/detail?id=419277 and
> > go/sw-privacy-review.
> 
> Also, this CL for serviceworkers corresponds with how the data for the other
> storage apis are handled.

Confirming that we went through this with Chrome privacy and Markus.
See Markus's doc at go/chrome-privacy-serviceworkers.
Also, this is consistent with how its done for other related APIs.

[markusheintz_, 2014-10-09 08:42:50]

On 2014/10/09 08:13:31, kenjibaheux wrote:
> On 2014/10/08 19:39:24, michaeln wrote:
> > > Yep, hooking up to cookie settings is the guidance from the privacy team.
> See
> > > the bug <crxref style="background-image:
url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/issue.png);">https://code.google.com/p/chromium/issues/detail?id=419277</crxref>
and
> > > go/sw-privacy-review.
> > 
> > Also, this CL for serviceworkers corresponds with how the data for the other
> > storage apis are handled.
> 
> Confirming that we went through this with Chrome privacy and Markus.
> See Markus's doc at go/chrome-privacy-serviceworkers.
> Also, this is consistent with how its done for other related APIs.

I think this is the right thing to do in the short run.

[jochen (gone - plz use gerrit), 2014-10-09 11:56:49]

so far looks good.

there needs to be something that reports service worker accesses to the tab
specific content settings (both blocked and not blocked)

actually showing this information in the UI can be a separate CL, but it should
also be release blocking to implement this. Just blocking something without any
indication in the UI what happened is not good

[falken, 2014-10-09 14:00:40]

On 2014/10/09 11:56:49, jochen wrote:
> so far looks good.
> 
> there needs to be something that reports service worker accesses to the tab
> specific content settings (both blocked and not blocked)
> 
> actually showing this information in the UI can be a separate CL, but it
should
> also be release blocking to implement this. Just blocking something without
any
> indication in the UI what happened is not good

Thanks for reviewing.

Sounds reasonable to block shipping on the UI especially if privacy team demands
it. Would it be acceptable to add the recording of accesses in the same CL that
adds the UI, since:
- adding recording without a UI doesn't seem to buy us much 
- splitting the CLs that way might save a review round trip

[jochen (gone - plz use gerrit), 2014-10-10 11:08:33]

fair enough

lgtm

[falken, 2014-10-10 15:39:51]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-10 15:40:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625533002/80001

[commit-bot: I haz the power, 2014-10-10 16:48:18]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-10 16:49:02]

Patchset 4 (id:??) landed as
https://crrev.com/3dbc36e7379336e40083eca8340e0d9471bc93ef
Cr-Commit-Position: refs/heads/master@{#299127}