Automatically log in to proxy

VERIFIED FIXED in mozilla1.9.3a5

Status

()

Core
Networking: HTTP
P2
enhancement
VERIFIED FIXED
8 years ago
4 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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.
Assignee: nobody → ben.bucksch
(Assignee)

Updated

8 years ago
Component: Password Manager → Networking: HTTP
Product: Toolkit → Core
QA Contact: password.manager → networking.http
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 223636
(Assignee)

Comment 3

8 years ago
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 :).
Attachment #405551 - Attachment is obsolete: true
Attachment #405568 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

8 years ago
Thanks, Justin, for pointing out the other bug with another patch.

I'll REOPEN this, to get a review for this patch.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Adding the const doesn't affect compatibility, so there's no reason not to do it, AFAIK.
(Assignee)

Updated

8 years ago
Attachment #405568 - Flags: review?(bzbarsky) → review?(cbiesinger)
(Assignee)

Comment 6

8 years ago
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).
Status: REOPENED → ASSIGNED
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.
Attachment #405568 - Flags: review?(cbiesinger) → review+
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.
A unit test would also be nice :)
(Assignee)

Comment 10

8 years ago
biesi: Thanks for the catch! Will fix that.
(Assignee)

Updated

8 years ago
Attachment #405568 - Flags: review?(dolske)
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...
(Assignee)

Comment 12

8 years ago
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

8 years ago
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!
(Assignee)

Comment 14

8 years ago
No, you can't. This is for developers only. Please wait until this is integrated in Firefox.
(Assignee)

Updated

8 years ago
Attachment #405568 - Flags: review?(dolske) → review?(bzbarsky)
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.
(Assignee)

Comment 16

8 years ago
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.
Attachment #405568 - Flags: review?(bzbarsky) → review?(gavin.sharp)
Attachment #405568 - Flags: review?(gavin.sharp) → review?(dolske)
(Assignee)

Comment 17

8 years ago
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?
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 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
Attachment #405568 - Flags: review+ → review-
(Assignee)

Comment 20

8 years ago
I wanted to wait for the other review before I touch and test the code again, but I'll adapt the patch tomorrow.
(Assignee)

Updated

8 years ago
Attachment #405568 - Flags: review?(dolske)
(Assignee)

Comment 21

8 years ago
Created attachment 425548 [details] [diff] [review]
Fix, v3
Attachment #405568 - Attachment is obsolete: true
Attachment #425548 - Flags: review?(cbiesinger)
(Assignee)

Comment 22

8 years ago
Created attachment 425577 [details] [diff] [review]
Fix, v4
Attachment #425548 - Attachment is obsolete: true
Attachment #425577 - Flags: review?(cbiesinger)
Attachment #425548 - Flags: review?(cbiesinger)
(Assignee)

Comment 23

8 years ago
Created attachment 425621 [details] [diff] [review]
Fix, v5

With unit test
Attachment #425577 - Attachment is obsolete: true
Attachment #425621 - Flags: review?(cbiesinger)
Attachment #425577 - Flags: review?(cbiesinger)
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
Attachment #425621 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 25

8 years ago
> 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?
(Assignee)

Comment 26

8 years ago
> 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.
(Assignee)

Comment 27

8 years ago
Created attachment 425793 [details] [diff] [review]
Fix, v6
Attachment #425793 - Flags: review?(cbiesinger)
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.
Attachment #425793 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 29

8 years ago
Created attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)

Removed the RFC-quote, leaving only the reference
Attachment #425621 - Attachment is obsolete: true
Attachment #425793 - Attachment is obsolete: true
Attachment #425797 - Flags: review?(cbiesinger)
(Assignee)

Updated

8 years ago
Attachment #425797 - Flags: superreview?(bzbarsky)
Attachment #425797 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

8 years ago
Attachment #425797 - Flags: review?(dolske)
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.
(Assignee)

Comment 31

8 years ago
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.
Attachment #425797 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 425797 [details] [diff] [review]
Fix, v7 - netwerk part checked in (comment 33)

API changes look fine.
(Assignee)

Comment 33

8 years ago
Commited netwerk/ part
<http://hg.mozilla.org/mozilla-central/rev/ba358a050536>
(Assignee)

Comment 34

8 years ago
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.
(Assignee)

Comment 35

8 years ago
dolske: ping
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!
Attachment #425797 - Flags: review-
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.
Attachment #425797 - Flags: review?(dolske) → review-
(Assignee)

Comment 38

8 years ago
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.
(Assignee)

Comment 39

8 years ago
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!
(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.
(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.
(Assignee)

Updated

8 years ago
Attachment #425797 - Attachment description: Fix, v7 → Fix, v7 - netwerk part checked in
(Assignee)

Updated

8 years ago
Attachment #425797 - Attachment description: Fix, v7 - netwerk part checked in → Fix, v7 - netwerk part checked in (comment 33)
(Assignee)

Comment 42

8 years ago
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.
Attachment #432817 - Flags: review?(dolske)
(Assignee)

Comment 43

8 years ago
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.
(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.
(Assignee)

Updated

8 years ago
Priority: -- → P2
Attachment #432817 - Flags: review?(dolske)
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.
Attachment #432817 - Attachment is obsolete: true
Attachment #437211 - Flags: review?(paul)
(Assignee)

Comment 46

8 years ago
> Also added tests.

Thank you! I was really stranded there.
Attachment #437211 - Flags: review?(paul) → review-
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(...))
(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.
Created attachment 437417 [details] [diff] [review]
Fix, v10 - Firefox part only
Attachment #437211 - Attachment is obsolete: true
Attachment #437417 - Flags: review?(paul)
Attachment #437417 - Flags: review?(paul) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/8c6006adbb00
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 51

8 years ago
Thanks!
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
Status: RESOLVED → VERIFIED
(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?
(Assignee)

Comment 54

7 years ago
Yes, thanks for pointing this out. I filed bug 646452.
Blocks: 910670
See Also: → bug 973064
Possible regression: (not yet reproduced) bug 973064
You need to log in before you can comment on or make changes to this bug.