[DurableStorage] Don't grant durable if origin cannot write cookies.

[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183, 652853, 521082
Committed: https://crrev.com/49882a41627a37025f9b5e3bdc3335ce8e0a2ffe
Cr-Commit-Position: refs/heads/master@{#423072}

[dmurph, 2016-09-30 19:07:29]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

BUG=521183
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln
BUG=521183
==========

[dmurph, 2016-09-30 19:07:39]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln
BUG=521183
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org
BUG=521183
==========

[dmurph, 2016-09-30 21:35:05]

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

[commit-bot: I haz the power, 2016-09-30 21:35:32]

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

[dmurph, 2016-09-30 21:36:19]

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

[dmurph, 2016-09-30 21:36:19]

Hey Michael,

This is a small patch to deny durable storage requests when cookies aren't
writable. I have test in there too. PTAL.

Thanks,
Daniel

[dmurph, 2016-09-30 21:42:47]

dmurph@chromium.org changed reviewers:
+ jww@chromium.org

[dmurph, 2016-09-30 21:42:47]

+Joel, just to make sure I'm doing this check correctly

[dmurph, 2016-09-30 21:43:01]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org
BUG=521183
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183
==========

[commit-bot: I haz the power, 2016-09-30 21:52:29]

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

[commit-bot: I haz the power, 2016-09-30 21:52:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[michaeln, 2016-09-30 22:19:03]

i'm not too familiar with this, hope my comment/question makes sense?

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:48:
embedding_origin)) {
The comment in the .h file says "or already granted", but i don't see any code
for that?

"Durability" is an attribute of an origin's stored data, but
IsSettingCookieAllowed() is dependent on the cross-origin-ness of a documents
parent document. True and False can't be the right answer at the same time.

If the permission has already been granted for an origin. I'm not sure a call
from a nested iframe should result in its revocation?

I think it probably makes sense that a call from a nested iframe can't initially
establish the permission.

[michaeln, 2016-09-30 22:21:21]

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:48:
embedding_origin)) {
just read the bug... maybe use IsSettingCookiesAllowed(requesting_origin,
requesting_origin) to eliminate 3rd party considerations

[jww, 2016-10-01 04:46:20]

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:38: const GURL&
embedding_origin,
Is embedding_origin guaranteed to be the top-level? I *think* first_party in the
call to IsSettingCookieAllowed is meant to be the top level origin, not just the
origin that has embedded the requester.

That is, if we have the setup:

top level frame
    |-> middle frame
        |-> requesting frame

I believe first_party should be the URL of "top level frame."

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:48:
embedding_origin)) {
On 2016/09/30 22:21:21, michaeln wrote:
> just read the bug... maybe use IsSettingCookiesAllowed(requesting_origin,
> requesting_origin) to eliminate 3rd party considerations

I don't know what dmurph@'s actually intent here, but if you're referring to
https://crbug.com/521183, it does say that the permission decision should match
the decision for cookies, so I don't think 3rd party considerations should
necessarily be eliminated.

Note that this isn't just for the "First-party cookie" attribute, but it is also
used for things like Web UI.

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:195:
TEST_F(DurableStoragePermissionContextTest, CookiesNotAllowed) {
You probably want a "CookiesAllowed" variant of this test to make sure that
durable storage is allowed, no?

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:216:
permission_context.DecidePermission(web_contents(), id, url, url,
Maybe a few other sanity checks of other variants of this? Like with different
emebedder and requestor URLs? Also user gesture false? Things like that?

[jww, 2016-10-01 04:46:57]

On 2016/10/01 04:46:20, jww wrote:
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> File chrome/browser/storage/durable_storage_permission_context.cc (right):
> 
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> chrome/browser/storage/durable_storage_permission_context.cc:38: const GURL&
> embedding_origin,
> Is embedding_origin guaranteed to be the top-level? I *think* first_party in
the
> call to IsSettingCookieAllowed is meant to be the top level origin, not just
the
> origin that has embedded the requester.
> 
> That is, if we have the setup:
> 
> top level frame
>     |-> middle frame
>         |-> requesting frame
> 
> I believe first_party should be the URL of "top level frame."
> 
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> chrome/browser/storage/durable_storage_permission_context.cc:48:
> embedding_origin)) {
> On 2016/09/30 22:21:21, michaeln wrote:
> > just read the bug... maybe use IsSettingCookiesAllowed(requesting_origin,
> > requesting_origin) to eliminate 3rd party considerations
> 
> I don't know what dmurph@'s actually intent here, but if you're referring to
> https://crbug.com/521183, it does say that the permission decision should
match
> the decision for cookies, so I don't think 3rd party considerations should
> necessarily be eliminated.
> 
> Note that this isn't just for the "First-party cookie" attribute, but it is
also
> used for things like Web UI.
> 
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> File chrome/browser/storage/durable_storage_permission_context_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> chrome/browser/storage/durable_storage_permission_context_unittest.cc:195:
> TEST_F(DurableStoragePermissionContextTest, CookiesNotAllowed) {
> You probably want a "CookiesAllowed" variant of this test to make sure that
> durable storage is allowed, no?
> 
>
https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
> chrome/browser/storage/durable_storage_permission_context_unittest.cc:216:
> permission_context.DecidePermission(web_contents(), id, url, url,
> Maybe a few other sanity checks of other variants of this? Like with different
> emebedder and requestor URLs? Also user gesture false? Things like that?

Also, FYI, I'll be OOO on Monday, so I won't be able to review again until
Tuesday.

[dmurph, 2016-10-04 22:00:03]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

[dmurph, 2016-10-04 22:00:20]

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

[dmurph, 2016-10-04 22:00:45]

I added a couple more checks here, relevant bugs added.

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:38: const GURL&
embedding_origin,
On 2016/10/01 04:46:20, jww wrote:
> Is embedding_origin guaranteed to be the top-level? I *think* first_party in
the
> call to IsSettingCookieAllowed is meant to be the top level origin, not just
the
> origin that has embedded the requester.
> 
> That is, if we have the setup:
> 
> top level frame
>     |-> middle frame
>         |-> requesting frame
> 
> I believe first_party should be the URL of "top level frame."

Ok, so I removed this check, and we just check the requesting origin.

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:48:
embedding_origin)) {
On 2016/09/30 22:19:03, michaeln wrote:
> The comment in the .h file says "or already granted", but i don't see any code
> for that?
> 
> "Durability" is an attribute of an origin's stored data, but
> IsSettingCookieAllowed() is dependent on the cross-origin-ness of a documents
> parent document. True and False can't be the right answer at the same time.
> 
> If the permission has already been granted for an origin. I'm not sure a call
> from a nested iframe should result in its revocation?
> 
> I think it probably makes sense that a call from a nested iframe can't
initially
> establish the permission.
> 

This comment was wrong - we only call this if the content setting is DEFAULT -
if we're block or allow, we don't call this. See:
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co...

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:48:
embedding_origin)) {
On 2016/10/01 04:46:20, jww wrote:
> On 2016/09/30 22:21:21, michaeln wrote:
> > just read the bug... maybe use IsSettingCookiesAllowed(requesting_origin,
> > requesting_origin) to eliminate 3rd party considerations
> 
> I don't know what dmurph@'s actually intent here, but if you're referring to
> https://crbug.com/521183, it does say that the permission decision should
match
> the decision for cookies, so I don't think 3rd party considerations should
> necessarily be eliminated.
> 
> Note that this isn't just for the "First-party cookie" attribute, but it is
also
> used for things like Web UI.

Ok, so I'm now also blocking granting this permission if we're in an embedded
frame, so this should solve both cases. (This is what the push notification
permission does).

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:195:
TEST_F(DurableStoragePermissionContextTest, CookiesNotAllowed) {
On 2016/10/01 04:46:20, jww wrote:
> You probably want a "CookiesAllowed" variant of this test to make sure that
> durable storage is allowed, no?

Cookies are allowed in all other tests.

https://codereview.chromium.org/2385653005/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:216:
permission_context.DecidePermission(web_contents(), id, url, url,
On 2016/10/01 04:46:20, jww wrote:
> Maybe a few other sanity checks of other variants of this? Like with different
> emebedder and requestor URLs? Also user gesture false? Things like that?

Added test for different embedder & requestor urls, which now should be blocked.

User gestures aren't considered.

[commit-bot: I haz the power, 2016-10-04 22:01:16]

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-10-04 22:41:18]

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

[commit-bot: I haz the power, 2016-10-04 22:41:18]

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

[jww, 2016-10-05 00:19:40]

lgtm with one clarification question.

https://codereview.chromium.org/2385653005/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/60001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:43: // Durable is
only allowed to be granted on top-level origins.
Just to clarify what you want to do here, this allows the permission to be set
if the request originates in an embedded *frame*, and the requirement is just
that that embedded frame's origin must match the top-level frame. Is that what
you want?

[jww, 2016-10-05 00:19:42]

lgtm with one clarification question.

[michaeln, 2016-10-05 00:22:41]

lgtm, but please see comments

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:41:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
A comment about how this is only expected to be called when the permission has
not yet be granted might be good. Otherwise it looks possible for a top level
frame and a nested cross origin iframe to get different answers for the same
origin and the same time, which wouldn't make sense.

Better yet, DCHECKs would speak even louder than a comment.

DCHECK_NEQ(ALLOWED, GetPermissionStatus(requesting_origin, embedding_origin));
DCHECK_NEQ(BLOCKED, GetPermissionStatus(requesting_origin, embedding_origin));

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:63: false /*
persist */, CONTENT_SETTING_DEFAULT);
Yup, DEFAULT seems right here, as a sticky BLOCK setting would otherwise prevent
future cookie setting changes from allowing durability.

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:75: true /* persist
*/, CONTENT_SETTING_ALLOW);
Q: Not about this CL specifically, but does unbookmarking revoke the permission?
Looks like ALLOW is persisted which should prevent future DecidePermission()
invocations?

Ultimately, the system will treat it as durable until this content setting value
is modified.

bool CookieSettings::IsStorageDurable(const GURL& origin) const {
  // TODO(dgrogan): Don't use host_content_settings_map_ directly.
  // https://crbug.com/539538
  ContentSetting setting = host_content_settings_map_->GetContentSetting(
      origin /*primary*/, origin /*secondary*/,
      CONTENT_SETTINGS_TYPE_DURABLE_STORAGE,
      std::string() /*resource_identifier*/);
  return setting == CONTENT_SETTING_ALLOW;
}

[dmurph, 2016-10-05 03:21:51]

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

[commit-bot: I haz the power, 2016-10-05 03:22:03]

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

[dmurph, 2016-10-05 03:22:37]

Thanks!

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:41:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2016/10/05 00:22:40, michaeln wrote:
> A comment about how this is only expected to be called when the permission has
> not yet be granted might be good. Otherwise it looks possible for a top level
> frame and a nested cross origin iframe to get different answers for the same
> origin and the same time, which wouldn't make sense.
> 
> Better yet, DCHECKs would speak even louder than a comment.
> 
> DCHECK_NEQ(ALLOWED, GetPermissionStatus(requesting_origin, embedding_origin));
> DCHECK_NEQ(BLOCKED, GetPermissionStatus(requesting_origin, embedding_origin));

Done.

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:63: false /*
persist */, CONTENT_SETTING_DEFAULT);
On 2016/10/05 00:22:40, michaeln wrote:
> Yup, DEFAULT seems right here, as a sticky BLOCK setting would otherwise
prevent
> future cookie setting changes from allowing durability.

