Last Comment Bug 521467 - Automatically log in to proxy
: Automatically log in to proxy
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: P2 enhancement with 2 votes (vote)
: mozilla1.9.3a5
Assigned To: Ben Bucksch (:BenB)
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 910670
  Show dependency treegraph
 
Reported: 2009-10-09 12:35 PDT by Ben Bucksch (:BenB)
Modified: 2014-02-14 15:39 PST (History)
13 users (show)
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix, v1. Not considering login failure. (2.95 KB, patch)
2009-10-09 12:41 PDT, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v2 (8.67 KB, patch)
2009-10-09 14:28 PDT, Ben Bucksch (:BenB)
cbiesinger: review-
Details | Diff | Splinter Review
Fix, v3 (9.01 KB, patch)
2010-02-05 14:08 PST, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v4 (10.99 KB, patch)
2010-02-05 16:05 PST, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v5 (27.24 KB, patch)
2010-02-05 22:32 PST, Ben Bucksch (:BenB)
cbiesinger: review+
Details | Diff | Splinter Review
Fix, v6 (27.60 KB, patch)
2010-02-08 06:42 PST, Ben Bucksch (:BenB)
cbiesinger: review+
Details | Diff | Splinter Review
Fix, v7 - netwerk part checked in (comment 33) (27.46 KB, patch)
2010-02-08 07:08 PST, Ben Bucksch (:BenB)
cbiesinger: review+
dolske: review-
ehsan: review-
bzbarsky: superreview+
Details | Diff | Splinter Review
Fix, v8 - firefox part only (3.36 KB, patch)
2010-03-16 08:08 PDT, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v9 - Firefox part only (30.22 KB, patch)
2010-04-05 20:53 PDT, Justin Dolske [:Dolske]
paul: review-
Details | Diff | Splinter Review
Fix, v10 - Firefox part only (29.95 KB, patch)
2010-04-06 15:28 PDT, Justin Dolske [:Dolske]
paul: review+
Details | Diff | Splinter Review

Description Ben Bucksch (:BenB) 2009-10-09 12:35:21 PDT
Situation:
- HTTP proxy (e.g. Squid) which requires a username/password.
  Needed to get on the Internet.

Reproduction:
- Configure Firefox to use that proxy.
- Go to an Internet website.
  You get a Login dialog (from FF) for the proxy.
- Enter your credentials, check "[x] Save", press OK.
- Close browser
- Open browser, and go to Internet website
  (or use Internet Website as start page)

Actual result:
- You get the login dialog again, with your credentials prefilled,
  so you just have to press OK.

This is annoying, because it happens every time you start the browser, and it provides no security, because you just have to click OK.

Expected result:
- No prompt. The stored credentials are used automatically without dialog.
- If login fails with the stored credentials, the dialog comes up again.

Severity:
- The German government pays me to fix this, because it annoys them.
  5000 users in this department alone.
- Common case: Authanticated proxies are common in large companies.
- Frequency: Whenever the user starts the browser, so frequent annoyance.

Implementation:
Patch attached.

I added a hidden pref for the odd cases where the old behavior is wanted.
Comment 1 Ben Bucksch (:BenB) 2009-10-09 12:41:07 PDT
Created attachment 405551 [details] [diff] [review]
Fix, v1. Not considering login failure.

This automatically logs in.

However, it does not bring up the login prompt again when the login failed. This results in infinitely re-loading the page and never getting it. I need to find out where to catch the login failure and pass it to the password manager's PromptAuth or whatever calls that.
Comment 2 Justin Dolske [:Dolske] 2009-10-09 13:54:05 PDT

*** This bug has been marked as a duplicate of bug 223636 ***
Comment 3 Ben Bucksch (:BenB) 2009-10-09 14:28:22 PDT
Created attachment 405568 [details] [diff] [review]
Fix, v2

I had to change netwerk to know whether a previous login attempt (with the stored credentials) failed, and pass it to the login manager via a new flag.

If nsIAuthInformation is frozen, and we can't even add a const, I'll just hardcode the 16 :).
Comment 4 Ben Bucksch (:BenB) 2009-10-09 14:31:11 PDT
Thanks, Justin, for pointing out the other bug with another patch.

