84834 - We need an API that returns the password given the host and/or user name

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P1)

Description

[racham]

Today, a password stored in the password manager is obtained when when we go
through routines like PromptPassword ( & GetPasswordwithUI()), which involve UI
elements to be passed in as input params. This arrangement is not convenient for
some other tasks where password is needed (if exists) without having to go
through UI tied up routines.

For example, in mailnews, we have a feature (bug 80296) where one can get
messages for multiple accounts by clicking 'Get All New Messages' on GetMsg
button dropmarker menupopup. What we do there is to get the server list and
query each server for the password. If the password value exists (say stored in
the password mgr in one of the earlier sessions), then that account is
authenticated. So, we can safely (wihtout having to worrying about password
popup) call getmessages on that server and get messages. Today, we are getting
null values back for some of the servers who passwords are already stored. Hence
the feature doesn't work as expected. So, the routine GetPassword() in
nsMsgIncomingServer has to be smarter than it is right now in returning the
password and we need Password Mgr's help to do that.

All GetPassword() does today is to make a copy of m_password and return the
same.
http://lxr.mozilla.org/seamonkey/source/mailnews/base/util/nsMsgIncomingServer.cpp#661

Today sometimes, Though the password actually exists, the m_password is not set
yet. 

Reason : we haven't made a call to GetPasswordWithUI yet (which will set the
m_password) which then will go contact SignonService and gets the password. 

Is it legal to get the password without having to go thru GetPasswordWithUI..?
Yes. It should be. 

When I query a server (whose password is stored) for it's password and if I get
back a null/empty string..then that's an error condition. So, GetPassword(), if
m_password is empty, should call nsPasswordMgr's API (that I am requesting here)
and return the passwrod if it is there. That's why we need that API. I am sure
some one else will also need such a thing as we go forward.

Another instance in mailnews where we need this feature desparately is the Biff
feature. Today Biff on any second account will fail as
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgBiffManager.cpp#316
simply returns a null password. It works for the first account because user
might have actually clicked on GetMsg button (or some other authentication)
which calls GetPasswordWithUI(). Sheela, if you have bug number for this, please
state it here.

We need an API in nsIPasswordManager that goes something like 
string getPassword(string hostname, wstring username) or
string getPassword(string hostname)

I spoke to Steve Morse about the current abilities of the password manager. He
already has an idea on what I am looking for. 

I talked to mScott about the current scenario and we both agree we may need such
an API. I also talked to Sheel about the existing error condition in Biff.

Adding Seth & Scott Putterman to the cc list.

bhuvan

Comment 1

[racham]

Adding the dependency and adjusted milestone of this bug based on that. 

Comment 2

[racham]

Attachment: patch that I used to test the concept getting the info from password mgr to make things work in mailnews..! It worked. Just attaching for your reference. But it will be better to add an api that returns the password string.

Comment 3

[Viswanath Ramachandran]

Scott (Putterman) - could you add more on why this is an rtm stopper, I could
not follow the description in bug 80296 completely. Steve - how easy is this to
add i.e. time taken? 

Comment 4

[Stephen P. Morse]

It's simple to implement, probably no more than a day or two.  It's a matter of 
adding the following additional attribute to 
extensions/wallet/public/nsIPassword.idl

   readonly attribute wstring password;

and then writing some c++ code in nsPasswordManager.cpp to fetch that attribute.

Comment 5

[scottputterman]

Vishy, mailnews has a couple of bugs where biff and Get Msgs for multiple
accounts should work for all accounts that we know the password for on startup.
 One of the goals for mailnews was to have better multiple account management
and without this the user is going to be forced to access each account before
they can do anything with them.

Bhuvan, in a worse case scenario, could you check your patch in?

Comment 6

[Stephen P. Morse]

Unless I misunderstood the patch, I don't think it's the kind of thing you want 
to check in.  It's not a fix at all but merely a test.

Are you going to be fetching all the mail passwords when mail starts up?  If so, 
the user would then be prompted for his master password if he chose to have his 
data encrypted.  Is that the behavior that you want?

Comment 7

[racham]

Steve,

When user clicks on GetAllNewMessages, I do want to check all those servers
which can get messages for their passwords. Then if I find password for a given
server, it's considered as an authenticated account and hence we will get
messages for that server without putting any password prompt dialogs..

With API you susggested 'readonly attribute wstring password;' which sitename's
password is this going to return ? The API I am looking for is more specifically
looking for the password for a given hostname (and/or username). I would like
PasswordMgr to do the enumeration or any other kind of algorithm to find the
corresponding password. 

If the API you will add is just the attribute, then the caller has to do the
enumeration to extract the password from the password objects..right ? I would
like PasswordMgr to handle that logic. So, besides the API you suggested, it
will be useful if you can also provide us another API which returns password
like the ones I suggested in my previous updates 

* wstring getPassword(string hostname, wstring username)
* wstring getPassword(string hostname)

I will be on aim as bhuvanr, please do ping me or I will ping you if I find you.
thanks.

Scott, 

We can go with workaround. The only issue with that will be, it does these
enumerations everytime to find out if the password for a given server exists and
that may cost us something. That's why if we have the password string back, then
we can simply make GetPassword() to learn about it (by filling m_password) and
make it not ask the password again.

bhuvan

Comment 8

[Stephen P. Morse]

I disagree.  The enumeration interface is currently the way the password manager 
makes it's data available.  So if a caller wants the password for a particular 
url, it should be the caller's responsibility to go through the enumeration 
until finding the desired one.

