Comment on attachment 152495 [details] [review]
accept-policies.diff

Is there a corresponding webkit bug yet?

>+ * Keep in mind that if the #SoupCookieJarAcceptPolicy
>+ * %SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY is set you'll need to use
>+ * soup_cookie_jar_set_cookie_with_third_party(), otherwise the jar

typo in last line (s/third/first/)

>+ * soup_cookie_jar_set_cookie_with_first_party:
>+ * @first_party: the URI for the main document

I think you need a little more explanation (either there or in the main function doc). "the URI for the main document" only makes sense if you already know what it means.

Actually, it looks like you aren't documenting the SoupCookieJarAcceptPolicy enum, so you can describe what first-party/third-party means in depth there, and then just refer the reader to that from other places.

>+soup_cookie_jar_set_cookie_with_first_party (SoupCookieJar *jar, SoupURI *uri,
>+					     const char *first_party,

would @first_party make more sense as a SoupURI?

An alternative API here would be instead of passing the first-party URI, to instead pass the SoupMessage that the cookie originated from, and then just call soup_message_get_first_party() on that message. Actually, you wouldn't need @uri in that case either... (I wonder why we didn't do it that way before?) But that also seems like it could potentially also be more useful if someone else adds an "accept-cookie" signal later, for implementing more complicated policies.

> process_set_cookie_header (SoupMessage *msg, gpointer user_data)

you're re-parsing the message's first_party string into a SoupURI every time through the loop here. Though that would become irrelevant if soup_message_get_first_party_uri returned a SoupURI...

>+		if (first_party == NULL &&
>+		    priv->accept_policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY)
>+			continue; /* Can't check anything */

Not certain that's the right choice, especially since it's inconsistent with soup_cookie_jar_set_cookie()

>+		     soup_cookie_domain_matches (nc->data, uri->host) == TRUE) ||

ew. don't say "== TRUE"

>+ * soup_cookie_domain_matches:
>+ * @cookie: a #SoupCookie
>+ * @host: a URI
>+ *
>+ * Checks if the @cookies's domain and @host match in the sense that
>+ * @cookie should be sent when making a request to @host.

Extra "s" in "@cookies's". And also this check is used for two different purposes:

 * Checks if @cookie's domain matches @host in the sense that
 * @cookie should be sent when making a request to @host, or
 * that @cookie should be accepted when receiving a response
 * from @host.

>+	g_return_val_if_fail (cookie, FALSE);
>+	g_return_val_if_fail (host, FALSE);

Standard gnome style is to say "cookie != NULL", etc in return_if_fails, so that you get a more explicit error message. ("assertion 'cookie != NULL' failed")

>+	 * Alias for the #SoupMessage:first-party property. (The URI
>+	 * loaded in the application when the message was queued.)

also very unclear if you don't already know what it means

