346942 - Finalize anti-phishing UI

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Jeff Walden [:Waldo]]

We need finalized anti-phishing UI to reflect exactly what options are available:

- no anti-phishing checking
- checking using a list downloaded every X minutes provided by Mozilla
- asking a dedicated provider to check every URL as it's visited

Comment 1

[Jeff Walden [:Waldo]]

I intend to get the ball rolling on this tomorrow morning.

Comment 2

[Jeff Walden [:Waldo]]

Current UI:
| [X] Check to see if the site I'm loading is a forgery           |
|     (o) Use a list of known forgeries provided by [ Google |v]  |
|     ( ) Ask [ Google |v] to check every site I visit            |

My suggested UI:
| [X] Check to see if the site I'm visiting is a forgery          |
|     (o) Check using a downloaded list of forged sites           |
|     ( ) Send each site to [ Google |v] to be checked            |

This solves several problems:

- third-party providers will only be able to provide on-demand checking
- make it clearer that sites the user visits are sent to the on-demand provider
- keep it short and sweet

This addresses the concerns which I remember needed addressing (I'm not finding my notes on this at the moment -- probably back at the apartment), except possibly for not mentioning "anti-phishing" anywhere so that the preferences are easier to find.  Thoughts from anyone else?  Beltzner?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #2)
> My suggested UI:
> | [X] Check to see if the site I'm visiting is a forgery          |
> |     (o) Check using a downloaded list of forged sites           |
> |     ( ) Send each site to [ Google |v] to be checked            |

Better! Some refinements:

| [ ] Tell me if the site I'm visiting is a suspected forgery     |
|     (o) Check using a downloaded list of suspected sites        |
|     ( ) Check by asking [ Google |v] about each site I visit    |

> are easier to find.  Thoughts from anyone else?  Beltzner?

Legal requirement to use "suspected forgery" instead of "forgery", and I was uncomfortable with "send each site" since all we're really doing is sending the address of the site. Further objections? :)

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> Legal requirement to use "suspected forgery" instead of "forgery"

Bah, that's a holdover from copying from your original comment in bug 340677, not intentional at all -- thanks for catching it, tho!  I can easily imagine not noticing it during implementation.  :-)

> I was uncomfortable with "send each site" since all we're really doing is
> sending the address of the site. Further objections? :)

Mm, parallelism...and while I think it's slightly less clear about privacy concerns, I believe it's only negligibly less clear.

Anyway, I'm on this!  Disclaimer UI (cf. bug 340740) will either be in the first patch or in an immediate followup patch.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #4)
> Anyway, I'm on this!  Disclaimer UI (cf. bug 340740) will either be in the
> first patch or in an immediate followup patch.

Will the disclaimer UI hurt less and/or provide real links to places? Or at the very least allow copy/paste out of it? Should we file a follow up for those?

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Patch (doesn't replace ugly dialog)

Here's the revised preference window UI and associated code changes.  I haven't changed the butt-ugly dialog because I haven't been able to connect with the right people to see about getting privacy policy URLs from providers.  That can be a followup patch, and the code changes to make that happen would be very isolated anyway (probably just changing _userAgreedToPhishingEULA and maybe its calling code to pass in the selected provider to retrieve the correct privacy policy).

Mark, while doing this I (think I) added a11y support for the custom provider option, based off the patch for bug 344572.  Could you check and make sure I did this correctly?

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Work in progress toward non-sucky UI

This combines the previous patch with more work at putting up something nice instead of a butt-ugly dialog with unclickable, uncopyable links; it requires that providers add a preference "browser.safebrowsing.provider.###.privacy.url" containing the URL to a privacy policy, which is then loaded similar to what happens with plugin licenses.  The OK button is disabled until the page load completes, and the user must click OK to actually enable on-load checking.

Things to resolve:
- "Blah blah" isn't informative -- need descriptive text for the dialog
  (before and after the iframe?  don't know)
- determine and use descriptive buttons (i.e., no OK/Cancel)
- handle URI load failure (404, timeout, etc.) gracefully by showing a fallback
  privacy policy (or handle this another way?  I don't know)
- handle URI absence better (e.g., either don't display such providers or do the
  fallback policy)
- get a reasonable dialog size
- clean up method docs in phishEULA.js and remove comments from original
  scavenged file
- add support for caching providers which the user has agreed to use,
  privacy-wise (browser.safebrowsing.provider.###.privacy.optedIn == true)
- run leak monitor on dialog code to be certain nothing leaks

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 232628 [details] [diff] [review]
Patch (doesn't replace ugly dialog)

looks good, works well, thanks Jeff!

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 232628 [details] [diff] [review]
Patch (doesn't replace ugly dialog)

This is necessary to not display a choice of providers for not-on-demand checking, which we never actually supported...

Comment 10

[Jeff Walden [:Waldo]]

First patch in on trunk, leaving open to deal with getting rid of the butt-ugly dialog...

Comment 11

[Jeff Walden [:Waldo]]

beltzner: help with the dialog text?  The current, completely-not-ready strings and UI can be seen at <http://whereswalden.com/files/temp/screenshot.png>.  ;-)

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232628 [details] [diff] [review]
Patch (doesn't replace ugly dialog)

a=drivers, so say we all.

Comment 13

[Jeff Walden [:Waldo]]

First patch checked in, branch and trunk.  Work continues apace on part 2 where I remove the dialog; I expect a patch in a few hours at most.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: No more text dialog!

> - "Blah blah" isn't informative -- need descriptive text for the dialog
>   (before and after the iframe?  don't know)
> - determine and use descriptive buttons (i.e., no OK/Cancel)

Beltzner!!!


> - handle URI load failure (404, timeout, etc.) gracefully by showing a
>   fallback privacy policy (or handle this another way?  I don't know)
> - handle URI absence better (e.g., either don't display such providers or do
>   the fallback policy)

This patch forces the preference to be set to a chrome URL; if it isn't set, the menulist isn't even populated with that provider as an option.  (This means we need to get such a policy in for the Google one ASAP.  Tony?)


> - get a reasonable dialog size

Probably blocking on the strings, but I don't know whether this will want a localized size or not.


> - add support for caching providers which the user has agreed to use,
>   privacy-wise (browser.safebrowsing.provider.###.privacy.optedIn == true)

Once you agree once to the privacy policy, you'll never see it again, either within the current prefwindow opening or in any subsequent one, without twiddling the pref manually.


> - run leak monitor on dialog code to be certain nothing leaks

I don't see anything leaking.

Comment 15

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> This patch forces the preference to be set to a chrome URL; if it isn't set,
> the menulist isn't even populated with that provider as an option.  (This
> means we need to get such a policy in for the Google one ASAP.  Tony?)

Tony: can you look into getting one of these (or pointing me at one if one already exists)?

Comment 16

[Tony Chang (Google)]

(In reply to comment #15)
> (In reply to comment #14)
> > This patch forces the preference to be set to a chrome URL; if it isn't set,
> > the menulist isn't even populated with that provider as an option.  (This
> > means we need to get such a policy in for the Google one ASAP.  Tony?)
> 
> Tony: can you look into getting one of these (or pointing me at one if one
> already exists)?

So we need an xhtml file with the EULA?  It should be ok to just take the previous text and put it in a file (and turn the urls into links).  It doesn't have to look fancy-- the text is fine.

I'll whip one up tomorrow if no one else gets to it before then.

Comment 17

[Tony Chang (Google)]

Attachment: v1: eula xhtml file

Eula text in xhtml+dtd file.

I tried to use a simple target="_blank" for the links, but that didn't work (a window would open, but the url wouldn't load).  I ended up using window.open instead.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233283 [details] [diff] [review]
No more text dialog!

>+<!ENTITY phishDialog.title  "Privacy Policy">

"Phishing Protection Privacy Agreement"

>+<!ENTITY phishBefore.label   "Blah blah blah before">

"Selecting this option will send the address of web pages you are viewing to %S. To continue, please review the following privacy agreement:

>+<!ENTITY phishAfter.label   "Blah blah blah after">

"Do you want to accept this privacy agreement and ask %S about each site you visit?"

>+phishAccept=OK

"Accept and Continue"

>+phishDecline=Cancel

"Cancel"

Default button should be "Cancel", %S should be the selected provider name. If we can't get that variable from here, then just use "the selected provider", I guess.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233388 [details] [diff] [review]
v1: eula xhtml file

>+<!ENTITY phish.eulatext "If you choose Google as your provider for Phishing Protection in 
>+Enhanced protection mode,

suggest: "If you choose to check with Google about each site you visit,"

This way it matches against the option they've clicked on that gets them here.

Comment 20

[Tony Chang (Google)]

Attachment: v2: eula xhtml file with small wording change

Comment 21

[Jeff Walden [:Waldo]]

Attachment: No more text dialog, v2

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 233517 [details] [diff] [review]
v2: eula xhtml file with small wording change

From my point of view, this looks fine.  I'm not entirely sure whether my review suffices for this or not, so you may want to try and get another review to be safe.

Comment 23

[Tony Chang (Google)]

(In reply to comment #22)
> (From update of attachment 233517 [details] [diff] [review] [edit])
> From my point of view, this looks fine.  I'm not entirely sure whether my
> review suffices for this or not, so you may want to try and get another review
> to be safe.

Maybe this can be merged with your CL?  Adding mconnor as a reviewer anyway.

Comment 24

[Reed Loden [:reed]]

Uh, dumb question maybe... Where's phishEULA.js?

Comment 25

[Jeff Walden [:Waldo]]

Attachment: No more text dialog, v2.1

Forgot a -N in the diff command -- d'oh!

Comment 26

[Tony Chang (Google)]

Where should I put translations of the EULA text?  Should I just attach dtd files to this bug?

Comment 27

[Mike Connor [:mconnor]]

Comment on attachment 233735 [details] [diff] [review]
No more text dialog, v2.1


>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ browser/components/preferences/phishEULA.js	15 Aug 2006 07:23:12 -0000
>@@ -0,0 +1,166 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****

this should be preprocessed out, but we can do that as a followup

>+  init: function ()
>+  {
>+    const Cc = Components.classes, Ci = Components.interfaces;
>+
>+    var providerNum = window.arguments[0].providerNum;
>+
>+    var ok = document.documentElement.getButton("accept");
>+    var cancel = document.documentElement.getButton("cancel");
>+    var phishBefore = document.getElementById("phishBefore");
>+    var phishAfter = document.getElementById("phishAfter");

ok/cancel are static, so they should be ents and set with buttonlabelaccept/cancel

>+    // the URL must load before the policy can be accepted -- it's enabled when
>+    // the policy is fully loaded
>+    ok.disabled = true;

use buttondisabledaccept="true" on the dialog element instead of doing this.

>+    var prefb = Cc["@mozilla.org/preferences-service;1"]
>+                  .getService(Ci.nsIPrefService)
>+                  .getBranch("browser.safebrowsing.provider.");

if using the Cc prefix, trailing periods and align directly with Cc

    var prefb = Cc["@mozilla.org/preferences-service;1"].
                getService(Ci.nsIPrefService).
                getBranch("browser.safebrowsing.provider.");

>+    // init before-frame and after-frame strings
>+    // note that description only wraps when the string is the element's
>+    // *content* and *not* when it's the value attribute
>+    var providerName = prefb.getCharPref(providerNum + ".name");

so, as I mentioned on IRC, this is wrong, but we'll live with it for now.  It should be a unicode-safe pref, in the future, so please file a followup.


>+  _progressListener:
>+  {
>+    onStateChange: function (aWebProgress, aRequest, aStateFlags, aStatus)
>+    {
>+      // enable the OK button when the request completes
>+      const Ci = Components.interfaces;
>+      if ((aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
>+          (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW)) {
>+        // check for load failure here, maybe?

definitely, if we fail we really want a fallback string to be displayed, and we don't want to enable the OK button.

>+          var scheme = Cc["@mozilla.org/network/io-service;1"]
>+                         .getService(Ci.nsIIOService)
>+                         .extractScheme(url);

same style nit as earlier.

r=me with all of those addressed

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Patch to be committed to trunk (incl. EULA text patch)

For posteriorioriority...

Comment 29

[Jeff Walden [:Waldo]]

(In reply to comment #27)
> if using the Cc prefix, trailing periods and align directly with Cc

For the record, I think this alignment is stupid.  :-P

> definitely, if we fail we really want a fallback string to be displayed, and
> we don't want to enable the OK button.

This is vaguely complicated, especially right now, so -->followup.

Also, the final patch included the addition of browser.safebrowsing.provider.0.privacy.url, which I didn't notice hadn't been included in posted patches.  Last-minute once-overs are awesome!

Followup bug is bug 348820, which I'll attack with gusto in the morning.

Comment 30

[Mike Connor [:mconnor]]

Jeff did in fact land this already.

Comment 31

[Jeff Walden [:Waldo]]

(In reply to comment #26)
> Where should I put translations of the EULA text?  Should I just attach dtd
> files to this bug?

Here seems as reasonable as any other place.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 233964 [details] [diff] [review]
Patch to be committed to trunk (incl. EULA text patch)

l10n impact, needs to be on branch to make v2, the existing dialog is horrible, etc.

Comment 33

[Chris Beard]

We should standardize on UI for license and other agreements that need to be legally binding. There is now a license binding available for this. (see bug 348389 comment 12)

For example, the dialog and strings for this bit of UI should more properly be:

----
Phishing Protection Terms of Service

Selecting this option will send the address of web pages you are viewing to Google. To continue, please review and accept the following terms of service.

[present terms of service with scroll bars]

( ) I do accept these Phishing Protection terms of service
(*) I do NOT accept these Phishing Protection terms of service

[Cancel] [OK]
----

Apologies for the late notice, we're running from behind on legal reviews and such. ;-/

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233964 [details] [diff] [review]
Patch to be committed to trunk (incl. EULA text patch)

Looks good, two small nits:

1. s/I do accept/I accept/

2. can you make the dialog a little longer so we don't need the scrollbar on mac? Right now you're off by about 1.5 lines ...

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Waldo: is the branch version of this all rolled up in bug 348820?

Comment 36

[Jeff Walden [:Waldo]]

(In reply to comment #35)
> Waldo: is the branch version of this all rolled up in bug 348820?

No, but it could be if you wanted it to be without too much effort.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Just wondering why the nomination is on a patch that's listed as being "for trunk". Is that the one you need to approve to then land the follow up patch? Let me know, and I'll take out my mighty a+ stick!

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233964 [details] [diff] [review]
Patch to be committed to trunk (incl. EULA text patch)

this patch is carrying r=mconnor from previous, and now gets a=beltzner on behalf of drivers for landing on the MOZILLA_1_8_BRANCH please mark fixed1.8.1 as you land.

Comment 39

[Jeff Walden [:Waldo]]

Fixed on branch.  Work to implement the revised UI of comment 33 and address followup issues is happening in bug 348820.