205541 - Selecting "Unblock Cookies..." while on a site that you'd previously blocked allows the site to set cookies rather than just removing the block

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Porter Glendinning]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030507
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030507

It seems to me that "unblock" should not equal "allow" in the Tools > Cookie
Manager menu. However, if you're on a site for which you've blocked cookies, and
you choose "Unblock Cookies from this Site" it doesn't just remove the block
entry for that site from the Cookie Manager. Instead it changes the block entry
into an allow entry.

Maybe what is needed in the menu are boolean-state items, like the ones under
the View > Show/Hide menu, which would allow you to toggle the state of both
"Block Cookies..." and "Allow Cookies..." The true states would need to be
mutually exclusive, of course, but both could be false indicating no entry for
that site on the Cookie Sites list at all.

Reproducible: Always

Steps to Reproduce:
1. Block cookies for a site.
2. Open Cookie Manager and view site's entry in the Cookie Sites list.
3. Select "Unblock Cookies..." from the Tools > Cookie Manager menu.
4. Look at the Cookie Sites entry in the Cookie Manager again.

Actual Results:  
Cookie Sites entry is set to "site can set cookies"

Expected Results:  
Cookie Sites entry should simply be removed.

Comment 1

[Sander]

Bumped this off mvl in irc for some feedback. The main problem here is when you
individually allow or block cookies (ask everytime), and then remember this
decision. When you then unblock (or block) from the site, you usually will not
want this decision remembered, but will rather want to have it changed temporarily.
When whitelisting goes in, the current behaviour will make more sense (though
you might still want some sort of toggle like the reporter proposes), but for
now this is definitely not optimal. Either we need to 
1) change the menu entry to say "unblock and allow" to at least take away the
confusion, or, 
2) add a manu entry, so we get three: "block", "unblock" and "unblock and allow"
(maybe "unblock and remember") - which will become four when whitelisting goes
in, or,
3) (better for now IMO) we need to change the behaviour of the menu entry back
to what it was, just completely removing the listing of the site, so that it
goes back to the 'neutral' state.

In any case, to remove confusion I think it would be good if one of these three
solutions could go in before 1.4 - then afterwards with cookie whitelisting we
can figure out what would be the best solution. (The more I think about it, the
better I like the pure toggle of checkmarks before "allow" and "block", although
of course they'd need to be mutually exclusive, and things might get problematic
with different domain-levels.

Comment 2

[Dan Mellem]

I think this is a regression. Mozilla worked this way up through version 1.3.
Verified with build 20030312 (1.3 final). I haven't tried the builds in between
to isolated when it changed but it may have occured at the same time the dialog
box was reworked.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

I think the suggestion in comment 0 sounds nice. It will still work when
whitelisting becomes possible.
Or, if it is too confusing, have three options: allow, block, default. (with one
checkmark)

Sander: There currently is no such thing as "block without remembering". It will
always be saved. What is possible is to remove the host from cookperm.txt, but
that will be saved too.

jag: do you have an opinion here?

Comment 4

[Mike Connor [:mconnor]]

Okay, we could do this as a radio-style submenu a la Text Size:

 º Allow Cookies From This Site
   Use Default Cookie Settings
   Block Cookies From This Site

This would both show what the current permissions are, along with allowing a
clear choice to change the behaviour.  Thoughts?

Comment 5

[Mike Connor [:mconnor]]

-> me

Comment 6

[Mike Connor [:mconnor]]

Attachment: patch

First off, this little bit of UI also applies to image permissions, so I made
the same changes to that submenu as well, to preserve the consistency.	I
decided I liked the current disabling of options (based on current permission)
well enough, I simply added the "Use Default" option to remove permissions, and
changed Unblock to Allow.

And since I was in the neighbourhood, I made the alerts into proper
promptService alerts.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 134231 [details] [diff] [review]
patch

neil, this should a) make this UI a little more intuitive and b) clean up the
dialogs a bit.

Comment 8

[Mike Connor [:mconnor]]

Attachment: patch with proper accesskeys

Comment 9

[[e-mail]]

Comment on attachment 134232 [details] [diff] [review]
patch with proper accesskeys

Bah, I wish I'd never looked at cookieNavigatorOverlay.xul - I wonder who
really owns this comment:
"for some unexplainable reason, CheckForImage() keeps getting called repeatedly
as we mouse over the task menu."
Also this should never appear in an overlay:
<menupopup id="taskPopup" onpopupshowing="CheckForVisibility()">
Explanations available on request.

