113515 - FTP depends on wallet

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Chris Waterson]

FTP currently has a hard-coded dependency on wallet. This should be factored.

Comment 1

[Chris Waterson]

Attachment: use observer service instead of nsIWalletService directly

This patch replaces the direct call to nsIWalletService to a more generic call
to the nsIObserverService. I've updated the nsWalletService to listen on the
``ftp-login-failed'' event, as well.

Comment 2

[Chris Waterson]

(Sorry if there was already another bug filed on this: I couldn't find one.)

Comment 3

[Darin Fisher]

patch looks good to me (sr=darin)... cc'ing bbaetz and dougt too.

Comment 4

[Stephen P. Morse]

+  else if (!nsCRT::strcmp(aTopic, "ftp-login-failed")) {
+    // An FTP login failed; clean out any information we've stored
+    // about the URL where the failure occurred.

There is no need for wallet to single out ftp.  This could be a general login 
failure from any other module as well, for example mailnews.  I suggest changing 
the comment to be "A login failed..." and changing the identifying string from 
"ftp-login-failed" to simply "login-failed".

Comment 5

[Stephen P. Morse]

With above change, r=morse

Comment 6

[Chris Waterson]

Attachment: s/ftp-login-failed/login-failed/

Good idea. I've changed ftp-login-failed to login-failed.

Comment 7

[Stephen P. Morse]

Comment on attachment 60410 [details] [diff] [review]
s/ftp-login-failed/login-failed/

r=morse

Comment 8

[Alec Flett]

Comment on attachment 60410 [details] [diff] [review]
s/ftp-login-failed/login-failed/

awesome!
nice catch

Comment 9

[Bradley Baetz (:bbaetz)]

nice! r=bbaetz

Comment 10

[Chris Waterson]

See also bug 113540 for a foray into mailnews.

Comment 11

[Chris Waterson]

So I thought I'd actually _test_ this before checking it in. It turns out that
this feature doesn't *currently* work, either with or without my patch. The FTP
servers that I've tested are returning a 530 for `login failed', so it never
trips the code to call into the password manager:

  $ ftp ftp.netscape.com
  Connected to ftp.netscape.com.
  220 ftp25c.newaol.com FTP server (SunOS 5.7) ready.
  Name (ftp.netscape.com:waterson): blarch
  331 Password required for blarch.
  Password:
  530 Login incorrect.
  Login failed.

Should I just check it in anyway, and file another bug? Or should I just treat
all 530's as login failed, bad password? (This would regress bug 44324, not sure
which is worse.)

I did a quick test (abusing gcc.gnu.org, which sets a low limit), and there
doesn't look like any easy way to parse the response message to differentiate a
login failure due to password failure from a login failure due to user limit.
Here's the response I got:

  /usr/src/seamonkey/debug/dist/bin$ ftp gcc.gnu.org
  Connected to gcc.gnu.org.
  220 FTP server ready.
  Name (gcc.gnu.org:waterson): anonymous
  331 Guest login ok, send your complete e-mail address as password.
  Password:
  530-Sorry, the ftp server has too many active ftp connections (20/20).
  530-
  530-Most major projects served by this machine have mirror sites you
  530-can use instead of this server.  The following pages have the
  530-official mirror lists for each project.
  530-
  530-Mirror Lists:
  530-
  530-GCC:         http://gcc.gnu.org/mirrors.html
  530-Sourceware:  http://sources.redhat.com/mirrors.html
  530-eCos:        http://sources.redhat.com/ecos/mirror.html
  530-Cygwin:      http://sources.redhat.com/cygwin/mirrors.html
  530 Login incorrect.
  Login failed.

Note the `Login incorrect.', which is also given for an invalid password. So, I
guess we should either remove the FTP password checking code altogether, or
remove the 530 testing. Anyone care to flip a coin? (cc'ing rjc and jud, who
appear to have created this impasse.)

Comment 12

[Judson Valeski]

just checkin what you have. I'm sure doug has a bug filed on the 530 problem;
doug, if not, can you open one? There is a login failed state that simply can't
be captured (ie fails it too, perhaps using the "other" way of failing that we
do, but, none-the-less, you can hose their client too). IIRC, we need both paths
in there, we'll land in both (depends on the server).

Comment 13

[Chris Waterson]

Sheesh, this is awful. It turns out that if I revert my patch, and then #if 0
out the 530 checking, it _still_ doesn't work. FTP appears to correctly call
into the wallet code, but the subsequent retry doesn't work and the FTP
transaction times out. Worse, even after a restart (and wallet should've
forgotten the password), the FTP transaction _still_ times out!

Anyway, I'm not going to wade in and fix this. I've landed my change to decouple
the direct dependency on the wallet service.

Comment 14

[Chris Waterson]

I've filed bug 114118 for this issue. dougt, feel free to dup it or whatever.