Last Comment Bug 770705 - Update about:permissions to allow editing per site 3rd party cookie prefs
: Update about:permissions to allow editing per site 3rd party cookie prefs
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: Firefox 18
Assigned To: [:mmc] Monica Chew (no longer reading bugmail)
:
Mentors:
Depends on: 796113 770691
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 15:28 PDT by David Dahl :ddahl
Modified: 2012-11-21 08:40 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add "Allow first party only" to about:permissions. (4.59 KB, patch)
2012-09-19 15:09 PDT, [:mmc] Monica Chew (no longer reading bugmail)
margaret.leibovic: feedback+
Details | Diff | Splinter Review
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref (5.53 KB, patch)
2012-09-24 14:56 PDT, [:mmc] Monica Chew (no longer reading bugmail)
no flags Details | Diff | Splinter Review
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref (6.93 KB, patch)
2012-09-24 15:04 PDT, [:mmc] Monica Chew (no longer reading bugmail)
margaret.leibovic: review+
Details | Diff | Splinter Review
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref (6.87 KB, patch)
2012-09-24 16:22 PDT, [:mmc] Monica Chew (no longer reading bugmail)
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2012-07-03 15:28:43 PDT
In bug 770691, we are adding more plumbing to CookiePermission and CookieService that can block or allow blocking or allowing per-site 3rd party cookie operations. We need to surface this in the about:permissions UI
Comment 2 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-19 15:09:23 PDT
Created attachment 662707 [details] [diff] [review]
Add "Allow first party only" to about:permissions.

Try in progress, mochitest passes: https://tbpl.mozilla.org/?tree=Try&rev=a037709c5a71
Comment 3 Mardeg 2012-09-19 15:21:03 PDT
Is there a bug open to also add "Allow first party only" to View Page Info, "Permissions" section?
Comment 4 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-19 15:41:42 PDT
Hi Mardeg,

Not that I know of, though that's a good idea.

Thanks,
Monica
Comment 5 Dão Gottwald [:dao] 2012-09-19 15:50:21 PDT
Note that Page Info > Permissions is the primary UI for this, for the time being. about:permissions is still a hidden feature.
Comment 6 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-20 11:52:40 PDT
OK -- in the meantime, I'd like to surface this in about:permissions first. Dao, would you mind giving a review?
Comment 7 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 11:50:45 PDT
Comment on attachment 662707 [details] [diff] [review]
Add "Allow first party only" to about:permissions.

Gavin suggested Margaret would be a good reviewer.
Comment 8 :Margaret Leibovic 2012-09-24 13:25:11 PDT
Comment on attachment 662707 [details] [diff] [review]
Add "Allow first party only" to about:permissions.

Review of attachment 662707 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good, but I think you need to watch out for the "All Sites" case.

::: browser/components/preferences/aboutPermissions.xul
@@ +126,5 @@
>                        oncommand="AboutPermissions.onPermissionCommand(event);">
>                <menupopup>
>                  <menuitem id="cookie-1" value="1" label="&permission.allow;"/>
>                  <menuitem id="cookie-8" value="8" label="&permission.allowForSession;"/>
> +                <menuitem id="cookie-9" value="9" label="&permission.allowFirstPartyOnly;"/>

Do you want this option to be visible for "All Sites" to allow users to set a global preference? If so, you need to update the PermissionDefaults cookie code:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.js#269

If there is no global preference, you can add some code in updatePermission to hide it in the |!this._selectedSite| case, similarly to how we do for the _noGlobalAllow/_noGlobalDeny permission types:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.js#749

::: browser/components/preferences/tests/browser_permissions.js
@@ +16,5 @@
>  const PERM_UNKNOWN = 0;
>  const PERM_ALLOW = 1;
>  const PERM_DENY = 2;
> +// cookie specific permissions
> +const PERM_FIRST_PARTY_ONLY = 9;

At first I thought you accidentally deleted PERM_SESION, but I see it's not used, so that's fine. If you're going to get rid of it, you should also get rid of it in browser_chunk_permissions.js.
Comment 9 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 14:55:51 PDT
Hi Margaret, thanks for the review. I asked on #security and the consensus was that there should not be a global setting for ALLOW_FIRST_PARTY_ONLY. This is equivalent to disallowing 3rd party cookies, which is already settable in Preferences > Privacy > Use custom settings for history, and adding another way to set the same preference, with different wording, would be confusing.

I added code to hide ALLOW_FIRST_PARTY_ONLY when no site selected, although it's pretty ugly. Also deleted PERM_SESION. Please have another look.

Thanks,
Monica
Comment 10 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 14:56:39 PDT
Created attachment 664231 [details] [diff] [review]
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref
Comment 11 :Margaret Leibovic 2012-09-24 14:59:23 PDT
(In reply to Monica Chew from comment #9)

> I added code to hide ALLOW_FIRST_PARTY_ONLY when no site selected, although
> it's pretty ugly. Also deleted PERM_SESION. Please have another look.

I don't see this in the new patch. Maybe the new changes didn't make it in?
Comment 12 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 15:04:04 PDT
My bad, forgot to qref before exporting. Fixing.
Comment 13 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 15:04:50 PDT
Created attachment 664238 [details] [diff] [review]
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref
Comment 14 :Margaret Leibovic 2012-09-24 15:51:38 PDT
Comment on attachment 664238 [details] [diff] [review]
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref

Review of attachment 664238 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with my comment addressed.

::: browser/components/preferences/aboutPermissions.js
@@ +769,5 @@
>        if (aType == "plugins") {
>          document.getElementById("plugins-pref-item").hidden =
>            !Services.prefs.getBoolPref("plugins.click_to_play");
>          return;
> +      } else if (aType == "cookie")

You don't need an |else| here, since the if statement above returns. You can just put this if statement on its own on the line below.

::: browser/components/preferences/tests/browser_permissions.js
@@ +257,5 @@
> +
> +    // change a site-specific cookie permission, just for fun
> +    let cookieMenuList = getPermissionMenulist("cookie");
> +    let cookieItem = gBrowser.contentDocument.getElementById("cookie-" +
> +                                                             PERM_FIRST_PARTY_ONLY);

Nit: Just put this on the same line above. It's ok if the line is a little long :)
Comment 15 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 16:22:51 PDT
Created attachment 664263 [details] [diff] [review]
Changes to about:permissions to expose ALLOW_FIRST_PARTY_ONLY as a site specific cookie pref

Thanks, Margaret! Comments fixed.
Comment 16 [:mmc] Monica Chew (no longer reading bugmail) 2012-09-24 16:26:50 PDT
Mochitests passed, try in progress: https://tbpl.mozilla.org/?tree=Try&rev=a55f464f7b36
(previous revision passed here: https://tbpl.mozilla.org/?tree=Try&rev=a037709c5a71)

r+ by margaret in comment 14
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-09-24 18:53:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1984535f059
Comment 18 Mardeg 2012-09-24 20:24:55 PDT
Filed bug 793948 per comment 4
Comment 19 Ed Morley [:emorley] 2012-09-25 06:18:32 PDT
https://hg.mozilla.org/mozilla-central/rev/c1984535f059
Comment 20 Ioana (away) 2012-11-21 08:40:56 PST
Verified as fixed on Firefox 18 (20121119042013).

Note You need to log in before you can comment on or make changes to this bug.