403420 - notification bar should show the host for the login it's asking to save

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Jo Hermans]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111104 Minefield/3.0b2pre

I'm running Minefield with the default homepage <http://www.mozilla.org/projects/minefield/>, but I'm behind the proxy-server of my company. So I have to supply a username and password when I startup, but they're saved so I never see the dialogbox.

Today, I was surprised to see a new prompt appearing. It was the correct realm, but a different proxyserver, for which I haven't got a loginname/password saved. Not the first time that this happened, sometimes we get redirects to various backup servers when the main one fails.

After the page loaded, I noticed the save password toolbar appearing, prompting me to save the password. But I still couldn't see which server was responsible for the prompt. But it definitely wasn't the server that gave me the content (which was www.mozilla.org actually). So the prompt looked out of place.

Can't we mention the servername (and maybe the realm) in that toolbar? Or in an info-field ? I know that it was present in the design of bug 226735, but it seems to have been dropped in bug 226735 comment 53. It's not too bad in the common case where the server is the one that served up the data (the name is in the locationbar). But that is *not* the case for a proxyserver.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Comment 3

[Justin Dolske [:Dolske]]

Oh, meant to note: The eTLD/IDN code is lifted from DownloadUtils.jsm.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 337154 [details] [diff] [review]
Patch v.1

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+    _getShortDisplayHost: function (aURIString) {

>+            var uri = this._ioService.newURI(aURIString, null, null);
>+            var baseDomain = eTLDService.getBaseDomain(uri);

getBaseDomain just gets the innermost URI, then gets the asciiHost and passes it to getBaseDomainFromHost. I wonder whether we can avoid the round trip through nsIURI? Do we need some of the other stuff the downloadutils code does? I wonder whether we should just import that and call it directly?

>diff --git a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties

>-savePasswordText = Do you want %S to remember this password?
>+saveLoginText = Do you want %S to remember the password for %S?

/embedding/browser/gtk/src/EmbedPasswordMgr.cpp uses this string - not sure what the status of that code is. Perhaps it should just be removed from mozilla-central if it's not being maintained there?

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #4)
> I wonder whether we can avoid the round trip through nsIURI? 

Not really, unless we hack in code to tunnel in a nsURI from earlier in the save-password codepath (which would be ugly). This isn't perf-critical code, so I think it's best to leave as-is.

> Do we need some of the other stuff the downloadutils code does?

Nope. I only mention it for reference's sake.


> >-savePasswordText = Do you want %S to remember this password?
> >+saveLoginText = Do you want %S to remember the password for %S?
> 
> /embedding/browser/gtk/src/EmbedPasswordMgr.cpp uses this string - not sure
> what the status of that code is. Perhaps it should just be removed from
> mozilla-central if it's not being maintained there?

Ugh. Yeah, afaik it's not maintained and it would seem best to just remove it. I guess I can just file a bug to remove it?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> (In reply to comment #4)
> > I wonder whether we can avoid the round trip through nsIURI? 
> 
> Not really, unless we hack in code to tunnel in a nsURI from earlier in the
> save-password codepath (which would be ugly).

No, I mean the opposite. You have a string, and you're creating a nsIURI to pass in to getBaseDomain, which ends up just getting asciiSpec and calling getBaseDomainFromHost. I'm wondering whether the stuff getBaseDomain is doing is necessary - if it isn't, you could just call getBaseDomainFromHost directly. 

> > Do we need some of the other stuff the downloadutils code does?
> 
> Nope. I only mention it for reference's sake.

What about the port stuff? Even if it does some stuff we don't much care about (e.g. handling file://), it seems to me like it would be best to avoid duplication and just use that module, but I guess it does a bunch of other stuff with strings that would just be overhead. Add a comment mentioning where this was copied from, at least?

> Ugh. Yeah, afaik it's not maintained and it would seem best to just remove it.
> I guess I can just file a bug to remove it?

Yep. CC romaxa@gmail and timeless?

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #6)

> No, I mean the opposite. You have a string, and you're creating a nsIURI to
> pass in to getBaseDomain, which ends up just getting asciiSpec and calling
> getBaseDomainFromHost.

The problem here is that the string we have (login.hostname) is in a format like "scheme://www.host.com:1234", and we need to extract out just the "www.host.com" part. Running it through nsIURI seems like the best way to do that, he only other way I can think of would be to regex it out, but that makes me wary of future problems. It's unfortunate to have to convert between a URI and string, but there's already so much other stuff going on in the process of building up the prompt that this seems inconsequential. 

> What about the port stuff?

I don't think it's worth showing the port. The goal here is to provide a cue to the user as to where the login is being saved for, so stripping the host down to the bare essentials should be fine.

> Even if it does some stuff we don't much care about
> (e.g. handling file://), it seems to me like it would be best to avoid
> duplication and just use that module

Well, we're talking about just 2 lines of code. I was just pointing it out as a review aid, since DM does something similar and it's good to look for pitfalls that way.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> The problem here is that the string we have (login.hostname) is in a format
> like "scheme://www.host.com:1234"

Oh, right - it's using asciiHost, not spec. Makes sense.

> I don't think it's worth showing the port. The goal here is to provide a cue to
> the user as to where the login is being saved for, so stripping the host down
> to the bare essentials should be fine.

Well normally we try to display origins (scheme+host+port) in similar dialogs (e.g. http auth) so that it's possible to differentiate. I agree that it's probably not a big deal in this case, but if it was easy to do and we got it free as part of sharing the code, I wouldn't have complained :)

> Well, we're talking about just 2 lines of code.

The method is more than two lines of code. I just don't like having two functions that do (mostly) the same thing in two different places. It's small and using the downloads module directly has other costs, though, so you're right.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

I thought it might also be a good idea to show the *username* that is being saved, so this patch builds upon the last one by doing that. One complication here is that since the username string is controlled by the site (which might be up to no good), we want to avoid problems like bug 244273. The risk is much lower in this context, though.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 337585 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+    get _ellipsis() {

>+                var prefSvc = Cc["@mozilla.org/preferences-service;1"].
>+                              getService(Ci.nsIPrefService);
>+                this.__ellipsis = prefSvc.getComplexValue("intl.ellipsis",
>+                                      Ci.nsIPrefLocalizedString).data;

Does this work? I would imagine you'd need to QI to nsIPrefBranch to call getComplexValue...

>     _showSaveLoginDialog : function (aLogin) {

Why not show the username in this case as well?

>diff --git a/toolkit/components/passwordmgr/test/test_notifications.html b/toolkit/components/passwordmgr/test/test_notifications.html

> function getNotificationBox(aWindow) {
>-/*
>     var chromeWin = aWindow
>                         .QueryInterface(Ci.nsIInterfaceRequestor)
>                         .getInterface(Ci.nsIWebNavigation)
>                         .QueryInterface(Ci.nsIDocShellTreeItem)
>                         .rootTreeItem
>                         .QueryInterface(Ci.nsIInterfaceRequestor)
>+                        .getInterface(Ci.nsIDOMWindow)
>+                        .QueryInterface(Ci.nsIDOMChromeWindow);

Is this last QI needed?

>+      case 18:

>+        expectedText = /^Do you want .+ to remember the password for \"nowisthetimeforallgoodmentocom\u2026\" on example.org\?$/;

The hardcoded \u2026 means the test will fail for localized builds that modify intl.ellipsis, which probably isn't ideal... can you just get it from the pref here as well? Kind of a pain, but might save someone some trouble later on.

The rest looks OK, but this patch is missing the changes to the .properties files. It would be best to use the %1$S form (rather than just %S) for the multiple-substitution strings, and be sure to include an L10n note (might also mention the truncation/quote replacing).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> The hardcoded \u2026 means the test will fail for localized builds that modify
> intl.ellipsis, which probably isn't ideal... can you just get it from the pref
> here as well?

Actually, just loosening up the regex to have it ignore the character there would probably be fine!

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Updated with review nits.

(In reply to comment #10)
> Does this work? I would imagine you'd need to QI to nsIPrefBranch to call
> getComplexValue...

Oops! Fixed. I thought I just cut'n'pasted this from somewhere, but all similar code already in the tree looks right.

> Is this last QI needed?

N/A in this patch, because this code was moved to a common file. But you seemed to suggest it was, back in bug 422974 comment 10. :)

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Screencap of Patch v.3

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 345601 [details] [diff] [review]
Patch v.3

Factor out the username sanitizing code rather than duplicating it?

>diff --git a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties

>+saveLoginText = Do you want %1$S to remember the password for "%2$S" on %3$S?
>+saveLoginTextNoUsername = Do you want %1$S to remember this password on %2$S?

L10n note needed to explain the parameters and truncation/quote removal here...

(In reply to comment #12)
> But you seemed to suggest it was, back in bug 422974 comment 10. :)

Well, I was operating under the assumption that someone actually relied on being able to access nsIDOMChromeWindow attributes/methods, given the comment in that patch :) I didn't see anyone relying on it in this case.

Comment 15

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

Comment on attachment 345601 [details] [diff] [review]
Patch v.3

uir=lol+

Comment 16

[Justin Dolske [:Dolske]]

Fixed nit from comment 14 and pushed changeset e8b8677097d0.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> Factor out the username sanitizing code rather than duplicating it?

Looks like you missed this? I filed bug 462525.

Comment 18

[Justin Dolske [:Dolske]]

yeah, oops. :/