Acknowledged.

https://codereview.chromium.org/2385653005/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:75: true /* persist
*/, CONTENT_SETTING_ALLOW);
On 2016/10/05 00:22:40, michaeln wrote:
> Q: Not about this CL specifically, but does unbookmarking revoke the
permission?
> Looks like ALLOW is persisted which should prevent future DecidePermission()
> invocations?
> 
> Ultimately, the system will treat it as durable until this content setting
value
> is modified.
> 
> bool CookieSettings::IsStorageDurable(const GURL& origin) const {
>   // TODO(dgrogan): Don't use host_content_settings_map_ directly.
>   // https://crbug.com/539538
>   ContentSetting setting = host_content_settings_map_->GetContentSetting(
>       origin /*primary*/, origin /*secondary*/,
>       CONTENT_SETTINGS_TYPE_DURABLE_STORAGE,
>       std::string() /*resource_identifier*/);
>   return setting == CONTENT_SETTING_ALLOW;
> }

Nope, we currently give it forever, until the user clears browsing data. We'll
have to solve that in a future patch. I put that bug on my shortlist.

https://codereview.chromium.org/2385653005/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2385653005/diff/60001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:43: // Durable is
only allowed to be granted on top-level origins.
On 2016/10/05 00:19:40, jww wrote:
> Just to clarify what you want to do here, this allows the permission to be set
> if the request originates in an embedded *frame*, and the requirement is just
> that that embedded frame's origin must match the top-level frame. Is that what
> you want?

