94775 - Chosing "remember password" and entering a wrong password doesn't bring the password dlg for mail again

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P2)

Description

[marina]

*** observed with 2001-08-09 build ( present in 0.9.2 as well) ****
Steps to reproduce:
- setup the Web mail account through the account wizard;
- chose the web mail account, select the Inbox;
- when prompted to enter your password enter a wrong one and check the box for
the pasword being remembered;
//note: you get 4 "Login failed" notifications and then the connection to the
server is lost, the chance to re-enter a correct pasword never comes up ( it
doesn't happen with other accounts, the password dlgbox will come up for you to
enter a correct password)

Comment 1

[Ninoschka Baca]

Appears to be a Webmail/AOL specific problem in conjunction with Password
Manager. Workaround: I had to select Tasks|Privacy & Security|Password
Manager|Clear Sensitive Information, exit/restart, then the login dialog
appeared again.

Setting QA Contact to huang.

Comment 2

[scottputterman]

reassigning to Cavin.  Webmail/AOL bugs should be bugscape bugs.

Comment 3

[marina]

Cavin, what are we going to do with this bug? i filed bug # 10147 in bugscape
for this problem.

Comment 4

[extremely]

This has started happening for me in the past few days. It appeared about the
same time as bug#101575 which makes me think they may be related. I have avoided
clearing all my security info in hopes of finding a culprit. Since both bugs
come and go (I change that password every month and use about every third daily
build...) I wonder if it isn't something out of sync in the .js or reg info.

Comment 5

[Michal 'hramrach' Suchanek]

I had to delete the remembered password that was no longer valid from security
manager to be asked for new one. But by now I do not get any passowrds for mail
remembered. 

Comment 6

[Cavin Song]

Adding keyword 'nsbeta1+' since the corresponding bugscape bug 10147 has it.

Comment 7

[Cavin Song]

Related info from corresponding bugscape bug:

------ Additional Comment #4 From Cavin Song 2002-03-28 16:59 -------

Other than the problem described in the bug the following is also fixed:

No password dialog shows up if you enter a wrong password and hit OK the 1st 
time. You get an error dialog ("Login failed") and that's it, plus the busy 
cursor is never turned off.

A patch is coming which involves both mozilla and ns changes.



------- Additional Comment #5 From Cavin Song 2002-03-28 17:00 -------

Created an attachment (id=4907)
Proposed mozilla patch, v1

Patch on the mozilla tree.



------- Additional Comment #6 From Cavin Song 2002-03-28 17:02 -------

Created an attachment (id=4908)
Proposed ns patch, v1

Patch on the ns tree.



------- Additional Comment #7 From Cavin Song 2002-03-28 17:10 -------

The patch to the mozilla mainly, in case of login error, resets the url state so 
that next login will be successful.

The patch ns tree mainly resets the internally stored user id and redirector 
type since the login was not successful.



------- Additional Comment #8 From [e-mail] 2002-03-29 08:17 
-------

re the mozilla patch, two things:

1. The name of the variable Reset... should be reset..., lower case r.
2. Could you move the code the gets queue element 0 and QI's it to an nsImapUrl
to one place, instead of having (with your change) 3 places where we do that?
This would involve probably declaring the local imap url comptr at the top level
of the function, but only setting it if urlqueue count > 0, then checking if it
was non null in the other two places where we're pulling it out of the url 
queue.

the ns patch looks good, r=bienvenu on that one.



------- Additional Comment #9 From Cavin Song 2002-03-29 10:14 -------

Created an attachment (id=4918)
Incorporated review comments.



------- Additional Comment #10 From [e-mail] 2002-03-29 10:19 
-------

r=bienvenu for the third patch.



------- Additional Comment #11 From Scott MacGregor 2002-03-29 13:14 -------

(From update of attachment 4918 [details] [diff] [review])
sr=mscott

Comment 8

[Cavin Song]