I'll REOPEN this, to get a review for this patch.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-10-09 14:35:00 PDT
Adding the const doesn't affect compatibility, so there's no reason not to do it, AFAIK.
Comment 6 Ben Bucksch (:BenB) 2009-10-19 13:07:54 PDT
FWIW, I think the overlap with the other bug is small, because the UI for proxy (at every browser start, and always the same server) from the UI for HTTP servers on the net, so I think this patch still holds as-is (and will help a bit with the other bug).
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2009-12-01 07:56:31 PST
Comment on attachment 405568 [details] [diff] [review]
Fix, v2

+   * We have already tried to log in during this connection,
+   * (with auth values either from a previous user prompt
+   * or from the password manager),
+   * but it failed, so we now ask the user to provide a new,
+   * correct login.

Can you fix the line wrapping here?

Also, "connection" is a bit inexact, since the retry might actually happen on a new TCP connection (if the server sent connection: close). Maybe s/during this connection/for this channel/?

I think it's also clearer to change the parenthetical comment to (with auth values from a previous promptAuth call).

r=biesi on the netwerk/ part.
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2009-12-01 07:59:24 PST
Actually I think you need to split up mTriedAuth into two variables, one for the proxy and one for the server itself, otherwise you could incorrectly set PREVIOUS_FAILED.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2009-12-01 07:59:38 PST
A unit test would also be nice :)
Comment 10 Ben Bucksch (:BenB) 2009-12-01 08:04:15 PST
biesi: Thanks for the catch! Will fix that.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2009-12-01 08:06:48 PST
I do like the idea of the patch though. Prompting for proxy passwords that we already have seems unnecessary. Maybe it would be worth figuring out why we didn't do it this way originally...
Comment 12 Ben Bucksch (:BenB) 2009-12-01 08:10:04 PST
Dolske, I realize you have a different patch in bug 223636, with infobar or the link for both HTTP host and proxy. I like the idea, for HTTP hosts. I think HTTP proxies are a very different usecase, though. See initial description, please. 

Can you please review the passwordmgr part? It's fairly small and straight-forward, too.
Comment 13 mark.jo 2010-01-01 05:22:21 PST
I use AutoAuth 1.3 and FF 3.5.1, but it doesn't work. 
How can I install the patch "Fix, v2" ?
I hope someone can help me. Thanks!
Comment 14 Ben Bucksch (:BenB) 2010-01-01 06:59:01 PST
No, you can't. This is for developers only. Please wait until this is integrated in Firefox.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-02-03 08:14:19 PST
Ben, I'm not sure what you need my review for here.  Christian reviewed the necko parts already and I'm not a peer for the other parts.
Comment 16 Ben Bucksch (:BenB) 2010-02-03 11:43:12 PST
Comment on attachment 405568 [details] [diff] [review]
Fix, v2

bz, I picked you because you reviewed one of the more recent changes to the same file, namely bug 475053, per hg log. On closer inspection, you reviewed the netwerk part, so sorry to bother you.

Trying gavin.
Comment 17 Ben Bucksch (:BenB) 2010-02-03 11:47:24 PST
gavin, you changed reviewer to dolske. I was waiting for 2 months for dolske to review this (comment 12), and therefore now changed the review request. Who else can review it?
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-02-03 15:02:38 PST
Dolske and I (and any other toolkit peer, technically), but dolske is probably best suited. Have you tried emailing him or pinging on IRC?
Comment 19 Christian :Biesinger (don't email me, ping me on IRC) 2010-02-04 04:16:44 PST
Comment on attachment 405568 [details] [diff] [review]
Fix, v2

Actually I think I want to see the final version of the patch before this gets checked in, so making this a minus for now
Comment 20 Ben Bucksch (:BenB) 2010-02-04 06:03:07 PST
I wanted to wait for the other review before I touch and test the code again, but I'll adapt the patch tomorrow.
Comment 21 Ben Bucksch (:BenB) 2010-02-05 14:08:38 PST
Created attachment 425548 [details] [diff] [review]
Fix, v3
Comment 22 Ben Bucksch (:BenB) 2010-02-05 16:05:30 PST
Created attachment 425577 [details] [diff] [review]
Fix, v4
Comment 23 Ben Bucksch (:BenB) 2010-02-05 22:32:22 PST
Created attachment 425621 [details] [diff] [review]
Fix, v5

With unit test
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2010-02-08 02:46:06 PST
Comment on attachment 425621 [details] [diff] [review]
Fix, v5

+++ b/netwerk/base/public/nsIAuthInformation.idl
+   * @see also RFC 2616, Section 10.4.2
+   * "If ... the user agent has already attempted authentication at least once,
+   * then the user SHOULD be presented ... the response"

I don't think this quote is relevant. It means that the browser should display the page that the server sent. But we only do that once the user cancels the prompt.

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+    }
     else