Anyway, they way the menus disable looks odd... I'd really appreciate it if you
could switch it to a radiobutton style (of course you'll need to check that the
user doesn't select the current setting...)

>+        case "cookieDefault":

>+        case "imagesDefault":
None of the other case names have an s in them :-/

Comment 10

[Mike Connor [:mconnor]]

Attachment: patch v2 (radio-style)

Comment 11

[[e-mail]]

Comment on attachment 134275 [details] [diff] [review]
patch v2 (radio-style)

>+      document.getElementById("UseCookiesDefault").setAttribute("checked",blocked == nsIPermissionManager.UNKNOWN_ACTION);
Space after comma.

>+          var perm = nsIPermissionManager.ALLOW_ACTION;
>+          if (perm == permissionmanager.testPermission(uri, "cookie"))
>+            break;
>+          permissionmanager.add(uri, "cookie", perm);
I wonder why my build didn't complain about the redeclaration of var perm :-/
I'm not even convinced (but open to persuasion) that it's worth having as a
separate variable.

>+          promptService.alert(window,element.getAttribute("title"),
>+                              element.getAttribute("msg"));
Space after comma.

>         case "imageAllow":
>           permissionmanager.add(uri, "image", nsIPermissionManager.ALLOW_ACTION);
You forgot to check the existing permission here.

>         case "imageBlock":
>-          permissionmanager.add(uri, "image", nsIPermissionManager.DENY_ACTION);
>+          var perm = nsIPermissionManager.DENY_ACTION;
>+          if (perm == permissionmanager.testPermission(uri, "image"))
>+            break;
A thought: if you return; here...

>+          permissionmanager.add(uri, "image", perm);
>           element = document.getElementById("BlockImages");
>-          alert(element.getAttribute("msg"));
>+          promptService.alert(window,element.getAttribute("title"),
>+                              element.getAttribute("msg"));
>           break;
Leave this as a break...

>         default:
And return here...

>       }
Then you can alert here...

>+<!ENTITY cookieCookiesDefaultCmd.label "Use Default Cookie Permissions">
>+<!ENTITY cookieCookiesDefaultCmd.accesskey "u">
"U", otherwise the u of Default will get underlined. (Double-check the others
too.)

Comment 12

[Mike Connor [:mconnor]]

Attachment: v3, with suggested changes

the perm var doesn't really accomplish much in retrospect, its gone now.  Also,
I changed the accesskeys to all uppercase, just to always force it since that
is obviously the intended behaviour.

Comment 13

[[e-mail]]

Comment on attachment 134296 [details] [diff] [review]
v3, with suggested changes

>+      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                    .getService(Components.interfaces.nsIPromptService);
>       switch (action) {

>+          return;
>       }
Just spotted that you could have moved the promptService variable here.
>+      promptService.alert(window,element.getAttribute("title"), element.getAttribute("msg"));

Comment 14

[Mike Connor [:mconnor]]

Attachment: final patch

with the promptService var moved, no other changes, carrying over r+

Comment 15

[Alec Flett]

Comment on attachment 134312 [details] [diff] [review]
final patch

looks fine, but I like that we had a common function before (enableElement) to
do the document.getElementById() and so forth:
-      enableElement("BlockCookies", blocked !=
nsIPermissionManager.DENY_ACTION);
+      document.getElementById("UseCookiesDefault").setAttribute("checked",
blocked == nsIPermissionManager.UNKNOWN_ACTION);

sr=alecf but extra points if you merge that stuff into a new common function.

(Also, the wording for the by-default labels is a little awkward.. not that I
can think of anything better!)

Comment 16

[Mike Connor [:mconnor]]

Attachment: ooh! ooh! extra points!

Comment 17

[[e-mail]]

Comment on attachment 134364 [details] [diff] [review]
ooh! ooh! extra points!

so i'm ok with this as it stands, but we do need to be aware of this problem:

>+        case "imageDefault":
>+          if (permissionmanager.testPermission(uri, "image") == nsIPermissionManager.UNKNOWN_ACTION)
>+            return;
>+          permissionmanager.remove(uri.hostPort, "image");

using uri.hostPort directly here will work in most cases, but not all.
nsIPermissionManager::Add() and ::TestPermission() take the uri directly, and
do some massaging to it in some cases. Remove() will be expecting that massaged
string. unfortunately, it's hard to change Remove() to take the uri directly,
because it's handy to e.g. get an enumerator from the permissionmanager, find
an nsIPermission you want to remove, and then call Remove() using the relevant
host string obtained from said nsIPermission.

i'm not sure how we could solve it. maybe we could just provide a conversion
helper on nsIPermissionManager to convert a URI to a key. then we could enforce
the distinction between the URI and the key a little better perhaps?

mvl?

Comment 18

[Mike Connor [:mconnor]]

checked in

file:/// urls can't be removed this way, but I think that's a safe enough case
for now, but I'll leave this open until mvl responds or we spin that off into a
new bug

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

I'd suggest to file a new bug.
The UI is ok, it's the backend that needs some work. The problem is that you
can't find out what to remove. For example, if you block domain.com from setting
cookies, TestPermissions will return blocked for sub.domain.com. But calling
Remove with sub.domain.com won't do anything. We need to decide what should happen.

Comment 20

[Mike Connor [:mconnor]]

Bug 224327 filed.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 227294 has been marked as a duplicate of this bug. ***

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 230559 has been marked as a duplicate of this bug. ***

Comment 23

[[e-mail]]

>>+          promptService.alert(window,element.getAttribute("title"),
>>+                              element.getAttribute("msg"));
>Space after comma.
Bah, this never got fixed, did it :-(

Comment 24

[benc]

V: 
Mozilla 1.6f, Mac OS X