Attachment: Fix the problem.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 76801 [details] [diff] [review]
Fix the problem.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 10

[Cavin Song]

Fix checked in.

Comment 11

[Karen Huang]

Reopening this bug and updating the summary to general IMAP accounts since I can 
reproduce this problem for judge/nsmail-1 accounts too. 
This is not specific for Webmail -- Removing Webmail on the summary.
Build 05-20-08-1.0.0

Comment 12

[Michael Buckland]

Discussed at mail news bug meeting.  Decided to [ADT2 RTM]this bug.

Comment 13

[Cavin Song]

Two problems with the (original) bug:

1. We need to create wallet service for "login-failed" notification so that when
login fails the cached password can be removed.

This problem has been fixed by bug 121926 so it's no longer a problem.

2. Even if (1) is implemented we still can't remove/reset the cached password 
because the stored 'passwordRealm', basically an url like 
"imap://[e-mail]", does not match the one that is used to 
locate/find the cached password when login fails. So, when users first login and 
check the "Remember password" checkbox we store something like:
   
     "imap://[e-mail]"

in the cache. This is done by the GetServerURI() call in 
nsMsgIncomingServer::GetPasswordWithUI() before calling PromptPassword().

Then when login fails (due to wrong password) we try to locate the cached 
password using the following url:

     "imap://[e-mail]/"

Note that this time we append a '/' to the end of the url, and because these two 
strings don't match we can't find the cached password and thus can't clean it 
up. This is done by the GetSpec() call in nsWalletlibService::Observe() before 
calling SI_RemoveUser().

   The easy way to fix this is to append server delimiter to the end of an url 
in nsMsgIncomingServer::GetPasswordWithUI() before passing it to 
PromptPassword().

Comment 14

[David :Bienvenu]

Cavin, does that mean that the site will show up as "imap://user@host/" ? It
would be nicer if it showed up as imap://user@host w/o the trailing slash -
would it be possible to remove the trailing slash, if any? 

Comment 15

[Cavin Song]

Ccing morse for this issue. Can you take a look at comment #13 and see if you
have a better idea of fixing it? Thanks. The bottom line is that we call
SetSpec() with one string and GetSpec() returns a different one (with trailing '/').

Comment 16

[Stephen P. Morse]

If I'm understanding this correctly, the fix proposed in comment 13 will start 
adding new logins for the realm having the slash whereas there will be legacy 
logins already saved without the slash.  So does that mean that when the user 
displays his various webmail logins (using the password-manager dialog) he'll 
see some with the slash and some without it?  Also will the old legacy logins no 
longer be prefilled for the user?

Another fix would be to modify password manager so that its test for a match 
should ignore trailing slashes on the realm.

Comment 17

[Cavin Song]

Yes, we can also fix it in the password mgr (ie, in si_RemoveUser()) to ignore
the trailing slash when comparing realms. But I'm not sure if this will break
other things since I don't know all the cases where this function will be
called. So you think this is safe to do?

Comment 18

[Stephen P. Morse]

Yes, it should be safe.  The trailing slash should never make a difference, so 
it may be that other clients of password-manager were having the same problem 
but didn't know it yet.

Comment 19

[Cavin Song]

OK, I'll make the change and you can review it. Thanks.

Comment 20

[Cavin Song]

Attachment: Fix original problem for the bug, v1

Strip off trailing ''/ of input realm, if present, before making comparison in
si_GetURL().

Comment 21

[Cavin Song]

morse, can you review the patch? Thx.

Comment 22

[Stephen P. Morse]

patch looks good to me.  Just some really minor nits:

  put "if (urlCount) {" all on one line to be consistent with my style
  throughout rest of file

  looks like your patch inadvertently introduced an extra indentation on
  the line starting with "if(url->passwordRealm..."

r=morse with or without those fixed.

Comment 23

[David :Bienvenu]

Comment on attachment 84839 [details] [diff] [review]
Fix original problem for the bug, v1