+    {

The style in this file is:
  }
  else {

+++ b/netwerk/test/unit/test_auth_proxy.js
I think you're supposed to put testcases in the public domain these days, per:
http://www.mozilla.org/MPL/license-policy.html
and
https://bugzilla.mozilla.org/show_bug.cgi?id=512247#c11

(I wrote the initial version of test_authentication.js, you have my permission to put it in the public domain)

+    //dump("promptAuth\n");

please don't check in commented out code without explanation

+    try {
+
+    // never HOST and PROXY set at the same time in prompt

body of a try block should be indented

+    do_check_eq((authInfo.flags & Ci.nsIAuthInformation.AUTH_HOST) > 0,
+            (authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) == 0);
+    var isProxy = (authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) > 0;

I think this would be easier to read if you used != instead of >

+    dump("with: " + ((cred.flags & FLAG_WRONG_PASSWORD) > 0 ? "wrong password" : "")  + " " + ((cred.flags & FLAG_PREVIOUS_FAILED) > 0 ? "previous failed" : "")  + " " + ((cred.flags & FLAG_RETURN_FALSE) > 0 ? "return false" : "") + "\n");

80 characters please

+      return new Cancelable(function() {
+          callback.onAuthAvailable(context, authInfo);
+        });

that doesn't seem quite corrrect, you'd call onAuthAvailable twice if you get cancelled (once from runLater, once from the cancelable).

+  QueryInterface: function authprompt2_qi(iid) {

authprompt2 -> cancelable

+    //url = "http://somesite/auth"; // TODO returns HTTP 400, despite proxy prefs

You need httpserv.identity.add("http", "somesite", 80);
See http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/README

(If it was the proxy prefs that didn't work, you wouldn't get a 400, you'd get a failed channel (request.status would be a failure code))

r=biesi on the netwerk part
Comment 25 Ben Bucksch (:BenB) 2010-02-08 03:02:36 PST
> I don't think this quote is relevant.

It's relevant insofar as this patch allows to meet the RFC requirement.

> I think you're supposed to put testcases in the public domain these days, per:
> http://www.mozilla.org/MPL/license-policy.html

I wasn't aware of that. The MPL is still acceptable for testcases, though. So, the patch should be fine as-is.

Note that the concept of public domain doesn't exist under German law (meaning German authors can't put things under public domain, at least not under public domain in the US law sense). CC0 may still be a valid license in Germany, I don't know. I am fine with liberal use of the code, but I do not feel comfortable of waiving the right to attribution (i.e. IMHO, the copyright / authorship notice must stay), in general.

> commented out code

will remove

> You need httpserv.identity.add("http", "somesite", 80);
> http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/README

meh. Will do

> +      return new Cancelable(function() {
> +          callback.onAuthAvailable(context, authInfo);
> +        });
> 
> that doesn't seem quite corrrect, you'd call onAuthAvailable twice if you get
> cancelled (once from runLater, once from the cancelable).

That's non-trivial (I'd probably use a 'closure' variable). It doesn't seem used anyways. Shall I just return null as nsICancelable?
Comment 26 Ben Bucksch (:BenB) 2010-02-08 06:34:41 PST
> You need httpserv.identity.add("http", "somesite", 80);

Thanks for that, it worked nicely.

> I wrote the initial version of test_authentication.js

Nice.

I fixed the cancelable to not call twice, although I think it's overkill.
I left the RFC comment and the license, per reasons above.
Fixed the other nits.
Comment 27 Ben Bucksch (:BenB) 2010-02-08 06:42:40 PST
Created attachment 425793 [details] [diff] [review]
Fix, v6
Comment 28 Christian :Biesinger (don't email me, ping me on IRC) 2010-02-08 07:03:41 PST
Comment on attachment 425793 [details] [diff] [review]
Fix, v6

r=biesi, except:

> It's relevant insofar as this patch allows to meet the RFC requirement.

Well this patch still doesn't show the user the response. And with or without the patch, the user can see the server response by canceling the prompt.

So I'm not sure what you're implying with this comment. A strict reading of that RFC section seems to say that we should show the response in the background while we're displaying the prompt to the user, but we don't do that and this patch doesn't help with it.
Comment 29 Ben Bucksch (:BenB) 2010-02-08 07:08:00 PST
Created attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)

Removed the RFC-quote, leaving only the reference
Comment 30 Justin Dolske [:Dolske] 2010-02-10 04:26:59 PST
I think I'm generally ok with this, though the part I haven't convinced myself of is if the nsILoginMetaInfo flag approach (bug 223636) is the right way to do this. Specifically, it would be less than ideal to split the "do this automatically" control into different places (login info for http auth, pref for proxy auth), and I'd want to avoid any future ambiguity of the pref saying one thing but the login saying the other.

But if we do decide to go with a login flag like 223636, it should be simple enough to just migrate the pref's setting for all existing moz-proxy:// logins, thus avoiding the problems above.
Comment 31 Ben Bucksch (:BenB) 2010-02-10 05:06:05 PST
Oh, you mean whether to use a pref("signon.autologin.httpproxy", true); or saving that "login automatically" in the login manager? Yes... With the pref, we can set the default for proxies to true and for HTTP hosts to false.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2010-02-11 19:16:07 PST
Comment on attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)

API changes look fine.
Comment 33 Ben Bucksch (:BenB) 2010-02-12 09:48:48 PST
Commited netwerk/ part
<http://hg.mozilla.org/mozilla-central/rev/ba358a050536>
Comment 34 Ben Bucksch (:BenB) 2010-02-27 13:58:27 PST
dolske, what do we do here? See comment 31, I think the pref is useful even with the nsILoginMetaInfo flag. You can always add the latter later for specific overrides, and the pref is for the default for proxies.
Comment 35 Ben Bucksch (:BenB) 2010-03-10 05:53:38 PST
dolske: ping
Comment 36 :Ehsan Akhgari 2010-03-11 15:12:30 PST
Comment on attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)

Let's disable auto-authenticating when the user is inside the PB mode, and file a follow-up bug to make that happen only if we're using a PAC configuration.

To clarify, if we're dealing with a global proxy, all requests will have to go through it, and disabling auto-auth inside the PB mode would just hurt the user experience.  However, if we're using a PAC, it might be the case that the proxy is only being used for certain domains, which would mean that we'll let the proxy know it's us when going to those domains, which is against the intention of the PB mode.

Ben, please ask for my review on a version of this patch which implements the idea in the first paragraph and includes tests for it.  Thanks!
Comment 37 Justin Dolske [:Dolske] 2010-03-11 15:21:12 PST
Comment on attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)


>--- a/browser/app/profile/firefox.js
...
>+// Login in to HTTP proxy without showing auth prompt,
>+// if the password manager has the password
>+pref("signon.autologin.httpproxy", true);

This should go in /modules/libpref/src/init/all.js, with the other signon.* prefs.

Probably better to call it signon.autologin.proxy... I believe this auth path is protocol agnostic (certainly https goes through here, I believe FTP proxy auth does too? And I'm just going to pretend that I didn't see we _still_ offer the option of a Gopher proxy...). Looks like we don't support SOCKS auth yet, so no problem there.


>--- a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+            var hadFailure = false;
>+            if (aAuthInfo.flags & Ci.nsIAuthInformation.PREVIOUS_FAILED) {
>+                hadFailure = true;
>+                this.log("previous login failed, so not using stored login");
>+                // Need to fill |foundLogins| below anyways, so that
>+                // later code knows to update the existing entry.
>+            }

