Closed Bug 84834 Opened 23 years ago Closed 23 years ago

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

Categories

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

x86
Windows NT

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: racham, Assigned: morse)

References

Details

(Whiteboard: [pdt+])

Attachments

(4 files)

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
Adding the dependency and adjusted milestone of this bug based on that.
Blocks: 80296
Severity: normal → major
Target Milestone: --- → mozilla0.9.2
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?
Blocks: 85227
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.
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?
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?
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
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.
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
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.
> 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.
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?
> 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.
racham, please review the patch, alecf please superreview it. Thanks.
Status: NEW → ASSIGNED
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
r=racham.
I will also test it with my patch in mailnews and post the test results.
> 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.
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"
> 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
Ignore above comment -- I hit submit button by accident when I meant to hit clear.
Attached patch changing pswd to password (deleted) — Splinter Review
Blocks: 83989
(my sr=alecf still stands.. thanks for the change!)
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.
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
Keywords: nsbeta1+
Priority: -- → P1
Whiteboard: [pdt+]
>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.
a=blizzard for drivers for the trunk
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Verified fix checked into lxr.mozilla.org
Status: RESOLVED → VERIFIED
No longer blocks: 80296
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: