114118 - password manager does not automatically forget incorrect passwords

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P2)

Description

[Chris Waterson]

This is a follow-on bug to bug 113515. The problem is that rjc added code to
check for a 530 trying to fix bug 44325. The problem is that an FTP server will
return a 530 for _any_ login failure.

Comment 1

[Chris Waterson]

Note also that even _if_ we remove the 530 checking, there is still a problem
with the FTP protocol (or perhaps with wallet) where the connection just times
out. Anyway, feel free to mark this bug as INVALID. If you decide to do that,
then please remove the password manager code from nsFTPConnectionThread.cpp.

Comment 2

[Robert John Churchill]

In Chris' first comment, make that bug # 44324 instead of bug # 44325, BTW.  :)

Comment 3

[Bradley Baetz (:bbaetz)]

I'm not sure what the correct behaviour is from a UI point of view. The current
behvaiour is obviously incorrect, though.

Given that we cannot differentiate between login failure types, I think the best
thing to do is to display the error, and then somehow tell the password manager
to  reprompt if we try again. It shouldn't lose the stored password, but it
shouldn't return it automatically, either. Is this possible?

I will not get to this before I return at the end of Jan. Feel free to take this
bug from me.

Comment 4

[Bradley Baetz (:bbaetz)]

Lets try for 0.9.9

Comment 5

[Charles Manske]

Just a note: In Composer's new publishing code, we remove the password from
the database if the user unchecks the "Use password manager to save password"
checkbox. Of course that's only if our nsIWebProgressListener callbacks are called.
So this bug won't affect the publishing feature.

Comment 6

[Stephen P. Morse]

Charlie, what about if the user enters an incorrect password when publishing?  
Do you detect that and remove the password from the database then too?

It's the responsibility of each application (http auth, ftp auth, publishing, 
etc) to explicitly call on password manager to remove incorrect paswords.  
Password manager cannot detect if the password was invalid.

Comment 7

[Bradley Baetz (:bbaetz)]

Attachment: patch

OK, try this. The problem was that the string we were asking for was different
to the string we were giving to delete the stored password.

This patch causes us to always use the PrePath. However, the observer callback
takes a url and then gets its spec, which has a trailing /, so we need to add
that to the initial prePath, which is passed in as a string (Why doesn't the
observer callback take a string, btw?)

Our behaviour for when login is denied is now the same as ns4, see bug 124561
for changing that.

Comment 8

[Charles Manske]

Stephen: Composer publishing totaly manages the username and password -- we 
store the username with other publishing data, and call to the PasswordManager
for that. If we get the appropriate nsIWebProgressListener prompt callbacks, 
we will take care of fixing any username and password errors. Unfortunately, bugs
like bug 124561 stand in the way of that working right now :(

Comment 9

[Darin Fisher]

what about using nsIPasswordManager instead of this observer notification stuff?
that's what HTTP does.  nsIPasswordManager.idl is part of necko.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: v2

So we now have two ways to cancel this then... Bleh.

Comment 11

[Darin Fisher]

Comment on attachment 70662 [details] [diff] [review]
v2

sr=darin

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 70662 [details] [diff] [review]
v2

looks okay.

Comment 13

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 14

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 15

[benc]

Mozilla, 1.1 Mac OS X
VERIFIED: someone tell me if I got this wrong...

STEPS:
1- go to FTP URL w/ user w/o password. Enter incorrect password, check password
manager. 

NOTE: failure to login.

2- go to same URL again.

NOTE: prompt re-appears.

3- enter correct password, check password manager.

4- Quit browser, re-access URL, login is automatic.

If composer is depending on this, please have the composer QA file a separate
bug, mark this as depends, then verify it there.

If I did this wrong, please reopen.

Comment 16

[benc]

checked remaining plats.