I don't think this block is needed; hadFailure is only checked once below, at the same time as another aAuthInfo.flag... Just check both flags in the same conditional.


>+                // Allow automatic login
>+                /* This is a login to an HTTP proxy. It appears essentially
>+                   whenever you start the browser, and we prefill auth,
>+                   so it's rather annoying (and pointless) to prompt. */

Second comment not needed.

>+                if (aAuthInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY &&

Have this block just set a canAutologin flag, and make the very last clause in the main try{} block return when this flag is true. This will help for when bug 465636 lands, because I think I'll have that patch change things here so canAutologin just skips the prompt call, and then run through the 2nd half of promptAuth so the timestamp/usage accounting is correct.

>+                    try {
>+                        var prefs = Cc["@mozilla.org/preferences-service;1"].
>+                            getService(Ci.nsIPrefBranch);
>+                        auto = prefs.getBoolPref("signon.autologin.httpproxy");
>+                    } catch (e) { alert(e); }

No need for a try/catch here (and certainly not alert! :), since this pref should always be present (after moving to all.js).

Should probably just use the new Services.jsm (bug 512784) here. Actually, I'll just whip up a quick patch to make Login Manager start using that.

Also...

1) After a chat with Ehsan, we decided that automatic proxy login should be disabled when in private browsing mode. There may be cases where we should allow it anyway (eg, no autologin if using a PAC file, allow if you're using a single global proxy), but we can figure that out in a followup bug.

2) Need some UI tests here. Should be easy to add, see toolkit/components/passwordmgr/test/test_prompt.html for a bunch of existing tests. Mainly want to check:
  - no autologin unless it's a proxy
  - autologin works for a proxy login
  - prompt shown when autologin enabled but auth failed.
  - no autologin when in PB mode.
Comment 38 Ben Bucksch (:BenB) 2010-03-11 15:29:09 PST
Ehsan, I don't understand what you're saying. First, what is "PB mode"? And which specific change do you want from me?

I should add a check in the login manager code which checks the proxy prefs and does the auto-login only if the user is *not* using PAC proxy config files?

Personally, I think auto-login is still useful for PAC proxy config. Typically, the PAC file would exclude internal hosts and send to the proxy for all others. Even if it's the other way, the user likely will need the proxy during a typical browsing session (otherwise the PAC file would be a waste of resources).

Or, to look at it from another angle: If you stored the password, the prompt gives no additional protection. The prompt makes sense for third party HTTP hosts, because you want to decide whether you identify to the site or not. Proxies, however, are always trusted and close to the user, esp. with PAC files. There's no need to decide whether to identify yourself.

So, I think it doesn't matter whether the user uses manual config or PAC. In fact, I think PAC users would see it as a major setback that they cannot take advantage of the auto login that this implements.
Comment 39 Ben Bucksch (:BenB) 2010-03-11 15:35:41 PST
My comment overlapped with dolske's. I see from his comment that you mean "PB" = "Private Browsing".
That makes somewhat sense, I have no objection to that.