Comment 9

[racham]

OK Steve. That's fine. Please go ahead with readonly attribute wstring password;
in nsIPassword.idl and corresponding cpp code as you suggested.

I will attach a new patch based on that API in the dependent bugs. Once you
check in, I will do testing for correctness and then request reviews.

thanks
bhuvan

Comment 10

[scottputterman]

To answer Steve's question, for the case of biff, we don't want the master
password dialog coming up. For Get All Messages I think that would be ok.

Comment 11

[Stephen P. Morse]

> To answer Steve's question, for the case of biff, we don't want the master
> password dialog coming up.

You won't be able to stop it.  If your data is encrypted, PSM will automatically 
pop up that dialog and I have no control over that.  So tell me if you still 
want me to implement this interface change.

Comment 12

[scottputterman]

Jennifer what's your feeling on this?  Would you rather see a master password
come up when doing biff on startup or not have biff work for accounts that
remember their password.

Steve is there any way to make an api that allows us to specify that we want the
password only if it's unencrypted?

Comment 13

[Stephen P. Morse]

> Steve is there any way to make an api that allows us to specify that we
> want the password only if it's unencrypted?

Not cleanly.

Comment 14

[Stephen P. Morse]

Attachment: adding pswd to the enumeration in the API

Comment 15

[Stephen P. Morse]

racham, please review the patch, alecf please superreview it.  Thanks.

Comment 16

[racham]

Steve, 

What is the master password and how/when is it set by the user ? Will the user  
be asked for master password when the password to be featched is an encrypted 
one ? Howmuch effort is involved if we have to only fetch unencrypted passwords 
?

Though I only need info to the extent of knowing whether or not password is 
avialable to solve my bugs, it is not optimal at all. Because every time I do 
GetAllNewMessages OR when biff activity is triggered, I have to enumearate thru 
all password elements to find if the password entry is then there..that's 
expensive.

bhuvan

Comment 17

[racham]

r=racham.

Comment 18

[racham]

I will also test it with my patch in mailnews and post the test results.

Comment 19

[Stephen P. Morse]

> What is the master password and how/when is it set by the user ?

When the user decides he wants to encrypt his data, he needs to select an 
encryption key.  That's the "master password."  He "sets" it at the time that he 
asks to have his data encrypted.  Furthermore, he needs to supply it in each 
browser session at the first point in time that the database is to be accessed.

> Will the user be asked for master password when the password to be featched
> is an encrypted one ?

Yes, if he has not already supplied it for the browser session.

> Howmuch effort is involved if we have to only fetch unencrypted passwords ?

There's no clean way for you to specify that in the API

> every time I do GetAllNewMessages OR when biff activity is triggered,
> I have to enumearate thru all password elements to find if the password
> entry is then there..that's expensive.

If you are suggesting as an alternative that password manager do the iteration 
for you, that's just as expensive but on the other side of the interface.  So 
nothing would be saved.

Comment 20

[Alec Flett]

it seems unnecessary (and unfair to developers whose first language is not
english and might not understand such abbreviations) to call this "pswd" when it
could just be called "password"

sr=alecf if you rename that attribute to "password"

Comment 21

[Stephen P. Morse]

> it seems unnecessary (and unfair to developers whose first language is not
> english and might not understand such abbreviations) to call this "pswd"
> when it could just be called "password"

But there is already a "password" object being used in this coding

Comment 22

[Stephen P. Morse]

Ignore above comment -- I hit submit button by accident when I meant to hit 
clear.

Comment 23

[Stephen P. Morse]

Attachment: changing pswd to password

Comment 24

[Alec Flett]

(my sr=alecf still stands.. thanks for the change!)

Comment 25

[racham]

Works fine when tested with mailnews patches in the dependent bugs.

Steve, thanks for adding the interface as needed so quickly. I have few more
questions regd the master password and the ways we can avoid the dialog..Will
send you mail regd that.

thanks,
bhuvan.

Comment 26

[Brendan Eich [:brendan]]

The hand-off of memory allocated by SINGSIGN_Enumerate to its caller follows
XPCOM rules, but then that memory is handed off to new nsPassword(...) in a
non-XPCOM fashion, and there's a leak if operator new fails:

-      nsIPassword *password = new nsPassword(host, user);
+      nsIPassword *password = new nsPassword(host, user, pswd);
       if (password == nsnull) {
         return NS_ERROR_OUT_OF_MEMORY;
       }

All of host, user and pswd are leaked should NS_ERROR_OUT_OF_MEMORY bite here. 
Why not use nsXPIDLStrings and have new nsPassword copy?  Or to minimize change
and copying overhead, at least document the non-XPCOM handoff here and spot-fix
the leak with some nsMemory::Free calls in the error return case?

/be

Comment 27

[Stephen P. Morse]

Attachment: revised patch for nsPasswordManger.cpp to address brendan's concerns

Comment 28

[jglick]

>Jennifer what's your feeling on this?  Would you rather see a master password
>come up when doing biff on startup or not have biff work for accounts that
>remember their password.

If I understand correctly, if a user is using the Master Password feature to 
store their Mail passwords, when Mail launches, and a password is needed to 
retrieve messages, if the user has not already supplied their Master Password 
for another component, they should be asked for their Master Password now, at 
startup of Mail.

Comment 29

[Christopher Blizzard (:blizzard)]

a=blizzard for drivers for the trunk

Comment 30

[Stephen P. Morse]

Fix checked in.

Comment 31

[Terri Preston]

Verified fix checked into lxr.mozilla.org