>+	g_object_class_install_property (
>+		object_class, PROP_FIRST_PARTY,
>+		g_param_spec_string (SOUP_MESSAGE_FIRST_PARTY,

This definitely seems like it should be a SoupURI...

>+const char*
>+soup_message_get_first_party (SoupMessage *msg)

likewise, etc

>+	cookies-accept-policy-test \

just call it cookie-test. Then you don't have to reindent everything. (And we need more cookie tests in the future anyway, so we can just keep adding them to this file. kov actually included some others in bug 605543, but the test program was stylistically wrong so I didn't end up using it.)

>+    else
>+        g_assert_not_reached ();

I try to avoid g_assert/g_error in the libsoup tests, so that I can get as many errors as possible out of each test program rather than having it bail out on the first one. You can just change this to "g_return_if_reached()" though, and that will still cause the test to fail if it's hit (because test_init() sets up its own g_log handler).

>+            g_assert (g_slist_length (l) == validResults[i].n_cookies);

likewise, just do an if, and a debug_printf followed by errors++ if they don't match

(In reply to comment #1)
> >+soup_cookie_jar_set_cookie_with_first_party (SoupCookieJar *jar, SoupURI *uri,
> >+					     const char *first_party,
> 
> would @first_party make more sense as a SoupURI?
> 
> An alternative API here would be instead of passing the first-party URI, to
> instead pass the SoupMessage that the cookie originated from, and then just
> call soup_message_get_first_party() on that message. Actually, you wouldn't
> need @uri in that case either... (I wonder why we didn't do it that way
> before?) But that also seems like it could potentially also be more useful if
> someone else adds an "accept-cookie" signal later, for implementing more
> complicated policies.

This cannot work, since (I'd say) the set_cookie APIs are mostly meant for cases when you are setting cookies that do not come from HTTP headers. In our case, it's JS cookies, so there's simply no SoupMessage to pass.

> 
> > process_set_cookie_header (SoupMessage *msg, gpointer user_data)
> 
> you're re-parsing the message's first_party string into a SoupURI every time
> through the loop here. Though that would become irrelevant if
> soup_message_get_first_party_uri returned a SoupURI...

Right, we can do this.

> 
> >+		if (first_party == NULL &&
> >+		    priv->accept_policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY)
> >+			continue; /* Can't check anything */
> 
> Not certain that's the right choice, especially since it's inconsistent with
> soup_cookie_jar_set_cookie()

You mean it's inconsistent because there's a warning in the set_cookie function?

> >+	cookies-accept-policy-test \
> 
> just call it cookie-test. Then you don't have to reindent everything. (And we
> need more cookie tests in the future anyway, so we can just keep adding them to
> this file. kov actually included some others in bug 605543, but the test
> program was stylistically wrong so I didn't end up using it.)

Heh, ok. I actually thought of doing this out of laziness, but then decided it was worth it to give it a proper name :)

> 
> >+    else
> >+        g_assert_not_reached ();
> 
> I try to avoid g_assert/g_error in the libsoup tests, so that I can get as many
> errors as possible out of each test program rather than having it bail out on
> the first one. You can just change this to "g_return_if_reached()" though, and
> that will still cause the test to fail if it's hit (because test_init() sets up
> its own g_log handler).
> 
> >+            g_assert (g_slist_length (l) == validResults[i].n_cookies);
> 
> likewise, just do an if, and a debug_printf followed by errors++ if they don't
> match

Right, will do that.

(In reply to comment #2)
> > An alternative API here would be instead of passing the first-party URI, to
> > instead pass the SoupMessage that the cookie originated from

> This cannot work, since (I'd say) the set_cookie APIs are mostly meant for
> cases when you are setting cookies that do not come from HTTP headers. In our
> case, it's JS cookies, so there's simply no SoupMessage to pass.

Hm... I was assuming there was some way to get a SoupMessage from the Document, but I guess it's already gone by this point? OK, so a SoupURI then.

> > Not certain that's the right choice, especially since it's inconsistent with
> > soup_cookie_jar_set_cookie()
> 
> You mean it's inconsistent because there's a warning in the set_cookie
> function?

I think when I wrote that I thought the cookie was being accepted in this case.

Still, as currently written, if an app sets a cookie policy of NO_THIRD_PARTY, but doesn't call soup_message_set_first_party() on a message, then cookies received via HTTP would be silently dropped, but cookies received via javascript would be noisily dropped? Or I guess you're expecting that WebKit won't call soup_cookie_jar_set_cookie_with_first_party() if document.firstPartyForCookies is null? Actually, that makes sense. OK, keep it like it is. The docs for NO_THIRD_PARTY should mention this though (that if you set that policy and don't use _set_first_party, then the cookie will be assumed to be third party).

I think it would make sense to implement an additional policy, SOUP_COOKIE_JAR_ACCEPT_ASK, that would make use of a signal, and accept the cookie if the callback returned TRUE (and reject it if it returned FALSE or there was no callback registered). This would help reimplementing bug 337826 for ephy/webkit.

I'd be happy to try to implement it if it sounds like a good idea.

i was assuming we'd want something like that at some point, yeah. but i don't think there needs to be another policy type for it; the signal could just be in addition to the enum-based policy checks (and so it could be added later)

Do you mean having both a signal and a policy, and then checking first the callback, which could return ACCEPT, REJECT or DEFAULT, and accept it, reject it, or check the policy, respectively? (and if there was no callback, check the policy too).

Sure for landing the initial patch first, and after that implement the signal.

The way I'd do it is that the signal handler returns TRUE to accept the cookie and FALSE to reject it, and returning FALSE causes the signal to stop being propagated. And then if no handler returns FALSE (or there's no handler), then the default signal implementation would check the policy and return TRUE or FALSE according to that.

So, eg, to implement "no third party except if the site is whitelisted", you'd have to set the policy to ACCEPT_ALWAYS, and implement all of the logic in the signal handler.

Created attachment 152867 [details] [review]
cookiesacceptpolicy.diff

OK, I think this addresses all review points.

Comment on attachment 152867 [details] [review]
cookiesacceptpolicy.diff

Mostly right

>+/**
>+ * SoupCookieJarAcceptPolicy:

I've mostly avoided gtk-docs in header files. Move this to soup-cookie-jar.c. Say, right before soup_cookie_jar_set_cookie_with_first_party().

Actually, we should probably have soup_cookie_jar_get/set_accept_policy(), in which case, document SoupCookieJarAcceptPolicy right before those.

>+        g_assert_not_reached ();

you missed the suggested fixes to cookie-test

>+	SoupURI *uri;
>+        SoupCookieJar *jar;

also, cookie-test.c has a mix of spaces and tabs. please fix.

>+            g_object_set (jar, "accept-policy", validResults[i].policy, NULL);

this is what made me think maybe we should have soup_cookie_jar_set_accept_policy().


Feel free to commit after fixing these things.

I committed this after further fixes as commit 9deb863e27177ad47586b01be8b4e2ad91a62cc7. I'll close this bug, a new one can be opened if anyone wishes to add some support for custom policies.