dolske, your review comments make sense, I'll adjust the code.
(I don't understand the "Have this block just set a canAutologin flag" paragraph yet, but I guess I will when I look at the code.)

Thanks!
Comment 40 Justin Dolske [:Dolske] 2010-03-11 16:58:23 PST
(In reply to comment #37)

> Should probably just use the new Services.jsm (bug 512784) here. Actually, I'll
> just whip up a quick patch to make Login Manager start using that.

Just did this in bug 551851.
Comment 41 :Ehsan Akhgari 2010-03-11 17:48:06 PST
(In reply to comment #38)
> Ehsan, I don't understand what you're saying.

Sorry for the confusion.  I assumed that everyone knows the jargon related to my own module, which is obviously not true.  :-)

If there is any more confusion, please let me know.
Comment 42 Ben Bucksch (:BenB) 2010-03-16 08:08:58 PDT
Created attachment 432817 [details] [diff] [review]
Fix, v8 - firefox part only

Changes:
- moved first block down
- removed comment
- changed to |canAutologin|
- removed try/catch around prefs reading and used Services
- don't auto-login when in private browsing mode

I did not understand:
> make the very last clause in the main try{} block return when this
> flag [canAutologin] is true

I could move this further down, but that would make it run through all the notification bar stuff necessarily. If you think that's a good idea nevertheless, you can still change that, e.g. in bug 465636, trivially.

> 2) Need some UI tests here.

I'm having a problem here: Spent an hour trying to grasp and strip down test_prompt.js and test_prompt_async.js, so far unsuccessfully.
Comment 43 Ben Bucksch (:BenB) 2010-03-16 08:11:09 PDT
Ops, please pretend that "this.log(" line there doesn't exist.
FWIW, I tested this against an authenticated proxy, and it works fine, with both pref settings, and when the server password changes unilaterally, and when the user enters a wrong password.
Comment 44 Christian :Biesinger (don't email me, ping me on IRC) 2010-03-19 06:43:42 PDT
(In reply to comment #37)
> Probably better to call it signon.autologin.proxy... I believe this auth path
> is protocol agnostic (certainly https goes through here, I believe FTP proxy
> auth does too? And I'm just going to pretend that I didn't see we _still_ offer
> the option of a Gopher proxy...). Looks like we don't support SOCKS auth yet,
> so no problem there.

I agree that the pref should be renamed, but fwiw, the "gopher" and "ftp" proxies are really just an HTTP proxy, from the point of view of firefox.
Comment 45 Justin Dolske [:Dolske] 2010-04-05 20:53:55 PDT
Created attachment 437211 [details] [diff] [review]
Fix, v9 - Firefox part only

Updated to trunk, since my landing of bug 465636 means this needed to update timestamps even when not prompting. Also added tests.

Also, I flipped the default to *false* for the new pref, such that autologin doesn't happen unless the user changes this pref... I got a little wary about this needing some more security review before setting the default to true. But there's no reason for that to hold up landing this preffed off.
Comment 46 Ben Bucksch (:BenB) 2010-04-06 03:52:04 PDT
> Also added tests.

Thank you! I was really stranded there.
Comment 47 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-04-06 13:41:16 PDT
Comment on attachment 437211 [details] [diff] [review]
Fix, v9 - Firefox part only

>     promptAuth : function (aChannel, aLevel, aAuthInfo) {
>         var selectedLogin = null;
>         var checkbox = { value : false };
>         var checkboxLabel = null;
>         var epicfail = false;
>+        var canAutologin = false;

We should s/var/let/ in new code, but it can slide here for consistency.

>-        var ok = this._promptService.promptAuth(this._window, aChannel,
>-                                aLevel, aAuthInfo, checkboxLabel, checkbox);
>+        var ok;
>+        if (canAutologin)
>+            ok = true;
>+        else
>+            ok = this._promptService.promptAuth(this._window,
>+                                                aChannel, aLevel, aAuthInfo,
>+                                                checkboxLabel, checkbox);

It seems a bit silly to say if(true) ok = true, so why not something like this:
> var ok = canAutoLogin ||
>          this._promptService.promptAuth(this._window, aChannel, aLevel,
>                                         aAutoInfo, checkboxLabel, checkbox);

>-var pwmgr, tmplogin, login1, login2A, login2B, login2C, login2D, login2E, login3A, login3B, login4;
>+var pwmgr, tmplogin, login1, login2A, login2B, login2C, login2D, login2E, login3A, login3B, login4, proxyLogin;

didDialog was never defined, can you add it here.

>+  proxyLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+               createInstance(Ci.nsILoginInfo);
> 
>   login1.init("http://example.com", null, "http://example.com",
>               "", "examplepass", "", "");
>   login2A.init("http://example2.com", null, "http://example2.com",
>                "user1name", "user1pass", "", "");
>   login2B.init("http://example2.com", null, "http://example2.com",
>                "user2name", "user2pass", "", "");
>   login2C.init("http://example2.com", null, "http://example2.com",

Please init proxyLogin here - the method is called initLogins :). Or at the very least, add a comment that proxyLogin is inited in test 121.

> // ===== test 1 ===== 
> var testNum = 1;
> startCallbackTimer();
> isOk = prompter1.prompt(dialogTitle(), dialogText, "http://example.com",
>                         Ci.nsIAuthPrompt.SAVE_PASSWORD_NEVER, "abc", result);
> 
> ok(isOk, "Checking dialog return value (accept)");
>+ok(didDialog, "handleDialog was invoked");
> is(result.value, "xyz", "Checking prompt() returned value");

All (or most) of the true cases are actually depending on the state of the previous test. If didDialog is not set to false before running the next test, it will be true for the next test regardless of whether or not a dialog was actually shown. So it really should be set to false at the beginning of each test.

Actually the whole didDialog check is moot. handleDialog will never be called if there isn't a dialog (because startCallbackTimer isn't called), so it could never become true in those cases. So we can really only test that a dialog was shown, not that one wasn't. If a dialog was shown when we weren't expecting it we'd get a timeout, so that should be good enough.

>+  pb.privateBrowsingEnabled = false;
>+  prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
>+}
>+
>+
>+prefs.clearUserPref("signon.autologin.proxy");

clearUserPref can throw if it doesn't have a user value, so check first with
> if (prefs.prefHasUserValue(...))
Comment 48 Justin Dolske [:Dolske] 2010-04-06 15:14:55 PDT
(In reply to comment #47)

> didDialog was never defined, can you add it here.

It's defined in prompt_common.js.

> > startCallbackTimer();
> > isOk = prompter1.prompt(dialogTitle(), dialogText, "http://example.com",
> >                         Ci.nsIAuthPrompt.SAVE_PASSWORD_NEVER, "abc", result);
> > 
> > ok(isOk, "Checking dialog return value (accept)");
> >+ok(didDialog, "handleDialog was invoked");
> 
> All (or most) of the true cases are actually depending on the state of the
> previous test.

Actually, no. startCallbackTimer() resets it (which is why I'm manually resetting it for the cases where there's no dialog expected, and thus startCallbackTimer() isn't called).

> Actually the whole didDialog check is moot. handleDialog will never be called
> if there isn't a dialog (because startCallbackTimer isn't called), so it could
> never become true in those cases. So we can really only test that a dialog was
> shown, not that one wasn't. If a dialog was shown when we weren't expecting it
> we'd get a timeout, so that should be good enough.

Ah, poop, you're right.

> clearUserPref can throw if it doesn't have a user value, so check first with
> > if (prefs.prefHasUserValue(...))

It can't throw, because the test before this is testing the default behavior (which is to prompt). When the default changes, we'll flip around the tests as well as the value of the pref that's set.
Comment 49 Justin Dolske [:Dolske] 2010-04-06 15:28:51 PDT
Created attachment 437417 [details] [diff] [review]
Fix, v10 - Firefox part only
Comment 50 Justin Dolske [:Dolske] 2010-04-08 01:20:46 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/8c6006adbb00
Comment 51 Ben Bucksch (:BenB) 2010-04-10 05:37:24 PDT
Thanks!
Comment 52 Arie Paap [:wildmyron] 2010-04-19 19:48:20 PDT
Verified with Minefield:

No prompt for proxy password after startup.
Change proxy password -> Minefield prompts for new password.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100419 Minefield/3.7a1pre ID:20100419035943
Comment 53 Dão Gottwald [:dao] 2011-03-30 06:45:07 PDT
(In reply to comment #45)
> Also, I flipped the default to *false* for the new pref, such that autologin
> doesn't happen unless the user changes this pref... I got a little wary about
> this needing some more security review before setting the default to true. But
> there's no reason for that to hold up landing this preffed off.

Should there be a bug on enabling this by default?
Comment 54 Ben Bucksch (:BenB) 2011-03-30 06:53:36 PDT
Yes, thanks for pointing this out. I filed bug 646452.
Comment 55 Manish Goregaokar [:manishearth] 2014-02-14 15:39:50 PST
Possible regression: (not yet reproduced) bug 973064

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