sr=bienvenu

Comment 24

[Cavin Song]

Fix checked in.

Comment 25

[timeless]

this appears to have caused a nice string related crash, reviewers need to be 
aware of the behavior of ::Last. it will shoot you for calling it on an empty 
string. morse believes this caused bug 147022 and whichever other duplicate 
stacks we get today/tomorrow.

Comment 26

[scottputterman]

Cavin, how does this differ from 121926 and was the string crash that timeless
mentions caused by this?

Comment 27

[Cavin Song]

To fix this problem we need the patch from bug 121926 (which was fixed prior to
my work on this), plus the patch I submitted in this bug (to cleanup the cached
password since users want passwords to be remembered). See comment #13 for details.

Bug 147022 is caused by the patch of this bug because we did not make sure the
'password realm' is not null or empty before making a call to string's Last()
routine.

Comment 28

[scottputterman]

Karen, can you verify this on the trunk?  Cavin, will you check in your fix to
147022 as part of this checkin or should we nominate that as well?  

Comment 29

[Cavin Song]

Let's nominate 147022 as well. It's easier to track the problem that way.

Comment 30

[Cavin Song]

Hmm, I must have used mozilla to test aol mail accounts and it actually worked 
as I expected.

A patch is coming to fix the webmail/aol login problem. The previous patch fixes 
the generic imap server login problem.

Comment 31

[Cavin Song]

Attachment: Fix webmail/aol login problem, v1

When redirect login fails, invoke ForgetPassword(), instead
SetPassword(nsnull), to clean up all cached passwords.

Comment 32

[Karen Huang]

Yes. When I verified on yesterday trunk build. The original fix did work on 
Netscape Messaging Server(dredd), but did not work for AOL and Webmail.
Hope above fix will fix it all.......

Comment 33

[David :Bienvenu]

Comment on attachment 85882 [details] [diff] [review]
Fix webmail/aol login problem, v1

sr=bienvenu

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 85882 [details] [diff] [review]
Fix webmail/aol login problem, v1

r=[e-mail]

Comment 35

[Cavin Song]

Fix checked into the trunk.

Comment 36

[Karen Huang]

Verified on 06-03-08-trunk build:
Netscape Messaging Server: dredd/judge -> Passed
AOL IMAP: Passed
Netscape Webmail: Passed
Password dialog did prompt again after type in and remember this wrong password.

Comment 37

[Karen Huang]

Marking as verified.

Comment 38

[esther]

*** Bug 149156 has been marked as a duplicate of this bug. ***

Comment 39

[scottputterman]

adding adt1.0.1+ for checkin to the branch along with 147022 and 148502.  Please
get drivers approval before checking in.

Comment 40

[Cavin Song]

Since this bug along with 147022 and 148520 are related I'm going to post a 
patch which shows all the changes that will be checked into the branch. This way 
we can all see what has been changed to fix all 3 bugs.

Comment 41

[Cavin Song]

Attachment: Merged patch from fixes for bugs 94775, 147022, 148520

This is what'll be cheked into the branch once it's approved.

Comment 42

[Judson Valeski]

please checkin to the 1.0.1 branch. once there remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 86709 [details] [diff] [review]
Merged patch from fixes for bugs 94775, 147022, 148520

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 44

[Cavin Song]

Fix landed in the branch.

Comment 45

[Karen Huang]

Verified on branch build for all the platforms:
Windows2000 06-10-08-1.0.0
Linux 06-10-07-1.0.0
Mac 06-10-05-1.0.0

Netscape Messaging Server: dredd/judge -> Passed
AOL IMAP: Passed
Netscape Webmail: Passed
Password dialog did display again after type in and remember this wrong 
password.
Changing keywords from fixed1.0.1 to verified1.0.1.

Comment 46

[Asa Dotzler [:asa]]

*** Bug 151559 has been marked as a duplicate of this bug. ***