It can happen at the top level origin or in a frame w/ the same origin as the
top level origin. The embedding origin is the last committed url on the web
contents, it is always populated. See:
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_co...

[dmurph, 2016-10-05 03:24:29]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-10-05 03:24:34]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-10-05 03:24:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jww@chromium.org
Link to the patchset: https://codereview.chromium.org/2385653005/#ps80001
(title: "addressed comments, added test")

[commit-bot: I haz the power, 2016-10-05 03:24:50]

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

[dmurph, 2016-10-05 04:06:04]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies or is in a
third party frame.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

[commit-bot: I haz the power, 2016-10-05 04:06:08]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies or is in a
third party frame.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

[commit-bot: I haz the power, 2016-10-05 04:06:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-05 04:08:09]

Description was changed from

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082
==========

to

==========
[DurableStorage] Don't grant durable if origin cannot write cookies.

R=michaeln@chromium.org, jww@chromium.org
BUG=521183,652853,521082

Committed: https://crrev.com/49882a41627a37025f9b5e3bdc3335ce8e0a2ffe
Cr-Commit-Position: refs/heads/master@{#423072}
==========

[commit-bot: I haz the power, 2016-10-05 04:08:10]

Patchset 5 (id:??) landed as
https://crrev.com/49882a41627a37025f9b5e3bdc3335ce8e0a2ffe
Cr-Commit-Position: refs/heads/master@{#423072}