43886 - Single Signon no longer usable from browser-generated forms

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[esther]

Using build 2000-06-26 on win98, mac and linux the mail password dialog is 
missing the "Use Password Manager to save this password"  check box.  Because of 
this, the user can't save a password if they have migrated a profile.  Also 
Note: the checkbox for saving the password in the Account Setup|Name and 
Password doesn't work with this build and is going to be removed so a New 
Account using the Account Wizard can't save password either.  

1. In 4.7 have a profie that can be migrated
2. Launch netscape.exe -installer to bring up the list with the 4.7 profiles 
that need to be migrated.
3. Select the account for migrating
4. Launch Messenger
5. Click In Box and Get Msg

Result: you get a password dialog for entering your password only.  Doesn't have 
a check box for Saving the password
Expected:  you should get a password dialog for entering your password and a 
check box for Saving the password

Comment 1

[Stephen P. Morse]

Not only has mailnews been severed from single signon, but so has http 
authentication and ftp authentication. :-(

Looks like this is due to Warren's recent change of adding the savePassword 
parameter to nsSingleSignOnPrompt::PromptPassword.  The routine is being called 
with a value of 0 which means SINGSIGN_SAVE_PASSWORD_NEVER instead of 2 
(SINGSIGN_SAVE_PERMANENTLY).

I could probably fix this by redefining the constants in singsign.h but I don't 
think that was what Warren intended when he made this change.  I believe that 
the problem is in the call to the routine.  Prior to Warren's change, the caller 
used to pass in a boolean for this parameter and now it is supposed to be a 
trinary value.  But I think that the callers are still passing in booleans.

For example, in nsNewsFolder.cpp, line 1411, I see that the fourth parameter is 
PR_TRUE.  It should be SINGSIGN_SAVE_PERMANENTLY.

Where else is this called from?  All the caller will need to be updated.  Not 
only the callers to PromptPassword, but the callers to PromptUsernameAndPassword 
and the callers to Prompt.

Changing the summary and nominating for nsbeta2.

BTW, Warren's change was made on June 16.  I'm surprised nobody saw this problem 
before now.

Comment 2

[Warren Harris]

Wow. Sorry if I missed updating some call sites. I thought I had grepped for 
them all. Don't you just hate how C/C++ #defines, enums and integers get all 
jumbled together by the compiler?

Comment 3

[Stephen P. Morse]

Here are the calls to PromptPassword that need to be fixed:

extensions/psm-glue/src/nsPSMUICallbacks.cpp#316
netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#910
mailnews/base/util/nsMsgIncomingServer.cpp#634
mailnews/compose/src/nsSmtpServer.cpp#211
mailnews/local/src/nsPop3Protocol.cpp#2494
mailnews/local/src/nsPop3Protocol.cpp#2494

Here are the calls to PromptUsernameAndPassword that need to be fixed:

netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#826
netwerk/protocol/http/src/nsHTTPChannel.cpp#1835

And it looks like the only relevant call the Prompt from mailnews was already 
updated to use the parameter nsIPrompt::SAVE_PASSWORD_PERMANENTLY.  This is in:

mailnews/news/src/nsNewsFolder.cpp#1476

Comment 4

[lchiang]

laurel - when this is fixed, contact sairuh and let her know to test the other 
browser areas which use this. Thanks.

Comment 5

[jefft]

Hey, Steve thanks much for your efforts. I'll working on the mailnews side right 
away. Appreciated.

Comment 6

[Stephen P. Morse]

Error in above list.  I accidentally duplicated the entry for 
nsPop3Protocol.cpp.  One of those entries should have been the 
following:

mailnews/news/src/nsNewsFolder.cpp#1411
 

Comment 7

[Stephen P. Morse]

Jeff,

Hold on -- I'm working on all the changes right now.  I'm about to put it up for 
review

Comment 8

[jefft]

Okay, Steve. I'll reassign the bug to you. Thanks much,

Comment 9

[Stephen P. Morse]

I'm about to attaching a set of diffs for code review.  Besides the parameter 
changes listed above, I'm also changing the names of the parameter since there 
was lack of consistency between the names defined in singsign.h and in 
nsIPrompt.idl.

I'm a bit confused.  The old binary parameter was sometimes passed in as TRUE 
and sometimes as FALSE.  I can't understand how that ever could have worked.  
The only time that we don't want to have the checkbox is in the case of the call 
from PSM.  Can someone shed some light here?

Here is what the binary values were set to:

FALSE: nsFtpConnectionThread.cpp (2 places)
FALSE: nsHTTPChannel.cpp
TRUE: nsMsgIncomingServer.cpp
TRUE: nsSmtpServer.cpp
FALSE: nsPop3Protocol.cpp
TRUE: nsNewsFolder.cpp

TRUE: nsPSMUICallbacks.cpp

Comment 10

[Stephen P. Morse]

Attachment: Diffs for code review

Comment 11

[jefft]

For mailnews they should be all true. The one in nsPop3Protocol.cpp is #ifdef 
out. The patch looks good to me. 

Comment 12

[Stephen P. Morse]

Fixes checked in.

Comment 13

[Blake Ross]

the resolution on this died.  correcting, sorry for spam everybody.

Comment 14

[Blake Ross]

fixed

Comment 15

[laurel]

Well, the original mail problem is ok using current builds, I can save login
passwords to the password manager. 

I'm going to change this QA assignee to sairuh for the browser side. 

Comment 16

[sairuh (rarely reading bugmail)]

sorry for the delay... in any case, this is working fine (tested using http
authentication), 2000.07.31.04-m17 commercial branch (mac, linux, winnt).