Allows AwCookieManager to block ThirdParty cookies

Allows AwCookieManager to block ThirdParty cookies

BUG=11678084
TEST=AndroidWebviewTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266013

[mkosiba (inactive), 2014-04-17 18:27:30]

looks good :)

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_cookie_access_policy.h (right):

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy.h:80: // BLOCK_ALL_COOKIES and
BLOCK_ALL_THIRD_PARTY_COOKIES) depending on
this is a bit too detailed - you can see all this from looking a the code. Maybe
move the bit you added at the top of the class here?

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy.h:86: // are allowed to get/set
that cookie.
redundant comment - you can infer as much from the name.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_cookie_access_policy_unittest.cc (right):

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:43:
url_google_analytics_("http://www.googleanalytics.izzle") {
url_first_party_("http://first/")
url_third_party_("http://third/")

also, these could be static constants.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:60: 
are any of the lines below actually needed for the test to pass?

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:70: return
AwCookieAccessPolicy::GetInstance()->GetGlobalAllowAccess();
to make these shorter maybe add a method to your test class?

  AwCookieAccessPolicy* policy() {
    return AwCookieAccessPolicy::GetInstance();
  }

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:146:
EXPECT_TRUE(AllowSetCookie(url_google_, url_google_, "", NULL, 0, 0, NULL));
since you've already bothered to write test wrappers of these methods might as
well let them supply the default params like null or 0.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:189:
SetThirdPartyAllowAccess(true);
this should probably be a separate test

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:255:
TestWebServer thirdParty = webServer;
I think this is redundant given the comment above.

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:273:
cookieUrl = cookieUrl.replace("localhost", "127.0.0.1");
maybe have makeUrlWithCookie take an extra boolean (or enum) that says whether
it's supposed to fake 'third party' and move this line to that method?

[hjd_google, 2014-04-22 13:34:41]

ptal

Think I've fixed most of the issues, the unittests look a lot better now :)

In terms of
https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
did you mean just the comments or the separate variables too?

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_cookie_access_policy.h (right):

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy.h:80: // BLOCK_ALL_COOKIES and
BLOCK_ALL_THIRD_PARTY_COOKIES) depending on
On 2014/04/17 18:27:30, mkosiba wrote:
> this is a bit too detailed - you can see all this from looking a the code.
Maybe
> move the bit you added at the top of the class here?

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy.h:86: // are allowed to get/set
that cookie.
On 2014/04/17 18:27:30, mkosiba wrote:
> redundant comment - you can infer as much from the name.

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
File android_webview/browser/aw_cookie_access_policy_unittest.cc (right):

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:43:
url_google_analytics_("http://www.googleanalytics.izzle") {
On 2014/04/17 18:27:30, mkosiba wrote:
> url_first_party_("http://first/")
> url_third_party_("http://third/")
> 
> also, these could be static constants.

Yeah, these were the values from the existing cookie unittests they did seem
kind of unhelpful.

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:60: 
On 2014/04/17 18:27:30, mkosiba wrote:
> are any of the lines below actually needed for the test to pass?

Apparently not, I'm confused about what errors I was seeing before.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:70: return
AwCookieAccessPolicy::GetInstance()->GetGlobalAllowAccess();
On 2014/04/17 18:27:30, mkosiba wrote:
> to make these shorter maybe add a method to your test class?
> 
>   AwCookieAccessPolicy* policy() {
>     return AwCookieAccessPolicy::GetInstance();
>   }

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:146:
EXPECT_TRUE(AllowSetCookie(url_google_, url_google_, "", NULL, 0, 0, NULL));
On 2014/04/17 18:27:30, mkosiba wrote:
> since you've already bothered to write test wrappers of these methods might as
> well let them supply the default params like null or 0.

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/browser/aw_c...
android_webview/browser/aw_cookie_access_policy_unittest.cc:189:
SetThirdPartyAllowAccess(true);
On 2014/04/17 18:27:30, mkosiba wrote:
> this should probably be a separate test

Done.

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:255:
TestWebServer thirdParty = webServer;
On 2014/04/17 18:27:30, mkosiba wrote:
> I think this is redundant given the comment above.

Do you mean the assignments or just the comments?

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:273:
cookieUrl = cookieUrl.replace("localhost", "127.0.0.1");
On 2014/04/17 18:27:30, mkosiba wrote:
> maybe have makeUrlWithCookie take an extra boolean (or enum) that says whether
> it's supposed to fake 'third party' and move this line to that method?

Done.

[mkosiba (inactive), 2014-04-23 12:51:22]

looks good, just a couple more nits

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:255:
TestWebServer thirdParty = webServer;
On 2014/04/22 13:34:41, hjd wrote:
> On 2014/04/17 18:27:30, mkosiba wrote:
> > I think this is redundant given the comment above.
> 
> Do you mean the assignments or just the comments?

the assignments.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy.cc:94: return
StaticCookiePolicy(GetPolicy()).CanSetCookie(url, first_party)
nit: since get and set have the same policy it might be a bit less code to move
GetPolicy here and then implement AllowGet as:

bool AwCookieAccessPolicy::AllowGet(...) {
  return AllowSet(...);
}

up to you though.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_cookie_access_policy_unittest.cc (right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Back to the future? It's 2014 now :)

Seriously though: New files get the current year (and the new policy is to not
include the '(c)' bit), you don't touch the copyright on old files though.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:7: #include
"base/run_loop.h"
needed?

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:98:
base::MessageLoopForIO loop_;
do you still need this?

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:115:
EXPECT_TRUE(AllowSetCookie(kUrlFirstParty, kUrlFirstParty));
you could refactor this into a separate method, something like

  void DoCookiePolicyTest(bool expected_accept_first_party, bool
expected_accept_third_party) {
    EXPECT_EQ(expected_accept_first_party, AllowSetCookie(...));
    ...
  }

and call that from each of the cases

or use gtest's built in support:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:240:
public void testThirdPartyCookie() throws Throwable {
like I mentioned offline - you probably want a test that uses the JS API too.

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:267:
String cookieUrl, url;
I think the style is to declare + initialize. If the line is too long break
after =, so

  String cookieUrl =
      makeUrl...

cant't remember if indent is supposed to be 4 or 8 spaces in this case though.

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:303:
private String makeUrlWithCookie(TestWebServer webServer, String path, String
key,
seems like you only ever call this with thirdParty == true, so maybe rename
method to makeThridPartyUrlWithCookie(...) and drop the extra bool?
Alternatively have two methods:
makeUrlWithCookie(...) and toThirdPartyUrl(String url) { return url.replace... }

[hjd_google, 2014-04-24 12:48:30]

ptal

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_cookie_access_policy.cc (right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy.cc:94: return
StaticCookiePolicy(GetPolicy()).CanSetCookie(url, first_party)
On 2014/04/23 12:51:22, mkosiba wrote:
> nit: since get and set have the same policy it might be a bit less code to
move
> GetPolicy here and then implement AllowGet as:
> 
> bool AwCookieAccessPolicy::AllowGet(...) {
>   return AllowSet(...);
> }
> 
> up to you though.

I'll leave this one as we said.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_cookie_access_policy_unittest.cc (right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/04/23 12:51:22, mkosiba wrote:
> Back to the future? It's 2014 now :)
> 
> Seriously though: New files get the current year (and the new policy is to not
> include the '(c)' bit), you don't touch the copyright on old files though.

The dangers of copy-paste :)

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:7: #include
"base/run_loop.h"
On 2014/04/23 12:51:22, mkosiba wrote:
> needed?

See below.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:98:
base::MessageLoopForIO loop_;
On 2014/04/23 12:51:22, mkosiba wrote:
> do you still need this?

When I remove it I get an error at url_request.cc(295) because 
URLRequest::Init DCHECKs base::MessageLoop::current() :(

URLRequest doesn't seem to use MessageLoop directly but I left it in for now.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:115:
EXPECT_TRUE(AllowSetCookie(kUrlFirstParty, kUrlFirstParty));
On 2014/04/23 12:51:22, mkosiba wrote:
> you could refactor this into a separate method, something like
> 
>   void DoCookiePolicyTest(bool expected_accept_first_party, bool
> expected_accept_third_party) {
>     EXPECT_EQ(expected_accept_first_party, AllowSetCookie(...));
>     ...
>   }
> 
> and call that from each of the cases
> 
> or use gtest's built in support:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...
>

I had a look at the parameterized tests I think it would come out looking like
this (pseudocode): 
 
TEST_P(AwCookieAccessPolicyTest, CookiePolicy) {
  globalAllowAccess, thirdPartyAllowAccess, 
      expectedFirstParty expectedThirdParty = GetParams();

  SetGlobalAllowAccess(globalAllowAccess);
  SetThirdPartyAllowAccess(thirdPartyAllowAccess);

  EXPECT_EQ(expectedFirstParty, AllowSetCookie(...));
  EXPECT_EQ(expectedFirstParty, AllowGetCookie(...));
  EXPECT_EQ(expectedFirstParty, OnCanGetCookies(...));
  EXPECT_EQ(expectedFirstParty, OnCanSetCookie(...));

  EXPECT_EQ(expectedThirdParty, AllowSetCookie(...));
  EXPECT_EQ(expectedThirdParty, AllowGetCookie(...));
  EXPECT_EQ(expectedThirdParty, OnCanGetCookies(...));
  EXPECT_EQ(expectedThirdParty, OnCanSetCookie(...));
}

INSTANTIATE_TEST_CASE_P(
    CookiePolicy,
    AwCookieAccessPolicyTest,
    ::testing::Values(
    // global, thirdparty, firstparty allowed, thirdparty allowed 
    {false, false, false, false},
    {false, true,  false,  false},
    {true,  false, true,  false},
    {true,  true,  true,  true},
));

Which is really concise but I'm not sure how readable it is especially since the
tests would come out named:

PolicyTests/AwCookieTest.Policy/0 for {true, true, false false}
etc.

which isn't especially useful.

Currently I've kept the explicitly separate tests but extracted some of the
duplication, let me know what you think.

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:240:
public void testThirdPartyCookie() throws Throwable {
On 2014/04/23 12:51:22, mkosiba wrote:
> like I mentioned offline - you probably want a test that uses the JS API too.

Done.

I added it as a separate test for clarity.

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:267:
String cookieUrl, url;
On 2014/04/23 12:51:22, mkosiba wrote:
> I think the style is to declare + initialize. If the line is too long break
> after =, so
> 
>   String cookieUrl =
>       makeUrl...
> 
> cant't remember if indent is supposed to be 4 or 8 spaces in this case though.

Done.

https://codereview.chromium.org/241143002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:303:
private String makeUrlWithCookie(TestWebServer webServer, String path, String
key,
On 2014/04/23 12:51:22, mkosiba wrote:
> seems like you only ever call this with thirdParty == true, so maybe rename
> method to makeThridPartyUrlWithCookie(...) and drop the extra bool?
> Alternatively have two methods:
> makeUrlWithCookie(...) and toThirdPartyUrl(String url) { return url.replace...
}

Done.

[mkosiba (inactive), 2014-04-24 13:19:28]

lgtm

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_cookie_access_policy_unittest.cc (right):

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:98:
base::MessageLoopForIO loop_;
On 2014/04/24 12:48:30, hjd wrote:
> On 2014/04/23 12:51:22, mkosiba wrote:
> > do you still need this?
> 
> When I remove it I get an error at url_request.cc(295) because 
> URLRequest::Init DCHECKs base::MessageLoop::current() :(
> 
> URLRequest doesn't seem to use MessageLoop directly but I left it in for now.

ah, ok. That's fine. It just means that URLRequests call out the IO message loop
explicitly by id.

https://codereview.chromium.org/241143002/diff/20001/android_webview/browser/...
android_webview/browser/aw_cookie_access_policy_unittest.cc:115:
EXPECT_TRUE(AllowSetCookie(kUrlFirstParty, kUrlFirstParty));
On 2014/04/24 12:48:30, hjd wrote:
> On 2014/04/23 12:51:22, mkosiba wrote:
> > you could refactor this into a separate method, something like
> > 
> >   void DoCookiePolicyTest(bool expected_accept_first_party, bool
> > expected_accept_third_party) {
> >     EXPECT_EQ(expected_accept_first_party, AllowSetCookie(...));
> >     ...
> >   }
> > 
> > and call that from each of the cases
> > 
> > or use gtest's built in support:
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...
> >
> 
> I had a look at the parameterized tests I think it would come out looking like
> this (pseudocode): 
>  
> TEST_P(AwCookieAccessPolicyTest, CookiePolicy) {
>   globalAllowAccess, thirdPartyAllowAccess, 
>       expectedFirstParty expectedThirdParty = GetParams();
> 
>   SetGlobalAllowAccess(globalAllowAccess);
>   SetThirdPartyAllowAccess(thirdPartyAllowAccess);
> 
>   EXPECT_EQ(expectedFirstParty, AllowSetCookie(...));
>   EXPECT_EQ(expectedFirstParty, AllowGetCookie(...));
>   EXPECT_EQ(expectedFirstParty, OnCanGetCookies(...));
>   EXPECT_EQ(expectedFirstParty, OnCanSetCookie(...));
> 
>   EXPECT_EQ(expectedThirdParty, AllowSetCookie(...));
>   EXPECT_EQ(expectedThirdParty, AllowGetCookie(...));
>   EXPECT_EQ(expectedThirdParty, OnCanGetCookies(...));
>   EXPECT_EQ(expectedThirdParty, OnCanSetCookie(...));
> }
> 
> INSTANTIATE_TEST_CASE_P(
>     CookiePolicy,
>     AwCookieAccessPolicyTest,
>     ::testing::Values(
>     // global, thirdparty, firstparty allowed, thirdparty allowed 
>     {false, false, false, false},
>     {false, true,  false,  false},
>     {true,  false, true,  false},
>     {true,  true,  true,  true},
> ));
> 
> Which is really concise but I'm not sure how readable it is especially since
the
> tests would come out named:
> 
> PolicyTests/AwCookieTest.Policy/0 for {true, true, false false}
> etc.
> 
> which isn't especially useful.
> 
> Currently I've kept the explicitly separate tests but extracted some of the
> duplication, let me know what you think.

current approach looks great!

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:267:
String cookieUrl = makeCookieUrl(webServer, "/cookie_1.js", "test1", "value1");
nit: I'd say
    toThirdPartyUrl(makeCookieUrl(...

or 

    toThirdPartyUrl(
        makeCookieUrl(...

is clearer. up to you though.

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:340:
String cookieUrl
cookieScriptUrl? :)

[hjd_google, 2014-04-24 13:47:08]

fixed toThirdPartyUrl

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java
(right):

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:267:
String cookieUrl = makeCookieUrl(webServer, "/cookie_1.js", "test1", "value1");
On 2014/04/24 13:19:29, mkosiba wrote:
> nit: I'd say
>     toThirdPartyUrl(makeCookieUrl(...
> 
> or 
> 
>     toThirdPartyUrl(
>         makeCookieUrl(...
> 
> is clearer. up to you though.

Yeah I've changed it, I couldn't decide between having to break the line and
mutating the string variable but on balance I feel worse about the mutation.

https://codereview.chromium.org/241143002/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java:340:
String cookieUrl
On 2014/04/24 13:19:29, mkosiba wrote:
> cookieScriptUrl? :)

I left this to be consistent with the others since it wasn't going to fit on the
line anyway with toThirdPartyUrl :)

[mkosiba (inactive), 2014-04-24 14:51:55]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-04-24 14:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hjd@google.com/241143002/50001

[commit-bot: I haz the power, 2014-04-24 22:36:45]

Change committed as 266013